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

BREAKING: remove absolute true/false from the api. #9395

Merged
merged 24 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- BREAKING: remove absolute true/false from the api. [#9395](https://github.com/fabricjs/fabric.js/pull/9395)
- refactor(Canvas): BREAKING deprecate `getPointer`, add new getScenePoint and getViewportPoint methods, removed `restorePointerVpt`, extended mouse events data [#9175](https://github.com/fabricjs/fabric.js/pull/9175)
- fix(Object): Fix detection of falsy shadows in Object.needsItsOwnCache method [#9469](https://github.com/fabricjs/fabric.js/pull/9469)
- feat(util): expose `calcPlaneRotation` [#9419](https://github.com/fabricjs/fabric.js/pull/9419)
Expand Down
72 changes: 72 additions & 0 deletions e2e/tests/selection/selection-hit-region/index.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { expect, test } from '@playwright/test';
import setup from '../../../setup';
import { CanvasUtil } from '../../../utils/CanvasUtil';

setup();

test('Selection hit regions', async ({ page }) => {
const canvasUtil = new CanvasUtil(page);
// prepare some common functions
await canvasUtil.executeInBrowser((canvas) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there was an officail way to add window methods but I see that page.exposeBinding/exposeFunction run in the playwright context and not the window context

Copy link
Contributor

Choose a reason for hiding this comment

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

but I think instead you could use evaluateHandle and then you can pass the handle back to the window methods and execute it.

Nevermind though

Copy link
Member Author

Choose a reason for hiding this comment

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

i just wanted it to work without duplicating code between the different tests.
This would be probably something to do in the index.ts file of the test rather than the test itself.

I don't think is very important now.

const render = ({ x, y }: fabric.XY, fill: string) => {
const ctx = canvas.getTopContext();
ctx.fillStyle = fill;
ctx.globalAlpha = 0.5;
ctx.beginPath();
ctx.fillRect(x, y, 1, 1);
ctx.fill();
};

window.renderRectsPadding = (rects) => {
for (let y = 0; y <= canvas.height; y += 2) {
for (let x = 0; x < canvas.width; x += 2) {
rects.some((rect) => {
if (canvas._checkTarget(rect, new fabric.Point(x, y))) {
render({ x, y }, rect.fill as string);
}
});
}
}
};
});

await canvasUtil.executeInBrowser((canvas) => {
const group = canvas.getObjects()[0] as fabric.Group;
const rects = group.getObjects();
window.renderRectsPadding(rects);
});

expect(await new CanvasUtil(page).screenshot()).toMatchSnapshot({
name: 'group-padding.png',
});

await canvasUtil.executeInBrowser((canvas) => {
const group = canvas.getObjects()[0] as fabric.Group;
const rects = group.getObjects();
group.scaleX = 0.5;
group.scaleY = 0.5;
group.rotate(45);
canvas.centerObject(group);
canvas.contextTopDirty = true;
canvas.renderAll();
window.renderRectsPadding(rects);
});

expect(await new CanvasUtil(page).screenshot()).toMatchSnapshot({
name: 'transformed-group-padding.png',
});

await canvasUtil.executeInBrowser((canvas) => {
const group = canvas.getObjects()[0] as fabric.Group;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could use the ObjectUtil instead of the canvas util

const rects = group.getObjects();
canvas.setZoom(2);
canvas.viewportCenterObject(group);
canvas.contextTopDirty = true;
canvas.renderAll();
window.renderRectsPadding(rects);
});

expect(await new CanvasUtil(page).screenshot()).toMatchSnapshot({
name: 'zoomed-transformed-group-padding.png',
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
48 changes: 48 additions & 0 deletions e2e/tests/selection/selection-hit-region/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* Runs in the **BROWSER**
* Imports are defined in 'e2e/imports.ts'
*/

import { Rect, Group } from 'fabric';
import { beforeAll } from '../../test';

beforeAll((canvas) => {
canvas.setDimensions({ width: 500, height: 500 });

const rect = new Rect({
width: 200,
height: 200,
padding: 30,
left: 40,
top: 50,
fill: 'green',
opacity: 0.5,
});
const rect2 = new Rect({
width: 200,
angle: 10,
top: 60,
left: 250,
height: 200,
padding: 6,
fill: 'blue',
opacity: 0.5,
});
const rect3 = new Rect({
width: 200,
angle: 45,
left: 60,
top: 260,
height: 200,
padding: 0,
fill: 'purple',
opacity: 0.5,
});

const group = new Group([rect, rect2, rect3], {
subTargetCheck: true,
});
canvas.add(group);

return { group, rect, rect2, rect3 };
});
10 changes: 6 additions & 4 deletions src/Collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
object: FabricObject,
index: number,
array: FabricObject[]
) => any

Check warning on line 88 in src/Collection.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
) {
this.getObjects().forEach((object, index, objects) =>
callback(object, index, objects)
Expand Down Expand Up @@ -311,6 +311,8 @@
* Given a bounding box, return all the objects of the collection that are contained in the bounding box.
* If `includeIntersecting` is true, return also the objects that intersect the bounding box as well.
* This is meant to work with selection. Is not a generic method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This is meant to work with selection. Is not a generic method.

Why can't it be used in general? It is a good piece of logic

Copy link
Member Author

Choose a reason for hiding this comment

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

it was written for intersection on the canvas with the drag rectangle.
We are not promoting it now, promoting is possible without breaking later.

* @param {TBBox} bbox a bounding box in scene coordinates
* @param {{ includeIntersecting?: boolean }} options an object with includeIntersecting
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not very useful info but the above it so that should be moved here IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

Include objects that are contained in or intersect the bounding box

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that i wanted to copy a parameter section from another function and i pulled in a piece of comment without noticing. removing it.

* @returns array of objects contained in the bounding box, ordered from top to bottom stacking wise
*/
collectObjects(
Expand All @@ -327,10 +329,10 @@
if (
object.selectable &&
object.visible &&
((includeIntersecting && object.intersectsWithRect(tl, br, true)) ||
object.isContainedWithinRect(tl, br, true) ||
(includeIntersecting && object.containsPoint(tl, true)) ||
(includeIntersecting && object.containsPoint(br, true)))
((includeIntersecting && object.intersectsWithRect(tl, br)) ||
object.isContainedWithinRect(tl, br) ||
(includeIntersecting && object.containsPoint(tl)) ||
(includeIntersecting && object.containsPoint(br)))
) {
objects.push(object);
}
Expand Down
5 changes: 1 addition & 4 deletions src/benchmarks/raycasting.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,7 @@ export const cornerPointContainsPoint = (point, cornerPoint) => {

class Test1 extends FabricObject {
containsPoint(point, absolute, calculate) {
return cornerPointContainsPoint(
point,
this._getCoords(absolute, calculate)
);
return cornerPointContainsPoint(point, this._getCoords(calculate));
}
}

Expand Down
Loading
Loading