Incorrect rendering with line-dasharray + data-driven line-width

[jfirebaugh]

Dashed lines rely on scaling the dash length by the line width at the previous round zoom level. Thought this code appears to respect data-driven properties because it has a featureProperties parameter, in fact this branch is evaluated only in contexts where this parameter is undefined, namely setting up paint['line-dasharray'] for use here. (In effect a special-case of #3044.)

This means that layers which combine line-dasharray with a data-driven line-width will not render correctly.

cc @mollymerp @lucaswoj

[jfirebaugh]

@ansis Can you think of any way to fix this besides adding an extra uniform/attribute that stores "line width at the previous round zoom level", and then doing the calculation in the shader? It would be a uniform if the line-width is not data-driven and a vertex attribute if it is.

[ansis]

We could multiply by the line width at the tile's zoom level. This could result in more stretching/squishing for overscaled/underscaled tiles but I think it could be fine. This value doesn't change with zooming so it could be baked into a single attribute.

[jfirebaugh]

@lucaswoj @mollymerp Should we roll-back data-driven line-width until we can find a solution to this issue?

[mollymerp]

I don't think we should roll back -- I think DDS for line-width without line-dasharray is a feature people will appreciate. Can we mention in the style spec that property functions are not supported with dash-array at this time?

[jfirebaugh]

I'm really reluctant to get into the business of maintaining caveats like "property A supports data-driven functions, except not in combination with property B". That produces an API that where user expectations are confounded seemingly at random: a frustrating user experience.

I think we need to fix this before shipping data-driven line-width, or roll it back.

[jfirebaugh]

BTW, per the test-case added in mapbox/mapbox-gl-test-suite@1e04807 "incorrect" in this case means "nothing is rendered".

[mollymerp]

@ansis do you have any time tomorrow to talk about what a fix to this would entail? Happy to put the time in to get this working properly.

@jfirebaugh is it ok with you if we keep the original changes in master but refrain from listing the line properties as supported by property functions in the style spec until everything is working correctly?

[ansis]

@mollymerp sure

[mollymerp]

After chatting w @ansis and @jfirebaugh it has become apparent that implementing this correctly will require either a breaking change to the style spec (converting line-dasharray units to pixels instead of relative values), adding many exceptions to the current way property functions are evaluated, or (from @ansis):

Add a "line-dasharray-line-width" internal ghost property that would be derived from "line-width" and handled-completely separately all the way through the shader. On the -js side you'd add a fake spec property and add a if (name === 'line-width') setPaintProperty(layer, 'line-dasharray-line-width', transformSomehow(value)) within setPaintProperty. transformSomehow would convert the width function to a width-at-integer-zoom-level function.
Basically adding some way to make a transformed copy of a spec property that is never exposed to users
(properties that are automatically derived from other properties)

For now I will roll back DDS for line-width and line-dasharray, and later follow up with proposal/discussion around how we want to proceed.

[ansis]

I still think breaking the style spec is the way to go, but I'll try to refine my suggestion in the previous comment:

Make line-width always pass two functions to the shader: line-width and line-integer-zoom-width. Both would always be added to the buffers and evaluated but the result from the second would only be used when line-dasharray is present.

[jfirebaugh]

Removing release-blocker label since we've rolled back data-driven line-width for the time being.

[mollymerp]

Tracking discussion for the breaking style-spec change here: mapbox/mapbox-gl-style-spec#633.

[jfirebaugh]

Thanks @mollymerp! Let's move discussion there -- reading this bug can be kind of confusing now that the original issue no longer exists.