fix(v6): 4th PR of Group Rewrite 🎛️ nested controls 😜 #7861

ShaMan123 · 2022-04-04T18:39:11Z

This PR fixes control rendering.
There are a few unresolved comments in the original PR, please take a look.

Main Changes

commit

This PR ports #7758 to master

Visuals

controls12.png
controls13.png

#7744 (review)

in case object is a collection

ShaMan123 · 2022-04-09T05:42:27Z

@melchiar the changes you are suggesting belong to #7859

src/shapes/object.class.js

src/mixins/collection.mixin.js

src/shapes/group.class.js

melchiar · 2022-04-09T06:22:29Z

@melchiar the changes you are suggesting belong to #7859

ugh...just saw this now

ShaMan123 · 2022-04-09T06:24:16Z

Continue there, I'll patch up what you've suggested later on.
Regarding nesting if statements I don't really like it. You prefer it because of perf?

melchiar · 2022-04-09T06:26:32Z

Continue there, I'll patch up what you've suggested later on. Regarding nesting if statements I don't really like it. You prefer it because of perf?

I try to avoid multiple checks for the same conditions.

ShaMan123 · 2022-04-09T06:28:22Z

I dislike nesting logic, I prefer it flat as possible

melchiar · 2022-04-09T06:35:59Z

fair enough. this is excellent by the way 👍

stale · 2022-04-25T00:41:34Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2022-05-01T00:05:10Z

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.04 |    75.97 |    85.7 |   82.76 |                                               
 fabric.js |   83.04 |    75.97 |    85.7 |   82.76 | ...,30844,30918,30929-30994,31117,31216,31452 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur · 2022-05-01T00:08:34Z

Catching up with this.

Some logic is leaking between PRs. this belongs to #7861

asturur · 2022-05-01T23:55:54Z

src/mixins/object_geometry.mixin.js

-      return util.calcRotateMatrix(this);
+    _calcRotateMatrix: function (absolute, accountForFlipping) {
+      var angle = absolute ? this.getTotalAngle() : this.angle;
+      accountForFlipping && this.flipX && (angle -= 180);


i wonder how nested flippings behave.

I wonder/forgot why I accounted for flipX only for nested objects, why not for all objects?

It's bad. Good catch.
Isn't flipX/flipY a z plane transformation? If so wouldn't it be better to multiply 3d matrices?

rotation as used by fabric is a 2d plane transformation. So it shouldn't relate to z plane transformations. I think this is the cause of the problem (in all flipX/Y logic)

What about ml-matrix?
https://mljs.github.io/matrix/classes/QrDecomposition.html

@asturur Thoughts?

https://codepen.io/ktamas77/pen/AXmmeX?editors=0010

I guess we can use matrix projection from 3d -> 2d...

But it opens up 3d and I'm not sure we want that.
So I vote to remove flip props as they are broken

https://en.wikipedia.org/wiki/Orthographic_projection

https://jsfiddle.net/7wdbq1vL/1/

src/mixins/object_geometry.mixin.js

remove `_calcRotateMatrix`, `_calcTranslateMatrix`

github-actions · 2022-05-02T05:09:40Z

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.05 |    75.98 |   85.69 |   82.78 |                                               
 fabric.js |   83.05 |    75.98 |   85.69 |   82.78 | ...,30850,30924,30935-31000,31123,31222,31458 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur · 2022-05-02T08:00:52Z

src/mixins/object_geometry.mixin.js

+      var vpt = this.getViewportTransform(),
+          center = this.getCenterPoint(),
+          tMatrix = [1, 0, 0, 1, center.x, center.y],
+          rMatrix = util.calcRotateMatrix({ angle: this.getTotalAngle() - (!!this.group && this.flipX ? 180 : 0) }),
+          positionMatrix = multiplyMatrices(tMatrix, rMatrix),
+          startMatrix = multiplyMatrices(vpt, positionMatrix),
+          finalMatrix = multiplyMatrices(startMatrix, [1 / vpt[0], 0, 0, 1 / vpt[3], 0, 0]),
+          transformOptions = this.group ? fabric.util.qrDecompose(this.calcTransformMatrix()) : undefined,
+          dim = this._calculateCurrentDimensions(transformOptions),


Just to be clear, i m sure there are bugs with angle compensation but is not important. This PR improves the situation anyway.

(!!this.group && this.flipX ? 180 : 0)
I can't imagine this is good enough for sure.

asturur · 2022-05-02T08:02:15Z

For the reader of the future.
If the controlbox of an object appear different when is inside a group from when is outside, that is probably possible and is not considered a bug. As long as controls works in both situation that is considered an acceptable quirk.

ShaMan123 · 2022-05-02T08:43:09Z

For the reader of the future. If the controlbox of an object appear different when is inside a group from when is outside, that is probably possible and is not considered a bug. As long as controls works in both situation that is considered an acceptable quirk.

😨

I see it as a bug in qrDecompose. Fixing it means moving to 3d transformation matrix or dropping flipX/Y for good (I think that's not a bad idea - very strange feature)

For the reader of the future - We are sorry this is how it is. STAY AWAY from flipX/Y.

asturur · 2022-05-03T12:56:18Z

flipX and flipY are totally normal, and are just an abstraction for a negative scale that is totally normal too.
You can't propose someone to have nested groups of different types but not being able to flip an image, is a basic feature imho.
qrDecompose isn't bugged in a strict sense, it just does what it can do.
As we talked before we have more trasform variable than 6 numbers and we don't have as many relationship between numbers.

is like when you get a 6, was it 3 x 2 or 6 x 1? you can't know.

I m not sure a different matrix would help, unless rotation and scale occupy different cells in the matrix.

[scaleX * cosA] [ -sinA ] [translateX]
[sinA] [scaleY * cosA] [translateY]

idependently from the fact that the mulitplication between trasnformation make everything even more complicated, the biggest lie is in the fact that the position 'a' and 'd' of the transform matrix are occupied by scale ( and so flip ) and a part of the rotation. with the cosine that can be negative or positive and the scale too, once you have those multipled together, if you decompose a matrix without knowing how the object ( and the parents ) are flipped or not flipped, you can't get out of it.
Oh and skewX and skewY are also there somewhere together with rotation. I just see more variables than equations between them i can think of.

ShaMan123 · 2022-05-03T19:45:06Z

flipX and flipY are totally normal, and are just an abstraction for a negative scale that is totally normal too.

Is it negative scale or 180deg rotation around the x/y axis? Or are the 2 cases the same thing?

Oh and skewX and skewY are also there somewhere together with rotation

Confusing indeed. However the characteristic polynomial might serve as an answer in this case (not sure about it though) 'cause if a matrix is multiplied by a rotation matrix it doesn't have a solution for the characteristic polynomial. Meaning that we can check to see if there is a solution and if so we know the angle was 0. And I read about extracting a rotation matrix be it's heavy stuff.
ShaMan123@22e472b

Did you look at ml-matrix?

Apart from qr decomposition we can do something with eigenvalues that might help determine the rotation...

I really don't know enough linear algebra to solve this.

I just see more variables than equations between them i can think of.

Yes, Is true.

ShaMan123 added 30 commits April 3, 2022 23:32

apply 7744 patch (no binaries)

1a191a7

apply 7744 patch (binaries)

26e8392

commit #7744 review

7d793b3

#7744 (review)

patch2

2708029

Update spray_brush.class.js

02a8ffa

enable text editing under group

0d3690f

migrate back to isDescendantOf

ff0346a

render bg

812f02a

fix(): clear canvas from entire object tree

053946c

in case object is a collection

fix(): center object in case object is nested

db76c44

Update eraser_brush.mixin.js

04aad5b

Update canvas.class.js

8c1c535

Update canvas_events.mixin.js

3fd4af2

lint

58e9be2

safeguard from adding group to itself

c2ef2f5

Update object_ancestry.mixin.js

f0145b5

patch tests from v6!

cb20b81

Update canvas_grouping.mixin.js

347f80d

Update canvas.class.js

a56d55e

Update group_layout.js

2a426e7

Update group_layout.js

5e3fc1f

remove redundant logic

2b1f680

Update canvas_grouping.mixin.js

1ad5cc8

fix(): calling sendPointToPlane

11c828f

Update group.class.js

b6fa3dc

refactor enterGroup

e9afc89

invalidate groups after active selection change

d8f8251

_shouldSetNestedCoords

c58a274

perf(): refrain from contains

c0f5571

Update canvas_events.mixin.js

9a223d0