-
Notifications
You must be signed in to change notification settings - Fork 290
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
Fix label default repeat groups #687
Conversation
Generally speaking, this makes the most sense to me as a default:
I'll need to review the examples a bit more... @bcamper is there a dev URL I can point at to play with this new code against the Mapzen scenes? |
@nvkelso there is a preview build at: And more generally, thanks to Netlify, you can always get a live demo of any branch at: And library build at: 🙌 Agree that top-level layer is a good middle-ground default. |
@nvkelso will you be able to take a look at this soon? I'd like to get this fixed -- and any behavioral changes made -- for upcoming v0.17.0 release. I think this is now the only thing outstanding. Let me know if you need more assistance! |
👀 |
Looking at this location in San Francisco, I see a regression in map labels – more are shown than before in a duplicative way. See Clarification: The new branch looks fine for default map... but not when adding in the bike map overlay when it looks worse. Perhaps turning that on exposes a bug in the "top-level" layer logic? The bike map overlay does modify the priority of labels, so maybe the interplay between these changed in this branch? Like the priority is now honored but not dedup'd? Because all the bike and road labels are grouped inside the top-level roads layer I expected there to be almost no difference between the 2 renders. The roads layer doesn't set a custom repeat group, except for shields. |
I do like the overall balance of labels better in the new branch – less minor road labels that cross major streets. But often there's duplicate major street labels introduced in that process. I think it's okay to knock some of the minor labels out, but not to make room for duplicate labels). Here's zoom 15 in SF, with circle annotations this time! |
OK what you're seeing makes sense, because this branch simply "fixes" the behavior to be the original intent, where sub-layers repeat independently, hence additional duplicates that are from different road layers. I think this just confirms that this is probably not the actual behavior we want. I will update this branch to implement the suggestion to default the repeat logic to the top-level style layer, and then let's compare again. |
(e.g. all labels under `roads` layer, including sub-layers, are in one repeat group)
3d50074
to
e3faf62
Compare
Alright, this branch is updated so that the @nvkelso feel free to double-check if inclined; from my Differ tests, all screens are restored to the same labels as pre-this-branch (allowing that there is some randomness in which labels "win" on page load). |
New setup looks good, ship it! |
This fixes an issue with label repeat group logic. The intended behavior is for labels to apply repeat rules within their unique scene layer group. For example, labels for a major and minor road with the same name will be allowed to repeat near each other. This was intended to catch uncommon but still existent cases where labels of different feature classes are desirable to show.
However, now that the bug is fixed, I notice that there are some suboptimal cases in Mapzen styles where extra labels are being shown nearby to each other and a bit crowded -- often 2 but sometimes even 3 -- because they are different road types, including ramps, etc. See the extra Market St and Park Row labels below:
This can be addressed in the style by putting all of the road labels into one
repeat_group
, or at least into fewer, to consolidate them. That's what I've done in the default demo here as an example.This leads me to wonder if we should consider changing the default instead. A couple proposals:
repeat_group
by default (this is what the bug was doing!)roads
, instead of getting as granular as each sub-layer). This would still allow cases like roads and parks with the same name to repeat.In either case, the default could still be overridden with a custom
repeat_group
.@nvkelso please review the above and give your thoughts on restoring the original behavior vs. using a new default.