WTF!!! Rendering System Broken

[Chris]

There's a bug in Actors::removeCell() in actors.cpp regarding the iterator usage there. You need to change the code to something like

Code: Select all

    for(std::map<MWWorld::Ptr, Animation*>::iterator iter = mAllActors.begin(); iter != mAllActors.end();)
    {
      if(iter->first.getCell() == store){
        delete iter->second;
        mAllActors.erase(iter++);
      } else {
        ++iter;
      }
    }

to not invalidate the iterator. The incorrect code caused a crash on cell-change in gdb but wasn't crashing without a debugger attached.

Shouldn't that be

Code: Select all

iter = mAllActors.erase(iter);

? Existing iterators may not remain valid after erase/insert/etc, and erase() returns the next iterator after the one that was removed.

[Zini]

Nope. The iterator is incremented before erase is called. All good.

[Yacoby]

Nope. The iterator is incremented before erase is called. All good.

I was like what? No. And then I double checked where the sequence point is. That is really really subtle.

[Zini]

I love my C++

[raevol]

Sounds like a good candidate for some quality code commenting.

[Zini]

Actually, that is more or less a standard technique. And if you have seen it once, it's pretty obvious.

[Yacoby]

Yup. It is just a bit counter intuitive on first sight (i.e. it doesn't work as "expected").

[psi29a]

Inline code execution, it is like the 'order of operations' in code.

[Chris]

Nope. The iterator is incremented before erase is called. All good.

Hmm, I always worked under the assumption that all preexisting iterators become invalid after the number of elements in a container changes, regardless of what element it references.

Looking at some online C++ references, it seems different container types have different rules about what iterators become invalid after an erase/insert... which I think kinda reenforces the point. E.g., if the map is later changed to a vector, that iterator will potentially become invalid (depending on implementation details), but it can be an intermittent, hard-to-track error. If it's changed to another container type, who knows how this code will react.

Just my thoughts on the matter, anyway. You're right, though, that it seems the current code is valid (for C++11 at least, anyway).