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

Don't apply cullRequestsWhileMoving optimization when tileset is moving #8598

Merged
merged 2 commits into from
Feb 6, 2020

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Feb 6, 2020

For #8580

Only applies the cullRequestsWhileMoving optimization if a tileset is stationary. This is to fix cases where the camera is tracking a moving tileset like #8580. You could do something more complicated like comparing the camera delta with the tileset delta #8580 (comment) but I wanted to keep it simple. It's not very common for tilesets to be moving so I think it's ok to just disable the optimization.

I put together a Sandcastle to simulate the fix. The NYC buildings are shifting slightly and the camera is moving. In local sandcastle you see tiles stream in during the flight path while in master you don't.

Local sandcastle with the fix
cesium.com sandcastle without the fix

@lilleyse lilleyse requested a review from OmarShehata February 6, 2020 22:07
@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

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

@lilleyse lilleyse changed the title Don't cull tiles when tileset is moving Don't apply cullRequestsWhileMoving optimization when tileset is moving Feb 6, 2020
@OmarShehata
Copy link
Contributor

Thanks @lilleyse ! This is the right fix in my opinion, since 3D Tilesets are assumed to be stationary by default, and this is an additional optimization we make in the common case, that we can just turn off for this uncommon case. I can confirm this fixes the issue brought up in #8580 (comment).

I was going to ask if we should consider making a getter for cullRequestsWhileMoving since tileset.cullRequestsWhileMoving is no longer the "true" value, but I think it's an internal detail that this is disabled when the tileset is moving. So if you set this value to true, it should be true when you query it, even when the tileset is moving, since we document that this optimization only applies for stationary tilesets.

The only way I can think to break this is if the tileset happens to be moving by modifying tileset.root.transform instead of tileset.modelMatrix (Sandcastle example). Although I would say this isn't worth the extra check to account for.

Thoughts?

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 6, 2020

The only way I can think to break this is if the tileset happens to be moving by modifying tileset.root.transform instead of tileset.modelMatrix (Sandcastle example). Although I would say this isn't worth the extra check to account for.

Ah yes! I thought of that case as well but forgot to mention it here. I looked at the code in #8580 to make sure it was targeting modelMatrix instead of the root transform. I also agree it's not worth accounting for since it's not really the "sanctioned" way of moving a tileset, but if it does happen it will be an easy fix.

@OmarShehata OmarShehata merged commit 6b35dc6 into master Feb 6, 2020
@OmarShehata OmarShehata deleted the dont-cull-when-moving branch February 6, 2020 22:59
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.

3 participants