Skip to content

Reorganize how normalized instance attributes are handled #10771

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

Merged
merged 8 commits into from
Sep 8, 2022

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Sep 8, 2022

Fixes #10766

As of the latest main, instance feature IDs weren't working because they were incorrectly being dequantized in GltVertexBufferLoader.

As described in #10766 (comment), we thought it would be better to not dequantize in the loader at all, only dequantize when absolutely necessary. It turns out there's only one such case where we need to dequantize on the CPU: ROTATION attributes with normalize: true. Translations and scales are not quantized according to the table in the EXT_mesh_gpu_instancing spec.

To summarize, this PR:

  • Removes CPU dequantization code from GltfVertexBufferLoader and GltfLoader
  • Only dequantizes rotations in InstancingPipelineStage

EDIT: Whoops forgot to add Sandcastle links:

  • Sandcastle from the issue -- it should look like a gradient in this branch:
    image
  • Sandcastle to try the test model -- NOTE: Since instancing bounding spheres use the translation min/max and don't take into account the rotations (that would be more expensive), the top-most box sticks out a little too much and gets clipped with some camera views.

image
2022-09-08_BoundingVolumeNotQuiteRight

@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ 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.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ptrgags
Copy link
Contributor Author

ptrgags commented Sep 8, 2022

Okay, this is now ready for review. @sanjeetsuhag could you review when you get a chance?

@ptrgags ptrgags requested a review from sanjeetsuhag September 8, 2022 18:39
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. Seems to be the correct fix. Thanks @ptrgags!

@sanjeetsuhag sanjeetsuhag merged commit 5f80a38 into main Sep 8, 2022
@sanjeetsuhag sanjeetsuhag deleted the fix-instancing-feature-ids branch September 8, 2022 19:14
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.

Feature IDs not working correctly for GPU instance metadata example
3 participants