Questions about managing states

[AdventWolf]

Alright it has been a while since I have programmed but I have found some free time. State managing has really held me back because I can't seem to grasp how to do it! I started trying to put together a very simple state manager by looking at different resources but I'm having problems.

Here's the basic jest of it:

Code: [Select]

#ifndef ENGINE_HPP_
#define ENGINE_HPP_

#include <iostream>
#include <vector>

#include <SFML/Graphics.hpp>

class CGameState;

class Engine
{
public:

void Init(const string& title, int width=1024, int height=768,
     int bpp=32, bool fullscreen=false);
void Cleanup();

void ChangeState(CGameState* state);
void PushState(CGameState* state);
void PopState();

void HandleEvents();
void Update();
void Draw();

bool Running() { return m_running; }
void Quit() { m_running = false; }

sf::RenderWindow screen;

private:

std::vector<CGameState*> states;
bool m_running;
bool m_fullscreen;
};
#endif

Code: [Select]

#include "Engine.hpp"
#include "gamestate.hpp"

void Engine::Init(const string& title, int width, int height,
int bpp, bool fullscreen)
{
mode = sf::VideoMode(width, height, bpp);
Window.Create(mode, title);

m_fullscreen = fullscreen;
m_running = true;

printf("CGameEngine Init\n");
}

void Engine::Cleanup()
{
// cleanup the all states
while ( !states.empty() )
{
states.back()->Cleanup();
states.pop_back();
}
printf("GameEngine Cleanup\n");
}

void Engine::ChangeState(CGameState* state)
{
// cleanup the current state
if ( !states.empty() ) {
states.back()->Cleanup();
states.pop_back();
}

// store and init the new state
states.push_back(state);
states.back()->Init();
}

void Engine::PushState(CGameState* state)
{
// pause current state
if ( !states.empty() ) {
states.back()->Pause();
}

// store and init the new state
states.push_back(state);
states.back()->Init();
}

void Engine::PopState()
{
// cleanup the current state
if ( !states.empty() ) {
states.back()->Cleanup();
states.pop_back();
}

// resume previous state
if ( !states.empty() ) {
states.back()->Resume();
}
}

void Engine::HandleEvents()
{
// let the state handle events
states.back()->HandleEvents(this);
}

void Engine::Update()
{
// let the state update the game
states.back()->Update(this);
}

void Engine::Draw()
{
// let the state draw the screen
states.back()->Draw(this);
}

Code: [Select]

#ifndef GAMESTATE_H
#define GAMESTATE_H

#include "Engine.hpp"

class CGameState
{
public:
virtual void Init() = 0;
virtual void Cleanup() = 0;

virtual void Pause() = 0;
virtual void Resume() = 0;

virtual void HandleEvents(Engine* game) = 0;
virtual void Update(Engine* game) = 0;
virtual void Draw(Engine* game) = 0;

void ChangeState(Engine* game, CGameState* state) {
game->ChangeState(state);
}

protected:
CGameState() { }
};

#endif

Code: [Select]

#ifndef INTROSTATE_HPP_
#define INTROSTATE_HPP_

#include <SFML/Graphics.hpp>
#include "gamestate.hpp"

class CIntroState : public CGameState
{
public:
    void Init();
    void Cleanup();

    void Pause();
    void Resume();

void HandleEvents(Engine* game);
void Update(Engine* game);
void Draw(Engine* game);

static CIntroState* Instance() {
return &m_IntroState;
}

protected:
CIntroState() { }

private:
static CIntroState m_IntroState;
sf::Sprite OpenMenu;

};

#endif

Code: [Select]

#include <iostream>
#include <SFML/Graphics.hpp>

#include "Engine.hpp"
#include "IntroState.hpp"

CIntroState CIntroState::m_IntroState;

void CIntroState::Init()
{
std::cout << "start" << std::endl;
sf::Image Open;
if( !Open.LoadFromFile("OpenGameMenu.png") )
std::cout << "ERROR" << std::endl;

OpenMenu.SetImage(Open);
}

void CIntroState::HandleEvents(Engine* game)
{
sf::Event event;
    while( game->screen.GetEvent(event) )
    {
        if( event.Type == sf::Event::Closed )
{
            game->Quit();
}
}
}

void CIntroState::Draw(Engine* game)
{
game->screen.Draw(OpenMenu);
}

void CIntroState::Update(Engine* game)
{
game->screen.Clear();
game->screen.Display();
}


Code: [Select]

#include <iostream>
#include "Engine.hpp"
#include "IntroState.hpp"

int main()
{
Engine game;

// initialize the engine
game.Init( "Engine" );

// load the intro
game.ChangeState( CIntroState::Instance() );

// main loop
while ( game.Running() )
{
game.HandleEvents();
game.Update();
game.Draw();
}

// cleanup the engine
game.Cleanup();
std::cout << "END OF MAIN" << std::endl;
std::cin.get();
return 0;
}

But my problem is I'm not sure how to manage the Window, when I try to draw a sprite to the window, I get nothing. In here, I create a window in the engine class, but how do I reach the window while I'm in the states to change the window? What would be a simple implementation of the IntroState to create an window output, while being able to switch to another state that uses the same window?

[Groogy]

Give access either trough friends ( bad OOP ) or a getter function returning a reference/pointer to it. I would though prefer to pass a pointer to the window instance trough the states constructor so we avoid opening up our private data.

And as of why you can't see anything. Look in your Update function. You clear the view, then display it. And afterwards draw the sprite. See the problem?

[AdventWolf]

Could you show an example please? I spent some more time trying to work it out but I couldn't get it  

[Groogy]

Well first. Fix so that you can view on the screen. "Make it work, do it right then make it fast" which our teachers keep repeating...

Here's your problem areas:

Code: [Select]


game.Update();
game.Draw();


You first update and then draw. Logical and should work. BUT!
When we look here:

Code: [Select]


void CIntroState::Update(Engine* game)
{
   game->screen.Clear();
   game->screen.Display();
}


This means that we first Clear the screen, then display what's on it. And then we reach the Draw function of the state and draw to the screen. Which means nothing is shown on this frame. Next frame will do exactly the same thing. Clear the window and show what's on it. Which will be exactly like the last frame, nothing. Because we clear everything on it before showing it.

When you've made this work I'll show you how to do it right when it comes to how to share the Window between the engine and states.

[AdventWolf]

"Make it work, do it right then make it fast", I've been going by that mindset, when I try to do something I try to do it as simple as I can then expand from there.

And thanks! I probably wouldn't have noticed the mistake in the Update() function. I am getting closer now, when I run the program I can see a white background where the sprite should be, but the sprite doesn't show.

Though I did try to display a string and it works just fine.

Code: [Select]

void CIntroState::Init()
{
std::cout << "start" << std::endl;
sf::Image Open;
if( !Open.LoadFromFile("OpenGameMenu.png") )
std::cout << "ERROR" << std::endl;

OpenMenu.SetImage(Open);
str = sf::String("STRINGY!");
str.SetSize(50);
str.SetColor(sf::Color(180,0,0));

.........
.........

void CIntroState::Update(Engine* game)
{
game->screen.Clear();
}

void CIntroState::Draw(Engine* game)
{
game->screen.Draw(OpenMenu);
game->screen.Draw(str);
game->screen.Display();
}

It goes - create objects ->
clear -> draw -> display -> (loop)

It seems right, but I'm not sure where the problem lies.

EDIT: Well I copied the code to create the sprite over to the draw function and the sprite appeared, so I need to fix the code in the Init() function to get the sprite to appear.

EDIT: Ok I have it in a workable condition now, lol. I'll try to add another state to the program. What tips do you have to make it better?

[Groogy]

Well the sprite only holds a reference to the image object. So when you exit the function the image object will be deleted from memory and not exist anymore, and thus the sprite will only draw a white background(Your application would've crashed but seems you got lucky).

To solve this you have to store the image somewhere else. Quick fix is to save it as a private instance variable in the IntroState class.

Why SFML is designed like this is to keep the memory usage low and is also faster. If we use pointers to memory instead we only need to allocate the memory once and several sprites can use the same image.
Let's say we'll do it like this:

Code: [Select]

sf::Image image1("someImage.png");
sf::Image image2 = image1;
&image2 != &image1  // NOT THE SAME IMAGES!

What we do here is actually copying the images so image1 and image2 is not the same image objects but the same images pixel-wise.

I don't know your C++ knowledge so thought it couldn't hurt to just tell you the things many in my class often forget.

[AdventWolf]

Yeah that was the problem. I had the Sprite object saved in the class, but I didn't have the Image object saved in the class.

And yeah I know the difference with copying and references, but I didn't know I had to keep the Image object around even after declaring the Sprite object.

EDIT: I created another state and kept switching between the two and I didn't see any memory leaks so that's cool  . I'm going to keep messing with it to figure some stuff out. This takes a load off, I can finally do states.

[Groogy]

Aight Time to do it right. You should place sf::RenderWindow screen as a private variable in Engine and then pass it to the states trough their Draw and Update functions. Or some other way depending on how you want to use it. Will the Engine be used for anything in the Draw function? If not then you can just switch it's argument to be a sf::RenderWindow pointer instead.

Also the Clear and Display function calls should be located in the Draw function too. Update should handle logics and no rendering.

[AdventWolf]

Ok, screen is private and I suppose you mean I need a getter for it so the states can access it?

I have this:

Code: [Select]


//in Engine.hpp
private:
sf::RenderWindow screen;

public:
sf::RenderWindow* GetWindow();

......
......

//in Engine.cpp
sf::RenderWindow* GetWindow()
{
return *screen;
}


But i get an undeclared identifier error on screen for some reason. I only get the error in the GetWindow() function.

[Groogy]

Ok, screen is private and I suppose you mean I need a getter for it so the states can access it?

I have this:

Code: [Select]


//in Engine.hpp
private:
sf::RenderWindow screen;

public:
sf::RenderWindow* GetWindow();

......
......

//in Engine.cpp
sf::RenderWindow* GetWindow()
{
return *screen;
}


But i get an undeclared identifier error on screen for some reason. I only get the error in the GetWindow() function.

You are dereferencing a variable which is not a pointer. Probably because of that. You write & to get an address of a variable. So you want to write "return &screen"

And not what I had in mind but it works. You want to avoid Setter and Getter functions if they isn't actually needed. Since they remove the purpose of encapsulating data in an object.

What I ment was:

Code: [Select]


void CIntroState::Draw(sf::RenderTarget *canvas)
{
   canvas->Clear();
   canvas->Draw(OpenMenu);
   canvas->Display();
}


The argument can just simply be RenderWindow too, I made it like this so that a state can have more uses.

[AdventWolf]

I'm not sure what the deal was with screen there, I initially tried the reference but it didn't work so I tried the pointer.

And I see what you mean with RenderTarget, that seems like a great idea but changing

Code: [Select]

void CIntroState::Draw(Engine *game)

to

Code: [Select]

void CIntroState::Draw(sf::RenderTarget *canvas)

caused nothing but problems, should I change it in Engine.hpp and Engine.cpp? Because they are initially empty.

I have this:

Code: [Select]

//Engine.hpp
void HandleEvents();
void Update();
void Draw();


Code: [Select]

//Engine.cpp
void Engine::HandleEvents()
{
// let the state handle events
states.back()->HandleEvents(this);
}

void Engine::Update()
{
// let the state update the game
states.back()->Update(this);
}

void Engine::Draw()
{
// let the state draw the screen
states.back()->Draw(this);
//std::cout << "engine draw" << std::endl;
}


Code: [Select]

//gamestate.hpp
virtual void HandleEvents(Engine* game, sf::RenderWindow* canvas) = 0;
virtual void Update(Engine* game) = 0;
virtual void Draw(sf::RenderTarget *canvas) = 0;


Code: [Select]

//CIntroState.hpp
void HandleEvents(Engine* game,sf::RenderTarget *canvas);
void Update(Engine* game);
void Draw(sf::RenderTarget *canvas);


Code: [Select]

//CIntroState.cpp
void CIntroState::HandleEvents(Engine* game,sf::RenderTarget *canvas)
{
sf::Event event;
while( canvas->GetEvent(event) )
    {
//std::cout << "IN EVENT" << std::endl;
        if( event.Type == sf::Event::Closed )
{
            game->Quit();
}
else if( event.Type == sf::Event::MouseButtonReleased && event.MouseButton.Button == sf::Mouse::Left )
{
game->ChangeState( CPlayState::Instance() );
}
}
}

void CIntroState::Update(Engine* game)
{
}

void CIntroState::Draw(sf::RenderTarget *canvas)
{
canvas->Clear();
canvas->Draw(OpenMenu);
canvas->Draw(str);
        canvas->Display();
}


There is a lot of (this) conflict, so how do I need to format the code somehow to allow me to use RenderTarget? I spent a while trying to change it up but it seems to have endless errors.

[AdventWolf]

I finally found some more free time today so I went back at it and it works good. This is what I have:

Code: [Select]

\\in Engine.hpp
sf::RenderWindow screen;

Code: [Select]

\\in Engine.cpp
void Engine::Draw()
{
states.back()->Draw(&screen);
}

Code: [Select]

\\in gamestate.hpp
virtual void Draw(sf::RenderWindow* canvas) = 0;

Code: [Select]

\\in introstate.hpp
void Draw(sf::RenderWindow* canvas);

Code: [Select]

\\in introstate.cpp
void CIntroState::Draw(sf::RenderWindow* canvas)
{
canvas->Clear();
canvas->Draw(OpenMenu);
canvas->Display();
}

Now this code works, but is it correct? I thought it was a little awkward to have &screen but it worked. Thanks so much for your help.

[Groogy]

Awkward? That's completely right. You can also make it into a reference if you want to instead of having it as a pointer if you feel that using the &-operator is awkward.

Also remember placing sf::RenderWindow screen as private.

[heishe]

This is an excellent case for the Singleton Pattern. If you don't know what it is, google it up, it's pretty simple.

[Nexus]

There are several problems with this design.

• The Engine class has no clear area of responsibility. That's already apparent from the name. It handles game logics, event handling and graphical output at once.
• Init() and Cleanup() functions are C-style. You should use C++ constructors and destructors, as they can enforce proper initialization and destruction as well as consistent class invariants.
• You include headers you don't need (<iostream> in Engine.hpp, or "Engine.hpp" in GameState.hpp). While this may be no issue at the moment, clearly structuring dependencies becomes the more important the bigger the project grows.
• Beware of overusing singletons. Although they look promising and easy-to-use in the beginning, they bring a lot of hidden costs in the long term, such as higher complexity, hard-to-track side-effects and more maintenance (when adapting to code that uses the singleton). Singletons intend to guarantee single instantiation, but are often abused as better global variables. Additionally, not every class that has only one object needs to be a singleton.