Game crashes for no obvious reason, help please

Started by
10 comments, last by Paradigm Shifter 11 years, 3 months ago
I do one thing it runs fine. I try it again, it crashes. what's happening?

I am trying to use for the first time a finite state machine in my game. The whole reason of making the game is to use a FSM. So, I have two states, STATE_MAIN_MENU and STATE_EXIT.

I have my abstract class, two derived classes for the two states and all. I have already done a similar console application and everything works fine, but here when I am getting from main menu to exit the game crashes, but not all the times. It runs fine for 2-3 times (deletes currentState instance of State_Main_Menu and then currentState = new State_Exit()). The thing is that I put a cout at the constructor of State_Exit and it does construct the class. But then when a function of class is about to run it crushes.

I can't explain it any better, I am attaching the code if anyone could help me I'd appreciate it, because I am getting crazy here!

Thanks in advance!
Advertisement

The way to solve this isn't to ask others to go through all of your code, but to run your program in a debugger. If that doesn't show you exactly what the problem is, it will at least get you closer to finding it. At that point, if you can't figure it out for yourself, you'll have a snippet of code to post and more information with which to ask for help.

You have to remember that the other members here aren't sitting around waiting to debug other people's programs. They are working on their own games, doing their jobs, studying, spending time with their families and so on. Most of them will have neither the time nor the inclination to debug your app for you. For example, I'm typing this from the Point of Sale system at my hot dog shop and have no way to debug it for you, but even if I were at home I'd be unlikely to do it because I have several projects of my own waiting for me. Sure, one or two (or a few) people might be willing to take time out to do it. But you'll get more eyes on your code and increase the likelihood of getting help if you do some of the grunt work first.

I hope this will help:

I am not familiar with the debugger, I think that the State instance doesn't have a value.
For some reason this doesn't work well

void CStateManager::change_state(GameStates *cur_state, int next_state){

if (next_state != STATE_NULL)
delete cur_state;

switch (next_state) {
case STATE_MAIN_MENU: cur_state = new State_Main_Menu(); break;
case STATE_EXIT: cur_state = new State_Exit(); break;
}
}

and it doesn't execute the cur_state = new State_Exit();

BUT, I have a cout at the constructor and I do get the output so the object is constructed.

You have a scope problem. The value of your pointer, cur_state, being an argument, only exists within this function, and will be forgotten once you return from it (while any reference it was pointing to, will not). So, the delete instruction will destroy the reference the pointer is pointing to, and the new instruction will correctly create a new instance and make cur_state point to it.

However, as soon as you return from the function, the value of the cur_state pointer (which is, the address of the instance you just allocated) will be lost, and the caller will be left with the old value, which was the address of the instance you just deleted a few lines earlier. This is, in essence, because you passed the pointer by value and not by reference. The solution is to pass the pointer by reference, or pass a pointer to the pointer, or return the new pointer instead.

Basically, the thing to understand is that you did not pass cur_state to your change_state function, but only its value. Whatever pointer variable you passed as an argument to this function, will be left unchanged, a copy of it will be made, and discarded after the function returns. The unfortunate result being, that the pointer to "new State_Exit()" never made it beyond the "return" statement. As soon as the function returned, the copy holding the new, valid pointer was discarded, and you kept on going with the old value, which now points to a deleted instance.

The new State_Exit() object is there, well and alive. You just don't know where it is, and have no reference to it anywhere in your code and no way to access it beyond the function where it was created, because you lost your pointer to it after said function returned.

Try making cur_state a reference to a pointer instead, that way you will be affecting the real cur_state variable and not just a copy, and your code should work. You can also use a pointer to a pointer (so that you can now affect the inner pointer outside the function too, as well as the object, which is pointed to by the pointed pointer), but this is more C-ish and more error-prone.

“If I understand the standard right it is legal and safe to do this but the resulting value could be anything.”

You have a scope problem. The value of your pointer, cur_state, being an argument, only exists within this function, and will be forgotten once you return from it (while any reference it was pointing to, will not). So, the delete instruction will destroy the reference the pointer is pointing to, and the new instruction will correctly create a new instance and make cur_state point to it.

However, as soon as you return from the function, the value of the cur_state pointer (which is, the address of the instance you just allocated) will be lost, and the caller will be left with the old value, which was the address of the instance you just deleted a few lines earlier. This is, in essence, because you passed the pointer by value and not by reference. The solution is to pass the pointer by reference, or pass a pointer to the pointer, or return the new pointer instead.

Basically, the thing to understand is that you did not pass cur_state to your change_state function, but only its value. Whatever pointer variable you passed as an argument to this function, will be left unchanged, a copy of it will be made, and discarded after the function returns. The unfortunate result being, that the pointer to "new State_Exit()" never made it beyond the "return" statement. As soon as the function returned, the copy holding the new, valid pointer was discarded, and you kept on going with the old value, which now points to a deleted instance.

The new State_Exit() object is there, well and alive. You just don't know where it is, and have no reference to it anywhere in your code and no way to access it beyond the function where it was created, because you lost your pointer to it after said function returned.

Try making cur_state a reference to a pointer instead, that way you will be affecting the real cur_state variable and not just a copy, and your code should work. You can also use a pointer to a pointer (so that you can now affect the inner pointer outside the function too, as well as the object, which is pointed to by the pointed pointer), but this is more C-ish and more error-prone.

I understand the problem now, thank you.
The call to the function was this:

CStateManager::change_state(this, nextState);

How can I modify it so that "this" is passed by reference? (If it's possible at all)
I understand the problem now, thank you.
The call to the function was this:

CStateManager::change_state(this, nextState);

How can I modify it so that "this" is passed by reference? (If it's possible at all)

Ah, I see. No, it is illegal to modify "this", at least in C++. This means your design has failed, and you need to rethink it. Why do you need different classes for State_Main_Menu, State_Exit, etc...? Is an enum flag not enough? What are you trying to do, exactly? It looks like you are trying to implement a state machine for your GUI, but this is clearly not the right approach here, as the language is very clearly telling you "don't do this".

“If I understand the standard right it is legal and safe to do this but the resulting value could be anything.”



I understand the problem now, thank you.
The call to the function was this:

CStateManager::change_state(this, nextState);

How can I modify it so that "this" is passed by reference? (If it's possible at all)



Ah, I see. No, it is illegal to modify "this", at least in C++. This means your design has failed, and you need to rethink it. Why do you need different classes for State_Main_Menu, State_Exit, etc...? Is an enum flag not enough? What are you trying to do, exactly? It looks like you are trying to implement a state machine for your GUI, but this is clearly not the right approach here, as the language is very clearly telling you "don't do this".




It's my first try to use a finite state machine in my game (the point of the whole game was to learn how to write a FSM), to navigate through the menus. Apparently I need to find some other way to do this. But as I say to my original post I did almost an exact console application that worked this way.

Here is the code

void CStateManager::change_state(GameStates *cur_state, int next_state){

	if (next_state != STATE_NULL)
		delete cur_state;

	switch (next_state){
		case STATE_MAIN_MENU: cur_state = new State_Main_Menu(); break;
		case STATE_PLAYING: cur_state = new State_Playing(); break;
		case STATE_EXIT: cur_state = new State_Exit(); break;
	}
}

State_Main_Menu::State_Main_Menu(){
	stateID = STATE_MAIN_MENU;
	nextState = STATE_NULL;
}

void State_Main_Menu::Event(){
	cin >> nextState;
}

void State_Main_Menu::Update(){
	CStateManager::change_state(this, nextState);
}

void State_Main_Menu::Render(){
	cout << "MAIN MENU" << endl;
	cout << "Press '3' to play"  << endl;
	cout << "Press '0' to continue in Main Menu"  << endl;
//	system("cls");
}

int main(int argc, char* argv[]){
	
	GameStates *myState = new State_Main_Menu();

	while (myState->Get_nextState() != STATE_EXIT){
		
		myState->Event();
		myState->Update();
		myState->Render();
	}

	return 0;
}
and it worked as intended, never get an exception by that code. All I was doing was somewhat copying that and adding the SDL stuff!

I'm sure I posted this a lot, but: just because something "seems" to be working just fine, doesn't mean it's actually right. C++ is a language where half the standard seems to say "this is undefined behavior, and this, and this, and this". It is absolutely the last language you should be using for trial and error coding, because stuff will work just fine when you "try something" and will blow up in your face two days later. Why? Because somewhere something completed unrelated in your code will affect the memory layout or eventually overwrite the objects memory you have so happily been accessing and executing after you already deleted it.

The language itself does little to no error checking for you, the compiler will mostly point out syntax errors and many things won't trigger a warning. There is no hand holding and you are expected to know what you're doing. That's also why most people on this forum will tell beginners to stay away from it and start with another language. Not because it is so hard to make something compile. Not because the syntax is difficult or the standard library is so complex. Because you can write horribly bad and bug ridden code and it will still let you believe that everything is working perfectly fine... until it doesn't.

So the usual reply to 99% of all "but it was working before" is "yes, because you have been incredibly lucky".

Reasons why it worked before? Maybe you never called it with a different state. Maybe it kept using the deleted state and you never noticed. Maybe you were incredibly lucky that your new state was placed at exactly the location of your previous state. There's lots of reasons why something happened to work in C++ when it never should have.

f@dzhttp://festini.device-zero.de

[quote name='Trienco' timestamp='1357106333' post='5016582']

Reasons why it worked before? Maybe you never called it with a different state. Maybe it kept using the deleted state and you never noticed. Maybe you were incredibly lucky that your new state was placed at exactly the location of your previous state. There's lots of reasons why something happened to work in C++ when it never should have.

[/quote]

My guess is because this is was simple console application, when he deleted and recreated "this", it ended up being created at the same address it was before, so it really just happened to work by pure chance, since the old and new pointers were the same (so the old value was still valid). Now in his game with SDL thrown in the mix, the heap is more disorganized and this doesn't happen... and boom.

I could be completely wrong, of course, but something in this code just doesn't seem right. It's not supposed to work.

“If I understand the standard right it is legal and safe to do this but the resulting value could be anything.”

A pattern that I frequently use is to have my state->update() method return a shared_ptr to the state the game should switch to. If update returns an empty pointer, no state change occurs. Something like this (very quick, incomplete and un-tested, but it shows the gist of it)
enum EStates
{
	STATE_NULL,
	STATE_MENU,
	STATE_PLAYING,
	STATE_EXIT
};

class GameState
{
	public:
	GameState(){}
	virtual ~GameState(){}

	virtual std::shared_ptr<GameState> update()=0;
	virtual void event()=0;
	virtual void render()=0;

	virtual unsigned int getState()=0;
};


class MainMenuState : public GameState
{
	public:
	MainMenuState() : GameState(), nextState(STATE_NULL){}
	~MainMenuState(){}

	unsigned int getState()
	{
		return STATE_MENU;
	}

	std::shared_ptr<GameState> update()
	{
		switch(nextState)
		{
			case STATE_NULL: return std::shared_ptr<GameState>(); break;  // Return empty ptr
			case STATE_PLAYING: return std::shared_ptr<GameState>(std::make_shared<PlayingState>()); break;
			case STATE_EXIT: return std::shared_ptr<GameState>(std::make_shared<ExitState>()); break;
			default: return std::shared_ptr<GameState>(); break;
		}
	}

	void event()
	{
		cin >> nextState;
	}

	void render()
	{
		cout << "MAIN MENU" << endl;
		cout << "Press '3' to play"  << endl;
		cout << "Press '0' to continue in Main Menu"  << endl;
	}

	private:
	unsigned int nextState;
};

int main()
{
	std::shared_ptr<GameState> state=std::make_shared<MainMenuState>();

	while(state->getState()!=STATE_EXIT)
	{
	    state->render();
	    state->event();
		std::shared_ptr<GameState> newstate=state->update();
		if(newstate)
		{
			state=newstate;
			newstate.reset();
		}
	}
}

Update performs a switch on nextState as in your code, and returns either an empty pointer if no state change should occur, or it creates a new state object (std::make_shared news an object and assigns it to a shared_ptr in one call, safely) and returns a shared_ptr to the new state object. In the main loop, the return result of update is checked, and if it is a non-empty pointer, then state is assigned to the new pointer. This essentially overwrites the old state that was assigned previously, which will then drop out and be automatically and safely deleted.

Any time you find yourself doing what amounts to delete this you are almost certainly making an error, regardless of whether or not it seems to work. That sort of thing is just not wise to do unless you are experienced enough to know what is really going on.

This topic is closed to new replies.

Advertisement