Fix: Preserve canvas position when calling noSmooth() in WEBGL #7568
Conversation
|
Hi @davepagurek, I've rebased my changes onto Since my fix involves preventing unnecessary canvas recreation in WebGL mode, some tests might need updating. Should I update the reference screenshots or investigate further? The other tests and linting checks are successful. Let me know how you'd like to proceed! Thanks! |
There was a problem hiding this comment.
Thanks for making this PR! Looks like the addition of a p5. prefix is the main cause of the test issues, hopefully it'll work after taking that out!
Also it looks like an auto-formatter ran on the file so there's still a fair amount of noise in the diff, would it be possible to turn that off when editing?
src/webgl/p5.RendererGL.js
Outdated
| @@ -930,15 +930,22 @@ class RendererGL extends Renderer { | |||
| const w = this.width; | |||
| const h = this.height; | |||
| const defaultId = this.canvas.id; | |||
| const isPGraphics = this._pInst instanceof Graphics; | |||
| const isPGraphics = this._pInst instanceof p5.Graphics; | |||
There was a problem hiding this comment.
Looks like the addition of p5. here is a problem (and similarly in another spot in Element.call(...).) I believe Graphics is imported at the top so it doesn't need to be referenced through the p5 namespace, can you try taking those out?
There was a problem hiding this comment.
Got it! I'll turned off my auto-formatter and will remove the p5. prefix, including the one in Element.call(...). Let me know if there's anything else I should adjust. Thanks for the review.
|
Hi @davepagurek, All tests have now passed, and I’ve turned off my auto-formatter to prevent unnecessary diffs in future too. Let me know if anything else needs adjustment. Thanks! |
There was a problem hiding this comment.
Thanks, I'm glad to see tests are working now! The last thing I think we'll want to do is to add a unit test to make sure that future changes to p5 won't accidentally break this change without us knowing. We could add a test that changes the position of a webgl canvas, then calls noSmooth, and then checks to see that the new element has the same position style attributes as before?
Let me know if I can help explain anything about unit testing in p5!
src/webgl/p5.RendererGL.js
Outdated
| if (isPGraphics) { | ||
| const pg = this._pInst; | ||
| pg.canvas.parentNode.removeChild(pg.canvas); | ||
| pg.canvas = document.createElement("canvas"); | ||
| const node = pg._pInst._userNode || document.body; | ||
| node.appendChild(pg.canvas); | ||
| Element.call(pg, pg.canvas, pg._pInst); | ||
| p5.Element.call(pg, pg.canvas, pg._pInst); |
There was a problem hiding this comment.
Does this still work if we take off the p5 prefix here too as it was before?
| p5.Element.call(pg, pg.canvas, pg._pInst); | |
| Element.call(pg, pg.canvas, pg._pInst); |
There was a problem hiding this comment.
Yes, I tested it after removing the p5. prefix, and everything works correctly. The canvas still preserves its position after calling noSmooth().
|
Added a unit test to verify that noSmooth() preserves canvas position in WebGL. Also added comments in p5.RendererGL.js for clarity. All tests passed locally, but one CI test is still failing. |
| import { describe, it, expect } from 'vitest'; | ||
|
|
||
| // Importing p5 | ||
| import '../../../lib/p5.js'; |
There was a problem hiding this comment.
I think this import is giving it trouble because lib is only built on release or when manually built.
Other test files tend to import p5 differently (see for example test/unit/webgl/RendererGL.js.) It might be easier (and maybe more organized too) to actually just put this test case into the existing RendererGL test file, as then you can just use the myp5 variable it has already imported and created.
There was a problem hiding this comment.
Got it! I'll move the test into p5.RendererGL.js and reuse the existing myp5 instance. Thanks for the suggestion!
|
Moved the noSmooth() canvas position test to |
🔧 Resolves #7548
Changes:
p5.RendererGL.jsby ensuringnoSmooth()does not trigger a full reset in WebGL mode.noSmooth(), avoiding unintended resets whenposition(x, y)is used.Screenshots of the Change:
✅ The canvas now maintains its position correctly after calling
noSmooth().✅ PR Checklist
npm run lintpasses