-
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
Clear tiles with zoom dependent line-width
layers from the terrain render cache
#12510
Conversation
dfd3d0b
to
fb92092
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.
The change looks good in itself, like you mentioned it's a bit worrisome to regress on performance with that change, putting the links to the benchmarks of the style for reference:
Before going forward with this change, is there any alternative approach we could think about? For example, only trigger the render cache reload on larger/visible line-width differences (e.g. > 1/2px) in order to try and retain performance while still giving accurate results.
Otherwise, we should probably prefer the accuracy of the result and optimize this in the style itself.
@karimnaaji I've tried an approach that @mourner suggested — to clear terrain render cache on map mapbox-gl-js/src/terrain/terrain.js Lines 285 to 287 in 1a19e93
https://sites.mapbox.com/benchmap-js-results/runs/1289 |
Yes that's a good idea and seems like a good compromise, the benchmarks look good now 👍 |
1a19e93
to
38e90a3
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.
👍 Fix looks good to me
Note: I've tried to cover the issue in the render test but couldn't reproduce it test case{
"version": 8,
"metadata": {
"test": {
"operations": [
["setProjection", "globe"],
["wait"],
["zoomTo", 2.9, {"duration": 2000}],
["wait"]
]
}
},
"center": [0, 0],
"zoom": 2.0,
"pitch": 0,
"bearing": 0,
"sources": {
"geojson": {
"type": "geojson",
"data": {
"type": "MultiLineString",
"coordinates": [
[
[
0,
20
],
[
20,
0
],
[
0,
-20
],
[
-20,
0
],
[
0,
20
]
]
]
}
}
},
"layers": [
{
"id": "background",
"type": "background",
"paint": {
"background-color": "white"
}
},
{
"id": "line",
"type": "line",
"source": "geojson",
"paint": {
"line-width": 10,
"line-opacity": 1
}
}
]
}
|
GL JS is not considering a line width change as a prerequisite to clear off tiles from the terrain render cache. Because of that, while zooming between two zoom levels, GL JS is using a stale zoom in
getPixelsToTileUnitsMatrix
during the entire range [z..z+1].This fix clears tiles with zoom-dependent
line-width
layers from the terrain render cache. One thing that bugs me is that most Mapbox core styles use a composite source with zoom-dependant line layers. So we are basically disabling the terrain render cache for the composite source in core styles here.Closes #11956
Benchmarks
After running benchmarks comparing the main branch with this PR, there is a regression in
frameTimeAverage
andpercentDroppedFrames
, see:streets-v11
https://sites.mapbox.com/benchmap-js-results/runs/1266streets-v12
https://sites.mapbox.com/benchmap-js-results/runs/1267Before
before-line-width-fix.mov
After
after-line-width-fix.mov
Launch Checklist
mapbox-gl-js
changelog:<changelog>fix blur on draped lines while zoom-in</changelog>