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

Fix double drawing on fill outlines #2757

Closed
lucaswoj opened this issue Jun 16, 2016 · 7 comments
Closed

Fix double drawing on fill outlines #2757

lucaswoj opened this issue Jun 16, 2016 · 7 comments

Comments

@lucaswoj
Copy link
Contributor

lucaswoj commented Jun 16, 2016

If you draw a semi-transparent fill layer, the outline is drawn twice, resulting in a outline that is twice the desired opacity and a mix of the fill color and the outline color.

See the demo at http://jsbin.com/peraxujibo/6/edit?html,output

I'm not yet sure if this behaviour is new or intentional.

cc @samanpwbb

@lucaswoj lucaswoj changed the title Fix double drawing on fill borders Fix double drawing on fill outlines Jun 16, 2016
@samanpwbb
Copy link
Contributor

samanpwbb commented Jun 20, 2016

Version 19+ and version 18 handle transparency really differently:

Started in version 19.0 – http://codepen.io/samanbb/pen/aZmRdx
Version 18.0 – http://codepen.io/samanbb/pen/vKXVGL

The older behavior is the expected behavior. Almost all styles except those designed in the last few weeks will assume transparency renders like in the bottom example.

@samanpwbb
Copy link
Contributor

There was some discussion about how to "correctly" handle opacity in the style spec a while ago: mapbox/mapbox-gl-style-spec#273

I proposed:

What if setting RGBA fill/stroke was per-feature while setting Opacity was per layer?

Seems like the recent change was accidental. Ideally we have both per-feature and per-layer opacity, and the way this works is clearly defined in the style spec.

Short term, I would love to see the recent change to opacity-* rules reverted so styles designed before v19 look the way the designer wanted them to look.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Jun 27, 2016

Thanks for the clarification @samanpwbb. I misunderstood the bug before.

This change in behaviour is due to us merging earcut #1606.

Ideally we have both per-feature and per-layer opacity, and the way this works is clearly defined in the style spec.

As of GL JS v0.19.0 every style property is defined per-feature. Pre-earcut fill style properties were the only exception to this rule. As far as I know, all style properties in GL Native have always been per-feature. (Can you confirm, @jfirebaugh?).

It is possible that GL Native has always been rendering translucent polygons like this and only now is GL JS rendering them consistently.

I will open a ticket about supporting per-feature and per-layer style properties in mapbox-gl-style-spec.

Short term, I would love to see the recent change to opacity-* rules reverted so styles designed before v19 look the way the designer wanted them to look.

We can revert the behaviour if

  • the new GL JS behavior is inconsistent with GL Native behaviour
  • the GL and carto teams agree that the new GL JS behaviour is undesirable
  • there exists a performant way to restore pre-earcut behaviour with earcut

Next Steps

  • add mapbox-gl-test-suite test for overlapping translucent polygons
  • decide whether or not we need to revert the new behaviour (then do so) we should not
  • create a ticket in mapbox-gl-style-spec to discuss what per-feature / per-layer style properties would look like (already existed: Add a per-layer opacity property mapbox-gl-style-spec#273)
  • update Mapbox styles and do a new release to mitigate the rendering change

@lucaswoj
Copy link
Contributor Author

img_1509

screen shot 2016-06-27 at 4 28 21 pm

I have confirmed that GL Native renders polygon opacity per-feature, like GL JS as of v0.19.0.

This leads me to believe we should keep the existing per-feature opacity rendering and adjust our default styles to work on that assumption.

@samanpwbb
Copy link
Contributor

one issue:

if fill is semitransparent, it is now impossible to use antialiasing without a noticeable stroke. Do you see any way around that?

There will continue toge many cases where per-layer rather than per-feature styling is desirable, will wait for that discussion in the style spec repo though.

@jfirebaugh
Copy link
Contributor

Original issue discussing rendering behavior for overlapping semi-transparent fills is #859. As noted there, the behavior prior to earcut was considered a bug.

@samanpwbb, can you provide a minimized test case for the issue with antialiasing?

@lucaswoj
Copy link
Contributor Author

All of the "Next Steps" outlined in this ticket have been 🚢ed!

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

No branches or pull requests

3 participants