Godot: Arrays/Dictionaries allow modifying contents during iteration

Created on 22 Sep 2019  路  6Comments  路  Source: godotengine/godot

Godot version:
2e065d8

Issue description:
While debugging my game, I bumped into this piece of code:

    for id in shared_objects:
        if not is_instance_valid(shared_objects[id]):
            shared_objects.erase(id)

Godot doesn't see any problem here, even though doing this will totally break iteration and skip some elements. IMO modifying collection during iteration should raise an error, because it's never an intended behavior and yields unexpected results.

bug confirmed gdscript

Most helpful comment

What's the proper way to rewrite that piece of code if editing arrays in loops is forbidden?

I use this code:

var invalid_objects := []

for id in shared_objects:
    if not is_instance_valid(shared_objects[id]):
        invalid_objects.append(id)

for id in invalid_objects:
    shared_objects.erase(id)

All 6 comments

I think this would be a GDScript-specific feature, for these reasons:

  • It concerns a partivu==cular language feature not present in all languages
  • This feature would require a check on every mutating operation so might add significant overhead in faster languages.

C# already have this. At least for standard (non-Godot) collections.

I do this but by looping backward so erasing won't mess up the indexing. What's the proper way to rewrite that piece of code if editing arrays in loops is forbidden?

Just raising a warning should be sufficient, no?

What's the proper way to rewrite that piece of code if editing arrays in loops is forbidden?

I use this code:

var invalid_objects := []

for id in shared_objects:
    if not is_instance_valid(shared_objects[id]):
        invalid_objects.append(id)

for id in invalid_objects:
    shared_objects.erase(id)

With arrays, it's easy to remove elements while iterating. It's just not as efficient to write in GDScript, because it requires a small tweak to the iterator:

    var i = 0
    while i < len(array):
        if not is_instance_valid(array[i]):
            array.remove(i)
        else:
            i += 1
````

Or, if order doesn't matter, this variant spares shifting every element (may matter for very large arrays, or if indexes matter cuz it's less housekeeping):

```gdscript
    var i = 0
    while i < len(array):
        if not is_instance_valid(array[i]):
            array[i] = array[-1]
            array.pop_back()
        else:
            i += 1

But when iterating dictionaries there is pretty much no workaround at that language level... unless you iterate a copy of the keys, or use a second array. It might be possible with maps based on a sorted tree but these don't exist in GDScript.

Still valid in db5ea78b7

Was this page helpful?
0 / 5 - 0 ratings