Fix ellipseMode(CORNERS) and rectMode(CORNER) #7290
Conversation
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page! |
|
Hi, Thanks for taking up this task. I believe I have tested this and it works well. I have a concern: are you planning to write tests for it in the If so, I don't think that approach would work for your case. Perhaps we could write unit or visual tests instead. What's your opinion? |
modeAdjust() now always returns the top-left corner and a positive width and height.
|
Hi, I've added proper visual test and removed |
@all-contributors please add @martinleopold for bug, code |
|
I've put up a pull request to add @martinleopold! 🎉 |
|
|
||
| for (let mode of MODES) { | ||
| visualTest(`Mode ${mode}`, function(p5, screenshot) { | ||
| p5.createCanvas(240, 500); |
There was a problem hiding this comment.
Alright, I believe creating a larger canvas might be an issue. When the tests run, a bigger canvas could make them take a bit longer due to its size. Plus, including files that are 15-20kB could make the codebase pretty heavy. Could we perhaps reduce it to 50x50? That would probably take up minimal space in bytes.
By doing this, we could include the tests one by one. Like, for each quadrant we can add one tests or something like that? For reference, you might want to check out this link: https://github.com/processing/p5.js/pull/6783/files#diff-aefa4b8e5287fbd0cdf37a6ec8d27322a4e5c893e99ec74d32053e60e2bdfd7d
Quadrant-1 -> all modes -> 1 particular tests? Or do you have any idea how could 50X50 size canvas could be fit for testing? I am also not 100% sure.
Maybe by using a 50x50 size, we could adjust the parameter values of _shapeCorners accordingly?
Resolves #7289
Changes:
This mainly resolves a rendering/crashing issues with
ellipseMode(CORNERS)affectingellipse()andarc(); see issue above). It also fixes a related issue with usingrectMode(CORNER)and negative widths and/or heights.The bulk of the changes are in
modeAdjust()which should now properly handle the modes and return a normalized bounding box. As a result a few lines of handling negative widths and heights needed to be removed fromarc()and_renderEllipse().I've included a test sketch in test/shape-modes.html where you can cycle through all the different modes and shapes. I'm happy to update this, if somebody could direct me on how to properly test this case. I'm not sure, since it's mostly about the rendering output.
Screenshots of the change:
PR Checklist
npm run lintpasses