Program crashes with STD::map find()
Why are you using pointers to GameComponents as your keys? Where are the pointed-at GameComponents? Similarly about the values.
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.
Author
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.
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.
Author
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.
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.
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.
Author
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.
Let's see if we can clean this up:
Are you SURE you want that last line to read like that, and not like this??
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
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
Author
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();Thanks for the help. We went from a large broken function, to a much smaller and easier to read function :)
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?
Author
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>();
Quote:Original post by TokikoThat'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?
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>();
[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.]
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
