Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lua connect and disconnect event handler bug #827

Open
zygzagZ opened this issue Mar 4, 2017 · 14 comments · May be fixed by #835
Open

Lua connect and disconnect event handler bug #827

zygzagZ opened this issue Mar 4, 2017 · 14 comments · May be fixed by #835
Labels

Comments

@zygzagZ
Copy link
Contributor

zygzagZ commented Mar 4, 2017

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
Copy link
Contributor Author

zygzagZ commented Mar 4, 2017

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
Copy link
Contributor Author

zygzagZ commented Mar 4, 2017

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
Copy link
Contributor

conde2 commented Mar 4, 2017

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
Copy link
Contributor Author

zygzagZ commented Mar 4, 2017

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++

@zygzagZ zygzagZ linked a pull request Apr 22, 2017 that will close this issue
@iryont iryont added the Bug label Apr 22, 2017
@iryont
Copy link
Collaborator

iryont commented Apr 22, 2017

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
Copy link
Contributor Author

zygzagZ commented Apr 22, 2017

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
Copy link
Owner

edubart commented Apr 24, 2017

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
Copy link
Contributor Author

zygzagZ commented Apr 24, 2017

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
Copy link
Collaborator

iryont commented Apr 24, 2017

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
Copy link
Contributor Author

zygzagZ commented Apr 24, 2017

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
Copy link
Collaborator

iryont commented Apr 24, 2017

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
Copy link
Contributor Author

zygzagZ commented Apr 24, 2017

Exactly, that's why I added createHandler

@danilopucci
Copy link

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

@andersonfaaria
Copy link
Contributor

@danilopucci I don't think this project has a maintainer anymore, would you mind talking to edubart and see if he could let you be one of the maintainers, or even better, try to switch to otland fork instead? https://github.com/otland/otclient there at least we have the staff constantly reviewing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants