Template Klasse CMatrix (Typ(4/4))

Habe gerade keine Lust mich einzuloggen, aber mal ein paar Dinge dazu:

1. ist es sehr ungeschickt im Header die "d3d9.h" zu includieren, es sei denn Du verfolgst einen anderen Sinn als ich dachte.

2. ist es irgendwie ungeschickt die D3DMatrix zu wrappen. Wenn überhaupt solltest Du die D3DXMatrix benutzen da die intern noch optimiert ist.

3. sind auch diese Methoden sehr ungeschickt:

void SetAsMatrixProjection(IDirect3DDevice9*) const; 
    void SetAsMatrixWorld(IDirect3DDevice9*) const; 
    void SetAsMatrixView(IDirect3DDevice9*) const;

Aber bevor ich weitermache? Welchen Sinn soll dein Template haben?

Bashar

Ohne Anspruch auf Vollständigkeit:

Technische Fehler:

FillMemory(m_data, sizeof(CMatrix), m);

Ist FillMemory nicht eine Winapi-Funktion, die einen Speicherbereich byteweise füllt? Da wird also zuerst m in einen Ganzzahltyp (unsigned char?) umgewandelt und dann in den Speicherbereich rengeprügelt, der wahrscheinlich für float oder double gedacht ist. Hätte dir beim Testen eigentlich auffallen müssen. Es sei denn FillMemory ist eine von dir geschriebene Funktionstemplate oder überladene Funktion, die du unterschlagen hast.

Warum geben die Multiplikationsoperatoren Referenzen auf statische Objekte zurück? Das kann ziemlich eklig werden, erst Recht, wenn du die Klasse in verschiedenen Threads verwendest.
Kein Test auf Selbstzuweisung. memcpy funktioniert nur sicher, wenn Quell- und Zielbereich sich nicht überschneiden.

Designfehler:

Warum geben die Zuweisungsoperatoren const-Referenzen zurück? Das verhindert, dass du A = B = C; schreiben kannst.
Wieso gibt Identity eine const-Referenz zurück? Das Objekt wurde doch gerade erst verändert ...
T muss anscheinend zwangsweise POD sein, da du mit memcpy arbeitest (BTW siehe auch den FillMemory-Einwand oben). Kann man machen, sollte aber dokumentiert werden.
Seltsame Kopplung an D3D. CMatrix ist anscheinend eine D3D-Matrix mit zusätzlichen Memberfunktionen?
Große Teile der Klasse gehen davon aus, dass T == float, besonders deutlich wird das in ProjectionMatrix. Dann lass doch das Template-Gedöns gleich weg ...

Ich würd sagen, die Matrix hat nen falschen Namen. Das ist ja nur eine 3D Matrix.

Was soll denn das sein? *this = data;

Seraph

Der größte Witz an der Sache ist das Template ansich. Hättest Du wirklich eine eigene Matrix geschrieben wären einige Dinge ja noch sinnvoll, da D3DMatrix meines Wissens aber nur floats unterstützt ...

Bist Du evtl. ein wenig masochistisch veranlagt?

Mis2com

LOL, war wohl ein Griff ins KLo. ^^

Nagut, dann enttemplatesiere ich die mal...

Möge man es als Übung betrachten.

Patrick

Seraph schrieb:

Bist Du evtl. ein wenig masochistisch veranlagt?

LOL

Also ich könnte das Template für meine OpenGL programmierung benutzen weil ich lieber double als float benutze und da könnte man dann seinen Dateityp angeben (z.B. nen eigenen für Abwärtskompatibilität). aber 1. Fehlen da haufenweise funktionen und 2. ist das Konzept irgendwie naja...

Mis2com

Nagut, die paar Funktionen, die noch nötig sind, kann ich theoretisch dazu machen, ich habe sie gestern ja auch nur auf den letzten Drücker fertigbekommen, das wird alles folgen.
Aber wie gesagt, jetzt wird sie zu reinem <float>
Und dann bekommt sie noch ein paar Funktionen hier und da und dann ist die Engine v0,39 fertig

MfG MAV

Jester

das static wird nicht nur bei Multithreaded eklig!

nehmen wir mal A,B als Matrizen und v,w als Vektoren:

vector<float> c = A*v+B*w;

wir mit ziemlicher Sicherheit schief gehen:
Es wird A*v ausgewertet, was wir aber kriegen ist nur die Referenz auf das statische Objekt, dann wird B*w ausgewertet und das statische Objekt wird wieder geändert... das heißt im Endeffekt kriegen 2*B*w raus. Und das schönste ist: Der Compiler kann auch andere Reihenfolgen machen, das heißt: Jeder Compiler darf was anderes rauskriegen und sogar der selbe Compiler darf verschiedene Ergebnisse an verschiedenen Stellen für den gleichen Ausdruck liefern...

Warum schreibst Du eigentlich eine Matrix, wenn nichtmal Dein vector richtig gefixt ist? Laut Source im ZFX dreht Dein Vector mit dem unären operator- immer noch den Vektor um, anstatt nur eine Kopie zu drehen.

Mis2com

OK, static verwende ich nicht merh...

template<class T> CVector<T> CVector<T>::operator-()
{
	CVector<T> temp(*this);
	temp.m_x = -m_x;
	temp.m_y = -m_y;
	temp.m_z = -m_z;

	return temp;
}

Ist das nicht korrekt?

MfF MAV

Jester

Doch, das stimmt, ist aber nicht sonderlich schnell, warum konstruieren+zuweisen, wenn Du schon einen passenden Konstruktor hast?

template<class T> const CVector<T> CVector<T>::operator-()
{
return CVector<T>(-m_x, -m_y, -m_z);
}

tut es doch auch.
Rückgabewert sollte außerdem konnst sein, da man dem Wert sonst was zuweisen kann und das ist nicht sinnvoll!

Achja, Dein operator* der Matrix ist mal wieder nicht mit Hilfe des operator*= implementiert. Tippst Du gerne viel?

MfG Jester

Mis2com

Dem Ersteren sitmme ich zu.

Aber wenn ich einfach nur CMatrix zurückgebe, ist das doch dann eine Kopie, die man auch verändern darf, oder?

Und beim Letzteren... ich bin mir einfach nicht sicher, ob man das so machen kann i.d.F.

MfG MAV

Jester

Naja,

-v=blablabla ist halt nicht sinnig, daher sollte es konst sein.
Man sollte sich beim Entwurf immer int als Vorbild nehmen, ist etwas bei ints erlaubt, dann sollte man es auch erlauben, erlauben es die ints nicht, dann sollte man es auch nicht machen, oder kurz: "do as the ints do"

a+b=c ist mit ints verboten, also sollte eine selbst geschriebene klasse das auch nicht zulassen. Das macht die Benutzung Deiner Klassen intuitiver.

und die operatoren mit Hilfe anderer zu implementieren ist fast immer möglich!

Erst Kopie anlegen, dann mit dieser die Operation ausführen, Kopie zurückgeben:

template<class T> const CMatrix<T> CMatrix<T>::operator*(const CMatrix<T>& m) const 
{ 
   CMatrix<T> temp(*this); // Copy-Konstruktor
   temp*=m;                // operator*=, der macht keine Kopien und so
   return temp;    
}

Der Vorteil ist: wenn Du operator*= einmal richtig hast, dann haste auch den operator* gleich mit richtig und denken mußte auch nix mehr dabei.

MfG Jester

Mis2com

Ja, ich dachte aber folgendes (das mit const ist jetzt glaube ich ich klar; danke :))

CMatrix& CMatrix::operator*=(const CMatrix& m)
{
	CMatrix temp;

	for(unsigned short i = 0; i < 4; ++i)
		for(unsigned short j = 0; j < 4; ++j)
			for(unsigned short k = 0; k < 4; ++k)
				temp.m_data[j][i] += m_data[k][i] * m.m_data[j][k];
	return (*this = temp);
}

// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

const CMatrix CMatrix::operator*(const CMatrix& m) const
{
	CMatrix temp;

	for(unsigned short i = 0; i < 4; ++i)
		for(unsigned short j = 0; j < 4; ++j)
			for(unsigned short k = 0; k < 4; ++k)
				temp.m_data[j][i] += m_data[k][i] * m.m_data[j][k];
	return temp;
}

Wennn ich jetzt in * den Operator *= verwende, dann legt er insgesamt 2 temporäre Variablen an, nämlich in * und in *=

Diese Variante müsste daher IMHO schneller sein, oder?

MfG MAV

Jester

Okay, das sehe ich ein. Dennoch solltest Du den Code nicht zweimal schreiben!
Wie wäre es mit einem privaten Member

void Multiply(const CVector<T> & A, const CVector<T> & B, CVector<T> & Erg)
{
   // Code berechnet A*B und schreib in Erg rein.
}

operator*
{
   // temp anlegen
   Multiply(A,B, temp);
   return temp;
}

operator *=
{
   // temp anlegen
   Multilply(*this, A, temp);
   *this = temp;
}

Bei Multiply muß man halt aufpassen, daß temp wirklich von A und B verschieden ist, sonst knallt es, aber die Funktion ist ja privat, daher ist es nicht so shlimm.

Walli

Oder halt in *= schreiben "*this = *this * matrix2;".

Jester

Jo, aber das ist wohl eher unüblich. Normalerweise macht man eher * mit *=.
Andersrum hab ich's bis jetzt nur bei Mis2Com's CVector gesehen...

Bashar

Jester schrieb:

Normalerweise macht man eher * mit *=.

Weil man *= oft in-place machen kann. Das klappt bei der Matrix leider nicht ...

b7f7

in der multiply() sache darf man nur nicht vergessen zu testen ob

A||B == Erg sind an sonsten gibt es ein Drama.

Mis2com

Ach ich lass es jetzt einfach mal so, dieser kleine Schönheitsfehler ist IMHO zu verkraften

Danke für euer aller Hilfe.

Das war eben meine zweite Templateklasse, vorher hab ich die Dinger netmal angerührt.

Jetzt bin ich besser informiert, thx.

MfG MAV