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

Matrix2D not translating the same as previous versions #539

Closed
mcfarljw opened this Issue Dec 19, 2014 · 12 comments

Comments

Projects
None yet
5 participants
@mcfarljw
Contributor

mcfarljw commented Dec 19, 2014

I'm specifically referring to this commit and line:

p.translate = function(x, y) {

When I revert the translate method to what it was in previous versions things display correctly:

this.tx += x;
this.ty += y;

Here is the bit of code we're using, that given the same data input produces different matrix values between versions:

var ms = shape.getMatrix();
var sx = data.w / spriteBounds.width;
var sy = data.h / spriteBounds.height;
ms.scale(sx, sy);
ms.translate(-data.w / 2, -data.h / 2);
ms.rotate(data.rot * Math.PI / 180);

Any thoughts on why this might be happening?

@yar3333

This comment has been minimized.

Show comment
Hide comment
@yar3333

yar3333 Dec 19, 2014

Contributor

I have a same problem (http://community.createjs.com/discussions/easeljs/17712-matrix2d-in-080), Looks like some matrix methods works different in 0.8.0. As a result, I will use in my project matrix class from 0.7.1 to calculations.

Contributor

yar3333 commented Dec 19, 2014

I have a same problem (http://community.createjs.com/discussions/easeljs/17712-matrix2d-in-080), Looks like some matrix methods works different in 0.8.0. As a result, I will use in my project matrix class from 0.7.1 to calculations.

@MannyC

This comment has been minimized.

Show comment
Hide comment
@MannyC

MannyC Dec 19, 2014

Contributor

There were some changes made to the Matrix2D class and then later reverted. Looks like the translate method was accidentally altered during the commit referenced above.

Contributor

MannyC commented Dec 19, 2014

There were some changes made to the Matrix2D class and then later reverted. Looks like the translate method was accidentally altered during the commit referenced above.

MannyC added a commit to MannyC/EaselJS that referenced this issue Dec 19, 2014

@MannyC

This comment has been minimized.

Show comment
Hide comment
@MannyC

MannyC Dec 19, 2014

Contributor

See PR #540

Contributor

MannyC commented Dec 19, 2014

See PR #540

@mcfarljw

This comment has been minimized.

Show comment
Hide comment
@mcfarljw

mcfarljw Dec 19, 2014

Contributor

@MannyC Thanks for confirming and taking care of that!

Contributor

mcfarljw commented Dec 19, 2014

@MannyC Thanks for confirming and taking care of that!

@gskinner

This comment has been minimized.

Show comment
Hide comment
@gskinner

gskinner Dec 19, 2014

Member

The Matrix2D class was previously not internally consistent in it's matrix orientation. In v0.8.0, we have fixed this by standardizing on one orientation (the same one used in Flash, and from what we can glean, CSS & Context2D). This has unfortunately changed the results from the transformation methods (scale() / rotate() / translate()). They have effectively switched from being the equivalent of an prepend operation to an append operation.

You should be able to get similar results by either swapping the order of your method calls, or using append/prependTransform, or appendMatrix / prependMatrix.

Here's a quick sketch that shows this in action:
http://jsfiddle.net/cv8h1oh4/

It may be worthwhile for us to add pre versions of the methods. For example: preTranslate(x,y). These would behave the same as the old methods.

Apologies for the confusion. Please do always review the VERSIONS file to see CRITICAL changes that may affect your project, though in this case it really isn't as informative as it should be - I'll address that.
https://github.com/CreateJS/EaselJS/blob/master/VERSIONS.txt

Member

gskinner commented Dec 19, 2014

The Matrix2D class was previously not internally consistent in it's matrix orientation. In v0.8.0, we have fixed this by standardizing on one orientation (the same one used in Flash, and from what we can glean, CSS & Context2D). This has unfortunately changed the results from the transformation methods (scale() / rotate() / translate()). They have effectively switched from being the equivalent of an prepend operation to an append operation.

You should be able to get similar results by either swapping the order of your method calls, or using append/prependTransform, or appendMatrix / prependMatrix.

Here's a quick sketch that shows this in action:
http://jsfiddle.net/cv8h1oh4/

It may be worthwhile for us to add pre versions of the methods. For example: preTranslate(x,y). These would behave the same as the old methods.

Apologies for the confusion. Please do always review the VERSIONS file to see CRITICAL changes that may affect your project, though in this case it really isn't as informative as it should be - I'll address that.
https://github.com/CreateJS/EaselJS/blob/master/VERSIONS.txt

@gskinner gskinner added the question label Dec 19, 2014

@gskinner

This comment has been minimized.

Show comment
Hide comment
@gskinner

gskinner Dec 19, 2014

Member

@MannyC if you feel like taking a stab at the pre methods, it would be appreciated. If not, we'll tackle them in the not too distant future. You / we should also add unit tests, something like:

var mtx1 = new Matrix(1,2,3,4,5,6).preTranslate(10,20);
var mtx2 = new Matrix(1,2,3,4,5,6).prependMatrix(new Matrix().translate(10,20);
return mtx1.equals(mtx2);
Member

gskinner commented Dec 19, 2014

@MannyC if you feel like taking a stab at the pre methods, it would be appreciated. If not, we'll tackle them in the not too distant future. You / we should also add unit tests, something like:

var mtx1 = new Matrix(1,2,3,4,5,6).preTranslate(10,20);
var mtx2 = new Matrix(1,2,3,4,5,6).prependMatrix(new Matrix().translate(10,20);
return mtx1.equals(mtx2);
@gskinner

This comment has been minimized.

Show comment
Hide comment
@gskinner

gskinner Dec 19, 2014

Member

I updated VERSIONS to have more appropriate info on this topic.

Member

gskinner commented Dec 19, 2014

I updated VERSIONS to have more appropriate info on this topic.

@mcfarljw

This comment has been minimized.

Show comment
Hide comment
@mcfarljw

mcfarljw Dec 19, 2014

Contributor

Excellent, thanks for the clarification and quick responses from you both. I can confirm that taking those changes into account has resolved the issue.

Contributor

mcfarljw commented Dec 19, 2014

Excellent, thanks for the clarification and quick responses from you both. I can confirm that taking those changes into account has resolved the issue.

@mcfarljw mcfarljw closed this Dec 19, 2014

@MannyC

This comment has been minimized.

Show comment
Hide comment
@MannyC

MannyC Dec 19, 2014

Contributor

Okay, I'll have to do some research on this. I would be worried that by interpreting translate as being in the post translation coordinate system then we're moving away from all other Matrix2D type implementations. It could be a case of deprecating translate and putting in an appendTranslate and a prependTranslate/preTranlsate to make it clearer

Contributor

MannyC commented Dec 19, 2014

Okay, I'll have to do some research on this. I would be worried that by interpreting translate as being in the post translation coordinate system then we're moving away from all other Matrix2D type implementations. It could be a case of deprecating translate and putting in an appendTranslate and a prependTranslate/preTranlsate to make it clearer

@gskinner

This comment has been minimized.

Show comment
Hide comment
@gskinner

gskinner Dec 19, 2014

Member

Great! Apologies we didn't do a better job communicating the change!

We try hard to minimize breaking changes, but this was a time where we realized it had to be done sometime, and we may as well rip the bandaid off sooner than later. It was actually one of the changes that contributed to the choice to release as v0.8.0 instead of v1.0 this time around.

Member

gskinner commented Dec 19, 2014

Great! Apologies we didn't do a better job communicating the change!

We try hard to minimize breaking changes, but this was a time where we realized it had to be done sometime, and we may as well rip the bandaid off sooner than later. It was actually one of the changes that contributed to the choice to release as v0.8.0 instead of v1.0 this time around.

@gskinner

This comment has been minimized.

Show comment
Hide comment
@gskinner

gskinner Dec 19, 2014

Member

@MannyC - cool. I definitely appreciate any thoughts you have.

I believe we are now consistent with Context2D and CSS (ie. issuing transformations in the same order will give the same visual results). Flash on the other hand is the opposite (translate prepends). This is definitely a tricky topic, since there doesn't seem to be any industry standard on nomenclature (or even matrix orientation), and to be fair, I really don't consider myself an expert.

Member

gskinner commented Dec 19, 2014

@MannyC - cool. I definitely appreciate any thoughts you have.

I believe we are now consistent with Context2D and CSS (ie. issuing transformations in the same order will give the same visual results). Flash on the other hand is the opposite (translate prepends). This is definitely a tricky topic, since there doesn't seem to be any industry standard on nomenclature (or even matrix orientation), and to be fair, I really don't consider myself an expert.

@FumioNonaka

This comment has been minimized.

Show comment
Hide comment
@FumioNonaka

FumioNonaka Jan 8, 2015

I wrote the article, "Critical changes of the Matrix2D's transform methods in the EaselJS 0.8.0", to show some snippets to get the same results as old versions with version 0.8.0.

I wrote the article, "Critical changes of the Matrix2D's transform methods in the EaselJS 0.8.0", to show some snippets to get the same results as old versions with version 0.8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment