Unified get/set lua functions in luascript.cpp/h

[EvilHero90]

I've just recently stumbled upon that idea in regards of working with c# Why do we not unify the get/set methods into one in core logic and split them in compat.lua into it's fragments again. This could save us a lot of lines regarding get/set in core logic (faster compilation time) but still does not break backwards compatibility. This could also be referenced to #2169 I'm going here with a simple example on how we could achieve this

registerMethod("Player", "vocation", LuaScriptInterface::luaPlayerVocation);

int LuaScriptInterface::luaPlayerVocation(lua_State* L)
{
    // get: player:vocation() set: player:vocation(id or name or userdata)
    Player* player = getUserdata<Player>(L, 1);
    if (!player) {
        lua_pushnil(L);
        return 1;
    }

        // if we have one parameter only which is the userdata, then we expect the 'get' function
    if (lua_gettop(L) == 1) {
        pushUserdata<Vocation>(L, player->getVocation());
        setMetatable(L, -1, "Vocation");
        return 1;
    }

        // we have more than 1 parameter we expect the 'set' function
    Vocation* vocation;
    if (isNumber(L, 2)) {
        vocation = g_vocations.getVocation(getNumber<uint16_t>(L, 2));
    } else if (isString(L, 2)) {
        vocation = g_vocations.getVocation(g_vocations.getVocationId(getString(L, 2)));
    } else if (isUserdata(L, 2)) {
        vocation = getUserdata<Vocation>(L, 2);
    } else {
        vocation = nullptr;
    }

    if (!vocation) {
        pushBoolean(L, false);
        return 1;
    }

    player->setVocation(vocation->getId());
    pushBoolean(L, true);
    return 1;
}

compat.lua

function Player:getVocation() return self:vocation() end
function Player:setVocation(voc) return self:vocation(voc) end

compared to:

registerMethod("Player", "getVocation", LuaScriptInterface::luaPlayerGetVocation);
registerMethod("Player", "setVocation", LuaScriptInterface::luaPlayerSetVocation);

int LuaScriptInterface::luaPlayerGetVocation(lua_State* L)
{
    // player:getVocation()
    Player* player = getUserdata<Player>(L, 1);
    if (player) {
        pushUserdata<Vocation>(L, player->getVocation());
        setMetatable(L, -1, "Vocation");
    } else {
        lua_pushnil(L);
    }
    return 1;
}

int LuaScriptInterface::luaPlayerSetVocation(lua_State* L)
{
    // player:setVocation(id or name or userdata)
    Player* player = getUserdata<Player>(L, 1);
    if (!player) {
        lua_pushnil(L);
        return 1;
    }

    Vocation* vocation;
    if (isNumber(L, 2)) {
        vocation = g_vocations.getVocation(getNumber<uint16_t>(L, 2));
    } else if (isString(L, 2)) {
        vocation = g_vocations.getVocation(g_vocations.getVocationId(getString(L, 2)));
    } else if (isUserdata(L, 2)) {
        vocation = getUserdata<Vocation>(L, 2);
    } else {
        vocation = nullptr;
    }

    if (!vocation) {
        pushBoolean(L, false);
        return 1;
    }

    player->setVocation(vocation->getId());
    pushBoolean(L, true);
    return 1;
}

[Kamenuvol]

I would prefer another unification system using enums for each data: player:setVocation(voc) -> player:setValue(PLAYER_VOCATION, voc) player:getVocation() -> player:getValue(PLAYER_VOCATION)

[EvilHero90]

I don't really get the idea of what you're suggesting, could you provide a practical example of why this could be any better?

[ranisalt]

I like that. Remembers my C++ patterns (overloading instead of get and set) and somewhat how I like to work with Python (@property)

[djarek]

@Kamenuvol I don't particularly like that, because it's still verbose and requires matching on the engine side... If we don't want to have explicit lua functions for each property, might as well override the indexing metafunctions so that player.vocation = voc dispatches to the setVocation function.

[EvilHero90]

first implementation of such can be seen in this commit

[ranisalt]

might as well override the indexing metafunctions

Didn't know that was possible with Lua. Way to go, certainly.

[djarek]

Here's an example:

mt = {
    __index = function(self, key)
            if key == 'b' then
                return self:getValue()
            else
                return nil
            end
        end,
    __newindex = function(self, key, value)
            if key == 'b' then
                return self:setValue(value)
            else
                return nil
            end
        end }

a = {}

function a:setValue(v)
    print('setting value to: ' .. (v or "nil"))
end

function a:getValue()
    print('getting value')
    return 42;
end

local old = setmetatable(a, mt)
print('a.b: ' .. (a.b or "nil"))
a.b = 1
print('a.b: ' .. (a.b or "nil"))
print('a.b: ' .. (a.b or "nil"))
a.b = 2
print('a.b: ' .. (a.b or "nil"))

We can take this approach (indexing metamethods dispatch to existing setters/getters implemented in the engine or the indexing metamethods can also be implemented on the C++ side, skipping the need for additional functions.

[EvilHero90]

Another approach (this was just a test in a lua compiler)

local test = {}
setmetatable(test, {
  __newindex = 
    function(self, key, value, ...)
      local str = key:sub(1,1):upper()..key:sub(2)
      rawset(self, "get"..str, value)
      rawset(self, "set"..str, value)
    end,
  __call =
    function(self)
      for k,v in pairs(self) do
        print(k,v)
      end
    end
})
local voc = 1
test.vocation = function(self, new)
  if new then
    voc = new
  end
  return print(voc)
end
test()
test:getVocation()
test:setVocation(10)
test:getVocation()

Will create the get and set functions automaticly from the metatable once the function is added to newindex or we just rely on call and just initialize it once at start to create the functions, both ways would work well.

[EvilHero90]

or we take this next level and make streams for direct access like:

local stream = player:stream()
stream.health = 200
stream.healthMax = 2000
player:upstream(stream)

[nekiro]

This would cause a lot of confusion imo. It's much more clear to cast getSomething and setSomething rather than something in both cases.

[ramon-bernardo]

I like @EvilHero90 approach. Let's avoid using getters and setters in favor of:

local player = Player()
player.health = 100
player.level = 2000
player.speed = 500

[Codinablack]

I like @EvilHero90 approach. Let's avoid using getters and setters in favor of:

local player = Player()
player.health = 100
player.level = 2000
player.speed = 500

and what happens when someone does this?

player.health = true

[ranisalt]

and what happens when someone does this?

player.health = true

The same thing that happens when they run player:setHealth(true)

[ramon-bernardo]

When player:setHealth(true) is set, the player is permanently disconnected from the server. They will have zero health and will never receive packets again, nor will they die.