Pretty advanced pointer problem

Started by
27 comments, last by krippy2k8 11 years, 8 months ago
Hello again Gamedev forum!

I'm back with a mind twister. I've sat with this for a few hours and haven't moved an inch forward. I am beaten.

The problem is with a pointer. I've "watched" it while debugging to follow it. And basically this is how it goes.

Object created, pointer is correct -> sends it around abit until finally arrived in Class "Content" -> pointer is correct -> Assign new pointer the same value -> Still correct -> Adjust some values in the object with help from pointer -> Goes back to gameLoop -> back into Content class -> update function running -> Program crashes

The reason why it crashes is because it tries to update the same values it changed earlier with the pointer stored in the class. The pointer has now changed from being a pointer to a Class to being a pointer to the Classes Interface which no real code in it.

And the annoying thing is that I've got 3 different classes that uses the same interface and the error only occurs with this newly created class, ergo I've must have screwed something up. But i can for the love of god not locate it.

I'm going to post the code to these steps now as well as upload the project in case someone wants to try to debug it on his/her own.

Object created, with the createGold function:

int gold = 20*level*(diff/2) + (rand() % 20*level*2 - 10*level);
if(gold > 0)
{
lootContVector.at(vectorsize-1)->moveToBag(createGold(x, y, gold));
}

Item* itemMngr::createGold(int x, int y, int amount)
{
Item* generateditem = NULL;
generateditem = new Currency(x, y, amount);
if(generateditem != NULL)
{
itemVector.push_back(generateditem);
}
return generateditem;
}


Sends it forward to a newly created lootcontainer's function

bool LootContainer::moveToBag(Item* itemAdd)
{
bool itemMoved = false;
for(vector<Container*>::iterator it = containers.begin(); it != containers.end(); it++)
{
if(!itemMoved)
{
if(!(*it)->getFull())
{
(*it)->addItem(itemAdd);
itemMoved = true;
}
}
}
return itemMoved;
}


Adds it to the content and changes some values

void Container::addItem(Item* itemAdd)
{
if(content != NULL)
{
removeItem();
}
if(itemAdd != NULL)
{
content = itemAdd;
content->setInCont(true);
content->setX(contBox.x);
content->setY(contBox.y);
full = true;
}
}


Goes back to the gameloop which sends an update command that eventually reaches the Container class within lootcontainer

void Container::update()
{
if(content == NULL)
{
full = false;
}
else
{
content->setX(contBox.x);
content->setY(contBox.y);
}
}


This code is where it crashes: "[color=#ff0000]content->setX(contBox.x);". If i comment that out it will instead crash on the render function. The reason is that the pointer is no longer (Currency*) but (class Item*) in the "watches window".

Another weird thing it that is does work sometimes, if you are quick enough to spawn the loot... but only once.

If you want to download this yourself you should know that it is my first project and probably has a lot of sloppy code in it. To make the error occur you must spawn a Chest by pressing "R" and open it up.

Greatly appreciates help smile.png

EDIT: The attached file does not come with any DLL's... i am running all the SDL DLL's and by adding these files to the project folder you should be able to run it: https://www.dropbox.com/s/p8jz7na8yodojwc/0.0.0080.rar
Advertisement
I do not have SDL and I'm using VS, gcc is a bit more ahead with C++11 features than VS, so I cannot compile your code, maybe you could also attach compiled debug build? VS should be able to attach and debug.

Other than that, does Container::content pointer change, or it's still the same? You don't seem to mention that (or I missed it).
Yeah, sure mate.

Here it is: https://www.dropbox.com/s/73vwpo8rrofmb6n/Debug.rar
The C++ keyword 'break' lets you break out of the most recent loop, so this:

for(vector<Container*>::iterator it = containers.begin(); it != containers.end(); it++)
{
if(!itemMoved)
{
if(!(*it)->getFull())
{
(*it)->addItem(itemAdd);
itemMoved = true;
}
}
}

...can be converted to:
for(vector<Container*>::iterator it = containers.begin(); it != containers.end(); it++)
{
if(!(*it)->getFull())
{
(*it)->addItem(itemAdd);
break;
}
}


That's unrelated to your problem, though.
I expect the cause of your crash is here: [s](you have "if(content != NULL)" twice in a row)[/s] [color=#ff0000](oops, misread the code statements)
void Container::addItem(Item* itemAdd)
{
//If 'content' (whatever that is!) is not NULL.
if(content != NULL)
{
removeItem(); //Maybe you invalidate the pointer here by mistake?
}
//You repeat the same condition statement... Did you mean "is null?". You probably want an 'else' here.
if(itemAdd != NULL)
{
content = itemAdd;
content->setInCont(true);
content->setX(contBox.x); //What is 'contBox'?
content->setY(contBox.y);
full = true;
}
}
I'm pretty sure with any modern day compiler, your able to stop the execution of code at your desired lines. So start from the top of the stack,

I have a feeling your are slicing, you could try declare your setX and setY as virtual (in the inheritted classes) - or it could be possible that you've casted an object that doesn't even have the setX/setY

As you are calling a Pointer to Item, if you wish to have Polymorphic functionality you need to declare virtual on the desired functions eg

in weapon.cpp

void Weapon::setX(int x)
{
box.x = x;
}


is now


virtual void Weapon::setX(int x)
{
box.x = x;
}


I'm not sure what object Item* is supposed to represent, I've gone through your code briefly but I have not compiled it
Well without debug symbols I can't really debug, also disassembly looks way too clean to be debug build. It crashes on nullptr error, looks like virtual function call. Do you ever use memcpy() or something similar, that could corrupt itemAdd somewhere before Container::Update()?

Edit:
After running several times I noticed it doesn't crash that often. Somehow I feel it might be heap corruption.
[s]FYI, I'll probably add more to this post in a little bit, so expect a few edits over the next hourish...[/s] Whoa, didn't realize it was such a big code dump and it was a Code::Blocks project. Nevermind, I'm not gonna wade through it.



Item* itemMngr::createGold(int x, int y, int amount)
{
Item* generateditem = NULL;
generateditem = new Currency(x, y, amount);
if(generateditem != NULL)
{
itemVector.push_back(generateditem);
}
return generateditem;
}


Don't do this. Not like that. [font=courier new,courier,monospace]new[/font] will not return [font=courier new,courier,monospace]NULL[/font]. Ever*. It will throw an exception if it fails. Specifically, it will throw a [font=courier new,courier,monospace]std::bad_alloc[/font] exception. You should [s]probably[/s]definitely be using smart pointers too.
[size=2]*At least it won't ever return [font=courier new,courier,monospace]NULL[/font] when called like that. You have to explicitly use [font=courier new,courier,monospace]nothrow[/font] if you want it to return [font=courier new,courier,monospace]NULL[/font] on an allocation failure instead of throw an exception.
[size=2][ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

The C++ keyword 'break' lets you break out of the most recent loop, so this:

for(vector<Container*>::iterator it = containers.begin(); it != containers.end(); it++)
{
if(!itemMoved)
{
if(!(*it)->getFull())
{
(*it)->addItem(itemAdd);
itemMoved = true;
}
}
}

...can be converted to:
for(vector<Container*>::iterator it = containers.begin(); it != containers.end(); it++)
{
if(!(*it)->getFull())
{
(*it)->addItem(itemAdd);
break;
}
}


That's unrelated to your problem, though.


Thank, will use that in the future! But the reasons i use the bool there is because other code need to check if that code succeeds or not ;)


I expect the cause of your crash is here: [s](you have "if(content != NULL)" twice in a row)[/s] [color=#ff0000](oops, misread the code statements)
void Container::addItem(Item* itemAdd)
{
//If 'content' (whatever that is!) is not NULL.
if(content != NULL)
{
removeItem(); //Maybe you invalidate the pointer here by mistake?
}
//You repeat the same condition statement... Did you mean "is null?". You probably want an 'else' here.
if(itemAdd != NULL)
{
content = itemAdd;
content->setInCont(true);
content->setX(contBox.x); //What is 'contBox'?
content->setY(contBox.y);
full = true;
}
}



Content is declared as "Item* content" and stores the pointer to the object which is locked in container (think of a bag slot in WOW or any other RPG).

removeitem sets the content variable to NULL, so i'm sure that not the problem. Actually i dont think the problem is in the Container class since both the class Weapon and Armor which got Item as an interface works fine.

contBox is the location and size of the container.


I'm pretty sure with any modern day compiler, your able to stop the execution of code at your desired lines. So start from the top of the stack,

I have a feeling your are slicing, you could try declare your setX and setY as virtual (in the inheritted classes) - or it could be possible that you've casted an object that doesn't even have the setX/setY

As you are calling a Pointer to Item, if you wish to have Polymorphic functionality you need to declare virtual on the desired functions eg

in weapon.cpp

void Weapon::setX(int x)
{
box.x = x;
}


is now


virtual void Weapon::setX(int x)
{
box.x = x;
}


I'm not sure what object Item* is supposed to represent, I've gone through your code briefly but I have not compiled it


I actually doesn't really know how to debug roperly. I've learned myself to use Watches and Call-stack aswell as breakpoints. And as i said in my first post "This code is where it crashes: "content->setX(contBox.x);". If i comment that out it will instead crash on the render function. The reason is that the pointer is no longer (Currency*) but (class Item*) in the "watches window".".

Whilst googeling around on the problem i was having i say some post about slicing. I don't really know what it is thought. Might have to read into that.

Why would i make them virtual in the inheritted classes? What would that change, i only thought you made them virtual in interfaces.

Item* is an interface all the items will be using in the game. At the moment there are only "Weapon", "Armor" and the broken "Currency". this is my first project ever so i'm pretty sure i'll have to rewrite my code from the start at some point, even now i see how much worse i was at coding when i look at the little older code in my project.


Well without debug symbols I can't really debug, also disassembly looks way too clean to be debug build. It crashes on nullptr error, looks like virtual function call. Do you ever use memcpy() or something similar, that could corrupt itemAdd somewhere before Container::Update()?

Edit:
After running several times I noticed it doesn't crash that often. Somehow I feel it might be heap corruption.


I compiled it as debug, what did you mean i should do?

To your Edit:
Yeah, it doesn't but when you debug in CodeBlocks it crashes every time, but like you said, when you just run the code it doesn't crash all the time.

----

I think the problematic code lies within itemmngr.cpp in the randLoot func and createGold func. Or maybe in the Currency class itself. Because that is the only place i treat Currency any different from Weapon or Armor (also part of Item) and they work fine.

FYI, I'll probably add more to this post in a little bit, so expect a few edits over the next hourish...

[quote name='Tallkotten' timestamp='1344797036' post='4968788']

Item* itemMngr::createGold(int x, int y, int amount)
{
Item* generateditem = NULL;
generateditem = new Currency(x, y, amount);
if(generateditem != NULL)
{
itemVector.push_back(generateditem);
}
return generateditem;
}


Don't do this. Not like that. [font=courier new,courier,monospace]new[/font] will not return [font=courier new,courier,monospace]NULL[/font]. Ever*. It will throw an exception if it fails. Specifically, it will throw a [font=courier new,courier,monospace]std::bad_alloc[/font] exception.
[size=2]*At least it won't ever return [font=courier new,courier,monospace]NULL[/font] when called like that. You have to explicitly use [font=courier new,courier,monospace]nothrow[/font] if you want it to return [font=courier new,courier,monospace]NULL[/font] on an allocation failure instead of throw an exception.
[/quote]

Yeah, result of tiredness and a code-paste of the code above thanks thought, will fix it!

Also, thank you very much for putting for much time into this. Really kind of you!
Largely unrelated to your question but from the look of it in the event that moveToBag returns false then the Item pointer is just discarded and the object is not deleted. Really you should prefer to use smart pointers for this kind of thing, either std::shared_ptr (if your compiler supports it) or boost::shared_ptr are appropriate here. Chances are that better use of RAII containers would also help with the problem you're having now.

Afraid I've no other insight into why your code is crashing. When it does crash does the content pointer still have the same value (i.e. pointing to the same address) or has it been changed to something else or been nullified? If it is not the same value then clearly the problem is that the content pointer is being changed. If the the pointer is unmodified then the object has probably been prematurely deleted or corrupted somehow. I haven't looked at the full code but the problem might also be with accessing the x member of contBox, the full source code or crash details might trivially disprove that theory though :-)

This topic is closed to new replies.

Advertisement