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

Remove "fill-outline-color" #4088

Open
lucaswoj opened this issue Feb 1, 2017 · 10 comments
Open

Remove "fill-outline-color" #4088

lucaswoj opened this issue Feb 1, 2017 · 10 comments
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)

Comments

@lucaswoj
Copy link
Contributor

lucaswoj commented Feb 1, 2017

From @jfirebaugh on January 26, 2015 19:30

As discussed in #199 and #223.

fill-outline-color is currently used in the bright and outdoors styles. @nickidlugash, @peterqliu can you comment on the importance of the property in those styles and in general? Would it be ok to remove for v7 without an immediate replacement via a "polygon" type or "multiple types" feature?

Copied from original issue: mapbox/mapbox-gl-style-spec#240

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

From @peterqliu on January 26, 2015 19:35

👍 causes no breaking changes for me.

Looking forward to polygons

@lucaswoj lucaswoj added the breaking change ⚠️ Requires a backwards-incompatible change to the API label Feb 1, 2017
@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

From @nickidlugash on January 26, 2015 20:18

@jfirebaugh For these styles (and styles based on Mapbox Streets in general), the two features that depend the most on fill-outline-color are water and buildings. For these features, having an outline option is pretty important.

Would it be ok to remove for v7 without an immediate replacement via a "polygon" type or "multiple types" feature?

How long do we think it'll take to implement either a replacement (or optimize the performance of drawing lines enough that creating a second GL layer for the outlines won't kill performance)? If this is something that we're definitely doing soon, we can remove outlines for v7. I will want to manually update the v7 styles though, so I can optimize the way they look sans outline.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

From @jfirebaugh on October 7, 2016 0:37

We really should do this. fill-outline-color adds complex special cases to the renderer for very little value.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

From @nickidlugash on October 7, 2016 14:38

@jfirebaugh we use fill-outline-color in our private styles and public styles for buildings and road polygons. If we decide to remove it, we'll need to coordinate a styles update with the spec update.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

From @mourner on November 7, 2016 22:59

I was initially worried about performance implications of this, but it looks like the only massive layer we use it for is buildings, which only appear on z15+, which is very easy to render compared to heavier zooms like z11-13. So I'm 👍 on this.

@1ec5 1ec5 added the cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) label Feb 3, 2017
@1ec5
Copy link
Contributor

1ec5 commented Mar 10, 2017

fill-outline-color is also used by the shape annotation API in the native SDKs, for functional parity with MapKit’s MKOverlayPathRenderer.strokeColor and the Google Maps SDK’s GMSPolygon.strokeColor. mapbox/mapbox-gl-native#6181 could end up replacing the core annotation API implementation with a style layer–based implementation, but there would be some inconvenience in the fact that a feature collection would be required to represent each polygon overlay.

Beyond the native SDKs, fill-outline-color would also come in handy for mapbox/simplespec-to-gl-style#25 (comment).

@lucaswoj
Copy link
Contributor Author

@1ec5 Our intention after removing this property is to recommend that users use an additional line layer for strokes (or provide some syntactic sugar that effectively creates this line layer).

The existing fill-outline-color is a hack and limited to 1px width.

@1ec5
Copy link
Contributor

1ec5 commented Mar 10, 2017

I think the fact that many (most?) higher-level APIs provide options analogous to fill-stroke-color and fill-stroke-width suggests that we should seriously consider providing built-in support for them as well. But I understand that the current implementation is limited by the OpenGL facility it’s hooking into.

or provide some syntactic sugar that effectively creates this line layer

That sounds good. That’s basically the proposal (at a higher level) in mapbox/mapbox-gl-native#6181 (comment).

@jfirebaugh
Copy link
Contributor

I think the fact that many (most?) higher-level APIs provide options analogous to fill-stroke-color and fill-stroke-width suggests that we should seriously consider providing built-in support for them as well.

See #4087.

@gabimoncha
Copy link

There could also be cases where the fill-stroke isn't needed. For example when showing population density and the zoom level is really small, it can be a bit annoying during zoom transition.
screenshot 2019-02-21 at 16 52 05

For example in this case it's needed just when the zoom level is increased.
screenshot 2019-02-21 at 16 48 41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)
Projects
None yet
Development

No branches or pull requests

4 participants