Quote:Original post by REspawn
Cheers Zahlman.
I have only been doing c++ for about 2/3 weeks now, i was using c# and mdx until then and i got sick of the speed so i moved to c++.
Well... the techniques I'm going to be showing you here are largely applicable in any language (although C# doesn't have you worrying so much - if at all - about memory management, the general idea of "use the standard library, already" always applies, and there are also techniques shown here WRT organizing your code and avoiding redundancy).
As for performance, I really suspect your problems are elsewhere... the language won't make up for an inefficient design, and C# actually is capable of some surprising optimization and can approach C++'s speed for a lot of tasks.
And as for your PM... I'd rather finish work on the one piece of code here, mmkay? Hopefully you will get enough good ideas from this to figure out what to do with the rest of your stuff. I only have X amount of time to spare before I have to start charging for the service ;s
Anyway. In pass 2, I cut down a bunch of the redundancy in the code (accessing deeply nested members over and over), mainly by using references, as a stop-gap measure (until we can organize things properly). I also found some bugs (well, some instances of one type of bug), which might be all that was actually "going wrong" (while I couldn't get the code to crash for me with the provided sample model, that doesn't mean it was ok ;P ): in a few places, you had a 'geometry->' that should have been 'geometry[some subscript].', presumably left over from changing that member from being a scalar to being an array. (Note that at the time that you dereference, the distinction is lost; 'geometry->' is equivalent to 'geometry[0].', so those lines would access the beginning element of the array and thus be inconsistent with surrounding statements working with some other element).
Note that with the technique shown in pass 2, it becomes effectively impossible to make those kinds of mistakes. But note also that the techniques from the next two or so passes will also prevent that class of mistakes, and are also moving the code closer to the goal. ;)
BAD EVIL WRONG THINGS.txt (Actually, I am only listing the new stuff here, but my copy of the file includes the stuff from pass 1.)
- Made formatting more consistent in general. I would like to strongly recommend putting in braces for all your if's, for's etc. even where the body is a single statement - it becomes a lot safer that way (otherwise you can more easily make mistakes when adding or removing code).- Don't write comments that don't say anything. In particular, commenting a structure with a comment that simply restates the name of the structure, or labelling the member variables of a class as "variables", tells the reader exactly nothing useful (and hardly gives a better target for searching, either).- Declare variables near first use. Don't pre-declare a variable to be used for loops and then reuse it. It obfuscates your code and will not make things any faster. Note that in C++, you can declare your loop counter within the for-statement, and it will scope that variable to the for-body.- In C++, prefer pre-increment rather than post-increment for loop counters. (We'll actually be replacing a lot of these "counted" loops eventually, but...)- Don't compare to boolean literals. The thing that you're comparing with is already a boolean: "if it is raining" is clearer than "if it is true that it is raining", and "if it is not raining" is clearer than "if it is false that it is raining". Write clear things. (Also, clear, short expressions are less likely to contain mistakes, because they contain fewer points at which a mistake could be made.)- Don't comment out code to disable it. Use a version control system instead. (I restored the commented-out code from ~DFFModel, assuming that it was part of the reported problem.)- Use references to avoid repeated, well, references to the same object. References are the pronouns of the C++ language; they let you alias things. Examples are shown in the destructor and elsewhere. HOWEVER, the need for this technique suggests further design problems, namely that responsibility for tasks is not being properly assigned. The DFFModel is doing all the work, when most of it should be delegated to the contained objects. This will be addressed in a later pass over the code. Of course, for primitive types, it's generally OK to just copy the value into a temporary to refer to it. (But note that changes to the copy won't affect the original, as does happen with a reference.) Alternately, instead of reading into a data member and then repeatedly referring to the member, read into a temporary and refer to the (much more simply named) temporary repeatedly, and also copy the temporary to the member. (This is different from the "stupid string logic" situation from pass 1 in two ways: first, we're working with a primitive type, and second, the value is referred to several times instead of just once.- Watch out for this pattern:if (foo) { do(something); do(something_else);} else { do(something);}You're going to 'do something' first, regardless, so it's cleaner to just do it first, and avoid creating the impression that it somehow has something to do with the if-logic:do something;if (foo) { do(something_else);}Notice that this time (as well as in several places in the actual code) this allows avoiding an else-clause altogether.- I assume this: for (int i = 0; i < clump.geometryList.geometry->vertexCount; ++i) readBinary(is, clump.geometryList.geometry[geometryPos].vertexInfo);should have been: for (int i = 0; i < clump.geometryList.geometry[geometryPos].vertexCount; ++i) readBinary(is, clump.geometryList.geometry[geometryPos].vertexInfo);See, this would be one of the problems that copying-and-pasting code gets you into. ;) And potentially one of the bugs that may have led to your problems with memory allocation. As it was, it compiles (because 'clump.geometryList.geometry' is a pointer, after all), but would always use the vertexCount of element 0 to decide how many vertices to read. If one of the geometries had fewer vertices than the zeroth, then its array would get overflowed, invoking undefined behaviour.A similar problem occurs in parseMaterialListData: clump.geometryList.geometry->materiallist.materialPos = (uint)-1;Here, the materialPos of the zeroth geometry gets repeatedly reset, while others are uninitialized.Again, in parseTextureData, a similar problem. Of course, it's easy to lose these ->s in a sea of text like 'clump.geometryList.geometry->materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.strings[0].string' :S- tempInt isn't any better of a variable name than temp.DFFModel.h
#ifndef DFF_MODEL_H#define DFF_MODEL_H#include <iosfwd>typedef unsigned uint;typedef unsigned short word;typedef unsigned char byte;typedef int DWORD;struct Vertex { float x, y, z; DWORD color; float u, v; enum FVF { FVF_Flags = 7 };};struct RWExtension { char* data;};struct RWAtomic { uint frameNumber; uint geometryNumber; uint unknown1; uint unknown2;};struct D3DVECTOR { float x, y, z; };typedef D3DVECTOR* LPD3DVECTOR;struct RWFrameListData{ D3DVECTOR rotmatrixone; D3DVECTOR rotmatrixtwo; D3DVECTOR rotmatrixthree; D3DVECTOR cordsoffset; uint parentframe; uint unknown;};struct RWFrameList { RWFrameListData* framesData; RWExtension* extensions; uint frameCount; bool loaded;};struct RWGeometryBoundingInfo { float boundingSpherex; float boundingSpherey; float boundingSpherez; float boundingSphereRadius; uint hasPosition; uint hasNormals;};struct RWGeometryColorInfo { byte red; byte green; byte blue; byte alpha;};struct RWGeometryFaceInfo { word vertex2; word vertex1; word flags; word vertex3;};struct RWGeometryMappingInfo { float u; float v;};struct RWGeometryNormalInfo { float x; float y; float z;};struct RWGeometryVertexInfo { float x; float y; float z;};struct RWString { char* string;};struct RWTexture { word textureFilter; word unknown; RWString strings[2]; RWExtension extension; bool uninitalized;};struct RWMaterial { RWTexture texture; uint unknown; uint red; uint green; uint blue; uint alpha; uint unknown2; uint textureCount; uint unknown3; uint unknown4; uint unknown5;};struct RWMaterialList { RWMaterial* materials; uint materialCount; uint materialPos; uint* unknown;};struct RWMaterialSplitInfo { uint faceIndex; uint materialIndex; uint* vertex;};struct RWMaterialSplit { uint triangleStrip; uint splitCount; uint faceCount; RWMaterialSplitInfo* spiltInfo;};struct SubsetMap { uint textureIndex; uint subsetId;};struct D3DXMESH { void Release() {}};typedef D3DXMESH* LPD3DXMESH;struct RWGeometry { LPD3DXMESH mesh; word flags; word unknown; uint triangleCount; uint vertexCount; uint morphTargetCount; float ambient; float diffuse; float specular; RWGeometryColorInfo* colorinfo; RWGeometryMappingInfo* mappinginfo; RWGeometryFaceInfo* faceinfo; RWGeometryBoundingInfo boundingInfo; RWGeometryVertexInfo* vertexInfo; RWGeometryNormalInfo* normalInfo; RWMaterialList materiallist; RWMaterialSplit materialsplit; SubsetMap* subsetMap; bool hasColors; bool hasMapping; bool hasBaseColors; bool hasNormals; bool loaded; bool meshCreated;};struct RWGeometryList { RWGeometry* geometry; uint geometryCount;};struct RWClump { uint objectCount; RWFrameList frameList; RWGeometryList geometryList; RWAtomic atomic; RWExtension extension;};struct SectionHeader { uint sectionID; uint sectionSize; uint version;};class DFFModel { public: DFFModel(char* sourcefile, std::istream& is); ~DFFModel(); private: void readHeader(std::istream& is); void parseSection(std::istream& is); void parseData(std::istream& is); void parseClump(std::istream& is); void parseClumpData(std::istream& is); void parseFrameList(std::istream& is); void parseFrameListData(std::istream& is); void parseGeometryList(std::istream& is); void parseGeometryListData(std::istream& is); void parseGeometry(std::istream& is); void parseGeometryData(std::istream& is); void parseMaterialListData(std::istream& is); void parseMaterialList(std::istream& is); void parseMaterialSplit(std::istream& is); void parseMaterial(std::istream& is); void parseTexture(std::istream& is); void parseTextureData(std::istream& is); void parseAtomic(std::istream& is); void parseAtomicData(std::istream& is); void parseExtension(std::istream& is); void parseUnused(std::istream& is); char file[256]; SectionHeader header; SectionHeader previousHeader; RWClump clump; uint geometryPos;};#endifDFFModel.cpp
#include "DFFModel.h"#include <iostream>using std::istream;std::ios_base::seekdir cur = std::ios_base::cur;template <typename T>void readBinary(istream& is, T& t) { is.read(reinterpret_cast<char*>(&t), sizeof(t));}char* createBuffer(istream& is, int size) { char* result = new char[size]; is.read(result, size); return result;}template <typename T>void readBinary(istream& is, T*& t) { T::cannot_use_this_function_with_a_pointer;}DFFModel::DFFModel(char* sourcefile, istream& is) { strcpy(file, sourcefile); geometryPos = (uint)-1; readHeader(is);}DFFModel::~DFFModel() { for (int i = 0; i < clump.geometryList.geometryCount; ++i){ RWGeometry& geometry = clump.geometryList.geometry; if (geometry.loaded) { if (geometry.meshCreated) { geometry.mesh->Release(); } if (geometry.hasColors) { delete[] geometry.colorinfo; } if (geometry.hasMapping) { delete[] geometry.mappinginfo; } delete[] geometry.faceinfo; delete[] geometry.vertexInfo; if (geometry.hasNormals) { delete[] geometry.normalInfo; } delete[] geometry.subsetMap; RWMaterialSplit& materialsplit = geometry.materialsplit; for (int j = 0; j < materialsplit.splitCount; ++j) { delete[] materialsplit.spiltInfo[j].vertex; } delete[] materialsplit.spiltInfo; RWMaterialList& materiallist = geometry.materiallist; for (int j = 0; j < materiallist.materialCount; ++j) { RWTexture& texture = materiallist.materials[j].texture; if (!texture.uninitalized) { delete[] texture.strings[0].string; delete[] texture.strings[1].string; } } delete[] materiallist.unknown; } } delete[] clump.geometryList.geometry; RWFrameList& frameList = clump.frameList; if (frameList.loaded) { for (int j = 0; j < frameList.frameCount; ++j) { delete[] frameList.extensions[j].data; } delete[] frameList.extensions; delete[] frameList.framesData; }}//------------------------------------------------------------------------------// All functions below this line related to reading the dff file, one section// at a time//------------------------------------------------------------------------------void DFFModel::readHeader(istream& is) { previousHeader = header; readBinary(is, header); parseSection(is);}void DFFModel::parseSection(istream& is) { switch (header.sectionID) { case 1: parseData(is); break; case 2: parseUnused(is); break; case 3: parseExtension(is); break; case 6: parseTexture(is); break; case 7: parseMaterial(is); break; case 8: parseMaterialList(is); break; case 14: parseFrameList(is); break; case 15: parseGeometry(is); break; case 16: parseClump(is); break; case 20: parseAtomic(is); break; case 26: parseGeometryList(is); break; case 1294: parseMaterialSplit(is); break; default: parseUnused(is); break; }}void DFFModel::parseData(istream& is) { switch (previousHeader.sectionID) { case 6: parseTextureData(is); break; case 8: parseMaterialListData(is); break; case 14: parseFrameListData(is); break; case 15: geometryPos++; parseGeometryData(is); break; case 16: parseClumpData(is); break; case 20: parseAtomicData(is); break; case 26: parseGeometryListData(is); break; }}void DFFModel::parseClump(istream& is){ readHeader(is);}void DFFModel::parseClumpData(istream& is) { readBinary(is, clump.objectCount); if (header.version == 268697599 || header.version == 402915327 || header.version == 469893130) { is.ignore(8); } clump.frameList.loaded = false; readHeader(is);}void DFFModel::parseFrameList(istream& is) { readHeader(is);}void DFFModel::parseFrameListData(istream& is) { RWFrameList& frameList = clump.frameList; int count; readBinary(is, count); frameList.frameCount = count; frameList.framesData = new RWFrameListData[count]; frameList.extensions = new RWExtension[count]; frameList.loaded = true; for (int i = 0; i < count; ++i) { RWFrameListData frameData = frameList.framesData; readBinary(is, frameData.rotmatrixone); readBinary(is, frameData.rotmatrixtwo); readBinary(is, frameData.rotmatrixthree); readBinary(is, frameData.cordsoffset); readBinary(is, frameData.parentframe); readBinary(is, frameData.unknown); } for (int i = 0; i < count; ++i) { int temp; readBinary(is, temp); is.seekg(-4, cur); if (temp == 3) { previousHeader = header; readBinary(is, header); frameList.extensions.data = createBuffer(is, header.sectionSize); } } readHeader(is);}void DFFModel::parseGeometryList(istream& is) { readHeader(is);}void DFFModel::parseGeometryListData(istream& is) { int count; readBinary(is, count); clump.geometryList.geometryCount = count; clump.geometryList.geometry = new RWGeometry[count]; for (int x = 0; x < count; x++) { RWGeometry& geometry = clump.geometryList.geometry[x]; geometry.loaded = false; geometry.hasColors = false; geometry.hasMapping = false; geometry.hasNormals = false; geometry.meshCreated = false; geometry.hasBaseColors = false; } readHeader(is);}void DFFModel::parseGeometry(istream& is) { readHeader(is);}void DFFModel::parseGeometryData(istream& is) { std::streampos whereYouShouldBe = is.tellg() + std::streamoff(header.sectionSize); RWGeometry& geometry = clump.geometryList.geometry[geometryPos]; readBinary(is, geometry.flags); readBinary(is, geometry.unknown); readBinary(is, geometry.triangleCount); readBinary(is, geometry.vertexCount); readBinary(is, geometry.morphTargetCount); if (header.version == 134283263 || header.version == 784) { readBinary(is, geometry.ambient); readBinary(is, geometry.diffuse); readBinary(is, geometry.specular); } if ((8 & geometry.flags) == 8) { geometry.hasColors = true; geometry.colorinfo = new RWGeometryColorInfo[geometry.vertexCount]; for (int i = 0; i < geometry.vertexCount; ++i) { readBinary(is, geometry.colorinfo); } } if ((4 & geometry.flags) == 4) { geometry.hasMapping = true; geometry.mappinginfo = new RWGeometryMappingInfo[geometry.vertexCount]; for (int i = 0; i < geometry.vertexCount; ++i) { readBinary(is, geometry.mappinginfo); } } geometry.faceinfo = new RWGeometryFaceInfo[geometry.triangleCount]; for (int i = 0; i < geometry.triangleCount; ++i) { readBinary(is, geometry.faceinfo); } readBinary(is, geometry.boundingInfo); geometry.vertexInfo = new RWGeometryVertexInfo[geometry.vertexCount]; for (int i = 0; i < geometry.vertexCount; ++i) { readBinary(is, geometry.vertexInfo); } if ((16 & geometry.flags) == 16) { geometry.normalInfo = new RWGeometryNormalInfo[geometry.vertexCount]; for (int i = 0; i < geometry.vertexCount; ++i) { readBinary(is, geometry.normalInfo); } } //check position -- I R REDUNDANT NOW! assert(is.tellg() == whereYouShouldBe); geometry.loaded = true; readHeader(is);}void DFFModel::parseMaterialList(istream& is) { readHeader(is);}void DFFModel::parseMaterialListData(istream& is) { RWMaterialList& materiallist = clump.geometryList.geometry[geometryPos].materiallist; int count; readBinary(is, count); materiallist.materialCount = count; materiallist.materials = new RWMaterial[count]; materiallist.unknown = new uint[count]; materiallist.materialPos = (uint)-1; for (int i = 0; i < count; ++i) { readBinary(is, materiallist.unknown); } readHeader(is);}void DFFModel::parseMaterialSplit(istream& is) { RWGeometry& geometry = clump.geometryList.geometry[geometryPos]; RWMaterialSplit& materialsplit = geometry.materialsplit; int count; readBinary(is, materialsplit.triangleStrip); readBinary(is, count); materialsplit.splitCount = count; readBinary(is, materialsplit.faceCount); materialsplit.spiltInfo = new RWMaterialSplitInfo[count]; geometry.subsetMap = new SubsetMap[count]; for (int i = 0; i < count; ++i) { RWMaterialSplitInfo& splitinfo = materialsplit.spiltInfo; int faceCount; readBinary(is, faceCount); splitinfo.faceIndex = faceCount; readBinary(is, splitinfo.materialIndex); splitinfo.vertex = new uint[faceCount]; for (int j = 0; j < faceCount; ++j) { readBinary(is, splitinfo.vertex[j]); } } readHeader(is);}void DFFModel::parseMaterial(istream& is) { RWMaterialList& materiallist = clump.geometryList.geometry[geometryPos].materiallist; materiallist.materialPos++; RWMaterial& material = materiallist.materials[materiallist.materialPos]; readBinary(is, material.unknown); readBinary(is, material.red); readBinary(is, material.green); readBinary(is, material.blue); readBinary(is, material.alpha); readBinary(is, material.unknown2); readBinary(is, material.textureCount); readBinary(is, material.unknown3); readBinary(is, material.unknown4); readBinary(is, material.unknown5); readHeader(is);}void DFFModel::parseTexture(istream& is) { readHeader(is);}void DFFModel::parseTextureData(istream& is) { int temp; RWMaterialList& materiallist = clump.geometryList.geometry[geometryPos].materiallist; RWMaterial& material = materiallist.materials[materiallist.materialPos]; RWTexture& texture = material.texture; texture.uninitalized = false; readBinary(is, texture.textureFilter); readBinary(is, texture.unknown); readBinary(is, temp); is.seekg(-4, cur); if (temp == 2) { previousHeader = header; readBinary(is, header); texture.strings[0].string = createBuffer(is, header.sectionSize); readBinary(is, temp); is.seekg(-4, cur); if (temp == 2) { previousHeader = header; readBinary(is, header); texture.strings[1].string = createBuffer(is, header.sectionSize); readBinary(is, temp); is.seekg(-4, cur); if (temp == 3) { previousHeader = header; readBinary(is, header); texture.extension.data = createBuffer(is, header.sectionSize); } } } readHeader(is);}void DFFModel::parseAtomic(istream& is) { readHeader(is);}void DFFModel::parseAtomicData(istream& is) { readBinary(is, clump.atomic);}void DFFModel::parseExtension(istream& is) { readHeader(is);}void DFFModel::parseUnused(istream& is) { is.ignore(header.sectionSize); readHeader(is);}
