My most amazing error yet! [complicated]

[therabidwombat]

Alright, so for school, we're working with direct x to create a 3D engine that will have heightmaps, cameras, animations, collision, etc...I've been working on it for four weeks now, and of that, over three weeks I've spent stumped without even heightmaps working. The reason for this completely evades me; my code is simply functioning in ways that do not make sense to me.

The initial problem was in my heightMap::init() function. The function takes a few values:

bool heightMap::initHeightmap(IDirect3DDevice9 *nDevice, char *fname, int nNumVertsPerRow, int nNumVertsPerCol, int nCellSpacing, float nHeightScale)
{

When in debug mode, all of those values were what I expected them to be (a valid pointer, CoastMountain64.raw, 64, 64, 10, 0.5).

However, when I tried to assign those values to members of my class, it would not work. For example:

numVertsPerCol = nNumVertsPerCol;

would result in numVertsPerCol having a value of -1163005939 (copied directly from debug mode). All of the assignments in my init function have had similar results. In order to solve this, I had to use while loops, as follows:

while(numVertsPerCol != nNumVertsPerCol) numVertsPerCol = nNumVertsPerCol;

This solved the problem, but I think that somehow that made things worse. All of my assignments worked properly, but my heightmap was crashing my program. I traced it and found that in the computeInds() function, which was supposed to compute the indices, the class was erasing itself. Here's the code from the computeInds() function:

bool heightMap::computeInds()	
{
	HRESULT hr = 0;
	hr = device->CreateIndexBuffer(numTriangles * 3 * sizeof(WORD), D3DUSAGE_WRITEONLY,D3DFMT_INDEX16, D3DPOOL_MANAGED, &ib,NULL);
	if(hr != D3D_OK) return false;
	WORD *indices = new WORD[cellWidth * cellHeight - 1];
	if(ib->Lock(0, 0, (void**)&indices, 0) != D3D_OK)
			debugger::print("Could not lock index buffer.");
	int pos = 0;	
	for(int i = 0; i < cellWidth; i++)
	{
		for(int j = 0; j < cellHeight; j++)
		{
			indices[pos]	= i * vertsPerRow + j;
			indices[pos+1]= i * vertsPerRow + j + 1;
			indices[pos+2]= (i+1) * vertsPerRow + j;
			indices[pos + 3]= (i+1)*vertsPerRow + j;
			indices[pos + 4] = i * vertsPerRow + j + 1;
			indices[pos + 5]= (i+1) * vertsPerRow + j + 1;	
		pos += 6;
		}//for(int j = 0; j < cellHeight; j++)
	}//for(int i = 0; i < cellWidth; i++)
	ib->Unlock(	);
	return true;					
}

initially, the class was erasing itself on the WORD* indices... line, but when I commented that line out, the next line erased the class, and if I commented the entire function out until the beggining of the first for loop, it would still erase the class.

Eventually, I stopped doing compute inds inside a function, and tried moving it into the init function, but when I did that, the class begin erasing itself on the screwed up while loops, and putting compute inds back into a function didn't fix this.

I gave up and began recoding my 3D engine again. However, I was encountering all the same problems as before, except that this time, the class didn't erase itself in the computeInds function, but in the function that reads a raw file. Frustrated, and worried that perhaps I was corrupting the stack as a friend had suggested, or that my direct x device was doing something silly I hadn't meant ot tell it to do, I ran teh code from the very first line in debug mode, tracing what happened with every line of code.

The only global variable declared before the WinMain function is a

game* Glitch = new game();

which creates an instance of a game class (which is a class that holds the functionality for my entire application), although the default constructor is empty. The first Line of coding in WinMain is:

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE prevInstance, PSTR cmdLine, int showCmd)
{
	if(Glitch->init(hInstance))

Stepping into the game->init function, the first two lines create an instance of an inifilereader class that I wrote (it's very basic), and then attempt to open an ini file that controls the size of the window/filename/fullscreen mode. I know that the ini file class works, as I have used it in a previous project, and I had directly copied and pasted it without modifying it. However, in this iteration, it was broken. Here's the code for those lines:

iniFileReader i;
i.loadFile("Glitch.ini");

I entered the ini file load file function, which is as follows:

bool iniFileReader::loadFile(char* filename)
{
		FILE* theFile;
		char temp[1];
		int count;
		while(count!= 0) 
			count = 0;
		fopen_s(&theFile, filename, "r");
		if(theFile == NULL)
			return false;	
		while(!feof(theFile))
		{
			fread(temp, 1, 1, theFile);
			count++;
		}//while(!feof(theFile))

		fileText = new char[count];
		rewind(theFile);
		fread(fileText, 1, count, theFile);
		initialised = true;
		return true;
}//iniFileReader::loadFile(char* filename)

As you can see, I put the count = 0 in a while loop, because like in my heightmap::init function, unless I put it in the while loop, it was ending up being junk data, although in this case, it's a large positive number instead of a large negative number.

The interesting thing is that even with the while loop, count ends up being a large file name.

In addition, while running through the while loop, the value of 'filename' (the char* that I pass in) changes. Which makes no sense, as it is not referenced in any way. Additionally, the values it is changing to are not even values stored in the ini file - I do not know where they are comign from, but they seem to be fragments of something, probably code or another file.

Additionally, count++ is actually not incrementing count. Instead, it's giving count random values. The first few digits of count remain the same, but the last three change in values between 200-400.

At this point, I stopped debugging the application. So here's what I know:

1) I have not yet initialised a windows class or direct x, so that's not the problem.

2) the ini reader load file function is essentially the first thing to happen, and it's screwing up pretty badly.

3) I am very confused.

I have been working on this problem for almost four weeks now, and this includes talking with it about several friends who are also good at programming, asking for help from my roommate (who is in the same program), and sitting down with my teacher and trying to figure it out for almost an hour, to no avail.

Any help is greatly appreciated.

EDIT: Formatted code better

[Salem]

> As you can see, I put the count = 0 in a while loop
How many times does it execute?
How is it different from just saying
count = 0;

> WORD *indices = new WORD[cellWidth * cellHeight - 1];
You're WAY off here.
Even assuming a simple loop such as this

	int pos = 0;	
	for(int i = 0; i < cellWidth; i++)
	{
		for(int j = 0; j < cellHeight; j++)
		{
			indices[pos]	= i * vertsPerRow + j;
			pos += 1;
		}//for(int j = 0; j < cellHeight; j++)
	}//for(int i = 0; i < cellWidth; i++)

You would need
WORD *indices = new WORD[cellWidth * cellHeight];

Your loop needs
WORD *indices = new WORD[cellWidth * cellHeight * 6];
entries in it.

Basically, you're hosing way more memory than you allocated, trashing all sorts of other peoples memory, and seeing all sorts of weirness as a result.

> while(!feof(theFile))
Read the FAQ on why using feof() in a control loop is bad.

[therabidwombat]

> As you can see, I put the count = 0 in a while loop
How many times does it execute?
How is it different from just saying
count = 0;

In this execution, it's not operating any differently than count = 0, and the loop is running only once. The reason that I put it in a while loop is that in my heightMap initialisation, many of my variables would not initialise properly unless I put them into while loops that continually put the value into them until the value was right (as in the beggining of the post).

> WORD *indices = new WORD[cellWidth * cellHeight - 1];
You're WAY off here.
Even assuming a simple loop such as this

	int pos = 0;	
	for(int i = 0; i < cellWidth; i++)
	{
		for(int j = 0; j < cellHeight; j++)
		{
			indices[pos]	= i * vertsPerRow + j;
			pos += 1;
		}//for(int j = 0; j < cellHeight; j++)
	}//for(int i = 0; i < cellWidth; i++)

You would need
WORD *indices = new WORD[cellWidth * cellHeight];

Your loop needs
WORD *indices = new WORD[cellWidth * cellHeight * 6];
entries in it.

Basically, you're hosing way more memory than you allocated, trashing all sorts of other peoples memory, and seeing all sorts of weirness as a result.

I'll take that into consideration, but I don't think that's not the problem. For one thing, I never got that far in my new version of the engine before things start erasing and screwing up, so that code doesn't even exist in the second version. Secondly, a lot of things are breaking before they reach that code.

However, I will fix that, and hopefully it will make some difference. Thanks for pointing that out.

> while(!feof(theFile))
Read the FAQ on why using feof() in a control loop is bad.

Will do.

[Salem]

> Secondly, a lot of things are breaking before they reach that code.
I'm assuming that since you've so ably demonstated the ability to trash memory that you've done so in many other places as well.

The fact that it gets past some of them whilst still "working" doesn't make them correct.

Your code is broken, all that remains is to figure out the root cause from the apparent effects.

[swoopy]

>I'll take that into consideration, but I don't think that's not the problem.
If you think about how far out-of-bounds that loop is writing to memory, there's no telling what problems it's causing. You're trying to write to over six times the amount of memory you allocated. That's a very large overshoot.

[therabidwombat]

>I'll take that into consideration, but I don't think that's not the problem.
If you think about how far out-of-bounds that loop is writing to memory, there's no telling what problems it's causing. You're trying to write to over six times the amount of memory you allocated. That's a very large overshoot.

I'm very aware of what a big deal that much extra memory being written is, and I'm not claiming that that wasn't a problem - however, as I stated above, because I don't even have said code in my new version, I dont' believe that it could be the problem.

Your code is broken, all that remains is to figure out the root cause from the apparent effects.

No disputing you on that, I'm still trying to figure out where it's breaking though.

[Salem]

Well, you pretty much have to go through your code one line at a time looking at every single array (to check it's size) and dynamically allocated block of memory.

What you need to look for:
- using a pointer before you allocate it
- using a pointer after you free it
- allocating the wrong amount (you've proved you can do this)
- freeing the same block twice
- freeing something which was never allocated (say an array)

> the ini reader load file function is essentially the first thing to happen, and it's screwing up pretty badly.
Option 1

1. Create a backup of your project.
2. Delete all the code which cannot execute before this function.
3. If you end up with main() and iniFileReader::loadFile(), and it still crashes, then post the WHOLE thing.

Option 2
Spend $$$ on tools like "bounds checker" or "purify" to help you locate the root cause without having to edit what you have.

Please tell me you're not using threads as well.

[vart]

If bounds checker is not available
You can also use some very strict static code checkers (for exsample buffer overruns can be found by them) - like PCLint - it brings a lot more warnings then the regular VC compiler.

At least set warning level at maximum in your compiler and don't just ignore them

[Laserve]

I dont know if its been said already but you're opening the inifile as text and then proceed to read from it binary with fread. Use the standard C++ streams or fgets.

fopen_s(&theFile, filename, "r");
if(theFile == NULL)
  return false;	
  
while(fgets(buf, n theFile))
{
}

and hmm now I look at the rest of the inireader code. it's really crappy :P

[vart]

And of course you don't need to use the long loop reading it one byte at a time for checking the file size
use fsetpos and ftell for example

[Laserve]

I thought ftell doesnt work on windows for text files because \n will be converted to \r\n. So you cant rely on ftell for getting a file size.
I think you have to use stat() or some windows API to get actual file size.
But with the inifile you dont have to know the size, just read in line by line and set the config vars to whatever is read

[therabidwombat]

Alright, after I've fixed these bugs I'll work on my ini file reader and make it a little claner. Thanks for your advice on that.

However, the ini file reader is not the key problem here. I can comment out the ini file reader and just use temporary values in the code [that's what I'm currently doing, although when I do uncomment it, nothing has changed). The reason I posted the ini file problem in here with the rest of it is:

1) The errors I'm encountering in the ini file mimic the problems I've had elsewhere

2) The ini file happens incredibly early in the code, including before windows and direct x are initialised

But the real problem is my heightmap class, and why it's erasing itself. Although I think that if I fix that, the ini file would probably work again...I'm pretty surethat it's the same problem, ESPECIALLY considering that the ini file reader has worked for me in another project.

You can also use some very strict static code checkers (for exsample buffer overruns can be found by them) - like PCLint - it brings a lot more warnings then the regular VC compiler.

At least set warning level at maximum in your compiler and don't just ignore them

I looked at PCLint, but it costs money, I hate to sound like a cheapo but are there any free versions/programs like that at this point?

I set my compiler warning to max, and cleared all of the warning that showed up (except 4 - which are for variables passed in to functions that aren't used. Three were from WinMain and one is from a function where I just haven't used the variable yet, although that function has not yet been called before things are screwing up).

I tried running it after that, with the ini file reader uncommented, and I found that the count variable was behaving differently. Quite simply...C++ didn't recognise it, when I ran my mouse over it, no value popped up, and when I put it in the watch window, it said that 'symbol 'count' cannot be found'. It seems that the variable is being completely ignored now.

Please tell me you're not using threads as well.

XD That would be frightening

3. If you end up with main() and iniFileReader::loadFile(), and it still crashes, then post the WHOLE thing.

That's *essentially what I have right now...below is the code in my main file:

#include "game.h"

//globals
game *Glitch = new game();

bool Render(float timeDelta)	
{
	Glitch->beginRender();		
		//RenderCode Here
	Glitch->endRender();		
	return true;				
}//bool Render(float timeDelta)

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE prevInstance, PSTR cmdLine, int showCmd)
{
	//initialisation stuff
	if(Glitch->init(hInstance))	
	{					
		Glitch->mainLoop(Render);
	}//if(Glitch->init(hInstance))
	else			
	{
		MessageBoxA(NULL,	
			"Error initialising Glitch.",
			"Glitch", 0);			
	}//else
	//cleanup stuff
	delete(Glitch);		
	return 0;		
}

And here's the code in Glitch->init(hInstance):

bool game::init(HINSTANCE hInstance)
{
	iniFileReader i;
	i.loadFile("Glitch.ini");

	bool fullscreen = false;
	char* name = "Glitch";
	DWORD style = WS_VISIBLE;

	if(fullscreen)	
		style |= WS_POPUP | WS_EX_TOPMOST;
	int width = 640; int height = 480;	

	D3DDEVTYPE devicetype = D3DDEVTYPE_HAL;

	WNDCLASSEX wcex;		
	ZeroMemory(&wcex, sizeof(wcex));
	wcex.cbSize	= sizeof(WNDCLASSEX);	
	wcex.style	= CS_HREDRAW | CS_VREDRAW;
	wcex.lpfnWndProc = (WNDPROC) WndProc;
	wcex.hInstance= hInstance;
	wcex.hCursor= LoadCursor(NULL, IDC_ARROW);
	wcex.lpszClassName= name;

	HWND hWnd;
	hWnd = CreateWindow(name, name, style, CW_USEDEFAULT, 0, width, height, NULL,NULL, hInstance, NULL);

	if(!hWnd)
	{
		return false;
	}//if(!hWnd)

	ShowWindow(hWnd, SW_SHOW);
	UpdateWindow(hWnd);	

	HRESULT hr;	
	LPDIRECT3D9 d3d9 = NULL;
	if(NULL == (d3d9= Direct3DCreate9(D3D_SDK_VERSION)))
	{
		return false;	
	}//if(NULL == d3d9)

	D3DCAPS9 caps;
	d3d9->GetDeviceCaps(D3DADAPTER_DEFAULT, devicetype, &caps);

	int vp = 0;
	if(caps.DevCaps & D3DDEVCAPS_HWTRANSFORMANDLIGHT)		
	{
		vp = D3DCREATE_HARDWARE_VERTEXPROCESSING
	}//if(caps.DevCaps & D3DDEVCAPS_HWTRANSFORMANDLIGHT)
	else
	{
		vp = D3DCREATE_SOFTWARE_VERTEXPROCESSING;
	}//else

	D3DPRESENT_PARAMETERS d3dpp;
	ZeroMemory(&d3dpp, sizeof(d3dpp));	
	d3dpp.BackBufferWidth = width;
	d3dpp.BackBufferHeight = height;
	d3dpp.BackBufferCount = 1;
	d3dpp.BackBufferFormat = D3DFMT_A8R8G8B8;
	d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD;
	d3dpp.hDeviceWindow = hWnd;
	d3dpp.Windowed = !fullscreen;
	d3dpp.EnableAutoDepthStencil = true;
	d3dpp.AutoDepthStencilFormat = D3DFMT_D32;
	d3dpp.FullScreen_RefreshRateInHz = D3DPRESENT_RATE_DEFAULT;
	d3dpp.PresentationInterval = D3DPRESENT_INTERVAL_IMMEDIATE;

	hr = d3d9->CreateDevice(D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hWnd, vp, &d3dpp, &device);

	if(hr != D3D_OK)
	{
		d3dpp.AutoDepthStencilFormat = D3DFMT_D16;
		hr = d3d9->CreateDevice(D3DADAPTER_DEFAULT, devicetype, hWnd, vp, &d3dpp, &device);
		if(hr != D3D_OK)
		{
			d3d9->Release();
			return false;	
		}//if(FAILED(hr))
	}//if(FAILED(hr))

	d3d9->Release();
	h = new heightMap();
	h->initHeightmap(device, "coastMountain64.raw", 64, 64, 10, 0.5);

	rendering = false;
	BGColor = D3DCOLOR_XRGB(0, 0, 0);
	D3DXMATRIX proj;
	D3DXMatrixPerspectiveFovLH(&proj, D3DX_PI / 2, (float)width / (float)height, 0, 1000);
	device->SetTransform(D3DTS_PROJECTION, &proj);
	//sets the camera
	D3DXVECTOR3 pos(6, 0, 0);
	D3DXVECTOR3 target(0, 0, 0);
	D3DXVECTOR3 up(0.0f, 1.0f, 0.0f);

	D3DXMATRIX result;
	D3DXMatrixLookAtLH(&result, &pos, &target, &up);
	device->SetTransform(D3DTS_VIEW, &result);

	return true;

}//game::init(HINSTANCE hInstance)

And here's the heightMap::initheightMap function:

bool heightMap::initHeightmap(IDirect3DDevice9 *nDevice, char *fname, int nNumVertsPerRow,   
int nNumVertsPerCol, int nCellSpacing, float nHeightScale)
{
	device = nDevice;
	numVertsPerRow = nNumVertsPerRow;
	numVertsPerCol = nNumVertsPerCol;
	cellSpacing = nCellSpacing;
	heightScale = nHeightScale;
	numCellsPerRow = numVertsPerRow - 1;
	numCellsPerCol = numVertsPerCol - 1;
	width = numCellsPerRow * cellSpacing;
	depth = numCellsPerRow * cellSpacing;
	numVertices = numVertsPerRow * numVertsPerCol;
	numTriangles = numCellsPerRow * numCellsPerCol * 2;
	heightScale = nHeightScale;

	if(!readRawFile(fname))
	{
		return false;
	}//if(!readRawFile(fname))
	return true;
}//bool heightMap::init(IDirect3DDevice9 *nDevice, char *fname, int nNumVertsPerRow, ...

bool heightMap::readRawFile(char *fname)
{
	std::vector<BYTE> in(numVertices);		
	std::ifstream inFile(fname, std::ios_base::binary);	
	if(inFile == NULL) return false;

	inFile.read((char*)&in[0], in.size());
	inFile.close();

//	map.resize(numVertices);

	for(unsigned int i = 0; i < in.size(); i++)
	{
//		map[i] = in[i];
	}
	return true;
}

(NOTE: in the readRawFile function, the commented out variable 'map' is a vector that I commented out thinking that it may be the cause fo the problem, which it wasn't.)

[CornedBee]

> As you can see, I put the count = 0 in a while loop
How many times does it execute?
How is it different from just saying
count = 0;

One of them is undefined behaviour as far as the standard is concerned

[swoopy]

One of them is undefined behaviour as far as the standard is concerned

I see nothing undefined about it. It's just unnecessary.

[Salem]

> std::vector<BYTE> in(numVertices);
> inFile.read((char*)&in[0], in.size());
See, I worry when I see random casting going on inside C++ programs.

Are you even allowed to just take the address of the first element of a vector, and assume that everything is contiguous from that point?
Now maybe BYTE is a special case, but then again maybe it isn't.

unsigned char *in = new unsigned char[numVertices];
inFile.read( in, numVertices );
...
delete [ ] in;

maybe a bit lower level, but it sure doesn't depend on the internal storage layout of an STL container like vector.

> I see nothing undefined about it. It's just unnecessary.
Pedantically, even reading an uninitialised value may cause a trap for anything which isn't a char. The strange while loop to clear a value has to do a read before a write.