chore/fix(v6): prerequisites for Group

[ShaMan123]

same as #7710

BREAKING _getTransformedDimensions, now accepts an options object.
FIX: calcOCoords

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.26 |    76.65 |    86.4 |   82.99 |                                               
 fabric.js |   83.26 |    76.65 |    86.4 |   82.99 | ...,29773,29898,29978-30043,30166,30265,30482 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.27 |    76.68 |    86.4 |      83 |                                               
 fabric.js |   83.27 |    76.68 |    86.4 |      83 | ...,29773,29898,29978-30043,30166,30265,30482 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.27 |    76.68 |    86.4 |   82.99 |                                               
 fabric.js |   83.27 |    76.68 |    86.4 |   82.99 | ...,29769,29894,29974-30039,30162,30261,30478 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.24 |    76.59 |   86.34 |   82.97 |                                               
 fabric.js |   83.24 |    76.59 |   86.34 |   82.97 | ...,29768,29893,29973-30038,30161,30260,30477 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.18 |    76.56 |   86.34 |   82.91 |                                               
 fabric.js |   83.18 |    76.56 |   86.34 |   82.91 | ...,29754,29879,29959-30024,30147,30246,30463 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.17 |    76.52 |   86.34 |    82.9 |                                               
 fabric.js |   83.17 |    76.52 |   86.34 |    82.9 | ...,29754,29879,29959-30024,30147,30246,30463 
-----------|---------|----------|---------|---------|-----------------------------------------------

[asturur]

This change in theory breaks the control api, because is passing to the conrol position handler a different matrix depending if it is in a group or not.
I don't think that is a real issue since controls in group nowadays do not really work.

I think those changes are fine, but i want to understand why we are adding scaleX and scaleY to getTransformedDimensions.
I would be eventually more happy to remove any option from that and move the special calculations to a util.

also getTotalAngle needs a double check for edge cases.

Let me think about it.

The very important part of this PR is the matrix shuffling away from rendering and put inside calcOcoords, and that seems good to me.

[ShaMan123]

It actually fixes the control api, so yes, that's breaking :).

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.18 |    76.56 |   86.34 |   82.91 |                                               
 fabric.js |   83.18 |    76.56 |   86.34 |   82.91 | ...,29754,29879,29959-30024,30147,30246,30463 
-----------|---------|----------|---------|---------|-----------------------------------------------

[asturur]

What is fixing that in the api?

[ShaMan123]

I mean that the control api was receiving bad data, wrong position of controls.
Now it is correct.

[asturur]

i would expect this PR to fix this rendering here:

from the visual test: controlboxes with skewY, green is wrong and needs fix

[asturur]

Or at least we should have a unit test that changes output before and after the change.

Or someone could come in 6 months, put the matrices as they were before, thinking that are better in another position and we don't have anything that catches the regression.

If no output is changing, fine, we are just making the code more straight and more generic but we are not breaking any behaviour of fix a bug.

[ShaMan123]

If no output is changing, fine, we are just making the code more straight and more generic but we are not breaking any behaviour of fix a bug.

I didn't understand,
Did the visual test work before the change?
Are these objects inside a group?

[ShaMan123]

How do I run this test? Can you try merging this on top of v6! and viewing the result?

[ShaMan123]

The problem with oCoords is that the calculation doesn't take into account skewing.
In addition the bbox of the border is calculated in a different way and I'm pretty sure oCoords doesn't comply.
I don't understand what goes where.

[ShaMan123]

And I remember we said that the rendering of controls should be looked into deeply

[ShaMan123]

And even more we talked about dropping double coord system...
I don't really understand why the calculations of aCoords and oCoords are different (except from vpt)

[ShaMan123]

calcLineCoords is broken, now I see. I doesn't take into account the group
I am confused

[asturur]

What i m saying is just, why no tests are failing? If no tests fails either we miss a test that cover the case we are changing, or we are not changing oCoords.

To run that test you can do:

npm run test:visual:single test/visual/control_rendering.js

you can delete controls12.png from the golden folder and see if it gets created equal before and after building the lib.

[asturur]

And even more we talked about dropping double coord system... I don't really understand why the calculations of aCoords and oCoords are different (except from vpt)

They are different for historical reason.
oCoords existed initially. Zoom and pan did not exists.
A developer came after a bounty to implement pan/zoom.
oCoords got adapted to work on zoom and pan.
All the functions about intersection, contain and such were adapted for the zoom.
At that point to know where an object was on the canvas you had to update oCoords, and then remove the zoom/pan from it in order to get to absolute coordinates.
At that point aCoords ( absolute coords ) were added, to represent the 4 corners of the object, in design/canvas space, without the viewport.

The 4 corners of the object and controls mostly overlapped, until in fabric 4.0 the control api was introduced that freed the controls from static position.
But mouse targeting was still relying on oCoords to find the objects, so lineCoords were added, that are the oCoords version of the aCoords.

In theory lineCoords aren't necessary anymore and aCoords should do it after a proper refactor of the pointer interaction.

Calculations are today different because oCoords are completely custom and developer driven in theory and oCoords have to support padding, that is a pain, is the equivalent of the pain strokeUniform on controls. Because padding is independent from everything ( zoom, and transformations ) but needs to be added back and pre-transformed.

[ShaMan123]

Thanks for the explanation.
So I want to ask, are oCoords necessary as well? If they are good only for mouse interactions and we can transform (easily enough) the point into the canvas space is there any need for them? If they are needed shouldn't they be calculated from aCoords?
I have a more drastic idea/proposal. But a question first. Is this mess caused because of canvas transform?
I am beginning to think that canvas transform is a bad idea, too many cases to handle. However, implementing it using a Layer sounds relatively straight forward.
What I mean to say is maybe we should rethink the design of canvas. What should it do, what shouldn't it do and something else should and so on.
I believe the design spec got messed up because group was inadequate so it never came up as a solution (rather, a problem) and made everyone think of workarounds. Canvas transform seems to be such a workaround.
Think of a group/layer that goes in between the canvas and the objects as part of design. It will be in charge of transformations. The up side is that it leverages existing logic to so, object transform logic, and relieves canvas from that task. It's how it should be. SVG uses group like that, it's a building block. And I think we should do that as well.
Same goes for layout. I have opened issues for custom layout but the truth is that I can use the new group to achieve it.
I understand the need for incremental change. It's reasonable, maintainable, steady and healthy. But I don't want us to waste time on fixing hard, time-consuming logic instead of altering the design.

So for example we can decide, everything coming from outside of canvas doesn't enter until it is relative to canvas (and have 1 function that handles this transformation). This will imply positioning controls relative to canvas as well. I think it's the better way.

I can go on about design. I have a concept for canvas. So that devs can build there own flow for fabric with a custom stack of canvases and layers. Then rewriting top canvas will become unnecessary. The dev decides to push a canvas to stack and configures a task for it in the render cycle.

[ShaMan123]

This change in theory breaks the control api, because is passing to the conrol position handler a different matrix depending if it is in a group or not.

It doesn't really affect the control because I simply applied the group transform early on in calcOCoords instead of in the event handler

[asturur]

oCoords are probalby not necessary anymore, but i want to take care of that part of refactor, because it requires so much old stuff that i want to be sure you don't get stuck in an endless comment ping pong.

ViewportTransform i think is not messy, is a very simple concept camera view, the issue that arise are only for things that we don't want affected by the viewport transform. padding for example and other UI elements developers want to add.

I don't want to redesign everything is an unnecessary risk and a large undertake. For developer too.

Running a test with controls and then approving ( unless my test is the failing test )

[github-actions]

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.15 |    76.49 |   86.36 |   82.87 |                                               
 fabric.js |   83.15 |    76.49 |   86.36 |   82.87 | ...,29750,29875,29955-30020,30143,30242,30459 
-----------|---------|----------|---------|---------|-----------------------------------------------

[asturur]

i think the final drawing is the same:

and same test run on this code:

the drawing is broken in both cases, so we are not really breaking anything more

[ShaMan123]

oCoords are probalby not necessary anymore, but i want to take care of that part of refactor, because it requires so much old stuff that i want to be sure you don't get stuck in an endless comment ping pong.

Great