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 line dash color handling #742

Merged
merged 2 commits into from
Feb 5, 2020
Merged

Fix line dash color handling #742

merged 2 commits into from
Feb 5, 2020

Conversation

bcamper
Copy link
Member

@bcamper bcamper commented Jan 12, 2020

This PR revamps the logic for applying line dash patterns and textures. The prior logic had some holes and inconsistencies, including #741. It implements the following logic in the fragment shader for lines (and also replaces some conditional if/else shader branching with nested mix functions):

  • A line can have an active texture sampler in two cases:
    • Implicitly, through a dash pattern (the texture is automatically generated from the dash pattern).
      • The dash pattern defines a foreground color (via the regular color parameter, available in the shader as the vertex color) and background color (via the dash_background_color parameter, available in the fragment shader as u_dash_background_color).
      • The dash texture is a monochrome representation of the pattern, with opaque white for the foreground and transparent black for the background. By using a monochrome texture, the same texture can be reused for the same dash pattern with different foreground/background color combinations.
    • Explicitly, assigned via the texture parameter (in style or draw group).
      • The color values in the texture are meant to be used directly (similar to other style texture sampling behaviors).
  • If a line has a dash texture, then in the fragment shader:
    • A uniform u_has_dash indicates this, with a float value of 0 or 1.
    • We determine whether the fragment is in the foreground or background of the dash pattern, and assign the corresponding color, using a mix function controlled by the line texture alpha channel (following the monochrome texture described above).
      • mix(u_dash_background_color, color, _line_color.a), // choose dash foreground or background color
  • If the line has an explicit texture, then in the fragment shader:
    • Apply the vertex color to the texture color ("tinting" it), consistent with other similar behaviors, e.g. raster textures on polygons.
      • color * _line_color, // no dash: tint the line texture with the vertex color
  • The determination of which texture type (dash pattern or explicit texture) to use is accomplished through another mix function, controlled by the u_has_dash uniform described above.
  • Finally, for alpha/blending behavior:
    • When opaque blending is active, a simple "alpha cutout" behavior is applied, in which any alpha pixel with an alpha of less than 0.5 (TANGRAM_ALPHA_TEST) is discarded. This enables basic transparency-like behavior for dashes and textures when using opaque blending.
    • When other blend modes are active, no special logic is applied, and the alpha value resulting from the above color logic will be used as-is.

- alpha of background should not affect foreground
- more consistent handling of alpha cutout behavior for opaque blending
@bcamper
Copy link
Member Author

bcamper commented Jan 12, 2020

@matteblair It looks like the same issue described in #741 also exists in Tangram ES (because it looks like it more or less copies the previously faulty Tangram JS logic... 😅). I took a couple stabs at fixing it, and found some other edge cases (the alpha cutout test was too early, causing inconsistent alpha handling depending on texture + blend mode combinations), and found it easier to just revamp to the logic in this PR, which hopefully is clearer (but using nested mix which isn't always intuitive either). Please give it a look and let me know if it makes sense!

We should open a corresponding issue to #741 for ES as well.

@bcamper
Copy link
Member Author

bcamper commented Jan 12, 2020

Actually, I don't think Tangram ES suffers from the same specific behavior reported in #741, current logic here:

https://github.com/tangrams/tangram-es/blob/master/core/shaders/polyline.fs#L67-L82

But, I still think it has related issues, particularly with the order of the alpha cutout test vs. the tinting (vertex color multiplied by line texture color), and would benefit from a review in conjunction with this PR.

@bcamper
Copy link
Member Author

bcamper commented Jan 24, 2020

Hey @matteblair, any further thoughts here? I'd like to cut a minor release with @meetar's geojson-vt upgrade. I tried to be complete with the logic description in the PR, but the actual shader code is pretty concise ;)

Copy link
Member

@matteblair matteblair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I checked this out in Tangram ES too and there's a similar problem, so I applied the same logic implemented here (see tangrams/tangram-es#2137, though there are minor differences because Tangram ES still only has dash defined per-style).

@bcamper bcamper merged commit 1d2f73d into master Feb 5, 2020
@bcamper bcamper deleted the dash-color-fix branch February 5, 2020 18:58
@bcamper bcamper added this to the v0.20.1 milestone Feb 16, 2020
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

Successfully merging this pull request may close these issues.

2 participants