-
Notifications
You must be signed in to change notification settings - Fork 291
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
Subdivide line labels by segment on overzoomed tiles #277
Conversation
Targeting this for 0.6.2 release. |
@karimnaaji can you take a look please? |
Yep, looking at this in details. |
Logic looks good to me! It's a nice way to handle this. I will add an issue to track this on -es. |
@karimnaaji good catch! I'll check it out. |
|
||
// Build one or more labels for a line geometry | ||
buildLineLabels (size, line, options, labels) { | ||
let subdiv = Math.min(options.subdiv, line.length); |
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.
@karimnaaji I think the issue you found is corrected just by changing this to Math.min(options.subdiv, line.length - 1)
. That avoids a segment with 0
for both start and end, and keeps them evenly spaced. (I keep forgetting that we need to limit to line segments, not points, hence the - 1
.) I think it's more straightforward to keep the different label placements to their own separate segment ranges (rather than letting multiple labels try the whole line range, which can only be less efficient since multiple labels would try the same segment, and only one can win?).
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.
Yep that fixes the issue. I haven't run it but the code path seems ok for that case now.
I think it's more straightforward to keep the different label placements to their own separate segment ranges
That's true it makes sense, and I feel like this would also help to keep the segment spacing more homogeneous.
Subdivide line labels by segment on overzoomed tiles
We've found that our line labelling logic is insufficient for overzoomed tiles, because it only places one label per line, which often provides enough density at each tile's "native" zoom, but is too sparse (sometimes dramatically so) when tiles are overzoomed multiple levels. See #273.
This PR increases line label density by attempting to place multiple labels per line. For each line, up to
2^dz
tiles are placed, wheredz
is the number of levels of overzooming being applied. For example, a z16 tile would still place 1 (2^0) label at z16, but when displayed at z18 the same line would place up to 4 (2^2) labels. This is done with a "coarse subdivide", splitting the line by segment. Each label placement is assigned a range of segments in which it can place.Examples with and without subdivide:
This could likely be improved further, either with more precise (fractional segment) subdivide, and/or by inverting the pattern so that instead of creating multiple
LineLabel
instances per feature, we have oneLineLabel
instance that can emit multiple labels. But this approach was a straightforward and significant near-term improvement, and fits well with the current architecture (we already generate multiple label instances forMulti
geometries). We could also adjust the heuristic to place more labels, but in my testing I found that label repeat distance was usually the limiting factor (we could adjust that default repeat value to increase density if desired).@karimnaaji and I discussed this, some more investigation should be done on the ES side. He thought the same issues might apply, but from a quick glance I am not seeing the same magnitude of label sparsity on device.