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

Make ModelExperimental's bounding sphere update with model matrix #10078

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Feb 4, 2022

Fixes #10074.

This PR fixes a bug where ModelExperimental's bounding sphere wouldn't be affected by changes in the model matrix. I added a unit test for it and observed that it worked in @lilleyse's sandcastle.

@j9liu j9liu requested a review from ptrgags February 4, 2022 21:42
@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 this PR does work as advertised. Though the bounding volume update code is spread out in several places, so we might want to review it holistically to see what if anything needs to be updated/simplified The model matrix and bounding sphere get updated in a few places:

  • in buildDrawCommands.js I see that a bounding volume is computed from the render resources' copy, in-place. Does that make sense to do still? Note that sometimes buildDrawCommands() gets called a second time if draw commands need to be re-built (such as making a big change like swapping out a custom shader or style). Though since the whole pipeline gets re-run, this might be okay.
  • I see that the bounding sphere gets updated in ModelMatrixUpdateStage -- we might need something similar for the final model bounding volume (like in the review comment below)

We can discuss this more on a call if needed since this is a little more nuanced than it first appears.

Source/Scene/ModelExperimental/ModelExperimental.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Feb 7, 2022

@ptrgags - just wanted to clarify your buildDrawCommands.js comment. I see that the boundingSphere is transformed by the model matrix in place. Is the concern that if buildDrawCommands needs to get run again, the boundingSphere will get transformed by the model matrix twice?

@ptrgags
Copy link
Contributor

ptrgags commented Feb 7, 2022

@j9liu yes, that's what I meant, though now that I think about it, even when rebuildDrawCommands() gets run, the entire pipeline gets run, so the bounding sphere I believe would be created from scratch so I think it's okay.

@j9liu
Copy link
Contributor Author

j9liu commented Feb 7, 2022

@ptrgags - got it. Is there anything else I should look at to wrap-up this PR?

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 closer, though I noticed that the original code's _modelMatrix vs modelMatrix is a bit confusing, must have been missed in a past review. I left some notes of how to neaten that up.

@ptrgags
Copy link
Contributor

ptrgags commented Feb 7, 2022

@j9liu looks good now, I'll merge once CI passes!

@ptrgags ptrgags merged commit 2de96fe into main Feb 7, 2022
@ptrgags ptrgags deleted the modelexperimental-boundingSphereFix branch February 7, 2022 21:15
@ptrgags
Copy link
Contributor

ptrgags commented Feb 7, 2022

thanks @j9liu!

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.

ModelExperimental bounding sphere not updated when model matrix changes
4 participants