Skip to content
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

Don't draw halo pixels "underneath" text pixels #2897

Merged
merged 5 commits into from
Jul 25, 2023

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Jul 21, 2023

This prevents unexpected color blending when labels are partially transparent. This also allows fully transparent text with an opaque halo for an "outline" effect.

We came across the previous behavior at Felt because our UI gives users control over text-color and text-halo-color and allows setting the alpha channel. I think if you have a partially transparent label, you don't expect the of the text to be blended with the halo color.

Here's the previous behavior with transparent text-color and a black halo:
Screenshot 2023-07-21 at 12 37 58 PM

Here's the previous behavior with text-opacity: 0.5 and blue text with a yellow halo -- note how it becomes green:
Screenshot 2023-07-21 at 12 37 35 PM

This adds a tiny amount of extra work to the symbol_sdf fragment shader, but in my experience a little extra math in shaders rarely has measurable perf impact. I don't see anything jump out from the benchmark results:

Screenshot 2023-07-21 at 11 51 01 AM Screenshot 2023-07-21 at 11 45 39 AM Screenshot 2023-07-21 at 11 45 33 AM

cc @makella @ibesora

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

This prevents unexpected color blending when labels are partially transparent.
This also allows fully transparent text with an opaque halo for an "outline" effect.
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Patch coverage: 97.67% and project coverage change: +0.01% 🎉

Comparison is base (fac50bf) 74.48% compared to head (9336919) 74.49%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2897      +/-   ##
==========================================
+ Coverage   74.48%   74.49%   +0.01%     
==========================================
  Files         238      238              
  Lines       19002    19018      +16     
  Branches     4284     4285       +1     
==========================================
+ Hits        14153    14167      +14     
- Misses       4849     4851       +2     
Files Changed Coverage Δ
src/ui/handler_manager.ts 96.47% <50.00%> (ø)
src/geo/transform.ts 97.41% <100.00%> (-0.04%) ⬇️
src/render/terrain.ts 96.04% <100.00%> (+0.36%) ⬆️
src/source/vector_tile_worker_source.ts 96.10% <100.00%> (-0.05%) ⬇️
src/ui/camera.ts 95.14% <100.00%> (+0.01%) ⬆️
src/ui/map.ts 82.92% <100.00%> (-0.04%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisLoer ChrisLoer force-pushed the cloer/transparent_text_body branch from 153d232 to 9336919 Compare July 24, 2023 23:12
@ChrisLoer
Copy link
Contributor Author

Oops, my first take at this didn't pass tests because marking the "inside" of the halo as the edge of the text left a small overlap area where even in the fully opaque case, partially transparent text (from the edge anti-aliasing) could be drawn over a partially transparent inner halo edge, and you'd get some slight unexpected color mixing from beneath the halo. I fixed that by just making the halo wider on the inside by one gamma increment -- one side effect of this is that if you intended to use this intentionally to get an "outline" text effect, you'd probably have to make your font slightly bigger than you'd expect to get the effect you want.

The icon-halo-color/opacity test is I think actually a good example of what was broken before -- in the previous test expectation, the middle of the dot is slightly red, which doesn't really seem like what you'd expect.

@HarelM
Copy link
Collaborator

HarelM commented Jul 25, 2023

Can you please add a change log entry?
Otherwise, this looks good.

@HarelM HarelM merged commit 616e151 into maplibre:main Jul 25, 2023
@wipfli
Copy link
Contributor

wipfli commented Mar 3, 2024

@louwers is there an issue in Native for this?

@wipfli
Copy link
Contributor

wipfli commented Mar 3, 2024

As a rule of thumb I think whenever somebody edits a shader in MapLibre GL JS, we need an issue in Native. And vice versa...

@louwers
Copy link
Collaborator

louwers commented Mar 3, 2024

Some edits to shaders have no discernible changes to rendering output. A better rule of thumb would probably be when someone adds or edits a render test. Even better would be if this was automated somehow.

I'll have a look and create an issue.

@louwers
Copy link
Collaborator

louwers commented Mar 3, 2024

I don't see any green in the picture in the first post. Here is the current behavior of MapLibre Native:

actual

Edit: ah, now I see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants