Thread: Generic Resource_Manager WIP with lots TODO

Just posting this for a peer review. Largely incomplete..

Resource_Manager.h

Code:

#pragma warning(disable: 4786)

#include <vector>
#include "MS3D.H"
#include <map>

class Resource_Cache // The "containers" and their data structures
{
friend class Resource_Manager;
public:
	struct	MS3DModelRenderData
	{
		float Position[3]; // will be changed to vectors eventually
		float Orientation[3];
		MS3DModel *pModel;
	};

	std::vector<MS3DModelRenderData*> MS3DModels; // holds pointers to all MS3D Models
private:

	bool Add_MS3DModel_To_Cache(std::string Filename); // loads a milkshape model to the cache

};

template < typename resource_t >
class Manager					// container managers
{

    std::map< std::string , resource_t* > Resources;

public:

    resource_t & Load( const std::string & filename );


	virtual ~Manager( void )
	{
		std::map< std::string ,  resource_t * >::iterator destroyer, end;
		for ( destroyer = Resources.begin() , end = Resources.end() ; destroyer != end ; ++destroyer ) 
		{
			delete destroyer->second;
		}
	}	

};

class Resource_Manager	// Global Manager
{
	Resource_Cache GlobalResources;
    Manager<MS3DModel> Models;

	std::string get_extension(std::string Filename);

public:
	void Add_Resource(std::string Filename)
	{
		
		if (get_extension (Filename ) == "ms3d")
		{
			GlobalResources.Add_MS3DModel_To_Cache(Filename);
		}
		
	}

};

Resource_Manager.cpp

Code:

#include "Resource_Manager.h"
#include <windows.h>

////////////////////////////////////
// Resource Cache //////////////////
////////////////////////////////////
bool Resource_Cache::Add_MS3DModel_To_Cache(std::string Filename)
{
	MS3DModelRenderData *Model;
	Model = new MS3DModelRenderData;
	Model->pModel = new MS3DModel;

	if ( Model->pModel->Load_MS3D_Model( Filename ) == false )
	{
		MessageBox( NULL, "Couldn't load the model data.", "Error", MB_OK | MB_ICONERROR );
		return 0;									// If Model Didn't Load, Quit
	}

	 MS3DModels.push_back(Model); 

	return 1;
}
///////////////////////////////////////////////
///////////////////////////////////////////////
///////////////////////////////////////////////

///////////////////////////////////////////////
// Manager Template ///////////////////////////
///////////////////////////////////////////////
template < typename resource_t >
resource_t & Manager<resource_t>::Load( const std::string & filename ) 
{
	std::map< std::string , resource_t* >::iterator entry = Resources.find( filename );
    if ( entry != Resources.end() ) // if the entry is found
	{ 
		return * entry->second; // return the reference to the resource
    } 
	else
	{
		resource_t* resource( new resource_t( filename ) );
		
		// first we gotta load the resource, then we can make a pair
		// and return the reference
	
		resources.insert( std::make_pair( filename , resource ) );
		return * resource;
    }
}
///////////////////////////////////////////////
///////////////////////////////////////////////
///////////////////////////////////////////////


///////////////////////////////////////////////
// Resource Manager ///////////////////////////
///////////////////////////////////////////////
std::string Resource_Manager::get_extension(std::string Filename)
{
	std::string::size_type last_dot_index = Filename.rfind( '.' );
	std::string extension = Filename.substr( last_dot_index + 1 ); //+1 to not include the dot...
	return extension;
}
///////////////////////////////////////////////
///////////////////////////////////////////////
///////////////////////////////////////////////

Thats about it so far, pretty simple...

TODO list includes:

Make it so the Add_Resource function uses the managers to compose a list of already loaded resources and their references.

Add more functionality to the Load functions in Resource_Cache to return a reference to a resource, so that you can properly pair up a filename with its cooresponding resource


Hmm, that is all I can think of...

EXPLANATION:

Well, Resource_Cache holds containers of information, right now currently only for MS3D Models. The Manager template manages the I/O of the Resource_Cache, the Resource_Manager class decides which manager template gets utilized (one for each type of resource reference)...

a few comments

• you're passing your strings by value, as discussed in other thread
• instead of
  
  Code:

  std::vector<MS3DModelRenderData*> MS3DModels;

  consider using a boost.ptr_vector to make memory management easier
• I would typedef
  
  Code:

  std::map< std::string , resource_t* > Resources;

  to
  Code:

   typedef typename std::map< std::string , resource_t* > ResourceMap;

  this makes declarations like
  
  Code:

  std::map< std::string ,  resource_t * >::iterator

  much easier to read.
  
  Code:

  ResourceMap::iterator

that's a very brief glance

Thanks for the review..

Question, Do you think this is a logically correct way to go about doing this?

I'm having trouble figuring out the order of operations for a Global Add_to_Cache function...


Pseudocode

Code:

void Add_Resource(std::string & filename)
{
   if (get_extension( filename ) == "ms3d")
      A. use template to check for already existing file
     A1. If exists, pushback a reference to the resource into the vector // done
     A2. If doesn't exist, proceed to load file, return the reference
     B2. Use template to make the filename and the reference a pair to prevent future reloading
};

This would suggest I can't have one Load template function...

I need a check_existence function that performs a return reference function, and another function that takes a filename and a reference and makes a pair out of the two..

I can't do steps A and B2 in the same function, I don't think...

Any suggestions?

Is this going to be a Windows resource manager? If so, it sounds like a cool idea.

Well, it is ment to be an internal system for a game really...

I plan on adding support for .ms3d, .mp3's, maybe a few others, really only things pertaining to the game..

I would typedef

Code:
std::map< std::string , resource_t* > Resources;
to
Code:
typedef typename std::map< std::string , resource_t* > ResourceMap;
this makes declarations like

Code:
std::map< std::string , resource_t * >::iteratormuch easier to read.

Code:
ResourceMap::iterator

I see no reason to do this. In fact it may confuse you later. I'm not a huge fan of typedefs like that because it obfuscates the code and makes the huge game engine you are coding that much more difficult to read since you gotta flip back and forth between the header and the cpp using the typedef. Personal opinion but I stay away from a lot of that stuff as much as possible.

I like how you have wrapped the manager into a template class since most of the managers do act the same it makes sense to make a template class and then set the data type later. I may in fact use this design in my own system. It sure makes more sense to write the manager code once and use it many times, than write a manager class for every object in the game. I like it.

Thanks very much for the response Bubba..

The main thing I'm concerned about is how I'm dealing with references and pointers, I'm really worried theres holes and hanging pointers lieing around...

Can anyone spot any problems with the pointers and such?

And finally, Proof of concept, completed version... currently only works with .ms3d files..

Resource_Manager.h

Code:

#pragma warning(disable: 4786)

#include <vector>
#include "MS3D.H"
#include <map>

class Resource_Cache
{
friend class Resource_Manager;
public:
	struct	MS3DModelRenderData
	{
		float Position[3]; // will be changed to vectors eventually
		float Orientation[3];
		MS3DModel *pModel;
	};

	std::vector<MS3DModelRenderData*> MS3DModels; // holds pointers to all MS3D Models
private:

	MS3DModel * Add_MS3DModel_To_Cache(std::string Filename); // loads a milkshape model to the cache

};

template < typename resource_t >
class Manager
{

    std::map< std::string , resource_t* > Resources;

public:

	bool Check_Existance( const std::string & filename );
    resource_t * Return_Resource_Reference( const std::string & filename );
	void Add_To_List( const std::string & Filename, resource_t* );

	virtual ~Manager( void )
	{
		std::map< std::string ,  resource_t * >::iterator destroyer, end;
		for ( destroyer = Resources.begin() , end = Resources.end() ; destroyer != end ; ++destroyer ) 
		{
			delete destroyer->second;
		}
	}	

};

class Resource_Manager
{
	Resource_Cache GlobalResources;
    Manager<MS3DModel> Models;

	std::string get_extension(std::string Filename);

public:

	void Add_Resource(std::string Filename);


};

Resource_Manager.cpp

Code:

#include "Resource_Manager.h"
#include <windows.h>
#include <iostream>
#include "assert.h"
////////////////////////////////////
// Resource Cache //////////////////
////////////////////////////////////
MS3DModel * Resource_Cache::Add_MS3DModel_To_Cache(std::string Filename)
{
	MS3DModelRenderData *Model;
	Model = new MS3DModelRenderData;
	Model->pModel = new MS3DModel;

	if ( Model->pModel->Load_MS3D_Model( Filename ) == false )
	{
		MessageBox( NULL, "Couldn't load the model data.", "Error", MB_OK | MB_ICONERROR );
		assert(Model->pModel != NULL);
		return 0;
	}

	 MS3DModels.push_back(Model); 

	return Model->pModel;
}
///////////////////////////////////////////////
///////////////////////////////////////////////
///////////////////////////////////////////////

///////////////////////////////////////////////
// Manager Template ///////////////////////////
///////////////////////////////////////////////
template < typename resource_t >
resource_t * Manager<resource_t>::Return_Resource_Reference(const std::string & filename ) 
{
	std::map< std::string , resource_t* >::iterator entry = Resources.find( filename );
    if ( entry != Resources.end() ) // if the entry is found
	{ 
		return entry->second; // return the reference to the resource
    } 
/*
	else
	{

		resource_t* resource( new resource_t( filename ) );
		
		// first we gotta load the resource, then we can make a pair
		// and return the reference
	
		Resources.insert( std::make_pair( filename , resource ) );
		return * resource;
    }
*/
	else return false;
}

template < typename resource_t >
bool Manager<resource_t>::Check_Existance( const std::string & filename )
{
	std::map< std::string , resource_t* >::iterator entry = Resources.find( filename );
    if ( entry != Resources.end() ) // if the entry is found
	{ 
		return true;
    } 
	else return false;
}

template < typename resource_t >
void Manager<resource_t>::Add_To_List(const std::string & Filename, resource_t * Resource)
{
	Resources.insert(std::make_pair( Filename , Resource) );
}
///////////////////////////////////////////////
///////////////////////////////////////////////
///////////////////////////////////////////////



///////////////////////////////////////////////
// Resource Manager ///////////////////////////
///////////////////////////////////////////////
std::string Resource_Manager::get_extension(std::string Filename)
{
	std::string::size_type last_dot_index = Filename.rfind( '.' );
	std::string extension = Filename.substr( last_dot_index + 1 ); //+1 to not include the dot...
	return extension;
}

void Resource_Manager::Add_Resource(std::string Filename)
{
		
	if (get_extension (Filename ) == "ms3d")
	{
		if ( Models.Check_Existance (Filename) == true  )
		{
			std::cout << "Resource is on the list!" << std::endl;
			Resource_Cache::MS3DModelRenderData * RenderObject = new Resource_Cache::MS3DModelRenderData;
			RenderObject->pModel = Models.Return_Resource_Reference ( Filename );
			GlobalResources.MS3DModels.push_back(RenderObject);
		}
		else
		{
			std::cout << "Resource not on list!" << std::endl;
			
			Models.Add_To_List(Filename, GlobalResources.Add_MS3DModel_To_Cache(Filename));
		}
	}
}		
///////////////////////////////////////////////
///////////////////////////////////////////////
///////////////////////////////////////////////

And the test.cpp

Code:

#include "Resource_Manager.h"

Resource_Manager Global_Manager;
int main()
{
	Global_Manager.Add_Resource("AttackShip.ms3d");
	Global_Manager.Add_Resource("AttackShip.ms3d");
	return 0;
}

And finally, the output.. Very simple, telling us it wasn't in the list before, therefore it loads the file, then the second line of output is telling us the file is now on the list with a proper resource, and then just returns the cooresponding resource pointer...

Code:

Resource not on list!
Resource is on the list!
Press any key to continue

Originally Posted by Bubba

I see no reason to do this. In fact it may confuse you later. I'm not a huge fan of typedefs like that because it obfuscates the code and makes the huge game engine you are coding that much more difficult to read since you gotta flip back and forth between the header and the cpp using the typedef. Personal opinion but I stay away from a lot of that stuff as much as possible.

what happens when you want to change from a map to a multimap (or some other associative container)? In my version you change the typedef and everything else works. In yours, you have to change every instance of the map, not to mention every iterator, const_iterator, value_type, etc.

Code in the future tense.

and if you don't like typedefs I presume you always use

Code:

std::basic_string<char, char_traits<char>, allocator<char> >

instead of std::string?

you need to know the API of any class you use anyway, so "flipping back and forth between the header and the cpp" isn't really anymore of an issue with a typedef then not.

I've grown to love the Singleton pattern, even if its commonly known overusing it is bad. I think the information that a class is a singleton is of no concern to that class, so should be kept seperate (like in a System or Manager or through Boost).

I modified your code a bit to match my system a little, and attempted to clean it up to what I think would be logical (and as compulsion I've changed a lot of formatting). It doesn't mean its right, and I concertainly wouldn't expect it to be since I had some troubles making out the entire system, but maybe it'll give you a different perspective - who knows.

Code:

template< typename tnResource >
class ResourceContainer
{
public:
	bool CheckExists(const std::string&);
	tnResource* Find(const std::string&);
	void Add(const std::string&, tnResource*);
	void Remove(const std::string&, tnResource*) {}

	virtual ~Manager(void)
	{
		std::map< std::string, tnResource* >::const_iterator I;
		for(I = m_Resources.begin(); I != m_Resources.end(); ++I) 
		{
			delete I->second;
		}
	}

private:
	std::map< std::string, tnResource* > m_Resources;
};

template< typename tnResource >
tnResource* ResourceContainer< tnResource >::Find(const std::string& p_sFilename) 
{
	std::map< std::string, tnResource* >::const_iterator entry = m_Resources.find(p_sFilename);

	if(entry != Resources.end()) // if the entry is found
		return entry->second; // return the reference to the resource
	else
		return false;
}

template< typename tnResource >
bool ResourceContainer<tnResource>::CheckExists(const std::string& p_sFilename)
{
	std::map< std::string, tnResource* >::const_iterator entry = m_Resources.find(p_sFilename);

	if(entry != m_Resources.end()) // if the entry is found
		return true;
	else
		return false;
}

template< typename tnResource >
void ResourceContainer< tnResource >::Add(const std::string& p_sFilename, tnResource* Resource)
{
	m_Resources.insert(std::make_pair(p_sFilename, Resource));
}

class Manager // singleton god
{
public:

	ResourceManager* Resources()
	{
		if(!m_ResourceManager)
			m_ResourceManager = new ResourceManager();

		return m_ResourceManager;
	}

	D3DTextureHandler* Textures()
	{
		if(!m_D3DTextureHandler)
			m_D3DTextureHandler = new D3DTextureHandler();

		return m_D3DTextureHandler;
	}

	Manager* Instance()
	{
		if(!m_Instance)
			m_Instance = new Manager();

		return m_Instance;
	}

protected:
	Manager()
	{

	}

private:
	Manager* m_Instance;

	ResourceManager* m_ResourceManager;
	D3DTextureHandler* m_D3DTextureHandler;
};

class ResourceManager
{
public:
	void Add(std::string p_sFilename)
	{
		switch(GetExtension(p_sFilename).c_string())
		{
			case "ms3d": m_MS3DModelManager->Create(p_sFilename);
			break;
			case "ai": //m_AIMotionManager->Create(p_sFilename);
			break;
			default:
			break;
		}
	}

	void Remove(std::string p_sFilename)
	{

	}

	std::string GetExtension(std::string p_sFilename)
	{
		return p_sFilename.substr(p_sFilename.rfind('.') + 1); //+1 to not include the dot...
	}

	MS3DModelManager* MS3DModels()
	{
		if(!m_MS3DModelManager)
			m_MS3DModelManager = new MS3DModelManager();

		return m_MS3DModelManager;
	}

	AIMotionManager* AIMotions()
	{
		if(!m_AIMotionManager)
			m_AIMotionManager = new AIMotionManager();

		return m_AIMotionManager;
	}

private:
	MS3DModelManager* m_MS3DModelManager;
	AIMotionManager* m_AIMotionManager;
};

class MS3DModelManager
{
	MS3DModel* Create(std::string p_sFilename)
	{
		MS3DModelRenderData* ModelData = new MS3DModelRenderData;
		ModelData->Model = new MS3DModel;

		m_Models.push_back(ModelData);

		if(m_Resources.CheckExists(p_sFilename) == false)
		{
			std::cout << "Resource was not on the list!" << std::endl;
			
			m_Resources.Add(p_sFilename, ModelData->Model);  
		}

		return ModelData->Model;
	}

	void Destroy(std::string p_sFilename)
	{

	}

private:
	struct MS3DModelRenderData
	{
		float Position[3]; // will be changed to vectors eventually
		float Orientation[3];
		MS3DModel *Model;
	};

	std::vector< MS3DModelRenderData* > m_Models; // holds pointers to all MS3D Models
	ResourceContainer< MS3DModel > m_Resources;
};

int main()
{
	Manager::Instance()->Resources()->Add("AttackShip.ms3d");
	Manager::Instance()->Resources()->Add("AttackShip.ms3d");
}

I believe I'm about to take a new approach in this system...

I'm going to remove the renderlist (the list of MS3D Models) from the manager, and "outsource" that to my rendering engine...

Since various engines will be calling the resource creation and pulling functions - we already know what kind of manager we need to use, no more type switching.

There will be a handle, and a Manager, both templates.

The handle is essentially a proxy object, I.E -> an object that holds a raw pointer with an overloaded -> operator, so we don't throw raw pointers around, prevents possible memory leaks..

The manager will manage a list of loaded resources, call a copy constructor on the handles if resources already exist...

This way the resource manager can be encapsulated, and not intertwined with the creation of a render list... This will take a few days to code.

I'm not sure exactly where you're going with this, but you are aware that the handle would be hard to .. handle if it was a template eh?

I like the idea of using a different class to handle the resources, and not intertwine them into the render list, but then again that might be a little slow.

About the dangling pointer problem, that would be solved by using IDs or a little further into your idea, smart pointers (http://www.gamedev.net/reference/art...ticle1954.asp). I'm personally am going with IDs, and a templated smart pointer (which doesn't need to be handled generically, and its generally just a DWORD) which notifies the manager when an ID is no longer being referenced. That's not really necessary, and could have been handled either way.

I didnt even notice this was a render list, so I could be wrong about your intentions.

Hmmm, I'm pretty sure I'm destroying all encapsulation and modularity by closely tieing together the renderlist and the resource manager this way..

Right now I'm creating a renderlist directly through the resource manager.. To keep in the spirit of a generic resource manager, this simply won't do. But my question is, what system would create a resource list, if not the resource manager?

Right now, on other forums, this system is being challenged as unmodular and not encapsulated enough for its own good. Maybe I need to re-evaluate what exactly a resource manager should do...

A. Look up resources by string-based key and protect against multiple copies of resources with the same key.

B. Manage and protect the lifetime of those resources (creation and destruction; prevent users deleting a resource via a raw pointer)

A suggests that I should add the functionality to keep a list of loaded resources, as well as a reference to the resource accompanying the string. I should be able to search for a string, check its existance, and not load the resource again.

B suggests that the resource manager itself should handle the loading of a resource, the destruction of a resource, and prevent clients from deleting the resource. I.E the Rendering engine should not be able to remove a resource from the system.


1. The worst thing you could do it provide your client raw pointers. But the client needs to have some form of the actual resource or it won't be able to call methods on that resource.

This means, when a client, such as a rendering engine, requests a resource, we shouldn't give it a raw pointer. That could lead easily to dangling pointers and memory leaks. But the engine needs something, so we can give it an object that holds the pointer.. a proxy object, or, I.E -> a handle.

2. The resource manager itself simply keeps a list of resources and their names. It is responsible for allocating these resources (it does so in LoadResource(), which I haven't written here) and freeing them as needed (either when the manger itself is destroyed, or when the manager decides the resource hasn't been accessed recently enough, or the client calls a method on the manager to forcibly release a resource).

A couple of things, sort of related to stuff I have been doing (completely different, actually, but as you'll see very much the same).

Firstly you'll probably only have one resource manager for the app, correct? So that qualifies as a singleton. To implement this pattern, basically you make the constructor private, and make a public static class method called Instance() that returns a pointer to a resource manager. The point of the pattern is to guarantee that only one copy of the Resource manager exists. Here is a basic sample:

Code:

// resourcemanager.h
class ResourceManager {
    private:
    // Prevent explicit construction
    ResourceManager();
    ResourceManager(const ResourceManager&);
    static ResourceManager* pInstance;
    public:
    // This is the only way a caller can construct this class
    static ResourceManager* Instance();
};

// resourcemanager.cpp

// The pointer to the resourceManger:
ResourceManager* ResourceManager::pInstance = 0;

ResourceManager::ResourceManager()
{
    // any init here
}

ResourceManager* ResourceManager::Instance()
{
    // only one instance will ever exist
    if (!pInstance) pInstance = new ResourceManager();

    return pInstance;
}

Second thing is, it sounds like you're constructing objects from strings, which means you might be better off with an object factory. Basically what you'll gain here is that you can construct any object with exactly the same code, and without coupling your resource manager to any of the actual resources. Also, as you add new resources, you won't need to modify the factory, and if you come up with a suitably general Resource base class, you won't have to modify the Manager code either.

I'm just throwing this code together so you can get the idea (it's loosely based on the code provided in "Modern C++ Design"). If you have any questions, let me know.

Code:

// First you have a common base class for all resources:
class Resource {
    // some kind of common interface code goes here
};

// Here's the ResourceFactory (it's a singleton, but I'm getting 
// lazy about coding it, so I just put an Instance() function. But 
// you code it just like the Singleton above.

class ResourceFactory {
public:
    ResourceFactory* Instance();
    typedef Resource* (*CreateResourceCallback)();

private:
    typedef std::map<std::string, CreateResourceCallback> CallbackMap;

public:
    bool RegisterResource(const std::string& name, CreateResourceCallback cb) {
        return callbacks_.insert(CallbackMap::value_type(name,cb)).second;
    }

    bool UnregisterResource(const std::string& name) {
        return callbacks_.erase(name) == 1;
    }

    Resource* Create(const std::string& name) {
        CallbackMap::const_iterator i = callbacks_.find(name);
        if (i == callbacks_.end()) {
            throw std::runtime_error("Unknown resource type");
        }
        return (i->second)();
     }
private:
    CallbackMap callbacks_;
};

// Now, each new resource registers itself with the Factory:

class MS3DResource : public Resource {
  // whatever code goes here for your resource
};

// This is the callback for the Factory:
MyResource* MS3DResourceCreator()
{
    return new MS3DResource();
}

// This is a way to guarantee that your resource registers itself when the app is loading:
class FactoryRegistrar {
    FactoryRegistrar() {
        ResourceFactory::Instance()->Register("MS3D",MS3DResourceCreator);
    }
} MS3DRegistrar;

// Then at the end of all this, you can construct the resource
// from only a string:

ResourceManager::Instance()->Create("MS3D");

Well...that's it. I know I screwed something up here somewhere. Hopefully nobody complains because I didn't break things up into files as needed, didn't include <map>, <string>, etc. where appropriate.

Also, I think you can find a generic implementation of Singleton and ObjectFactory if you google for Loki. That's the name of the library that "Modern C++ Design" is based on.