Fix crash when using pipeworks Node Breaker

[bewlay]

Filters the fake player used by the pipeworks mod.

Adresses #9. With the new player attributes storage, using the Node Breaker crashes the game (as the fake player has no get_attribute method) instead of depleting the player's stamina.

[sofar]

No, this is not a correct fix.

is_fake_player is not a proper API method or member of obj player.

Instead we should use player:is_player() instead. If that crashes, we should test that player.is_player is indeed defined, perhaps.

So I think you'll end up with something like this:

if player and player.is_player and player:is_player() then

[sofar]

Also, don't write this 4x. Instead, fix it at around line 84 or so once.

[bewlay]

Fair point about writing it only once. Checking for is_player() won't work with pipeworks, though, as they define is_player = delay(true) in their fake player definition (see here). Actually, exhaust_player() already checks for is_player().

[sofar]

How does this fix the crash? Or should I say, what is the actual crash message (backtrace) when this crash happens?

This patch is still wrong, we shouldn't blind test non-standard members of the player object.

My bet is that it still crashes in the same line.

if not player or not player.is_player or not player:is_player() then

that still looks like the proper solution to me.

[bewlay]

Well, both patches "work" (as in, I tested them -- without patch, crash; with patch, no crash). Maybe I misunderstood you, I thought you were merely talking about writing cleaner code.

To clarify how I see the situation: Pipeworks creates a fake player object and passes it to the digging callback. The fake player object doesn't implement some methods, specifically not get_attribute(). Calling that method in stamina causes the crash (see stacktrace below).

The game doesn't crash because of the call to player:is_player() as you seem to assume (?). The pipeworks fake player does implement is_player(), and it always returns true. Your proposal only adds not player.is_player to the checks already performed, which doesn't do anything in our situation, as we're not dealing with player objects without an is_player() method.

As the fake player's is_player() method always returns true, that is not a useful way to distinguish between normal and fake players. Maybe it should return false, I don't know enough about pipeworks to decide that. I didn't find any other methods in the official API on the wiki to determine whether a player is fake or not. The accepted way to do that seems to be to check for is_fake_player (Developer wiki, Issue comment).

Now, is_fake_player is no official API, in fact, I didn't even find API documentation for pipeworks specifically. Still, both pipeworks changing that part of the API and anyone using is_fake_player for something else seem unlikely to me. As for the "blind" testing: I'm quite new to lua, but as I understand it, any unassigned members are just nil, so that real player objects don't define that member is no problem. As I said, it would be a problem if at some point real player objects would define is_fake_player as any non-nil non-false value (most likely as a method, anything else would be dumb), but that would just make the fix non-functional (and wouldn't cause any crash).

I agree, though, that any official or well-documented way to do this would be better.

Stacktrace (some paths removed):

ServerError: AsyncErr: environment_Step: Runtime error from mod 'stamina' in callback environment_Step(): .../mods/stamina/init.lua:31: attempt to call method 'get_attribute' (a nil value)
stack traceback:
    .../mods/stamina/init.lua:31: in function <.../mods/stamina/init.lua:30>
    (tail call): ?
    .../mods/stamina/init.lua:94: in function 'exhaust_player'
    .../mods/stamina/init.lua:364: in function 'callback'
    ...games/minetest/minetest/bin/../builtin/game/item.lua:528: in function <...games/minetest/minetest/bin/../builtin/game/item.lua:453>
    (tail call): ?
    .../mods/pipeworks/wielder.lua:358: in function 'act'
    .../mods/pipeworks/wielder.lua:142: in function 'wielder_on'
    .../mods/pipeworks/wielder.lua:178: in function 'action_on'
    ...est/mods/minetest-mod-mesecons/mesecons/internal.lua:190: in function '?'
    .../mods/minetest-mod-mesecons/mesecons/actionqueue.lua:93: in function 'execute'
    .../mods/minetest-mod-mesecons/mesecons/actionqueue.lua:84: in function '?'
    ...s/minetest/minetest/bin/../builtin/game/register.lua:412: in function <...s/minetest/minetest/bin/../builtin/game/

[Ekdohibs]

As the original author of the pipeworks node breaker code, I can explain it here: the nodebreaker's is_player function has to return true, since a lot of code would instead assume that the nodebreaker is an entity and not a player. However, is_fake_player is provided, as a field and not a function, so that anyone can test for fake players in their code without having to worry whether pipeworks is installed or not: if something is not a nodebreaker's fake player, then is_fake_player will be nil. However, the nodebreaker should never have caused a crash because of calls to player: fields: the reason it does is that it that this code was written some time ago, and that the player API was extended since then, so it would need to be updated but I unfortunately haven't done it yet. Also note that the nodebreaker often masquerades as its owner, for protection purposes, so not checking that it is a fake player even when this bug is fixed could have strange consequences, like the nodebreaker exhausting its owner.

[sofar]

ok, now I understand

I consider this a bug in pipeworks.

If a node creates a fake player object, then it's not a player but a node. It's wrong that pipeworks therefore returns true in player:is_player() since that is, according to lua_api.txt the only way to tell code that the object ref is a valid PlayerSAO (or whatever it is called).

Therefore, I really think that pipeworks should make is_player() return false

This shouldn't matter for protection checks, you can still :get_player_name() and all, if pipeworks properly sets up those methods.

[Ekdohibs]

The problem is, :is_player() is also the only way to make the difference between players and entities. Therefore, you would want it to return true in many cases because if you don't, then some code you would want to be executed won't be. However, I agree there is a bug in pipeworks as well: namely, that its API is not up-to-date. The thing is, we want pipeworks nodebreakers to behave exactly like players would, except in the specific case when you want to explicitely make that distinction: that's what is_fake_player is for.

[sofar]

a7829a7

I'm solving it slightly different.

I really do not want to rely on is_fake_player since some other mod is going to make up something else.

However, what I really care about is :get|set_attribute(). So I just test for those explicitly. If they exist, even if fake, we assume it's a player.

Tested and behaves OK.