Problems with matrix class

Started by
7 comments, last by Zakwayda 16 years, 2 months ago
Hi I'm writing a matrix class but im running into a few hitches, it seems that my matrix multiplication function isn't working and as a result neither is my CGE_MatrixRotationYawPitchRoll (which concatinates the results of rotating by Z,X and Y). However it may be that my rotations are incorrect as I can only test them individually at the moment, please have a look and let me know if you see anything wrong:

// Yaw Pitch Roll Rotation
inline
FLOAT4x4 * CGE_MatrixRotationYawPitchRoll(FLOAT4x4 * pOut,float Yaw,float Pitch,float Roll)
{
	
	static FLOAT4x4 ZRot;
	static FLOAT4x4 XRot;
	static FLOAT4x4 YRot;

	CGE_MatrixRotationZ(&ZRot,Yaw);
	CGE_MatrixRotationX(&XRot,Pitch);
	CGE_MatrixRotationY(&YRot,Roll);

	CGE_MatrixMultiply(pOut,&ZRot,&XRot);
	CGE_MatrixMultiply(pOut,pOut,&YRot);

	return pOut; 
}

// Calls these 3 rotate functions
inline
FLOAT4x4 * CGE_MatrixRotationX(FLOAT4x4 * pOut,float Angle)
{

	float cosTheta = cosf(Angle);
	float sinTheta = sinf(Angle);
	pOut->_22 = cosTheta;
	pOut->_23 = -sinTheta;
	pOut->_32 = sinTheta;
	pOut->_33 = cosTheta;

	return pOut;
}

inline
FLOAT4x4 * CGE_MatrixRotationY(FLOAT4x4 * pOut,float Angle)
{
	float cosTheta = cosf(Angle);
	float sinTheta = sinf(Angle);
	pOut->_11 = cosTheta;
	pOut->_13 = sinTheta;
	pOut->_31 = -sinTheta;
	pOut->_33 = cosTheta;

	return pOut;
}

inline
FLOAT4x4 * CGE_MatrixRotationZ(FLOAT4x4 * pOut,float Angle)
{
	float cosTheta = cosf(Angle);
	float sinTheta = sinf(Angle);

	pOut->_11 = cosTheta;
	pOut->_12 = -sinTheta;
	pOut->_21 = sinTheta;
	pOut->_22 = cosTheta;

	return pOut;
}

// Then multiplies the results together using this function
inline
FLOAT4x4 * CGE_MatrixMultiply(	FLOAT4x4 * pOut,
								const FLOAT4x4* pM1,
								const FLOAT4x4* pM2
								)
{
	int i,j,k;

	static FLOAT4x4* pMat1 = new FLOAT4x4(pM1);
	static FLOAT4x4* pMat2 = new FLOAT4x4(pM2);

	for (int i=4; i >= 0; --i) {
		for (int j=4; j >= 0; --j) {
			for (int k = 4; k >= 0; --k) {
				pOut[j] += pMat1[k] * pMat2[k][j];
			}
		}
	}


	static char buffer[255];
	sprintf(buffer,"\n %f,%f,%f,%f \n %f,%f,%f,%f \n ,%f,%f,%f,%f \n ,%f,%f,%f,%f \n",	pOut->_11,pOut->_12,pOut->_13,pOut->_14,
																					pOut->_21,pOut->_22,pOut->_23,pOut->_24,
																					pOut->_31,pOut->_32,pOut->_33,pOut->_34,
																					pOut->_41,pOut->_42,pOut->_43,pOut->_44);

	OutputDebugStringA((LPCSTR)buffer);

	sprintf(buffer,"-------------------");
	OutputDebugStringA((LPCSTR)buffer);
	return pOut;



}

The output of my debug string shows that only element _11 of the matrix is being incremented every frame. Can anyone see the problem? Thanks
Advertisement
There are several problems with your code. The first thing is, that creation of rotation matrix only sets the 4 matrix elements affected by the rotation angle, but leaves the other elements of the matrix undefined.

The next problem is your code for matrix multiplication. Index values for a 4x4 matrix should range from 0 to 3 (or 3 to 0 if you want to count downwards), but not from 4 to 0. Also think about what happens if your output matrix is one (or even both) of the input matrices. Also for matrix multiplication your output matrix is not initialized before calling += on the matrix elements resulting in undefined contents of the matrix as a whole.

1. What is the initial state of your matrices? Identity? Zero? I ask because a) your rotation functions only set a few of the matrix elements and leave the rest unchanged, and b) it looks like your multiplication function does not zero the destination matrix before accumulating the matrix product.

2. Why do your multiplication loops go backward? Also, it looks like these loops iterate 5 times rather than 4, and access invalid elements as well (i.e. index '4').

3. Is this C++? If so, you have at least two memory leaks. Also, you don't want to be allocating memory dynamically inside a matrix multiplication function.

4. Out of curiosity, what is your purpose and goal here? I ask partially because your approach mirrors closely that of the DirectX math library, so if you're working in an environment that supports DX, it seems it would be easier to just use DX rather than try to re-create the functionality from scratch. If it's cross-platform support you're after, there are many cross-platform libraries available as well.

5. The approach you're taking is quite C-like. If this is C++ (as it appears to be), and you are intent on writing your own math library, I'd recommend taking advantage of some of the many tools offered by (and idioms common to) the C++ language (for example, using references instead of pointers where appropriate).
just to clarify, the matrix class is initialised to the identity. I am not using the D3DX functions because I am attempting (rather unsucesfully) to write my own just to learn how to do it.
Quote:Original post by treeway
just to clarify, the matrix class is initialised to the identity. I am not using the D3DX functions because I am attempting (rather unsucesfully) to write my own just to learn how to do it.
In that case:

1. Functions that build transforms (such as your rotation functions) should initialize the output matrix to identity before further modifying the elements.

2. The multiplication function should initialize the output matrix to 0 before accumulating the matrix product.

3. The loops should iterate from 0 to 3 (or vice versa), as previously noted.

4. The multiplication function should not allocate temporary matrices dynamically (they should be created on the stack).

There are probably some other things that should be changed as well, but that's enough for one post.

[Edit: The i, j, and k variables at the top of the mult function are not used. Scoping rules should prevent them from being a problem, but they should be taken out just the same.]
Thanks for the help so far but I still have a problem, If i zero the ouput matrix when using the output matrix as one of the inputs, it means that I lose what was originally in there.

inlineFLOAT4x4 * CGE_MatrixMultiply(	FLOAT4x4 * pOut,								const FLOAT4x4* pM1,								const FLOAT4x4* pM2								){	int i,j,k;	for (int i=0; i < 4; ++i) {		for (int j=0; j < 4; ++j) {			pOut[j] = 0;			for (int k = 0; k < 4; ++k) 			{				pOut[j] += pM1[k] * pM2[k][j];			}		}	}																			return pOut;}	CGE_MatrixMultiply(pOut,pOut,&ZRot); // With this call pOut gets zeroed when it is also an input parameter


I have also fixed the Rotation functions to set all the other values of the matrix to identity values do scaling and position do the same?

thanks
Quote:Original post by treeway
Thanks for the help so far but I still have a problem, If i zero the ouput matrix when using the output matrix as one of the inputs, it means that I lose what was originally in there.

*** Source Snippet Removed ***
You will need to perform the multiplication using copies of the input matrices. It looks like you were actually trying to do this initially, it's just that you weren't doing it correctly. Specifically, the local copies should not be static (if they are, they will only be initialized as copies of the input matrices the first time the function is called), and they should not be allocated dynamically (this doesn't effect the behavior of the function, mathematically speaking, but it is slow, and it does cause a memory leak).
Quote:I have also fixed the Rotation functions to set all the other values of the matrix to identity values do scaling and position do the same?
Just think through it. All of these matrix types (cardinal-axis rotations, scaling, translation) only modify a subset of the input matrix's elements. Any elements that are not modified retain whatever values they had previously. If these values are not identity (and they might very well not be, depending on how the matrix was used previously), the resulting transform will be different from what you intend at best, and meaningless at worst.

So yes, matrices should be initialized to identity before the transform (any transform, more or less) is constructed.
you're right, I was trying to 'optimize' the functions by only changing the parts of the matrix which are changed during the rotation / translation/ whatever but it would mean any garbage data was left in the parts of the matrix that were not changed. just because I initialised it to the identity matrix didn't assure that it was still an identity when it came to performing the translation. When you say I should perform the multiplication on a copy, that is what i was trying to do orignally but you say I should have the copies external to the function? should I have them in the matrix class itself or global?
Quote:Original post by treeway
When you say I should perform the multiplication on a copy, that is what i was trying to do orignally but you say I should have the copies external to the function? should I have them in the matrix class itself or global?
No, the copies should be local, like this:

inlineFLOAT4x4* CGE_MatrixMultiply(    FLOAT4x4* pOut,    const FLOAT4x4* pM1,    const FLOAT4x4* pM2){    assert(pOut);    assert(pM1);    assert(pM2);    FLOAT4x4 pMat1(*pM1);    FLOAT4x4 pMat2(*pM2);    for (int i = 3; i >= 0; --i) {        for (int j = 3; j >= 0; --j) {            for (int k = 3; k >= 0; --k) {                pOut[j] += pMat1[k] * pMat2[k][j];            }        }    }    return pOut;}

I'm not sure how your FLOAT4x4 objects are defined or how your indexing works, so I didn't modify the way the local copies are indexed (you'll have to do that yourself as appropriate).

This topic is closed to new replies.

Advertisement