Access Violation

[Hucaru]

Hi, I am making a game using SFML. I have implemented a state handling system and am just filling in the bare bones e.g. closing the game and changing states. However I have discovered an issue. When I close the window within the intro/splash state everything is fine. However when I close it from the menu state I get an access violation. The following is the layout of the classes:

class cStates{
public:
	void state(int *s);
	void handle(sf::Window *w);
	virtual void Initialise(bool *r) = 0;
	virtual void Draw(sf::Window *w) = 0;
	virtual void Input(sf::Window *w) = 0;
	virtual void Clean() = 0;
protected:
	int *cState;
	bool *run;
};

class cEngine{
public:
	bool Initialise();
	void Loop();
private:
	sf::Window window;
	cStates *states[4];
	int curState;
	int oldState;
	bool run;
};

The state class is mainly virtual and the intro and menu classes are as follows:

class cIntro: public cStates{
public:
	void Initialise(bool *r);
	void Draw(sf::Window *window);
	void Input(sf::Window *window);
	void Clean();
private:
};

class cMenu: public cStates{
public:
	void Initialise(bool *r);
	void Draw(sf::Window *window);
	void Input(sf::Window *window);
	void Clean();
private:
};

The input function is the only function within the states that actually does anything at the moment and for intro and menu it looks like the following:

void cIntro::Input(sf::Window *window){
	sf::Event event;

	while (window->pollEvent(event)){
		if (event.type == sf::Event::KeyReleased && event.key.code == sf::Keyboard::Space){
			*cState = 1;
		}
		if (event.type == sf::Event::Closed){
			window->close();
			*run = false;

		}
	}
}

void cMenu::Input(sf::Window *window){
	sf::Event event;

	while (window->pollEvent(event)){
		if (event.type == sf::Event::KeyReleased && event.key.code == sf::Keyboard::Space){
			*cState = 0;
		}
		if (event.type == sf::Event::Closed){
			window->close();
			*run = false;

		}
	}
}

As mentioned this only occurs when closing the window, which encompass and only on the menu state:

if (event.type == sf::Event::Closed){
	window->close();
	*run = false;
}

This leads me to believe that it has something to do with one of the pointers but that's as far as my debugging skills can go.

[Fransje]

Make sure you allocate memory for run before using it (and don't forget to delete afterwards).

[kbw]

Do cState and run really need to be pointers? As far as I can tell, they're probably not initialised and certainly not released.

You shouldn't have any protected data.

[Hucaru]

Thanks for the replies. I have moved the variables out of protected and into their respective classes as private. Also kbw I think they should be pointers as they change the data for variables that are part of the engine class to determine what state the program is in as well as to close the program down. The program now closes properly without any errors, without deleting the pointers. I then add delete ptr and this causes debug assertion failed to appear and when debugging it is the delete pointer that is causing it.

Sample code:

void cIntro::Clean(){
	delete run;
	delete cState;
}

void cMenu::Clean(){
	delete run;
	delete cState;
}

when program is asked to close this runs first for housekeeping before leaving the loop:

	for (int i = 0; i<3;i++)
		states[i]->Clean();

[ne555]

¿Did you create them with new ?

> they change the data for variables that are part of the engine class
¿Shouldn't the ownership be in the `engine' class then?
¿why is `state' managing their lifetime?

Also, you may consider sending a message to the object, instead of messing around with its organs

[Hucaru]

Also, you may consider sending a message to the object, instead of messing around with its organs

I will look into this. I have not come across the word organs before. Could you please elaborate on what you mean?

No I did not create them with new. Do you only delete variables that are created with the new prefix?

[LB]

Hucaru wrote:

I will look into this. I have not come across the word organs before. Could you please elaborate on what you mean?

It is an analogy.

You tell someone to do something, you don't move their arms for them.
You tell an object to do something, you don't change its variables for it.

[Hucaru]

I see so if I have understood this correctly I need to do something like

object.ChangeState() rather than passing the address of the variable I wish to change? If this is correct then withint the structure of my program I don't see how to do it as the structure is like this:

main:
create engine object

engine.cpp
set up the environment, such as current state, state list, create the window and handle the main game loop.

states (e.g. intro, menu, credits etc)
contain virtual functions that are called by from within engine object.

The system I have in place for changing state and ending the program has me passing the address of these two variables to the current state.

Engine main loop as well as the initialising function (called only once) looks like:

bool cEngine::Initialise(){
	curState = 0;
	oldState = 0;
	window.create(sf::VideoMode(RES_X,RES_Y), "RPG");

	states[0] = new cIntro;
	states[1] = new cMenu;
	states[2] = new cGame;
	states[3] = new cCredits;

	states[curState]->Initialise(&run, &curState);

	return true;
}

void cEngine::Loop(){
	while(run){
		if (oldState != curState)
		{
			states[curState]->Initialise(&run, &curState);
			oldState = curState;
		}
		states[curState]->Input(window);
		states[curState]->Draw(window);
	}
	for (int i = 0; i<3;i++)
		states[i]->Clean();
}

with the following header:

class cEngine{
public:
	bool Initialise();
	void Loop();
private:
	sf::RenderWindow window;
	cStates *states[4];
	int curState;
	int oldState;
	bool run;
};

It is currently working correctly. However am I writing 'bad' code?

[ne555]

> Do you only delete variables that are created with the new prefix?
Exactly (except that new is not a prefix), and everything that you new must be deleted (otherwise is a leak)
So you must delete each state.


> I need to do something like object.ChangeState() rather than passing the address of the variable I wish to change?

//void cEngine::Loop()
//states[curState]->Initialise(&run, &curState);
states[curState]->Initialise(this); //consider using the constructor instead

class cStates{
//	int *cState;
//	bool *run;
	cEngine *engine;
};

//void cIntro::Input(sf::Window *window)
	//*cState = 1;
	engine->next_state();
	//*run = false;
	engine->stop();

[Hucaru]

Okay thanks a lot! I will try and implement this. So I need to have the hierarchy be State Manager -> Engine. Rather than Engine -> State Manager?

EDIT: I have tried to do the following method but cannot conceptually visualise the structure of the program.

[ne555]

¿hierarchy?
You've got two objects that talk to each other.

The engine pass it itself to the `state::Initialize()', so they know who to respond to

[Hucaru]

Okay I will give this a try.