🔒 Locked

Memory allocation

Started by REspawn Apr 20, 2007 at 2:33 PM 9 replies 2.1k views

Original Post

Apr 20, 2007 at 2:33 PM

Hi, Im after running into a problem in c++ I have my model class, and inside this class there are structs, an example of one of these structs would be a vertex data struct, since i dont know until i read the model how many verts there will be i use a pointer and dynamic allocation. Everyhting in the model loader is working fine when i read a single file on its own off the desktop for example. But in the full system it reads the model files from inside one large file. But where i run into trouble is i get a std::bad_alloc error at random places in the file when declaring how big the dynamic arrays are. Is there something extra i need to do when i am going to be allocating memory like that? Cheers for any help, -Dave

KulSeran

195

KulSeran

Apr 20, 2007 at 3:44 PM

Debug and Check the allocation sizes before allocation.
You are likely attempting to allocate some huge number of bytes. This in turn is likely due to you reading the
wrong data from the file. (ie you lost your place in the file parsing)

REspawn

Author

REspawn

Apr 20, 2007 at 4:08 PM

Im not, i tought that to at first, so i steped through the program step by step as it read the data to make sure it was coming in right and it is.

Im 100% sure that the data coming in is correct.

Sneftel

296

Sneftel

Apr 20, 2007 at 4:10 PM

Step through it in the debugger and see what size arrays are being allocated.

REspawn

Author

REspawn

Apr 20, 2007 at 5:20 PM

100% right values coming in.

This is why it makes no sense to me, the file it messes up on the part just before it messes up is a char pointer dec.
string[0] = new char[20];

If i take that file out and just read it on its own from the desktop it dosent crash.

////

Edit: after the above test, i made the post and then i tought why not just loop the reading of the file i know works, if its a memory problem them it will mess up and it did.

I tried to read the file 20 times over, even with cleaning up the memory it used after each go it messed up.

////

Edit: Could using the same var to read each one cause problems, even if i delete it when done and use the new to recreate it for the next read?

yadango

Apr 20, 2007 at 5:30 PM

calling new char[20] shouldn't throw a bad_alloc, would really need to see your full loading function, important structure defs, and a sample file that makes your function go boom.

REspawn

Author

REspawn

Apr 20, 2007 at 5:48 PM

I uploaded the code...
http://www.turnipfan.com/dev/SampleProblem.rar

I also uploaded a tool for viewing the contents of the file and the format for the file incase its of use to anyone...
http://www.turnipfan.com/dev/EXTRAS.rar

Cheers for the help,
-Dave

Zahlman

257

Zahlman

Apr 20, 2007 at 11:53 PM

Um.

Oh my god.

I see you've been a member here since 2003. I don't know what you think you've been learning/getting taught since then, but it sure as hell doesn't look like anything I'd normally call "C++". I also don't really know how it's possible to write this much code of this caliber without suspecting that something must be wrong with the general approach.

You might want to hang around For Beginners for a while and try to get a fresh start. Also, RTFF to hopefully get at least a few good ideas.

Here's my first pass at fixing things, along with comments on things that popped out while getting this far. I'd estimate I've, so far, done less than 10% of the work that's really required to bring things up to standard.

(EDIT: Please note that because I don't have any D3D stuff set up locally, I have taken out references to those headers and put in stubs for the relevant types, in order to get the code into a state where I can at least compile and test the basic functionality. The alleged problem isn't to do with D3D, after all.)

(EDIT 2: Manually linewrapped stuff in the first box, per request.)

BAD EVIL WRONG THINGS.txt

- Don't rely on #pragma once.- 'Stream' is completely useless. This is C++; instead of putting your own potentially buggy wrappers around ancient APIs, use perfectly good new ones - in this case, <iostream>. (Worse, I can only assume you know about such a thing and are deliberately avoiding it for what will surely turn out to be a hilarious reason; my evidence is that you would otherwise not even think of the term "stream" to describe your object.) Not to mention, there isn't even such a thing as <stdio.h> any more. In C++, we spell that <cstdio>, on the mind-bogglingly bizarre occasions that we might see fit to use it.- Don't keep things as members unless they're logically part of the representation of the object. Member functions can have parameters just like free functions, you know.- RAII concept #1: Don't make 'load' functions. The constructor is supposed to do that work. If you pass a stream to a constructor, the expectation is that theconstructor will read the data from the stream. Don't use 'loaded' flags as members, either. If you can't load the object, it shouldn't exist; throw an exception from the constructor. Also, don't define a default constructor if there's nothing it can meaningfully do.- Re "where you should be": If something isn't what you expect, don't just set it and assume things are OK. There's a *reason* things didn't work out right, and you should probably *do something* about it; which in turn requires that you*find out*. This is what assertions and/or exceptions are for.- sprintf() is NOT a copying function. The following code is BROKEN:clump.geometryList.geometry->materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.strings[0].string = new char[header.sectionSize];	tempString = new char[header.sectionSize];	stream->getBytes(tempString, header.sectionSize);sprintf(clump.geometryList.geometry->materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.strings[0].string, tempString);	delete [] tempString;It will crash if the read-in 'tempString' happens to contain any formatspecifiers, because no arguments are explicitly provided. This is just one example of what's so incredibly evil about the old C-style I/O. But more to the point, sprintf() was never documented as being any kind of string-copying function; C provides strcpy() for that. But more to the point than that, there'sno need to copy here at all. The current process is:- Allocate buffer 1.- Allocate buffer 2.- Read data into buffer 2.- Copy contents of buffer 2 to buffer 1.- Deallocate buffer 2.Wouldn't it make more sense to just do this?- Allocate buffer 1.- Read data into buffer 1.:P (note that you can read into the buffer just fine before you assign to thatdata member).As it turns out, I'm going to replace all of this completely later, but just thought I'd point it out as an example of complete lack of understanding and/or thought.- On a related note: 0 should generally not be a special case. The following code is silly:if(header.sectionSize != 0)	stream->getbytes(&clump.geometryList.geometry->materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.extension.data, header.sectionSize);}If the size is indeed 0, then the call should correctly do nothing. Otherwise, the problem is in the implementation of getbytes().- Working with file offsets is usually wrong-minded. Normally, the things you need to read are in sequence, so after reading one item, you don't need to know where to see to in order to find the next - you are already there, by definition. The conclusion is that the 'fileoffset' parameter to the DFFModel constructor is likely to be useless in general. In fact, a quick inspection reveals that the 'offset' member of the class is set and then never actually used. So I removed it.- To facilitate the use of iostream for reading, I made a simple wrapper function around the iostream's .read() member function. This allows us to use the magic of templates to avoid needing to worry about the sizeof() what we're reading. This doesn't work quite right for pointers, of course (because it wouldtry to read in a value for the address, rather than reading stuff into the pointed-at region), so I made another function to create and fill a char allocation of a specified size. This is totally temporary and will be replaced later when things are in good enough shape that I can show you how to avoid doing all the memory management yourself. But it did lead at least to a little extra cleanup, namely being able to remove an ugly 'char* tempString' variable.- Don't artificially pause your programs at the end. Also, main() implicitly returns 0 at the end, so you don't need to worry about that, either.

SampleProblem.cpp

#include <fstream>#include <iostream>#include "DFFModel.h"int main(){	std::cerr << "TOP OF MAIN\n";	for(int i=0; i<20; i++){		std::ifstream dffstream("sample_file.dff", std::ios::binary);		std::cerr << i << '\n';		DFFModel* model = new DFFModel("sample_file.dff", dffstream);		delete model;		std::cerr << '~' << i << '\n';	}}

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;#pragma region -- Structures --// Vertexstruct Vertex{	float x, y, z;	DWORD color;	float u, v;	enum FVF{ FVF_Flags = 7  };};// Extensionstruct RWExtension{	char* data;};// Atmoicstruct RWAtomic{	uint frameNumber;	uint geometryNumber;	uint unknown1;	uint unknown2;};struct D3DVECTOR { float x, y, z; };typedef D3DVECTOR* LPD3DVECTOR;// Frame list datastruct RWFrameListData{	D3DVECTOR rotmatrixone;	D3DVECTOR rotmatrixtwo;	D3DVECTOR rotmatrixthree;	D3DVECTOR cordsoffset;	uint parentframe;	uint unknown;};// Frame liststruct RWFrameList{	RWFrameListData*	framesData;	RWExtension*		extensions;	uint				frameCount;	bool				loaded;};// Bounding infostruct RWGeometryBoundingInfo{	float boundingSpherex;	float boundingSpherey;	float boundingSpherez;	float boundingSphereRadius;	uint hasPosition;	uint hasNormals;};// Geometry color infostruct RWGeometryColorInfo{	byte red;	byte green;	byte blue;	byte alpha;};// Geometry face infostruct RWGeometryFaceInfo{	word vertex2;	word vertex1;	word flags;	word vertex3;};// Grometry Mapping infostruct RWGeometryMappingInfo{	float u;	float v;};// Geometry Normal infostruct RWGeometryNormalInfo{	float x;	float y;	float z;};// Geometry Vertex Infostruct RWGeometryVertexInfo{	float x;	float y;	float z;};// Stringstruct RWString{	char* string;};// Texturestruct RWTexture{	word		textureFilter;	word		unknown;	RWString	strings[2];	RWExtension extension;	bool		uninitalized;};// Materialstruct RWMaterial{	RWTexture texture;	uint unknown;	uint red;	uint green;	uint blue;	uint alpha;	uint unknown2;	uint textureCount;	uint unknown3;	uint unknown4;	uint unknown5;};// Material Liststruct RWMaterialList{	RWMaterial*	materials;	uint		materialCount;	uint		materialPos;	uint*		unknown;};// Material Split Infostruct RWMaterialSplitInfo{	uint	faceIndex;	uint	materialIndex;	uint*	vertex;};// Material Splitstruct RWMaterialSplit{	uint					triangleStrip;	uint					splitCount;	uint					faceCount;	RWMaterialSplitInfo*	spiltInfo;};//Subset mapstruct SubsetMap{	uint textureIndex;	uint subsetId;};struct D3DXMESH {	void Release() {}};typedef D3DXMESH* LPD3DXMESH;// Geometrystruct 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;};// Geometry Liststruct RWGeometryList{	RWGeometry* geometry;	uint		geometryCount;};// Clumpstruct RWClump{	uint			objectCount;	RWFrameList		frameList;	RWGeometryList	geometryList;	RWAtomic		atomic;	RWExtension		extension;};//Section Headerstruct SectionHeader{	uint sectionID;	uint sectionSize;	uint version;};#pragma endregionclass 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);	//variables	char			file[256];	SectionHeader	header;	SectionHeader	previousHeader;	RWClump			clump;	uint			geometryPos;};#endif

DFFModel.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(){	int x;	for(int i=0; i<clump.geometryList.geometryCount; i++){		if(clump.geometryList.geometry.loaded){			if(clump.geometryList.geometry.meshCreated)				clump.geometryList.geometry.mesh->Release();						if(clump.geometryList.geometry.hasColors)				delete [] clump.geometryList.geometry.colorinfo;			if(clump.geometryList.geometry.hasMapping)				delete [] clump.geometryList.geometry.mappinginfo;			delete [] clump.geometryList.geometry.faceinfo;			delete [] clump.geometryList.geometry.vertexInfo;						if(clump.geometryList.geometry.hasNormals)				delete [] clump.geometryList.geometry.normalInfo;						delete [] clump.geometryList.geometry.subsetMap;			for(x=0; x<clump.geometryList.geometry.materialsplit.splitCount; x++)				delete [] clump.geometryList.geometry.materialsplit.spiltInfo[x].vertex;			delete [] clump.geometryList.geometry.materialsplit.spiltInfo;			for(x=0; x<clump.geometryList.geometry.materiallist.materialCount; x++){				if(clump.geometryList.geometry.materiallist.materials[x].texture.uninitalized == false){					delete [] clump.geometryList.geometry.materiallist.materials[x].texture.strings[0].string;					delete [] clump.geometryList.geometry.materiallist.materials[x].texture.strings[1].string;				}			}			delete [] clump.geometryList.geometry.materiallist.unknown;		}	}	delete [] clump.geometryList.geometry;	//Memory leak, fix this	/*if(clump.frameList.loaded){		for(x=0; x<clump.frameList.frameCount; x++)			delete [] clump.frameList.extensions[x].data;		delete [] clump.frameList.extensions;		delete [] clump.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;		case 39056126:			//parseFrame(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){	int i, tempInt;	readBinary(is, clump.frameList.frameCount);	clump.frameList.framesData = new RWFrameListData[clump.frameList.frameCount];	clump.frameList.extensions = new RWExtension[clump.frameList.frameCount];	clump.frameList.loaded = true;			for(i=0; i<clump.frameList.frameCount; i++){		readBinary(is, clump.frameList.framesData.rotmatrixone);		readBinary(is, clump.frameList.framesData.rotmatrixtwo);		readBinary(is, clump.frameList.framesData.rotmatrixthree);		readBinary(is, clump.frameList.framesData.cordsoffset);		readBinary(is, clump.frameList.framesData.parentframe);		readBinary(is, clump.frameList.framesData.unknown);	}		for(i=0; i<clump.frameList.frameCount; i++){		readBinary(is, tempInt);		if (tempInt == 3){			is.seekg(-4, cur);			previousHeader = header;			readBinary(is, header);			clump.frameList.extensions.data = createBuffer(is, header.sectionSize);		}		else			is.seekg(-4, cur);	}		readHeader(is);}void DFFModel::parseGeometryList(istream& is){	readHeader(is);}void DFFModel::parseGeometryListData(istream& is){	readBinary(is, clump.geometryList.geometryCount);	clump.geometryList.geometry = new RWGeometry[clump.geometryList.geometryCount];	for(int x=0; x<clump.geometryList.geometryCount; x++){		clump.geometryList.geometry[x].loaded		 = false;		clump.geometryList.geometry[x].hasColors	 = false;		clump.geometryList.geometry[x].hasMapping	 = false;		clump.geometryList.geometry[x].hasNormals	 = false;		clump.geometryList.geometry[x].meshCreated	 = false;		clump.geometryList.geometry[x].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);	int i = 0;	readBinary(is, clump.geometryList.geometry[geometryPos].flags);	readBinary(is, clump.geometryList.geometry[geometryPos].unknown);	readBinary(is, clump.geometryList.geometry[geometryPos].triangleCount);	readBinary(is, clump.geometryList.geometry[geometryPos].vertexCount);	readBinary(is, clump.geometryList.geometry[geometryPos].morphTargetCount);		if (header.version == 134283263 || header.version == 784){		readBinary(is, clump.geometryList.geometry[geometryPos].ambient);		readBinary(is, clump.geometryList.geometry[geometryPos].diffuse);		readBinary(is, clump.geometryList.geometry[geometryPos].specular);	}	//Color info	if ((8 & clump.geometryList.geometry[geometryPos].flags) == 8){		clump.geometryList.geometry[geometryPos].hasColors = true;		clump.geometryList.geometry[geometryPos].colorinfo = new RWGeometryColorInfo[clump.geometryList.geometry[geometryPos].vertexCount];		for(i=0; i<clump.geometryList.geometry[geometryPos].vertexCount; i++)			readBinary(is, clump.geometryList.geometry[geometryPos].colorinfo);	}	//Mapping info	if((4 & clump.geometryList.geometry[geometryPos].flags) == 4){		clump.geometryList.geometry[geometryPos].hasMapping = true;		clump.geometryList.geometry[geometryPos].mappinginfo = new RWGeometryMappingInfo[clump.geometryList.geometry[geometryPos].vertexCount];		for(i=0; i<clump.geometryList.geometry[geometryPos].vertexCount; i++)			readBinary(is, clump.geometryList.geometry[geometryPos].mappinginfo);			}	//Face info	clump.geometryList.geometry[geometryPos].faceinfo = new RWGeometryFaceInfo[clump.geometryList.geometry[geometryPos].triangleCount];	for(i=0; i<clump.geometryList.geometry[geometryPos].triangleCount; i++)		readBinary(is, clump.geometryList.geometry[geometryPos].faceinfo);	//Bounding info	readBinary(is, clump.geometryList.geometry[geometryPos].boundingInfo);	//Vertex info	clump.geometryList.geometry[geometryPos].vertexInfo = new RWGeometryVertexInfo[clump.geometryList.geometry[geometryPos].vertexCount];	for(i=0; i<clump.geometryList.geometry->vertexCount; i++)		readBinary(is, clump.geometryList.geometry[geometryPos].vertexInfo);	//Normal info	if((16 & clump.geometryList.geometry[geometryPos].flags) == 16){		clump.geometryList.geometry[geometryPos].normalInfo = new RWGeometryNormalInfo[clump.geometryList.geometry[geometryPos].vertexCount];		for(i=0; i<clump.geometryList.geometry[geometryPos].vertexCount; i++)			readBinary(is, clump.geometryList.geometry[geometryPos].normalInfo);	}	//check position -- I R REDUNDANT NOW!	assert(is.tellg() == whereYouShouldBe);	clump.geometryList.geometry[geometryPos].loaded = true;	readHeader(is);}void DFFModel::parseMaterialList(istream& is){	readHeader(is);}void DFFModel::parseMaterialListData(istream& is){	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materialCount);	clump.geometryList.geometry[geometryPos].materiallist.materials = new RWMaterial[clump.geometryList.geometry[geometryPos].materiallist.materialCount];	clump.geometryList.geometry[geometryPos].materiallist.unknown   = new uint[clump.geometryList.geometry[geometryPos].materiallist.materialCount];	clump.geometryList.geometry->materiallist.materialPos = (uint)-1;	for(int i=0; i<clump.geometryList.geometry[geometryPos].materiallist.materialCount; i++)		readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.unknown);		readHeader(is);}void DFFModel::parseMaterialSplit(istream& is){	int x;	readBinary(is, clump.geometryList.geometry[geometryPos].materialsplit.triangleStrip);	readBinary(is, clump.geometryList.geometry[geometryPos].materialsplit.splitCount);	readBinary(is, clump.geometryList.geometry[geometryPos].materialsplit.faceCount);	clump.geometryList.geometry[geometryPos].materialsplit.spiltInfo = new RWMaterialSplitInfo[clump.geometryList.geometry[geometryPos].materialsplit.splitCount];	clump.geometryList.geometry[geometryPos].subsetMap =			   new SubsetMap[clump.geometryList.geometry[geometryPos].materialsplit.splitCount];	for(int i=0; i<clump.geometryList.geometry[geometryPos].materialsplit.splitCount; i++){		readBinary(is, clump.geometryList.geometry[geometryPos].materialsplit.spiltInfo.faceIndex);		readBinary(is, clump.geometryList.geometry[geometryPos].materialsplit.spiltInfo.materialIndex);				clump.geometryList.geometry[geometryPos].materialsplit.spiltInfo.vertex = new uint[clump.geometryList.geometry[geometryPos].materialsplit.spiltInfo.faceIndex];		for(x=0; x<clump.geometryList.geometry[geometryPos].materialsplit.spiltInfo.faceIndex; x++)			readBinary(is, clump.geometryList.geometry[geometryPos].materialsplit.spiltInfo.vertex[x]);	}		readHeader(is);}void DFFModel::parseMaterial(istream& is){	clump.geometryList.geometry[geometryPos].materiallist.materialPos++;	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].unknown);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].red);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].green);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].blue);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].alpha);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].unknown2);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].textureCount);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].unknown3);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].unknown4);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].unknown5);			readHeader(is);}void DFFModel::parseTexture(istream& is){	readHeader(is);}void DFFModel::parseTextureData(istream& is){	int tempInt;			clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.uninitalized = false; //extra check for when reclaiming memory	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.textureFilter);	readBinary(is, clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.unknown);	readBinary(is, tempInt);	if (tempInt == 2) {		is.seekg(-4, cur);		previousHeader = header;		readBinary(is, header);		clump.geometryList.geometry->materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.strings[0].string = createBuffer(is, header.sectionSize);		readBinary(is, tempInt);		if(tempInt == 2){			is.seekg(-4, cur);			previousHeader = header;			readBinary(is, header);			clump.geometryList.geometry[geometryPos].materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.strings[1].string = createBuffer(is, header.sectionSize);			readBinary(is, tempInt);			if(tempInt == 3){				is.seekg(-4, cur);				previousHeader = header;				readBinary(is, header);				clump.geometryList.geometry->materiallist.materials[clump.geometryList.geometry[geometryPos].materiallist.materialPos].texture.extension.data = createBuffer(is, header.sectionSize);			}			else				is.seekg(-4, cur);		}		else			is.seekg(-4, cur);	}	else		is.seekg(-4, cur);	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);}

TBH, I didn't even really know where to start, because there's so much redundancy, disorganization, and general ignorance of the standard library here. I decided to bite a big bullet and temporarily ignore the redundancy and disorganization, and start by attacking the specific bits of ignorance of the standard library that seem almost *willful* - in particular, the reinvention of a "stream". And I'm not even done with that; in pass 2, in addition to putting in std::strings where appropriate, I'll be putting in std::vectors to replace manually managed (count, array) pairs. I'll note that your original comments for SampleProblem.cpp make reference to 'a vector', so you seem to know this thing exists. Why not USE IT?

[Edited by - Zahlman on April 21, 2007 3:53:40 PM]

REspawn

Author

REspawn

Apr 21, 2007 at 10:21 AM

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++.

Sent you a pm.

iMalc

182

iMalc

Apr 21, 2007 at 4:01 PM

Applause to Zahlman for putting so much time and effort into that post![cool]

(and for making it fit better across the screen width)

Zahlman

257

Zahlman

Apr 22, 2007 at 4:51 PM

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;};#endif

DFFModel.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);}

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.