Jump to content

  • Log In with Google      Sign In   
  • Create Account

Deriving but keeping data


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
18 replies to this topic

#1 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 20 August 2012 - 05:15 AM

Hello!

About a week ago i posted on this forum asking for help with a pointer problem. People were very helpful and gave me loads of tips to read up on.

So this week i've been reading like crazy and i am ready to basically re-write my project. I still doesn't know how to solve one thing thought.

At the moment i have a loader class which basically loads in images, fonts and eventually sounds. Every class is derived from this one. But as someone pointed out in the previous thread that must mean that all the files are loaded several times since the actual loading takes place in the constructor.

So for every class that's being made it must contact that constructor again and re-load everything.

I've gotten some tips from my dad and one was to make a static function and variable of the class to make sure it only loads once. He showed me an example of how to make it work. I'm just wondering how you guys tackle this problem and if static is the way to go or if there are several ways to do this!

Thank in advance!

Sponsor:

#2 Álvaro   Crossbones+   -  Reputation: 13914

Like
2Likes
Like

Posted 20 August 2012 - 06:05 AM

`static' is rarely the way to go. I don't quite understand your situation, but "Every class is derived from this one" sounds like a horrible design in pretty much any circumstance.

I don't know what other thread you are talking about. You should make this one self contained, if at all possible. What exactly is your design?

#3 Bregma   Crossbones+   -  Reputation: 5469

Like
1Likes
Like

Posted 20 August 2012 - 06:09 AM

You may need to separate you classes more. One class is for containing the loaded data, a separate class for instances of that data. Objects of the 'instance class' contain state about the individual instances (say, position and life points) and a pointer to the loaded data that contains data such as bitmaps.
Stephen M. Webb
Professional Free Software Developer

#4 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 20 August 2012 - 06:21 AM

I am separating my classes, that is why i'm redesigning the program.

But image this. The class loads an image for a grass tile as well as the player model.

Both classes needs to be able to access their variable(image). But both the tile and player class is already derived from other classes.

How do i get them (without sending the class Loader to their class via a function) to access the same class and have access to draw out images from the same selection of variables.

I basically need the loader function to only exist in one copy, so that i never recreates itself.

I know i could solve this by sending the adress via a pointer to all the classes i create. But it just seems like a hassle if there is a simpler way.

Edited by Tallkotten, 20 August 2012 - 06:23 AM.


#5 SiCrane   Moderators   -  Reputation: 9670

Like
0Likes
Like

Posted 20 August 2012 - 07:55 AM

You might want to post some code that demonstrates exactly what you're doing, both with and without the static.

#6 Bregma   Crossbones+   -  Reputation: 5469

Like
0Likes
Like

Posted 20 August 2012 - 08:20 AM

I basically need the loader function to only exist in one copy, so that i never recreates itself.

If your loader needs state, or provides a virtual interface, you will need to pass it to your object constructors, because they need it. As simple as possible, but no simpler.

If your loader does not need to track its own state, or if it consists of a single static interface, you have yourself a factory function. It does not need to be a member of a class, it's better to have it as a namespace-level function.

It sounds to me (and I'm just guessing) that you want to pass a factory object (a "builder") to your object constructors. The factory object caches the loaded tiles and doles out a pointer to the loaded tile to each instance that asks for it.

Perhaps something like this.
[source lang="cpp"]class Tile { /* ... */ };class TileFactory{public: Tile* get_tile(std::string const& tile_name) { if (cache_.find(tile_name) == cache_.end()) { cache_[tile_name] = load_tile(tile_name); } return cache_[tile_name]; }private: std::map<std::string, Tile> cache_;};class ObjectBase{public: ObjectBase(TileFactory& tile_factory, std::string const& tile_name) : tile_(tile_factory(tile_name)) { } virtual ~ObjectBase() { }protected: Tile* tile_;};class BackgroundObject: public ObjectBase{public: BackgroundObject(TileFactory& tile_factory, std::string const& tile_name) : ObjectBase(tile_factory, tile_name) { }};int main(){ TileFactory tile_factory; BackgroundObject grass(tile_factory, "grass"); BackgroundObject house(tile_factory, "house");}[/source]

If you need a whole clot of such factories, you can combine them into a single struct and pass that through, too.
Stephen M. Webb
Professional Free Software Developer

#7 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 20 August 2012 - 08:47 AM

You might want to post some code that demonstrates exactly what you're doing, both with and without the static.


Right now i don't have any code except a class with variables that gets initiated when the constructor runs.

I am asking this since i need help creating the class i need.
But here is some code for example:

Imagine that these classes are located in different files as well!

Class Loader
{
public:
	 Loader()
	 {
		  SDL_Surface* tile = IMG_Load("tile.png"); //img_load is my own func
		  SDL_Surface* player = IMG_Load("player.png");
	 }
	 SDL_Surface* tile;
	 SDL_Surface* player;
}


Class Tile: public Loader
{
public:
	 Tile();
	 draw(SDL_Surface* screen)
	 {
		  //draw out the image tile from Loader
	 }
}


Class Player: public Loader
{
public:
	 Player();
	 draw(SDL_Surface* screen)
	 {
		  //draw out the image tile from Loader
	 }
}


main
{
	 init()
	 {
	 	 Loader loader;
	 	 Tile tile;
	 	 Player player;
	 }
	 draw
	 {
		  tile.draw();
		  player.draw();
	 }

	 init();
	 draw();
}

In that case isn't tile and player variables inside Loader being created twice. Once for class Tile and once for class Player?

Edited by Tallkotten, 20 August 2012 - 08:54 AM.


#8 Álvaro   Crossbones+   -  Reputation: 13914

Like
0Likes
Like

Posted 20 August 2012 - 08:56 AM

Class Player: public Loader

I assume you meant `class', and not `Class'. But besides that, I think that line is the essence of your problem. To me that line reads "Player is a Loader". It means that anywhere I would you a Loader, I could use a Player instead, because Player is a type of Loader. If that doesn't sound right to you, neither should the line of code I quoted.

IMHO, people get exposed to inheritance too early when they are learning programming, so they think it's the way you do everything. You probably don't need inheritance at all in this situation. If a Tile and a Player need access to a Loader for some reason (although I wouldn't do it that way either), they should each have a member function that is a pointer to the Loader, and this should be set in the constructor.

#9 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 20 August 2012 - 09:02 AM

Class Player: public Loader

I assume you meant `class', and not `Class'. But besides that, I think that line is the essence of your problem. To me that line reads "Player is a Loader". It means that anywhere I would you a Loader, I could use a Player instead, because Player is a type of Loader. If that doesn't sound right to you, neither should the line of code I quoted.

IMHO, people get exposed to inheritance too early when they are learning programming, so they think it's the way you do everything. You probably don't need inheritance at all in this situation. If a Tile and a Player need access to a Loader for some reason (although I wouldn't do it that way either), they should each have a member function that is a pointer to the Loader, and this should be set in the constructor.


Yeah, i mane class.

That's my old code. Like i said i am re-writing it all now. Had inheritage on way to many places. So i'm currently in a process of fixing LOADS of newbi errors
Ok, but is it any other way to solve this than so send it to the constructor? or i that the way to go?

Edited by Tallkotten, 20 August 2012 - 09:03 AM.


#10 rip-off   Moderators   -  Reputation: 8741

Like
1Likes
Like

Posted 20 August 2012 - 09:16 AM

What about something like this:
type Loader
{
    def Image load(string name) { /* ... */ }


    private Cache<Image> cache;
}

type Tile
{
    def Tile(Loader loader)
    {
	    image = loader.load("tile");
    }


    def draw(Screen screen)
    {
	    screen.draw(image);
    }
   
    private Image image;
}

type Player
{
    def Player(Loader loader)
    {
	    image = loader.load("player");
    }


    def draw(Screen screen)
    {
	    screen.draw(image);
    }
   
    private Image image;
}

def main()
{
    Loader loader = new Loader();
    Tile tile = new Tile(loader);
    Player player = new Player(loader);


    // ...

    tile.draw(screen);
    player.draw(screen);
}


#11 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 20 August 2012 - 09:35 AM

What about something like this:

type Loader
{
	def Image load(string name) { /* ... */ }


	private Cache<Image> cache;
}

type Tile
{
	def Tile(Loader loader)
	{
		image = loader.load("tile");
	}


	def draw(Screen screen)
	{
		screen.draw(image);
	}
  
	private Image image;
}

type Player
{
	def Player(Loader loader)
	{
		image = loader.load("player");
	}


	def draw(Screen screen)
	{
		screen.draw(image);
	}
  
	private Image image;
}

def main()
{
	Loader loader = new Loader();
	Tile tile = new Tile(loader);
	Player player = new Player(loader);


	// ...

	tile.draw(screen);
	player.draw(screen);
}


Yeah i guess i'll have to go for something like that. Maybe smarter to use pointers thought. Since i want the image to change during the game.

Why is it bad to use a static function?
This is the simple example my dad drew up for me (makes me sound really young, I'm 19 if anybody cares)
class Loader
{

     private static Loader myLoader;
     public static &Loader files(); //loads all the files, so this is called when myLoader is created 
     Loader()
	 {
          if(myLoader == NULL)
          {
               myLoader = new Loader();
               myLoader.files();
               return myLoader;
          }
      } 
}

Nowadays he codes in c# so something might be wrong in the code and i haven't executed it to look for errors, but you get the idea.

#12 ndssia   Members   -  Reputation: 172

Like
0Likes
Like

Posted 21 August 2012 - 06:29 AM

return myLoader should be outside that IF I think; and it looks like the Singleton pattern.

It does work, but can cause complications in testing and multithreading in some cases.

#13 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 21 August 2012 - 08:12 AM

return myLoader should be outside that IF I think; and it looks like the Singleton pattern.

It does work, but can cause complications in testing and multithreading in some cases.


So you'd rather recommend me assigning a pointer to the "loader" upon creation of the object?

#14 BeerNutts   Crossbones+   -  Reputation: 3007

Like
1Likes
Like

Posted 21 August 2012 - 12:23 PM

IMO, the easiest way to solve this is to don't even worry about classes. You can use basic C-style functions to load your images and return the pointers to them in a generic Utility.cpp file, like this:

// in Utility.h define this
SDL_Surface GetImage(std::string imageName);

// in Utility.cpp, do this
std::map<std::string, SDL_Surface*> ImageMap;

SDL_Surface *GetImage(std::string imageName)
{
  // check if this image is in the map
  if (ImageMap.find(imageName) == std::map::end) {
	// load it once into the map
	ImageMap[imageName] = IMG_Load(imageName);
  }
  return ImageMap[imageName];
}

// Then, when you want to load an image, #include "Utility.h" and call
SDL_Surface *PlayerImage = GetImage("player.png");
You can put Utility into a Utility namespace if you want, but that's just another option.
Good Luck.

Edited by BeerNutts, 21 August 2012 - 12:25 PM.

My Gamedev Journal: 2D Game Making, the Easy Way

---(Old Blog, still has good info): 2dGameMaking
-----
"No one ever posts on that message board; it's too crowded." - Yoga Berra (sorta)

#15 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 21 August 2012 - 01:18 PM

IMO, the easiest way to solve this is to don't even worry about classes. You can use basic C-style functions to load your images and return the pointers to them in a generic Utility.cpp file, like this:


// in Utility.h define this
SDL_Surface GetImage(std::string imageName);

// in Utility.cpp, do this
std::map<std::string, SDL_Surface*> ImageMap;

SDL_Surface *GetImage(std::string imageName)
{
  // check if this image is in the map
  if (ImageMap.find(imageName) == std::map::end) {
	// load it once into the map
	ImageMap[imageName] = IMG_Load(imageName);
  }
  return ImageMap[imageName];
}

// Then, when you want to load an image, #include "Utility.h" and call
SDL_Surface *PlayerImage = GetImage("player.png");
You can put Utility into a Utility namespace if you want, but that's just another option.
Good Luck.


Thanks! Will that code only run once? No matter how many times i include the file?

#16 rip-off   Moderators   -  Reputation: 8741

Like
1Likes
Like

Posted 21 August 2012 - 01:39 PM

No code runs when you include a file. C++ is compiled, which means that there is a tool which translates your code into something that will execute.

At runtime, the image will be loaded only once. This is because the first time a file name is referenced, the ImageMap has no entry with that name. Thus the test to determine if the entry is in the map will fail, and the image is then loaded and inserted into the map. Subsequent times, the ImageMap will have an entry with the name, so the existence test will pass and the early image will be returned.

Of course, care now needs to be taken of these shared surfaces. It would be incorrect to call SDL_FreeSurface() until you know that all other references to this surface are out of scope.

#17 BeerNutts   Crossbones+   -  Reputation: 3007

Like
0Likes
Like

Posted 21 August 2012 - 02:01 PM

Of course, care now needs to be taken of these shared surfaces. It would be incorrect to call SDL_FreeSurface() until you know that all other references to this surface are out of scope.


This is true. In general, the example I gave, you would only call a FreeAllImages() (which loops through the ImageMap and call SDL_FreeSurface()) either at the end of the program, or possibly when changing levels and you know no-one is holding onto old images.
My Gamedev Journal: 2D Game Making, the Easy Way

---(Old Blog, still has good info): 2dGameMaking
-----
"No one ever posts on that message board; it's too crowded." - Yoga Berra (sorta)

#18 rip-off   Moderators   -  Reputation: 8741

Like
1Likes
Like

Posted 21 August 2012 - 02:11 PM

To follow on from my last point, we need to control the lifetime of the objects in the map. This is especially important because the map is global, so it will be destroyed after main() ends. If you call SDL_Quit() during main, it is undefined whether you can safely call SDL_FreeSurface() on the pointers in the map.

This is one good reason to not use a global for this.

If we ignore that aspect for the moment, one way to achieve the cleaning up of the surfaces is to use smart pointers. Smart pointers try to automatically clean up after they are done. They are fairly clever, but you can confuse them in certain circumstances that won't arise in the context of this (if you're curious, they don't work with cyclical references).

If your compiler supports it, you can use the standard smart pointers std::shared_ptr or std::weak_ptr in the map. If your compiler doesn't support these new classes, consider upgrading! If you cannot or will not upgrade, the boost libraries can provide these classes for you.

Shared pointer is a really nice class that manages shared ownership of an interesting object. When the last owner object that needs an object is finished using it, the shared pointer will cleanup the owned object. The clients would store a shared_ptr. You can specify SDL_FreeSurface() as the "cleanup" function when you create the smart pointer. This is important, because shared_ptr defaults to trying to delete the object which was passed, which is incorrect for objects that were not allocated with new.

Something like this:
#include <map>
#include <string>
#include <memory>

#include "SDL.h"
#include "SDL_image.h"

typedef std::shared_ptr<SDL_Surface> SurfacePtr;
typedef std::map<std::string, SurfacePtr> SurfaceCache;
SurfaceCache surfaceCache;

SurfacePtr load(const std::string &name)
{
	SurfaceCache::iterator i = surfaceCache.find(name);
	if(i != surfaceCache.end())
	{
		return i->second;
	}

	SDL_Surface *loaded = IMG_Load(name.c_str());
	if(loaded == NULL)
	{
		// TODO: handle this here?
	}
	SurfacePtr surface(loaded, &SDL_FreeSurface);
	surfaceCache.insert(std::make_pair(name, surface));
	return surface;
}
Note this code shows a reasonably idiomatic search for and add entries to a std::map. It avoids extra, unnecessary lookups in the best case. In constrast, BeerNutts's simpler code performs at least two, and worst case three searches for an element.

But we are still left with the situation I mentioned earlier, where the map is destroyed after main. In general, running complicated code like constructors and destructors outside main isn't a good idea. You cannot depend on exactly when these functions will be called relative to the rest of your program. If you aren't careful, it is easy to trigger undefined behaviour.

One solution is to manage the lifetime of the global in main(). main() creates the global and exports it in a controlled manner. Something like the following:

// image.h
#ifndef IMAGE_H
#define IMAGE_H

#include <map>
#include <string>
#include <memory>

#include "SDL.h"
#include "SDL_image.h"

typedef std::shared_ptr<SDL_Surface> SurfacePtr;
typedef std::map<std::string, SurfacePtr> SurfaceCache;

SurfacePtr load(const std::string &name);

#endif

// image.cpp
#include "image.h"

SurfaceCache &globalSurfaceCache();

SurfacePtr load(const std::string &name)
{
	SurfaceCache &surfaceCache = globalSurfaceCache();

	SurfaceCache::iterator i = surfaceCache.find(name);
	if(i != surfaceCache.end())
	{
		return i->second;
	}

	SDL_Surface *loaded = IMG_Load(name.c_str());
	if(loaded == NULL)
	{
		// TODO: handle this here?
	}
	SurfacePtr surface(loaded, &SDL_FreeSurface);
	surfaceCache.insert(std::make_pair(name, surface));
	return surface;
}

main.cpp
#include <iostream>
#include <cstdlib>

#include "image.h"

namespace {
	static SurfaceCache *hiddenSurfaceCache = 0;
}

SurfaceCache &globalSurfaceCache() {
	return *hiddenSurfaceCache;
}

int main(int, char**)
{
	if(SDL_Init(SDL_INIT_VIDEO) < 0)
	{
		std::cerr << "Failed to initialise SDL: " << SDL_GetError() << '\n';
		return 1;
	}

	std::atexit(&SDL_Quit);

	SDL_Surface *screen = SDL_SetVideoMode(800, 600, 0, SDL_SWSURFACE);
	if(!screen)
	{
		std::cerr << "Failed to set video mode: " << SDL_GetError() << '\n';
		return 1;
	}

	SurfaceCache surfaceCache;
	hiddenSurfaceCache = &surfaceCache;

	bool running = true;
	while(running)
	{
		// ...
	}
	hiddenSurfaceCache = 0;
	return 0;
}
This offsets most of the damage of the global. The lifecycle of the cache is automatically controlled in main(), and the pointer is nullified before it is invalidated (by falling off the end of main). I've also hidden the declaration of globalSurfaceCache() inside image.cpp, so client code will not directly interact with it.

#19 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 22 August 2012 - 01:49 AM

Thanks for the answers!!

About the lifetime. The images I'm loading are sheets (meaning there are several images per file i load). This enables me to load one file for all the different tiles instead of loading everyone individually.

I am planning on having them sorted kind of like this. inhouseTilesSheet which contains all the tiles to use in houses. Then I'll have the game check if there is a need for them to be loaded, if not they don't. So if i am just out and wandering I've only got loaded what that scene has in it. Hope i make myself clear, just woke up.
Going to have to re-read your response to fully grasp it later ;)




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS