Skip to content

Remove old overloads for bezierVertex() and quadraticVertex() #7600

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.

Sign up for GitHub

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

Merged
davepagurek merged 2 commits into from
Mar 5, 2025
Merged

Remove old overloads for bezierVertex() and quadraticVertex() #7600

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f4f3ee
Remove old overloads for bezierVertex() and quadraticVertex()
davepagurek Mar 4, 2025
a954f9b
Use bezierVertex for first point
davepagurek Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
272 changes: 1 addition & 271 deletions src/shape/vertex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,20 +663,7 @@ function vertex(p5, fn){
* @param {Number} z4 z-coordinate of the anchor point.
*/
fn.bezierVertex = function(...args) {
Copy link
Collaborator

@GregStanton GregStanton Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (args.length === 2 * 3 || args.length === 3 * 3) {
// Handle the legacy case where all bezier control points are provided
// at once. We'll translate them into 3 individual calls.
const stride = args.length / 3;

const prevOrder = this._renderer.bezierOrder();
this._renderer.bezierOrder(3);
for (let i = 0; i < args.length; i += stride) {
this._renderer.bezierVertex(...args.slice(i, i + stride));
}
this._renderer.bezierOrder(prevOrder);
} else {
this._renderer.bezierVertex(...args);
}
this._renderer.bezierVertex(...args);
};

/**
Expand Down Expand Up @@ -1239,263 +1226,6 @@ function vertex(p5, fn){
this._renderer.endShape(mode, count);
};

/**
* Adds a quadratic Bézier curve segment to a custom shape.
*
* `quadraticVertex()` adds a curved segment to custom shapes. The Bézier
* curve segments it creates are similar to those made by the
* <a href="#/p5/bezierVertex">bezierVertex()</a> function.
* `quadraticVertex()` must be called between the
* <a href="#/p5/beginShape">beginShape()</a> and
* <a href="#/p5/endShape">endShape()</a> functions. The curved segment uses
* the previous vertex as the first anchor point, so there must be at least
* one call to <a href="#/p5/vertex">vertex()</a> before `quadraticVertex()` can
* be used.
*
* The first two parameters, `cx` and `cy`, set the curve’s control point.
* The control point "pulls" the curve towards its.
*
* The last two parameters, `x3`, and `y3`, set the last anchor point. The
* last anchor point is where the curve ends.
*
* Bézier curves can also be drawn in 3D using WebGL mode. The 3D version of
* `bezierVertex()` has eight arguments because each point has x-, y-, and
* z-coordinates.
*
* Note: `quadraticVertex()` won’t work when an argument is passed to
* <a href="#/p5/beginShape">beginShape()</a>.
*
* @method quadraticVertex
* @param {Number} cx x-coordinate of the control point.
* @param {Number} cy y-coordinate of the control point.
* @param {Number} x3 x-coordinate of the anchor point.
* @param {Number} y3 y-coordinate of the anchor point.
* @chainable
*
* @example
* <div>
* <code>
* function setup() {
* createCanvas(100, 100);
*
* background(200);
*
* // Style the curve.
* noFill();
*
* // Draw the curve.
* beginShape();
* vertex(20, 20);
* quadraticVertex(80, 20, 50, 50);
* endShape();
*
* describe('A black curve drawn on a gray square. The curve starts at the top-left corner and ends at the center.');
* }
* </code>
* </div>
*
* <div>
* <code>
* function setup() {
* createCanvas(100, 100);
*
* background(200);
*
* // Draw the curve.
* noFill();
* beginShape();
* vertex(20, 20);
* quadraticVertex(80, 20, 50, 50);
* endShape();
*
* // Draw red lines from the anchor points to the control point.
* stroke(255, 0, 0);
* line(20, 20, 80, 20);
* line(50, 50, 80, 20);
*
* // Draw the anchor points in black.
* strokeWeight(5);
* stroke(0);
* point(20, 20);
* point(50, 50);
*
* // Draw the control point in red.
* stroke(255, 0, 0);
* point(80, 20);
*
* describe('A black curve that starts at the top-left corner and ends at the center. Its anchor and control points are marked with dots. Red lines connect both anchor points to the control point.');
* }
* </code>
* </div>
*
* <div>
* <code>
* // Click the mouse near the red dot in the top-right corner
* // and drag to change the curve's shape.
*
* let x2 = 80;
* let y2 = 20;
* let isChanging = false;
*
* function setup() {
* createCanvas(100, 100);
*
* describe('A black curve that starts at the top-left corner and ends at the center. Its anchor and control points are marked with dots. Red lines connect both anchor points to the control point.');
* }
*
* function draw() {
* background(200);
*
* // Style the curve.
* noFill();
* strokeWeight(1);
* stroke(0);
*
* // Draw the curve.
* beginShape();
* vertex(20, 20);
* quadraticVertex(x2, y2, 50, 50);
* endShape();
*
* // Draw red lines from the anchor points to the control point.
* stroke(255, 0, 0);
* line(20, 20, x2, y2);
* line(50, 50, x2, y2);
*
* // Draw the anchor points in black.
* strokeWeight(5);
* stroke(0);
* point(20, 20);
* point(50, 50);
*
* // Draw the control point in red.
* stroke(255, 0, 0);
* point(x2, y2);
* }
*
* // Start changing the first control point if the user clicks near it.
* function mousePressed() {
* if (dist(mouseX, mouseY, x2, y2) < 20) {
* isChanging = true;
* }
* }
*
* // Stop changing the first control point when the user releases the mouse.
* function mouseReleased() {
* isChanging = false;
* }
*
* // Update the first control point while the user drags the mouse.
* function mouseDragged() {
* if (isChanging === true) {
* x2 = mouseX;
* y2 = mouseY;
* }
* }
* </code>
* </div>
*
* <div>
* <code>
* function setup() {
* createCanvas(100, 100);
*
* background(200);
*
* // Start drawing the shape.
* beginShape();
*
* // Add the curved segments.
* vertex(20, 20);
* quadraticVertex(80, 20, 50, 50);
* quadraticVertex(20, 80, 80, 80);
*
* // Add the straight segments.
* vertex(80, 10);
* vertex(20, 10);
* vertex(20, 20);
*
* // Stop drawing the shape.
* endShape();
*
* describe('A white puzzle piece drawn on a gray background.');
* }
* </code>
* </div>
*
* <div>
* <code>
* // Click the and drag the mouse to view the scene from a different angle.
*
* function setup() {
* createCanvas(100, 100, WEBGL);
*
* describe('A white puzzle piece on a dark gray background. When the user clicks and drags the scene, the outline of a second puzzle piece is revealed.');
* }
*
* function draw() {
* background(50);
*
* // Enable orbiting with the mouse.
* orbitControl();
*
* // Style the first puzzle piece.
* noStroke();
* fill(255);
*
* // Draw the first puzzle piece.
* beginShape();
* vertex(-30, -30, 0);
* quadraticVertex(30, -30, 0, 0, 0, 0);
* quadraticVertex(-30, 30, 0, 30, 30, 0);
* vertex(30, -40, 0);
* vertex(-30, -40, 0);
* vertex(-30, -30, 0);
* endShape();
*
* // Style the second puzzle piece.
* stroke(255);
* noFill();
*
* // Draw the second puzzle piece.
* beginShape();
* vertex(-30, -30, -20);
* quadraticVertex(30, -30, -20, 0, 0, -20);
* quadraticVertex(-30, 30, -20, 30, 30, -20);
* vertex(30, -40, -20);
* vertex(-30, -40, -20);
* vertex(-30, -30, -20);
* endShape();
* }
* </code>
* </div>
*/

/**
* @method quadraticVertex
* @param {Number} cx
* @param {Number} cy
* @param {Number} cz z-coordinate of the control point.
* @param {Number} x3
* @param {Number} y3
* @param {Number} z3 z-coordinate of the anchor point.
*/
fn.quadraticVertex = function(...args) {
let x1, y1, z1, x2, y2, z2 = 0;
if (args.length === 4) {
[x1, y1, x2, y2] = args;
} else {
[x1, y1, z1, x2, y2, z2] = args;
}
// p5._validateParameters('quadraticVertex', args);
const prevOrder = this.bezierOrder();
this.bezierOrder(2);
this.bezierVertex(x1, y1, z1);
this.bezierVertex(x2, y2, z2);
this.bezierOrder(prevOrder);
return this;
};

/**
* Sets the normal vector for vertices in a custom 3D shape.
*
Expand Down
7 changes: 0 additions & 7 deletions test/unit/core/vertex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,6 @@ suite('Vertex', function() {
});
});

suite('p5.prototype.quadraticVertex', function() {
test('should be a function', function() {
assert.ok(myp5.quadraticVertex);
assert.typeOf(myp5.quadraticVertex, 'function');
});
});

suite('p5.prototype.bezierVertex', function() {
test('should be a function', function() {
assert.ok(myp5.bezierVertex);
Expand Down
27 changes: 20 additions & 7 deletions test/unit/visual/cases/shapes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,13 @@ visualSuite('Shape drawing', function() {
setup(p5);
p5.beginShape();
p5.vertex(10, 10);
Copy link
Collaborator

@GregStanton GregStanton Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

@limzykenneth limzykenneth Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

p5.bezierVertex(10, 10, 15, 40, 40, 35);
p5.bezierVertex(25, 15, 15, 25, 15, 25);
p5.bezierVertex(10, 10);
p5.bezierVertex(15, 40);
p5.bezierVertex(40, 35);

p5.bezierVertex(25, 15)
p5.bezierVertex(15, 25)
p5.bezierVertex(15, 25);
p5.endShape();
screenshot();
});
Expand All @@ -204,9 +209,13 @@ visualSuite('Shape drawing', function() {
setup(p5);
p5.beginShape();
p5.vertex(10, 10);
Copy link
Collaborator

@GregStanton GregStanton Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

p5.quadraticVertex(10, 10, 15, 40);
p5.quadraticVertex(40, 35, 25, 15);
p5.quadraticVertex(15, 25, 10, 10);
p5.bezierOrder(2);
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();
screenshot();
});
Expand Down Expand Up @@ -367,7 +376,9 @@ visualSuite('Shape drawing', function() {
p5.beginShape();
p5.vertex(10, 10, 0);
p5.vertex(10, 40, -150);
p5.quadraticVertex(40, 40, 200, 40, 10, 150);
p5.bezierOrder(2);
p5.bezierVertex(40, 40, 200);
p5.bezierVertex(40, 10, 150);
p5.endShape(p5.CLOSE);

screenshot();
Expand All @@ -379,7 +390,9 @@ visualSuite('Shape drawing', function() {
p5.beginShape();
p5.vertex(10, 10, 0);
p5.vertex(10, 40, -150);
p5.bezierVertex(40, 40, 200, 40, 10, 150, 10, 10, 0);
p5.bezierVertex(40, 40, 200);
p5.bezierVertex(40, 10, 150);
p5.bezierVertex(10, 10, 0);
p5.endShape();

screenshot();
Expand Down
7 changes: 3 additions & 4 deletions test/unit/webgl/p5.Geometry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,9 @@ suite('p5.Geometry', function() {

myp5.beginShape();
myp5.vertex(-20, -50);
myp5.quadraticVertex(
Copy link
Collaborator

@GregStanton GregStanton Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

-40, -70,
0, -60
);
myp5.bezierOrder(2);
myp5.bezierVertex(-40, -70);
myp5.bezierVertex(0, -60);
myp5.endShape();

myp5.beginShape(myp5.TRIANGLE_STRIP);
Expand Down