Lua connect and disconnect event handler bug

[zygzagZ]

Hello.
There is a bug that sometimes makes handlers for LocalPlayer trigger for Creature events etc.
I.e. in a (not so) special case any handler bound to a derived class can be triggered by base class event.

Steps to reproduce:
bind lua handler to Creature.onPositionChange that does nothing
bind lua handler to LocalPlayer.onPositionChange(player) that prints player:getName()

Expected behaviour:
The only prints in console are local player's name.

Actual behaviour:
When any creature moves, it's name appears in console

The reason behind this is that connect function searches for list of signal handlers in any of base classes and if it hasn't found any, it creates one in current class.
The call to bind handler to LocalPlayer finds one in Creature (that is base class of LocalPlayer) and inserts event handler there, so calls by Creature events are also triggering LocalPlayer handlers.

The fix would be using rawget and rawset to always add trigger handler in current object, and in signalcall to iterate over metatables in order to trigger handlers of all base classes.

Please confirm that this is actually a bug, and not a feature.

[zygzagZ]

I am currently trying to reproduce this on latest commit

Adding the following snippet at the end of GameInterface.init makes it occur:
connect(Creature, { onPositionChange = function(c) print('Creature.onPositionChange: ' .. c:getName()) end }) addEvent(function() connect(LocalPlayer, { onPositionChange = function(c) print('LocalPlayer.onPositionChange: ' .. c:getName()) end }) end)
What happens:

Creature.onPositionChange: Player
LocalPlayer.onPositionChange: Player
Creature.onPositionChange: Eryn
LocalPlayer.onPositionChange: Eryn
Creature.onPositionChange: Player
LocalPlayer.onPositionChange: Player
Creature.onPositionChange: Eryn
LocalPlayer.onPositionChange: Eryn

[zygzagZ]

In order to fix this I would need to change LuaInterface::signalCall function to also call base classes event handlers or LuaInterface::registerClass, so that base class knows all classes that derive from it (so it can install event handlers in derived classes too).
I think that first approach is better.

[conde2]

Creature can be:
Monster
Player
Npc

Connecting Creature.onPositionChange will be triggered for any of the above cases, it is correct that it will be called for any creature movement.

But LocalPlayer.onPositionChange should be called just for LocalPlayer

Eryn is another player in another client ?

[zygzagZ]

Eryn is actually an NPC in tfs
The problem is that currently after connecting onPositionChange for Creature, every next connection to this event for Player, Npc or Monster will be connected to Creature.
By changing slightly connect function in util.lua I managed to circumvent this behaviour, but the events stopped propagating (if one handler is bound for localplayer, and the other is for creature, then the other won't be called for localplayer event)
I am looking for a way to iterate over metatables in c++

[iryont]

It is a bug. It happens because of __index method within metatable of all derived classes which fallbacks to the base class and thus gets the handler from it.

I did take a look at your fix and it seems to be quite extensive. However, considering the fact that it appears to work in reversed order (create a handler within derived class and then a next handler within base class) then I don't quite understand why it won't work with rawget and rawset instead. Anyway, I should be able to check the issue more closely tomorrow, so stay tuned.

Meanwhile I will summon two guys:

@edubart
@hrsantiago

[zygzagZ]

hasValue could be rewritten to be using rawget, but I don't see any point in it.
However only storing handlers inside derived class doesn't work, because then signalcall only gets the topmost callbacks table. That's why there's createHandler function that adds a handler to call signalcall on next base class that has any callbacks connected when an event occurs.

[edubart]

Indeed it's a bug, the fix could be much simplified and cleaner by using a rawget(object, signal) and no additional methods, but I will leave it for you guys to fix.

[zygzagZ]

If I understand the issue correctly only the hasValue(object, value) can be replaced with rawget.
The createHandler is still needed. Maybe it can be shortened a bit, but not got rid of.

[iryont]

He means replacing these two:

https://github.com/edubart/otclient/blob/master/modules/corelib/util.lua#L56
https://github.com/edubart/otclient/blob/master/modules/corelib/util.lua#L65

if not object[signal] then

=>

if not rawget(object, signal) then

I thought about the same, but I wasn't sure whether it will be enough. I didn't have time to test it yet.

[zygzagZ]

I don't think that these two changes are enough, because then base class event handlers would be skipped (if event happened for a player and had some handlers there, the event handlers for creature wouldn't be called)

[iryont]

Ah, I believe I do understand you now. You mean if local player triggers onPositionChange it should fire all handlers from both Creature and LocalPlayer, but instead it will only fire the top ones within derived class (LocalPlayer) leaving those from Creature. However, since local player is also a creature it should fire them all.

[zygzagZ]

Exactly, that's why I added createHandler

[danilopucci]

Seems that issue is solved in #835 which already has an open PR. Does anyone tested it? Is it ready to merge this PR?