Pointer Comparison

[Fenrir]

I can't seem to figure out why this is happening.

Code: [Select]

void AI::die()
{
    Creature::die();
    Level* thisLevel = getLevel();
    Player* player = getDungeon()->getPlayer();
    if(player != NULL)
    {
        if(player->getLevel() == thisLevel && thisLevel->LOS(player->getX(),player->getY(),getX(),getY()))
        {
            player->addMessage("The " + getName() + " dies.");
        }
    }
    if(thisLevel->getTileAt(getX(),getY()).flags == 0)
    {
        thisLevel->setTileAt(getX(),getY(),Dungeon::TI_CORPSE);
    }
    thisLevel->removeCreature(this);
    delete this;
}

"delete this;" in the above code results in a crash. When that line is commented out and a creature dies, a different creature disappears. It seems to be removing the wrong creature from the level's vector of creatures on the map. If that's so, the following code must be to blame.

Code: [Select]

void Level::removeCreature(Creature* remove)
{
    for(int i=0; i<creatures.size(); i++)
    {
        if(creatures[i] == remove)
        {
            creatures.erase(creatures.begin()+i-1);
        }
    }
}

"creatures(i) == remove" seems to be true for every creature in the list.

My apologies if my explanation is rather muddled.

If you need to see more code I'll post it, but it won't be pretty! I can also post some screen shots of exactly what happens when "delete this;" in the AI::die() function is commented out.

[corremn]

We are talking C++??

I assume AI inherits from creature?? That down not sound right.  Creature should inherit from AI.  Anyway you need to proably give us some sort of class information.

Umm, I am sure you should NOT be deleting this while you are in a function of this. Sounds very dangerous to me. Although it is valid code, I am guessing your this object was not created with new.  If you created your object with new it should not crash, but is a bad idea generally.
I would do something like this.

Code: [Select]


CreatureManager::die(Creature *creature)
{
   if(remove!=NULL)
   {
       creature->die();
       delete creature;
   }
}
(Assuming creature was dynamically allocated, otherwise as I suspect it does not need to be deleted. Also make sure it is not crashing in the object destructor ).
I would also move the level->removeCreature and the player message from the AI::die and put them in the above method.

As for your other problem...

Code: [Select]

if(creatures[i] == remove) should work fine if creatures is a vector of creature pointers.   I.e

Code: [Select]

std::vector<Creature*> creatures;   If it is not a list of pointers then you are using some sort of overwriten == operator that I dont know about.

Also I am not sure of the logic of this line,  note that it will probably crash when you delete the first element. 

Code: [Select]

creatures.erase(creatures.begin()+i-1);You  also  should return from the function when you do this line. 
Do something safer like this.

Code: [Select]

std::vector<Creature*> creatures;

for(std::vector<Creature*>::iterator it = creatures.begin();it!=creatures.end();)
{
    if(*it == remove)
         it=creatures.erase(it);
    else
        ++it;

}or for this case

Code: [Select]

for(std::vector<Creature*>::iterator it = creatures.begin();it!=creatures.end();++it)
{
    if( *it == remove)
   {
        creatures.erase(it);
        return;
    }
   
}

Happy coding.

[corremn]

Hey, can you post your player->addMessage() code.

That looks like something I should be doing.  I am still using c style
i.e. addMessage(char * ...);   

as my sig demonstrates.

I have been meaning to look into upgrading to c++ messges, but if you have already done it...

[Krice]

http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.15

[Fenrir]

We are talking C++??

I assume AI inherits from creature?? That down not sound right.  Creature should inherit from AI.  Anyway you need to proably give us some sort of class information.

Umm, I am sure you should NOT be deleting this while you are in a function of this. Sounds very dangerous to me. Although it is valid code, I am guessing your this object was not created with new.  If you created your object with new it should not crash, but is a bad idea generally.

Both Player and AI inherit from Creature which inherits from Entity.

The object was created with new. The crash wasn't actually happening at the delete statement, though my pathetic explanation made it sound that way. The crash was happening later when the Level was looping through the creatures and drawing them. Since my removeCreature() function was messed up, the wrong creature got removed from the list and the pointer to the deleted one stayed, so the function that draws creatures to the screen was trying to access something that wasn't there.

I read that for every new statement, one should have a delete statement to free the memory allocated. Since creatures is a vector of pointers to Creature objects, should I be deleting them when the level is destroyed?

Code: [Select]

Level::~Level()
{
    // Actual code from my game
    delete [] map;
    delete [] flagmap;
   
    // Should I do the following?
   for(std::vector<Creature*>::iterator it = creatures.begin();it!=creatures.end();++it)
   {
      delete creatures[i];
   }
   creatures.clear();
}
The level is the only thing that ever points to creatures, but that might change. Perhaps I should make the Dungeon class's (represents the whole game; has a linked list of Level objects) destructor go through all its Level objects and make them delete their creatures. It doesn't seem right to have a bunch of dead creatures taking up memory, but I don't think it matters too much in a little project like this.

Code: [Select]

for(std::vector<Creature*>::iterator it = creatures.begin();it!=creatures.end();++it)
{
    if( *it == remove)
   {
        creatures.erase(it);
        return;
    }
   
}


This fixes the problem, thanks! I hope you don't mind if I just copy/paste.

Hey, can you post your player->addMessage() code.

That looks like something I should be doing.  I am still using c style
i.e. addMessage(char * ...);  

as my sig demonstrates.

I have been meaning to look into upgrading to c++ messges, but if you have already done it...

Are you sure you want my code after seeing some of it?

Code: [Select]

void Player::addMessage(std::string message)
{
    messBuffer.push_back(message);
    messList.push_back(message);
}
Both messBuffer and messList are vectors of strings in the Player class. messBuffer stores the recent messages that the player hasn't seen yet and is emptied after they are displayed. messList stores all messages received throughout the game so the player can review them later.

Code: [Select]

void Player::displayMessages()
{   
    if(messBuffer.size() > 0)
    {
        int linePos=0; // horizontal position of the current message
        for(int i=0; i<messBuffer.size(); i++)
        {
            mvaddstr(0,linePos,messBuffer[i].c_str());
            linePos += messBuffer[i].length() + 1; // Move the position of the line forward the length of the message and add some space.
            if(messBuffers.size() > i+1 && linePos+messBuffer[i+1].length() > COLS-6) // If the next message is longer than the screen and the 6 characters needed to add "(more)"....
            {
                mvaddstr(0,linePos,"(more)");
                getch(); // Wait for the player.

                // Redraw the screen.
                erase();
                getLevel()->print();
                attron(COLOR_PAIR(getColor()));
                mvaddch(getY(),getX(),getSymbol());
                linePos=0;
            }
           
        }
        messBuffer.clear();
    }
}
I hope that helps, but I doubt it, considering how much more experienced you are.

I'm using pdcurses. What are you using?

[corremn]

Both Player and AI inherit from Creature which inherits from Entity.

Ok that makes sense, I assumed that AI was the AI routines not an AI controlled creature.

The object was created with new. The crash wasn't actually happening at the delete statement, though my pathetic explanation made it sound that way. The crash was happening later when the Level was looping through the creatures and drawing them. Since my removeCreature() function was messed up, the wrong creature got removed from the list and the pointer to the deleted one stayed, so the function that draws creatures to the screen was trying to access something that wasn't there.

You probably should test for NULL pointers, and skip any null ones.  I.e If(pointer) pointer->DoSomething();
Or at least use assert statements or throw exceptions when a null pointer is accessed and was not expected.

You will find that pointers are easy to use and efficient but come with their own problems. (that you are experiencing).  I am starting to move away from pointers and are using map with unique creature references and I dont dynamically allocated memory for them. So once my map goes out of scope they are automatically deleted.  But everyway has its problems.  Pointers are still easy and fast.  Some sort of smart pointer would be best.  (which will be handled  better in the next c++ update btw;))

I read that for every new statement, one should have a delete statement to free the memory allocated. Since creatures is a vector of pointers to Creature objects, should I be deleting them when the level is destroyed?

Yes use delete everytime new is used. 
I delete creatures when they are killed, like you did, and when a level is destroyed.
Note that before you use delete creatures; you should test to make sure it is not null. Once again with a if(creatures) statement.

Code: [Select]

Level::~Level()
{
    // Actual code from my game
    delete [] map;
    delete [] flagmap;
   
    // Should I do the following?
   for(std::vector<Creature*>::iterator it = creatures.begin();it!=creatures.end();++it)
   {
      if(creatures[i]) //test to see if valid pointer
        delete creatures[i];
   }
   creatures.clear();
}


[/quote]

Are you sure you want my code after seeing some of it?

Code: [Select]

void Player::addMessage(std::string message)
{
    messBuffer.push_back(message);
    messList.push_back(message);
}

I hope that helps, but I doubt it, considering how much more experienced you are.

I'm using pdcurses. What are you using?

I totally forgot about the std::string + operater!  However you cant use numbers easily with this approach.
Hmm, something to look at anyway. thanks a lot.

I am not that experienced, I am c++ self taught myself and always have more to learn.

I am using C++/SDL/OpenGL.   This allows me to easily switch between tiles and emulated ascii.

[Fenrir]

However you cant use numbers easily with this approach.

What do you mean exactly? You can use the '+' operator to add an integer to a string.

I am using C++/SDL/OpenGL.   This allows me to easily switch between tiles and emulated ascii.

SDL is just a bit too complicated for me at this point. pdcurses is really limited, but it's easy to use. libtcod looks awesome, so I might give that a try instead of going the SDL route. I don't like tiles, anyway.

[corremn]

However you cant use numbers easily with this approach.

What do you mean exactly? You can use the '+' operator to add an integer to a string.

Not with out turning it into a string first.  str += '1'; will work. str+=1; will not.
std::sprintf or std::stringsrtreams are genreally used. However I would like to know if I am wrong.

[Fenrir]

Oh, you're right. Never mind.

Thanks for all the help, corremn. This thread has been educational.

EDIT:
Woah, I can't believe I mixed up "your" and "you're". Fixed.