Throw exception when unsupported extension is required#2628
Throw exception when unsupported extension is required#2628javagl wants to merge 6 commits intojMonkeyEngine:masterfrom
Conversation
jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/GltfModelKey.java
Outdated
Show resolved
Hide resolved
|
Thanks!
I think there is no problem in adding this to an unit test here . |
|
Then I'll add this I think that it could make sense to try and cover more of the loader functionality with unit tests in general. Should I also consider adding a test for the |
|
Sure, the more unit tests, the better. Feel free to add any unit test you deem necessary. |
|
Before committing/pushing my local state, I'd like to confirm that the approach is OK:
The actual test for the extension handling should be straightforward @Test
public void testRequiredExtensionHandling() {
// By default, the unsupported extension that is listed in
// the 'extensionsRequired' will cause an AssetLoadException
Assert.assertThrows(AssetLoadException.class, () -> {
GltfModelKey gltfModelKey = new GltfModelKey("gltf/TriangleUnsupportedExtensionRequired.gltf");
Spatial scene = assetManager.loadModel(gltfModelKey);
dumpScene(scene, 0);
});
// When setting the 'strict' flag to 'false', then the
// asset will be loaded despite the unsupported extension
try {
GltfModelKey gltfModelKey = new GltfModelKey("gltf/TriangleUnsupportedExtensionRequired.gltf");
gltfModelKey.setStrict(false);
Spatial scene = assetManager.loadModel(gltfModelKey);
dumpScene(scene, 0);
} catch (AssetLoadException ex) {
ex.printStackTrace();
Assert.fail("Failed to load TriangleUnsupportedExtensionRequired");
}
}The test for the Draco handling has some degrees of freedom for how "deep" and "strict" the test should be. A first shot would be this: @Test
public void testDracoExtension() {
try {
Spatial scene = assetManager.loadModel("gltf/unitSquare11x11_unsignedShortTexCoords-draco.glb");
Node node0 = (Node) scene;
Node node1 = (Node) node0.getChild(0);
Node node2 = (Node) node1.getChild(0);
Geometry geometry = (Geometry) node2.getChild(0);
Mesh mesh = geometry.getMesh();
// The geometry has 11x11 vertices arranged in a square,
// so there are 10 x 10 * 2 triangles
VertexBuffer indices = mesh.getBuffer(VertexBuffer.Type.Index);
Assert.assertEquals(10 * 10 * 2, indices.getNumElements());
Assert.assertEquals(VertexBuffer.Format.UnsignedShort, indices.getFormat());
// All attributes of the 11 x 11 vertices are stored as Float
// attributes (even the texture coordinates, which originally
// had been normalized(!) unsigned shorts!)
VertexBuffer positions = mesh.getBuffer(VertexBuffer.Type.Position);
Assert.assertEquals(11 * 11, positions.getNumElements());
Assert.assertEquals(VertexBuffer.Format.Float, positions.getFormat());
VertexBuffer normal = mesh.getBuffer(VertexBuffer.Type.Normal);
Assert.assertEquals(11 * 11, normal.getNumElements());
Assert.assertEquals(VertexBuffer.Format.Float, normal.getFormat());
VertexBuffer texCoord = mesh.getBuffer(VertexBuffer.Type.TexCoord);
Assert.assertEquals(11 * 11, texCoord.getNumElements());
Assert.assertEquals(VertexBuffer.Format.Float, texCoord.getFormat());
dumpScene(scene, 0);
} catch (AssetLoadException ex) {
ex.printStackTrace();
Assert.fail("Failed to import unitSquare11x11_unsignedShortTexCoords");
}
} It is testing that the data can be loaded, and contains the right attributes with the right number of elements and types. Two things that I'm not sure about:
I considered to go further, and maybe even check the decoded values in the buffers. For example, one could check all buffer values, or at least e.g. that the "upper right" one has the texture coordinates (1.0, 1.0), as in float tx = (float) texCoord.getElementComponent(110, 0);
float ty = (float) texCoord.getElementComponent(110, 1);
Assert.assertEquals(1.0f, tx, 0.0f);
Assert.assertEquals(1.0f, ty, 0.0f);to ensure that the |
|
Thanks, i don't see anything wrong in this approach.
Yes, i think it is generally fine, the layout of imported models should not change anyway
looks good to me |
|
I changed the initialization of the I think that at some point, it could be worth considering to add tests that check the decoded data. The specific case of the Draco-compressed @riccardobl Ready for a (maybe final) review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a stricter handling of unsupported but required glTF extensions. By default, the loader will now throw an AssetLoadException, which provides clearer error feedback than the previous behavior of logging an error and potentially failing later with an unrelated exception. The strictness can be disabled via GltfModelKey#setStrict(false). The changes include refactoring the extension handling logic for better clarity and adding unit tests to cover the new behavior.
My review focuses on ensuring the new default behavior is consistently applied and improving the clarity of the new API's documentation. Overall, this is a valuable improvement for asset loading robustness.
jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/CustomContentManager.java
Outdated
Show resolved
Hide resolved
jme3-plugins/src/gltf/java/com/jme3/scene/plugins/gltf/GltfModelKey.java
Show resolved
Hide resolved
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
2 participants
Fixes #2617
Current state
Until now, the glTF loader only printed a
SEVEREerror message when an unsupported extension was declared in theextensionsRequiredof a glTF asset. As a result, it could happen that ...In both cases, the error could show up as a arbitrary exception during loading or rendering. It could be a
NullPointerException,IndexOutOfBoundsException, or whatever - with no clear indication for what caused this exception.New state
This PR changes the behavior as follows:
extensionsRequired, then the default behavior is that the loader immediately bails out with anAssetLoadException, with an appropriate error message to indicate the reason for the errorGltfModelKey#setStrict(boolean)function. When explicitly settinggltfModelKey.setStrict(false), then any unsupported extension will still (only) cause aSEVEREerror message (and likely, but not necessarily, cause an exception later...)Testing
I'm not entirely sure about the best test procedure here. I tested it locally, in two ways:
I tested it by loading the SheenWoodLeatherSofa that was mentioned in #2385 . It now causes the exception
Instead of that random NPE that it caused until now.
I also created a dedicated tests for the extension declaration handling. For this, I created the following asset, which is just the (embedded) version of a single triangle, with extension declarations for testing:
{ "extensionsUsed": [ "KHR_texture_transform", "EXAMPLE_unsupported_extension" ], "extensionsRequired": [ "KHR_texture_transform", "EXAMPLE_unsupported_extension" ], "scene" : 0, "scenes" : [ { "nodes" : [ 0 ] } ], "nodes" : [ { "mesh" : 0 } ], "meshes" : [ { "primitives" : [ { "attributes" : { "POSITION" : 1 }, "indices" : 0 } ] } ], "buffers" : [ { "uri" : "data:application/octet-stream;base64,AAABAAIAAAAAAAAAAAAAAAAAAAAAAIA/AAAAAAAAAAAAAAAAAACAPwAAAAA=", "byteLength" : 44 } ], "bufferViews" : [ { "buffer" : 0, "byteOffset" : 0, "byteLength" : 6, "target" : 34963 }, { "buffer" : 0, "byteOffset" : 8, "byteLength" : 36, "target" : 34962 } ], "accessors" : [ { "bufferView" : 0, "byteOffset" : 0, "componentType" : 5123, "count" : 3, "type" : "SCALAR", "max" : [ 2 ], "min" : [ 0 ] }, { "bufferView" : 1, "byteOffset" : 0, "componentType" : 5126, "count" : 3, "type" : "VEC3", "max" : [ 1.0, 1.0, 0.0 ], "min" : [ 0.0, 0.0, 0.0 ] } ], "asset" : { "version" : "2.0" } }`The default behavior here is also the expected one:
When loading this with
gltfModelKey.setStrict(false), then the result is only a log message:SCHWERWIEGEND: Extension EXAMPLE_unsupported_extension is required for this file. The behavior of the loader is unspecified.The main point here is: I'm not sure how to "consolidate" these tests. That
TriangleUnsupportedExtensionRequired.gltffile is currently not checked in (but it is small, so it could probably be added...).And I did the tests with manual tweaks to the
TestGltfLoadingclass. I can always check in "my local, current state" of this class. But I wondered whether there is a way to streamline the different tests that should be done, without someone having to twiddle with// commented-outlines.However, for the time being, here is my current local state of the
TestGltfLoading, in case someone wants to try it out locally: