Vector iterator not incrementable

[Callum5042]

I'm trying to get elements from a vector to get deleted once they hit a certain range.


.lib file

void Engine::DestroyObject(const std::string name)
{
	std::vector<Object>::const_iterator iObj = object["Bullet"].begin();
	try
	{
		object.at("Bullet").erase(iObj);
	}
	catch (std::out_of_range)
	{
		MessageBoxA(NULL, "Error: Out of range", "Error", MB_OK | MB_ICONERROR);
	}
}

Executeable

for (auto& obj : engine->object["Bullet"])
{
	obj.y -= 4;
	if (obj.y < -16)
	{
		engine->DestroyObject("Bullet");
	}
}

Now when I put a break; under the engine->DestroyObject("Bullet"); it will work, but it will too slow.

How would I make it work with out breaking the loop?

[pogrady]

I didn't realize that you could pass a string as an index to std::vector.

consider using std::map instead.

[cire]

I'm making the assumption that Engine::object is a std::map type, with a std::string as the key, and a std::vector of "base object pointers" representing objects of the type that corresponds with the key.

If that's the case, Engine::DestroyObject is almost certainly wrong. Anytime you use operator[] on a map and the key ("Bullet" in this case) doesn't exist, the key is added to the map, so object.at("Bullet") will never fail. It is entirely possible, however that the vector associated with that key is empty in which case invoking erase on it is likely to result in an out of range exception. Also, Engine::DestroyObject erases the only the first item in the vector. This is important.

The for loop suffers from a different problem. You iterate through the vector associated with the key "Bullet", updating y values of those objects and if the y value exceeds a certain threshold, you want to delete that item from the vector. But, you don't. What you actually do is call Engine::DestroyObject which erases the first item in the vector, not the item you actually wanted to remove (unless, by coincidence, it happened to be the first item in the vector.) Now, when you erase an item in a vector it also invalidates all iterators that point to that item or any items that occurred after it in the vector. Since you're erasing the first element, you invalidate all iterators into the vector.

Your range-based for loop was using an iterator into the vector. That iterator is invalid, but it continues to be used as if it wasn't.

Not good.

[Callum5042]

I'm confused on how a map would help here though,

and btw just some other information, Object is a structure, and object["Bullet"].begin(); is a map.

std::map<std::string, std::vector<Object>> object;

[cire]

Actually, that would make object["Bullet"].begin() a std::vector<Object>::iterator.

[Callum5042]

cire, I mena just object is a map, and I read through what you said above.

I changed a few things around, I relised that I had my argument not even related to the key.

void Engine::DestroyObject(const std::string name)
{
	std::vector<Object>::iterator iObj = object[name].begin();
	iObj = object[name].erase(iObj);
}

But with what you said have stumped me. I'm not sure how would be the best way to solve this.

You do know what I'm trying to do which is good, but I still don't understand why if I do this.

for (auto& obj : engine->object["Bullet"])
		{
			obj.y -= 4;
			if (obj.y < 64)
			{
				engine->DestroyObject("Bullet");
				break;
			}
		}

It works, but like I said it doesn't do it right, with it being slow and deleting the first element.

How would the best way be at doing this?

[cire]

You could implement the loop like so:

    auto & bullets = engine->object["Bullet"] ;  // for convenience.
    auto bullet = bullets.begin() ;
    while ( bullet != bullets.end() )
    {
        if ( (bullet->y -= 4) < -16 )
            bullet = bullets.erase(bullet) ;
        else
            ++bullet ;
    }

but you cannot use a range-based for, as the iterator must be updated within the loop.

[Callum5042]

Alright thanks, it works.