Issue on moveevents EquipItem and DeEquipItem calls when equipping item with transformDeEquipTo

[lucasoares]

I've found a interesting issue regarding to the DeEquipItem and EquipItem movements implementations.. First of all, I think they are being called more than it should be. I debugged and when equipping a soft boots, for example, it is called this number of times:

[02:50:05.525] [MoveEvent::EquipItem] Called for: 6132
[02:50:05.526] [MoveEvent::EquipItem] Called for: 6132
[02:50:05.527] [Player::postAddNotification] Called for: 6132
[02:50:05.527] [MoveEvent::EquipItem] Called for: 6132
[02:50:05.527] [Player::postRemoveNotification] Called for: 6132
[02:50:05.527] [MoveEvent::DeEquipItem] Called for: 6132
[02:50:05.529] [Player::postAddNotification] Callled for: 2640
[02:50:05.529] [MoveEvent::EquipItem] Called for: 2640

It is called 3 times for the 6132 id and one for the 2640 (which is the transformation of the soft boots when being used).

The code flow in theory is:

1 - equip a soft boots
2 - calls equip item
3 - transform it to the item id of a soft boots being used which make:
3.a - calls the postAddNotification for the new item
3.b - calls the postRemoveNotification for the old item
4 - register abilities for the old item
6 - unregister abilities for the old item (because of the postRemoveNotification call)
6 - register abilities for the new item

That should leave only the abilities for the new item configured, right? But I think there is something wrong with how this is handled in the code.

The current code makes the abilities to be configured twice for the 6132 item and only removed once in the postRemoveNotification (which calls the DeEquipItem).

In my case this is a problem because I have a use-case scenario where both items have abilities (6132 and 2640). This makes the abilities to not work properly. For example, if both item ids have max HP stat of 200, each time the player equips this item it will add 400 HP instead of 200, but when de-equiping it, it will only remove 200 HP (the player will keep 200 extra HP even without the item).

The Movement EquipItem have a check (isItemAbilityEnabled) to avoid configuring abilities twice, but when both items (before transformation and after transformation) have abilities, this is not working. This check is made before the transformation and it is set only after the item transformation. This will make only a single call (of the three calls) to the EquipItem to not re-execute the abilities, but the other two will.

This can be easly reproduced by adding the following attribute for both item ids (6132 and 2640):

    <attribute key="maxhealthpoints" value="100"/>

The code that deals with these notifications to update item data is veeery difficult to read and to understand and I did not have time yet to deep dive into it.

I was able to find two ways to fix this, but I don't know which one is better oronly a single way to fix this but I don't know if it will bring any side effect:

1 - Remove the if(player->isItemAbilityEnabled(slot)) check from both EquipItem and DeEquipItem functions so it is executed always when called. I think this fix may be danger. The person that coded this probably added this check because stats were being duplicated (remember: the EquipItem is called THREE times when equipping an item with transformation).

2 - Do not register abilities for items that have it.transformEquipTo = true on the EquipItem method call (a simple return true after the transformation call) and also check for transformEquipTo on the DeEquipItem to not unregister abilities too. This will make only the item after transformation to register the abilities. This may be the best fix. I tested and it works perfectly. May need more testing.

PS.: this is a bug but only affects items with transformation and that have abilities configured for both item ids, which should not be something common. In my case I modified the code to allow to load abilitites directly from attributes to make it possible to make an item to add max HP (for example) directly from LUA using item attributes. Since transformations copies the attributes from/to the old/new item, both items ended up with abilities.