Optimise some matrix operations, based on patch from ortalo.
Concern RaisedrP9141
Actions

Description

Optimise some matrix operations, based on patch from ortalo.
Fixes #750.

Details

Auditors

vladislavbelov

Committed

philip

Apr 1 2011, 8:33 PM

Parents

rP9140: # Add idle worker button, based on patch from veprbl.

Branches

Unknown

Tags

Unknown

Event Timeline

philip committed rP9141: Optimise some matrix operations, based on patch from ortalo..Apr 1 2011, 8:33 PM

vladislavbelov raised a concern with this commit.Aug 9 2020, 10:41 PM

vladislavbelov added a subscriber: vladislavbelov.

vladislavbelov added inline comments.

/ps/trunk/source/maths/Matrix3D.cpp

300

That change is wrong - incorrect order.

Let assume we have scale matrix from SetScaling:

| s_x, 0, 0, 0|
| 0, s_y, 0, 0|
| 0, 0, s_z, 0|
| 0  0,   0, 1|

And we want to multiply another matrix by the scale one. Let take a look at the matrix multiplication formula:

		return CMatrix3D(
			_11*matrix._11 + _12*matrix._21 + _13*matrix._31 + _14*matrix._41, // > a11 > _11
			_11*matrix._12 + _12*matrix._22 + _13*matrix._32 + _14*matrix._42, // > a12 > _12
			_11*matrix._13 + _12*matrix._23 + _13*matrix._33 + _14*matrix._43, // > a13 > _13
			_11*matrix._14 + _12*matrix._24 + _13*matrix._34 + _14*matrix._44, // > a14 > _14

			_21*matrix._11 + _22*matrix._21 + _23*matrix._31 + _24*matrix._41, // > a21 > _21
			_21*matrix._12 + _22*matrix._22 + _23*matrix._32 + _24*matrix._42, // > a22 > _22
			_21*matrix._13 + _22*matrix._23 + _23*matrix._33 + _24*matrix._43, // > a23 > _23
			_21*matrix._14 + _22*matrix._24 + _23*matrix._34 + _24*matrix._44, // > a24 > _24

			_31*matrix._11 + _32*matrix._21 + _33*matrix._31 + _34*matrix._41, // > a31 > _31
			_31*matrix._12 + _32*matrix._22 + _33*matrix._32 + _34*matrix._42, // > a32 > _32
			_31*matrix._13 + _32*matrix._23 + _33*matrix._33 + _34*matrix._43, // > a33 > _33
			_31*matrix._14 + _32*matrix._24 + _33*matrix._34 + _34*matrix._44, // > a34 > _34

			_41*matrix._11 + _42*matrix._21 + _43*matrix._31 + _44*matrix._41, // > a41 > _41
			_41*matrix._12 + _42*matrix._22 + _43*matrix._32 + _44*matrix._42, // > a42 > _42
			_41*matrix._13 + _42*matrix._23 + _43*matrix._33 + _44*matrix._43, // > a43 > _43
			_41*matrix._14 + _42*matrix._24 + _43*matrix._34 + _44*matrix._44  // > a44 > _44
		);

By multiplication formula _11 element of the scale matrix should affect _11, _21, _31 and _41. But that change affects _11, _12, _13 and _14. That sequence happens in case we multiply the scale matrix by the another one (reverse order). So the change broke behaviour, as it was different before.

/ps/trunk/source/maths/tests/test_Matrix3d.h

125

Someone was lucky that the test is passed. On more clean values of the m matrix the test doesn't work.

This commit now has outstanding concerns.Aug 9 2020, 10:41 PM