Player object vs. player name?

[sorcerykid]

I've long wondered why nearly all player-specific API functions of Minetest expect a player name as an argument, yet a player object is provided to callbacks.

Callbacks

Player object provided
minetest.register_on_dieplayer( )
minetest.register_on_dignode( )
minetest.register_on_player_hpchange( )
minetest.register_on_item_eat( )
minetest.register_on_joinplayer( )
minetest.register_on_newplayer( )
minetest.register_on_player_receive_fields( )
minetest.register_on_punchnode( )
minetest.register_on_respawnplayer( )
nodedef.on_place( )
nodedef.on_drop( )
nodedef.on_use( )
nodedef.on_dig( )
nodedef.on_punch( )
nodedef.on_receive_fields( )
nodedef.allow_metadata_inventory_move( )
nodedef.allow_metadata_inventory_put( )
nodedef.allow_metadata_inventory_take( )
nodedef.on_metadata_inventory_move( )
nodedef.on_metadata_inventory_put( )
nodedef.on_metadata_inventory_take( )
nodedef.after_place_node( )
nodedef.can_dig( )
nodedef.after_dig_node( )
nodedef.on_rightclick( )

Player name provided
minetest.register_on_protection_violation( )

API Functions

Player name expected
minetest.get_player_by_name( )
minetest.ban_player( )
minetest.chat_send_player( )
minetest.create_detached_inventory( )
minetest.get_player_information( )
minetest.get_player_ip( )
minetest.get_player_radius_area( )
minetest.is_protected( )
minetest.kick_player( )
minetest.player_exists( )
minetest.record_protection_violation( )
minetest.show_formspec( )
minetest.set_player_password( )
minetest.sound_play( )

Player name or object are expected
minetest.check_player_privs( )
minetest.get_player_privs( )
minetest.set_player_privs( )
minetest.add_particlespawner( )

Since player objects are only used for manipulating or examining player properties, it necessitates repeatedly converting to a player name before being able to use any of the API functions above. Wouldn't it be more consistent if the player name was passed to the callbacks instead? Or, in the very least, maybe the player object could contain a "name" field to avoid this redundancy altogether?

Right now most mods have callbacks that look somewht like this:

Code: Select all

minetest.register_on_joinplayer( function( player )
     if minetest.check_player_privs( player:get_player_name( ) ) then ... end
     :
     minetest.chat_send_player( player:get_player_name( ) )
     :
end )

Of course it's entirely possible to create a local a variable, like player_name, in this situation. But that's kind of redundant in its own right, since the variable is only needed for two API calls at which point any marginal gain of readability and efficiency is offset by the extra variable declaration and assignment. In larger subgames where there may be upwards of 50+ registered callbacks, it really does add up.

If the name were a field of the object, then I think source code would be much easier to follow and maintain.

Code: Select all

minetest.register_on_joinplayer( function( player )
     if minetest.check_player_privs( player.name ) then ... end
     :
     minetest.chat_send_player( player.name )
     :
end )

Just some thoughts and suggestions. Thanks for reading!

[stu]

It seems fairly simple to me. If a function only requires a player name then that would be preferred over passing around potentially stale userdata references, though I agree there should at least be some consistency in the api functions. For callbacks it is probably more useful to provide a valid player object.

In short, api functions should require the minimum amount of information while callbacks should provide the maximum amount.

[Linuxdirk]

It seems fairly simple to me. If a function only requires a player name then that would be preferred over passing around potentially stale userdata references

So instead of passing an easily validatable user reference (just do if player ~= nil to make sure the player is still online) you prefer to pass a random string?

[stu]

It seems fairly simple to me. If a function only requires a player name then that would be preferred over passing around potentially stale userdata references

So instead of passing an easily validatable user reference (just do if player ~= nil to make sure the player is still online) you prefer to pass a random string?

`player ~= nil` just tells you the reference is valid, the player could have logged out for all Lua cares :P

[Linuxdirk]

`player ~= nil` just tells you the reference is valid

The dev wiki says about the player object: “These are players in the game.” so it’s safe to assume that a valid player reference references to an actually logged on player.

[rubenwardy]

stu is right, a player reference may be present but invalid if the reference is held until after the player logs out. This is because a player reference is basically a pointer to the player object in C++.

[Linuxdirk]

So it’s more reliable to only provide a random string that is checked if it is a player name?

Code: Select all

local do_stuff = function (name)
    local player = minetest.get_player_by_name(name)
    if player == nil then return false end
    -- Do something with `player` here
end

Like so?

[stu]

So it’s more reliable to only provide a random string that is checked if it is a player name?

Code: Select all

local do_stuff = function (name)
    local player = minetest.get_player_by_name(name)
    if player == nil then return false end
    -- Do something with `player` here
end

Like so?

Indeed, that is probably the best way to ensure the player is valid at the time of execution. Testing the validity of `userdata` can be somewhat non-trivial: https://github.com/minetest/minetest/issues/6791 strings are also much easier to store in staticdata or metadata, for example.

[sorcerykid]

If a function only requires a player name then that would be preferred over passing around potentially stale userdata references

Thanks for the response. It seems from this discussion, that relying on player names is preferable to passing around player objects. Moreover, many callbacks probably can work perfectly fine without ever needing access to the player object. And the player object must always be cross referenced back to a player name in order to do anything API-related whatsoever.

It's also trivial to obtain a player object, if needed. It just requires maintaining an associative array of player names to object references, which is what I do in quite a few of my mods:

Code: Select all

player_obj = players[ player_name ]

vs.

player_obj = minetest.get_player_by_name( player_name )

Therefore, I don't see an overwhelming advantage to callbacks being provided an object rather than simply a player name, which is a far more portable representation as you pointed out (e.g. node meta, json, etc.)

Of course, the object references aren't entirely bad, per se. I just wish there was a more concise means of cross referencing the player name, ideally via a field of the object itself. Having to call a function to obtain the value of an immutable property (player names are guaranteed to never change during a session) seems excessive and redundant. That would be analogous to having a function GetBooleanValueTrue( ) that always returns the boolean value of true ;P

[rubenwardy]

With that you have to maintain a list of players, and ensure it is kept correct.

This is technically better than an array:

Code: Select all

local gplayer = minetest.get_player_by_name
gplayer(player_name)

But with a good editor/IDE it's better to have expressive and readable code than short code, so minetest.get_player_by_name is better



The main lesson is to never store a player reference between callbacks. Instead obtain references using the get player function or from callbacks. Generally the API accepts a player object for things that require the player object, although you have pointed out some weirdness. It's probably better for the functions to all accept either