Force line label to keep unflipped in defined rotating angle range to suppress flickering #10622
Conversation
|
Going to give this a careful review in just a few minutes, but excited to see a PR for it! 🎉 Regarding whether it needs to be a property or not, my vote would be to hard-code a constant in the vicinity of 3-5 degrees and apply it to all labels. My sense would be that there's probably a number that's small enough that slightly inverted text won't be unpleasant or noticeable, but large enough to dramatically reduce the flipping. Based on testing while driving, I mostly noticed this happening when I was driving down a long straight road and the angle was probably only ~1-2 degrees past vertical. |
|
@rreusser thank you for the comments, I fully agree with you that a small angle range would be enough to mitigate the flickering issue for navigation use cases since the zoom level is relatively high. |
|
After playing with this a bit, I think we might want a slightly different approach. The benefit is that it solves text flipping when text is near vertical, but the downsides are that 1) static maps would show slightly inverted text, and 2) the flipping threshold is shifted to e.g. +5 degrees and is less noticeable since it doesn't coincide with vertical roads, but is not actually removed. Based on slack discussion, maybe an alternate approach is a Schmitt trigger. (Think of a thermostat that has a dead zone near the set point and doesn't quickly flicker on/off when the temperature is near the set point.) This would require storing the upright/flipped state on the symbol, and not update it when it's inside a dead zone (vertical +/- a couple degrees). The downside is needing to store that state somewhere, but the upside is that new maps render correctly so that the static api would be unaffected. @ansis suggested maybe storing this state on |
|
@rreusser thanks for the suggestion! I will dig deeper and try with your proposal. |
|
@rreusser I updated the pr by following the idea of caching the flip stage, but there are 3 failing render tests, I will have to check further for the reasons. |
|
Pardon the delay, @zmiao! I'll give this another look later this morning. |
There was a problem hiding this comment.
This looks great! 😄 Just a couple nits, but nothing significant.
The only extra thing that would be nice is a render test, if possible. The text-max-angle/line-center test was the closest test I could find. Maybe we could use this test as a template and:
- render the map with one of the labels inside the retain range
- wait (or idle?)
- change the bearing until the text is slightly past vertical
- wait
- if this fix works, the text should be slightly upside down
(Oh, I also have never successfully modified the symbol code before, so I don't have 100% confidence that I'm an appropriate final reviewer, just in case this touches some subtleties like single glyphs or CJK or other things symbols handle.)
src/data/array_types.js
Outdated
| @@ -911,6 +914,8 @@ class PlacedSymbolStruct extends Struct { | |||
| get crossTileID() { return this._structArray.uint32[this._pos4 + 10]; } | |||
| set crossTileID(x: number) { this._structArray.uint32[this._pos4 + 10] = x; } | |||
| get associatedIconIndex() { return this._structArray.int16[this._pos2 + 22]; } | |||
| get needsFlipping() { return this._structArray.uint8[this._pos1 + 46]; } | |||
| set needsFlipping(x: number) { this._structArray.uint8[this._pos1 + 46] = x; } | |||
There was a problem hiding this comment.
Maybe rename needsFlipping to flipState: undefined | flipRequired | flipNotRequired? I think either is fine, but on my first time through the symbol code, I got a bit confused whether needsFlip indicated a change or whether it indicated a state.
There was a problem hiding this comment.
We will have to use the uint8 type as {type: 'Uint8', name: 'flipState'} in symbol_attributes.js, so that PlacedSymbolStruct could be regenerated based on this new descriptor.
zmiao
force-pushed
the
zmiao-line-label-flip
branch
from
7172b7f to
f3f926c
Compare
There was a problem hiding this comment.
@zmiao I think this is good to go, but I'll mark this "Request changes" due to release schedule.
If it's alright, let's hold off on merging this until after the beta release we're starting today/Monday. I'm really happy to get this fix in place, but I think it's low-priority for web maps compared to native/navigation, and fog is already plenty of a feature to make sure we don't introduce regressions.
|
@rreusser understood, please feel free to merge it later. |
|
Beta is out the door and looks like this is good to go. Merging. Thanks, @zmiao ! 👏 👏 |
Launch Checklist
Fix: #10423
co-worked with @rreusser.
This pr introduced a field
flipStateforPlacedSymbolStruct, so that the placed line symbol's flipping state could be recorded every time after reprojection. This flip state could be taken as a reference for making further flipping decisions.Besides, the flip state retains range (+/- 5 degrees around the vertical direction) is introduced, within which the flip decision is taken directly from the symbol's
flipState, so that symbol's flipping state could be kept as the same when rotating back and forth around the vertical direction.Visual effects provided by @rreusser:
mapbox-gl-jschangelog:<changelog>When line lablels are inside the flip state retaining range (+/- 5 degrees around the vertical direction), the lables' flip state will be kept the same.</changelog>