Fix line-pattern artifacts under overscaling and underscaling

[karimnaaji]

This fixes overscaling/underscaling of line pattern as well as render differences between different physical pixel ratio sprites when used as line patterns. Refer mapbox/mapbox-gl-native#15472

Visual differences

The behavior is changed to match the following:

1. When the line pattern fits on a line smaller than its own resolution, we downscale while keeping the aspect ratio at integer zoom level
2. When the line pattern fits on a line larger than its own resolution, we upscale while keeping the aspect ratio at integer zoom level
3. When the line pattern has different pixel ratio attributes (1x, 2x, 3x...), the pattern screenspace resolution should remain stable (e.g. the physical resolution shouldn't impact the size at which the pattern is rendered but instead increase the quality of the visual while keeping its logical resolution)

The current behavior does not account for 1. and 3.

Before

underscale-2x@2x

underscale-3x@2x

overscale-1x@2x

overscale-2x@2x

After

underscale-2x@2x

underscale-3x@2x

overscale-1x@2x

overscale-2x@2x

Shader differences

This will vary per-platform/shader compiler but gives an idea of the difference.
Try to move for more mad instructions instead of div. Per-instruction count differences in shader line_pattern.fragment.glsl :

before: 6 mad, 12 div, 12 add, 9 mul
after: 13 mad, 8 div, 6 add, 9 mul

This increases the number of mad instructions (multiply-add) for fewer cycles and decreases the number of divisions and additions.

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• manually test the debug page
• tagged @mapbox/studio and @mapbox/map-design-team if this PR includes style spec or visual changes
• tagged @mapbox/gl-native there are shader changes that will require porting to native
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Improve scaling of patterns used in line-pattern on all device resolutions and pixel ratios</changelog>

[ansis]

Line width can change with zoom. Now that the rendered width of the pattern is scaled by the line width, the width of the pattern can change with zoom. This creates the marching ants effect (see the show-marching-ants branch and zoom in slowly).

Dashed lines had a similar problem and this was fixed but it's pretty hacky. We added an internal pseudo-paint-property called floorwidth:

mapbox-gl-js/src/shaders/line_sdf.vertex.glsl

Line 46 in 2a0ae8e

#pragma mapbox: initialize lowp float floorwidth

mapbox-gl-js/src/style/style_layer/line_style_layer.js

Lines 75 to 76 in 2a0ae8e

	(this.paint._values: any)['line-floorwidth'] =
	lineFloorwidthProperty.possiblyEvaluate(this._transitioningPaint._values['line-width'].value, parameters);

I think you should be able to copy the floorwidth pragmas from line_sdf.vertex.glsl and use that value instead. I think it might not play nicely with the crossfading but that's a mess anyways.

1. When the line pattern fits on a line smaller than its own resolution, we downscale while keeping the aspect ratio at integer zoom level

The new behavior seems good here but is this a breaking change? I'm not sure if the existing behavior is intentional but someone could be depending on it. I can kind of understand how some patterns may be clipped and still look intentional. I think we could probably get away with changing this and considering it a fix instead of a breaking change.

[karimnaaji]

@ansis good catch! That makes sense to use floorwidth value instead of the dynamic width, and it does indeed fix the marching ants issue.

The new behavior seems good here but is this a breaking change? I'm not sure if the existing behavior is intentional but someone could be depending on it. I can kind of understand how some patterns may be clipped and still look intentional. I think we could probably get away with changing this and considering it a fix instead of a breaking change.

Thanks for bringing that up because it's something that could be concerning.

Someone could depend on that in some particular situation, the most accidental use would be if it gave this result and the user worked around/with it. Do you know whether the stretching and/or clipping were considered when this was introduced?

[ansis]

the most accidental use would be if it gave this result and the user worked around/with it.

Yep, most likely.

Do you know whether the stretching and/or clipping were considered when this was introduced?

I think I may have introduced this a long time ago but it probably wasn't carefully considered and it may have been before we had people actually using this. It was also never documented. I think I was also designing this for things that were more "textures" instead of "patterns", for example a rough paper surface or pencil shading where clipping is ok and scaling could be problematic. But I'm not sure I've really seen people using it like this in practice. And I have seen a lot of people surprised by the behavior.

I think we should make this change. I'm not sure whether it should be considered breaking. I think it's right on the fence. If we do consider it breaking we should probably wait until we have other breaking changes we need to make.

[chloekraw]

@karimnaaji great work on this and thanks for the thorough PR description. I learned quite a bit just from reading it!

Considering whether this change is breaking is a good idea, and we should call it out at the top of the changelog as a softly breaking change with a link to the PR description. Maybe we’ll get feedback in the beta that makes us think twice about merging.

In general, I think it’s pretty unlikely that a designer who has created a pattern wants that pattern to visually look different at any point. Designers tend to want things to look pixel perfect exactly in the way that they’ve designed it, positive and negative space included. Else, they would intentionally design the variant as a separate pattern and use that at the zoom levels they would like that pattern to appear. I think it’s more likely that some customers don’t mind the current behavior as opposed to intentionally wanting it and expecting it in their design.

The feedback in these tickets also make me think that this PR would be a welcome change.

• #8752
• #6296

[karimnaaji]

@ansis I've tested this on native and haven't noticed any issues on my manual testing mapbox/mapbox-gl-native@0572a16. Let me know if there would be any other concerns you'd like me to address and otherwise we could call this a softly breaking change to prevent any surprise on the user's end.

[karimnaaji]

To recap some of the discussions we have two options:

• Keep this change as is and communicate it as a bug fix with visual breaking change, requiring changes to assets in the following case:
  • The asset is intended to be displayed as a surface pattern, such as in this stylesheet and any line has a line width smaller than the pattern it tries to fit to, resulting in automatic visual differences: before vs after
  • Advantage: we fix this type or this type of clipping issue, deal better with different pixel ratio
  • Disavantage: we might cut off some valid use cases, introducing a requirement to revisit some assets in the use case previously described
• Introduce a boolean toggle in the stylespec for line-pattern, defaulting to the current behavior, otherwise to this new behavior.
  • Advantage: we support more use cases and we are backward compatible, not introducing potential regressions and carry all the advantages of the breaking change option
  • Disavantage: we introduce new stylespec information

[karimnaaji]

@ansis, do you have any further thoughts? I have gathered some concerns from the map teams but the visual impact would be on a very few number of internal styles and, to their knowledge, none of external styles; where in such case we would communicate how to get the initial behavior back. But I'd like to discuss our options to move this forward, especially the second one as it would be the least disruptive for the user.

[chloekraw]

• The asset is intended to be displayed as a surface pattern, such as in this stylesheet and any line has a line width smaller than the pattern it tries to fit to, resulting in automatic visual differences: before vs after

I'm not sure if it's just me, but I can barely see the difference between the before and after jsfiddles when I zoom. 🤔 Is scaling problematic if you add a rough paper surface or pencil shading texture to a line? If we have a visual example of that, that would be compelling to me.

Introduce a boolean toggle in the stylespec for line-pattern, defaulting to the current behavior, otherwise to this new behavior.

I have a slight preference for not introducing an option but I can get behind it. So the change would be making line-pattern accept an optional second argument where you specify whether the pattern is clipped, and by default it's true?

Would it be better to make the developer specify clip or scale, so that we leave open the possibility of supporting some other option in the future?

[karimnaaji]

Is scaling problematic if you add a rough paper surface or pencil shading texture to a line? If we have a visual example of that, that would be compelling to me.

When using a texture for use as a surface, it scales to fit, it might be desired or undesired depending on the use case.

So the change would be making line-pattern accept an optional second argument where you specify whether the pattern is clipped, and by default it's true?
Would it be better to make the developer specify clip or scale, so that we leave open the possibility of supporting some other option in the future?

Yes to both of these questions, I think we generally are missing control over how we would like textures to fit. So having a few texture mapping parameters to control how this works in both 2d and 3d is probably something we should consider, such as a pattern-mapping option with a few different parameters expressing whether it should scale to fit while keeping the aspect ratio, or remain at constant resolution.

[ansis]

Thanks for all the back and forth on this! Looks good

[chloekraw]

@karimnaaji I have a suggestion for modifying this changelog entry from

<changelog>Fix line-pattern artifacts under overscaling and underscaling</changelog>

to

<changelog>Improve scaling of patterns used in line-pattern on all device resolutions and pixel ratios</changelog>

[karimnaaji]

@chloekraw updated!