Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix skew handling #1006

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix skew handling #1006

wants to merge 1 commit into from

Conversation

msand
Copy link

@msand msand commented Jan 9, 2019

No description provided.

@msand
Copy link
Author

msand commented Jan 9, 2019

@gskinner
Copy link
Member

Hi @msand - could you please provide a little more info on this pull request? What specific problems does it fix? How have you tested it? Does it change any outputs from Matrix2D? Thanks!

@msand
Copy link
Author

msand commented Jan 29, 2019

It changes the output for any use of skewX and/or skewY in append/prependTransform, and the skew method, as they were doing something quite different than skewing. Additionally, it removes redundant computations when using them.
I have a tailor made/stripped down version of it in use in react-native-svg: https://github.com/react-native-community/react-native-svg/blob/master/lib/Matrix2D.js
You can check that it's correct by doing the matrix multiplication by hand, and noticing that any factors of zero and one have been simplified away, otherwise it's equivalent.

@gskinner
Copy link
Member

I did some quick testing, and this gives wildly different results from the current code. This likely breaks compatibility with Animate (which is what the original Matrix2D was designed to facilitate), and will also break existing projects using EaselJS.

I'll try to do a test in the next little bit against Animate to confirm it's an issue.

My understanding of this PR is that it's intended to align our implementation with CSS transforms? I think that's worthwhile, but we'd need a way to approach that so that it doesn't break old content or compatibility with Animate.

@MannyC
Copy link
Contributor

MannyC commented Jan 30, 2019

This came up before in #548 where you suggested possibly creating css aligned versions of the methods, such as cssSkew

@gskinner
Copy link
Member

Ah, right! I knew this seemed familiar. Thanks @MannyC for the reminder

@gskinner
Copy link
Member

I'd be open to a modified PR that took that approach (providing CSS compatible methods)

@msand
Copy link
Author

msand commented Jan 30, 2019

Well when I was stripping it down, I noticed it had different results from the transform attribute from the svg spec, and that the redundant multiplications could be simplified away. So, I thought I would make a quick pr here to at least raise awareness among the maintainers here, in case you would like to act on it somehow. I'm not currently dependent on Easel changing these, just thought I'd share my observations. And, in general, it would be nice to have at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants