[Bugfix] entity.die() during iteration over grid.entities invalidates list iterator #273

New issue

Closed

opened 2026-03-07 23:23:17 +00:00 by john · 0 comments

john commented

2026-03-07 23:23:17 +00:00

Owner

Summary

Calling entity.die() inside a for entity in grid.entities loop calls entities->erase(it) which invalidates the list iterator held by the Python iterator object. While the UIEntityCollectionIter does check for size changes, the iterator itself (self->current and self->end) can become invalidated by the erase operation.

Root Cause

UIEntity.cpp:761-788 (die method):

auto it = std::find_if(entities->begin(), entities->end(), ...);
if (it != entities->end()) {
    grid->spatial_hash.remove(self->data);
    entities->erase(it);          // Erases from the std::list
    self->data->grid.reset();
}

UIEntityCollectionIter stores iterators at construction time:

iterObj->current = self->data->begin();
iterObj->end = self->data->end();
iterObj->start_size = self->data->size();

For std::list, erasing an element only invalidates iterators to the erased element. So if die() erases the current element (the one the Python loop just yielded), self->current is fine (it was already advanced past the yielded element). But if die() erases a different element that happens to be self->end or the element self->current points to, the iterator is invalidated.

The size check (start_size comparison at line 27-31) will catch the size change and raise RuntimeError: EntityCollection changed size during iteration. However, this is a confusing error for users calling entity.die() in a loop.

Reproduction

import mcrfpy

grid = mcrfpy.Grid(grid_size=(10, 10))
for i in range(5):
    mcrfpy.Entity((i, 0), grid=grid)

# This will raise RuntimeError on the second iteration
for entity in grid.entities:
    entity.die()  # Removes from list, changes size

Impact

The size check prevents actual undefined behavior (which is good), but makes it impossible to iterate and remove entities. Users must collect entities first:

# Workaround
dead_entities = [e for e in grid.entities if should_die(e)]
for e in dead_entities:
    e.die()

Fix Options

Document the limitation: Add a note to die() documentation that it cannot be called during iteration
Deferred removal: die() marks the entity for removal; actual removal happens after the iteration completes
Safe iteration pattern: Provide grid.entities.remove_if(predicate) as a safe bulk removal API
Copy-based iteration: Change the iterator to iterate over a snapshot of the list

Option 2 (deferred removal) is the most user-friendly but most complex to implement. Option 1 is the pragmatic short-term fix.

Severity

Medium — does not cause crashes (size check catches the modification), but causes confusing RuntimeError for a natural game pattern (iterating entities and removing dead ones).

## Summary Calling `entity.die()` inside a `for entity in grid.entities` loop calls `entities->erase(it)` which invalidates the list iterator held by the Python iterator object. While the `UIEntityCollectionIter` does check for size changes, the iterator itself (`self->current` and `self->end`) can become invalidated by the erase operation. ## Root Cause `UIEntity.cpp:761-788` (die method): ```cpp auto it = std::find_if(entities->begin(), entities->end(), ...); if (it != entities->end()) { grid->spatial_hash.remove(self->data); entities->erase(it); // Erases from the std::list self->data->grid.reset(); } ``` `UIEntityCollectionIter` stores iterators at construction time: ```cpp iterObj->current = self->data->begin(); iterObj->end = self->data->end(); iterObj->start_size = self->data->size(); ``` For `std::list`, erasing an element only invalidates iterators to the erased element. So if `die()` erases the *current* element (the one the Python loop just yielded), `self->current` is fine (it was already advanced past the yielded element). But if `die()` erases a *different* element that happens to be `self->end` or the element `self->current` points to, the iterator is invalidated. The size check (`start_size` comparison at line 27-31) will catch the size change and raise `RuntimeError: EntityCollection changed size during iteration`. However, this is a confusing error for users calling `entity.die()` in a loop. ## Reproduction ```python import mcrfpy grid = mcrfpy.Grid(grid_size=(10, 10)) for i in range(5): mcrfpy.Entity((i, 0), grid=grid) # This will raise RuntimeError on the second iteration for entity in grid.entities: entity.die() # Removes from list, changes size ``` ## Impact The size check prevents actual undefined behavior (which is good), but makes it impossible to iterate and remove entities. Users must collect entities first: ```python # Workaround dead_entities = [e for e in grid.entities if should_die(e)] for e in dead_entities: e.die() ``` ## Fix Options 1. **Document the limitation**: Add a note to `die()` documentation that it cannot be called during iteration 2. **Deferred removal**: `die()` marks the entity for removal; actual removal happens after the iteration completes 3. **Safe iteration pattern**: Provide `grid.entities.remove_if(predicate)` as a safe bulk removal API 4. **Copy-based iteration**: Change the iterator to iterate over a snapshot of the list Option 2 (deferred removal) is the most user-friendly but most complex to implement. Option 1 is the pragmatic short-term fix. ## Severity **Medium** — does not cause crashes (size check catches the modification), but causes confusing `RuntimeError` for a natural game pattern (iterating entities and removing dead ones).