C++ read access violation when deleting object

Question

In my game, I've created (likely a very bad) Entity Manager that handles all entities in my game. It will assign random ID's to these entities and then add them to a map:

player = new Player(entityManager);
player->SetPosition(S2D::Vector2(50, 250));
entityManager->AddEntity(player);

---

//Adds entites to a list and assigns a random ID to them
void EntityManager::AddEntity(Entity* entity)
{
    int ID = rand();
    for(int i = 0; i < IDS.size(); i++)
    {
        if(IDS[i] == ID)
        {
            ID = rand();
            i = 0;
        }
        else
        {
            break;
        }
    }

    entity->SetID(ID);
    entities.insert(pair<int, Entity*>(ID, entity));
}

Each of these entities has a Update and Render function that is ran by the Entity Manager through it's Update and Render functions which are called by the Game Loop:

void EntityManager::Update(float deltaTime)
{
    if(!entities.empty())
    {
        //Removes entities that need to be deleted
        for(int i = 0; i < toDeleteEntities.size(); i++)
        {
            RemoveEntity(toDeleteEntities[i]);
            toDeleteEntities.erase(toDeleteEntities.begin() + i);
        }

    //Runs the Update function of all entities and adds Entites that need to be deleted to a list
        for(const auto entity : entities)
        {       
            if(entity.second->ShouldDestroy())
            {
                toDeleteEntities.push_back(entity.second);
            }

            entity.second->Update(deltaTime);
        }
    }
}

void EntityManager::Render()
{
    if(!entities.empty())
    {
        for(const auto entity : entities)
        {
            entity.second->Render();
        }
    }
}

As well as looping through each entity, the Update function also loops through a vector called "toDeleteEntities" which contains entities that are to be deleted. Each Entity has a boolean called "Destroy" and if it's set to true, the Update function in the Entity Manager will add that entity to the vector.

//Runs the Update function of all entities and adds Entites that need to be deleted to a list
    for(const auto entity : entities)
    {       
        if(entity.second->ShouldDestroy())
        {
            toDeleteEntities.push_back(entity.second);
        }

        entity.second->Update(deltaTime);
    }

Then before that for loop, the Update function will run this for loop that will remove each entity from the "toDeleteEntities" vector and then pass it onto another function that will delete it:

//Removes entities that need to be deleted
    for(int i = 0; i < toDeleteEntities.size(); i++)
    {
        RemoveEntity(toDeleteEntities[i]);
        toDeleteEntities.erase(toDeleteEntities.begin() + i);
    }

That delete function is this:

//Removes an Entity from the Game
void EntityManager::RemoveEntity(Entity* entity)
{
    if(entity->GetID() == -572662307)
    {
        return;
    }

    entities.erase(entity->GetID());
    delete entity;
}

The if Statement is there because of a previous error I had where I believe the for loop would pass the same entity to the delete function twice resulting in it trying to delete a already deleted object. I know it would be better to fix the error but truthfully it's easier to just do that XD.

Anyway hopefully that's enough context, if you need anymore then just ask. So all of this seems to work correctly somewhat with this Gem entity I have, it runs correctly and gets deleted without problems. But I've noticed when I attempt to add new entities and try and delete them, I get this _Pnext error:

And I just can't figure out why. It seems to be coming from this loop here in the xutility class:

Does anyone know why this error is happening?

EDIT Screenshot of the Call Stack:

Answer 1

Looks like the issue is your toDeleteEnemies loop. As you remove entries, your list will shrink, but your counter is still going up. So when you get past half way, you're now out of bounds of the array. You can also optimize this either way by just clearing the list after going through each entry, as you are only acting on each item once

Example:

for (auto entity : toDeleteEntities)
    RemoveEntity(entity);
toDeleteEntities.clear();

A few other suggestions, Use smart pointers, to avoid having to manually call delete (and end up with other potential memory bugs).

And, avoid hard coded numbers (magic numbers). Define them as constants somewhere and refer to them by name

One other note, if you are adding an item to the toDelete list, you probably don't need to call its update method afterwards