fix(): skewing controls #8380

ShaMan123 · 2022-10-17T08:02:27Z

Motivation

Skewing is buggy in some cases .
#8344 has introduced changes that break the skewing controls.

Description

Existing logic is brilliant! It used the distance between the pointer and the origin point to determine the size of the target's axis (twice the diff). Then it extracted skewing as follows:

$$ \displaylines{ x' = x + y \cdot shear_x \\ shear_x = \tan(skew_x) \\ skew_x = \arctan(\frac{x'-x}{y}) } $$

Apparently the problem lied in the calculation of width (x) when calculating the skewed height (y) when skewing y

Problems with the previous impl:

skewing in a group - qrDecompose seems to mess everything up (I think)
skewing an object with skewing set on the counter axis - ~~fixed in https://github.com/fabricjs/fabric.js/compare/fix-skewing-controls2~~ (thanks to @luizzappa pointing out that skewX depends on skewY but not the other way round), still buggy inside group
It assumes the change to size is linear to the change in pointer, which was correct until we fixed stroke bbox calculations. ~~I can't think of a decent way around this unless we don't calculate the projections while skewing.~~ This led me to seek a solution that compares against the last skewing state.

I have tried to come up with a different approach, relying purely on linear algebra.
If fabric move away from decomposition then we can look into that (see Gist).
This PR is intended eventually to work only with matrices, but as long as fabric relays on decomposition skewY will not work.
I think if we can live without decomposing we should (canvas API doesn't decompose), but that's a tale for another time.
Another thought I had is that in the case of skewing Y we know the value of skewX so that might be enough to alter the decomposition to return a skewY value, @asturur ?

Changes

Fixed the second skewing interaction - skewing is now maintained.
Fixed cross skewing

Gist - IGNORED, here for future use only

Calculating Shearing

We are looking for the shearing values of the transform matrix defined by the pointer offset.

$$ \displaylines{ \Delta shear= \begin{bmatrix} 1 & x \\ y & 1 \end{bmatrix}, size= \begin{bmatrix} w \\ h \end{bmatrix} \\ \Delta pointer= \begin{bmatrix} p_x \\ p_y \end{bmatrix}= \Delta size = \Delta shear \cdot size= \begin{bmatrix} 1 & x \\ y & 1 \end{bmatrix} \cdot \begin{bmatrix} w \\ h \end{bmatrix}= \begin{bmatrix} h \cdot x \\ w \cdot y \end{bmatrix} \\ \begin{cases} p_x=h \cdot x \\ p_y= w \cdot y \end{cases} \Rightarrow \begin{cases} x=\frac{p_x}{h} \\ y=\frac{p_y}{w} \end{cases} \Rightarrow \Delta shear= \begin{bmatrix} 1 & \frac{p_x}{h} \\ \frac{p_y}{w} & 1 \end{bmatrix} } $$

Calculating the new transform

The following solves for skewX, ~~same process is done for skewY~~ (need to account for cross axis skewing)
This logic is not used in the PR but is here in case we decide to set the transform matrix directly instead of decomposed values.

First we need to calculate the transform without the existing shearing:

$$ \displaylines{ t = \begin{bmatrix} a' & 0 \\ b' & d' \end{bmatrix} \\ \begin{bmatrix} a & c \\ b & d \end{bmatrix} = \begin{bmatrix} 1 & s \\ 0 & 1 \end{bmatrix} \cdot t = \begin{bmatrix} 1 & s \\ 0 & 1 \end{bmatrix} \cdot \begin{bmatrix} a' & 0 \\ b' & d' \end{bmatrix} = \begin{bmatrix} a' + s \cdot b' & s \cdot d' \\ b' & d' \end{bmatrix} \\ \begin{cases} a = a' + s \cdot b' \\ b = b' \\ c = s \cdot d' \\ d = d' \end{cases} \Rightarrow \begin{cases} s = \frac{c}{d} \\ t = \begin{bmatrix} a - s \cdot b & 0 \\ b & d \end{bmatrix} \end{cases} \Rightarrow t = \begin{bmatrix} a - \frac{c}{d} \cdot b & 0 \\ b & d \end{bmatrix} } $$

Apply the new shearing

$$ \displaylines{ \Delta shear_x= \begin{bmatrix} 1 & \frac{p_x}{h} \\ 0 & 1 \end{bmatrix} \\ t'_x = \Delta shear_x \cdot t= \begin{bmatrix} 1 & \frac{p_x}{h} \\ 0 & 1 \end{bmatrix} \cdot \begin{bmatrix} a - \frac{c}{d} \cdot b & 0 \\ b & d \end{bmatrix} = \begin{bmatrix} a + (\frac{p_x}{h} - \frac{c}{d}) \cdot b & \frac{p_x}{h} \cdot d \\ b & d \end{bmatrix} } $$

Doing the same for skewY results in:
WRONG - we must figure out how to extract scaleX/skewX

$$ \displaylines{ s = \frac{b}{a} \\ \Delta shear_y= \begin{bmatrix} 1 & 0 \\ \frac{p_y}{w} & 1 \end{bmatrix} \\ t = \begin{bmatrix} a & c \\ 0 & d - \frac{b}{a} \cdot c \end{bmatrix} \\ t'_y= \Delta shear_y \cdot t = \begin{bmatrix} a & c \\ \frac{p_y}{w} \cdot a & d + (\frac{p_y}{w} - \frac{b}{a}) \cdot c \end{bmatrix} } $$

Another approach:

$$ \displaylines{ T = T_{translate} \cdot T_{rotate} \cdot T_{scale} \cdot T_{skewX} \cdot T_{skewY} \\ T_{no skew} = T_{translate} \cdot T_{rotate} \cdot T_{scale} = T \cdot T_{skewY} ^{-1} \cdot T_{skewX} ^{-1} \\ T_{apply Skew}(skewX', skewY') = T_{no skew} \cdot T_{skewX'} \cdot T_{skewY'} \\ } $$

I have managed to get skewX to work with both approaches. SkewY however...

TO DO

verify skewing one axis with existing skew value on the other
origin switching: change or account for size when skewing against direction to fit the mouse more accurately
skewing inside group

In Action

asturur · 2022-10-17T11:30:08Z

let's address the `i don't trust qrDecompose` first:

The fact you don't trust qrDecompose is because you don't accept that that has a narrow scope.
Given a series of transformations done in fabricjs order, or in an order that is equivalent, you obtain a matrix. qrDecompose is going to give you a set of EQUIVALENT parameters that represent that transformations if you apply them again in fabricJS order.
It can't tell you more because everything as been mashed together.
if i tell you 6, you can answer me with 5 + 1, 3 * 2, 6, 4 + 2. Are all valid answers and i can't pretend you guess the original one.
That is what qrDecompose is doing and is doing it good.

qrDecompose is a pillar of fabricJS geometry and a constrain, and we can't avoid using it, it would come at the cost of a lot of calculations when skew is involved.

Let's give the proper introduction to those bugs

Skew controls, compared to other controls, have an harsh logic.
They have visual bugs when both skewX and skewY are mixed together.
Skewing on both axis is an edge case and our brain difficultly wraps around it.

Which is this always buggy behaviour?
Let's start from the non buggy one.
Take a rectangle and skew it along one axe only, X or Y.

If you press down shift and start skewing, you will see that with one skew only, the mouse always follow the control and all is good, you don't ecounter surprises.

When both skew are used, you will see that the skew controls start to move at a different speed compared to the mouse cursor. this becase the calculation that are there can't reverse engineer this question:
Given an object with

angle A,
scale 4,2
skewX : 33
skewY: 12
taken the mouse at position x,y on the control for skewY
moving the mouse to x1 = x +3 and y1+ y+1
what value i have to give to skewY in order for the control to be the closest to the mouse cursor but still on the line determined by the side of the bounding box where the skew control is located?

I didn't answer that question, i didn't even try after a couple of attempts and we should be limited to pay a little price in code complexity for that to work.

Probably to improve the situation one thing to look at is to determine why incremental skew at every mouse move diverge so much. Is also non linear, because some combinations of skewX and skewY are just a rotation, and that means that this skewY control is going to move along the mouse as a sort of function of the angle too.

Maybe the skew control needs to just add and remove skew to the object withtout trying to reduce the calculation at the different sizes of the bounding boxes with and without

The issue with skew controls is the predictability of the next value of skew to assign when both skew are involved

how does inkscape handle it?

Inkscape use qrDecompose more.
If i remember correctly after skewing they reset the object transform with a sort of an equivalent trasnform and kill one of the skews. In practice i think they they use the equivalent of qrDecompose MORE.

Can we see how other interactive tool solve this issue with double skewing before trying to fix the code?

I ask this because we can't pursue a bug that is generated by a UI choice without first validating that the UI choice is good.
Maybe the fix is small, maybe is complicated and a time sink, but is not a bug in the skewing logic itself but on the expectations we set on the mouse movements and boundig box reactions.
I want we look at something similar first and see how it gets handled visually

Example of things i don't want too see or talk about

a binary search in future skew values that will make the mouse and the control conincident
(i ll add more if i can think of it)

src/controls/controls.actions.ts

ShaMan123 · 2022-10-17T17:16:30Z

Great context.
Thank you! Much appreciated.
What I saw after reading the skew controls (when I opened the PR) is that as you mentioned it doesn't start from the current skew state but starts from 0.
That seems a bug to me. That is why skew jumps back/flickers once you do a mouse down or use setDimensions

a binary search in future skew values that will make the mouse and the control coincident

they use the equivalent of qrDecompose MORE.

Can't guess what these mean.
What is the equivalent of qrDecompose MORE?

how does inkscape handle it?

Could you add visuals?

asturur · 2022-10-17T19:44:59Z

i need to install inkscape and record but i think is something to compare with and better than nothing.
I don't have adobe tools but maybe they do something as well.

I think that Inkscape normalize the bounding box each transformation to reduce issues and complexity, that is what i meant with use qrDecompose more. It flattens the transformation to remove one skew factor.

a binary search in future skew values that will make the mouse and the control conincident
^^^ I meant don't start search the correct skewY value exahustively jumping from 90 deg to 0, in order to guess the correct one, because that is a solution, but i don't want to use it

asturur · 2022-10-17T19:56:53Z

Inkscape normalize the bounding box on rotation and not in skew.
They get double skewing right, meaning no JUMPS, but they also have the control desynced with the mouse.
Probably jumps are removed if we remove the dependency from the bounding box size.
We do guess for the current transformation how much skew should be added for a mouse movement and we add eithout trying to match any position.

That would probably remove weird jumps entirely

progress progress(): position major progress fix(): flip cleanup no multiply good state! fix(): successive skewing cleaup

ShaMan123 · 2022-10-18T12:17:51Z

I have done a major refactoring that will probably enable not using qrDecompose for skewing (in case one day qrDecompose will fade out and matrix will be the only thing defining object transform).
Fixed the second skewing interaction - skewing is now maintained.
I will add an explanation soon.

Please check the interaction for accuracy.
I will need help to finalize skewY

ShaMan123

Updated description
origin switching is weird IMO now that successive interactions are supported
~~skewY after skewX is broken~~ seems to be FIXED

Please review

ShaMan123

fixed tests

ShaMan123 · 2022-10-18T16:55:22Z

src/shapes/object.class.ts

+          this.set('scaleX', scaleX);
+          this.set('scaleY', scaleY);
+          this.angle = angle;
+          this.skewX = skewX;


this is a case that you warned from
I tried calling set instead of manual assignment but since polys run a side effect this breaks.
I can set initialized=false so it won't run.

intialized false has to go away, do not make it more necessary that it is now.

ShaMan123 · 2022-10-18T16:58:50Z

The fact you don't trust qrDecompose is because you don't accept that that has a narrow scope.

To me that is dangerous since I can't use it everywhere and for everything so I keep my guards up.
IMO (could be a matter of taste) remembering such details is an overkill and in general feels fragile.
I avoid impl that don't cover all usable cases. And if by chance I must I rename them as DANGEROUS_XXX

ShaMan123

READY after comments and extensive skewing with:

scaling
flipping
rotating
initial skewing
double skewing
vpt
successive skewing in all the above

ShaMan123 · 2022-10-27T23:32:30Z

seems 91c5d72 fixed #8380 (comment) (files were displayed in B units, not KB)

asturur · 2022-10-28T06:58:38Z

Ok so i tried it, and the flip behaviour is changed.
Is not the end of the world but i m trying to fix it, with few result.
If i manage i ll add a PR over thi PR or i ll let it go

You can compare a single flip target with master to see that some direction changes when skewing and it just feels innatural.

TS provided! I started typing controls and it alerted the flip doesn't exist on `transform` so I found this bug that otherwise what have stayed hidden

ShaMan123 · 2022-10-28T07:03:27Z

So funny
I just found the bug and pushed the fix and then instantly received your comment
Try now!

ShaMan123 · 2022-10-28T07:04:44Z

You can compare a single flip target with master to see that some direction changes when skewing and it just feels innatural.

If this continues please share a screen cast

ShaMan123 · 2022-10-28T07:05:37Z

src/controls/controls.actions.ts

+
+    const finalHandler = wrapWithFireEvent(
+      'skewing',
+      wrapWithFixedAnchor((eventData, transform, x, y) =>


Do you think this affects perf?

asturur · 2022-10-28T07:07:30Z

Going to retry now, i was literally adding -1 and 1 randomly around with the flipX !== flipY logic

still remaining one edge case

ShaMan123

It is fixed now
But I have seen another related bug that exists on master as well
I think it happens only when skewing Y and when skewY * skewX < 0, then target is kind of flipped

In the video I am pulling the mr corner

Parcel.Sandbox.-.Google.Chrome.2022-10-28.11-15-18.mp4

asturur · 2022-10-28T08:51:51Z

yes now it behaves more consistently.
Any other eventual change in behaviour is irrelevant for me.

Good pr!

@luizzappa

Co-authored-by: @luizzappa

ShaMan123 added 3 commits October 12, 2022 10:50

WIP

ed66b81

Merge branch 'master' into fix-skewing-controls

0e5305a

Merge branch 'master' into fix-skewing-controls

c4633cb

ShaMan123 changed the title ~~fix() skewing controls~~ fix(): skewing controls Oct 17, 2022

ShaMan123 added the Work in progress label Oct 17, 2022

asturur reviewed Oct 17, 2022

View reviewed changes

src/controls/controls.actions.ts Outdated Show resolved Hide resolved

ShaMan123 added 2 commits October 18, 2022 15:10

fix skewing

e30dd32

progress progress(): position major progress fix(): flip cleanup no multiply good state! fix(): successive skewing cleaup

Update controls_handlers.js

465149f

ShaMan123 added 3 commits October 18, 2022 15:34

Update CHANGELOG.md

95b8b39

fix(): use set

06cdb64

Update object.class.ts

793df67

ShaMan123 added fix-next and removed Work in progress labels Oct 18, 2022

ShaMan123 added 2 commits October 18, 2022 17:54

Update controls.actions.ts

c6f000d

ws

d4996d1

ShaMan123 commented Oct 18, 2022

View reviewed changes

ShaMan123 added 5 commits October 18, 2022 18:30

fix(): flip assignment

606df66

goldens

cdc2c5e

revert(): svg stuff

c1f21d1

cleanup

aa48a3b

Update object.class.ts

4e84453

ShaMan123 mentioned this pull request Oct 18, 2022

fix(#8344): stroke projection #8374

Merged

ShaMan123 commented Oct 18, 2022

View reviewed changes

fix(): restore compensateScaleForSkew

5ee4741

ShaMan123 added 10 commits October 28, 2022 01:13

Update controls.actions.ts

e56d1f1

better comments

bc6e514

pass down skewingSide

6b11e11

comments

7753d72

distanceFromOrigin

e68d0c2

Update controls.actions.ts

0f7a201

Update controls.actions.ts

1a3146c

Update controls.actions.ts

d9c5bd5

fix(): counter flip

1ba2724

fix

43a5657

ShaMan123 requested a review from asturur October 27, 2022 23:02

ShaMan123 commented Oct 27, 2022

View reviewed changes

Update buildStats.mjs

91c5d72

fix: flip

f703f27

TS provided! I started typing controls and it alerted the flip doesn't exist on `transform` so I found this bug that otherwise what have stayed hidden

ShaMan123 commented Oct 28, 2022

View reviewed changes

ShaMan123 added 2 commits October 28, 2022 10:32

fix(): flipped

21c4dd1

still remaining one edge case

Update controls.actions.ts

7ba8f2e

ShaMan123 commented Oct 28, 2022

View reviewed changes

asturur approved these changes Oct 28, 2022

View reviewed changes

asturur merged commit d3e09e7 into master Oct 28, 2022

asturur deleted the fix-skewing-controls branch November 1, 2022 23:37

ShaMan123 mentioned this pull request Nov 7, 2022

Decode transform matrix #8425

Closed

frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023

fix(): skewing controls (fabricjs#8380)

27808bb

Co-authored-by: @luizzappa

ShaMan123 mentioned this pull request May 14, 2023

feat(Brush API): Observable, clipPath #8476

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(): skewing controls #8380

fix(): skewing controls #8380

ShaMan123 commented Oct 17, 2022 •

edited

Loading

asturur commented Oct 17, 2022 •

edited

Loading

ShaMan123 commented Oct 17, 2022 •

edited

Loading

asturur commented Oct 17, 2022

asturur commented Oct 17, 2022

ShaMan123 commented Oct 18, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 left a comment

ShaMan123 Oct 18, 2022

asturur Oct 20, 2022 •

edited

Loading

ShaMan123 commented Oct 18, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 commented Oct 27, 2022 •

edited

Loading

asturur commented Oct 28, 2022

ShaMan123 commented Oct 28, 2022 •

edited

Loading

ShaMan123 commented Oct 28, 2022

ShaMan123 Oct 28, 2022

asturur commented Oct 28, 2022

ShaMan123 left a comment •

edited

Loading

asturur commented Oct 28, 2022

fix(): skewing controls #8380

fix(): skewing controls #8380

Conversation

ShaMan123 commented Oct 17, 2022 • edited Loading

Motivation

Description

Changes

Gist - IGNORED, here for future use only

Calculating Shearing

Calculating the new transform

TO DO

In Action

asturur commented Oct 17, 2022 • edited Loading

let's address the i don't trust qrDecompose first:

Let's give the proper introduction to those bugs

how does inkscape handle it?

Example of things i don't want too see or talk about

ShaMan123 commented Oct 17, 2022 • edited Loading

asturur commented Oct 17, 2022

asturur commented Oct 17, 2022

ShaMan123 commented Oct 18, 2022 • edited Loading

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Oct 18, 2022

Choose a reason for hiding this comment

asturur Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

ShaMan123 commented Oct 18, 2022 • edited Loading

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

ShaMan123 commented Oct 27, 2022 • edited Loading

asturur commented Oct 28, 2022

ShaMan123 commented Oct 28, 2022 • edited Loading

ShaMan123 commented Oct 28, 2022

ShaMan123 Oct 28, 2022

Choose a reason for hiding this comment

asturur commented Oct 28, 2022

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

asturur commented Oct 28, 2022

ShaMan123 commented Oct 17, 2022 •

edited

Loading

asturur commented Oct 17, 2022 •

edited

Loading

let's address the `i don't trust qrDecompose` first:

ShaMan123 commented Oct 17, 2022 •

edited

Loading

ShaMan123 commented Oct 18, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

asturur Oct 20, 2022 •

edited

Loading

ShaMan123 commented Oct 18, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 commented Oct 27, 2022 •

edited

Loading

ShaMan123 commented Oct 28, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading