Sign in to follow this  
Vexator

Resource manager with std::map

Recommended Posts

I'm having trouble implementing a resource and entity manager using std::map's in my game engine. I've been using std::vector's before and it worked fine, but now that I added Lua scripting, I want all my resources and entities to be accessible by their filenames and names respectively. GetResource() and AddResource() seem to work fine, but as soon as I try to insert the newly created sound source (= entitiy) to the std::map in AddSoundSource(), my test application crashes. GetResource(), AddResource() in class Resource:
typedef std::map<std::string, Resource *> Resources;
typedef std::pair<std::string, Resource *> KeyResourcePair;
typedef std::map<std::string, Resource *>::iterator ResourcesIterator;

Resource *ResourceManager::GetResource( const std::string &filename )
{
	// look for resource
	ResourcesIterator kIterator;
	kIterator = m_hpkResources.find( filename );
	
	// return 0 if not found
	if( kIterator == m_hpkResources.end() )
		return 0;

	// return resource
	return ((*kIterator).second);
}

Resource *ResourceManager::AddResource( const std::string &filename )
{
	// look for resource, return if found
	Resource *pkResource = GetResource( filename );

	if( pkResource )
	{
		return pkResource;
	}

	// load resource
	// so far the only entities available are sound buffers,
	// so I can safely assume that the requested resource is
	// a sound buffer. Later, I'll have to check the filename
	// extension to create the appropiate entity
	pkResource = new SoundBuffer( filename );

	if( !pkResource->IsLoaded() )
	{
		return 0;
	}

	// add new resource to hash table
	m_hpkResources.insert( KeyResourcePair(filename, pkResource) );

	return pkResource;
}

GetSoundSource(), AddSoundSource() in class World:
typedef std::map<std::string, SoundSource *> SoundSources;
typedef std::pair<std::string, SoundSource *> KeySoundSourcePair;
typedef std::map<std::string, SoundSource *>::iterator SoundSourcesIterator;

SoundSource *World::GetSoundSource( const std::string &name )
{
	// look for sound source
	SoundSourcesIterator kIterator;
	kIterator = m_hpkSoundSources.find( name );

	// return 0 if not found
	if( kIterator == m_hpkSoundSources.end() )
		return 0;

	// return resource
	return ((*kIterator).second);
}

SoundSource *World::AddSoundSource( const std::string &name, const std::string &filename )
{
	// load sound buffer if it's not yet loaded
	SoundBuffer *pkBuffer = (SoundBuffer *)ResourceManager::GetSingleton().AddResource( filename );

	if( !pkBuffer )
	{
		return 0;
	}

	// create new sound source
	SoundSource *pkSource = new SoundSource( name, pkBuffer );

	// add new sound source to hash table
	m_hpkSoundSources.insert( KeySoundSourcePair(name, pkSource) ); // <- crashes here!

	return pkSource;
}

in my test application, I then call:
SoundSource *pkSource = g_pkWorld->AddSoundSource( "test", "test.ogg" );

which is supposed to check if the requested resource "test.ogg" has already been loaded, load it if it hasn't, and then create a new sound source and attach the resource to it. Any ideas what I'm doing wrong? I haven't worked with std::map's before, maybe I'm missing something? Thank you!

Share this post


Link to post
Share on other sites
Are you sure the World is properly initialized? m_hpkSoundSources is the first reference to a member variable in AddSoundSource. If g_pkWorld is a bad pointer when you make the call, a crash is typical.

What's the debugger say about the state of the relevant variables in scope when you insert a breakpoint on the line that crashes? In particular I'm interested in the values of this, m_hpkSoundSources, name and pkSource.

Share this post


Link to post
Share on other sites
there are a few better ways of using std::map


instead of return ((*kIterator).second);
you can use: return kIterator->second;


also, a better way to insert an item in a map:

instead of

m_hpkSoundSources.insert( KeySoundSourcePair(name, pkSource) );

you can use:

m_hpkSoundSources[ name ] = pkSource;

and yes, I know it's shocking, but its the right way to insert an item in a map.

Share this post


Link to post
Share on other sites
Quote:

there are a few better ways of using std::map:

instead of return kIterator->second;
you can use: return ((*kIterator).second);

This is equivalent, not better.

Quote:

also, a better way to insert an item in a map:

instead of

m_hpkSoundSources.insert( KeySoundSourcePair(name, pkSource) );

you can use:

m_hpkSoundSources[ name ] = pkSource;

and yes, I know it's shocking, but its the right way to insert an item in a map.

operator[] is not the "right" way, it is another way. insert() is perfectly correct.

Share this post


Link to post
Share on other sites
thank you for the quick replies!
yes, g_pkWorld is initialized before the my call to AddSoundSource. Both pkResource and pkSource are checked for validity as well before I try to insert them to the std::map's (I removed these checks in my post).

here is the debugger's output at the breakpoint:

Share this post


Link to post
Share on other sites
m_hpkResources is probably screwed (3452816845 is 0xCDCDCDCD is the clean memory fill, implying something is not properly initialized).

Also, note that m_pkBuffer and the world (this) live at the same address dispite being what I hope would be wholly distinct types (SoundBuffer and World)... why are they related? Is the value of m_uID sane, semantically? Something there certainly stinks.

Show the code dealing with the lifetime and interaction of the World object that contains m_hpkResources.

Also, I'm extremely suspicious of whatever this "ManagedObject" thing of your is. Stuff like "is allocated on stack" strikes me as a terrible thing to try and track, and something like that can easily cause weirdness related to memory issues like you appear to be exhibited. What is the motivation behind such an apparently ugly class?

Share this post


Link to post
Share on other sites
Quote:
also, a better way to insert an item in a map:

instead of

m_hpkSoundSources.insert( KeySoundSourcePair(name, pkSource) );

you can use:

m_hpkSoundSources[ name ] = pkSource;


These do not do the same thing at all!
Insert will try to add a new value, copy constructed from the pair you pass, tell you whether that succeeded or not. It will not affect an existing value in the map.
Operator[] will return a reference to the element you gave the key for, default-constructing it if it didn't exist. You then used that reference as an assignment target. That will update any element already present in the map.

Two very different things. Operator[] is easier to use, but due to language constraints it is primarily an update operation.

Share this post


Link to post
Share on other sites
your suspicion was justified! it works! ManagedObject is the base class needed for every object that is managed by my smart pointer class, but i was not using smart pointers for neither my resources nor my entities. i removed their dependence from ManagedObject and now it works fine. thank you guys!

Share this post


Link to post
Share on other sites
So this begs the question, why are you trying to hand-roll your own smart pointers instead of using those provided by Boost, which are likely to be far superior to yours in -- if nothing else -- their stability.

Smart pointers can be extremely difficult to get right. What do yours look like? It's likely you have additional latent bugs. You should strongly consider using Boost's shared_ptr or intrusive_ptr.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this