Version 2.0.2 breaks some fields

[chadj2]

I am seeing that GltfModelV2.java was removed in 2.0.2 and this is causing problems for me.

Below we can see that GltfModelV2.java was replaced with GltfModelCreatorV2.java.

$ git --no-pager diff --name-status jgltf-parent-2.0.1 jgltf-parent-2.0.2 -- jgltf-model/src/main/java/de/javagl/jgltf/model/v2
D	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/CamerasV2.java
A	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/GltfCreatorV2.java
R063	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/GltfModelV2.java	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/GltfModelCreatorV2.java
D	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/MaterialModelHandler.java
A	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/MaterialModelV2.java
D	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/MaterialStructure.java
A	jgltf-model/src/main/java/de/javagl/jgltf/model/v2/gl/package-info.java

A problem with this new workflow is that the when we create a DefaultGltfModel from the asset some fields like extensionsUsed asset.extras, and asset.copyright are not retained. When we call GltfModelWriterV2.writeEmbedded() with DefaultGltfModel there is nowhere to pass these fields.

I am currently working around this with my own version of writeEmbedded() that sets these manually.

    private void writeEmbedded(DefaultGltfModel gltfModel, OutputStream _os) throws IOException {
        GltfAssetV2 embeddedAsset = GltfAssetsV2.createEmbedded(gltfModel);
        de.javagl.jgltf.model.io.GltfWriter gltfWriter = new de.javagl.jgltf.model.io.GltfWriter();
        GlTF embeddedGltf = embeddedAsset.getGltf();
        
        // workaround to copy extensions and asset from old gltf object.
        embeddedGltf.setExtensionsUsed(this._gltf.getExtensionsUsed());
        embeddedGltf.setAsset(this._gltf.getAsset());
        
        gltfWriter.write(embeddedGltf, _os);
    }

In this new workflow an asset is created, contents are copied (imperfectly) to DefaultGltfModel and its contents are copied back to a new embedded asset. Why not use the original asset like before?

[javagl]

I have to apologize for this sort of incompatibility. I'm usually trying to be careful in that regard, and think that stability of the API is very important, and I'd really like to apply semantic versioning sensibly. The statement at the top of the README.md saying "Note: These libraries are still subject to change." only is a lame blanket excuse for breaking changes. I could come up with somewhat more specific excuses, but I guess that wouldn't be helpful (because they are low-level technical justifications), or not constructive (because they revolve around the fact that this whole library is a one-man, spare-time project)

---

More to the point:

I'm aware that the information about the extensionsUsed and extensionsRequired cannot sensibly be transported through the DefaultGltfModel right now. In fact, I recently worked on some extensions, and had to use the same (hacky) workaround that you described:

        DefaultGltfModel gltfModel = creator.createGltfModel();
        GltfAssetV2 gltfAsset GltfAssetsV2.createEmbedded(gltfModel);
        if (someHack)
        {
            gltfAsset.getGltf().addExtensionsUsed("...");
        }
        GltfAssetWriter writer = new GltfAssetWriter();
        writer.write(gltfAsset, new File(fullFileName));

So this is already on my "TODO" list.

(Improving extension support in general is an ongoing task. I did some smaller steps, for example, in commit https://github.com/javagl/JglTF/commit/a239813b93c2caa1dc86f06cc35ccb9f7a609431 , but there are more things to do.

The asset is a separate topic in some way. Right now, the asset object is pretty much hard-coded at https://github.com/javagl/JglTF/blob/523fcf0121b8b0aa03c404d9149cef8d95b67c5e/jgltf-model/src/main/java/de/javagl/jgltf/model/v2/GltfCreatorV2.java#L316 . But it probably makes sense to allow "transporting" some information there as well.

---

I think that supporting the required functionality should not be tooo hard.

For the extensions, that could just follow the pattern of the existing elements:

In GltfModel:

    List<String> getExtensionsUsed();
    List<String> getExtensionsRequired();

In DefaultGltfModel

    void addExtensionUsed(String extension);
    void removeExtensionUsed(String extension);
    void addExtensionsUsed(Collection<String> extensions);
    void clearExtensionUsed();
    // + the same for `Required`

Internally, these would take care of uniqueness of the elements, and, for example, make sure that each Required extension automatically appears as a Used one.

One thing that I'd consider is whether this could/should be wrapped into something like an ExtensionsModel:

interface ExtensionsModel {
    List<String> getExtensionsUsed();
    List<String> getExtensionsRequired();
}
class DefaultExtensionsModel implements ExtensionsModel {
   // Same functions as in `DefaultGltfModel` above
}

and then only have the

    ExtensionsModel getExtensionsModel();

function in the GltfModel and its implementation. This would only serve the purpose of "wrapping that part into one thing". Code paths that only need the ExtensionsModel could than more isolatedly operate on that, and wouldn't need the whole GltfModel, so I'd slightly lean towards that solution...

---

For the asset, some (minor) restructuring would be required. There'd probably be an AssetModel with the standard GltfModel#getAssetModel and DefaultGltfModel#getAssetModel methods. By default, this model would be populated with the version and generator, and extend AbstractModelElement accordingly to handle the extras and extensions.

---

And thoughts on that?

[chadj2]

One thing that I'd consider is whether this could/should be wrapped into something like an ExtensionsModel:

I like this idea and I think the gltf spec should have really managed extensions under a common tag. I have a use case that requires the use of glTF extensions to meet performance requirements and the implementation has been painful because of the lack of support.

For the asset, some (minor) restructuring would be required.

The reason I need to modify the asset is because I use the asset.extras to provide metadata to an application so it can properly render the file. For my application this was a breaking change.

One thing I want to point out is the common functionality between the attributes tag in the node and the mesh.

"nodes" : [ {
    "extensions" : {
      "EXT_mesh_gpu_instancing" : {
        "attributes" : {
          "TRANSLATION" : 3,
          "ROTATION" : 4,
          "SCALE" : 5,
          "_FEATURE_ID_0" : 6
        }
      },

 "meshes" : [ {
    "name" : "sphere(2)[0]-mesh",
    "primitives" : [ {
      "attributes" : {
        "POSITION" : 0,
        "COLOR_0" : 1,
        "NORMAL" : 3
      },
      "indices" : 2,
      "material" : 0,
      "mode" : 4
    } ]

I had to write 2 almost identical methods because they share no common interface.

    protected final Accessor buildAttrib(MeshGltfWriter _geoWriter, MeshPrimitive _meshPirimitive, String _attribute) {
        Accessor _accessor = buildBuffer(_geoWriter);
        if(_accessor == null) {
            return null;
        }
        
        int _accessorIdx = _geoWriter.getGltf().getAccessors().indexOf(_accessor);
        _meshPirimitive.addAttributes(_attribute, _accessorIdx);
        return _accessor;
    }

    protected final Accessor buildAttrib(MeshGltfWriter _geoWriter, GlTFMeshGpuInstancing _meshInstancing,
            String _attribute) {
        Accessor _accessor = buildBuffer(_geoWriter);
        if(_accessor == null) {
            return null;
        }
        
        int _accessorIdx = _geoWriter.getGltf().getAccessors().indexOf(_accessor);
        _meshInstancing.addAttributes(_attribute, _accessorIdx);
        return _accessor;
    }

I think that MeshPrimitive, GlTFMeshGpuInstancing,and any other property with attributes should share a common interface. That will allow for consolidation of code that generates attributes.

[javagl]

I'll try to address the issue of the extensions (probably with the ExtensionsModel approach) and the AssetModel in the next few days.

---

The point about GlTFMeshGpuInstancing: The support for this extension is in a very early stage (and not part of the official release), so I cannot add enough disclaimers here.

In general, I like the idea of avoiding redundancies and finding common interfaces. But classes like MeshPrimitive are auto-generated from the schema. So they will always and only be the "bare" representation of the JSON structure.

Iff I introduced an interface, then this would be an interface that is implemented by MeshPrimitiveModel (and maybe by a future GlTFMeshGpuInstancingModel, once this exists).

The code that you posted does not expose a degree of redundancy where I'd consider this. The only "common treatment" there is the addAttributes call. And this is just a Consumer<String, Integer>, for what it's worth. There might be other places where the commonality of "both objects having 'attributes'" might be used, but I don't see one that goes beyond what can be achieved by operating on the Map<String, Integer> instead of the containing object.

    // Even if this operated on the `...Model` classes, it would 
    // not really worth the abstraction, IMHO...

    void call() {
        buildAttrib(g meshPrimitive.getAttributes()::put, a);
        buildAttrib(g meshInstancing.getAttributes()::put, a);
    }

    protected final Accessor buildAttrib(MeshGltfWriter _geoWriter,
        BiConsumer<String, Integer> consumer, String _attribute)
    {
        ...
        consumer.accept(_attribute, _accessorIdx);
        return _accessor;
    }

[chadj2]

And this is just a Consumer<String, Integer>, for what it's worth.

The above statement fails to capture an important detail. The integer in the attributes is always an index to an Accessor.

[javagl]

I don't see how that's relevant. There is a function that is called with a String and an Integer. What these values "are" does not matter for the signature.

[javagl]

@chadj2 It took a bit longer than expected until I found the time, but there is a PR at https://github.com/javagl/JglTF/pull/77 - maybe you want to have a look. Otherwise, I'll merge it at some point, and schedule a release a bit later.

[chadj2]

This look helpful. I should be able to remove some of my workaround code.

[javagl]

@chadj2 If you have any feedback for the linked PR, just drop me a note (here or in the PR). Otherwise, I'll merge it soon, and see whether I can allocate some time to address some other (smaller) open issues and schedule a new release.

[javagl]

This is outdated insofar that 2.0.4 has been released. Yes, this might once more "break some fields". But given that this is a relatively large project, and a one-man-show, solely maintained in my spare time, there are trade-offs.

At least, I started a change log at https://github.com/javagl/JglTF/blob/master/CHANGES.md . Maybe that helps...