Throw exception when unsupported extension is required#2628
Throw exception when unsupported extension is required#2628javagl wants to merge 1 commit intojMonkeyEngine:masterfrom
Conversation
| * encounters an asset that contains an extension in its <code>extensionsRequired</code> declaration that | ||
| * is not supported. | ||
| */ | ||
| private boolean strictExtensionCheck; |
There was a problem hiding this comment.
| private boolean strictExtensionCheck; | |
| private boolean strictExtensionCheck = true; |
better setting it here imo
There was a problem hiding this comment.
It may be a detail of the "style". I'm usually leaving all fields uninitialized when they are declared, and initialize all of them in the constructor (as that's what constructors are for), with the usual "best practice" of making things final whenever possible, and making sure that there is (exactly) one constructor where this is happening.
However, if it is preferred, I'll initialize it at declaration time here (and remove the assignments in the constructors).
There was a problem hiding this comment.
Many people contribute to this codebase, so the more we can keep things "out of the way", the better. In this case it is just assigning a default, so for us it is better to initialize the field right away.
|
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 |
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: