Support EXT_mesh_primitive_restart glTF extension

[eringram]

Description

This PR adds support for the EXT_mesh_primitive_restart glTF extension. It is based on the implementation in iTwin.js-- see getMeshPrimitives in GltfReader.ts.

"Primitive restart" refers to starting a new primitive of the same type as the current primitive when a specific index value is encountered (the maximum value for that index buffer type). It is useful for batching multiple primitives of the same type in a single draw call. See the README in this PR for more detailed explanation and a simple example.

Issue number and link

Addresses #12764

Testing plan

• Added a unit test to GltfLoaderSpec.js that uses the MeshPrimitiveRestart.gltf test model and confirms that the new getMeshPrimitives function reduces the length of the primitives array from 8 to 4 (effectively combining the 4 groups of 2 primitives each).
  • The test model includes primitive restart groups for all 4 supported modes.
• Added unit tests that confirm the extension is not used if the glTF model violates the spec (i.e. uses a primitive in multiple groups, a primitive has an unsupported mode, etc.)
• You can also manually test by opening MeshPrimitiveRestart.gltf in Sandcastle and stepping through the code to confirm the extension is used. This is what the model looks like:

(the 4 groups are two green triangles, two blue triangles, two green line strips, and two blue line loops)

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @eringram! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

• Send in a Contributor License Agreement (CLA)
• Add yourself to the contributors file

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

[ggetz]

Thanks @eringram! I can confirm we have a CLA on file covering you.

[markschlosseratbentley]

I read through the code. This looks great; thanks @eringram.

[ggetz]

@eringram Is this PR ready for full or partial review? If so, could you please update the PR description with the current status? Thanks!

[eringram]

@eringram Is this PR ready for full or partial review? If so, could you please update the PR description with the current status? Thanks!

Sorry just updated the description! The code in GltfLoader.js is ready for review and still working on more thorough unit tests.

[eringram]

I updated the MeshPrimitiveRestart.gltf test data to include primitive groups for all the valid modes for the extension (line loop, line strip, triangle strip, triangle fan). Also added more unit tests to ensure getMeshPrimitives falls back to the default mesh.primitives if EXT_mesh_primitive_restart violates the spec. Two of those tests are failing, where the extension is used even if incorrect, so still fixing that.

Also, @javagl @ggetz any idea why this test that calls toBeRejectedWithError is failing to match the error message regex?

This is the output when it runs and says it fails, but the errors look identical to me:

Nevermind, just noticed the cases are different in the error messages...

3) does not load with EXT_mesh_primitive_restart if a group's indices accessor is invalid
     Scene/GltfLoader
     Expected a promise to be rejected with RuntimeError: /Failed to load glTF
Failed to load index buffer
indexDataType is required and must be a valid IndexDatatype constant./ but it was rejected with RuntimeError: Failed to load glTF
Failed to load index buffer
indexDatatype is required and must be a valid IndexDatatype constant.
Original stack:
Original stack:
Error
    at new DeveloperError (packages/engine/Source/Core/DeveloperError.js:39:11 <- Build/CesiumUnminified/Cesium.js:8893:13)
    at IndexDatatype.getSizeInBytes (packages/engine/Source/Core/IndexDatatype.js:62:9 <- Build/CesiumUnminified/Cesium.js:26912:11)
    at createIndicesTypedArray (packages/engine/Source/Scene/GltfIndexBufferLoader.js:248:35 <- Build/CesiumUnminified/Cesium.js:70949:45)
    at loadFromBufferView2 (packages/engine/Source/Scene/GltfIndexBufferLoader.js:227:37 <- Build/CesiumUnminified/Cesium.js:70930:39)
    at async Promise.all (index 7)
Handler stack:
Error
    at new RuntimeError (packages/engine/Source/Core/RuntimeError.js:38:11 <- Build/CesiumUnminified/Cesium.js:13822:13)
    at ResourceLoader.getError (Build/Ce ... ....
    at <Jasmine>
    at UserContext.<anonymous> (packages/engine/Specs/Scene/GltfLoaderSpec.js:4263:9 <- Build/Specs/SpecList.js:209938:9)
    at <Jasmine>

[eringram]

This is ready for full review.

Anyone have an idea why the prettier format check is failing? It does not cause a warning for CHANGES.md when I run locally.

[javagl]

(Quick, while in another call:)

Regarding the formatting: Indeed, something is strange here: #12801 (comment)

The test that is failing now is related to some error message not matching the expected one, at https://github.com/CesiumGS/cesium/pull/12792/files#diff-8f11fb72d71b01b789e5660c80ad63658fa8768871b77786e95b636aa918c6c4R4277 .

A bit more generally, I think that it's a bit dubious to check for the exact error message, even though this is done in many places. I think that this "overspecifies" the test. (I hope there are no plans on internationalization, FWIW...). One could try to "justify" that, but only with the severe lack of differentiation between types of errors: There should be far more than just RuntimeError and DeveloperError, and the latter is used in many places where the developer actually has no influence on anything, and it should rather be a DataError or so.

However, the test here could just be changed to expect a type error, but ... of course, such an error should preferably not be thrown to the user either...

[eringram]

Regarding the formatting: Indeed, something is strange here: #12801 (comment)

Ok good to know it wasn't just me, I ended up just reverting the whitespace. Not sure how to fix the discrepancy it but it seems related to the blanks-around-fences markdownlint rule.

A bit more generally, I think that it's a bit dubious to check for the exact error message, even though this is done in many places. I think that this "overspecifies" the test. (I hope there are no plans on internationalization, FWIW...). One could try to "justify" that, but only with the severe lack of differentiation between types of errors: There should be far more than just RuntimeError and DeveloperError, and the latter is used in many places where the developer actually has no influence on anything, and it should rather be a DataError or so.

However, the test here could just be changed to expect a type error, but ... of course, such an error should preferably not be thrown to the user either...

These considerations make sense, you're right that the lack of error types kind of leads to comparing the error messages like this. I tried to use a generic Error type, but now see that the test is timing out. Any idea about that? I see other tests using await expectAsync(...).toBeRejectedWithError() that don't appear to increase their timeout past 5000ms.

[eringram]

Tbh, after further thinking, the test that is failing is probably not necessary in the first place. Its purpose is to test that an error is thrown when a mesh primitive restart group's indices property is invalid (type isn't scalar, component type isn't unsigned integer). However, this is not specific to EXT_mesh_primitive_restart, but applies to the glTF being valid in general. The primitiveGroup.indices eventually becomes a mesh.primitive.indices here, so as long as mesh.primitive.indices being valid is tested, this not necessary.

[ggetz]

This is looking great @eringram!

For the sake of keeping the documentation up-to-date, please add EXT_mesh_primitive_restart to the list of supported extensions in Model.js.

[ggetz]

Once merged, this change will go into the next release, which will be on Sept 2 (typically it's on the 1st of each month, but Sept 1 is Labor Day in the US).

Please move this item to the H2 section above under a new "additions" subsection.

[eringram]

Fixed, I somehow missed the latest release header

[ggetz]

This file is already pretty brutal in terms of length and separation of concerns. What do you think of moving this function to it's own file?

[eringram]

I agree, moved to a new file.

[eringram]

@pmconne Not required, but checking to see if you intended to review this

[ggetz]

Looks great! Thanks @eringram!