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 join artifacts with 180º or very sharp angles #4008

Merged
merged 5 commits into from
Jan 18, 2017
Merged

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Jan 18, 2017

This fixes two separate (but in appearance seemingly related) 🐛s with line joins:

  • Miter line joins with 180º angles were evaluated to have (NaN, NaN) normal join vectors (e.g. a (1, 0) unit vector and a (-1, 0) unit vector add to (0, 0), whose unit vector is (NaN, NaN)). Because they weren't calculating extrusion vectors correctly these resulted in artifacts like
    image
    after:
    image

  • Miter and bevel line joins with very sharp angles and miter joins are evaluated via the "flipbevel" path: since the angle is so acute, the miter would be far too big, so we bevel it off instead. However, the flipbevel path had a normal vector flipped, so the join's vertices were added in the opposite order expected for indexing and they were creating artifacts such as
    image
    (Note: this explanation may not visually make sense, because with very sharp angles we add an extra vertex near the join (something about dashed lines; if you remove that vertex and ⏬ opacity you can see the incorrect triangle indexing:)
    image
    after:
    image

This also includes some comment spellchecking 😬

I've added two lines to all of the line-join tests in the render suite: one that turns 180º, and one that turns very acutely. These now produce the following results (previously these tests only had the left three lines):

test master line-artifacts
bevel bevel-master bevel-branch
bevel-transparent bevel-tr-master bevel-tr-branch
default default-master default-branch
miter miter-master miter-branch
miter-transparent miter-tr-master miter-tr-branch
round round-master round-branch
round-transparent round-tr-master round-tr-branch

(Turns out this bug wasn't actually ticketed 😬)

I'd like to get into #3809 / #2316 next (these are more or less the same issue, just applied to different join types), after I wrap up some other outstanding PRs — I dug just enough to know that those are pretty unrelated to ☝️.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality

@lbud
Copy link
Contributor Author

lbud commented Jan 18, 2017

Currently checking to see whether these affect gl-native too, since I've modified render tests here.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

👌

@lbud
Copy link
Contributor Author

lbud commented Jan 18, 2017

This does indeed need a gl-native parity PR: ticketed at mapbox/mapbox-gl-native#7778 and added ignores to line-join render tests.

@lbud lbud merged commit 1a4bd37 into master Jan 18, 2017
@lbud lbud deleted the line-artifacts branch January 18, 2017 23:28
@aaronlidman
Copy link

🙇

@lbud
Copy link
Contributor Author

lbud commented Jan 18, 2017

@aaronlidman sadly though this isn't the same as the line-offset issue you were seeing ☹️ going to hit that after I clear a few outstanding gl-js PRs off my plate.

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.

3 participants