BREAKING: remove absolute true/false from the api. #9395
Conversation
|
Build Stats
|
|
replace #9373 |
|
i need to add back some debug code so we can see against what we are targeting objects. |
|
Found the small error, now i have to write a test that is clear in the visual and the intent |
|
Ok this is ready for review. |
All the description is correct only for canvas level objects. |
| test('Selection hit regions', async ({ page }) => { | ||
| const canvasUtil = new CanvasUtil(page); | ||
| // prepare some common functions | ||
| await canvasUtil.executeInBrowser((canvas) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| }); | ||
|
|
||
| await canvasUtil.executeInBrowser((canvas) => { | ||
| const group = canvas.getObjects()[0] as fabric.Group; |
There was a problem hiding this comment.
you could use the ObjectUtil instead of the canvas util
src/mixins/eraser_brush.mixin.ts
Outdated
There was a problem hiding this comment.
I think we should revert this file in order to avoid conflicts
It is dead and handled by #8499
| private _pointIsInObjectSelectionArea(obj: FabricObject, point: Point) { | ||
| // getCoords will already take care of group de-nesting | ||
| let coords = obj.getCoords(); | ||
| const viewportZoom = this.getZoom(); | ||
| const padding = obj.padding / viewportZoom; | ||
| if (padding) { | ||
| const [tl, tr, br, bl] = coords; | ||
| // what is the angle of the object? | ||
| // we could use getTotalAngle, but is way easier to look at it | ||
| // from how coords are oriented, since if something went wrong | ||
| // at least we are consistent. | ||
| const angleRadians = Math.atan2(tr.y - tl.y, tr.x - tl.x), | ||
| cosP = cos(angleRadians) * padding, | ||
| sinP = sin(angleRadians) * padding, | ||
| cosPSinP = cosP + sinP, | ||
| cosPMinusSinP = cosP - sinP; | ||
|
|
||
| coords = [ | ||
| new Point(tl.x - cosPMinusSinP, tl.y - cosPSinP), | ||
| new Point(tr.x + cosPSinP, tr.y - cosPMinusSinP), | ||
| new Point(br.x + cosPMinusSinP, br.y + cosPSinP), | ||
| new Point(bl.x - cosPSinP, bl.y + cosPMinusSinP), | ||
| ]; | ||
| // in case of padding we calculate the new coords on the fly. | ||
| // otherwise we have to maintain 2 sets of coordinates for everything. | ||
| // we can reiterate on storing those on something similar to lineCoords | ||
| // if this is slow, for now the semplification is large and doesn't impact | ||
| // rendering. | ||
| // the idea behind this is that outside target check we don't need ot know | ||
| // where those coords are | ||
| } | ||
| return Intersection.isPointInPolygon(point, coords); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do you mind if I simplify the logic here using a padding vector?
Does it have to be private?
Can we instead change containsPoint to accept a 3rd param padding defaulting to 0?
I think that is much better both for fabric and for the dev
There was a problem hiding this comment.
my issue here is that Intersection is not a selection thing.
This is a method entirely done to check the selection and padding is selection only.
Why would you use containsPoint with padding? use checkTarget right away no?
if is padding is a selection thing.
Why would you use containsPoint instead of checkTarget?
Regarding private, i did that because i thought, let's see how it shapes out, become public is easier, or maybe then we need to change it radically or change its interface, and being private is a lesser deal.
Regarding the logic, i would like to see it first, so that we can compare is actually simpler.
This was just porting over what was before somewhere else, i m not attached to it.
Just dump how you would write this whole method in a comment.
There was a problem hiding this comment.
// expose as const, can be reused by other parts
const CORNER_ORIGIN = {
tl: new Point(-0.5, -0.5),
tr: new Point(0.5, -0.5),
br: new Point(0.5, 0.5),
bl: new Point(-0.5, 0.5),
};
// expose in ObjectGeometry
containsViewportPoint(point: Point, padding = 0) {
const vpt = this.getViewportTransform();
const paddingVector = new Point().scalarMultiply(padding * 2);
const [tl, tr, br, bl] = this.getCoords();
const viewportCoords = (
Object.entries({ tl, tr, br, bl }) as [
keyof typeof CORNER_ORIGIN,
Point
][]
).map(([key, point]) =>
sendPointToPlane(point, vpt).add(
paddingVector.multiply(CORNER_ORIGIN[key])
)
);
return Intersection.isPointInPolygon(point, viewportCoords);
}There was a problem hiding this comment.
There is a lot to say about this.
Is better explained in person but i can summarize that Object geometry is done with padding and viewport. We just remove the concept that padding is part of the object geometry, is just a selection concern, and saying 'containsViewportPoint' and adding back padding is rolling back the change.
Also in this example i don't understand who is rotating the padding vector. I see we are using a direction vector to correctly move the point in that direction, but where is the rotation?
There was a problem hiding this comment.
but where is the rotation?
I missed that
We just remove the concept that padding is part of the object geometry, is just a selection concern, and saying 'containsViewportPoint' and adding back padding is rolling back the change.
I have much to say about that. I have said that geometry must occur in the viewport but you did not take that into account so I don't see as rolling back
There was a problem hiding this comment.
I don't remember you saying that, or we wouldn't have remove the absolute from everywhere.
Anyway, next meeting then.
I l freeze everything meanwhile
src/canvas/SelectableCanvas.spec.ts
Outdated
There was a problem hiding this comment.
I am not sure this is extensive enough
What about using the same tehnique as the e2e test? Iterating over all points and calling the method and storing the result somehow?
Then it is a pixel perfect test and the test can e reduced in size using each I guess
There was a problem hiding this comment.
this test covers most of the point at the edge of selection, and cover cases with groups, padding, skew, strokeUniform.
I think is fine, and if it doesn't cover something that will bug out, we will extend the test.
|
This is ready to go. |
| * /** | ||
| * Moves an object to specified level in stack of drawn objects | ||
| * @param {TBBox} bbox a bounding box in scene coordinates | ||
| * @param {{ includeIntersecting?: boolean }} options an object with includeIntersecting |
There was a problem hiding this comment.
this is not very useful info but the above it so that should be moved here IMO
There was a problem hiding this comment.
Include objects that are contained in or intersect the bounding box
There was a problem hiding this comment.
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.
| @@ -311,13 +311,16 @@ export function createCollectionMixin<TBase extends Constructor>(Base: TBase) { | |||
| * 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. | |||
There was a problem hiding this comment.
| * 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
There was a problem hiding this comment.
it was written for intersection on the canvas with the drag rectangle.
We are not promoting it now, promoting is possible without breaking later.
|
follow up to change the padding calculation to use more matrix and vector names |
|
@ShaMan123 approved in meeting |
Motivation
Fabric is currently maintaining 2 sets of coordinates.
One that is a scene/design coordinate space and another that is the screen/viewport space.
The scene is called aCoords, while the screen/viewport are both oCoords and lineCoords.
Those 2 sets aren't simply connected by the viewport trasnformation but also they have the addition of padding that always threw a wrench in calculations and predictability of the api.
Each api of the geometry topic ( containsPoint, containsObject, getBoundingBox ) had an option to be called with a boolean called
absolute. In case the boolean was true, calculation were made around aCoords, while with false around lineCoords.The issue was that lineCoords included padding, and that meant that depending on the padding level of controls those intersection would trigger also over the padding area.
This created a bleeding between 2 different piece of logic, pure object geometry and selection/targeting functionalities.
Padding adds some pixels around the natural bounding box of the object either for aesthetic reasons or in order to facilitate the object selection when an object is too small.
Adding padding to an object doesn't make it larger or covering more area or intersecting with more objects.
So now padding is out of all the geometries.
to make an example, scaleToWidth would scale an object to X pixels, on screen pixels, including padding.
If you had padding set to 10 to an object, you would get the bounding box of the object including padding, then you would divide that by the scaledWidth of the objet to get a scale factor.
Imagine the scale factor was 0.5, you would scale down your object by half, but padding never scales, so again your object would always be either too small ( because you don't expect and you don't see padding ) or too large ( because you expect padding but padding doesn't get scaled by object scaling ).
object.lineCoords have no reason to exists and are gone
All the api that required absolute=true to be specified are loosing absolute option and that is true by default.
We are slowly stopping referring to those coordinates as absolute since it was also confusing.
List of affect apis:
Padding calculation is now a concern of the canvas and for now is not cached.
If you have an object with padding, every time you are selecting it or running a target check over it, you will run this calculation on top of the normal ones:
The code has been written with the idea of minimizing the calculation and to be consistent with the status of aCoords.
If aCoords are out of sync, padding will be out of sync too, that is ok.
Testing
This pr adds a new method that could be tested a bit more rigorously and requires manual verification since i conducted manual tests with objects and group but not too deeply