Sign in to follow this  
Tokiko

Program crashes with STD::map find()

Recommended Posts

Tokiko    138
Hello, I have a function that crashes my program when it tries to find an object in a map. Here is the code. Is it possible to see why this might be happening because I am stuck on this one...
void CollisionSet::addCollision( GameComponent* first, GameComponent* second )
{
    // adds the collision to the first component's list
    std::map< GameComponent*, CollisionQueue* >::iterator firstComponent = this->_collisionMap->find( first );
    
/*if ( firstComponent == this->_collisionMap->end() )
    {
        CollisionQueue* firstQueue;
        this->_collisionMap->insert( std::make_pair( first, firstQueue ) );
    }
Everything in the function is commented out, so this has to be the problem. Thanks

Share this post


Link to post
Share on other sites
Tokiko    138
Well I'll add something to check it directly, but they should be valid since I do some things with them before calling that function. I just realized that it's almost impossible for anyone to determine the problem from what I posted, so I'll look over it some more. Thanks anyways.

Share this post


Link to post
Share on other sites
RuntCreature    100
I'm talking about something like the following...You never actually create a map object to point to, so it crashes.


#include <map>

int main()
{
std::map<int *, char *> * pMap;

std::map< int*, char* >::iterator first = pMap->find( NULL );

return 0;
}




needs to be:


#include <map>

int main()
{
std::map<int *, char *> * pMap = new std::map<int*,char*>;

std::map< int*, char* >::iterator first = pMap->find( NULL );

delete pMap;

return 0;
}




It might also be that your object has been destructed and your this pointer is garbage.

Share this post


Link to post
Share on other sites
Deception666    182
Are you sure you are wanting to insert a non-assigned pointer into your _collisionMap.

From your code:

CollisionQueue* firstQueue; // this is not pointing to a valid object. are you handling this?
this->_collisionMap->insert( std::make_pair( first, firstQueue ) );

Share this post


Link to post
Share on other sites
Verg    450
Quote:
Original post by RuntCreature
Check your pointer to make sure it is pointing to a valid object. Thats my first guess. (The _collisionMap pointer)


If "find()" isn't crashing, and either "end()" or "insert()" is where the crash happens, it's likely that the pointer points to "something", but that the vtable is messed up.

It would help to see how the map instance is created... as you suggest.

Share this post


Link to post
Share on other sites
Tokiko    138
Ok here is the constructor:

CollisionSet::CollisionSet()
{
// initialize the collision set
this->_collisionMap = new std::map< GameComponent*, CollisionQueue* >();
}


and here is the whole function. I commented out everything then started with the first line which happened to crash the game. So that's how I figured that it is the problem.

void CollisionSet::addCollision( GameComponent* first, GameComponent* second )
{
// adds the collision to the first component's list
std::map< GameComponent*, CollisionQueue* >::iterator firstComponent = this->_collisionMap->find( first );

if ( firstComponent == this->_collisionMap->end() )
{
CollisionQueue* firstQueue;
this->_collisionMap->insert( std::make_pair( first, firstQueue ) );
}

Collision firstCollision( second );
firstComponent->second->addCollision( &firstCollision );

// adds the collision to the second component's list
std::map< GameComponent*, CollisionQueue* >::iterator secondComponent = this->_collisionMap->find( second );
if ( secondComponent == this->_collisionMap->end() )
{
CollisionQueue* secondQueue;
this->_collisionMap->insert( std::make_pair( second, secondQueue ) );
}

Collision secondCollision( first );
secondComponent->second->addCollision( &secondCollision );
}


I'll have to fix that make_pair too. A buddy of mine wrote this code, so I'm just trying to fix his bugs.

Share this post


Link to post
Share on other sites
Tokiko    138
I noticed that

firstComponent->second->addCollision( firstCollision );

Won't work if it is pointing to _collisionMap->end(), so I made sure that firstComponent points to the new element when the new pair is made. Also, after fixing that, it will do the initial search for first without crashing. I'm going to test it to see if it's working properly now.

Share this post


Link to post
Share on other sites
Zahlman    1682
Why are you using pointers to GameComponents as your keys? Where are the pointed-at GameComponents? Similarly about the values.

Share this post


Link to post
Share on other sites
Verg    450
Quote:
Original post by Tokiko
I noticed that

firstComponent->second->addCollision( firstCollision );

Won't work if it is pointing to _collisionMap->end(), so I made sure that firstComponent points to the new element when the new pair is made. Also, after fixing that, it will do the initial search for first without crashing. I'm going to test it to see if it's working properly now.


Similarly, your last line won't work if the iterator is bad:


secondComponent->second->addCollision( &secondCollision );


And I'm with Zahlman... why are you inserting "pointers to nothing" into your map? You may as well not insert anything... and just reference your map like this:


(*_collisionMap)[first] = new CollisionQueue();


Your calls to "insert" aren't having any affect, as written. If you invoke the bracket ([]) operator, you can just make an assignment whenever you want. If the key is in the map already... great! You'll wipe out whatever was there before.

(Not a good idea if it was a real pointer to something)

But as written, "insert" gives you absolutely nothing.

I would suggest using reference counted smart pointers instead of raw pointers in your map to make this strategy more air-tight.

Share this post


Link to post
Share on other sites
Tokiko    138
Thanks Verg, I'll have to try that out after work. I should have noticed these problems, but like I said, I didn't look over it in too much detail as I didn't write it.

I was thinking of using smart pointers in the game. I've already run into problems deleting pointers and causing memory violations, so I obviously don't have enough experience with them yet. I'll have to look into that.

Share this post


Link to post
Share on other sites
Tokiko    138
Here's what I have so far except I'm still having difficulties. Everything will work if the last line is commented out, this one: (*this->_collisionMap)[ second ]->addCollision( new Collision( first ) );
It is adding a collision to the second game components collision queue.
Someone asked above why we use pointers for the maps key. We have to because we are passing in derived objects of that GameComponent type.

void CollisionSet::addCollision( GameComponent* first, GameComponent* second )
{
// adds the collision to the first component's list
std::map< GameComponent*, CollisionQueue* >::iterator firstComponent = this->_collisionMap->find( first );
if ( firstComponent == this->_collisionMap->end() )
{
// this component does not yet have a queue for collisions, we must put a new one in the map
this->_collisionMap->insert( std::make_pair( first, new CollisionQueue() ) );
}
// create a collision and add it to the first components collision queue
(*this->_collisionMap)[ first ]->addCollision( new Collision( first ) );

// adds the collision to the second component's list
std::map< GameComponent*, CollisionQueue* >::iterator secondComponent = this->_collisionMap->find( second );
if ( secondComponent == this->_collisionMap->end() )
{
// this component does not yet have a queue for collisions, we must put a new one in the map
this->_collisionMap->insert( std::make_pair( second, new CollisionQueue() ) );
}
// create a collision and add it to the second components collision queue
// comment this line out for the game to run - leave it and the game crashes on a collision
(*this->_collisionMap)[ second ]->addCollision( new Collision( first ) );
}



So does anyone have any ideas why this only crashes for the 2nd part of the code? Could it be some weird memory problem with all these pointers? I'm at a loss.

Share this post


Link to post
Share on other sites
Tokiko    138
Sorry everyone, my friend is to blame for all of these errors. He first forgot to allocate the map in the constructor (which I fixed) but we just noticed that he didn't allocate the list inside the collisionQueue. All is good now. Thanks for everyone's help.

Share this post


Link to post
Share on other sites
Verg    450
Let's see if we can clean this up:


void CollisionSet::addCollision( GameComponent* first, GameComponent* second )
{
std::map< GameComponent*, CollisionQueue* >::iterator firstComponent = _collisionMap->find( first );

if ( firstComponent == _collisionMap->end() )
(*_collisionMap)[first] = new CollisionQueue();

(*_collisionMap)[first]->addCollision( new Collision( first ) );

std::map< GameComponent*, CollisionQueue* >::iterator secondComponent = _collisionMap->find( second );

if ( secondComponent == _collisionMap->end() )
(*_collisionMap)[second] = new CollisionQueue();

(*_collisionMap)[ second ]->addCollision( new Collision( first ) );
}




Are you SURE you want that last line to read like that, and not like this??


(*_collisionMap)[ second ]->addCollision( new Collision( second ) );


Also... take out the spurious "this->" stuff... unless you're passing in another parameter with the same name (_collisionMap) or you have one in the namespace surrounding your class (global to it)... you don't need it, and it makes everything easier to read.

And there's some obvious duplication there... you could just call one function twice and pass in the parameters.


C

Share this post


Link to post
Share on other sites
Tokiko    138
Quote:
Are you SURE you want that last line to read like that, and not like this??

(*_collisionMap)[ second ]->addCollision( new Collision( second ) );

Sorry there was a typo with the first collision and it should be passing second. So if I didn't type that wrong, you would be correct :P
Quote:
Also... take out the spurious "this->" stuff... unless you're passing in another parameter with the same name (_collisionMap) or you have one in the namespace surrounding your class (global to it)... you don't need it, and it makes everything easier to read.

Yes that would be easier to read, but the only reason why we use it for member variables is because we developed that habit at school. Our CS teachers wanted us to use it in our assignments and it may have been counted as a mark? Can't remember.
(*_collisionMap)[first] = new CollisionQueue();
Now with this line here. I was going to use that earlier, but I figured that it wouldn't work since you are trying to find the value paired with first, but if that key hasn't been added, how can you make a new collision? So I guess it will insert a new element if it is not found?

Thanks for the help. We went from a large broken function, to a much smaller and easier to read function :)

Share this post


Link to post
Share on other sites
Zahlman    1682
Quote:
Original post by Tokiko
Sorry everyone, my friend is to blame for all of these errors. He first forgot to allocate the map in the constructor (which I fixed) but we just noticed that he didn't allocate the list inside the collisionQueue. All is good now. Thanks for everyone's help.


Why would you be allocating instances of standard library containers? Just hold them as members.

The whole reason you are using standard library containers is so that they can do memory management work for you. Why go on and do it anyway?

Share this post


Link to post
Share on other sites
Tokiko    138
Quote:
Original post by Zahlman
Why would you be allocating instances of standard library containers? Just hold them as members.

The whole reason you are using standard library containers is so that they can do memory management work for you. Why go on and do it anyway?

I must have worded that wrong. I didn't create the object in the constructor. Like so:
map<int, string>* obj = new map<int, string>();


Share this post


Link to post
Share on other sites
jyk    2094
Quote:
Original post by Tokiko
I must have worded that wrong. I didn't create the object in the constructor. Like so:
map<int, string>* obj = new map<int, string>();
That's a little confusing. If the above is an excerpt from your actual code, then you are in fact allocating the map dynamically in the constructor. Which then brings us back to Zahlman's question: why?

[Edit: Actually, I'm guessing (based on the use of a local temporary) that the above code excerpt was just an example, but the samples you posted earlier indicate that you are indeed allocating the container dynamically in the object's constructor.]

Share this post


Link to post
Share on other sites
Verg    450
Quote:
Original post by Tokiko
Quote:
Are you SURE you want that last line to read like that, and not like this??

(*_collisionMap)[ second ]->addCollision( new Collision( second ) );

Sorry there was a typo with the first collision and it should be passing second. So if I didn't type that wrong, you would be correct :P


Well then you can simplify the code even more...


void CollisionSet::addCollision( GameComponent* component )
{
if ( _collisionMap->find( component ) == _collisionMap->end() )
(*_collisionMap)[component] = new CollisionQueue();

(*_collisionMap)[component]->addCollision( new Collision( component ) );
}

// And you would just call it twice, once for each component...
// since they don't depend on one another in the original function

addCollision( first );
addCollision( second );



Quote:

(*_collisionMap)[first] = new CollisionQueue();
Now with this line here. I was going to use that earlier, but I figured that it wouldn't work since you are trying to find the value paired with first, but if that key hasn't been added, how can you make a new collision? So I guess it will insert a new element if it is not found?


Exactly. But you should check for the element first or you would erase the old CollisionQueue, if it already existed.

And I'm with Zahlman... is there a compelling reason to dynamically create a map? You could just use the map as an instance variable, and then you don't have to destroy it later.

Share this post


Link to post
Share on other sites
Tokiko    138
Quote:
Original post by Verg
And I'm with Zahlman... is there a compelling reason to dynamically create a map? You could just use the map as an instance variable, and then you don't have to destroy it later.


This has been brought up a couple times, but I'm still not sure how I can fix this. I thought if you have a map declared in the class as a member, there are no problems with initializing it in the constructor. Can you or someone give me an example of what you would do in this case?

Quote:
void CollisionSet::addCollision( GameComponent* component )
{
if ( _collisionMap->find( component ) == _collisionMap->end() )
(*_collisionMap)[component] = new CollisionQueue();

(*_collisionMap)[component]->addCollision( new Collision( component ) );
}

// And you would just call it twice, once for each component...
// since they don't depend on one another in the original function

addCollision( first );
addCollision( second );


I wouldn't be able to simplify it to that. When I make the collision object for the first component, I need to pass the second component so that I know what collided with it. Same goes for the second component - I pass the first one to the collision object. With your code, you would be creating a collision between the component and itself.

Share this post


Link to post
Share on other sites
nullsquared    126
Quote:
Original post by Tokiko
Quote:
Original post by Verg
And I'm with Zahlman... is there a compelling reason to dynamically create a map? You could just use the map as an instance variable, and then you don't have to destroy it later.


This has been brought up a couple times, but I'm still not sure how I can fix this. I thought if you have a map declared in the class as a member, there are no problems with initializing it in the constructor. Can you or someone give me an example of what you would do in this case?


I believe they meant like so:

class YourClass {
std::map<GameComponent*, CollisionQueue*> _collisionMap;

[...]
};

The _collisionMap will be allocated on the stack, not the free-store (heap). Thus, no messy allocation/destruction (new/delete).

Share this post


Link to post
Share on other sites
Tokiko    138
Ok, so in other words, don't make it a pointer? Because that is the only difference between what I have and what you suggested.

Share this post


Link to post
Share on other sites
jyk    2094
Quote:
Original post by Tokiko
Ok, so in other words, don't make it a pointer?
Right - just make it a member variable of the class (as agi_shi's example illustrates).

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