Remove old overloads for bezierVertex() and quadraticVertex() #7600
Conversation
|
@limzykenneth let me know if we're good to merge this! |
| * @param {Number} y4 | ||
| * @param {Number} z4 z-coordinate of the anchor point. | ||
| */ | ||
| fn.bezierVertex = function(...args) { |
There was a problem hiding this comment.
Thanks for doing this Dave! This part looks good. I'll make a separate PR to move this over to custom_shapes.js, and I'll update the inline reference when I do.
test/unit/visual/cases/shapes.js
Outdated
| visualTest('Drawing with cubic beziers', function(p5, screenshot) { | ||
| setup(p5); | ||
| p5.beginShape(); | ||
| p5.vertex(10, 10); |
There was a problem hiding this comment.
Do we not already have tests for the new API? If we do, then maybe this whole test could be removed, rather than converting it to the new API? The same goes for the other tests that were updated.
If we do keep this test, could we replace p5.vertex(10, 10); with p5.bezierVertex(10, 10), to reflect our decision about the explicit first-vertex issue? Right now, I think the code should work the same either way because of how we implemented these features, but I guess it'd be good to align the tests with the API spec in case the implementation changes. (I've been keeping the main post at the top of #6766 up to date, including with our decision to move the legacy API into a compatibility add-on. So the description there can serve as an informal spec, and I'll base the new documentation on it.)
There was a problem hiding this comment.
These are visual tests, ie. essentially integration test so it is separate from unit test, so this should stay around as I don't see another visual test testing bezier curves. It is fine to replace the first vertex with bezierVertex.
test/unit/visual/cases/shapes.js
Outdated
| visualTest('Drawing with quadratic beziers', function(p5, screenshot) { | ||
| setup(p5); | ||
| p5.beginShape(); | ||
| p5.vertex(10, 10); |
There was a problem hiding this comment.
Same question as on line 196. If we can't remove the whole test, could we replace it with the following?
p5.beginShape();
p5.bezierOrder(2);
p5.bezierVertex(10, 10);
p5.bezierVertex(10, 10);
p5.bezierVertex(15, 40);
p5.bezierVertex(40, 35);
p5.bezierVertex(25, 15);
p5.bezierVertex(15, 25);
p5.bezierVertex(10, 10);
p5.endShape();(By the way, I'm guessing the reason for doubling up the vertices at (10, 10) is to make sure we get a line segment in that case.)
|
|
||
| myp5.beginShape(); | ||
| myp5.vertex(-20, -50); | ||
| myp5.quadraticVertex( |
There was a problem hiding this comment.
Same issue here as on lines 196 and 211. Could replace with the following:
myp5.beginShape();
myp5.bezierOrder(2);
myp5.bezierVertex(-20, -50);
myp5.bezierVertex(-40, -70);
myp5.bezierVertex(0, -60);
myp5.endShape();
|
@davepagurek If you want to make the |
Changes
Based on discussion in processing/p5.js-compatibility#1 and elsewhere, this removes the overloads for 1.x bezier functions, leaving just the 2.0 ones. The overloads are handled in the shape compatibility addon instead.
PR Checklist
npm run lintpasses