-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Conditionally render tiles to a separate tile canvas #4557
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
Conversation
04190a3 to
fc7d15b
Compare
|
Nice, Andreas! I gave it a quick test, unfortunately I am seeing gaps in Chrome (with rotation) and also in Firefox (both rotated and unrotated):
Why is it necessary to use the buffer for rotated views? Is there a difference on how the vector tile canvas is drawn compared to how raster tiles are drawn? |
fb3cb8c to
1bcaa40
Compare
|
@tsauerwein I made a few changes now, so at least without rotation things should be fine now. Can you please test again? For rotated views, I did not find a way to make things work without clipping - maybe you want to give it a try or think it through? Right now the tile context holds a rotated image. Without clipping would only work if there is a way for the tile context to hold an unrotated image with rotated labels and symbols. |
1bcaa40 to
c804966
Compare
|
@tsauerwein I have added another commit which changes the code so it now renders unrotated tiles with rotated labels to a rotated map canvas. This avoids the use of any clip geometries, but for some reason the labels do not work correctly when the pixel ratio is different than 1. I cannot find where the problem is. Can you take a look please? Maybe I am missing something obvious, or there is a bug elsewhere. |
|
@ahocevar I am still seeing gaps on Firefox also without rotation. I have been wondering about the difference to the raster tile renderer. The raster tile renderer first renders a canvas on which each tile image is drawn un-transformed. Then this canvas is drawn onto the main canvas using the transformation matrix. Right now, the position for each vector tile canvas is calculated using the transformation matrix. Do you think this could make a difference? |
|
Right, the difference is that the raster tile renderer does not render to the map canvas, but to a layer canvas that is then composed with other layers by the map renderer. That layer canvas is bigger than the map canvas, because it covers the complete required tile range and not just the visible viewport. However, that is inefficient - try to load 5 raster layers and pan the map, which is almost unusable. I'd like to move towards composing directly to the map canvas also for raster tile layers, so it'd be good to get this approach working. I consider it work in progress for now, and I'm sure the transforms and/or the way we put tiles on the map canvas can be improved. The biggest show stopper at the moment is that I cannot figure out why the canvas replay does not rotate labels properly when a pixel ratio other than 1 is used. I was hoping to get another pair of eyes looking at the code, potentially figuring out what might be wrong. |
42bd710 to
1153e3f
Compare
|
@tsauerwein I updated the transforms to use integer pixels everywhere, and now the gaps should really be gone in Firefox (when not rotated). Improvements for rotated views should also be possible. But I'd still appreciate if you could look at the code, to see if you find something wrong that causes the labels to be rendered incorrectly when the pixel ratio is not 1. |
|
I found the problem @tsauerwein - replay with text styles only works on contexts that are not transformed, because a transform is applied to the context, which resets the previous transform. I think I'll be able to fix this now. |
This? I have been wondering about that, but I didn't understand why it worked for a pixel-ratio of 1. Curious about your fix! :) |
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 pixel-ratio is not used in this transform (it used to be). Is that correct?
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 pixel ratio is now in pixelScale.
1153e3f to
ac64f7e
Compare
ac64f7e to
a08e4e2
Compare
|
@tsauerwein Thanks for your help! I think this is now ready for review. |
Because clip geometries are anti-aliased in most browsers, there will be tiny gaps between tiles. If tiles are rendered to a tile canvas which is then drawn to the map canvas upon composition, these gaps can be avoided. For rotated views, it is stil necessary to clip the tile, but in this case a 1-pixel buffer is used. This change also brings a huge performance improvement for panning, because the fully rendered tiles can be reused. Because of the added cost of using drawImage in addition to replaying the tile replay group, we fall back to directly drawing to the map canvas when the tile canvas would be too large, or during interaction/animation when resolution or rotation change.
This only works when the device pixel ratio is 1. Labels are incorrectly positioned and not at all rotated for other pixel ratios. I cannot find the cause for this problem.
a08e4e2 to
80f10ce
Compare
349d817 to
172fcfb
Compare
172fcfb to
9affb99
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.
Is this buffer still needed?
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 indeed be removed now that direct rendering is a corner case. Will do so tomorrow morning.
74cb818 to
38bb156
Compare
|
I am +1 for merging. Thanks for the great work, Andreas! |
|
Thanks for the review and your help @tsauerwein. |
Conditionally render tiles to a separate tile canvas
|
Nice to have improvements in performance! Do i understand it correctly, that thin white lines can still appear? |
|
@syntax42 Yes, on rotation and during zoom animations. But these will go away soon in another round of improvements. |






Because clip geometries are anti-aliased in most browsers, there will be tiny
gaps between tiles. If tiles are rendered to a tile canvas which is then drawn
to the map canvas upon composition, these gaps can be avoided. For rotated
views, it is stil necessary to clip the tile, but in this case a 1-pixel
buffer is used.
This change also brings a huge performance improvement for panning, because
the fully rendered tiles can be reused.
Because of the added cost of using drawImage in addition to replaying the tile
replay group, we fall back to directly drawing to the map canvas when the tile
canvas would be too large, or during interaction/animation when resolution or
rotation change.
Fixes #4329.