-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
optimize findLoadedParent #9050
Conversation
6cd5dbc
to
f1a98a1
Compare
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.
Could you please describe the algorithm a little bit more, ideally in code comments?
src/source/tile_id.js
Outdated
@@ -95,6 +95,16 @@ export class OverscaledTileID { | |||
} | |||
} | |||
|
|||
calculateScaledKey(targetZ: number, wrap: 0 | 1) { |
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.
I remember that wrap
can also be -1
in some situations, or 2
if we show tiles in an extreme wide map view.
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.
Added comment.
How does it sound 4673132#diff-e444e40548c379347d0aa642beffc811R99?
Renamed wrap to withWrap as it has boolean semantics here.
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.
it should be fine to use use wrap: boolean
and true
/false
for this
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.
it should be fine to use use
wrap: boolean
andtrue
/false
for this
Prefer to keep 1/0 as they are used in multiplication later. Through ternary or +withWrap would do the same. Documentation should cover it.
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.
@ansis Thanks.
After offline discussion, withWrap: boolean
used and +withWrap
in multiplication. Comment updated.
Updated patch pushed.
7d0431c
to
4673132
Compare
Overall this looks good to me! So the savings are mostly from not needing to create all the tileid objects? |
Yep. |
Performance optimization when rendering view with a lot of satellite tiles. Measurements on Chrome (Version 78.0.3904.108 with CPU 4x slowdown in performance tab) on MacMini i7 3.2 Ghz. `map.repaint = true` in http://localhost:9966/debug/satellite.html#12.5/38.888/-77.01866/0/60 Observed FPS count: master: 19-20FPS with this patch: 24.5-26 FPS
4673132
to
c71b504
Compare
Performance optimization when rendering view with a large number of raster tiles.
Measurements on Chrome (Version 78.0.3904.108 with CPU 4x slowdown in performance tab) on MacMini i7 3.2 Ghz, 3840 x 1440 display.
map.repaint = true
in debug/satellite.html with pitch of 60 degreesFPS count, like on the screenshot below:
master: 19-20FPS
with this patch: 24.5-26 FPS