chore(): Matrix util cleanup #8894
Conversation
|
Build Stats
|
src/util/misc/matrix.ts
Outdated
| export function calcRotateMatrix( | ||
| { angle = 0 }: TRotateMatrixArgs = {}, | ||
| x = 0, | ||
| y = 0 | ||
| ): TMat2D { | ||
| const angleRadiant = degreesToRadians(angle), | ||
| cosValue = cos(angleRadiant), | ||
| sinValue = sin(angleRadiant); | ||
| return [ | ||
| cosValue, | ||
| sinValue, | ||
| -sinValue, | ||
| cosValue, | ||
| x - (cosValue * x - sinValue * y), | ||
| y - (sinValue * x + cosValue * y), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
The parser calcRotateMatrix and the old calcRotateMatrix are the only one i don't want to mix.
I m not sure the pivot point in our geometry is identical to the one in the svg context.
There was a problem hiding this comment.
I will add a test comparing this and rotatePoint
There was a problem hiding this comment.
take a look at 5d36428 with critical eyes
There was a problem hiding this comment.
i m not worried about that use case.
Those matrices were the building block of calcTransformMatrix.
I would like to have the calcRotateMatrix used for calcTransformMatrix, different from the function that has to only serve the SVG parsing abilities.
Is ok if the svg parser call the old calcRotateMatrix and then adds the missin 2 values and we name that parseSVGRotateTransformAttribute.
I don't know of svg notations that may need different operation to the one that we have and that are identical to the one we were already using, so scale/skew/translate do not bother me.
There was a problem hiding this comment.
I don't really understand
I think I proved it is identical to how we use origin and since there aren't other use cases seems ok.
But I understand the mistrust.
I do think it is useful outside the parsing context.
There was a problem hiding this comment.
i just want the pivot point, that was part of the svg parsing and nothing else, to stay part of the svg parsing.
I don't want to mix them up because we save a bunch of code and because if we pass 0,0 at the pivot point then the function is functionally he same.
Let's leave the previous calcRotateMatrix and the svg one divided.
We may want to change the svg parsing code independently from the canvas code.
There was a problem hiding this comment.
I still don't get your point. As a standalone util it is great, isn't it?
Are you worried it does more calculations than before? Is that the point?
We may want to change the svg parsing code independently from the canvas code.
Then we change it accordingly.
I have encountered a number of times the need of this util and had to use rotatePoint to figure out the translation. That is why I want it exposed.
There was a problem hiding this comment.
Well the 4 addition and 4 multiplication are also unwelcome an extra point to have divided in 2 stages that is all i m asking.
There was a problem hiding this comment.
can I just have an early return instead if the origin is 0,0?
| return matrices.reduce( | ||
| (acc, matrix) => multiplyTransformMatrices(acc, matrix), | ||
| iMatrix | ||
| ); |
There was a problem hiding this comment.
we are changing a reduce to reduce right here nad i don't understand why.
I m incline to say don't touche the current transformation code at all, is not a cleanup anymore at that point
There was a problem hiding this comment.
I agree.
Touched it and became uneasy but the math is correct and I double checked that there is a test to cover it.
This proves it is all right: https://en.wikipedia.org/wiki/Matrix_multiplication#Associativity
Notice the args order passed to multiplyTransformMatrices
| (product: TMat2D, curr) => | ||
| curr ? multiplyTransformMatrices(curr, product, is2x2) : product, | ||
| iMatrix | ||
| ); |
There was a problem hiding this comment.
maybe i don't understan reduceRigth but it seems to me we are inverting changing the order of multiplication of matrices.
There was a problem hiding this comment.
This is the right order.
All tests pass.
https://en.wikipedia.org/wiki/Matrix_multiplication#Associativity
Notice the args order passed to multiplyTransformMatrices
There was a problem hiding this comment.
i know the result doesn't change because i know you know math. ( on top of that i know that property too ).
I opened the article anyway to see if maybe there was explained a mathematical property that was the reason for this change, but i couldn't find one.
So i don't know why you think that is the most correct order.
Why do you think is important to change it? I always have express preferences for don't touch what is not getting measurably better and i think you know it by now.
( apart that given the flawed float system of js, changing mutliplication order of floats ( the one inside the matrices ) isn't exactly the same thing. It could be, but sometimes it won't be)
( and apart that most of developers, inclding me, when they see reduceRight, they have to go and check how ti differs from reduce )
There was a problem hiding this comment.
This is how it is done mathmatically
Because of Associativity it doesn't matter. I wasn't sure at the time of writing but now I am.
So it can be turned into reduce but IMO this is the correct manner.
There was a problem hiding this comment.
added a test and renamed
There was a problem hiding this comment.
I will better explain.
Without Associativity it would matter.
Meaning, that using reduce (and not reduceRight) multiplies left to right and is correct only because of Associativity.
Multiplying using reduceRight is how matrix multiplication is defined
There was a problem hiding this comment.
It is confusing but this is how we do it in fabric using concatation of multiplyTransformMatrices
There was a problem hiding this comment.
after much thinking i will write you what i think.
I think that even removed any personal preference, the rule of no gain no change should be easy to agree with for everyone.
There is no gain here ( just minor losses ) ( i can't even find references about one order being more canon that the other ) and just the will to have this change.
This was hours ago, now you take this reduceRight and you keep it, my time and mood is not worth this change.
When i get to this point i the never want to talk about it anymore so please help me there.
what name do you suggest? |
That works for me |
Motivation
continues #8881
Description
Matrix utils belong to
utils, not toparserIt is wrong to have
utilsimport fromparserfor the following:utilsshould be pure as possible (parsershould importutils, also for the day we make the parser not part of the main bundle, if that day comes)Changes
Gist
In Action