Inventory tracking of dynamically allocated items

I have the following structure...

Code:

class CItem {};
class CWeapon: public CItem{};
class CArmor: public CItem{};
class CContainer: public CItem{};  //bags, chests, sacks, ...

A stripped-down definition of CContainer includes:

Code:

class CContainer : public CItem {
public:
    CContainer() {};
    virtual ~CContainer() {};

    CContainer& add(CItem*);        // manage contents_
    CContainer& remove(CItem*);  // manage contents_

    // Fills the instance from equip.dat. Builds up on CItem::load()
    virtual CContainer& load(const std::string&); 
private:
    std::vector<CItem*> contents_;
};

Let's assume I allocate memory for a CWeapon instance and as the game progress the player moves the weapon inside a container. When the destructor for that CWeapon instance is called, I'll be left with a dangling pointer in contents_ which I will want to manage.

More, the CContainer functions responsible for reporting the CContainer instance contents will either break or become undefined.

What is the best way to manage the pointer?

I'm thinking providing CItem, CWeapon, CArmor and CContainer (I allow containers inside containers) dtors with some form of checking to have them removed from the contents_ vector. A way to speed this would be to add a new CContainer* data member to all of them that would be either null or point to the container in which the object is.

The dtors would then be able to check this and call CContainer& remove(CItem*); before deleting it.

My question is: Is there a better way?

Use smart pointers.
If the container is the primary owner of the objects (i.e. the container represents the inventory) don't explicitely delete the objects; just remove them from the container. The smart pointer will take care of the rest.
If the container is not the primary owner (i.e. it's just an organization help) use weak pointers, and smart pointers for the primary owner, whatever that is. The container should then be smart enough to skip invalid weak pointers on querying.
I also seem to remember a container that automatically kills outdated weak pointers, but that might be Java.

Thanks CornedBee.

It's the second case; the container is not the owner of the object.
I guess I can implement the smart pointer through auto_ptr. It's how to implement a weak pointer that I will have to research. But that is definitely a thought.

Thanks again.

Ok... pointers are both fascinating and annoying. I think that's the same with the good things in life...

Just so that I now I got this straight...

After some reading around the boost library what probably will work for me is a combination of shared_ptr for the owner of the pointer and weak_ptr for the container elements. Would you agree?

Originally Posted by Mario F.

After some reading around the boost library what probably will work for me is a combination of shared_ptr for the owner of the pointer and weak_ptr for the container elements. Would you agree?

that sounds about right to me. you can use the expired member of the weak pointer in the container to check if it's still valid

I assume you are following my design I showed you on the game programming forum. Note that I said in that thread that (unless you are using BOOST), you cannot delete a pointer that you pass to an object in that object's destructor. You are just passing the pointer around so that another class can access that class's public functions, members, etc.

Code:

class A
{
  public:
    A() { }
    virtual ~A() { }
    
    void Foo() { }

};

class B
{
  A* m_pA;
 public:
   B(A* ptrA) { m_pA=ptrA;}
   virtual ~B() { delete m_pA;}

}

int main(void)
{
  A* pObjectA=new A();
  B* pObjectB=new B(pObjectA);

  delete pObjectB;

  //This line throws an exception because pObjectA is a NULL pointer
  pObjectA->Foo();    

  //This line will throw an exception on MSVC 6
  delete pObjectA;    

  
  return 0;
}

Note that pObjectA and B::m_pA are pointing at exactly the same portion of memory. So when B's destructor is called, it deletes A, which is pObjectA. So when delete pObjectA is called, it throws an exception. On some compilers, those that are more standard than MSVC 6, calling delete on a NULL pointer is perfectly valid and actually performs no operation. In MSVC 6 it will throw an exception. But in all compilers, if you delete pObjectB and then attempt to use pObjectA, it will throw an exception because pObjectA is NULL after deleting pObjectB. Note also that any class that uses pObjectA will also have a NULL pointer after pObjectB is deleted or goes out of scope. This could cause the entire system to crumble in a matter of nanoseconds.

Solutions:

• Use boost's pointer library which contains pointers that know the objects they are pointing to and whether or not they own the object they are pointing to.
• In your design notes, ensure that you understand which pointers are owned and which ones are not and make sure you only delete owned pointers.

The system I use to alleviate issues is this:

• All objects are responsible for their own cleanup. In other words, if an object pointer has been given or passed to a class, that class is NOT responsible for cleanup of the object.
• All memory and other resources are cleaned up by the owning object only.

Quite simply in my example: Since pObjectA and pObjectB are both objects in and of themselves, if pObjectA is passed to B, B is NOT responsible for cleaning it up. pObjectB should only clean up memory and resources that it specifically allocated during creation/construction. Other pointers to objects should be left alone.

• So, if the class didn't create the object pointed to, the class DOES NOT clean it up.
• If the class did create the object, the class DOES clean it up.

• Created=Clean up
• Given/Received/Passed=Do nothing

Another example: In my tile editor I have a document class that in turn has classes that manage tiles and maps. The document class creates the tile manager and map manager classes, so it cleans them up. However, the view class needs access to the tile manager class and the map manager class. So I put a pointer to both in the view class. But when the view class cleans up it does NOTHING with these pointers since the document class will clean them up when it goes out of scope.

CDocument
- CTileMgr
- CMapMgr

CView
- pointer to CDocument::CTileMgr
- pointer to CDocument::CMapMgr

CView does not own the two objects being pointed to therefore it is not responsible for cleanup. I did this because calling CDocument::GetMapMgr() and CDocument::GetTileMgr() everytime I need access to those would be annoying and slower. So I init the pointers once at creation:

Code:

void CView::Init(CMapMgr *pMapMgr,CTileMgr pTileMgr)
{
  m_pMapMgr=pMapMgr;
  m_pTileMgr=pTileMgr;
  m_bInit=true;
}

Cleanup of container objects
In your example the manager class should have a remove function that removes items from the vector. Make the vector member PRIVATE and control access to the vector through public functions. This way you can control exactly how and when objects are added/removed/inserted into/from the vector. Keeping the vector public is allowing other objects to misuse the vector and possibly break the manager class code.

Also before calling vector::erase(iterator), you should do: delete m_vVector[iIndex] and then call erase.

The first delete cleans up the C++ object inside of the vector, and the second line cleans up the vector.
Some documents state that calling erase() calls the destructor for the object, but so far I have not found this to be true. If you fail to call delete on a vector and think it is cleaned up by simply calling vector::clear(), you will leak every object in it if it was created on the heap.

Example from my texture manager. It uses a std::map, but same idea.

Code:

...
if (m_mapTiles.size()>0)
  {
    for (DWORD i=0;i<m_mapTiles.size();i++)
    {
      //CString text;
      //text.Format(L"Destroying tile #%u",i);
      //AfxMessageBox(text);
    
      m_mapTiles[i]->Destroy();
      delete m_mapTiles[i];
      m_mapTiles[i]=NULL;
    }
  
    m_mapTiles.clear();
  }
...

And Destroy for CTileGDI:

Code:

void CTileGDI::Destroy(void)
{
  delete m_pImage;
  delete m_pMask;
   
  m_pMask=NULL;
  m_pImage=NULL;
  
  //AfxMessageBox(L"Destroying raw data");  
  if (m_pBits) delete [] m_pBits;
 
}

I'm obviously cleaning up the entire collection in this code, but I'm sure you can figure out how to code a remove function. Remember if the user specifies an out of range ID/index, you should either assert, throw, or at least notify of an error.

If you have any questions PM me and perhaps we can specify a time and go on TeamSpeak and work through the problem together. And it's good that you are trying to get this resource management stuff down pat. It will help you in the long run.

Just a note: it is invalid and explicitely forbidden by the standard to use auto_ptr in containers. The reason are the move semantics of its copy constructor.

Lots of info there. Will give it an hard look. Thanks

Ok... I'm getting the hang of this, I think. However, I've hit a small obstacle and need help passing over it.

For now I'm skipping Bubba's excellent suggestions. I prefer to have my own code doing the pointer management, as your post above so clearly exemplified Bubba.. But I'm trying this with baby steps, so I figured that for the next few revisions of my code I'll use the boost libraries, untill I find myself comfortable enough to start coding my own manager class.

Ok... anyways... this is the problem:

A cutdown version of my CItem class just to put those typedefs into context for you good people:

Code:

#include <boost/shared_ptr.hpp>
#include <boost/weak_ptr.hpp>

class CItem {
public:
    typedef boost::shared_ptr<CItem> CItem_Ptr;
    typedef boost::weak_ptr<CItem> CItem_wPtr;
    
};

A cutdown version of my CContainer class:

Code:

class CContainer : public CItem {
public:
    CContainer() {};
    virtual ~CContainer() {};

    CContainer& add(CItem_Ptr);
    CContainer& remove(CItem_Ptr);

private:
    std::vector<CItem_wPtr> contents_;
};

And this is where I'm having problems; In the definition of add() when checking for duplicated entries (colored red):

Code:

CContainer& CContainer::add(CItem_Ptr obj) {

    CItem_wPtr ptr(obj); //creating weak_ptr

    if(obj.get() == this)
        throw std::invalid_argument("inserting container inside itself attempt");

    if( find(contents_.begin(), contents_.end(), ptr) != contents_.end() )
        throw std::invalid_argument("duplicate item attempt in inventory");

    if( !obj->isWearable(Container) ) {
        std::cerr << '\n' << "Object cannot be placed inside a container." << '\n';
        return *this;
    }

    if( obj->weight() > (int)maxweight_ || obj->volume() > (int)maxvol_ ) {
        std::cerr << '\n' << "Object is too big for this container." << '\n';
        return *this;
    }

    int currweight = (int)maxweight_ - this->weight();
    int currvol = (int)maxvol_ - this->volume();
    if( obj->weight() > currweight || obj->volume() > currvol ) {
        std::cerr << '\n' << "Container is too full." << '\n';
        return *this;
    }

    // ok to add
    //weight( weight() + obj->weight() );
    //volume( volume() + obj->volume() );
    //contents_.push_back(ptr);

    return *this;
}

I've got to the point where I understood boost::weak_ptr doesn't provide a equality operator overload. However, a predicate function or a function class for the find algorithm will not work either because what i'm searching for is any attempt to duplicate an object instance inside the container.

As in:

Code:

int main() {
    CItem::CItem_Ptr armor(new CArmor);
    armor->load("A001");

    CItem::CItem_Ptr armor2(new CArmor);
    armor2->load("A001");

    CContainer container;
    container.load("C001");

    container.add(armor);
    container.add(armor);  // Illegal. Would allow for items duplication
    container.add(armor2);  //Valid. Just another instance of the same object (2 helms +1)
}

Doh!

I can use a set, instead of a vector for my container!

the weak_ptr may not support operator==, but it does operator<

*slap self*

There is nothing wrong with using <boost>. I use it in my DX engine framework code to help me with pointers and COM objects. Things can get messy fast with COM objects in arrays and standard containers.

Originally Posted by Bubba

Things can get messy fast with COM objects in arrays and standard containers.

... and it doesn't even take COM as I'm just experiencing

Help me out here please. I'm almost done. But I've hit a brick wall now.

When I add items to the CContainer::set< boost::weak_ptr<CItem> > contents_, I also increase CContainer::weight_ and CContainer::volume_.

Obviously when the shared_ptr goes out of scope, the weak_ptr.expired() resolves to true. But... weight_ and volume_ were not updated accordingly.

Is the only solution to create a CContainerMngr class that will deal with these little problems? And could you give me a synopsis of that class please?