Sign in to follow this  
sheep19

State

Recommended Posts

My Stage class needs to have a state, so I create the class StageState.
#ifndef STAGE_STATE_H
#define STAGE_STATE_H

class StageState
{
public:
	virtual ~StageState() = 0 {}
};

class StageRunning : public StageState
{
};

class StagePaused : public StageState
{
};

class StageCompleted : public StageState
{
};

class StageQuit : public StageState
{
};

#endif

class Stage // abstract base class
{
	StageState *_state; // on the heap

protected:
	
	// advance frame
	static void FrameAdvance(Min::Timer &, uint delay, uint &frame, uint &nextFrame);

public:

	Stage() { _state = new StageRunning; }
	virtual ~Stage() { delete _state; }

	// draw contents
	virtual void Draw() = 0;
	
	// play the game
	virtual void Play() = 0;

	// handle key presses
	virtual void HandleKeys(const SDL_Event &) = 0;

	// set the state
	void SetState(StageState *newState) { delete _state; _state = newState; }

	// returns the state of the Stage
	StageState *State() const { return _state; }
};
Look at SetStage(). Story is a subclass of Stage. That's how SetStage is called.
void Story::HandleKeys(const SDL_Event &event)
{
	if( event.key.keysym.sym == SDLK_ESCAPE )
	{
		if( dynamic_cast<StagePaused*>(State()) ) // if the game is paused
			SetState(new StageRunning);
		
		if( dynamic_cast<StageRunning*>(State()) )
			SetState(new StagePaused);
	}
}

It needs the new operator. Also I have to use dynamic_cast to test what actual class it is. Is this a good way? (I didn't use enumerators because it gets mixed up with other enums in the other header files and I get compiler errors.)

Share this post


Link to post
Share on other sites
As it currently stands, your code behaves mostly like a "switch + enumeration" solution, but is much more complicated and error-prone.

Either you implement the state pattern properly by abstracting any state-dependent behavior into a virtual function (to avoid dynamic casting) and normalize your memory ownership policies appropriately with auto_ptr or shared_ptr, or you implement things using a simple enumeration, which you can define as a member of a class to avoid collisions:
class Stage
{
public:
enum State { COMPLETED, PAUSED, QUIT } state;
};

Used as Stage::COMPLETED (for instance).

Share this post


Link to post
Share on other sites
As a general rule of thumb, if you use dynamic_cast, you haven't thought your problem through.

Two observations:

1. Your argument against using enumerations is not convincing at all. If you get name conflicts, then maybe you need to think of better names.

2. In this case, since you are using class hierarchies anyway, it would be much cleaner to use virtual functions. You could add an abstract virtual HandleKeys function to StageState, which is then implemented in the different states to perform whatever actions are appropriate in the corresponding state.

Edit: bah, ninja'd

Share this post


Link to post
Share on other sites
I considered using the State pattern but Stage has many subclasses so I would need to make State subclasses for those too... or have a very large ugly if statement.


void StateRunning::HandleKeys(Stage *s, Event event)
{
if( /*s == stage1*/ )
blah blah
}



I'll use the enumerator in the class.

Share this post


Link to post
Share on other sites
Thanks :)

If Stage contains this function:

// set the state
virtual void SetState(StageState newState) = 0 { _state = newState; }


Will the function for the parent class be called too when it is overriden?

Share this post


Link to post
Share on other sites
No, you have to explicitly call the parent function yourself, for instance:

void Child::Frobnicate() {
// Do other things before
Parent::Frobnicate();
// Do other things afterwards
}


Substituting your child and parent class name where appropriate. Also, don't put that '= 0' there, it should only happen when you define pure virtual functions.

Share this post


Link to post
Share on other sites
This is the Stage class now:


class Stage // abstract base class
{
public:
enum StageState { RUNNING, PAUSED, COMPLETED, QUIT };

protected:
StageState _state;

// advance frame
static void FrameAdvance(Min::Timer &, uint delay, uint &frame, uint &nextFrame);

public:

Stage() { _state = RUNNING; }

// draw contents
virtual void Draw() = 0;

// play the game
virtual void Play() = 0;

// handle key presses
virtual void HandleKeys(const SDL_Event &) = 0;

// returns the state of the Stage
StageState State() const { return _state; }

// set the state
virtual void SetState(StageState newState) = 0;
};



Definition of Story (subclass of Stage)

#include "Stage.h"

using namespace Min;

Story::Story() : _background(NULL), _frame(0), _nextFrame(0), _text("courier_new.ttf", 20)
{
try
{
_background = new Sprite(0, 0, "images/backgrounds/story_0.png");
}
catch(...)
{
log.FunctionError("Story constructor", "Memory allocation for _background failed.");
}

_text.SetColor(255, 128, 0);
_text = "Maya...An ancient civilization...";

_timer.Start(); // start the timer

_nextFrame = _timer.GetTicks();
}

void Story::Draw()
{
_background->Blit(0, 0);
_text.Blit(100, 100);
}

void Story::Play()
{
if( State() == RUNNING )
{
if( _frame == 0 )
{
_text = "Maya...An ancient civilization...";
FrameAdvance(_timer, 3000, _frame, _nextFrame);
}
if( _frame == 1 )
{
_text = "Maya had predicted the world's devastation in 2012 due to an astrological phenomenon...";
FrameAdvance(_timer, 5000, _frame, _nextFrame);
}
if( _frame == 2 )
_text = "There is still hope though...";
}
}

void Story::HandleKeys(const SDL_Event &event)
{
const SDLKey &key = event.key.keysym.sym;

if( key == SDLK_ESCAPE )
{
if( State() == RUNNING )
SetState(PAUSED);

if( State() == PAUSED )
SetState(RUNNING);
}
}

void Story::SetState(Stage::StageState newState)
{
_state = newState;

if( State() == RUNNING )
_timer.Unpause();

if( State() == PAUSED )
_timer.Pause();
}

I think it's much better now.

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
(I didn't use enumerators because it gets mixed up with other enums in the other header files and I get compiler errors.)
A simple way to get around this is to wrap each enumeration in a namespace:
namespace StageState {
enum Type {
RUNNING,
PAUSED,
COMPLETED,
QUIT
};
}
You can then refer to the enumeration type as StageState::Type, and to individual enumeration values as StageState::Paused.

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
This is the Stage class now:
Stage::StageState is redundant. You could have named your enumeration "State" and used it as "Stage::State" when disambiguation is required.

Share this post


Link to post
Share on other sites
Quote:
Original post by ToohrVyk
Quote:
Original post by sheep19
This is the Stage class now:
Stage::StageState is redundant. You could have named your enumeration "State" and used it as "Stage::State" when disambiguation is required.


It was done automatically by VC++'s auto completion :)

Share this post


Link to post
Share on other sites
I tend to find all-in-one FSM-style state enumerations like that one tend not to scale very well as more states get added. Yet for a small amount of states it often feels a somewhat contrived design seemingly put in place to allow for adding more states in the future - see my previous statement about not scaling well.

It might not be worth your time changing it now since you already have something that works, but this isn't going to stop be telling you how I would have done it:

To represent a Completed state I would have given Play a bool return type to signal whether it's finished or not.

To represent a Quit state I would have given HandleKeys a bool return type to signal whether the user wants to quit or not.

To represent both Paused and Running states I would have introduced a bool paused; data member, then added a togglePause() function which can be used to toggle between paused and running without having to inspect the current state first and conditionally respond to it.

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