-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Only load visible tiles during 3D Tiles traversal #5788
Conversation
@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time. I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to I am a bot who helps you make Cesium awesome! Thanks again. |
@lilleyse thanks, i will check with our datasets |
@lilleyse I checked with some of our datasets. Its much better then before 👍
But maybe we can do another optimization. If I use the above sandcastlecode the new code loads 85 b3dm tiles (without this change 255). But only 21 tiles are selected. |
e0c404f
to
7abdc73
Compare
Agreed - something more needs to be done during the base traversal code to not load so aggressively. Setting |
@jbo023 Updated. I tested out a few other data sets and didn't notice any issues, but someone else should quickly check as they review. |
@austinEng any chance you have a few minutes to look at this? We would like to include this in tomorrow's release. |
@likangning93 could you also please test this on a few datasets? |
} | ||
var showAdditive = parent.refine === Cesium3DTileRefine.ADD && parent._screenSpaceError > maximumScreenSpaceError; | ||
|
||
return isVisible(tile._visibilityPlaneMask) && (!showAdditive || getScreenSpaceError(tileset, parent.geometricError, tile, frameState) > maximumScreenSpaceError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to check this, but I believe that by the time you call this function in loadTile
, the results of getScreenSpaceError
have already been cached in tile._screenSpaceError
which gets computed in visitStart
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
computeSSE
caches but getScreenSpaceError
doesn't, so this should be ok.
Thanks for taking a look, @austinEng! |
@likangning93 could you please look at this in time for the release on Friday? Thanks! |
I checked with several of our datasets and didn't find any problems. The requests are now the same or less as they were before the traversal refactoring. With textured tilesets we see a good reduction in requests due to the skiplod level change. |
Fixes #5477
The traversal was not checking the visibility of a tile before loading it. This primarily affected situations where at least one child was visible but many other children weren't. After running the numbers I noticed big wins on some additive-refinement tilesets like in #5477 (256 tiles vs 19), NYC (201 vs 176) and less for replacement-refinement (San Miguel: 278 vs 265).
@jbo023 can you double-check this for your tilesets?