-
-
Notifications
You must be signed in to change notification settings - Fork 689
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
Symbols: Allow text line breaks on lines #4124
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4124 +/- ##
==========================================
- Coverage 86.88% 85.08% -1.80%
==========================================
Files 242 242
Lines 33044 33040 -4
Branches 2011 1980 -31
==========================================
- Hits 28709 28111 -598
- Misses 3374 3856 +482
- Partials 961 1073 +112 ☔ View full report in Codecov by Sentry. |
Thanks for taking the time to open this PR! |
The history is a little interesting here. A text-max-width of 0 used to be treated as a special value that meant "absolutely no line breaks." This also applied to explicit newlines like in the current issue. This was changed for 'point' placements in order to allow the explicit newlines. The reason 'point' was singled out is because text-max-width only applies to 'point' placements and is set to 0 otherwise (https://github.com/maplibre/maplibre-gl-js/blob/main/src/symbol/symbol_layout.ts#L159). Going back a bit further, text-max-width applied to everything but 'line' placements. This seems to have to changed when 'line-center' was introduced (I think it was just 'point' and 'line' before that). This behavior seems to go back to the beginning. Similarly, the "text-max-width of 0 means no line breaks" goes back pretty far. This was part of a very intentional change to make it so text didn't wrap on 'line' placements. It's possible disabling the line wrapping instead of setting the max width to infinity was preferred for performance reasons? Will need to run the benchmarks. Given all that, I believe the best solution to solve the current issue and maintain most of the previous visual behavior is:
The style-spec should also probably be changed to specify that 'text-max-width' does not apply to 'line' and 'line-center' symbol-placements...? |
Benchmark results for a8c630e: all.pdf Don't see anything that looks terribly amiss. I wouldn't expect any major performance issues with this issue. The infinity gets resolved to a |
Thanks for looking into this! |
Thanks for opening this pull request. I found the behavior around newlines strange in the past and here you describe well what the situation is. This pull request will make existing maps look different which is something we should avoid. Is it possible to introduce an new style spec property to take account of the line breaks? |
@wipfli I think we should also avoid complicating the style due to old bugs or missing implementation. |
a8c630e
to
f4e68dc
Compare
What will happen if the line is not straight? Like when adding a text to a river? What will happen in this scenario when adding a new line? |
Since this is a breaking change I suggest we go into pre-release mode of MapLibre GL JS v5. |
@HarelM can you open a js-parity issue in MapLibre Native for this change? |
@louwers, I'm delegation opening the feature parity issue to you. |
@wipfli @HarelM Created issue and added it to the Render Test Parity Status Report |
Resolves #3814
The change is pretty simple. The text shaper was explicitly disallowing any text breaks if the 'symbol-placement' was not set to 'point'. This removes that logic, allowing text breaks to be processed for any 'symbol-placement' value. The other bits are just removing the now unused variable from calling functions.
No new unit test as the text shaper unit test is sufficient now that it's not specifically avoiding the problem.
The only pain point in my mind is that someone may be relying on this behavior for some reason. There doesn't appear to be any property on symbol layers or any expressions that could be used to quickly change back to the previous behavior. Though I'd argue if you don't want explicit line breaks in your data, don't add explicit line breaks in your data?
For non-explicit line breaks, setting 'text-max-width' to a large number should get back to the previous behavior.
Before (string is "A\nBB\nCCC"):
After (string is "A\nBB\nCCC"):
Launch Checklist
CHANGELOG.md
under the## main
section.