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

Fixed inner miter point adjustment when on opposite side of corner #6545

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

ShukantPal
Copy link
Member

@codecov-io
Copy link

codecov-io commented Apr 11, 2020

Codecov Report

Merging #6545 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #6545   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files          38       38           
  Lines        1902     1902           
=======================================
  Hits         1567     1567           
  Misses        335      335           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 876b16d...5652e58. Read the comment docs.

@bigtimebuddy
Copy link
Member

bigtimebuddy commented Apr 11, 2020

Better, but seems like the square in the lower-left is busted now:

https://pixijs.io/examples/?v=fix-6543#/graphics/advanced.js
Screen Shot 2020-04-11 at 2 03 03 PM

https://pixijs.io/examples/?v=v5.2.1#/graphics/advanced.js
Screen Shot 2020-04-11 at 2 03 23 PM

Here's a fiddle with just the broken bit:
https://jsfiddle.net/bigtimebuddy/bhsqdm85/

@ShukantPal
Copy link
Member Author

@bigtimebuddy Was the square busted before this PR too? Or is it because of this change?

@bigtimebuddy
Copy link
Member

bigtimebuddy commented Apr 11, 2020

Looks like because of this change.
https://pixijs.io/examples/?v=dev#/graphics/advanced.js

@bigtimebuddy
Copy link
Member

@SukantPal any ideas?

@ShukantPal
Copy link
Member Author

@bigtimebuddy I know where the problem is coming from. The math is incorrect. I’ll get this sorted tomorrow. It is because of the code where I prevent the inner miter point from being on the opposite side of the corner.

@ShukantPal
Copy link
Member Author

@bigtimebuddy I also want to present a formal (mathematical) proof that my fix will work and why.

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Apr 12, 2020

Proofs are good things, but please do make refs , where those formulas exist in other renderers.

Nothing bad in peeking at other people stuff to check for possible mistakes and places where you are actually smarter ;) Refs are also important for math articles!

@ShukantPal
Copy link
Member Author

@ivanpopelyshev I'll try.

@ShukantPal
Copy link
Member Author

@bigtimebuddy Why are the tests failing in CI? Something related to isMobile & passing on my machine.

@ShukantPal
Copy link
Member Author

Okay, this bug was related to the code that "prevented the inner miter point from being on the opposite side of the corner". By doing so, you eliminate a rare class of bugs that arise at high line widths. This animation will show you what happens when the width gets to large eventually: https://jsfiddle.net/ShukantPal/fhkd2r85/. Specifically, focus on the "inner" point of the joint. It rapidly rises up, as you would expect, however, it overshoots the first & last points, looking like this:

Screen Shot 2020-04-13 at 6 59 55 PM

Let me take an example in a Cartesian plane (no y-inversion): connect the points (0, 0), (10, 100), (20, 0) with a variable line width.

  • At width = 10, you will find that the inner edges will intersect at (10, 0).

  • Increasing the width from 10 will result the inner edges meeting at a y-value below 0. This means on the opposite side of the joint (the joint is (10, 100) on +ve y side).

I have setup a guide showing the math behind this here: https://www.desmos.com/calculator/gtyqjkoad6

@ShukantPal
Copy link
Member Author

To prevent the inner miter joint from going onto the other side, we can take check for "same-sidedness" of the it and the joint (p1) w.r.t the line joining p0 & p2. This guide explains how that works:

https://www.desmos.com/calculator/zwdrci6rwh

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

@bigtimebuddy bigtimebuddy merged commit db7e298 into dev Apr 14, 2020
@bigtimebuddy bigtimebuddy deleted the fix-6543 branch April 14, 2020 01:57
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.

None yet

4 participants