CesiumGS / cesium-native

Apache License 2.0
402 stars 205 forks source link

Fix problems with QuantizedMeshLoader-generated glTFs #828

Closed kring closed 4 months ago

kring commented 5 months ago

Before this PR, the glTFs generated from quantized-mesh tiles were technically invalid. Our game engine clients weren't too fussed about these problems, which is why we didn't notice.

Fixed the following problems:

csciguy8 commented 4 months ago

Do we have any unit tests for QuantizedMeshLoader::load? (an unsurprising question from me)

If not, sounds like it would be useful. Could mimic what a typical native client would do, and have all our assertions about what a proper glTF should look like.

kring commented 4 months ago

Do we have any unit tests for QuantizedMeshLoader::load?

Yes we do, though it looks like it missed a rename somewhere along the way and was named TestQuantizedMeshContent.cpp instead of TestQuantizedMeshLoader.cpp. I've now renamed it in this branch.

Could mimic what a typical native client would do, and have all our assertions about what a proper glTF should look like.

That would be a glTF validator. It would be great to have such a thing in cesium-native, but it's way outside the scope of this PR.

I've updated this PR to add some assertions about the things that were fixed, though. This is really just closing the barn door after the horse already bolted, and does nothing to prove the generated glTFs aren't incorrect in other ways we haven't already noticed. But it's easy to do and perhaps could catch regressions in the future, so why not?

j9liu commented 4 months ago

Looks good @kring ! I'll wait for CI to pass, then merge.