Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.7k
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 ourterms of service andprivacy 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?
Conversation
src/webgl/ShapeBuilder.js Outdated
| if(Math.abs(currZ-prevZ)<epsilon){ | ||
| contour[i+2]=prevZ; | ||
| } |
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
| }); | ||
| }); | ||
| visualSuite('Tessellation',function(){ |
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.
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 comment
The 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 comment
The 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
Hello@perminder-17@davepagurek thanks for the review
As you said that Visual test not showing the bug
Should I use the exacttextToContours()example from issue[p5.js 2.0 Bug Report]: Tessellation of beginShape/endShape with multiple contours sometimes produces weird results #8186?
That one clearly shows the glitchy tessellation before the fix. I can add that as the visual test.Also about Z coordinates
I included Z because I thought it would be safer to normalize all three axes consistently. But you're right that the original issue only mentions X/Y coordinates. Should I:- Test if the issue happens with Z as Dave suggested (rotate the shape)?
- Or remove Z normalization to keep the fix minimal?
Let me know and I'll update the PR accordingly and will also add similar changes to#8204
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.
As you said that Visual test not showing the bug
Should I use the exact textToContours() example from issue#8186?
That one clearly shows the glitchy tessellation before the fix. I can add that as the visual test.
For this one, maybe yes. you could add that in this scenario.
Test if the issue happens with Z as Dave suggested (rotate the shape)?
for this one, I tested it and I don't think we need to snap z coordinates.
perminder-17 left a comment
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.
Sorry, I missed@davepagurek comments. I don't think we need to snap the z changes here.https://editor.p5js.org/aman12345/sketches/eS1fDUT-w
- Remove Z-axis normalization (only X/Y needed based on testing)- Update visual test to use textToContours() example from issueprocessing#8186- Test now reproduces the actual bug that gets fixedResolvesprocessing#8186
avinxshKD commentedNov 24, 2025
Hey@davepagurek@perminder-17, made the necessary changes. Please review whenever you get a chance. |
| p5.createCanvas(200,200,p5.WEBGL); | ||
| p5.background(255); | ||
| p5.fill(0); | ||
| p5.noStroke(); | ||
| constfont=awaitp5.loadFont('/unit/assets/Inconsolata-Bold.ttf'); | ||
| constcontours=font.textToContours('p',0,0,60); | ||
| if(contours&&contours.length>0){ | ||
| p5.translate(-p5.width/4,-p5.height/4); | ||
| p5.beginShape(); | ||
| for(letcontourIdx=0;contourIdx<contours.length;contourIdx++){ | ||
| constcontour=contours[contourIdx]; | ||
| if(contourIdx>0){ | ||
| p5.beginContour(); | ||
| } | ||
| for(leti=0;i<contour.length;i++){ | ||
| p5.vertex(contour[i].x,contour[i].y,0); |
perminder-17Nov 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 think this is still not reproducing the issue for which you're adding the visual tests.
Please use this sketch for the tests:
functionsetup(){createCanvas(400,400,WEBGL);}functiondraw(){constcontours=[[[-3.8642425537109375,-6.120738636363637,0],[3.2025188099254267,-6.120738636363637,0],[3.2025188099254267,-4.345170454545455,0],[-3.8642425537109375,-4.345170454545455,0],[-3.8642425537109375,-6.120738636363637,0],],[[-1.8045834628018462,4.177556818181818,0],[-1.8045834628018462,-9.387784090909093,0],[0.29058699174360836,-9.387784090909093,0],[[0.2905869917436083,3.609374411367136,0]]// doesnt work[0.31044303036623855,4.068235883781435,0],[0.38522861430307975,4.522728865422799,0],[0.548044378107245,4.941051136363637,0],[0.8364672032828204,5.2932224887960775,0],[1.2227602871981542,5.526988636363637,0],[1.6572258237923885,5.634502949876295,0],[2.101666537198154,5.669034090909091,0],[2.6695604948237173,5.633568761673102,0],[3.0249619917436084,5.5625,0],[3.4510983553799726,7.4446022727272725,0],[2.8568950819856695,7.613138889205699,0],[2.3751340936529037,7.676962586830456,0],[1.8892600236717598,7.693181792704519,0],[1.2922705720786674,7.649533731133848,0],[0.7080836288276859,7.519788939617751,0],[0.14854153719815422,7.311434659090909,0],[-0.38643934048179873,7.00959666478984,0],[-0.858113258144025,6.61653855366859,0],[-1.25415732643821,6.1484375,0],[-1.5108595282965422,5.697682732328092,0],[-1.6824918355513252,5.207533878495854,0],[-1.7762971052870198,4.695933154267308,0],[-1.8045834628018462,4.177556818181818,0],]]background('red')push()stroke(0)fill('#EEE')scale(15)beginShape()for(constcontourofcontours){beginContour()for(constvofcontour){vertex(...v)}endContour()}endShape()stroke(0,255,0)strokeWeight(5)beginShape(POINTS)for(constcontourofcontours){for(constvofcontour){vertex(...v)}}endShape()pop()}
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().Modified
_tesselateShape()insrc/webgl/ShapeBuilder.js:1e-6(epsilon) of each other are normalized to use the exact same valueAdded visual test in
test/unit/visual/cases/webgl.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