🐛 [bug] Ability Container fails to initialize if preloaded with multiple Abilities

[danielbernard]

Description

AbilityContainer removes entries during initialization while granting, causing it to lose abilities.

To Reproduce

Create an AbilityContainer, add multiple Ability entries in the editor, run the scene and observe what abilities remain in the abilities Array.

Details

I added a few unique abilities to an Ability Container manually in the editor, and upon running, I was receiving index errors.
Out of bounds get index '2' (on base: 'Array[Ability]')
I observed two possible issues in the code:

First, this section of the _ready function

godot-gameplay-systems/addons/godot_gameplay_systems/attributes_and_abilities/nodes/ability_container.gd

Lines 199 to 201 in d634781

	for i in abilities.size():
	var ability = abilities[i - 1]
	grant(ability)

the for i in abilities.size() will return [0 ... n] rather than [1 ... n], so there's no need to subtract 1. This leads to accessing the last element of the array first.
Removing the -1 or replacing this with the following should let it process in order.:

 for ability in abilities:
 	grant(ability) 

Note that replacing the index iteration with objects allows the code to execute without errors, but the ability array is still missing entries even if the array changes shape (as occurs in the grant function).

Digging into the indexing error, I found that you get odd behavior when the grant(ability) call is performed; since it will remove the first instance of an ability in the AbilityContainer, it removes the entry currently being granted. This causes the original array shape to change, and the loop in the _ready function to get confused and skip entries.

godot-gameplay-systems/addons/godot_gameplay_systems/attributes_and_abilities/nodes/ability_container.gd

Lines 367 to 371 in d634781

	# Removes from abilities array if it's there. This avoids duplication which could lead to bugs.
	var ability_index = abilities.find(ability)
	if ability_index >= 0:
	abilities.remove_at(ability_index)

[danielbernard]

Oh; I just saw the commentary in #10.
The current design doesn't iterate in reverse, it hits the last element, then iterates back from 0.

To traverse in reverse, we could:

	for i in abilities.size():
		var ability = abilities[-i - 1]
		grant(ability) 

but iterating in reverse still gets mixed up. After the first grant/removal at -1, the end of the array is shortened, so -2 accesses the second to last element of the shortened array.

Switching to always access the last element works, if we're to assume that each entry will always be removed.

	for i in abilities.size():
		var ability = abilities[-1]
		grant(ability) 

I'm not sure if the removal (movement to granted_abilities, rather than simply adding the entry to the new array and only removing from abilities if there's a duplicate) is the expected behavior; I may be missing the way this is supposed to work during initialization.

[OctoD]

I agree with you, that system should be retouched a little bit, it's too clumsy to me now.

The main logic behind the use of these two arrays is that you could add abilities without granting them (a skill tree is a good example) and grant them after when some criteria are matched.

[danielbernard]

Makes sense! Good enough for now, and should be usable.
I'll get a little more familiar with using it and see if I think of anything.