Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ellipseMode(CORNERS) and rectMode(CORNER) #7290

Merged
merged 7 commits into from
Oct 20, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
39 changes: 35 additions & 4 deletions src/core/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,47 @@

import * as constants from './constants';

/*
This function normalizes the first four arguments given to rect, ellipse and arc
according to the mode.
It returns a 'bounding box' object containing the coordinates of the upper left corner (x, y),
and width and height (w, h). The returned width and height are always positive.
*/
function modeAdjust(a, b, c, d, mode) {
let bbox;

if (mode === constants.CORNER) {
return { x: a, y: b, w: c, h: d };

// CORNER mode already corresponds to a bounding box (top-left corner, width, height)
bbox = { x: a, y: b, w: c, h: d };

} else if (mode === constants.CORNERS) {
return { x: a, y: b, w: c - a, h: d - b };

// CORNERS mode uses two opposite corners, in any configuration.
// Make sure to get the top left corner by using the minimum of the x and y coordniates.
bbox = { x: Math.min(a, c), y: Math.min(b, d), w: c - a, h: d - b };

} else if (mode === constants.RADIUS) {
return { x: a - c, y: b - d, w: 2 * c, h: 2 * d };

// RADIUS mode uses the center point and half the width and height.
// c (half width) and d (half height) could be negative, so use the absolute value
// in calculating the top left corner (x, y).
bbox = { x: a - Math.abs(c), y: b - Math.abs(d), w: 2 * c, h: 2 * d };

} else if (mode === constants.CENTER) {
return { x: a - c * 0.5, y: b - d * 0.5, w: c, h: d };

// CENTER mode uses the center point, width and height.
// c (width) and d (height) could be negative, so use the absolute value
// in calculating the top-left corner (x,y).
bbox = { x: a - Math.abs(c * 0.5), y: b - Math.abs(d * 0.5), w: c, h: d };

}

// p5 supports negative width and heights for rectangles, ellipses and arcs
bbox.w = Math.abs(bbox.w);
bbox.h = Math.abs(bbox.h);

return bbox;
}

export default { modeAdjust };
13 changes: 1 addition & 12 deletions src/core/shape/2d_primitives.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,6 @@ p5.prototype.arc = function(x, y, w, h, start, stop, mode, detail) {
start = this._toRadians(start);
stop = this._toRadians(stop);

// p5 supports negative width and heights for ellipses
w = Math.abs(w);
h = Math.abs(h);

const vals = canvas.modeAdjust(x, y, w, h, this._renderer._ellipseMode);
const angles = this._normalizeArcAngles(start, stop, vals.w, vals.h, true);

Expand Down Expand Up @@ -547,16 +543,9 @@ p5.prototype._renderEllipse = function(x, y, w, h, detailX) {
return this;
}

// p5 supports negative width and heights for rects
if (w < 0) {
w = Math.abs(w);
}

// Duplicate 3rd argument if only 3 given.
if (typeof h === 'undefined') {
// Duplicate 3rd argument if only 3 given.
h = w;
} else if (h < 0) {
h = Math.abs(h);
}

const vals = canvas.modeAdjust(x, y, w, h, this._renderer._ellipseMode);
Expand Down
2 changes: 1 addition & 1 deletion test/node/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ suite('helpers/modeAdjust', function() {
});
test('should set mode to corners', function() {
result = helpers.modeAdjust(a, b, c, d, constants.CORNERS);
expect(result).to.eql({ x: 100, y: 200, w: -50, h: -50 });
expect(result).to.eql({ x: 50, y: 150, w: 50, h: 50 });
});
test('should set mode to radius', function() {
result = helpers.modeAdjust(a, b, c, d, constants.RADIUS);
Expand Down
3 changes: 2 additions & 1 deletion test/unit/spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ var spec = {
// Add the visual tests that you want run as part of CI here. Feel free
// to omit some for speed if they should only be run manually.
'webgl',
'typography'
'typography',
'shape_modes'
]
};
document.write(
Expand Down
118 changes: 118 additions & 0 deletions test/unit/visual/cases/shape_modes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
Helper function that draws a shape using the specified shape mode
p5 ............... The p5 Instance
shape ............ The shape to draw. Either 'ellipse', 'arc', or 'rect'
mode ............. The ellipseMode (for ellipse and arc), or rectMode (for rect)
Either p5.CORNERS, p5.CORNER, p5.CENTER or p5.RADIUS
x1, x2, x2, y2 ... Coordinates specifying the shape in CORNERS mode,
i.e. (x1, y1) and (x2, y2) specify two opposite corners P1 and P2
*/
function shapeCorners(p5, shape, mode, x1, y1, x2, y2) {
// Adjust coordinates for testing modes other than CORNERS
if (mode === p5.CORNER) {
// Find top left corner
let x = p5.min(x1, x2); // x
let y = p5.min(y1, y2); // y
// Calculate width and height
// Don't use abs(), so we get negative values as well
let w = x2 - x1; // w
let h = y2 - y1; // h
x1 = x; y1 = y; x2 = w; y2 = h;
} else if (mode === p5.CENTER) {
// Find center
let x = (x2 + x1) / 2; // x
let y = (y2 + y1) / 2; // y
// Calculate width and height
// Don't use abs(), so we get negative values as well
let w = x2 - x1;
let h = y2 - y1;
x1 = x; y1 = y; x2 = w; y2 = h;
} else if (mode === p5.RADIUS) {
// Find Center
let x = (x2 + x1) / 2; // x
let y = (y2 + y1) / 2; // y
// Calculate radii
// Don't use abs(), so we get negative values as well
let r1 = (x2 - x1) / 2; // r1;
let r2 = (y2 - y1) / 2; // r2
x1 = x; y1 = y; x2 = r1; y2 = r2;
}

if (shape === 'ellipse') {
p5.ellipseMode(mode);
p5.ellipse(x1, y1, x2, y2);
} else if (shape === 'arc') {
// Draw four arcs with gaps inbetween
const GAP = p5.radians(30);
p5.ellipseMode(mode);
p5.arc(x1, y1, x2, y2, 0 + GAP, p5.HALF_PI - GAP);
p5.arc(x1, y1, x2, y2, p5.HALF_PI + GAP, p5.PI - GAP);
p5.arc(x1, y1, x2, y2, p5.PI + GAP, p5.PI + p5.HALF_PI - GAP);
p5.arc(x1, y1, x2, y2, p5.PI + p5.HALF_PI + GAP, p5.TWO_PI - GAP);
} else if (shape === 'rect') {
p5.rectMode(mode);
p5.rect(x1, y1, x2, y2);
}
}


/*
Comprehensive test for rendering ellipse(), arc(), and rect()
with the different ellipseMode() / rectMode() values: CORNERS, CORNER, CENTER, RADIUS.
Each of the 3 shapes is tested with each of the 4 possible modes, resulting in 12 test.
Each test renders the shape in 16 different coordinate configurations,
testing combinations of positive and negative coordinate values.
*/
visualSuite('Shape Modes', function(...args) {
// Shapes to test
const SHAPES = [ 'ellipse', 'arc', 'rect' ];

// Modes to test (used with ellipseMode or rectMode, according to shape)
const MODES = [ 'CORNERS', 'CORNER', 'CENTER', 'RADIUS' ];

for (let shape of SHAPES) {
visualSuite(`Shape ${shape}`, function() {

for (let mode of MODES) {
visualTest(`Mode ${mode}`, function(p5, screenshot) {
p5.createCanvas(240, 500);
Copy link
Contributor

@perminder-17 perminder-17 Oct 5, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in this case using many smaller canvases instead of a few bigger ones, will actually make things slower. Because then we'll have the overhead of createCanvas() i.e. creating a rendering context much more often, and the overall codebase size will also be larger with many small PNGs because of the PNG headers.

And to thoroughly test everything, we have 4 modes, times 3 shapes, times 4 quadrants, times 4 combinations of points, equals 192 shapes to draw. I see no way around that. Bear in mind this adds 192 test for fundamental drawing operations that were previously untested.

Therefore I propose making the canvas in the tests as small as possible, while still testing all the cases.
I have reduced the canvas size from 240x500 to 60x125 px, which reduces the total size of the generated PNGs from 223 to 12 kB.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good!

p5.translate(p5.width/2, p5.height/2);

// Make the following calls to shapeCorners shorter
// by omitting p5, shape and mode parameters
function _shapeCorners(x1, y1, x2, y2) {
shapeCorners(p5, shape, p5[mode], x1, y1, x2, y2);
}

// Quadrant I (Bottom Right)
// P1 P2
_shapeCorners( 10, 10, 110, 60); // P1 Top Left, P2 Bottom Right
_shapeCorners( 10, 120, 110, 70); // P1 Bottom Left, P2 Top Right
_shapeCorners(110, 180, 10, 130); // P1 Bottom Right, P2 Top Left
_shapeCorners(110, 190, 10, 240); // P1 Top Right, P2 Bottom Left

// Quadrant II (Bottom Left)
_shapeCorners(-110, 10, -10, 60);
_shapeCorners(-110, 120, -10, 70);
_shapeCorners(-10, 180, -110, 130);
_shapeCorners(-10, 190, -110, 240);

// Quadrant III (Top Left)
_shapeCorners(-110, -240, -10, -190);
_shapeCorners(-110, -130, -10, -180);
_shapeCorners(-10, -70, -110, -120);
_shapeCorners(-10, -60, -110, -10);

// Quadrant IV (Top Right)
_shapeCorners( 10, -240, 110, -190);
_shapeCorners( 10, -130, 110, -180);
_shapeCorners(110, -70, 10, -120);
_shapeCorners(110, -60, 10, -10);

screenshot();
}); // End of: visualTest
} // End of: MODES loop

}); // End of: Inner visualSuite
} // End of: SHAPES loop
}); // End of: Outer visualSuite
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"numScreenshots": 1
}
2 changes: 1 addition & 1 deletion test/visual/visualTestList.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// List all visual test files here that should be manually run
const visualTestList = ['webgl', 'typography'];
const visualTestList = ['webgl', 'typography', 'shape_modes'];

for (const file of visualTestList) {
document.write(
Expand Down