Skip to content

Add height reference (clamp-to-ground) to ModelExperimental #10448

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 7 commits into from
Jun 10, 2022

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jun 10, 2022

This PR adds a heightReference option to ModelExperimental to allow for clamp-to-ground behavior. Most of the code is taken from Model, though ModelExperimental.update had to be reorganized so things could be updated in the right order (model matrix, bounding spheres, reference matrices).

Sandcastle for ModelExperimental, and a sandcastle for Model for comparison in behavior.

I tried to add rendering tests for this but it was difficult to because of the globe. Even if the Model is positioned very high, the globe still peeks over the horizon. Then, globe.show to false disables the clamp-to-ground behavior, so I can't prevent it from rendering during the unit test.

@j9liu j9liu requested a review from ptrgags June 10, 2022 18:21
@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

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

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@j9liu did a pass through the code, looks sensible though I had an idea for refactoring ModelExperimental.update().

I still need to try out the code in Sandcastle, I'll do that next

@ptrgags
Copy link
Contributor

ptrgags commented Jun 10, 2022

tried various sandcastles, seems to be working nicely!

@j9liu
Copy link
Contributor Author

j9liu commented Jun 10, 2022

@ptrgags updated!

@ptrgags
Copy link
Contributor

ptrgags commented Jun 10, 2022

@j9liu the update function looks a lot nicer now!, I'll merge once CI passes

@ptrgags ptrgags merged commit 1c0576f into main Jun 10, 2022
@ptrgags ptrgags deleted the model-experimental-height-reference branch June 10, 2022 21:24
@lilleyse
Copy link
Contributor

🥇 on the update cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants