Sign in to follow this  
JTR1080

problem code isolated but makes no sense

Recommended Posts

JTR1080    122
I have a complex problem in my tile engine that only occurs when i strictly use a loadMap function. For some reason when i read the tiles name from the text file into a string buffer on the third Node in the linked list it overwrites the second nodes stringHandle (ie bitmap3.bmp) with whatever is being read into it. So when it parses the list of surfaces it returns that second node which is not the correct surface, so every single tile painted is the second node (which is actually the first because the very first node is reserved as NULL anyway.) So it paints the first tile listed in the file for all the tiles. I've isolated the exact line this occurs so my question is why? here is the code:
//load the map of file specified
void map::loadMap(char* tileFile, videoMemory* memoryAlloc)
{  
	//debug buffer for breaking down file into components
	char tileBitmap[256];
	char buffer[128] = {""};
	long tileID;
	bool bWalkable;
	int xOfs, yOfs, zOfs;

	//debug
	printf ("next node string: %s", memoryAlloc->pFirst);

	//dont forget to create an origin (!!!INVALID COORDS BUT VALID RECT!!!)
	SDL_Surface* tileImage = memoryAlloc->querySurface("tile0.bmp");
	this->pOrigin = new CgameTile(-1, -1, tileImage, -1, "tile0.bmp", false);

	ifstream mapStream (tileFile);

	if (! mapStream.is_open())
  { cout << "Error opening file"; return; }


	//get header first!! first two lines are read as HEADER!
	mapStream.getline(buffer, 128);
	mapStream.getline(buffer, 128);

	while(!mapStream.eof())
	{
			//break line down into components
			
			mapStream >> tileID;
			mapStream >> xOfs;
			mapStream >> yOfs;
			//node 2 of the link list handleString changes to 
			//tileBitmap for unknown reason
			mapStream >> &tileBitmap[0];  //on this line
			mapStream >> zOfs;
			mapStream >> bWalkable;

			if(this->maxHeight<zOfs) { this->maxHeight=zOfs; }
			if(this->sizeOfMapX<xOfs) {this->sizeOfMapX=xOfs; }
			if(this->sizeOfMapY<yOfs) {this->sizeOfMapY=yOfs; }

			/*debug*/
			//cout << tileID << "," << xOfs << "," << yOfs << "," << tileBitmap << "," << zOfs << endl;

			tileImage = memoryAlloc->querySurface(&tileBitmap[0]);

			/*read data                        feed it into the constructor */
			CgameTile* newNode = new CgameTile(xOfs, yOfs, tileImage, zOfs, &tileBitmap[0], tileID, bWalkable);
			this->addTail(newNode);
	}
	return;
}

i've experienced this before somewhat, i would guess its some kind of memory going to a part of an array its not supposed to and overiting the handlestring (how i have no idea) but i already investigated tileBitmap by "watching" it and it goes to only the amount allocated and then terminates '\0' so it looks sound. so whats going on? thanks for any help

Share this post


Link to post
Share on other sites
Dave Hunt    4872
One thing I notice is that you are passing the address of tileBitmap to your CGameTile constructor. Unless the constructor makes a copy of the string, then all of your CGameTile instances will be using the same value for tileBitmap.

Share this post


Link to post
Share on other sites
JTR1080    122
isnt that what i want? the value of tileBitmap changes through each iteration until the end of the file.

The problem is when it reads from the file into the buffer (tileBitmap), it overwrites the videoMemory->pHead->pNext->handleString to whatever is being
(CgameSurface)
written into the buffer. So every CgameTile points to the same CgameSurface because the name is changed when it checks to see if it exists.

So why would reading from a file to a buffer (tileBitmap) overwrite somthing as unrelated as that to cause me these problems.

Share this post


Link to post
Share on other sites
Dave Hunt    4872
Quote:
Original post by JTR1080
isnt that what i want?


I don't know, since there is not enough code/information for me to know what you are doing with it.

You're talking about linked lists and 'stringHandle' and there is nothing in the code you posted that is related to that execept a call to addTail(). Perhaps if you posted more code (and some headers) I would be able to tell what was going on.

Share this post


Link to post
Share on other sites
JTR1080    122
loadMap() is part of the 'map' class which interfaces with the 'tile' class and 'VideoMemory' (handles surfaces) class to manage a 'map'

here is the main.cpp code of the tile engine

/*
TileWhirl'd ISOMETRIC TILING ENGINE - Loads tiles from a file or writes to a file
You can interface with the engine through the map source file functions, NOT WITH
TILE.CPP.
*/

/*
VideoMemory System - creates a list of active surfaces and has functions for periodic
garbage collection; if a node contains an empty surface. It is usefull so surfaces
can be queried and reused with little effort.
*/

#include <stdio.h>
#include "SDL.h"
#include "videoMemory.h"
#include "map.h"


SDL_Surface *screen;
SDL_Rect *screenRect;
SDL_Cursor *SDLcursor;

void cleanup(void);
void panUp(void);
void panDown(void);
void panLeft(void);
void panRight(void);
void panUpRight(void);
void panUpLeft(void);
void panDownRight(void);
void panDownLeft(void);
//global var
int startCenX;
int startCenY;

static SDL_Cursor *init_system_cursor(const char *image[]);

#define XOFS 11

videoMemory* vMemory = new videoMemory;
map* ourMap = new map;
bool bRunning = true;

int main(int nArg, char* pszArgs[])
{
//initialize SDL
if(SDL_Init(SDL_INIT_VIDEO|SDL_INIT_AUDIO)<0)
{
printf ("Error initializing %s \n", SDL_GetError());
return -1;
}

//cleanup at exit
atexit(cleanup);

//initialize the screen
SDL_Surface* image = new SDL_Surface;
image = SDL_LoadBMP("tile.bmp");

SDL_WM_SetIcon(image,NULL);
SDL_WM_SetCaption("TileWhirl'd ISOMETRIC TILING ENGINE", "tile.bmp");
screen=SDL_SetVideoMode(1024, 768, 0, SDL_HWSURFACE|SDL_DOUBLEBUF);
if(screen==NULL) { printf ("Could not initialize: %s\n", SDL_GetError()); }

screenRect = new SDL_Rect;
screenRect->x = 0;
screenRect->y = 0;
screenRect->w = screen->w;
screenRect->h = screen->h;
SDL_SetClipRect(screen, screenRect);


//ourMap->randomMap(vMemory, 30, 30);
//ourMap->createMap(vMemory, 30, 30); //MINIMUM SCROLL SIZE 23 23 this is for 1024x768
ourMap->loadMap("created.txt", vMemory); //23 23 is a 0-22x0-22 map
if(!ourMap->pOrigin) { printf ("Error could not load map!"); }

SDL_Event event;

startCenX = 15; //min startX 11
startCenY = 15; //min startY 11

SDLcursor = SDL_GetCursor();

while(bRunning)
{

SDL_FillRect(screen, screenRect, NULL); //loading dif maps
vMemory->killDeadNodes(); //i don't see how i will need this until i start
ourMap->renderMap(startCenX, startCenY); //11 11

while(SDL_PollEvent(&event))
{
switch(event.type)
{
case SDL_KEYDOWN:
switch(event.key.keysym.sym)
{
case SDLK_q:
bRunning=false;
break;
//move down and to the right
case SDLK_KP3:
panDownRight();
break;
//move up and to the left
case SDLK_KP7:
panUpLeft();
break;
//move up and to the right
case SDLK_KP9:
panUpRight();
break;
//move down and to the left
case SDLK_KP1:
panDownLeft();
break;
/////STRAIGHT MOVEMENT
//move up
case SDLK_KP8:
panUp();
break;
//move down
case SDLK_KP2:
panDown();
break;
//move left
case SDLK_KP4:
panLeft();
break;
//move right
case SDLK_KP6:
panRight();
break;
default:
break;
}
break;
case SDL_KEYUP:
switch(event.key.keysym.sym)
{
case SDLK_ESCAPE:
bRunning=false;
break;
default:
break;
}
break;
case SDL_QUIT:
bRunning=false;
break;
}/*switch(eventtype*/

}/*while(pollevent)*/
} /*while(running)*/

ourMap->saveMap("created.txt");

return 0;
}

void cleanup()
{
delete ourMap; //unloads the map in active memory
delete vMemory;
delete screenRect;
SDL_FreeCursor(SDLcursor);
SDL_Quit();
return;
}

////////////////////////////////////////////////////////////////
/// MOVEMENT FUNCTIONS: ORIENTED TO THE VIEW PORT ///////////
////////////////////////////////////////////////////////////////


void panUp()
{
//add X && Y
if((startCenX+XOFS)+1<ourMap->sizeOfMapX && (startCenY+XOFS)+1<ourMap->sizeOfMapY)
{
startCenX++;
startCenY++;
}
return;
}


void panDown()
{
//subtract X && Y
if((startCenY-XOFS)-1>=0 && (startCenX-XOFS)-1>=0)
{
startCenY--;
startCenX--;
}
return;
}

void panLeft()
{
//X- y+
if((startCenX-XOFS)-1>=0 && (startCenY+XOFS)+1<ourMap->sizeOfMapY)
{
startCenX--;
startCenY++;
}
return;
}
void panRight()
{
//X+ y-
if((startCenY-XOFS)-1>=0 && (startCenX+XOFS)+1<ourMap->sizeOfMapX)
{
startCenX++;
startCenY--;
}
return;
}
//now the angled movements
void panUpRight()
{
if((startCenY-XOFS)-1>=0) { startCenY--; }
return;
}
void panUpLeft()
{
if((startCenX-XOFS)-1>=0) { startCenX--; }
return;
}
void panDownRight()
{
if((startCenX+XOFS)+1<ourMap->sizeOfMapX) { startCenX++; }
return;
}
void panDownLeft()
{
if((startCenY+XOFS)+1<ourMap->sizeOfMapY) { startCenY++; }
return;
}

I will include the header files for each other class in the project:
CgameObj.h

// gameObj.h: interface for the CgameObj class.
// Abstract base class, Doesn't require an implementation file
//////////////////////////////////////////////////////////////////////

//technically it isnt an abstract class because its not virtual though right?

#if !defined(AFX_GAMEOBJ_H__2545A58E_50BC_4C99_9E1A_1CEDD79D2AF0__INCLUDED_)
#define AFX_GAMEOBJ_H__2545A58E_50BC_4C99_9E1A_1CEDD79D2AF0__INCLUDED_

#if _MSC_VER >= 1000
#pragma once
#endif // _MSC_VER >= 1000

#include "SDL.h"

class CgameObj
{
protected:
unsigned long objID;
unsigned int xOfs; //offset from 0,0 on the X axis (if it was 5,4 then 5)
unsigned int yOfs; //offset on the Y axis
SDL_Surface *objSurface; //pointer to the surface of the currentTile
SDL_Rect *objRect;
int height; //for depth and overlays
//void addSurface(char* surfaceName);
};

#endif // !defined(AFX_GAMEOBJ_H__2545A58E_50BC_4C99_9E1A_1CEDD79D2AF0__INCLUDED_)

CgameTile.h

// gameTile.h: interface for the CgameTile class.
//
//////////////////////////////////////////////////////////////////////

#if !defined(AFX_GAMETILE_H__729E4C47_9AF1_48F2_ACCF_655C56BFDA2C__INCLUDED_)
#define AFX_GAMETILE_H__729E4C47_9AF1_48F2_ACCF_655C56BFDA2C__INCLUDED_

#if _MSC_VER >= 1000
#pragma once
#endif // _MSC_VER >= 1000

#include "gameObj.h"

class CgameTile : protected CgameObj
{
public:
CgameTile();
//"Creating a tile"
CgameTile(int xPos, int yPos, SDL_Surface *tileSurface, int objHeight, char* tileBitmap, bool bWalk);
//"loading a tile"
CgameTile(int xPos, int yPos, SDL_Surface *tileSurface, int objHeight, char* tileBitmap, long tileID, bool bWalk);
virtual ~CgameTile();

CgameTile *NextNode;
CgameTile *PrevNode;
bool walkable;
private:
char tileBitmap[256];
friend class map;
};

#endif // !defined(AFX_GAMETILE_H__729E4C47_9AF1_48F2_ACCF_655C56BFDA2C__INCLUDED_)

CSurfaces.h

// surfaces.h: interface for the Csurfaces class.
//
//////////////////////////////////////////////////////////////////////

#if !defined(AFX_SURFACES_H__BFA82B16_E07C_4190_B5A9_F725997A3083__INCLUDED_)
#define AFX_SURFACES_H__BFA82B16_E07C_4190_B5A9_F725997A3083__INCLUDED_

#if _MSC_VER >= 1000
#pragma once
#endif // _MSC_VER >= 1000

#include "SDL.h"
#include <string>

using namespace std;

class Csurfaces
{
private:
SDL_Surface* pSurface; //reference surface
char* handleSurface; //handle to that surface
Csurfaces* pNext; //next node
Csurfaces* pPrev; //prev node
public:
Csurfaces();
Csurfaces(char* handleName);
virtual ~Csurfaces();
friend class videoMemory;
};

#endif // !defined(AFX_SURFACES_H__BFA82B16_E07C_4190_B5A9_F725997A3083__INCLUDED_)

map.h

// map.h: interface for the map class.
//
//////////////////////////////////////////////////////////////////////

#if !defined(AFX_MAP_H__80BB8323_9F61_407F_B21E_62E9B74069F0__INCLUDED_)
#define AFX_MAP_H__80BB8323_9F61_407F_B21E_62E9B74069F0__INCLUDED_

#if _MSC_VER >= 1000
#pragma once
#endif // _MSC_VER >= 1000

#include "gameTile.h"
#include "videoMemory.h"
#include <fstream>
#include <iostream>
#include <string>
//these are only used for random map gen
#include <cstdlib>
#include <ctime>

using namespace std;

class map
{
public:
map();
virtual ~map();

CgameTile* pOrigin;
int sizeOfMapX;
int sizeOfMapY;
int maxHeight;

//member functions
void loadMap(char* fileName, videoMemory* memoryAlloc);
void saveMap(char* fileName);
void createMap(videoMemory* memoryAlloc, int xBounds, int yBounds);
void randomMap(videoMemory* memoryAlloc, int xBounds, int yBounds);
void renderMap(int centerCoordX, int centerCoordY);
void unloadMap();

private:
void addTail(CgameTile* tail);
CgameTile* getTile(int xCoord, int yCoord, int zCoord);
};

#endif // !defined(AFX_MAP_H__80BB8323_9F61_407F_B21E_62E9B74069F0__INCLUDED_)

videoMemory.h

// videoMemory.h: interface for the videoMemory class.
//
//////////////////////////////////////////////////////////////////////

#if !defined(AFX_VIDEOMEMORY_H__3F889092_C3D6_45BA_8FDE_1EAA97A5581E__INCLUDED_)
#define AFX_VIDEOMEMORY_H__3F889092_C3D6_45BA_8FDE_1EAA97A5581E__INCLUDED_

#if _MSC_VER >= 1000
#pragma once
#endif // _MSC_VER >= 1000

#include "surfaces.h"

class videoMemory
{
public:
videoMemory();
virtual ~videoMemory();

Csurfaces* pFirst;

//member functions
void killDeadNodes(void);
SDL_Surface* querySurface(char* handleString);

private:
void addSurface(Csurfaces* newSurface);
void unloadVideoMemory(void);
};

#endif // !defined(AFX_VIDEOMEMORY_H__3F889092_C3D6_45BA_8FDE_1EAA97A5581E__INCLUDED_)

Share this post


Link to post
Share on other sites
Dave Hunt    4872
I'm sure the problem isn't in main.cpp. It must be somewhere in you class implementations. Also, you referred to 'handleString', but there is no member variable named 'handleString'. There is a 'handleSurface'. Perhaps that is what you meant. At any rate, there is nothing in the code you posted that would cause this.

P.S. Instead of using 'code' tags, you should use 'source' tags for large pieces of source code.

Share this post


Link to post
Share on other sites
Dave Hunt    4872
Quote:
Original post by Anonymous Poster
You forgot the mapStream.Close().


The mapStream will be closed by the destructor when the function returns.

Share this post


Link to post
Share on other sites
Zahlman    1682
As mentioned, there is only a single tileBitmap array, and it's local to the function. You may iteratively put different stuff in it, but if you are doing something silly like making an assignment "myTitle = inputTitle" in the CGameTile constructor, then all of your tiles point at the same character buffer - which isn't even valid after the function ends.

C++ provides tools for dealing with this sort of thing in a way that's easy and *works*. In particular, it provides std::string (which also avoids problems with needing a particular buffer length).


void map::loadMap(const char* tileFile, videoMemory* memoryAlloc) {
std::string tileBitmap; // I assume this is the name of a bitmap file
// anyway, whether you read into a char[] or a std::string with >>, it will
// only read one "word"
std::string ignored; // "buffer" line

long tileID;
bool bWalkable;
int xOfs, yOfs, zOfs;

//debug
cerr << "next node string: " << memoryAlloc->pFirst;

//dont forget to create an origin (!!!INVALID COORDS BUT VALID RECT!!!)
SDL_Surface* tileImage = memoryAlloc->querySurface("tile0.bmp");
pOrigin = new CgameTile(-1, -1, tileImage, -1, "tile0.bmp", false);

ifstream mapStream (tileFile);
if (!mapStream.is_open()) { cerr << "Error opening file"; return; }

//get header first!! first two lines are read as HEADER!
getline(mapStream, ignored);
getline(mapStream, ignored);

// While able to read in a whole line,
while(mapStream >> tileID >> xOfs >> yOfs >> tileBitmap >> zOfs >> bWalkable){
// update maxima
if(maxHeight<zOfs) { maxHeight=zOfs; }
if(sizeOfMapX<xOfs) { sizeOfMapX=xOfs; }
if(sizeOfMapY<yOfs) { sizeOfMapY=yOfs; }

// debug
// cerr << tileID << "," << xOfs << "," << yOfs << "," << tileBitmap
// << "," << zOfs << endl;

// You can get a const char* back from a std::string like this:
tileImage = memoryAlloc->querySurface(tileBitmap.c_str());

// Data is read; feed to ctor
// I will leave it to you to fix the ctor to accept the std::string.
// That way, it will actually copy as needed.
CgameTile* newNode = new CgameTile(xOfs, yOfs, tileImage, zOfs, tileBitmap, tileID, bWalkable);
addTail(newNode);
}
}



By the way:

- The initialization of a character buffer to {""} doesn't do what you think it does - you wanted either just "" or perhaps {0}. With the code as given, the first character is initialized to """ interpreted as a character" - where "" is a string literal, i.e. a pointer to somewhere in memory that contains an empty string. Then you get that value cast into a one-byte value and stored at the beginning of your buffer. As it happens, it doesn't matter because initialization wasn't required, but - ugh.

- "return" at the end of a void function is entirely unnecessary. Omit it.

- Why doesn't zOfs come next to xOfs and YOfs in the data (or constructor parameters)?

- You seem to be writing your own linked list; consider std::list instead. (Rr for that matter, any other standard library container - linked lists are usually taught as the conceptually-easiest resizing container to implement yourself, but are often not the best choice.)

Share this post


Link to post
Share on other sites
JTR1080    122
Quote:
Original post by Zahlman
As mentioned, there is only a single tileBitmap array, and it's local to the function. You may iteratively put different stuff in it, but if you are doing something silly like making an assignment "myTitle = inputTitle" in the CGameTile constructor, then all of your tiles point at the same character buffer - which isn't even valid after the function ends.

C++ provides tools for dealing with this sort of thing in a way that's easy and *works*. In particular, it provides std::string (which also avoids problems with needing a particular buffer length).


I think that is my problem, the constructor for the surface with the overloaded
value of char* handleName, sets that char* to whatever the address in memory
handleName is, which is probably tileBitmap array so when it gets changed to
the new variable it changes the handleSurface. I'm changing the members of Csurfaces to have char intstead of char* to hold the data, the only problem is converting between the two so i have to write some annoying code like

for(int i = 0; i<len(string; i++)
{
copy char[x] to memberChar[x];
}




hopefully it works

edit: problem solved! :)

Share this post


Link to post
Share on other sites
Lacutis    301
Quote:
Original post by JTR1080
Quote:
Original post by Zahlman
C++ provides tools for dealing with this sort of thing in a way that's easy and *works*. In particular, it provides std::string (which also avoids problems with needing a particular buffer length).


I think that is my problem, the constructor for the surface with the overloaded
value of char* handleName, sets that char* to whatever the address in memory
handleName is, which is probably tileBitmap array so when it gets changed to
the new variable it changes the handleSurface. I'm changing the members of Csurfaces to have char intstead of char* to hold the data, the only problem is converting between the two so i have to write some annoying code like
*** Source Snippet Removed ***
hopefully it works

edit: problem solved! :)


No, the problem as was mentioned by Zahlman is that you are using char's and char *'s when you should be using std::string. There really isn't many reasons now adays when you actually want to use a char *. It really is in your best interest to learn how to use std::string. It's way easier to handle and can be converted to a char * if needed to by some archaic api function.

You are basically doing a lot of work to work around a problem that already has a valid answer. Please consider reading up on std::string.

Share this post


Link to post
Share on other sites
Dave Hunt    4872
Quote:
Original post by Lacutis
No, the problem as was mentioned by Zahlman is that you are using char's and char *'s when you should be using std::string.


No, the problem was exactly as stated - assigning the address of the source to the destination, rather than making a copy. The "best" solution may be to use std::string instead, but that isn't the "problem". It is perfectly possible to get his code working without std::string. Less clean, but not impossible.

Share this post


Link to post
Share on other sites
CyberFox    198
char* something = something_else;

This will make something a pointer to something_else. You want to copy the data instead. In windows the following code will work like a charm:

strcpy( something, something_else );

Alternatively you could write a function that loops through both strings and copies the data over, this however requires assembly language to be done at a speed comparable to strcpy.

** edit **
Don't let using c style strings scare you, click here instead. [cool]

Share this post


Link to post
Share on other sites
Zahlman    1682
Quote:
Original post by Dave Hunt
Quote:
Original post by Lacutis
No, the problem as was mentioned by Zahlman is that you are using char's and char *'s when you should be using std::string.


No, the problem was exactly as stated - assigning the address of the source to the destination, rather than making a copy. The "best" solution may be to use std::string instead, but that isn't the "problem". It is perfectly possible to get his code working without std::string. Less clean, but not impossible.


I've been doing maintenance work recently... please don't tell me that "un-clean" code isn't a "problem". :)

Share this post


Link to post
Share on other sites
Dave Hunt    4872
Quote:
Original post by Zahlman
I've been doing maintenance work recently... please don't tell me that "un-clean" code isn't a "problem". :)


I wouldn't dream of it. I've dealt with other people's (and my own) unclean code for 25 years.

Now that the OP's code is working, perhaps he has learned something about pointers. Hopefully he will take a look at std::string for future use. But, he needed a solution to an immediate problem, not a lecture on the virtues of the standard C++ library.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this