vectors, pointers and wii<SOLVED>

[mv2112]

Ok so i am making an item system and there are these functions

Code: Select all

//itemsys.h
void ItemSys::Transfer(ItemSys*tar,int item_index)
{
for(int i=0;i<Items.size();i++)
{
     if(Items[i]->GetID()==item_index)
     {
           tar->AddItem(Items[i]); //Items is a std::vector<Item*>
           DeleteItem(item_index);
     }
}
}

void ItemSys::AddItem(Item*copy)
{
       Items.push_back(new Item(copy));
}

int ItemSys::AddItem(std::string file, int x, int y)
{
        Items.push_back(new Item(file,x,y,L,ID));
       ID++;
        return ID-1;
}

void ItemSys::DeleteItem(int index)
{
     for(int i=0;i<Items.size();i++)
    {
          if(Items[i]->GetID()==index)
           {
                   delete Items[i];
                   Items.erase(Items.begin()+i);
           }
    }
}

//item.h
Item::Item(Item*copy)
{
      *this=*copy; //sort of copy constructor
}

Item::Item(std::string file,int x, int y,lua_State * l,int id)
{
        //regular item loading function
      L=l;
      Pos.x=x; 
      Pos.y=y;
      File.open(file.c_str());
      //read from file bla bla bla
     ID=id;
}

int Item::GetID()
{
      return ID;
}

This is my problem:

Code: Select all

//half psuedo-code/C++
item_id=Map.ItemSys.AddItem("orb.txt",100,100); //returns the item id
if a button pressed
{
    Map.ItemSys.Transfer(&Player1.Inventory,item_id); //pick up item and it works
}
if b button pressed
{
     Player1.Inventory.Transfer(&Map.ItemSys,item_id); //this crashes after i pick up an item with the above function
}

The transfer function works once, but when i do it backwards (pick-up item vs drop item) it crashes my Wii. The ID of the object shouldnt change because its basically being copied when transfered to each itemsystem. Is my copy constructor bad? I dont know why it keeps crashing The item works fine before i add it to the player's inventory.
Im lost...

[K-Bal]

Iterators become undefined after erase. You need to do something like this:

Code: Select all

for(myList::iterator it = myList.begin(); it != myList.end(); )
{
  if( /*condition*/)
  {
    it = myList.erase(it);
    continue;
  }
  ++it;
}

[mv2112]

Where would i do that?

[K-Bal]

DeleteItems

[mv2112]

DeleteItems

Im not using iterators though, how would it become undefined?

[Bakkon]

Im not using iterators though, how would it become undefined?

You really should be instead of addressing by index. When you erase and item, the size is decreased, so once you reach the higher end of the loop, you'll be trying to access out of range data.

[mv2112]

Im not using iterators though, how would it become undefined?

You really should be instead of addressing by index. When you erase and item, the size is decreased, so once you reach the higher end of the loop, you'll be trying to access out of range data.

The index i'm searching for isnt the actual index of the item in the vector, the index is stored in the item itself and is retrieved with GetID(). So i just loop through the vector untill i find the item with the correct index:

Code: Select all

class Item
{
public:
      int GetID() {return ID;}
private:
      int ID; //has nothing to do with position in the vector
};

//in delete function
for(int i=0;i<Items.size();i++)
{
       if(Items[i]->GetID()==item_index)
       { 
                //item_index has nothing to do with i
                delete Items[i];
                Items.erase(Items.begin()+i);
       }
}

How would i be accessing an item that doesnt exist, plus this works when i transfer an item from the world to the player, but not when i transfer the item from the player back to the world.

[mv2112]

I am a retard and here is why:

Code: Select all

Item::~Item()
{
    SDL_FreeSurface(image);
}

I copy the item and then delete it when i transfer it from itemsys to itemsys. This calls the destructor so the pointer to the image is NULLed out and SDL causes the crash when the map tries to render the item whos image doesnt exist.

[XianForce]

Yeah... Start developing good coding practices. After you free the memory associated with a pointer, set it to NULL.

Code: Select all

Item::~Item()
{
     if(image)
          SDL_FreeSurface(image);
     image = NULL;
}

This way, the memory is only freed if it exists, and then it is set to NULL after it is freed.

If you try to render with a NULL surface, nothing bad happens, it just isn't rendered.

[mv2112]

Yeah... Start developing good coding practices. After you free the memory associated with a pointer, set it to NULL.

Code: Select all

Item::~Item()
{
     if(image)
          SDL_FreeSurface(image);
     image = NULL;
}

This way, the memory is only freed if it exists, and then it is set to NULL after it is freed.

If you try to render with a NULL surface, nothing bad happens, it just isn't rendered.

It actually crashed while trying to render the surface even after i set it to null in the destructor
My bad, looked at code wrong, you're right