-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(webgl): normalize nearly identical vertices before tessellation #8204
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: main
Are you sure you want to change the base?
fix(webgl): normalize nearly identical vertices before tessellation #8204
Conversation
|
🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors! Thank You! |
|
hey @davepagurek, kindly review when you get a chance |
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.
Hi, thanks for taking this on! The approach looks good. I was initially testing in p5 2.x, have we confirmed that this issue exists in 1.x? (Update: this exists in 1.x too, so we can keep this PR into main around, but we'll need another one into the dev-2.0 branch for 2.x.) Regardless, it may be a good idea to start by making the change just in 2.x, in the dev-2.0 branch instead of main.
test/unit/webgl/p5.RendererGL.js
Outdated
| myp5.loadPixels(); | ||
|
|
||
| // Check that center pixels are white (hole cut out properly) | ||
| const centerIdx = (myp5.width / 2 + myp5.height / 2 * myp5.width) * 4; |
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.
Grabbing pixels out of an array can be hard for contributors to debug when something breaks a test in the future, do you think we could add this as a visual test instead?
Converted pixel-checking unit test to visual test as suggested by maintainer. This makes the test easier to debug when issues arise in the future.
Thanks for the review @davepagurek I've converted the pixel-checking unit test to a visual test as you suggested. And also created a separate PR targeting the dev-2.0 branch with the same fixes, since the file structure is different there (ShapeBuilder.js instead of p5.RendererGL.Immediate.js). Here's the link #8221 Thanks |
Resolves #8186
Changes
This PR fixes tessellation artifacts that occur when drawing shapes with multiple contours where consecutive vertices have nearly identical coordinates (differing by ~1e-8 or similar small amounts). This commonly happens when using
font.textToContours().test/unit/visual/cases/webgl.js:Implementation Details
Modified
_tesselateShape()insrc/webgl/p5.RendererGL.Immediate.js:1e-6(epsilon) of each other are normalized to use the exact same valueAdded comprehensive unit test in
test/unit/webgl/p5.RendererGL.js:Technical Background
This workaround addresses a numerical precision issue in libtess, which is a JavaScript port of SGI C code from the 1990s. When consecutive vertices have coordinates that are almost (but not exactly) equal, libtess produces glitchy tessellation output. This issue is particularly likely to occur with contours automatically sampled from fonts via
font.textToContours().Screenshots of the change
Before:
After (with this fix):
PR Checklist
npm run lintpasses