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

Change units of line-dasharray to pixels #4158

Closed
lucaswoj opened this issue Feb 1, 2017 · 7 comments
Closed

Change units of line-dasharray to pixels #4158

lucaswoj opened this issue Feb 1, 2017 · 7 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 @mollymerp on December 22, 2016 16:45

After discussing with @ansis and @jfirebaugh it seems that the most straight-forward way to make data-driven line-width and line-dasharray possible, would be to change the units of line-dasharray to absolute pixels (from relative units of line-width) because

Dashed lines rely on scaling the dash length by the line width at the previous round zoom level.

This would introduce some complexity in stying lines with line-dasharray because they would need additional zoom functions to stay in sync with line-width to make sure the line-dasharray appears consistent as the map zooms. This seems like a reasonable tradeoff to keeping property function evaluation consistent across all paint properties (we'd need to introduce many exceptions to the current code path if line-dasharray stays a relative value).

Interested to hear thoughts on this! cc @lucaswoj @nickidlugash

Previous discussion on the possible approaches to solving this problem here: #3682

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

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

From @1ec5 on December 22, 2016 19:22

This would also be good for parity with MapKit in the iOS and macOS SDKs. (MapKit’s MKOverlayPathRenderer.lineDashPattern property is measured in screen points.)

@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 December 22, 2016 19:34

@mollymerp sounds like this is the best option.

If we do this, is it possible to also refactor the function behavior of line-dasharray to interpolate at integer zoom levels (similar to the behavior of layout number properties), as opposed to being piecewise? I believe that the reason this wasn't done in the first place is that line dasharrays are rendered like line patterns, so we matched the function behavior of line patterns. However, since we are generating the line-dasharray images in the renderer, would it not be possible to generate a new line-dasharray image at every integer zoom level, based on the interpolated value?

If it is possible to implement this ^, I think it would greatly reduce the burden on designers for matching line-dasharray to line-width, because it would then be possible to match the stops and base values. I think this would also more closely match user expectations for the line-dasharray values (I think it's currently the only number property that is piecewise).

FWIW though, I do personally already write line-dasharray values as functions about 50% of the time, because it is often desirable for the proportions of lines and gaps to change across zoom levels (e.g. it's often desirable for gaps to tighten up as you increase zoom level).

@lbud
Copy link
Contributor

lbud commented May 26, 2017

@anandthakker mentioned the other day that during a sprint earlier this year this was discussed, possibly with the conclusion that we wouldn't have to change units to pixels. @nickidlugash or anyone else — is there more information/discussion that hasn't been captured in this ticket?

@1ec5
Copy link
Contributor

1ec5 commented May 26, 2017

Could we add a new property expressed in pixels and deprecate line-dasharray? (Maybe call the new one line-dash-array? 😈) If both happen to be specified, the new property could take precedence over line-dasharray.

@mollymerp
Copy link
Contributor

@lbud I think we decided we could get around this by creating a new property (as @1ec5 suggests, but maybe not line-dash-array. I think we discussed line-dasharray-pixels or -px) to avoid a breaking change that necessitates style-spec version bump. I don't recall any discussion that we could get around this entirely without a breaking change coming in another form, but I could be misremembering.

@ansis
Copy link
Contributor

ansis commented May 26, 2017

@lbud I think we decided we could get around this by creating a new property (as @1ec5 suggests, but maybe not line-dash-array.

I think the problem is that supporting the current line-dasharray is hard when line-width is data-driven. A new property would help with making dashes data-driven, but wouldn't really help with this other case. Would we disallow any line-dasharray when line-width is data-driven?

@lbud
Copy link
Contributor

lbud commented Jun 1, 2017

Came up with a workaround for this ➡️ #4773. Closing here; will reopen in case of unforeseen pitfalls in that approach.

@lbud lbud closed this as completed Jun 1, 2017
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

5 participants