State machine - requesting advice

[Darkhog]

Time of implementing state machine in Super Heli Land is closer and closer, so I want to ask you for advice whether my idea of doing it is correct or not.

All states would inherit from TState class which would look more or less like this:


TState = class
public
Constructor Create;
Destructor Destroy;
procedure update;
procedure draw;
private
{...}
end;


There will be also PState, which would be pointer to TState:

PState = ^TState;

All children classes of TState would overload update and draw methods (first would be ran 60 times per second using Allegro timer, last would be called in main loop as fast as it can).

Main file would contain one PState declaration:

var CurrentState:PState;
and also single instances of every possible state (which in case of SHL would be only 4: Menu, options, actual game and high scores, with best high score being displayed in main menu.

Then after creation of each state, allegro's initialization would take place, then CurrentState would be set to main menu state:

CurrentState:=@MainMenuState;
then timer callback and main loop would be started.

The update (rendering) loop would look like that:

repeat
CurrentState^.draw;
until quit;

(quit is boolean variable) and timer callback would look like this:

procedure update();CDECL;
begin
//frame update
CurrentState^.update;
end;

States would be changed by setting CurrentState so it would point to other state.

So there's my idea. How much of it is correct way of doing this?
And what problems I'll face down the road (this will be my first time ever I'll implement state machine)?

[User137]

In something like this you don't need pointers with class types, so you can safely delete PState type. You can do

CurrentState:=MainMenuState;
It copies a reference to the MainMenuState object, not any actual data. Class variable is actually almost like pointer.

[SilverWarior]

You know what that is a great idea. Infact that is a genious idea. I never thought about handling the game states this way.
I can see many benefits that such approach could have in comparison to most oftenly used approach where you simply have one gloval variable teling your software in which state it is and then you use case statment to determine which part of the code should execute.
Main benefit would be the ability to have seperate initialization and finalization code for each state. Theese are being caled on creation or destruction of each state.
You can also easily make each state to actually have its own substates which could also be verry handy.
Also the same way as you are handling state specific drawing you could also handle state specific input handling. And all that by simply changing one variable.

[User137]

You just have to be careful on not to make it more complicated than the variable approach. Maybe i'm oldschool, but i still prefer the case-statement with variables. Main state, and possible sub-states, like:

TMainStates = (msIntro, msMenu, msGame);
Actually that's all i need from states i think, most of the time. From the top of my head i cannot think of a single scenario where i'd need sub-states or more main-states than that.

[SilverWarior]

Actually that's all i need from states i think, most of the time. From the top of my head i cannot think of a single scenario where i'd need sub-states or more main-states than that.

The place wehere I might use susbstates is right in the MainMenu for handling sub menues (New game screen, Save game screen, Load game screen, Options menu, Credists screen, etc.)

You are handling your games by only using three states? Where do you do initialization and finalization parts? Please don't tell me that you are one of those guys who make games in a way that every ingame resouce gets loaded a t the verry start of the aplication and not only when they are needed to.

[Darkhog]

You know what that is a great idea. Infact that is a genious idea. I never thought about handling the game states this way.
I can see many benefits that such approach could have in comparison to most oftenly used approach where you simply have one gloval variable teling your software in which state it is and then you use case statment to determine which part of the code should execute.
Main benefit would be the ability to have seperate initialization and finalization code for each state. Theese are being caled on creation or destruction of each state.
You can also easily make each state to actually have its own substates which could also be verry handy.
Also the same way as you are handling state specific drawing you could also handle state specific input handling. And all that by simply changing one variable.

Thanks! I didn't know you're flattener ;). Also for this:

Also the same way as you are handling state specific drawing you could also handle state specific input handling
I think it can be done by private function in specific child class which would be called from withing TState.update. Control doesn't have be any more precise than frame rate, anyways.

I also want to thank User137 for thing with pointers. Since I don't particularly like them thanks to you I'd be able to avoid them.

For switch-case thing... Well I've considered it, but I want to produce clean code. What if I'd like later on add yet another state? Then whatever function would have those switch-cases would get long (like hundreds lines of code long) and to get to actual state code (since e.g. you wanted to change something in behavior in a game state) you'd need to scroll through it. With my approach, every state can live in its own file and would be easy to modify.

I don't just want to make a game. I want to make maintainable game.

[User137]

Case structure for states can look like this:

// In render loop
case MainState of
msIntro: RenderIntro;
msMenu: RenderMenu;
msGame: RenderGame;
end;
...
// In the game loop
case MainState of
msIntro: LoopIntro;
msMenu: LoopMenu;
msGame: LoopGame;
end;
If you need to add another state, you can just make new procedures, and make related code there very cleanly. If you need substates, you can make new similar case inside LoopMenu for example. Depending on implementation, i might use menu classes of nxpascal, similar to IDE components in practise. If i click "Start game" button:
- In component approach the onClick event would be raised and handled, and then call StartGame() in it. I could simply hide or show entire panels of buttons and things.
- Oldschool way a StartGame() procedure would be called directly in some onMouseDown/Up/click event. There needs to be some integer variable telling index of button that was mouse-focused, and another variable telling which menu is open.
Another variables handling fadein, fadeout of states. Usually i only set NewState:=msGame; or something in the StartGame(). After the msGame has faded out, then set MainState:=NewState; . This can happen in the general loop, common for all states.

[Darkhog]

Still, seems like hacky solution to me. I JUST DON'T WANT TO FORGET MEANING OF MY CODE AFTER NOT LOOKING INTO IT FZOR 3 MONTHS OR MORE ANYMORE!!!

Erm... Sorry for that little burst. Anyway, I think there may be a problem with setting states from within other states.

Problem is that I won't be able to access global variable declared in main program from within other units (each state would get its own unit for clarity sake). Any ideas how to solve it?

[SilverWarior]

You can always add main unit in uses clause. And for avoiding ciclick dependancies you do it in implementation part of your state units.
So for instance you have UMain unit as general game unit and several other units each for its own state.
For you to be able to create theese state objects you need to add theese units into uses section of your UMain file.
But if you wanna acces objects and variables declared which are in UMain file from theese state units you add UMain file into uses section which you declare in implementation section:

...
implementation
uses UMain //Main game unit
...
Do not add UMain unit into uses section on top of your states units othevise you will cause cicklic redundancies (compiler won't be able to figure out which units need which) and you won't be able to compile your project.

[SilverWarior]

Still, seems like hacky solution to me.

It is not so teribly hacky. Until now I have always been using such approach. But I still think that your idea is actually better.
Anywhay in the end it all depends on how good you implement it. You can have great idea but if your implementation of this idea is terible so will be result.

[User137]

I have 1 rule of thumb for all applications i ever make: Never add application main unit to uses list of other units. It is really unmodular and bad habit. Only when it's really really necessary for 2 forms needing alot of communication between eachother, then i might consider cross-using.

Instead you can make for example gameunit.pas or gametypes.pas, and introduce all global types and variables there. This unit would use no other units in itself, but could be included up to all your other units. Kind of like this:
https://code.google.com/p/nxpascal/source/browse/trunk/src/nxTypes.pas
nxTypes is included throughout the whole engine, and all of the units have access to nxSetError() for consistent error reporting, types and other little things.

And on topic, if you would have to make a whole class implementation for each different state, then at least i would consider it more complicated than the case-procedure way. It might be smarter, but it might take a whole lot more code lines :)

[Darkhog]

Well, it might take more lines, but in the end I'll get nice clean and extensible code. And if I declare my state variable in e.g. Unit global, it'll modify same area of memory when accessed from unit A (which includes it) and unit B? Just want to be sure.

Also thanks for being so helpful. I'll be sure to re-read this thread when I'll do actual state machine implementation (for now I need to get done more important stuff like chunk rendering and tileset handling).

[laggyluk]

i've used it in few projects but i can't say i invented it :p
http://gamedevgeek.com/tutorials/managing-game-states-in-c/
check out the 'stack of states' thing - really usefull for menus

[Darkhog]

Laggyluk, interesting read, thanks.

Now that I've started actually implementing this, I'll post any problems I'll have with this in this thread.

[Darkhog]

Oops... My design seems to not working. Nothing past initialization messages which are part of main file is displayed as if it isn't even executed.

Here is project file code sans legacy, commented out code:

program alletest;

{$mode objfpc}{$H+}
{$apptype gui}
uses
{$IFDEF UNIX}{$IFDEF UseCThreads}
cthreads,
{$ENDIF}{$ENDIF}
Classes, allegro, sprites, PerlinNoiseUnit, WorldGen, boolUtils,
Tilesets, ChunkUtils, Globals, states;

procedure quitfunc();CDECL;
begin
quit:=true;
end;

procedure update();CDECL;
begin
//frame update
CurrentState.Update;
end;


begin
Randomize;
//initializing Allegro
if al_init then
begin
//setting up window

al_set_color_depth(al_desktop_color_depth);
al_set_gfx_mode(AL_GFX_AUTODETECT_WINDOWED,SCREENW ,SCREENH,SCREENW,SCREENH);
al_set_window_title('Super Heli Land');
al_set_close_button_callback(@quitfunc);
//installing keyboard
al_textout_ex(al_screen,al_font,'Installing keyboard... ',0,0,al_makeacol_depth(al_desktop_color_depth,128 ,128,128,255),al_makeacol_depth(al_desktop_color_d epth,0,0,0,255));
al_install_keyboard;
al_textout_ex(al_screen,al_font,'OK',600,0,al_make acol_depth(al_desktop_color_depth,0,255,0,255),al_ makeacol_depth(al_desktop_color_depth,0,0,0,255));

//installing timers
al_textout_ex(al_screen,al_font,'Installing timers... ',0,20,al_makeacol_depth(al_desktop_color_depth,12 8,128,128,255),al_makeacol_depth(al_desktop_color_ depth,0,0,0,255));
al_install_timer;
al_textout_ex(al_screen,al_font,'OK',600,20,al_mak eacol_depth(al_desktop_color_depth,0,255,0,255),al _makeacol_depth(al_desktop_color_depth,0,0,0,255)) ;

//installing update timer
al_textout_ex(al_screen,al_font,'Initializing update routine... ',0,40,al_makeacol_depth(al_desktop_color_depth,12 8,128,128,255),al_makeacol_depth(al_desktop_color_ depth,0,0,0,255));
al_install_int_ex(@Update,AL_BPS_TO_TIMER(60));
al_textout_ex(al_screen,al_font,'OK',600,40,al_mak eacol_depth(al_desktop_color_depth,0,255,0,255),al _makeacol_depth(al_desktop_color_depth,0,0,0,255)) ;
//loading marioland font
MarioFont:=al_load_font('marioland.pcx',nil,nil);

al_textout_ex(al_screen,al_font,'Creating main menu... ',0,120,al_makeacol_depth(al_desktop_color_depth,1 28,128,128,255),al_makeacol_depth(al_desktop_color _depth,0,0,0,255));
MainMenuState:=TMainMenuState.Create;
CurrentState:=MainMenuState;
al_textout_ex(al_screen,al_font,'OK',600,120,al_ma keacol_depth(al_desktop_color_depth,0,255,0,255),a l_makeacol_depth(al_desktop_color_depth,0,0,0,255) );

al_textout_ex(al_screen,al_font,'Starting main loop... ',0,140,al_makeacol_depth(al_desktop_color_depth,1 28,128,128,255),al_makeacol_depth(al_desktop_color _depth,0,0,0,255));

//main loop
repeat
CurrentState.BeforeDraw;
CurrentState.Draw;
CurrentState.Main;

until quit;
//destroying font, so we won't have any leaks
al_destroy_font(MarioFont);
if CurrentState<>nil then CurrentState.Destroy;
if MainMenuState<>nil then MainMenuState.Destroy;
end;
end.


And here is States unit which defines states:

unit States;

{$mode objfpc}{$H+}

interface

uses
Classes, SysUtils,sprites,allegro;

type

{ TState }

TState = class
public
constructor Create;
destructor Destroy;override;
procedure Update;
procedure BeforeDraw;
procedure Draw;
procedure Main;
private

end;

{ TMainMenuState }

TMainMenuState = class(TState)
public
constructor Create;
destructor Destroy;override;
procedure Update;
procedure BeforeDraw;
procedure Draw;
procedure Main;
private
BGSprite:TSprite;
SelectionShroomSprite:TSprite;
Buffer:AL_BITMAPptr;
StartItemSprite:TSprite;
OptionItemSprite:TSprite;
ExitItemSprite:TSprite;
MenuIndex:Integer;
destroying:boolean;

end;
function keyreleased(idx:Integer):Boolean;
implementation
uses globals;
var
last_al_key : AL_KEY_LIST;


{ TMainMenuState }

constructor TMainMenuState.Create;
var bmp:AL_BITMAPptr;
begin
inherited;
//creating title image
bmp:=al_load_pcx('title.pcx',@al_default_palette);
BGSprite:=TSprite.Create(bmp,bmp);
BGSprite.ScaleFactor:=4;
BGSprite.UpdateMask; //need to be called.
//loading selection mushroom
bmp:=al_load_pcx('mushroom.pcx',@al_default_palett e);
SelectionShroomSprite:=TSprite.Create(bmp,bmp);
SelectionShroomSprite.ScaleFactor:=4;
SelectionShroomSprite.UpdateMask; //need to be called.
SelectionShroomSprite.x:=32*4;
SelectionShroomSprite.y:=96*4;
SelectionShroomSprite.magentatransparent:=true;
//creating Start option sprite
bmp:=al_create_bitmap(5*8,8);
al_clear_to_color(bmp,al_makecol(255,0,255));
al_textout_ex(bmp,MarioFont,'Start',0,0,al_makecol (0,0,0),-1);
StartItemSprite:=TSprite.Create(bmp,bmp);
StartItemSprite.ScaleFactor:=4;
StartItemSprite.x:=40*4;
StartItemSprite.y:=96*4;
StartItemSprite.magentatransparent:=true;
//creating "Option" option sprite
bmp:=al_create_bitmap(6*8,8);
al_clear_to_color(bmp,al_makecol(255,0,255));
al_textout_ex(bmp,MarioFont,'Option',0,0,al_makeco l(0,0,0),-1);
OptionItemSprite:=TSprite.Create(bmp,bmp);
OptionItemSprite.ScaleFactor:=4;
OptionItemSprite.x:=40*4;
OptionItemSprite.y:=104*4;
OptionItemSprite.magentatransparent:=true;
//creating Exit option sprite
bmp:=al_create_bitmap(4*8,8);
al_clear_to_color(bmp,al_makecol(255,0,255));
al_textout_ex(bmp,MarioFont,'Exit',0,0,al_makecol( 0,0,0),-1);
ExitItemSprite:=TSprite.Create(bmp,bmp);
ExitItemSprite.ScaleFactor:=4;
ExitItemSprite.x:=40*4;
ExitItemSprite.y:=112*4;
ExitItemSprite.magentatransparent:=true;
//setting menu index to 0
MenuIndex:=0;
//creating drawing buffer so it won't blink
Buffer:=al_create_bitmap(SCREENW,SCREENH);
//telling we aren't destroying state just yet
destroying:=false;
end;

destructor TMainMenuState.Destroy;
begin
inherited Destroy;
//we need to set that to avoid potential segfaults related to drawing
destroying:=true;
//removing buffer
al_destroy_bitmap(Buffer);
//removing exit option sprite
ExitItemSprite.Destroy();
//removing "option" option sprite
ExitItemSprite.Destroy();
//removing start option sprite
StartItemSprite.Destroy();
//removing selection mushroom
SelectionShroomSprite.Destroy();
//removing title card
BGSprite.Destroy();
end;

procedure TMainMenuState.Update;
const LastMenuItem=2;
begin
inherited;
if not destroying then
begin //making sure we won't get any accidental segfaults

//handling keyboard
if keyreleased(AL_KEY_UP) then Dec(MenuIndex);
if keyreleased(AL_KEY_DOWN) then Inc(MenuIndex);
if MenuIndex>LastMenuItem then MenuIndex:=0;
if MenuIndex<0 then MenuIndex:=LastMenuItem;
case MenuIndex of
0 : begin SelectionShroomSprite.y:=96*4; end; //start
1 : begin SelectionShroomSprite.y:=104*4; end;//option
2 : begin SelectionShroomSprite.y:=112*4; end;//exit
end;
if (keyreleased(AL_KEY_ENTER) or keyreleased(AL_KEY_ENTER_PAD)) then
begin
case MenuIndex of
0 : begin end; //start
1 : begin end;//option
2 : begin quit:=true; self.Destroy; end;//exit
end;
end;
end;
end;

procedure TMainMenuState.BeforeDraw;
begin
inherited;
end;

procedure TMainMenuState.Draw;
begin
inherited;
if not destroying then
begin
//we embed it that way, just to avoid potential segfaults.
BGSprite.Draw(Buffer);
StartItemSprite.Draw(Buffer);
OptionItemSprite.Draw(Buffer);
ExitItemSprite.Draw(Buffer);
SelectionShroomSprite.Draw(Buffer);
al_blit(buffer,al_screen,0,0,0,0,SCREENW,SCREENH);
end;
end;

procedure TMainMenuState.Main;
begin
inherited;
end;

function keyreleased(idx: Integer): Boolean;

begin
Result := ((al_key[idx]=0) and (last_al_key[idx]<>0));
last_al_key:=al_key;
end;

{ TState }

constructor TState.Create;
begin
inherited;
//Here state is initialized. Every state needs to keep their own buffer for
//double/triple buffering. All resource loading should be done here.
end;

destructor TState.Destroy;
begin
inherited;
//You should dispose of any resources made in Create if you don't want to cause
//memory leak
end;

procedure TState.Update;
begin
//Update is called every frame (60 times per second).
//It is designed to update objects (collision check, movement)
//You shouldn't, however, do any write to variables (change values)
//that aren't primitives (writing to integers, strings, etc. is fine)
//or you'll get memory leak
end;

procedure TState.BeforeDraw;
begin
//Like Main, this is called in main loop and usage is similar.
//It is, however called before drawing.
end;

procedure TState.Draw;
begin
//Drawing is done here. It is called in main loop (as fast as it can).
end;

procedure TState.Main;
begin
//This method is also called in main loop. It is designed to do things
//that aren't drawing and needs to be called in main loop, for example
//rebuilding map, updating frame of TAnimatedSprite, etc.
end;

end.


//edit: Globals unit which may also be part of issue:

unit Globals;

{$mode objfpc}{$H+}

interface

uses
Classes, SysUtils,states,allegro;
var
CurrentState:TState;
MainMenuState:TMainMenuState;
Quit:Boolean;
MarioFont:AL_FONTptr;
const
SCREENW=640;
SCREENH=576;
implementation

end.

//edit #2: Also MainMenuState.Destroy gives me segfault in some function of ntdll, wait a minute... ntdll!LdrWx86FormatVirtualImage (1584). When I comment it out, it closes fine, but then I have mem leak..

[User137]

//if CurrentState<>nil then CurrentState.Destroy; // This you can't call.
//CurrentState = MainMenuState sometimes, and calling Destroy on already Destroyed object brings trouble...
if MainMenuState<>nil then MainMenuState.Destroy;

Not entirely sure, but i think you need to override them all. CurrentState is type TState, so calling CurrentState.Update etc must refer to object it was created as. Hence override.

TMainMenuState = class(TState)
public
constructor Create; override;
destructor Destroy; override;
procedure Update; override;
procedure BeforeDraw; override;
procedure Draw; override;
procedure Main; override;

[SilverWarior]

The reason why you are getting segfault on MainMenuState.Destroy is becouse you are calling inherited on the start of the destructor method.
In constructor (SomeObject.Create) you always call inherited on start of the method to make sure that constructor code from parent class is executed first.
But in destructor (SomeObject.Destroy) you should always call inherited last othervise destructor code from parent class is executed too fast and can lead to memory leaks due to only freeing part of your objects memory (only memory of parent class is being freed up).

As for nothing except initialization showing shouldn't you be calling the stuff from your Main Loop within the OnTimer events of your timers that you prepared in initialization part of your game?

[Darkhog]

Well, yes, but some things are needed to be called in main loop, like drawing. As for your suggestion regarding segfault (calling inherited in destroy last), it didn't fix the issue.

@User137: Segfault is even there if only I'm destroying only MainMenuState, without touching CurrentState.

As for overriding, let the compiler speak for itself:

states.pas(32,17) Error: There is no method in an ancestor class to be overridden: "TMainMenuState.Update;"
states.pas(33,17) Error: There is no method in an ancestor class to be overridden: "TMainMenuState.BeforeDraw;"
states.pas(34,17) Error: There is no method in an ancestor class to be overridden: "TMainMenuState.Draw;"
states.pas(35,17) Error: There is no method in an ancestor class to be overridden: "TMainMenuState.Main;"
states.pas(48,1) Fatal: There were 4 errors compiling module, stopping


And those methods are in ancestor (TState)!

[imcold]

Yes, but you need to mark a method as "virtual" in your ancestor to be able to override it.

[Darkhog]

It works, thanks! Now if only I could figure out that pesky segfault...

[User137]

Figured it out, you need to have virtual; at the end of the TState methods. If you also add them abstract; , then you don't need to make them implementation code at all. But that is only useful tip if you never plan to add them any code, and also if they're abstract, then they must be defined in all inheriting classes.

Can't see the source for segfault if you did also what SilverWarior said.

1 other tip that i could give, is making new unit for TState alone. Then you can use the same state-machine in all your projects. You could move the CurrentState variable there, and also make a universal procedure like

procedure RunStatesApp;
begin
repeat
CurrentState.BeforeDraw;
CurrentState.Draw;
CurrentState.Main;
until quit;
end;
that you would call from main program unit.
(Make sure there are no uses-list's in the universal state-unit)

[Darkhog]

Hm, thanks. May I send you project files so you can help me debugging this segfault (will try to check callstack before that)?

//edit: Callstack isn't helpful. It shows only that function I mentioned before (ntdll!LdrWx86FormatVirtualImage (1584)) and since it seems to be Windows function, I can't really do anything about it...

Question: Do modern operating systems (Windows/Linux/Mac) free up memory that was used of program even if it was leaked? If they are I could probably get away with not freeing states up at end of program without any damage for players. Of course best would be to fix underlying issue, but if you won't be able to help I'll need some sort of last resort.

[User137]

Hm, thanks. May I send you project files so you can help me debugging this segfault
Sure. You could also consider using some webservice to temporarily upload files. Speaking of which, i guess i'll have to try Dropbox myself. Are there better ones? Google drive sharing is not very anonymous i think, doesn't everyone see your email?
edit2: Seems i was wrong about the email thing. You only see yourself logged in google account, and possibly other accounts viewing them, but just their names.

edit:

Question: Do modern operating systems (Windows/Linux/Mac) free up memory that was used of program even if it was leaked?
Yes, they free it. But i'm not sure if attached debugger like used for Delphi or Lazarus handles that well.

[Darkhog]

Well, I'm only worried about players when they will want to play SHL several times in a row and they'll fill memory by accident. I'm not worried about me as developer, as I know what I'm getting into and in worst case, I can restart PC. Though I don't want players to need to restart their PCs.

Anyway, as I've said, this is last resort. I want to figure out how to actually fix that.

[SilverWarior]

Normaly on Windows when you close the application all of its memory is released. There are exceptions when a program is using two or more seperate memory managers but this is quite unlikely in your case.

Now you can send me the source code and I'll take a look at it to see what might be causing theese segfaults.

[User137]

I've had some look on the source code, and found some things which i PM'd. But i didn't find the crash on exit cause. It came down to al_destroy_bitmap(MaskBitmap); line. Are there some commands to issue for Allegro when application is quitting? I noticed it's sort of threaded, by linking a update procedure in it. But crash when destroying bitmap, which isn't nil, is very odd. That linked update thread wasn't responsible of drawing things. Main loop was already stopped at that time.