-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Globe: symbol and coveringTiles optimizations #4778
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4778 +/- ##
==========================================
+ Coverage 87.99% 88.08% +0.09%
==========================================
Files 265 265
Lines 37591 37659 +68
Branches 2337 2343 +6
==========================================
+ Hits 33078 33172 +94
+ Misses 3482 3456 -26
Partials 1031 1031 ☔ View full report in Codecov by Sentry. |
I've added a few comments. thanks for this! (it's currently closed due to globe branch deletion, it should be opened soon). |
New PR is here: @NathanMOlson - can you check that the covering tiles optimization and improvements done here are incorporated in your PR - I would like to separate responsibilities: have Nathan's PR address covering tiles stuff and have this PR focus on symbol optimizations. |
@HarelM @NathanMOlson thank you for your feedback! I will address it and improve this PR in a week or two, sadly I need to focus on other things in the meantime. I think this PR will not create merge conflicts with the terrain tile LOD PR, since that changes different parts of the covering tiles logic. |
The two optimizations for covering tiles in this PR are
I don't think these changes are ready to be incorporated in my PR. |
I would advise to remove covering tiles related logic from this PR and optionally add it to @NathanMOlson's PR if applicable. |
I have reverted most of coveringTiles changes, with one exception at line 84: return Math.min(centerDist, cameraDist) * 2 <= radiusOfMaxLvlLodInTiles * tileSize; // Multiply distance by 2, because the subdivided tiles would be half the size This is a simple bugfix, before this change globe displayed unnecessarily many tiles at high zoom levels. This alone improves performance a lot when camera is pitched. I will try to implement a better tile culling by using general bounding frustums instead of AABBs. Frustums can approximate the actual tile shape much more tightly, thus leading to more discarded tiles, thus improving performance. A frustum-frustum visibility test should not be much slower than frustum-AABB test, but I will measure the times and create a benchmark. I will also implement a tile bounding volume cache that does not leak memory. I will do that as a separate PR. |
Added a few more comments. Otherwise looks good enough to be merged. Thanks! |
Can you update the branch from main to resolve conflicts? |
# Conflicts: # CHANGELOG.md
Should be fixed now, I also fixed the changelog wording and the failing render test (reverted the expected image to its state before this PR). |
This PR ports symbol rendering optimizations from non-globe main branch to current globe-based main and adds some other globe-specific optimizations on top:
isTilePositionOccluded
(the information was already computed beforehand).Improved globe's coveringTiles performance by caching tile AABBs instead of recomputing them every time.Fixed globe's coveringTiles incorrectly returning hundreds of hidden tiles as visible if the camera is sufficiently pitched.Improved globe's coveringTiles tile visibility accuracy, reducing the number of returned tiles and thus improving performance.SymbolCollisionBoxGlobe
benchmark for symbol placement under globe projection. This is the same asSymbolCollisionBox
, just with globe projection used instead of mercator.Below are benchmark results for mercator projection. Here "main" essentially represents globe branch without optimizations ported over, thus it is relatively slow.
These next two figures are the new globe placement benchmark, first on the current main branch, then on this PR branch.