Game programming structure tips (PM log)

[Disch]

So I received some PMs with questions about how to improve some code. I usually tell people to post on the boards, but in this case I just replied to the PM. Realizing the error of that decision, I decided to ask permission to post the PM convo on the boards for all to see.

So without further ado, here's a PM log between Moarthenfeeling and myself:



Moarthenfeeling wrote
I'm sorry, to bother, but I have some really simple question and I don't want to create new topic.
I've got a function, which returns pointer to the sprite. Here it is:

Sprite* DrawTile(char tile)
{
	if(currentLevel.GetTileset()==0)
	{
		if(tile=='w')
			return building1wspr;
		else if(tile=='d')
			return building1dspr;
		else if(tile=='b')
			return building1spr;
		else if(tile=='f')
			return fencespr;
		else if(tile=='B')
			return boxspr;
		//etc.
	}
	if(currentLevel.GetTileset()==1)
	{
		if(tile=='f')
			return floorspr;
		else if(tile=='w')
			return wallspr;
		else return wallspr;
		//etc.
	}
}

Tiles are taken from map array.
Yes, it works perfect, but there are some troubles with reading such code, because creating more tilesets will become this code very long. Is there any solutions to make this code easier? I don't ask for the code, just need some advice :)

[Disch]

I wrote
You should objectify more.

A Tile is a clear "thing" that can be represented by an object. Therefore you should have a Tile class that does everything that a Tile would do.

Organizing this way makes things much easier.

class Tile
{
public:
  // give the Tile a Draw function so that it can be in charge of
  //   its own drawing
  void Draw(int destx,int desty,Screen* screen);  // <- something like that

  // put any properties of the tile here
  bool walkable;  // can the player walk on it?
  //.. etc
};

Then instead of using magic numbers to determine the tileset, you can just have the tileset be a collection of different Tiles:

std::vector<Tile> tileset;

// then when you create the tileset, generate your tiles and put them into
//   the vector
Tile foo;
foo.walkable = true;
foo.otherstuff = whatever;

tileset.push_back(foo);

The map can then be pointers to tiles, taken from the given tileset:

array2d<Tile*> map;

// the tile at coord 0,0 is a floor tile:
//  (floor tile = tile #0 in our tileset)
map(0,0) = &tileset[0];

// coord 0,1 is a wall (tile # 1 in the tileset)
map(0,1) = &tileset[1];

// etc.  

This makes it pretty easy to manage. It allow allows for quick and easy access to Tile members if you have the coords. Example, to draw:

for(y = 0 ....
{
  for(x = 0 ...
  {
    map(x,y)->Draw(...)
  }
}

or if you want to find out whether or not a spot is walkable:

if(map(x,y)->walkable)
{
  // ok to walk at x,y
}
else
{
  // can't walk at x,y
}

[Disch]

Moarthenfeeling wrote
Okay, I get it, thanks for the advice. I'm new to OOP, so some things are not clear for me, he-he. I should re-edit the code moar, yeah.
I hope, problems I'll have in teh future will be less difficult and they won't bother anyone. :)

Moarthenfeeling wrote again
Oh, I read your message again and understood, that you got me wrong :>

 if(tile=='d')
     return building1dspr;
else if(tile=='b')
     return building1spr;

tile is a char, which is taken from the .txt file of map. Every tile has it's char value, so according to this value I must return the pointer to the sprite. But if I'll hold the pointer in the tile, the initialization will be the same. Like:

if(currentcharfromtextfile=='d')
   tile.sprite=somesprite;
else if(currentcharfromtextfile=='s')
   tile.sprite=somesprite2; //etc.  

Is there any way to avoid this if-else constructions?

[Disch]

I wrote

>Oh, I read your message again and understood, that you got me wrong :>

I understood what you're doing ;P

Let me explain in more detail.

If you want to avoid else if chains like that, you need to make things nameless. I mean if you have a sprite named "buliding1dspr" then that's pretty rigid and there isn't really a way around unwieldly else if chains.

To remove those names, you need to make everything dynamic. So what does that mean?


It means putting more things into files, and loading them into generic resource managers. For example, you could have a tileset file that looks something like this:

d someimage.png foo bar baz
b anotherimage.png foo bar baz
...

Each line opens up with the tile ID (I used d and b per your example), followed with information about that tile, such as the image used to draw it, whether or not that tile is walkable, how to draw it (like src x,y coords, whether or not it's flipped, that kind of thing), etc, etc. Everything the tile would need in order to operate.

When the tileset is loaded, you would load that info into objects of your Tile class:

// something like this
void Tile::Load(std::istream& file)
{
  while(!eof)
  {
    file >> IDchar;
    file >> ImageName;
    file >> foo;
    //...
    //  where IDchar, ImageName, foo, etc are all members of 'Tile'
  }
}

Loading the tileset is as simple as opening the file, loading each tile, and sticking all the tiles into a container.

Now since your tiles are recognized by a special character (d, b) instead of a 0-based index, a std::map for holding the tiles might be more appropriate:

// Tileset class has the below member:
//   std::map<char,Tile>  tileset;

void Tileset::Load(std::istream& file)
{
  tileset.clear();

  while(!eof)
  {
    Tile t;
    t.Load(file);            // load the tile
    tileset[ t.IDchar ] = t; // put it in our map, easily indexed by our ID char
  }
}

Then when you load the map, you can use the ID char from the map to index your tileset and get the Tile*:

// Map class has members:
//  array2d<Tile*> map;
//  Tileset theTileset;
//

for( ...each tile... )
{
  char t = __the_char_pulled_from_the_file__;

  map(x,y) = &theTileset[t];
}

See how it works? You could also store the name of the tileset in your map file so that you can load the appropriate tileset when you load the map, rather than having to hardcode those.

But notice that everything is nameless. There's no hardcoded data that has to be accessed by a specific name in your source. Everything is fluid and dynamic. This is less restrictive, easier to code (after you get the hang of it), and much easier to change maps around if you want to later, or add new tiles, etc.



PS: I normally don't make long posts like these in PMs. This info could be really useful to other people too. Would you object if I posted these PM exchanges on the forums so other people could benefit from it?

[Moarthenfeeling]

Back to the very old thread... How do I erase every element from std::map<char,Tile> tileset, calling each element destructor from Tile class? I really need some help :D

[ne555]

¿ tileset.clear() ?

[Moarthenfeeling]

Nope, it doesn't work
map::clear will delete pointers to the objects, as far as I know

[ne555]

std::map<char,Tile> tileset; ¿pointers?

¿Did you mean std::map<char,Tile*> tileset; ?

[Computergeek01]

Maybe you could use map::erase(...)? http://www.cplusplus.com/reference/stl/map/erase/ Then have a static member of your class to track the last element?

[Moarthenfeeling]

ne555
No, tileset is defined as std::map<char,Tile> tileset;, but I don't think, that tileset.clear() will call each element's destructor.(I've tried this already) And the other problem is that Tile destructor( Tile::~Tile()) is called twice when I add new elements to the std::map<char,Tile> tileset;

Computergeek01, I've tried this too. But another problem is written above

[Disch]

How do I erase every element from std::map<char,Tile> tileset, calling each element destructor from Tile class

ne555 is correct. tileset.clear() is what you want.

map::clear will delete pointers to the objects, as far as I know

Nope. clear does what you'd expect. It calls dtors for every element in the map and removes them so the map is empty.

And the other problem is that Tile destructor( Tile::~Tile()) is called twice when I add new elements to the std::map<char,Tile> tileset;

This is because copies of the tile may be made when you add the elements to the map. The destructors are being called on the copies, but the element that actually gets put in the map is not destroyed.

Your problems are probably being caused by your Tile class being improperly copied. Did you write an appropriate copy ctor and assignment operator for it? Can you post the class here?

[Moarthenfeeling]

No, I did not. I wouldn't copy all the code. There's just some functions, which work perfect and do not really deal with tilesets.
I have a destructor like this

Tile::~Tile()
{
	if(sprite!=NULL)
	delete sprite; 
}

sprite is just a pointer, defined as Sprite* sprite
And when this destructor is called it somehow will delete not only the copy's sprite, but object sprite too!

EDIT: Oh wait, I'm wrong. Everything works fine at this moment. The problem is there:
map(w,h).sprite=tileset[t].sprite; //'t' is char taken from .txt file
So I assign one pointer from tileset to the pointer of map tile. And here's what I receive when I create breakpoint just after assigment:
sprite from tileset: 0x01884ce8
sprite from map: 0x00000000 (Null-pointer? WTF?)

[Disch]

If an object owns/manages memory, then you need to supply your own copy ctor and assignment operator, otherwise your class is improperly encapsulated.

The assignment operator and copy ctor provided by the compiler do not "deep copy" the object pointed to, they only copy the pointer itself.

Consider this example:

class Foo  // an ill-formed class that needs a copy ctor and = operator
{
public:
  int* p;
  Foo() { p = new int; }
  ~Foo() { delete p; }
};

int main()
{
  {
    Foo a;
    {
      Foo b = a;  //b is a copy of a.  However, note that both a.p and b.p point to the same int!

      cout << a.p << '\n' << b.p << endl;  // as illustrated here
    } // b's dtor called here.  Resulting in the pointer being deleted

    // a.p is now a bad pointer!  it was deleted by b's dtor!

  } // a's dtor called, which attempts to delete a.p, but a.p has already been deleted!
   // program probably will explode here as a result
}

One solution would be to write a copy ctor and assignment operator so that the pointer is deep copied when the object is copied:

//  example of how to deep copy
//   however... I don't recommend this for your particular problem.  See below

class Foo
{
public:
  int* p;
  
  Foo() { p = new int; }
  ~Foo() { delete p; }

  Foo(const Foo& r) { p = new int(*r.p); }  // creates a new int
  Foo& operator = (const Foo& r) 
  {
    // copies the data pointed to, not the pointer itself
    *p = *r.p;  // not p = r.p
    return *this;
  }
};

However this solution is probably bad for your needs. Assuming 'sprite' is just a sf::Sprite, you should just have a sf::Sprite object in your class and not use a pointer at all.


Of course it would help if I could see the Tile class to be sure.



_{on a side note, you don't need to check for a null pointer before deleting something. The delete operator already does that for you -- deleting a null pointer does nothing}

[Moarthenfeeling]

I'm using HGE for my project http://hge.relishgames.com/

//Tile.h
#include <iostream>
#include "hge.h"
#include "hgesprite.h"
class Tile
{
public:
	Tile();
	~Tile();
	void SetHGE(HGE* hge); //pointer to HGE object is neccessary to perform some operations
	void Load(std::istream& file); //load information about tile
	void LoadSprites(HGE* phge); //load sprite from image

	char ground; //type of first layer
	char walk; //type of collision
	char obj; //texture overlay
	char IDchar; //char identifier used with tileset[t]
	std::string ImageName; 
	hgeSprite* sprite; //here it is
	hgeSprite* objsprite; //and this is object sprite
	HTEXTURE texture; 
	HGE *phge; //pointer to HGE object :>
};

//Tile.cpp
#include "Tile.h"
#include <string>
#define UNICODE 

Tile::Tile()
{
	ground=NULL;
	walk=NULL;
	obj=NULL;
	IDchar=NULL;
	sprite=NULL;
	objsprite=NULL;
	texture=NULL;
	phge=NULL;
}

Tile::~Tile()
{
    delete sprite; 
    delete objsprite; 
    phge->Texture_Free(texture); 
}
void Tile::SetHGE(HGE *hge)
{
	phge=hge;
}
void Tile::Load(std::istream& file)
{
	std::string temp;
	getline(file, temp,'\n'); //getting string
	IDchar=temp[0]; //first symbol of string is ID
	for(int i=2;i<temp.length();i++) 
		ImageName+=temp[i];
}
void Tile::LoadSprites(HGE *phge)
{
	texture=phge->Texture_Load(ImageName.c_str()); //loading texture
	if(!texture) //if texture is not loaded we show error message box
	{
		std::string error;
		error+="Error! Cannot load '"+ImageName+"'. File not found.";
		MessageBoxA(NULL, error.c_str(), "Error", MB_OK | MB_ICONERROR | MB_APPLMODAL);
		phge->System_Shutdown();
		phge->Release();		
	}
	sprite=new hgeSprite(texture, 0, 0, phge->Texture_GetWidth(texture), phge->Texture_GetHeight(texture));
}

So, it works fine, but only when I delete destructor

[Disch]

Yeah I was right. Your Tiles are not being copied correctly, so whenever a copy is destroyed, it deletes the sprite and you're left with a bad pointer.

I would say just get rid of the pointers and use objects, but HGE doesn't seem to be encapsulated very well, so you might still have problems (HTEXTURE isn't even an object, it's a handle -- yuk)

Even if you solve the sprite problem, you have the same problem with your texture.

Let this be a lesson to you: Encapsulation is important.


I would recommend the following changes:

1) Make sprite and objsprite objects and not pointers. Then you don't have to worry about memory management. Much simpler, and solves the 'deleting the same pointer twice' problem.

2) Don't have your Tile own the texture like that. Resources like textures, music files, sound effects, etc should be owned by a separate resource manager, that way multiple Tiles can share the same texture.

You can still use an HTEXTURE in your Tile class, but don't call Texture_Load or Texture_Free from it. Instead, you would get it from your resource manager.

Basically how a resource manager works is you have a class which owns all your HTEXTUREs, and hands them out to other parts of your program when needed. It would be in charge of loading/freeing the textures and all that.

I could probably come up with one for you if you don't quite get the idea. I kind of love this stuff.

[Moarthenfeeling]

Yeah, I get it now. So, my game is just a very early prototype, and it's my first "serious" game, ha-ha, so I just need to clearly understand some basic concept, I think. And it won't be to difficult, to change some really important parts of the code. Show me some code, please :)

I just don't get how do I realize rendering function.

At this moment it's like:

for(int h=0;h<map.width;h++)
     for(int w=0;w<map.width;w++)
          map(w,h).GetSprite()->Render(*some arguments there*); //so I get pointer to sprite very easy. 

How am I supposed to change it?

I kind of love this stuff.

Oh, that's awesome!

[Disch]

Show me some code, please :)

Alright. =P Here's a simple resource manager you can use for your textures.

#include <string>
#include <map>
#include <stdexcept>
#include <whatever_header(s)_define(s)_HTEXTURE_and_related_functions>


class TextureManager
{
private:
    typedef std::map<std::string,HTEXTURE>  map_t;
    map_t       textures;
    HGE*        mHGE;

public:
    // Getting the one and only instance of the TextureManageer
    inline static TextureManager& Get(HGE* hge = 0)
    {
        static TextureManager theobj(hge);
        return theobj;
    }

    // Getting a texture
    HTEXTURE Texture(const std::string& name)
    {
        // see if the texture is already loaded
        map_t::iterator i = textures.find(name);

        if(i != textures.end())     // if we found it
            return i->second;       //   just return the alread-loaded texture

        // otherwise, we need to load it
        TEXTURE newtexture = mHGE->Texture_Load(name.c_str());

        if(!newtexture) // failed to load
            throw std::runtime_error("Failed to load texture:  " + name);

        // now that it's loaded, put it in our map
        textures[name] = newtexture;

        // and give it back to the caller
        return newtexture;
    }

    // Erasing all textures
    void Clear()
    {
        for(map_t::iterator i = textures.begin(); i != textures.end(); ++i)
            mHGE->Texture_Free(i->second);

        textures.clear();
    }

private:
    // all ctors/dtors/etc are private to prevent object copying
    TextureManager(HGE* hge) : mHGE(hge) { }
    ~TextureManager() { Clear(); }

    TextureManager(const TextureManager&);
    void operator = (const TextureManager&);
};

Then when you want a specific texture you just do this:

HTEXTURE whatever = TextureManager::Get().Texture("filename.png");

No need to worry about cleaning up textures or anything. The manager will take care of all of that. Also, if multiple objects need to use the same image, they can all share the same image instead of having to load it separately for each one.


The one thing that's weird here is that you need to initialize this because of that blasted HGE pointer (this lib you're using isn't encapsulated very well =x). To get around this, you'll need to call Get once when you first create your HGE object:

HGE* hge = however_you_create_your_HGE_pointer();
TextureManager::Get(hge); // just do this to initialize it 

Note you have to do this before to attempt to use the manager in any other way.

At this moment it's like:
...
How am I supposed to change it?

Well I would have the tile draw itself:

map(w,h).Draw( ... );

Give your Tile class a Draw member that draws itself.

[Moarthenfeeling]

Wow, thanks, everything is clear with this code!
And if I need SpriteManager it'll look the same?(with some changes, of course)

[m4ster r0shi]

Mmmm... Cool stuff...

[Disch]

m4ster r0shi pointed out a very critical bug in my code. I've edited to correct it, so please be sure you use the new code.

(I forgot to make the singleton object static -- big mistake!)
( line 18 was changed )