-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(webgl): normalize nearly identical vertices before tessellation #8221
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
base: dev-2.0
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -693,4 +693,35 @@ visualSuite('WebGL', function() { | |
| screenshot(); | ||
| }); | ||
| }); | ||
|
|
||
| visualSuite('Tessellation', function() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new visual test doesn’t actually cover this change. I see the same output before and after the patch. Please update the sketch to reproduce the bug which is fixed at your PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worst case we can just use the exact same tests as in the original issue. I don't fully understand either why this case seems OK but not the one in the issue haha. I wonder if my numbers were bigger in the original and that causes more numerical precision issues There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hello @perminder-17 @davepagurek thanks for the review
Let me know and I'll update the PR accordingly and will also add similar changes to #8204 |
||
| visualTest('Handles nearly identical consecutive vertices', function(p5, screenshot) { | ||
| p5.createCanvas(100, 100, p5.WEBGL); | ||
| p5.pixelDensity(1); | ||
| p5.background(255); | ||
| p5.fill(0); | ||
| p5.noStroke(); | ||
|
|
||
| // Contours with nearly identical consecutive vertices (as can occur with textToContours) | ||
| // Outer contour | ||
| p5.beginShape(); | ||
| p5.vertex(-30, -30, 0); | ||
| p5.vertex(30, -30, 0); | ||
| p5.vertex(30, 30, 0); | ||
| p5.vertex(-30, 30, 0); | ||
|
|
||
| // Inner contour (hole) with nearly identical vertices | ||
| p5.beginContour(); | ||
| p5.vertex(-10, -10, 0); | ||
| p5.vertex(-10, 10, 0); | ||
| // This vertex has x coordinate almost equal to previous (10.00000001 vs 10) | ||
| p5.vertex(10.00000001, 10, 0); | ||
| p5.vertex(10, -10, 0); | ||
| p5.endContour(); | ||
|
|
||
| p5.endShape(p5.CLOSE); | ||
|
|
||
| screenshot(); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason for snapping the z coordinates as well? I don't think we need to do that for z?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not 100% sure how libtess works internally, but maybe we could update my test sketch from the original issue to like, swap the x and z axes, and then draw it with a 90 degree rotation to see if we notice the same issue happening? If so, we probably need to do this for z too, otherwise maybe we don't