Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Cesium's use of glTF's optional skeleton property. #8756

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Apr 14, 2020

Cesium was using the optional glTF property skeleton as a required property, and it looks like it was only using it to discover a list of joint nodes, which are required to be in the glTF. This may be the result of historical changes along the way from early versions of glTF, I'm not sure.

In any case, this PR removes a pile of skeleton/forest searching logic and just uses the glTF joint list directly.

I tested CesiumMan of course, and a model of my own exported from Blender. Would be good to test this against a few more complex skinning examples to make sure nothing broke.

Fixes #8175.

@cesium-concierge
Copy link

Thanks for the pull request @emackey!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@lilleyse
Copy link
Contributor

All the rigged 1.0 and 2.0 sample models work, SimpleSkin works, but I can't see char.glb from #8175...?

@emackey
Copy link
Contributor Author

emackey commented Apr 15, 2020

I spent hours investigating char.glb (which is good, I need to learn more about skinning and rigging). I made my own sample models that use multiple skeletons, that use multiple meshes on one skeleton vs multiple skeletons, etc.

Finally I've discovered the problem: The root armature in char.glb has a uniform scaling factor of 0.01, making the model "too small" for Cesium. Removing this scaling factor fixes the model:

char_big.zip

You will still need to test on this branch of course, because the model also lacks the skeleton property and thus needs the fixes here.

I will say I'm much more confident in this branch after making so many different test models today.

VariousBendyThingTests.zip

@emackey
Copy link
Contributor Author

emackey commented Apr 15, 2020

So just to be clear, the original model in #8175 has multiple issues: (a) lack of skeleton property, support for which is fixed in this PR, and (2) scale is too small for Cesium, which I consider out of scope for what I'm trying to fix here.

@emackey
Copy link
Contributor Author

emackey commented Apr 15, 2020

I opened issue #8759 to track the scaling issue separately from the crashes reported in prior issues.

@lilleyse
Copy link
Contributor

Ah that was unexpected. Thanks for looking into it @emackey.

@lilleyse lilleyse merged commit 983d6dd into master Apr 15, 2020
@lilleyse lilleyse deleted the remove-skeleton-from-gltf-closet branch April 15, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animated glTF with no skeletons defined crashes
3 participants