Konstruktionsklassen auslagern?



  • Hi,

    Eine ähnliche Frage hatte ich ja schon Mal vor ein paar Wochen, aber so recht konnte ich da leider nicht weitermachen. Jetzt habe ich eine ähnliche Frage, die aber etwas konkreter ist.

    Und zwar habe ich eine Datenklasse, die alle möglichen Daten hält. Die Daten sind heterogen, es macht dennoch Sinn diese in einem großer Klasse zusammenzuhalten, da die Interaktion zwischen den ganzen Daten ständig benötigt wird und die Daten auch keinerlei Wiederverwendung erfahren.

    Dann habe ich eine Art Factory-Klasse, die Daten zusammenbaut. Von der Klasse gibt es nur eine Instanz, was mich jetzt schon wieder stört. Die Klasse baut alle möglichen Einzelteile zusammen. Wir haben z.B. einmal Vertices, einmal Animationen, einmal Kollisions-Bounding-Objekte usw.

    Und für die Erstellung von jedem dieser "Einzelteile" brauche ich jeweils mehrere Methoden und ein paar temporäre Daten.

    Zur Zeit reihe ich einfach alle Methoden und deren benötigten Methoden ungeordnet in die Loader-Klasse ein:

    namespace Engine
    {
    	class DLL_SPECIFIER AssimpModelLoader : public ModelLoader
    	{
    	private:
    [...]
    
    	private:
    		void	getNumberOfVertices();
    		void	extractMaterials				(const std::string& filename);
    		void	integrateVertices				();
    		void	integrateIndices				();
    		void	integrateTextureCoordinates		();
    		void	integrateNormals				();
    		void	integrateColors					();
    		void	integrateTangents				();
    		void	integrateNodes					();
    		void	integrateBones					();
    		void	integrateBoneIndicesAndWeights	();
    		void	integrateAnimations				();
    		void	createBoundingSphereTree		();
    
    		void	integrateSubNodes			(const aiNode& aiNode);
    
    		UsualVector		ConvertVector(const aiVector3D& vector);
    		UsualMatrix		ConvertMatrix(const aiMatrix4x4& matrix);
    		UsualQuaternion	ConvertQuaternion(const aiQuaternion& quaternion);
    
    	public:
    [...]
    	};
    

    Es würde aber Sinn machen, ein paar Funktionen zusammenzupacken und in eine neue Klasse auszulagern. Mich stört aber, dass es davon ja dann auch nur wieder eine Instanz gäbe. Und diese eine Instanz käme dann hier in dieser Loaderklasse vor, von der es ja auch nur eine Instanz gibt. 😞 Es macht halt wiederum Sinn, dass das ne Klasse ist, da die einzelnen Methoden nicht von außen aufgerufen können werden sollen und die privaten Daten auch nicht von außen zugreifbar sein sollen.

    Na ja, ich weiß jetzt nur nicht so Recht, ob ich so Subklassen mit den Methoden machen soll bzw. wie ich das sauber refactorn soll. Einfach ein paar Methoden in ne Klasse packen, davon eine Instanz erzeugen und ihr geben, was sie halt braucht?

    Ihr seht:

    1. Mich stört einfach, dass diese Klasse auf Dauer 100 Methoden haben wird.
    2. Ich habe keine Ahnung, ob das schlimm ist und inwiefern das schlimm ist, es sieht nur eben einfach scheiße aus.
    3. Ich finde ne Auslagerung in irgendwelche anderen Klassen auch hässlich. Freie Methoden find ich auch hässlich.
    4. Ich hab keine Ahnung, wie ich hier weitermachen soll. 😑

    Vll. hat irgendwer ne Idee, wie ich hier weiterkommen kann... Gern auch metamäßig, also mit welchen Methoden ich hier zu einer Lösung kommen kann.



  • Was ist das Problem an meiner Frage? 🙂

    Zu trivial, als dass man glauben könnte, das sei wirklich mein Problem? Hübsches Klassendesign ist mein wirklich eine meiner großen Schwächen.

    Jetzt habe ich beispielsweise auch eine Monstermethode implementiert:

    void AssimpModelLoader::createBoundingSphereTree()
    	{
    		// Create first Sphere at first
    
    		struct SphereConstructElement
    		{
    			std::vector<std::size_t>		vertexIDs;
    			std::size_t						sphereID;
    			std::size_t						parentSphereID;
    
    			SphereConstructElement() {}
    			SphereConstructElement(std::vector<std::size_t> pVertexIDs, std::size_t pSphereID, std::size_t pParentSphereID)
    				: vertexIDs(pVertexIDs), sphereID(pSphereID), parentSphereID(pParentSphereID) {}
    		};
    
    		typedef std::queue<SphereConstructElement> ConstructQueue;
    
    		// Insert Parent Construct Element
    		ConstructQueue constructQueue;
    		SphereConstructElement e;
    		for(std::size_t n = 0; n < numberOfVertices_; ++n)
    			e.vertexIDs.push_back(n);
    		e.parentSphereID = -1;
    		e.sphereID = 0;
    		constructQueue.push(e);
    
    		UsualVector vec;
    		std::size_t radiusVertex;
    		CoordinateType radius;
    
    		// Insert all Spheres
    		while(!constructQueue.empty())
    		{
    			auto& currentElement = constructQueue.front();
    
    			// Find center
    			vec = UsualVector(0.f, 0.f, 0.f);
    
    			BOOST_FOREACH(auto v, currentElement.vertexIDs)
    			{
    				vec += UsualVector(	aggregatedVertices_[v].x,
    									aggregatedVertices_[v].y,
    									aggregatedVertices_[v].z);
    			}
    
    			vec /= currentElement.vertexIDs.size();
    
    			// Find biggest Distance
    			radius = 0.f;
    			BOOST_FOREACH(auto v, currentElement.vertexIDs)
    			{
    				if(aggregatedVertices_[v].x - vec.GetX() + aggregatedVertices_[v].y - vec.GetY() + aggregatedVertices_[v].z - vec.GetZ() > radius)
    				{
    					radius = aggregatedVertices_[v].x - vec.GetX() + aggregatedVertices_[v].y - vec.GetY() + aggregatedVertices_[v].z - vec.GetZ();
    					radiusVertex = v;
    				}
    			}
    
    			radius = std::sqrt(	(aggregatedVertices_[radiusVertex].x - vec.GetX()) * (aggregatedVertices_[radiusVertex].x - vec.GetX()) +
    								(aggregatedVertices_[radiusVertex].y - vec.GetY()) * (aggregatedVertices_[radiusVertex].y - vec.GetY()) +
    								(aggregatedVertices_[radiusVertex].z - vec.GetZ()) * (aggregatedVertices_[radiusVertex].z - vec.GetZ()));			
    
    			GetBoundingSphereVector(*newModel_).push_back(BoundingSphere(	vec.GetX(), vec.GetY(), vec.GetZ(), radius,
    																			currentElement.parentSphereID));
    
    			currentElement.sphereID = GetBoundingSphereVector(*newModel_).size() - 1;
    
    			// Try subdividing model by different planes
    			std::size_t planeXYSide1 = 0, planeXYSide2 = 0, planeXZSide1 = 0, planeXZSide2 = 0, planeYZSide1 = 0, planeYZSide2 = 0;
    
    			std::vector<std::size_t> left, right, top, bottom, front, back;
    
    			BOOST_FOREACH(auto v, currentElement.vertexIDs)
    			{
    				if(aggregatedVertices_[v].z < vec.GetZ())
    					front.push_back(v);
    				else
    					back.push_back(v);
    
    				if(aggregatedVertices_[v].x < vec.GetX())
    					left.push_back(v);
    				else
    					right.push_back(v);
    
    				if(aggregatedVertices_[v].y < vec.GetY())
    					bottom.push_back(v);
    				else
    					top.push_back(v);
    			}
    
    			std::size_t bestDifference = difff(front.size(), back.size());
    			std::vector<std::size_t>* usedContainer = &front;
    
    			if(difff(left.size(), right.size()) < bestDifference)
    			{
    				bestDifference = difff(left.size(), right.size());
    				usedContainer = &left;
    			}
    
    			if(difff(top.size(), bottom.size()) < bestDifference)
    			{
    				bestDifference = difff(top.size(), bottom.size());
    				usedContainer = &top;
    			}
    
    			if(usedContainer->size() > 5) // const 50 Vertices!
    			{
    				constructQueue.push(SphereConstructElement(*usedContainer, 0, currentElement.sphereID));
    
    				if(usedContainer == &front)
    					constructQueue.push(SphereConstructElement(back, 0, currentElement.sphereID));
    				if(usedContainer == &left)
    					constructQueue.push(SphereConstructElement(right, 0, currentElement.sphereID));
    				if(usedContainer == &top)
    					constructQueue.push(SphereConstructElement(bottom, 0, currentElement.sphereID));
    			}
    
    			constructQueue.pop();
    		}
    	}
    

    Ich habe Mal alles gepostet in der Hoffnung, man kann daran besser sehen, wie eine gescheite Auslagerung erfolgen kann.
    Die obige Struktur brauche ich nur in der Methode, daher deklariere ich die intern. Wenn ich diese Methode in mehrere aufteilen würde, müsste ich ständig alle möglichen Objekte übergeben.

    Nächste Idee wäre also alles irgendwie in eine Klasse zu bauen, die aber dann wieder nur eine Instanz hat, die jedes Mal für das Laden angelegt wird. Widerspricht das nicht der Idee einer Klasse? Oder interessiert so etwas wie die "Idee der Klasse an sich" niemanden?

    Ich bin einfach absolut unfähig, so was schön zu verteilen. Ich kann Klassen bauen, wenn ich genau die Verantwortlichkeiten weiß bzw. wenn ich ein Objekt in der realen Welt dazu sehe, aber darüber hinaus fällt mir so einiges schwer. Gibt es irgendwelche Faustregeln oder irgendwas, wonach ich mich richten kann?



  • Gar nix zu sagen? 😕 Zu schwammig formuliert? Stilles Kopfschütteln?



  • Ich würde versuchen, die Algorithmen, die hinter deinen Initialisierern stehen zu generalisieren und als freie Funktionen zu implementieren. Anschließend bräuchte man zur Umsetzung deiner sie benutzenden Methoden wahrscheinlich keine Separierung mehr, um die Methoden übersichtlich zu halten.



  • Hi,

    Das Problem ist, dass die Algorithmen nicht generalisierbar sind, da sie sehr, sehr speziell sind.

    Der Loader ist eine Art Übersetzungsschicht. Ich habe Modelldaten in Format1 und will sie in Daten von Format2 umsetzen. Eine Generalisierung davon wäre beliebige Daten von einem bestimmten Format in ein anderes übersetzen zu lassen.

    Der Loader hat im Prinzip folgende Verantwortlichkeiten:
    * Er lässt durch den Assimp-Loader eine Modelldatei laden und parsen
    * Das geladene Modell wird jetzt in das Format meiner Engine übersetzt, indem mein Model-Objekt entsprechend befüllt wird; jede Füll-Methode übersetzt

    Eine Methode erzeugt einen Bounding-Sphere-Collision-Tree für ein bekanntes Mesh. Das wäre generalisierbar. Aber fast der ganze Rest ist einfach nur eine stupide Übersetzung, z.T. mit etwas mehr Logik, weil ich z.B. Animationen, die Nodes untergeordnet sind, welche in n:m zu Bones stehen, irgendwo anders unterordne oder weil ich eine baumartig angeordnete Node-Struktur in einen vector überführe, um Performance rauszuholen etc.

    Wenn ich das richtig sehe, fällt es mir da schwer zu sehen, wie ich daraus gut freie Funktionen machen kann.



  • Naja, wenn es nicht auslagerbar ist, wo soll es dann hin?^^ Wenn du halt viel Logik brauchst, dann kannst du die ja nicht einfach ins Nichts verbannen. Du könntest beispielsweise eine Übersetzerfunktion/Klasse Format1 <-> Format2 basteln, weil benutzer deiner Modelle wohl nach dem Laden nix mehr konvertieren wollen... aber dann musst du halt an zwei Baustellen arbeiten, wenn du etwas an deinem eigenen Format änderst.



  • irgendwie errinnert mich das hier dran
    aber wie du ja selbst weisst, ist klassen design nicht deine stärke 😉

    deine funktion ist jetzt auch nich so riesig, nur 120 zeilen
    für alle die warcraft 3 im battle.net spielen: es gibt da ein host programm, GHost++, da hat eine methode(die auch noch sehr oft aufgerufen wird) gut 3500 zeilen und eine andere klasse 2500 zeilen für die deklaration!!!

    aber zu deinem problem:
    das einfachste ist erstmal sich zu fragen und klarzumachen, was eine klasse überhaupt macht.
    bei dir gehts wenn ich das richtig sehe um spiele programmierung (bzw 3D programmierung), für kollisionen berechnen kannst du alle (im spiel enthaltenen, also nicht instantierte klassen) objekte als klasse machen (jedes einzelne objekt eine klasse) und diese alle von einer höheren klasse erben lassen die sich um kollisionen und physik kümmert.

    das würde anfangs so aussehen:
    du hast einen stein, einen ball, ein flugzeug

    jetzt erstellst du für alle 3 eine klasse und machs dir klar was die machen können müssen

    stein:
    -liegen
    -bewegen
    -was anderes treffen

    ball:
    -liegen
    -bewegen
    -was anderes treffen

    flugzeug:
    -bewegen
    -mit etwas kollidieren

    so als einfaches beispiel. daran sieht man: "oh das ist im prinzip ja alles dasgleiche, was die machen. die lasse ich alle von einer klasse ding erben"

    ding:
    -bewegen
    -kollidieren
    :position
    :masse
    ......

    flugzeug erbt von ding:
    -nehme leute auf
    -setzte leute ab

    ball erbt von ding:
    -ändere farbe
    ...

    stein erbt von ding:
    -liege faul rum

    irgendwo musst du natürlich eine liste von allen objekten haben die es gibt, und die übergibst du als parameter an kollision testen

    aber sowas ist eigentlich recht intuitiv wenn man objektorientierung und vererbung verstanden hat, finde ich(berichtigt mich ruhig)

    ich denke mir bei den ganzen paradigmen von c++ immer wieder:
    brauchen tuts man eigentlich nicht, man kannst auch so alles lösen, aber es macht die sache einfacher und lesbarer

    will sagen: funktionen brauchst man nicht, man kann den code ja direkt irgendwohin schreiben. klassen dasselbe nur im grösseren stil...

    lager mehr aus und mach dir klar was die klassen die du da schreibst machen sollen. und lass sie genau das machen und wissen! nicht mehr und nicht weniger (klein bissle mehr vielleicht 😉 aber nicht übertreiben)

    hoffe ich konnte irgendwo helfen



  • Wenn es dir nur um die Monstermethode geht, dann könntest du einzelne Anweisungsblöcke in eigene Funktionen packen. So sparst du dir auch den Kommentar, was im nächsten Block passiert. Das macht die ganze Sache wesentlich übersichtlicher und damit wartbarer.



  • Danke zunächst!

    Also ich drücke mich offensichtlich einfach nicht gut aus. Solche grundlegenden Sachen sind mir schon klar. Dass Hund und Hase vom Tier erben können ist nichts Neues; das hat allerdings auch nicht mit meinem spezifischen Problem zu tun. Ich habe mir die Fragen der Verantwortlichkeit und der logischen Unterteilbarkeit bereits gestellt. Ich frage hier, weil das einfach zu nichts geführt hat.

    Ich versuch es Mal genauer zu beschreiben, was ich unschön finde. Und bei jedem Punkt würde mich interessieren, a) ob es wirklich schlimm ist und b) wie ich es besser lösen könnte. Warum ich überhaupt frage? Weil ich intuitiv das Gefühl habe, es ist schlechter Stil, rational aber nicht darauf komme, wieso.

    Meine ModelLoader-Klasse ist im Prinzip nur eine große Funktion. Es gibt eine Instanz, man ruft eine Methode auf, diese klappert tausend Funktionen durch und liefert am Ende etwas zurück.

    => Idee wäre eine freie Funktion zu definieren, die viele andere freie Funktionen aufruft.
    => Problem: Es soll außer der Ladefunktion niemand die "Subfunktionen" aufrufen können.
    => Problem: Je nach Dateiformat möchte ich einen anderen Loader nutzen. Man soll später dynamisch weitere Loader hinzufügen können, die sich von der Funktionsweise (Format X -> MAGIC -> Engine-Format) nicht unterscheiden UND alle Zugriff auf die Interna von Model haben sollen. Das löse ich dadurch, dass die ModelLoader-Basisklasse mit Model befreundet ist und protected-Methoden bereitstellt, um Referenzen auf die Interna den einzelnen konkreten Loadern zu übergeben.

    Aus den Problemen ergibt sich für mich heraus, dass eine Klasse schon die korrekte Form zur Sammlung der Funktionen ist. Blöd ist, dass ich wie gesagt immer nur eine Instanz habe, was dagegen spricht. Wo ist die Lösung?

    Ich habe viele einzelne Methoden. Manche von diesen lassen sich vom Sinn zusammenfassen. Aber wie sollte ich diese wiederum als Elemente einer Klasse nochmal zusammenfassen?

    => Idee wäre eine weitere Klasse zu bauen, welche diese Methoden besitzt.
    => Problem: Die Klasse würde ich nur nutzen, weil mir nichts Besseres einfällt.
    => Problem: Entspricht das der Idee einer Klasse? Wieder Mal gäbe es nur eine Instanz, welche unterhalb des Loaders wäre, von dem es auch nur eine Instanz gibt.

    Ich würde also den Vorteil der Kapselung, die mir das Konzept Klasse bietet, ausnutzen, wenngleich nach meiner Vorstellung einer Klasse (Schablone für etwas mehrfach Instanziierbares) darunter leiden müsste. Intuitiv unsauber, rational keine Ahnung.

    Ich hoffe, mein Problem ist jetzt etwas deutlicher geworden.



  • Ja, so sieht es aus!
    Die Konvertierung ist eben nur ein Algorithmus, der ein Objekt bestimmten Typs zum Ergebnis haben sollte. Ich finde das ist ziemlich gut in einer freien Funktion aufgehoben. Die freien Helfer-Funktionen müssen gar nicht über die Modulgrenze hinaus sichtbar sein (anonymous namespace).
    Den Aufruf dieser Konvertierungsfunktion könntest du dann beispielsweise in Objekten, abgelitten von einer abstrakten Model-Converter-Klasse, erledigen, welche zudem noch die Dateinamenerweiterungen etc. überprüfen könnte und was weißt ich nicht und dann von einer Model-Loader-Registry dazu benutzt wird, um kontrete Modelle zu laden und in deinem Format auszuspucken.

    Und als ein Leitsatz unter der Beobachtung, dass das, was du da hast, bereits gut funktioniert: Was im Moment gut funktioniert ist praktisch immer besser als das was eigentlich besser wäre 😉



  • Eisflamme schrieb:

    Blöd ist, dass ich wie gesagt immer nur eine Instanz habe, was dagegen spricht. Wo ist die Lösung?

    Darin, dass du immer nur eine Instanz hast. Was ist daran blöd, was spricht dagegen? Das gibts auch als Entwurfsmuster: nennt sich Singleton

    Ich habe viele einzelne Methoden. Manche von diesen lassen sich vom Sinn zusammenfassen. Aber wie sollte ich diese wiederum als Elemente einer Klasse nochmal zusammenfassen?

    => Idee wäre eine weitere Klasse zu bauen, welche diese Methoden besitzt.
    => Problem: Die Klasse würde ich nur nutzen, weil mir nichts Besseres einfällt.
    => Problem: Entspricht das der Idee einer Klasse? Wieder Mal gäbe es nur eine Instanz, welche unterhalb des Loaders wäre, von dem es auch nur eine Instanz gibt.

    Schau mal nach dem "Single Responsibility Principle". Kapsele weg, was in der aktuellen Funktion grade nicht von Belang ist. Grade wenn du später andere ModelLoader schreibst, gibts doch sicherlich einige Dinge, die wiederholt werden. Ich kann mir vorstellen, dass das Laden von Modellen oft nach einem ähnlichen Schema abläuft - da würde dann eventuell das "Template Method Pattern" greifen.

    nach meiner Vorstellung einer Klasse (Schablone für etwas mehrfach Instanziierbares)

    Die Vorstellung ist nur bedingt richtig, siehe oben.



  • Hallo,

    Okay, das hilft mir alles schon etwas weiter. 🙂

    @pumuckl:

    Also Singleton ist ja aus vielerlei Gründen verpöhnt und ändert ja auch nur, dass man nicht mehrere Instanzen anlegen kann, während mich ja eher die einzige Instanz als logische Implikation stört. Aber wenn das nicht so schlimm ist oder es keine "Entartung" einer Klasse bedeutet, bin ich bereits zufrieden.

    "Single Responsibility Pattern" gefällt mir mit der Erklärung "nur ein Änderungsgrund" sehr gut. Allerdings ist das ne Frage der Formulierung. "Es hat sich etwas am Modellformat" geändert, ist der einzige Grund. "Es hat sich etwas an der Vertexdarstellung" geändert wäre aber einer von etwa zehn Gründen. Jetzt stehe ich vor der Wahl, entweder alles so zu belassen oder 10 kleinere Klassen zu machen. Was meinst Du mit wegkapseln? Methoden nehmen, in eine Klasse stecken und dann halt in meiner Loaderklasse für diese einzelnen "Aufgabenbereiche" wieder jeweils eine Klasse haben? Singletons sind global, meine "Subklassen" sollten aber Teile der Loaderklasse sein...

    Grade wenn du später andere ModelLoader schreibst, gibts doch sicherlich einige Dinge, die wiederholt werden. Ich kann mir vorstellen, dass das Laden von Modellen oft nach einem ähnlichen Schema abläuft - da würde dann eventuell das "Template Method Pattern" greifen.

    Sieht interessant aus, könnte vll. hilfreich sein. Aber verschiedene Ausprägungen der Sub-Algorithmen benötigen evtl. verschiedene Parameter. Bei meinem aktuellen Design haben meine Methoden ja die Freiheit, die kompletten Interna des neu zu erstellenden Models einsehen und setzen zu können. Hat Vor- und Nachteile, ich werde das Mal überdenken, danke. 🙂

    Jetzt schlägt Decimad eher freie Funktionen vor, Du hast aber an meinem Klassenansatz nichts kritisiert. Gibt es irgendwo Material oder Denkanstöße, wann ich in einem Fall wie meinem freie Funktionen und wann eine Kapselung durch Klassen lösen könnte? Freie Funktionen können anscheinend ja auch kapseln, indem ich anonyme namespaces nutze. Meine Vererbungsidee ist dann natürlich passé.

    @Decimad:
    Ok, ich kannte anonyme namespaces bis eben gar nicht. Das sieht praktisch aus.

    Den Aufruf dieser Konvertierungsfunktion könntest du dann beispielsweise in Objekten, abgelitten von einer abstrakten Model-Converter-Klasse, erledigen, welche zudem noch die Dateinamenerweiterungen etc. überprüfen könnte und was weißt ich nicht und dann von einer Model-Loader-Registry dazu benutzt wird, um kontrete Modelle zu laden und in deinem Format auszuspucken

    Okay, hier kann ich nicht ganz folgen. Ich versteh deine Idee so weit so: Die Model-Loader-Registry hat die ganzen ModelLoader, welche von einer abstrakten Basisklasse erben. Jetzt sollen die einzelnen ModelLoader eine Konvertierungsfunktion aufrufen, welche dann wiederum frei ist. Dann versteh ich aber nicht die ModelLoader-Klasse, welchen Sinn hat diese dann?

    Und als ein Leitsatz unter der Beobachtung, dass das, was du da hast, bereits gut funktioniert: Was im Moment gut funktioniert ist praktisch immer besser als das was eigentlich besser wäre

    Vll. bin ich zu wenig zielorientiert und zu sehr nach einem Design versessen, dass extrem sauber ist. Ich kann ja Mal einfach posten, was ich gerade habe und vll sieht man dann schon was.

    Also meine Basisklasse hat folgende Definition:

    class AssimpModelLoader;
    
    	class DLL_SPECIFIER ModelLoader
    	{
    	private:
    		typedef AssimpModelLoader*	AssimpModelLoaderPtr;
    
    		ModelLoader(const ModelLoader&);
    
    	protected:
    		// Methods for Manipulation of new Model
    		Model::MeshHolderVector&		GetMeshHolders(Model& model)	{return model.meshHolders_;}
    		Mesh&							GetMesh(Model& model)			{return model.aggregatedMesh_;}
    		Model::MaterialMap&				GetMaterialMap(Model& model)	{return model.materialMap_;}
    		Model::TextureMap&				GetTextureMap(Model& model)		{return model.textureMap_;}
    		Model::AnimationMap&			GetAnimationMap(Model& model)	{return model.animations_;}
    		Model::NodeVector&				GetNodeVector(Model& model)		{return model.nodes_;}
    		Model::BoundingSphereVector&	GetBoundingSphereVector(Model& model) {return model.boundingSphereTree_;}
    
    	public:
    		ModelLoader() : assimpModelLoader_(0) {}
    
    		virtual Model*	LoadModel(const std::string& filename);
    
    		virtual ~ModelLoader();
    
    	private:
    		AssimpModelLoaderPtr assimpModelLoader_;
    	};
    

    D.h. sie hält von jedem Loader jeweils eine Instanz. Möchte der Benutzer jetzt einen Loader verwenden, ruft er von der einen Instanz dieser Basisklasse (die übrigens nicht abstrakt ist, sorry), die LoadModel-Methode auf:

    Model* ModelLoader::LoadModel(const string& filename)
    	{
    		string::size_type pos = filename.find_last_of('.');
    		if(pos != string::npos)
    		{
    			string ending(filename.substr(pos + 1));
    
    			if(ending == "dae" || ending == "obj" || ending == "3ds" || ending == "x")
    			{
    				if(!assimpModelLoader_)
    					assimpModelLoader_ = new AssimpModelLoader;
    				return assimpModelLoader_->LoadModel(filename);
    			}
    			else
    				THROWEXCEPTION(NoSuitableModelLoader, "No suitable model loader could be found.");
    		}
    		else
    			THROWEXCEPTION(FileNotFound, "Filename " + filename + " is invalid.");
    	}
    

    Die ist schlau und guckt sich die Dateiendung an, um dann den Aufruf an die korrekte Ladeklasse weiterzuleiten. Es gibt z.Z. nur den Assimp-Loader.

    Und der Assimp-Loader, der also das eigentliche Laden übernimmt, macht jetzt die ganze Arbeit, wo ich dann zu dem eigentlichen Designproblem komme, dass ich nicht sicher bin, wie ich genau weiter kapsele.

    Diese Lösung in freie Funktionen umzubauen halte ich für schwierig und empfinde ich auch nicht so ganz als beste Lösung...



  • Die Model-Loader klasse hat das einheitliche Interface, kann die Datei-Erweiterungen checken etc. Die Registry fragt dann die Loader ab, welcher meint, eine bestimmte Datei lesen zu können. Das ganze hat den Vorteil, dass du immernoch die freie Funktion hast... (Also cut&paste in ein anderes projekt, wo du nur mal fix die datei laden willst) und in deinem aktuellen Projekt den ganzen automatischen Firlefanz hast.
    Dein if( ext == ".dae" ) return new ModelLoader()->LoadModel();
    ist mir natürlich jetzt wirklich etwas unschön aufgefallen 😉 Warum kein temporäres lokales Objekt?



  • Wenn ich 10 Modelle nacheinander lade, erstelle und zerstöre ich 10 Mal Instanzen.

    Und bei deiner Lösung habe ich jetzt nochmal eine Klasse mehr, oder? Aber den Vorteil, eine freie Funktion zu haben, finde ich jetzt so eine Sache. Die freie Funktion wäre für mich ja eine Alternative zu Klassen. Beides zu haben wirkt für mich nicht mehr so attraktiv.



  • Also welche beider Optionen ergibt denn hier mehr Sinn:

    // #1
    Model* m1 = ReadModelDae( "some.dae" );
    
    // #2
    DaeModelReader reader;
    Model* m2 = reader.Read( "some.dae" );
    

    Und anschließend würde ich mir sowas vorstellen:

    /*
       Hier könnte man natürlich mit entsprechenden Handler-Objekten noch mehr machen, als das nur auf die Datei-Endung festzulegen.
    */
    LoaderRegistry::Get().Register( "dae", ReadModelDae ); 
    LoaderRegistry::Get().Register( "3ds", ReadModel3ds );
    
    Model* m = LoaderRegistry::Get().Load( "myfile.3ds" );
    


  • Achso, der Vorteil an deiner Lösung ist, dass man dynamisch ModelLoader an Endungen mappen kann, sag das doch.

    Ansonsten ist #2 ja bereits implementiert wie oben beschrieben. Aber ein hartes Verdrahten von Endungen zu bestimmten Loadern reicht auch aus.

    Aber das Ganze hat jetzt nichts mit meiner Frage nach der internen Gliederung zu tun, oder? Mir ging es nur darum, mein Modell zu zeigen, um so was wie reine freie Funktionen auszuschließen, da ich ja irgendwo wohl Klassen brauche (nach meinem aktuellen Stand; wobei man das alles natürlich auch ohne Klassen simulieren könnte).



  • Ich seh das halt im Moment andersherum irgendwie: Du simulierst mit Objekten das Verhalten von freien Funktionen 😉



  • Also wenn ich freie Funktionen hätte, müsste jede Funktion zig Parameter erhalten, die ich ansonsten während der Ladezeit in einer Klasse halte. Zudem brauche ich u.U. ja noch andere Bibliotheken wie die externe Klasse der Assimp-Bibliothek. Auch hier gibt es eine Instanz, die erstellt und gespeichert werden muss. Das sind für mich schon Argumente für ne Klasse.

    Außerdem haben alle deine Beispiele ebenfalls Klassen. Ich verstehe nicht ganz, worauf Du hinaus willst.



  • Man kann solche x-tausend Parameter auch zu einem Kontext zusammenfassen (also beispielsweise einer struktur, die alle algorithmus-relevanten daten für alle zwischenschritte vorhält).

    Ob man das nun über einen Kontext mit freien Teilhilfsfunktionen oder über eine private Klasse mit Hilfsmemberfunktionen macht, ist ja letztlich egal, aber davon sollte nichts nach außen (zum Anwender) gelangen. Dass sich der Anwender selbst ein Loader-Objekt erstellen muss um ein Objekt zu laden, ist halt ein nach außen gewandertes Implementierungsdetail.

    Der Benutzer hat einen Dateinamen und will ein Modell davon erstellt haben. Das simpelste für ihn überhaupt ist folgende Signatur:

    Model* LoadDae( std::wstring& filename );
    

    Das hindert dich natürlich in keiner Weise, innerhalb von Load ein privates Helferobjekt mit Hilfsmethoden, zwischenspeichern etc. zu benutzen, oder ein Kontext-objekt mit freien Hilfsfunktionen zu füllen/verändern.

    Edit:
    Ich möchte ja gar nicht sagen: Benutze keine Klassen. Aber mir als Dau würde kein Grund daf+r einfallen, ein Objekt zu erstellen, dass mir nur eine einzige Aufgabe erfüllt, nämlich eine Datei zu laden und ein Modell auszuspucken. Ich denke mir: zwischen dem Laden 2er Dateien besteht doch eigentlich gar kein Zusammenhang, also kann ich ja jedes Mal eine andere Instanz davon benutzen. Danach würde ich mir überlegen: Hält das Objekt vor oder nach dem Aufruf von der Methode Load irgendwelche wichtigen Daten?



  • Ne, vorher und nachher gibt es da wenig. Aber wie regele ich den Zugriff auf die Interna der Klasse? Vorher ging einfach ein praktisches friend auf die ganze ModelLoader-Klasse. Jetzt müsste ich für jede Methode, die irgendwo irgendwas lädt ein extra friend einbauen.

    Dass ich vorher und nachher keine wichtigen Daten mehr erhalte, ist aber insofern kein Argument gegen eine Klasse, da das Template Method Pattern ja vorher und/oder nachher auch nichts mehr hält. Also ist es anscheinend ok, durch Klassen Konzepte umzusetzen. Und hier setze ich eben das Konzept um, dass abgeleitete Klassen die Möglichkeit erhalten, Interna der Klasse zu ändern, d.h. ich vererbe eine Verantwortung.


Anmelden zum Antworten