Repository navigation
Re-implement "avoid edges" for MapMode::Tile - #12520
Conversation
|
Tests are failing because we need ignores or native port for mapbox/mapbox-gl-js#6961 |
ansis
left a comment
There was a problem hiding this comment.
Is it OK to force avoidEdges: true for line labels? I'm counting on MapMode::Tile being a special case (i.e. api-gl) where this makes sense.
Yes. I'm pretty sure we used to do this
What further testing do we need? The tile-mode render test is the only real testing I've done of this because I don't have an easy way to run an api-gl staging version.
I'm not sure. Would it be useful to have a simple tile server we can run without all of api-gl?
Am I right that the "withinPlus0" logic still doesn't do anything even with this re-implemented?
It looks like it changes the behaviour for tiled rendering when a feature has avoidEdges and is right on the right boundary of the tile (when x === EXTENT). It looks like this was introduced for #6670. But I'm not sure that was the right fix.
Your changes seem good to me.
Am I leaving a trap for future maintainers by implying that it's easy to check against tile boundaries without building in strong checking that limits the logic to tiled (i.e. 0-pitch/0-bearing) mode? Maybe just add an assert in isInsideTile?
An assert seems good to me but this doesn't worry me too much.
b72dd50 to
ad8cfb3
Compare
- Fixes issue #12461. - Only implement "avoid edges" in MapMode::Tile since it's no longer relevant in Static or Continuous mode. - New: Force "avoid edges" to "true" for line labels, since in tile mode they'll always clip poorly at tile boundaries. - Remove unused "withinPlus0/inside" logic.
ad8cfb3 to
8ff7ac7
Compare


Fixes issue #12461.
In terms of performance, for continuous-rendered maps that don't care, this is an extra boolean-ish check for each symbol at placement time -- should be pretty negligible. For tiled maps, there's an extra check against the tile boundaries but it doesn't involve much extra projection and it might even be a performance win in that it will allow more collision-detection to early-exit near tile boundaries.
Open questions:
avoidEdges: truefor line labels? I'm counting onMapMode::Tilebeing a special case (i.e. api-gl) where this makes sense.isInsideTile?cc @ansis @ian29 @lilykaiser