svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.04k stars 1.08k forks source link

SVG.Matrix.scale() works differently than DOMMatrixReadOnly.scale() #1150

Closed mroloux closed 3 years ago

mroloux commented 4 years ago

When you pass in 4 arguments to SVG.Matrix.scale(), the resulting array is different than when using DOMMatrixReadOnly.scale().

Not sure if this is a bug, or it's me who doesn't understand what the cx and cy arguments of SVG.Matrix.scale() represent. But anyway:

new SVG.Matrix(2, 2, 2, 2, 2, 2).scale(10, 10, 10, 10)
=> Matrix {a: 20, b: 20, c: 20, d: 20, e: -70, f: -70}

new DOMMatrixReadOnly([2, 2, 2, 2, 2, 2]).scale(10, 10, 1, 10, 10) // third parameter of '1' is the z-scaling
=> DOMMatrix {a: 20, b: 20, c: 20, d: 20, e: -358, f: -358}

Here's the documentation for DOMMatrixReadOnly.scale(): https://developer.mozilla.org/en-US/docs/Web/API/DOMMatrixReadOnly/scale .

Fuzzyma commented 4 years ago

SVG does not use Dom matrices. Furthermore we have an own matrix implementation which is documented in the docs. It has nothing to do with anything the browser provides. 4 arguments are for scaleX, scaleY, originX, originY

mroloux commented 4 years ago

Sure, but in which way do originX and originY then differ from the arguments that you pass in to DOMMatrixReadOnly.scale()? That method also takes an originX and originY.

BTW: I tried the Matrix.scale() implementation from Raphael.js. That one produces consistent results with DOMMatrixReadOnly.scale()

Fuzzyma commented 4 years ago

I dont know what DomMatrix does but e and f is the x and y translation. When you have a matrix of (222)(222) and scale it by 10 with origin at 10, 10, what you do is translating the matrix to its origin (say make e and f equaling the origin), do the scale, and move it back by the same amount. No way -358 is a correct value for that operation. While -70 is:

 2 - 10 = -8 // translating to origin
 -8 * 10 = -80 // scaling
 -80 + 10 = -70 // moving back

Since the matrix (222)(222) is singular it is possible, that implementations calculate wrong numbers. Using a different matroix (like 1, 0, 0, 1, 10, 10) works as expected

mroloux commented 4 years ago

Thanks! You're right, the math adds up. Not sure why DOMMatrixReadOnly is behaving differently 🤔

At least I learned a lot about matrix math 😆

ibrierley commented 4 years ago

I think SVG.js is wrong here for what it's worth, I note in previous versions 2.5.0 for example I think it had it right, depending if it's trying to follow the SVG spec. -358 is correct. Here's my explanation.

To implement a translate on an existing matrix, we can't simply x,y add to e,f. We have to account for the existing matrix transformations.

Eg Suppose we have a scale of 2 on the X, eg a matrix of (2,0,0,1,0,0), when we do a translation, the end result of any translation will be 2x that given translated x value. So a translation of 10 on that, should give an e of 20, not 10.

So we have to account for those existing transformations. I think the correct implementation for a translation would be... e = e + xa + yc f = f + xb + yd;

So given the original question with a matrix of (2,2,2,2,2,2) and scale of (10,10,10,10) we would get translate, 2 + 10 2 + 10 2 = 42 scale, so matrix becomes (20,20,20,20,42,42) translate back, 42 + -10 20 + -10 20 = -358.

To highlight this...here are some examples, Firstly a normal SVG markup that I assume we want to replicate...examples just with a basic cube to highlight, I've reduced the matrix so it fits in a easier visible box. So matrix of 1.2,0.2,0.2,1.2,-85,-85 and translate of -2,-2

https://jsfiddle.net/rco1yx2m/1/ svg markup https://jsfiddle.net/a7uvxc8o/ svg.js 2.5.0 gets it correct, I've also console logged the scale of 2 example I mentioned earlier showing e of 20 https://jsfiddle.net/ub6yca05/ svg.js 3.0.16 goes astray, and has an e of 10

ibrierley commented 4 years ago

Just updated the jsfiddles to new ones, as I hadn't saved the final scale of 2 console check.

saivan commented 4 years ago

Well the question isn’t about correctness, it’s about usability. We have a relative flag that should do what you’re asking, but for a user, it’s much simpler to not have to think about the current transformation

Fuzzyma commented 4 years ago

To elaborate on saivans answer:

It depends on how you view the passed origin. If you handle the origin as a point in the transformed system then yes, you are right. However, when you view the origin as an untransformed point than we are correct. Our matrix implementation doesn't follow any specs but instead follows what is more usable throughout the library. And we found that having it this way works better for us

saivan commented 4 years ago

Also to elaborate further, here's a fiddle :D

https://jsfiddle.net/saivanh/z5xw9dpe/1/

We apply matrices in the opposite order, because for a user, that is the more natural thing. There is a flag to apply them in the standard order as well, but as I said, for a new user - this is not the "expected" behaviour. It's much simpler for later transformations to be independent of earlier ones.

ibrierley commented 4 years ago

Fair enough, I think that explains the difference for anyone who stumbles upon it anyway. We all have different things we find intuitive :).

saivan commented 3 years ago

I agree. But the library adopts a simplicity first approach to api design. So by default, any applied transformation is applied to the image “as is”. If your basis is in some random direction, you don’t need to worry about that if you just want to move your image to the right by 30 units 😛

If you do want to apply transformations relative to your basis, which is a totally useful thing to do, use the relative flag :) I’ll close this for now, but if you have anything else you’d like us to look at, just let us know.

ibrierley commented 3 years ago

Not interested in reopening...I think there's a bit of confusion (my fault with jsfiddles were a redherring) but if performing on the Matrix (rather than an element), there isn't a relative flag is there ?

Eg the original example.. new SVG.Matrix(2, 2, 2, 2, 2, 2).scale(10, 10, 10, 10), is there an equivalent which takes into account the existing matrix ?

Fuzzyma commented 3 years ago

You can use the transform method to do any of the transformations and there is where you can also pass the relative flag. We also have multiply and lmultiply in case it is useful for you.

SVG.Matrix(2, 2, 2, 2, 2, 2).transform({scale: 2, origin: [2,2], relative: true})