Post the entire compile error, from the Output pane in Visual Studio, with the Debug filter selected from the dropdown. Your compiler is telling you exactly where the problem is, all the way from the "real" error up through the templates of the standard library implementation and into your own code; it can be hard to read, but learning to interpret these error chains is immensely valuable.
The last chunk of the error output should point you exactly to what part of your code is causing the issue.
[edit] Afterthought: you should emplace_back the raw pointer, not the unique_ptr. In fact you shouldn't need to create a unique_ptr inside the function at all.
A list that does not disturb the order when pushed/popped?
List<Player> players = new List();
ah! Java/C# programmer in the room 
That's no C++.
Anyway.. to answer your question, you are trying to have a player ID being equal to his position in the list. This is basically, impossible, or possible but VERY error prone.
There are many ways to go around the problem, the one I use for my server stuff is to have the player ID as part of the Player data structure. My player list is a std::vector and then I have an helper function Player* getPlayerFromID(unsigned char id); that looks for a player with that ID in my list and returns it. A map might work better if you discover that your getPlayerFromID is a bottleneck.. personally it isn't so don't bother.
The other convoluted way would be to reassign IDs every time a player is added or removed to the list. The adding part is easy because it only involves the currently added player.. the removing is tricky because you'll have to notify all your clients "hey your player ID has changed".. which I think it's just a bad idea.
Assuming you won't have more than 256 player online, you can use a plain old array, like Player *players[256].
Memset to zero and you'll have all players=NULL.
When a player connect, scan the array and look for the first NULL; when a player leave, delete players[id], and set players[id]=null
Memset to zero and you'll have all players=NULL.
When a player connect, scan the array and look for the first NULL; when a player leave, delete players[id], and set players[id]=null
Author
Assuming you won't have more than 256 player online, you can use a plain old array, like Player *players[256].
Memset to zero and you'll have all players=NULL.
When a player connect, scan the array and look for the first NULL; when a player leave, delete players[id], and set players[id]=null
Honestly this was the simplest solution!
Just a quick question though, here's my removePlayer function:
int PlayerList::removePlayer(unsigned int id){
//std::cout << "===REMOVING PLAYER===" << std::endl;
if(players[id] != NULL){
delete players[id];
players[id] = NULL;
}
return 0;
}
It works when I remove the line "delete players[id];"
For some reason when I leave that in, I get an exception when I try to execute removePlayer(2) for example.
It says the heap gets corrupted.
Why is that? And also, why do I need to delete that piece of memory, because when I set it to NULL, the reference is gone anyways?
Or would the player instance still be in memory? If that's the case, how do I get around this issue where it corrupts the heap?
You need to post more of your code if you want us to tell you why it's crashing. 
Are you calling 'delete' on something you haven't called 'new' on?
Why are you using 'new' or 'delete' at all here? Even smart pointers here is probably unnecessary.
Your code probably shouldn't contain any occurrences of new or delete.
In C++, the 'new' keyword and the 'delete' keyword do things very differently from other languages you might have used, and need to be handled with care.
Why do you need to delete the memory of your players, anyway? Why not just mark them (with a bool) as not in use?
Why are your Player structs dynamically allocated instead of on the stack?
If you use C++11, which you are if you have access to std::unique_ptr, then you should use nullptr, not NULL. There is a reason for that.
I mentioned earlier, why does the order in memory of your players matter?
Why are you using a single variable (the ID) to handle more than one purpose (position in memory and identification)?
You definitely don't need a custom container.
Here's my suggested solution: std::unordered_map<PlayerID, Player>
If you want to *remove* a player from memory (why?), call myMap.erase(playerID).
But if you want to preserve the prior existence of a player, just use a boolean to mark him as no longer active. 
myMap.at(playerID).isActive = false;For new IDs, just hold the last used ID and increment it. Don't let the current size of your containers dictate your IDs if you're going to be removing elements.
PlayerID newPlayerID = ++lastUsedPlayerID;
I don't understand this new trend against new and delete. In a simple scenario like this, there's no reason not to use new and delete, it's ABC of programming with c++. If you can't handle a simple array of pointers, you have no hope with c++. It's a good exercise for the OP and understanding why he gets an error when deleting a player will save him a lot of troubles in the future.
@burden: your remove fn is correct, the error probably is in your create function
@burden: your remove fn is correct, the error probably is in your create function
I don't understand this new trend against new and delete. In a simple scenario like this, there's no reason not to use new and delete, it's ABC of programming with c++. If you can't handle a simple array of pointers, you have no hope with c++. It's a good exercise for the OP and understanding why he gets an error when deleting a player will save him a lot of troubles in the future.
It's the other way round, there's no reason to use new and delete here.
Author
Thank you all for the suggestions.
I'm going to go with Giallanon's suggestion, was very simple and it works!
Thanks guys.
I'm surprised that no one has mentioned pools in this thread -- they're very common in games and used for this purpose. It's basically an array combined with a free-list.
Normally a pool would be a container like a std::vector that stores data, but here's the basic version of the idea, which only manages handing out ID's, and not actually containing objects.
n.b. all code in this thread is written off the top of my head - may not compile or work...
Normally a pool would be a container like a std::vector that stores data, but here's the basic version of the idea, which only manages handing out ID's, and not actually containing objects.
n.b. all code in this thread is written off the top of my head - may not compile or work...
struct IdPool : NonCopyable
{
public:
IdPool( int capacity )
: nodes((Node*)malloc(sizeof(Node)*capacity)
, free(0)//point to the first node
{
assert( capacity );
//link each node to the next
for( int i=0, end=capacity-1; i!=end; ++i )
nodes[i].next = i+1;
nodes[capacity-1].next = -1;//terminate the list (-1 == null pointer)
}
~IdPool()
{
free(nodes)
}
int Alloc() // returns -1 if full, otherwise gives out an ID
{//try and pop one from the front of the free-list
int id = free;
if( free != -1 )
free = nodes[free].next;
return id;
}
void Free(int id)
{//push this node to the front of the free-list
nodes[id].next = free;
free = id;
}
private:
struct Node { int next; }//holds the index of the next free node
Node* nodes;
int free;//ID of the first unused node
};template<class T> struct Pool
{
Pool( int capacity )
: m_pool(capacity)
, m_data(capacity)
{
m_data.resize(capacity);
}
T* Alloc()
{
int id = m_pool.Alloc();
if( id == -1 )
return 0;
return &m_data[id];
}
void Free(T* obj)
{
int id = obj - &m_data[0];
assert( id>=0 && id<m_data.size() );
m_pool.Free(id);
}
private:
IdPool m_pool;
std::vector<T> m_data;
}...
: nodes((Node*)malloc( max(sizeof(Node),sizeof(T)) * capacity )
...
struct Node
{
int next;
void Alloc() { new(this) T(); }
void Free () { ((T*)this)->~T(); }
}
Node& At(int index)//use At(index) instead of nodes[index] now
{
const int nodeSize = max(sizeof(Node),sizeof(T));
return *(Node*)(((char*)nodes) + index * nodeSize);
}
public:
T* Alloc()
{
if( free == -1 )
return NULL;
Node& node = At(free);
free = node.next;
node.Alloc();//construct the object
return &node;
}
void Free(T* obj)
{
int id = obj - (T*)&nodes;
Node& node = At(id);
node.Free();//destruct the object
node.next = free;
free = id;
}
...I don't understand this new trend against new and delete.
[...]
it's ABC of programming with c++. If you can't handle a simple array of pointers, you have no hope with c++.
The 'trend' against new and delete is advocated by very experienced programmers, not just myself. Many of the people advocating against over-use of new and delete, are the people on the C++ standards committee. Granted, saying "person X is accredited must mean he's correct" is silly, but my point is, these people literally wrote the book(s) on new and delete. It's not that they, or myself, "can't handle a simple array of pointers" or "have no hope with c++", they created (and continue to develop) C++ and the C++ best-practices and advocate against unnecessary use of new and delete. 
[Here's one example - there are many other examples by many other C++ experts]
For example, a std::vector already dynamically allocates its memory. So you are stack-allocating a pointer to dynamically allocated memory to store an array of pointers to more dynamically allocated chunks of memory. Bwah? Why not just make that stack-allocated pointer point to the dynamically allocated memory directly? Unless, for performance reasons, you decide that you like distributing every allocated element into a different location in memory to prevent the CPU from effectively fetching them in bulk, and you actually like having to deallocate two pointers instead of one for every element read or write access.
...why not do it in the safer, faster, simpler, cleaner way?
Another example is, if new (or a new equivalent) is getting called once already in the vector, why call it once more for every element? It's the difference between 1 new call verses 1 new call and an additional new call for every element (i.e. for 50 elements, that means it's 1 call to new verses 51 calls to new), when the additional new calls add no benefit (in this situation).
I'm not advocating pre-optimization either - but I'm saying doing things the "right" way often leads, as a side-effect, to faster code anyway - or rather, to less bloated code.
If a container is holding pointers, then new and delete is something added on top of the container. I'm saying, wait a minute, why are we adding new and delete into a situation that doesn't benefit, and actually suffers, from the addition of it?
There are scenarios where new and delete are the best choice. I'm fairly confident that this is not one of them.
New and delete shouldn't be the default go-to choice - it's the last choice of a list of better possibilities.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
