search

CreateJS / EaselJS

The Easel Javascript library provides a full, hierarchical display list, a core interaction model, and helper classes to make working with the HTML5 Canvas element much easier.
http://createjs.com/
MIT License
8.11k stars 1.97k forks source link

Fix skew handling #1006

Open msand opened 5 years ago

msand commented 5 years ago

https://drafts.csswg.org/css-transforms/#SkewXDefined

gskinner commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

gskinner commented 5 years ago

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

gskinner commented 5 years ago

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

msand commented 5 years ago

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.