Point type fix

[Lazauya]

This makes use of the existing IPoint type by forcing things to implement it & replacing use of Point where applicable. If a function doesn't make use of any of the methods in Point, then we can safely replace it with IPoint. This improves generality and speed, because developers don't have to wrap every time they do { x: 10, y: 9 } in a more costly constructor.

[asturur]

This is something i wanted to discuss but i didn't have time.
Before it was a mixed bag of sometimes is a point sometimes is not.
I'm also on the alert usually with more costly constructors and do i need a class if i can do it with an object?

Is also true that developers do need stability, sometimes they just want to know if they will get a point or not, and converting points to new points is wasted time too.
So lately i softened myself to, i can't benchmark a speed difference isn't woth.

Can we benchmark the costly constructor compared to the object literal?
In speed or memory consumptions, GC time.
If we can't benchmark a difference in performance then is not worth imho.

[asturur]

To be clear, in this case there is the issue that if we accept IPOINT, and then tomorrow we need a POINT, we can't because it becomes a breaking change, so we add the conversion inside the function, going back to the point in which the extra code now is executed by everyone, or we need to add checks like, is this a Point? fine, othewise new Point from IPoint.

[Lazauya]

The speed improvement is only a circumstantial benefit IMO. It's more about ease of use. I can't imagine a scenario (right now, at least) where we'd only need an IPoint then need to change it to Point later on. If that DOES happen, then it's fine to just eat the cost of constructing the Point since it probably won't change much performance wise. After all, there are plenty of functions that already call the Point constructor. For a user of the API, I think it's more annoying to do new Point(x, y) instead of just doing { x, y }, and it just so happens to have the side effect of being a little faster.

[ShaMan123]

To be clear, in this case there is the issue that if we accept IPOINT, and then tomorrow we need a POINT, we can't because it becomes a breaking change, so we add the conversion inside the function, going back to the point in which the extra code now is executed by everyone, or we need to add checks like, is this a Point? fine, othewise new Point from IPoint.

This is very good point. Same as chainability.

@Lazauya the constructor is less annoying since you can do new Point({ x, y }) now.

I find it hard to believe there will be a significant or even a minor change to performance.
All major modern repos use classes for everything, AFAIK.

I want to raise my concern. This damages the simplification of core and of usage, which is one of my goals for the TS migration and therefore I am against.
Think of it, for one method you need to pass Point, for another method you can pass IPoint. It is a messy approach and is too much for the dev IMO, for me at least. I will use Point all the time.
Think of the following block:

const pointObj = { x, y }
IPointMethod(pointObj)  // great all good
PointMethod(pointObj)   /// ohh no, multiply doesn't exist on object ....

I wanted to suggest moving everything to Point internally as part of the discussion, we talked about that as well. All private/protected/internal methods should run with Point IMO since fabric calls them probably with a Point, we didn't check that and we should. If a method only uses IPoint but is called only with Point what do we gain?

[ShaMan123]

After checking the code, the following are pointless (haha, but they are pointful):

• brushes receive a Point from canvas, the rest of the methods are internal, use the received point, and should be marked as protected
• intersection uses only Point
• controls position handler the same
• vectors are only Point since we construct them from Point.subtract

I am for accepting IPoint in Point methods, that makes sense IMO

[ShaMan123]

@Lazauya I hope I didn't discourage you from contributing
You can ping me if you have something in mind you want to work on.

[Lazauya]

@Lazauya I hope I didn't discourage you from contributing
You can ping me if you have something in mind you want to work on.

No worries, I've just been away for the holidays and stuff and haven't been able to code recently.

[ShaMan123]

No worries, I've just been away for the holidays and stuff and haven't been able to code recently.

Doesn't sound too bad 😉

[Lazauya]

After checking the code, the following are pointless (haha, but they are pointful):

• brushes receive a Point from canvas, the rest of the methods are internal, use the received point, and should be marked as protected
• intersection uses only Point
• controls position handler the same
• vectors are only Point since we construct them from Point.subtract

I am for accepting IPoint in Point methods, that makes sense IMO

I agree and I rolled back most of my changes. I kept changes to the Point class and utils/vector.ts and utils/matrix.ts. Thoughts?

[ShaMan123]

src/util/misc/vectors.ts can be reverted as well since all vector logic is done with Point as well