consider size change when placing symbols #2065
Conversation
fix #1196 Symbols can size change within a single zoom level. When size changes all the units that are relative to size, like text-offset, change. Collision detection can't handle boxes that scale at different rates so we try to create a collision box that will cover the symbol at all sizes within that zoom level. It used to just use the size at z+1 with the assumption that text always increases and that the anchor is always within the collision box. This commit makes it use both the size at z and z+1 so that symbol is always within the collision box.
ansis
force-pushed
the
symbol-collision-size-change
branch
from
0860d90 to
9de1674
Compare
| this.adjustedIconSizes = [ | ||
| this.layer.getLayoutValue('icon-size', this.zoom, zoomHistory), | ||
| this.layer.getLayoutValue('icon-size', this.zoom + 1, zoomHistory) | ||
| ]; |
There was a problem hiding this comment.
This isn't an ideal general-case solution because zoom stops can be at fractional zooms. How would you feel about adding a getMaxLayoutValue(name, minZoom, maxZoom, zoomHistory) that does a little more function introspection?
There was a problem hiding this comment.
Yes, if a value between z and z+1 is bigger or smaller than both of the two values at z and z + 1 then this solution isn't ideal. Having a getMaxLayoutValue and a getMinLayoutValue would be more correct but I'm not sure it would make a significant enough difference to be worth the extra complexity.
For this to make an actual difference in practice the difference between the maximum size between z and z + 1 would need to be significantly bigger than the maximum of the value at z and z + 1.
It might be worth it. What do you think? If it's worth it, do you want to take a shot at adding this?
There was a problem hiding this comment.
It is a simple algorithm. It really ought to go in the mapbox-gl-function repo but I'd look the other way.
Math.max(at(minZoom), at(maxZoom), at(all stops between minzoom and maxzoom))
I have too much on my plate to tackle this in the next couple days. I think you should do it. Its 3 - 4 lines of code that'll
- be the fully correct solution to the problem
- eliminate all known edge cases
- expose an API that might be useful in the future
- hedge against ever having to look at this particular code again
|
@ansis any updates here? |
|
I am closing this PR because it has been inactive for a long time. The branch isn't going anywhere so please keep working on this feature and re-open the PR when it is ready! Let me know if you have any questions. |
fix #1196
Symbols were sometimes not contained by their collision boxes. This fixes that.
Symbols can size change within a single zoom level. When size changes all the units that are relative to size, like text-offset, change.
Collision detection can't handle boxes that scale at different rates so we try to create a collision box that will cover the symbol at all sizes within that zoom level.
It used to just use the size at z+1 with the assumption that text always increases and that the anchor is always within the collision box.
This commit makes it use both the size at z and z+1 so that symbol is always within the collision box.