Wieviele LOC für Funktion?
-
So, eine erste Überarbeitung meinerseits. Mit einigen Stellen bin ich noch nicht glücklich, aber ich denke man kriegt einen gewissen Eindruck. Obwohl es mir in den Fingern gejuckt hat hab ich drauf verzichtet, Variablennamen zu ändern, z.B. Model -> ModelFile, die in meinen Augen unnötigen m_-Präfixe und die ungarische Notation (die evtl. sogar von einem Framework vorgegeben sein mag).
Die neuen Methoden für ModelPart sind nicht zwingend erforderlich, runden den einen oder anderen Aufruf in meinen Augen aber ab. Wenn ModelPart vom Framework vorgegeben ist, kann man entweder freie Funktionen definieren die ein ModelPart als Argument nehmen oder von ModelPart ableiten (da es ein struct ist sollte das wenig Probleme bereiten).
void getModelPart(ModelPart* modelPart); void bfModel::ReadModelParts() { for (int i=0; i <m_iNumModelParts; ++i) { getModelPart(m_pModelParts + i); } } struct ChunkReader //keine Ahnung was das ständige ReadChunk bringen soll, aber naja... { bfChunk chunk; ChunkReader() : chunk() { ZeroMemory(&chunk,sizeof(chunk); } void read() { ReadChunk(chunk); } int getLastiID() { return chunk.iID; } }; template <typename T> void getVar(T& var); { fread_s(&var, sizeof(var), sizeof(var), 1, Model); } void getVertices(ModelPart* modelPart); void getIndices(Modelpart* modelPart); // Die Hauptfunktion - reicht um zu verstehen was im Groben passiert // void getModelPart(ModelPart* modelPart) { static ChunkReader chunkReader; chunkReader.read(); chunkreader.read(); getVar(modelPart->dVertexFormat); chunkreader.read(); getVar(modelPart->iVertices); getVar(modelPart->dVertexSize); getVar(modelPart->fSphere); getVar(modelPart->MinBox); //nehme an, dass MinBox vom Typ Box ist, sonst separate getBox() Funktion getVar(modelPart->MaxBox); getVertices(modelPart); chunkReader.read(); getVar(modelPart->iIndices); getIndices(modelPart); chunkReader.read(); if (chunkReader.getLastiID() != 0x4140) { stepBackOneIndex(); } else { getVar(modelPart->iModelNumber); getVar(modelPart->vKoords); //nehme an, dass vKoords vom eigenen Typ ist, sonst separate getKoords() Funktion } } void getVertices(ModelPart* modelPart) { size_t bufferSize = modelPart->getVertexBufferSize(); std::vector<BYTE> buffer(bufferSize); fread_s(&buffer[0], bufferSize, modelPart->dVertexSize, modelPart->iVertices, Model); modelPart->initVertexBuffer(); //s.u. modelPart->addVerticesToVertexBuffer(&buffer[0]); modelPart->updateVertexBuffer(); } void getIndices(Modelpart* modelPart) { size_t bufferSize = modelPart->getIndexBufferSize(); std::vector<BYTE> buffer(bufferSize); fread_s(&buffer[0], bufferSize, 6, modelPart->iIndices, Model); modelPart->initIndexBuffer(); modelPart->addIndicesToIndexBuffer(&buffer[0]); modelPart->updateIndexBuffer(); } //Zusätzliche ModelPart-Methoden size_t ModelPart::getVertexBufferSize() { return dVertexSize*iVertices; } void ModelPart::initVertexBuffer() { VertexBuffer.Init(getVertexBufferSize(), dVertexSize, dVertexFormat); } void ModelPart::addVerticesToVertexBuffer(BYTE* source) { VertexBuffer.AddVertices(iVertices, source); } void ModelPart::updateVertexBuffer() { VertexBuffer.Update(); } void ModelPart::initIdexBuffer() { IndexBuffer.Init(iIndices*6, 6); } size_t ModelPart::getIndexBufferSize() { return 6*iIndices; } void ModelPart::updateIndexBuffer() { IndexBuffer.Update(); } void ModelPart::addIndicesToIndexBuffer(BYTE* source) { IndexBuffer.AddIndices(iIndices, source); }/edit: eine vergessen:
void stepBackOneIndex() { fseek(Model,-6,SEEK_CUR); }
-
Interessante Sache, du hast die Funktion total Zerlegt.
Ich gebe dir mal noch die Sachen die aus dem ersten ersten Code von mir nicht ersichtlich waren:
//Das Struct auf dem der ModelPart basiert struct bfModelPart { DWORD dVertexFormat; int iVertices; int iIndices; DWORD dVertexSize; bfIndexBuffer IndexBuffer; bfVertexBuffer VertexBuffer; bfVector3 MinBox; bfVector3 MaxBox; float fSphere; int iModelNumber; bfVector3 vKoords; }; //Der Chunck Struct struct bfChunk { unsigned short iID; unsigned int iChunkSize; int iBytesRead; }; //Die Relevanten Klassenvariablen FILE *Model; int m_iNumModelParts; bfModelPart *m_pModelParts;//Modeleinzellteile //Read und SkipChunk void ReadChunk(bfChunk &chunk) { fread_s(&chunk.iID,2,2,1,Model); fread_s(&chunk.iChunkSize,4,4,1,Model); chunk.iBytesRead=6; }; void SkipChunk(bfChunk &chunk) { fseek(Model,chunk.iChunkSize-chunk.iBytesRead,SEEK_CUR); };Was es mit dem ReadChunk auf sich hat, die Datei ist Binär und hat einen Kopf unter dem die Größe steht, ReadChunk liest die nächsten 6Bytes aus. Die Funktion kann dann schauen welcher Teil dort steht, das macht es fürs erweiternd er Datei besodners Leicht weil man die Reihenfolge der Daten nur bedingt beachten muß. Die abfrage siehst du ind er Funktion nicht, aber die Zugehörige Mutterfunktion prüft die Daten, die Unterfunktion tut es (noch) nicht. Das ganze ist im Grunde nur Mittel zur Prüfung.
-
Xebov schrieb:
Shade bemängelt den Rohspeicher, er wird geholt, befüllt, an eine andere Klasse durchgereicht und dann wieder gelöscht. Nun ist aber die andere Klasse eine Standalone Vertexbuffer Klasse, die ist nicht allein für den Gebrauch im Model vorgesehen, um den Speicher aber wegzulassen müsste ich die Vertexbuffer Klasse um neue Funktionen erweitern, das hieße aber auch das ich jedesmal wenn ich ein neues Format für die Modelle unterstützen will auch die Vertexbuffer Klasse umbauen müsste obwohl sie eigentlich mit dem Model nichts zu tun hat.
Nein.
Eine recht simple Moeglichkeit ist zB einen iterator auf die rohen Daten zu verwenden statt die rohe Datei anzufassen. Dann hast du keine interop probleme mehr. Oder aber eine init-Klasse die dir eine vertexbuffer Klasse erstellt (mit move semantik zb).
Und genau hier kann ich der Argumentation nicht folgen weil sich das ganze dnan auf noch mehr Funktionen verteilen würde und man nichtmehr sagen kann das das die Modelklasse für alles zuständig ist.
Weil die Modelklasse ja auch nicht fuer alles zustaendig sein soll
pumuckls Code ist schonmal nicht schlecht und er hat es ohne redesign geschafft - er hat die vorhandenen Funktionen nur aufgebrochen. Macht die Wartung schonmal um Dimensionen einfacher. Und vorallem: du hast ploetzlich keine abhaengigkeit von fread_s mehr.
Jetzt stell dir mal vor jemand anderer muss den Code warten. Welche Version wuerde ihm wohl besser gefallen? Deine oder pumuckls?
-
Danke für die weiteren Infos, aber ich denk mal als Skizze reicht das - werd mich jetzt wieder meinen eigenen Codes zuwenden
Ich denk mal dass meine Funktion getModelPart beim ersten Durchlesen schon eher ahnen lässt was vor sich geht als deine bisherige, wo man beim allerersten Lesen hauptsächlich einen großen Haufen ähnliche fread_s-Aufrufe sieht
-
Shade Of Mine schrieb:
Jetzt stell dir mal vor jemand anderer muss den Code warten. Welche Version wuerde ihm wohl besser gefallen? Deine oder pumuckls?
Ich finde pumuckls Variante nicht schlecht, wie er ja selbst gesagt hat nicht ganz Perfekt, ich hätte da einige Verbesserungsideen, aber Grundsätzlich finde ichs gut.
-
Wenn ich das richtig verstanden habe und in jedem Chunk anhand der ID abzulesen ist was danach kommt, dann ist ein ModelPart in der Datei ja so aufgebaut:
chunk mit ID = MODEL_PART chunk mit ID = MODEL_PART_VERTICES vertexvariablen chunk mit ID = MODEL_PART_INDICES indexvariablen optional: chunk mit ID = MODEL_PART_KOORDS modellnumer und koordinatenwenn dem so ist würd ichs wie folgt gliedern:
class ModelReader { bfModel model; bfModelPart modelPart; bfChunk chunk; public: bfModel readModelFromFile(std::string modelFileName) { //unter anderem: readAllModelParts(); } private: void readAllModelParts() { for (i = 0; i < model.iNumModelParts; ++i) { checkAndReadModelPart(); model.modelParts.push_back(modelPart); } } void checkAndReadModelPart() { checkNextChunkIDEquals(MODEL_PART); readModelPart(); } void checkNextChunkIDEquals(unsigned int ID) { readChunk(); if (chunk.iID != ID) throw UnexPectedChunkException; } void readModelPart() { checkAndReadVertices(); checkAndReadIndices(); readOptionalKoords(); } cheackAndReadVertices() { checkNextChunkIDEquals(MODEL_PART_VERTICCES); readVertices(); } checkAndReadIndices() { checkNextChunkIDEquals(MODEL_PART_INDICES); readIndices(); } readOptionalKoords() { if(testNextChunkID(MODEL_PART_KOORDS)) readKoords(); } bool testNextChunkID(unsigned int ID) { readChunk(); if (chunk.iID == ID) return true; stepBackOneChunk(); return false; } //etc. you get the idea ;) };Wieder nur eine Skizze, die beteiligten Klassen müssten natürlich etwas geändert werden (z.B. einen Container für die Modelparts statt eines dynamischen Arrays).
-
Kann man machen, ich bevorzuge aber ein switch(iID), das macht das erweitern etwas einfacher weil man nicht unbedingt auf die Reihenfolge achten muß.
-
Xebov schrieb:
Kann man machen, ich bevorzuge aber ein switch(iID), das macht das erweitern etwas einfacher weil man nicht unbedingt auf die Reihenfolge achten muß.
Natürlich, ich war allerdings davon ausgegangen dass du eine feste Reihenfolge erwartest weil die ReadChunks in deinem Code so fest eingemeißelt waren
-
pumuckl schrieb:
Xebov schrieb:
Kann man machen, ich bevorzuge aber ein switch(iID), das macht das erweitern etwas einfacher weil man nicht unbedingt auf die Reihenfolge achten muß.
Natürlich, ich war allerdings davon ausgegangen dass du eine feste Reihenfolge erwartest weil die ReadChunks in deinem Code so fest eingemeißelt waren
Ja ich weiß, die Prüfung findet nur im groben statt, im Detail hatte ich das nie Implementiert. Aber sagmal was hast du eig gegen "m_" bei Membern, ich sehe da gerade du schreibst die alle klein.
-
Xebov schrieb:
Aber sagmal was hast du eig gegen "m_" bei Membern, ich sehe da gerade du schreibst die alle klein.
es ist nicht mehr notwendig, ja sogar eher störend, wenn man schön kleine funktionen schreibt.
-
Xebov schrieb:
Aber sagmal was hast du eig gegen "m_" bei Membern.
Es liefert keinerlei zusätzlich Informationen. Nicht qualifizierte Variablen sind entweder in der Funktion deklariert oder Klassenmember oder global. Globale Variablen sind pfui und daher schlimmstenfalls sehr selten, bestenfalls nicht vorhanden. Lokale Variablen sind in der Funktion, also innerhalb der letzten ~10 Zeilen deklariert und daher muss alles was nicht vor kurzem deklariert wurde ein Klassenmember sein, auch ohne m_. Das m_ stört entweder den Lesefluss, oder man hat sich bereits dran gewöhnt und ignoriert es einfach. Und Information die man ignoriert kann man gleich ganz weglassen. Andersum kann man durchaus mit einem g_ globale Variablen von Klassenmembern absetzen, da sie erstens eh nur selten vorkommen und daher das g_ nicht so leicht ignoriert wird, und zweitens genug Aufmerksamkeit wert sind dass man beim Lesen schon mal drüber stolpern darf.