OctoD / godot-gameplay-systems

⚔️ A plugin for Godot to create your gameplay systems in a Godot way.
MIT License
509 stars 49 forks source link

Remove abilities after grant loop #10

Closed Ariyn closed 1 year ago

Ariyn commented 1 year ago

Removing element during loop can miss some elements. (If there is 2 abilities, second ability will be left in abilities array without granted)

If element should be deleted during loop, other solution would be better than this one.

OctoD commented 1 year ago

Hi, thank you for the PR.

May I ask you in which scenarios a bug could be expected?

The PR aims to remove Ability resources from the abilities array when the node is ready, but not programmatically when the Ability is granted. This way, if you grant one or more abilities, there will be no check for duplicated abilities.

Another question: could this really expose the system to a bug?

The grant method checks first if the Ability can be granted, which checks if the Ability is already inside the granted_abilities and then proceeds to adding to this array while removing, if possible, from the abilities array (if given at mount time).

Thank you!

Ariyn commented 1 year ago

Hi, thank you for kind explanation. I think my PR needs more information, my apology. 🙇

Reason not to delete after grant

As you mentioned, I missed to remove ability after granted. I thought abilities can be added through inspector, not programmatically. It's my mistake.

Bug scenario

  1. Add 2 different abilities to same AbilityContainer node on godot inspector.
  2. When play the game, first ability on inspector works fine.
  3. Second ability does not works, because it is not granted.

So in this scenario, it happens like

var arr = [1,2,3,4,5]

for elem in arr:
    print(elem)
    arr.remove_at( arr.find(elem) )

# output
# 1
# 3
# 5

I think this PR should fix inside of _ready not grant. maybe copying abilities into other variable? Can you give me more your opinion?

Thank you!

OctoD commented 1 year ago

Silly me. Should never code when tired hahaha.

The _ready loop is wrong, it should iterate backward like this

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

This way all initial abilities will be granted properly.

You can just modify that loop this way, test it if it fits for you so I can merge, thank you!