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

Matrix multiplication fix #5030

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
5 participants
@oleg-akatsuki
Contributor

oleg-akatsuki commented Jan 10, 2014

Fixed order for matrix multiplication in getNodeToParentTransform

Matrix multiplication fix
Fixed order for matrix multiplication in getNodeToParentTransform
@dabingnn

This comment has been minimized.

Show comment
Hide comment
@dabingnn

dabingnn Jan 10, 2014

Contributor

Thank you, @oleg-akatsuki
@ricardoquesada It sounds that after we use kmMat4 instead of affineMatrix, the order of multiplication needs to be reversed. Will you give a review of this PR?

Contributor

dabingnn commented Jan 10, 2014

Thank you, @oleg-akatsuki
@ricardoquesada It sounds that after we use kmMat4 instead of affineMatrix, the order of multiplication needs to be reversed. Will you give a review of this PR?

@ricardoquesada

This comment has been minimized.

Show comment
Hide comment
@ricardoquesada

ricardoquesada Jan 11, 2014

Contributor

@dabingnn The correct order in the multiplication is the one that is already in CCNode.

@oleg-akatsuki What issues did you find in CCNode ? What bugs does your patch fix ? Could you post an sample ?
thanks.

Contributor

ricardoquesada commented Jan 11, 2014

@dabingnn The correct order in the multiplication is the one that is already in CCNode.

@oleg-akatsuki What issues did you find in CCNode ? What bugs does your patch fix ? Could you post an sample ?
thanks.

@oleg-akatsuki

This comment has been minimized.

Show comment
Hide comment
@oleg-akatsuki

oleg-akatsuki Jan 15, 2014

Contributor

Okay, here is an example.
I have my own Flash converter and player. Here is example of how I use setAdditionalTransform.

Add this code to Layer.
FlashTest* flash = FlashTest::create();
flash->setPosition(Point(300, 200));
addChild(flash);

In the code below change "anime/template2/box_anime_class/box_h.png" to path in your system. I have attached box_h.png to this comment.

class FlashTest : public cocos2d::Node
{
public:
FlashTest():currentFrame(0),timeFromStart(0){};
~FlashTest(){};

static FlashTest* create(){
    FlashTest *pRet = new FlashTest();
    if (pRet && pRet->init())
    {
        pRet->autorelease();
        return pRet;
    }
    CC_SAFE_DELETE(pRet);
    return pRet;
}

virtual bool init() override
{
Node::init();

sprite = Sprite::create("anime/template2/box_anime_class/box_h.png");
Size eSize = sprite->getBoundingBox().size;
Point aP = Point(66.00/eSize.width, (eSize.height - 56.00)/eSize.height);
sprite->setAnchorPoint(aP);
addChild(sprite);

frames.push_back(AffineTransformMake(1, 0, 0, 1, -36.55, 13.50));
frames.push_back(AffineTransformMake(1.019, 0, 0, 1.095, -35.40, 10.65));
frames.push_back(AffineTransformMake(1.038, 0, 0, 1.189, -34.15, 7.70));
frames.push_back(AffineTransformMake(1.057, 0, 0, 1.284, -32.95, 4.85));
frames.push_back(AffineTransformMake(1.076, 0, 0, 1.378, -31.70, 1.90));
frames.push_back(AffineTransformMake(1.095, 0, 0, 1.473, -30.45, -0.90));
frames.push_back(AffineTransformMake(1.114, 0, 0, 1.567, -29.25, -3.85));
frames.push_back(AffineTransformMake(1.133, 0, 0, 1.662, -28.00, -6.70));
frames.push_back(AffineTransformMake(1.152, 0, 0, 1.756, -26.80, -9.60));
frames.push_back(AffineTransformMake(1.171, 0, 0, 1.851, -25.60, -12.45));
frames.push_back(AffineTransformMake(1.153, 0, 0, 1.764, -26.70, -9.90));
frames.push_back(AffineTransformMake(1.136, 0, 0, 1.676, -27.80, -7.20));
frames.push_back(AffineTransformMake(1.118, 0, 0, 1.589, -28.95, -4.60));
frames.push_back(AffineTransformMake(1.101, 0, 0, 1.502, -30.10, -1.95));
frames.push_back(AffineTransformMake(1.083, 0, 0, 1.414, -31.20, 0.70));
frames.push_back(AffineTransformMake(1.066, 0, 0, 1.327, -32.30, 3.30));
frames.push_back(AffineTransformMake(1.048, 0, 0, 1.240, -33.40, 6.00));
frames.push_back(AffineTransformMake(1.031, 0, 0, 1.152, -34.60, 8.60));
frames.push_back(AffineTransformMake(1.013, 0, 0, 1.065, -35.70, 11.30));
frames.push_back(AffineTransformMake(0.996, 0, 0, 0.978, -36.75, 13.90));

scheduleUpdate();

return true;

}

void update (float deltaTime) override
{
int maxFrame = frames.size();
timeFromStart += deltaTime;
int newFrame = timeFromStart * 3;
if (newFrame == currentFrame && currentFrame != 0) {return;}
currentFrame = newFrame;
if (currentFrame >= maxFrame) {
currentFrame = 0;
timeFromStart = 0;
}

AffineTransform t = frames[currentFrame];
sprite->setAdditionalTransform(t);
}

std::vectorcocos2d::AffineTransform frames;
cocos2d::Sprite* sprite;

int currentFrame;
float timeFromStart;
};

You should see scaling rectangle with bottom left corner fixed. AffineTransform data is from Flash, the only "y" was fixed for cocos2d. So I am pretty sure it is accurate.
In alpha 0 and 1 everything was fine. Problem only with beta.
box_h

Contributor

oleg-akatsuki commented Jan 15, 2014

Okay, here is an example.
I have my own Flash converter and player. Here is example of how I use setAdditionalTransform.

Add this code to Layer.
FlashTest* flash = FlashTest::create();
flash->setPosition(Point(300, 200));
addChild(flash);

In the code below change "anime/template2/box_anime_class/box_h.png" to path in your system. I have attached box_h.png to this comment.

class FlashTest : public cocos2d::Node
{
public:
FlashTest():currentFrame(0),timeFromStart(0){};
~FlashTest(){};

static FlashTest* create(){
    FlashTest *pRet = new FlashTest();
    if (pRet && pRet->init())
    {
        pRet->autorelease();
        return pRet;
    }
    CC_SAFE_DELETE(pRet);
    return pRet;
}

virtual bool init() override
{
Node::init();

sprite = Sprite::create("anime/template2/box_anime_class/box_h.png");
Size eSize = sprite->getBoundingBox().size;
Point aP = Point(66.00/eSize.width, (eSize.height - 56.00)/eSize.height);
sprite->setAnchorPoint(aP);
addChild(sprite);

frames.push_back(AffineTransformMake(1, 0, 0, 1, -36.55, 13.50));
frames.push_back(AffineTransformMake(1.019, 0, 0, 1.095, -35.40, 10.65));
frames.push_back(AffineTransformMake(1.038, 0, 0, 1.189, -34.15, 7.70));
frames.push_back(AffineTransformMake(1.057, 0, 0, 1.284, -32.95, 4.85));
frames.push_back(AffineTransformMake(1.076, 0, 0, 1.378, -31.70, 1.90));
frames.push_back(AffineTransformMake(1.095, 0, 0, 1.473, -30.45, -0.90));
frames.push_back(AffineTransformMake(1.114, 0, 0, 1.567, -29.25, -3.85));
frames.push_back(AffineTransformMake(1.133, 0, 0, 1.662, -28.00, -6.70));
frames.push_back(AffineTransformMake(1.152, 0, 0, 1.756, -26.80, -9.60));
frames.push_back(AffineTransformMake(1.171, 0, 0, 1.851, -25.60, -12.45));
frames.push_back(AffineTransformMake(1.153, 0, 0, 1.764, -26.70, -9.90));
frames.push_back(AffineTransformMake(1.136, 0, 0, 1.676, -27.80, -7.20));
frames.push_back(AffineTransformMake(1.118, 0, 0, 1.589, -28.95, -4.60));
frames.push_back(AffineTransformMake(1.101, 0, 0, 1.502, -30.10, -1.95));
frames.push_back(AffineTransformMake(1.083, 0, 0, 1.414, -31.20, 0.70));
frames.push_back(AffineTransformMake(1.066, 0, 0, 1.327, -32.30, 3.30));
frames.push_back(AffineTransformMake(1.048, 0, 0, 1.240, -33.40, 6.00));
frames.push_back(AffineTransformMake(1.031, 0, 0, 1.152, -34.60, 8.60));
frames.push_back(AffineTransformMake(1.013, 0, 0, 1.065, -35.70, 11.30));
frames.push_back(AffineTransformMake(0.996, 0, 0, 0.978, -36.75, 13.90));

scheduleUpdate();

return true;

}

void update (float deltaTime) override
{
int maxFrame = frames.size();
timeFromStart += deltaTime;
int newFrame = timeFromStart * 3;
if (newFrame == currentFrame && currentFrame != 0) {return;}
currentFrame = newFrame;
if (currentFrame >= maxFrame) {
currentFrame = 0;
timeFromStart = 0;
}

AffineTransform t = frames[currentFrame];
sprite->setAdditionalTransform(t);
}

std::vectorcocos2d::AffineTransform frames;
cocos2d::Sprite* sprite;

int currentFrame;
float timeFromStart;
};

You should see scaling rectangle with bottom left corner fixed. AffineTransform data is from Flash, the only "y" was fixed for cocos2d. So I am pretty sure it is accurate.
In alpha 0 and 1 everything was fine. Problem only with beta.
box_h

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Feb 7, 2014

Contributor

@ricardoquesada
What's the status of this pull request?

Contributor

minggo commented Feb 7, 2014

@ricardoquesada
What's the status of this pull request?

@ricardoquesada

This comment has been minimized.

Show comment
Hide comment
@ricardoquesada

ricardoquesada Feb 8, 2014

Contributor

I have to double check it, and review the code to understand why it is not working for @oleg-akatsuki.
But I think the current order is correct... I know that if I invert the oder, it breaks skew and other parts of the code.

Contributor

ricardoquesada commented Feb 8, 2014

I have to double check it, and review the code to understand why it is not working for @oleg-akatsuki.
But I think the current order is correct... I know that if I invert the oder, it breaks skew and other parts of the code.

@dabingnn

This comment has been minimized.

Show comment
Hide comment
@dabingnn

dabingnn Feb 21, 2014

Contributor

@ricardoquesada @minggo As we used kazMath instead of CCAffineTransform in our develop branch, the multiplication order of concatenated transform need to be reversed. The pull request https://github.com/cocos2d/cocos2d-x/pull/4847/files has demonstrate this. We fix NodeToWorld Transform calculation bug by reverse the order of matrix multiplication.

The multiplication order on master branch is
screen shot 2014-02-21 at 10 00 50 pm
screen shot 2014-02-21 at 10 04 34 pm
These two are similar as NodeToWorld Transform calculation ,so these multiplication order should be reversed as this pull request suggests.
There may be some other similar situations, we need to find them out and fix them.

Contributor

dabingnn commented Feb 21, 2014

@ricardoquesada @minggo As we used kazMath instead of CCAffineTransform in our develop branch, the multiplication order of concatenated transform need to be reversed. The pull request https://github.com/cocos2d/cocos2d-x/pull/4847/files has demonstrate this. We fix NodeToWorld Transform calculation bug by reverse the order of matrix multiplication.

The multiplication order on master branch is
screen shot 2014-02-21 at 10 00 50 pm
screen shot 2014-02-21 at 10 04 34 pm
These two are similar as NodeToWorld Transform calculation ,so these multiplication order should be reversed as this pull request suggests.
There may be some other similar situations, we need to find them out and fix them.

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Feb 23, 2014

Contributor

Yep. Please checkout if there is any other place has the same issue.
Thanks.

Contributor

minggo commented Feb 23, 2014

Yep. Please checkout if there is any other place has the same issue.
Thanks.

@ricardoquesada

This comment has been minimized.

Show comment
Hide comment
@ricardoquesada

ricardoquesada Feb 23, 2014

Contributor

good finding. And you are correct.
Most probably OrbitCamera won't work if you change the order.

Why additionalTransform is using the incorrect order in v2.x, I don't know.
But currently it has the correct order, which is not compatible with v2.x

Contributor

ricardoquesada commented Feb 23, 2014

good finding. And you are correct.
Most probably OrbitCamera won't work if you change the order.

Why additionalTransform is using the incorrect order in v2.x, I don't know.
But currently it has the correct order, which is not compatible with v2.x

@dabingnn

This comment has been minimized.

Show comment
Hide comment
@dabingnn

dabingnn Feb 24, 2014

Contributor

The order used in v2.x are correct because we used CCAffineTransform. Their multiplication order are different.

Given a Case a node A has a child Node B, And B has an additional Transform.

  1. in v2.x version, the final NodeToWorld transform for Node B is affineTransform concatenation
    NodeToParent of(B) * NodeToWorld of(A), where NodeToParent of (B) is
    TransformPRS of (B) * additionalTransform of (B). TransformPRS of (B) means transform for scale, position and rotation.
    so the final transform concatenation is TransformPRS of (B) * additionalTransform of (B) * NodeToWorld of(A)
  2. in v3.x version, the final NodeToWorld transform for Node B is kazMath concatenation
    NodeToWorld of(A) * NodeToParent of(B), it is a tranposed version of v2.x.
    so the final transform concatenation should be NodeToWorld of(A) * additionalTransform of (B) * TransformPRS of (B). so NodeToParent of (B) is
    additionalTransform of (B) * TransformPRS of (B).
    Comparing this to the pull request. Our order is not correct.

The skewMatrix act the same as additionalTransform.

Besides, if our node has both skewMatrix and additionalTransform. In v2.x,
NodeToParent = TransformPRS * skewMatrix * additionalTransform, which means we got TranformPRS fisrt, then muliply it with skewMatrix, finally additionalTransform.
and in V3.x, the order should be NodeToParent = additionalTransform * skewMatrix * TransformPRS, which need us to get additionalTransform first, then skewMatrix, and at last for TranformPRS.

Contributor

dabingnn commented Feb 24, 2014

The order used in v2.x are correct because we used CCAffineTransform. Their multiplication order are different.

Given a Case a node A has a child Node B, And B has an additional Transform.

  1. in v2.x version, the final NodeToWorld transform for Node B is affineTransform concatenation
    NodeToParent of(B) * NodeToWorld of(A), where NodeToParent of (B) is
    TransformPRS of (B) * additionalTransform of (B). TransformPRS of (B) means transform for scale, position and rotation.
    so the final transform concatenation is TransformPRS of (B) * additionalTransform of (B) * NodeToWorld of(A)
  2. in v3.x version, the final NodeToWorld transform for Node B is kazMath concatenation
    NodeToWorld of(A) * NodeToParent of(B), it is a tranposed version of v2.x.
    so the final transform concatenation should be NodeToWorld of(A) * additionalTransform of (B) * TransformPRS of (B). so NodeToParent of (B) is
    additionalTransform of (B) * TransformPRS of (B).
    Comparing this to the pull request. Our order is not correct.

The skewMatrix act the same as additionalTransform.

Besides, if our node has both skewMatrix and additionalTransform. In v2.x,
NodeToParent = TransformPRS * skewMatrix * additionalTransform, which means we got TranformPRS fisrt, then muliply it with skewMatrix, finally additionalTransform.
and in V3.x, the order should be NodeToParent = additionalTransform * skewMatrix * TransformPRS, which need us to get additionalTransform first, then skewMatrix, and at last for TranformPRS.

@ricardoquesada

This comment has been minimized.

Show comment
Hide comment
@ricardoquesada

ricardoquesada Feb 24, 2014

Contributor

The bug is in v2.x.

Let me explain:

// v2.2. nodeToParentTransform()

// Correct order: First skew, then transform
m_sTransform = CCAffineTransformConcat(skewMatrix, m_sTransform);

// Incorrect order!! 
// Why it is doing Contact(transform, additionalTransform) ????
m_sTransform = CCAffineTransformConcat(m_sTransform, m_sAdditionalTransform);

// Instead, this should be the order
m_sTransform = CCAffineTransformConcat(m_sAdditionalTransform, m_sTransform);
}

I don't know why v2.2 has that order, it is not the correct one. And I don't know how Armature is using it, but by changing the order in the Concat() everything is confusing.

Instead, in v3.0 everything is using the correct order. Which is not compatible with v2.2, but it is using the correct one. Its use is simple to understand... If you pass an Additional Transform Matrix, it will work as expected.

In v2.2, if you pass an Additional Transform Matrix, the results are not the expected one.

If I'm missing something, please let me know.

Contributor

ricardoquesada commented Feb 24, 2014

The bug is in v2.x.

Let me explain:

// v2.2. nodeToParentTransform()

// Correct order: First skew, then transform
m_sTransform = CCAffineTransformConcat(skewMatrix, m_sTransform);

// Incorrect order!! 
// Why it is doing Contact(transform, additionalTransform) ????
m_sTransform = CCAffineTransformConcat(m_sTransform, m_sAdditionalTransform);

// Instead, this should be the order
m_sTransform = CCAffineTransformConcat(m_sAdditionalTransform, m_sTransform);
}

I don't know why v2.2 has that order, it is not the correct one. And I don't know how Armature is using it, but by changing the order in the Concat() everything is confusing.

Instead, in v3.0 everything is using the correct order. Which is not compatible with v2.2, but it is using the correct one. Its use is simple to understand... If you pass an Additional Transform Matrix, it will work as expected.

In v2.2, if you pass an Additional Transform Matrix, the results are not the expected one.

If I'm missing something, please let me know.

@dabingnn

This comment has been minimized.

Show comment
Hide comment
@dabingnn

dabingnn Feb 24, 2014

Contributor

I find the migration to kazMath mat4 in this commit.
0301f86#diff-51f5e8884b98f2f00abcda3117e1a3e3
So, Before this commit, the logic is the same as V2.2. Now their behaviours are different.

Contributor

dabingnn commented Feb 24, 2014

I find the migration to kazMath mat4 in this commit.
0301f86#diff-51f5e8884b98f2f00abcda3117e1a3e3
So, Before this commit, the logic is the same as V2.2. Now their behaviours are different.

@ricardoquesada

This comment has been minimized.

Show comment
Hide comment
@ricardoquesada

ricardoquesada Feb 24, 2014

Contributor

@dabingnn correct.

So, we could rollback the behavior, and make it work like in v2.2... which is not the correct behavior.

As an example, try to add a Translate matrix to additionalTransformMatrix in v2.2... it should not work since the order is not the correct one... (but I could be wrong).

Fixing these kind of bugs in a "silent" way is not recommended because users won't understand why v3.0 behaves differently, but on the other hand, additionalTransformMatrix does not work as expected. New users won't understand how to use it... BTW, who is using this feature ? It was added by the CocoStudio team, but where are they using it ?

Another thing that we could do is to remove additionalTransformMatrix from v3.0 since we already support auto-batching :)

Contributor

ricardoquesada commented Feb 24, 2014

@dabingnn correct.

So, we could rollback the behavior, and make it work like in v2.2... which is not the correct behavior.

As an example, try to add a Translate matrix to additionalTransformMatrix in v2.2... it should not work since the order is not the correct one... (but I could be wrong).

Fixing these kind of bugs in a "silent" way is not recommended because users won't understand why v3.0 behaves differently, but on the other hand, additionalTransformMatrix does not work as expected. New users won't understand how to use it... BTW, who is using this feature ? It was added by the CocoStudio team, but where are they using it ?

Another thing that we could do is to remove additionalTransformMatrix from v3.0 since we already support auto-batching :)

@dabingnn

This comment has been minimized.

Show comment
Hide comment
@dabingnn

dabingnn Feb 26, 2014

Contributor

@oleg-akatsuki
Thank you for your feedback on this issue.
cocos2d version in v2.x and v3.x are not compatible, additionalTransformMatrix is an example. The purpose of set additionalTransformMatrix to sprite is to get a concatenated transform. And in most cases, it can also achieved by scene graph hierarchy ( a transform of node can affects its children, which is also a transform concatenation).

The additionalTransformMatrix are first added to support cocostudio and Amarture. Now we are going to deprecate this interface. It is encouraged that you use scene graph hierarchy instead of additionalTransformMatrix to achieved your goal.

Is there any suggestion about this? Thank you.

Contributor

dabingnn commented Feb 26, 2014

@oleg-akatsuki
Thank you for your feedback on this issue.
cocos2d version in v2.x and v3.x are not compatible, additionalTransformMatrix is an example. The purpose of set additionalTransformMatrix to sprite is to get a concatenated transform. And in most cases, it can also achieved by scene graph hierarchy ( a transform of node can affects its children, which is also a transform concatenation).

The additionalTransformMatrix are first added to support cocostudio and Amarture. Now we are going to deprecate this interface. It is encouraged that you use scene graph hierarchy instead of additionalTransformMatrix to achieved your goal.

Is there any suggestion about this? Thank you.

@cocosQA

This comment has been minimized.

Show comment
Hide comment
@cocosQA

cocosQA Apr 29, 2014

Dear oleg-akatsuki:
We have created a new branch 'v3' to replace branch 'develop', and this PR has been moved from 'develop' branch to 'v3' branch, the new PR is #6468
The old 'develop' branch will be deleted soon, and this PR will be closed accordingly.

cocosQA commented Apr 29, 2014

Dear oleg-akatsuki:
We have created a new branch 'v3' to replace branch 'develop', and this PR has been moved from 'develop' branch to 'v3' branch, the new PR is #6468
The old 'develop' branch will be deleted soon, and this PR will be closed accordingly.

@minggo minggo closed this Apr 29, 2014

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