Proposal: Removing originX and originY

[asturur]

Those notes are the base of an evaluation of removing customization of origins for objects in fabricjs in order to possibly reduce complexity of the code and the api.
This idea fits in a long term simplification plan of dropping features of possible low utilization in order to improve maintainability of the code.
Please vote this message with a thumb up if you think is a good idea or a good change, or use the thumb down AND add a message under the second message explaining why you would like to maintain originX and originY

TLDR

remove originX / originY and make everything as if they are set to center/center.

Introduction - What are originX and originY

Origin(x/y) can be one of 3 values or a number. The combination of originX and originY represent which point of the object is associated to the values of the properties left and top.
Today the default values ia left/top and another popular combination is center/center
In the library originX and originY are used a lot for object transforms to find a point in which the object needs to be stable during scaling or rotation, and that is not being touched or changed.
Origins can also be a number when the standards value are not enough, usually between 0 and 1, but i used also origins like -2 and + 3 for cropping controls.

Why do we have OriginX and OriginY

I do not know, they were here in 2014 when i joined, i tried to search the PR history for information but i didn't find much.

Pros of having configurable origins

• Depending on how you feel comfortable you can set the objects on different anchor points, for example
• Rects and rectangular photos are easily understood when positioned by left/top
• Circle and Ellipse are easier when positioned by the center
• Paths and Polygon it depends from how the path and polygon looks like
• Text has different alignment, so we could argue that depending on the alignment of the text paragraph both left/center/right make sense, but definitely the vertical positioning as top is not a good default for text

Cons of having configurable origins

• Calculations have an extra layer of complexity.
  A very common operation is calculating the center of the object, because we draw based on the center. When the origin is set on left/top it means that the position we have is at the extremity of the bounding box, including stroke, strokeUniform and scaling and rotation. As a consequence every time we want to calculate the center (every render every object multiple times ) we go through the necessary calculations. If the origin were set to center/center, all those calculations are equivalent to just taking left/top property and consider it as the center.
• Another side effect of the origin is that they also represent the anchor point of visual rotation when we change angle to an object. Setting an object with angle 45 on an object that has origin in the center will rotate the object on the center, while if the object as left/top it will start rotating from that corner. To make an example, a rect position in left/top 0,0 with angle 90, usually disappears from the canvas. The rotation around a point that is not the center is very specific while rotating on the center feels natural for most of the operations.
• Changing originX and originY of an object moves the object as a side effect, that is non intuitive since the developer didn't change top and left.
• In general changes of usages of originX/Y during an application are rare, at least from my experience. I personally set it to center when i start and i don’t touch it anymore for all the development time. I never developed an application in which i had some objects set on the center and some not if not for short moments to obtain an effect in which i could have just used setPositionByOrigin every time i updated the position
• Maintainance cost. Every time we touch a feature that relates to positioning or calculation we have to write multiple tests that take in account different values of origins.

What happen if we remove it

If we remove originX and originY from an object, the following side effects are expected:

• Defaults of fabricJS break, require major version and an updater
• originX and originY property gets removed from the object and from defaults
  Import of old design is possibly broken and need an updater
• Demos and examples will have object shifted
• Hopefully code gets faster but for sure simpler thanks to the removal of extra calculation ( transformMatrix mainly )
• Despite the initial shock for who uses it, then things should get easier
• All the tests that have to account for multiple origins can be removed

Possible plan of action

We open a PR and we start deleting and following typescript we identify all the places where origins are in play.
In those places we keep the code working as the value of origin was center/center.

getRelativeCenterPoint becomes () => new Point(this.left, this.top);
between translateToOriginPoint and getPointByOrigin only one survives and probably gets renamed
translateToCenterPoint is probably removed
_getLeftTopCoords is probably removed

The ObjectOrigin class goes away and gets merged in the nearest class ( with time things have been moved/removed and now is small enough that with this final change maybe it doesn’t make sense anymore ).

An updater needs to be created so old designs do not break, details of how you use it are TBD.
Can be enforced on every operation and unremovable or optionally included, the option that is less painful for the developer that will have to migrate.

The issue with top and left properties

If we assume that the origins are center based, because is the easiest for calculation and the most neutral for objects, at this point top and left need to be renamed.
That is very destructive, so we need to add probably deprecated getters and setters for that that will set 2 new properties that will replace left/top ( x,y / position: Point, others... ). That shouldn’t happen in the same pr and it should probably not go extensively on all the codebase, maybe we do it where it matters and we iterate.

When this happens we need to increment the major version by 1.

[asturur]

Are you using originX and originY in a way that maybe is very useful and if they go missing your project will be hurt?
please comment under this message and tell us

[asturur]

• reserved

[zhe-he]

I support removing originX and originY. I can accept setting the center point as either left, top, or center, center by default, as long as there is a corresponding method to get the other value. For example, if the default center point is center, center and the rotation angle of the shape is a, getLeftTopByAngle(a) should return left, top. Or if the default center point is left, top and the rotation angle of the shape is a, getCenterByAngle(a) should return center, center.

I hope that the positioning of elements within the group can support originXByParent and originYByParent. Currently, the coordinates of elements within the group are all based on the center of the group by default. I may need them to be based on the top left corner.

2024-03-18.10.54.50.mov

[asturur]

The old methods are not going away.
This discussion is strictly around for removing originX and originY from the objects.
Having groups with different coordinate systems is unlikely but anyway is unrelated to this topic.
What you are showing in the video, unrelated, i think can be obtained fairly today with the custom controls.

[viveknigam3003]

Hey @asturur in my implementation of an ObjectContainer (using groups in v5.3) I'm using originX and originY to pin the object to one of the 9 possible positions inside a group. It is a requirement for the platform to support layout positioning. This way when I scale the object (in this case, an image) inside the group, it scales from that position itself. Having originX and originY simplifies the implementation significantly. Do let me know if you have any alternative approaches to handle this without originX and originY

Video for reference attached here

OriginX.mp4

[asturur]

Yes in that case i would imagine your 5.3 ( upgraded to fabric 7 or higher at this point ) button would do:

image.setPositionByOrigin(-group.width/2, -group.height/2, 'left', 'bottom')

and the scale textbox will need to do:

image.scaleX = number;
image.scaleY = number;
// and again
image.setPositionByOrigin(-group.width/2, -group.height/2, 'left', 'bottom')

The difference is that today the code related to image.setPositionByOrigin(-group.width/2, -group.height/2, 'left', 'bottom') is executed by every single positioning action automatically, what i am proposing is that when you need special positioning you will call the positioning function yourself.

This could be an example to replicate as a transition guide for example

I understand there is a bigger issue, you now need props to keep in the object to memorize which is the anchor point of that image. But you can still add them and you can still use originX and originY if you want, just they wan't be automatically used.

// capture the position you know you are using
image.setPositionByOrigin(-group.width/2, -group.height/2, image.specialPropIUseToTrackOriginX, image.specialPropIUseToTrackOriginY)
image.scaleX = number;
image.scaleY = number;
// and again
image.setPositionByOrigin(-group.width/2, -group.height/2, 'left', 'bottom')

Probably this is something to flesh out in the proposal itself, or maybe before this removal a better way to handle object state is auspicable

[viveknigam3003]

Ohh interesting approach, in my custom class ObjectContainer I have a properties.objectPosition value already which sets to "{originY}-{originX}" at this point (eg. objectPosition: 'top-left') so no problem with that 😄

Now my class's internal function can be rewritten as below:

_setObjectPositionToTopLeft() {
    const object = this.getObject() as fabric.Object;

    // OLD WAY
    // object.set({
    // 	originX: 'left',
    // 	originY: 'top',
    // 	left: -this.width / 2,
    // 	top: -this.height / 2,
    // });

    // NEW PROPOSED WAY
    object.setPositionByOrigin(new fabric.Point(-this.width / 2, -this.height / 2), 'left', 'top');

    this._setContainerProperty('objectPosition', 'top-left');
},

And the place where I update my zoom value (in my react component) could be updated with this:

const updateZoomValue = (value: number) => {
    setZoomValue(value);
    // Update the scale of the image inside the group
    const container = currentSelectedElements[0] as fabric.ImageContainer;

    // WITH NEW WAY
    container.setObjectPosition(layoutPosition);
    container.getObject().scale(value / 100);
    container.setObjectPosition(layoutPosition);

    canvas.renderAll();
};

I tested it out and with the above approach I'm getting the same result 🙌🏼

I had two small questions -

• Will setting the position twice not have any performance hits?
• And with this proposal, you'll just wish to remove the originX and originY property and not the functionality of setting position by origin itself right?

[asturur]

you do not need to set position twice, you need to read the position before scaling and apply it back when you are done.
In your scale action yes there will be a little bit more calculation, probably neglegible. That calculation today happens on every render for every object in every moment, even when you don't need it. That is why i was thinking of removing.

And yes we want just to remove the originX and originY customization per object, not the set of funcitons to position objects by a determined control point

[viveknigam3003]

Got it! This sounds great. Thanks a lot 😄

[chinyifei]

[asturur]

please open a new discussion this is out of topic

[vadimcoder]

I am for removal. All those LEFT, RIGHT, BOTTOM, TOP are unnecessary overconfiguration, and this behavior can easily be archived by something like setPositionByOrigin when needed. The problem with .left/.top remains but not super important I think.