Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Re-implement "avoid edges" for MapMode::Tile #12520

Merged
merged 1 commit into from
Aug 20, 2018
Merged

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Jul 31, 2018

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.

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:

  • 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.
  • 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.
  • Am I right that the "withinPlus0" logic still doesn't do anything even with this re-implemented?
  • 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?

cc @ansis @ian29 @lilykaiser

@ChrisLoer ChrisLoer requested a review from ansis July 31, 2018 19:57
@ChrisLoer
Copy link
Contributor Author

Before
screenshot 2018-07-31 12 58 43

After
screenshot 2018-07-31 12 57 56

@ChrisLoer
Copy link
Contributor Author

Tests are failing because we need ignores or native port for mapbox/mapbox-gl-js#6961

@ChrisLoer ChrisLoer added the Node.js node-mapbox-gl-native label Jul 31, 2018
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

 - 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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Node.js node-mapbox-gl-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants