Click here to Skip to main content
14,879,256 members
Please Sign up or sign in to vote.
4.00/5 (1 vote)
See more:
Hi,

I've written the following inventory management system, and I've come across a potential memory leak which I'm unsure how to handle. There are two issues here;

1. The std::vector<Item*> items is storing pointers to items which need to be deleted to free memory when the game ends - as it's a member variable, items itself will be deleted when PlayerInventory is deleted, but as for the pointers it contains, I'm not sure how to manage them.

2. In RemoveItemsOfType, I create a new item called nullItem and set the item in the given position to nullItem. Do I need to call 'delete items[slot];' before I do this to free the memory held by the item currently there, before I replace it with nullItem?

#include "Item.h"

void Item::Init(ItemType type)
{
	this->type = type;
}

ItemType Item::GetType()
{
	return type;
}
#include "PlayerInventory.h"

PlayerInventory::PlayerInventory()
{
	for (int i = 0; i < slots; i++)
	{
		Item* item = new Item();
		item->Init(ItemType::NONE);
		items.push_back(item);
	}
}

void PlayerInventory::SetItem(int slot, Item* item)
{
	items[slot] = item;
}

Item* PlayerInventory::GetItem(int slot)
{
	return items[slot];
}

void PlayerInventory::RemoveItemsOfType(ItemType type)
{
	for (int i = 0; i < slots; ++i)
	{
		if (GetItem(i)->GetType() == type)
		{
			Item* nullItem = new Item();
			nullItem->Init(ItemType::NONE);
			SetItem(i, nullItem);
		}
	}
}


What I have tried:

Thinking very hard about what to do but not knowing the answer so coming here for advice :)
Posted
Updated 25-May-18 11:27am
v2
Comments
Richard MacCutchan 25-May-18 8:40am
   
Why are you putting a new item in the slot? Why not just change the type of the existing one? But if you must do it your way then you need to delete the existing item before you overwrite the entry.
[no name] 25-May-18 8:49am
   
Hmm. I'm taking the wrong approach here. I need Item to be a base class which other classes can inherit from.

When PlayerInventory is deconstructed, and the member 'items' is deleted, will I need to delete each of the pointers in 'items' in PlayerInventory::~PlayerInventory()?
Richard MacCutchan 25-May-18 9:04am
   
Basically, yes. If you create an object with new then you must dispose of it with delete. Exactly where you do that will depend on the structure of your application.
[no name] 25-May-18 10:31am
   
If only I knew the answer while writing the question, maybe I could have gotten a 5-star. Damn it.
Richard MacCutchan 25-May-18 10:36am
   
Well things are improving, you got a 4 today.
[no name] 25-May-18 10:37am
   
yay! :>

Use a smart pointer. Try, for instance
C++
#include <iostream>
#include <vector>
#include <memory>
using namespace std;

class F
{
  int f;
public:
  F(int f):f(f) { cout<< "F(" << f << ") ctor called\n";}
  ~F() { cout << "F(" << f  << ") dtor called\n";}
};

int main()
{
  vector< F * > v;
  v.push_back(new F(1));

  vector < unique_ptr< F > > w;
  w.push_back( make_unique< F >(2) );
}

Output:
F(1) ctor called
F(2) ctor called
F(2) dtor called
   
Rule of thumb is, who creates memory has to delete it. So when you remove an vector you must delete the memory.

But you have the additional problem, that your vector may be copied or assigned somewhere deep in the vector implementation and so you may have 2 vectors with the same memory. Maybe it helps to overwrite these operators in which you copy the memory of the sourse vector.

I think you dont need to allocate the Item with new, but can use a typed vector.
C++
std::vector<Item> myList;
   
Comments
[no name] 25-May-18 9:04am
   
So in PlayerInventory, I have a member variable std::vector<Item*> items.

When the PlayerInventory is destructed, items will be automatically deleted too. However, does this mean those Item*s' memory is still allocated and need to be deleted within the destructor?
jeron1 25-May-18 10:11am
   
" in PlayerInventory, I have a member variable std::vector<Item*> items...."
"When the PlayerInventory is destructed, items will be automatically deleted too."
Someone correct me if I'm wrong, but that doesn't sound right to me. I think if you're going to 'new' objects, you need to 'delete' them, probably in the PlayInventory destructor. I'm in agreement with KarstenK on this in using the typed vector, then memory management is out of your hands.
[no name] 25-May-18 10:29am
   
I've been told that member variables, the std::vector itself, will be deleted when the class is destructed. I don't know whether that automatically deletes the pointers in the vector or not, which are created with 'new'.

the vector is declared as such;

public:
        std::vector<Item*> items;
KarstenK 26-May-18 4:23am
   
In C++ werent pointer deleted when the container gets deleted. This ONLY happens with smart pointers.

Tip of a seasoned developer: learn and understand the techniques you want to use ;-)
[no name] 26-May-18 10:20am
   
I understand best by trying, instead of reading a book and having no idea what anything actually translates to in a real project.
I have found that yes, you do have to manually delete vectors of pointers. If you use Visual Studio and the DEBUG_NEW macro you will see the memory leaks in the debugger if you don't. At least, I do. I came up with this little template function a while ago to help with this situation :
C++
// delete a vector of objects created with new

template< typename T >
void DeleteVector( std::vector<T> & v )
{
    size_t count = v.size();
    for( size_t n = 0; n < count; ++n )
        delete( v[n] );
    v.clear();          // just to be sure
}
If you use a malloc variant to allocate the pointers then you can make a variation of this function that calls free instead of delete and since free is not a typed operation the function does not need to be a template.
   
v2
Comments
[no name] 25-May-18 11:04am
   
I'll definitely have a good look into this. Would it be a good idea to have all of my classes include a .h with #define new DEBUG_NEW inside so I can collect all usages of 'new' in the entire project? I feel that would give me good insight into memory leaks anywhere they may be.

I've read that std::vector::clear() destroys and deletes each item in the vector, so I believe a simple call to clear() without the loop would be sufficient
jeron1 25-May-18 11:12am
   
"I've read that std::vector::clear() destroys and deletes each item in the vector,"
That probably applies to memory the vector itself allocates (the 4 or 8 bytes needed to hold your pointers) not the objects that those pointers are pointing to.
[no name] 25-May-18 11:18am
   
From cplusplus.com: "Removes all elements from the vector (which are destroyed), leaving the container with a size of 0."
jeron1 25-May-18 11:47am
   
The elements in your vector are pointers. Each 4 or 8 byte entry is removed/destroyed, not the objects they reference. Perhaps set a breakpoint on the destructor on an 'Item' and see if it gets called when your vector is destroyed.
[no name] 25-May-18 11:49am
   
Great idea, I didn't think of that. I'm actually using Unreal Engine 4 which uses Visual Studio for development, so I don't believe there's a way to set breakpoints but I can certainly output something to the console within the destructor
jeron1 25-May-18 12:02pm
   
No breakpoints or the ability to step through code? It's got to be in there somewhere, I can't imagine (nor would I consider) doing any development without it.
[no name] 25-May-18 12:05pm
   
It's a very unfriendly engine. The learning curve is sure to push away most people before they even begin. I've gotten to the point of anger with its uselessness, lack of C++ examples with the documentation or even a clear explanation, and the fact that they only seem to care about the blueprint element while completely disregarding those who want to have better control and program in C++. Everything is very difficult, you can't even have constructors with arguments.

Ask a generic question on google regarding unreal engine, you'll see almost only blueprint examples.

The UE4 way is the only way, no room for per-developer habits.
jeron1 25-May-18 12:15pm
   
Doesn't sound like fun.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)




CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900