-
Notifications
You must be signed in to change notification settings - Fork 120
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
Add library support to Luacontrollers #557
base: master
Are you sure you want to change the base?
Conversation
This allows mods to provide their own libraries that can be accessed from within a Luacontroller, for example to make working with advanced digilines peripherals somewhat easier. Libraries can be added to the mesecon.luacontroller_libraries table, and then the code running in the Luacontroller can use require() to request one. require() will return nil if the library is not present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for adding something like this, and for using require
.
But I don't see the need to change the envs of the funcs.
mesecons/util.lua
Outdated
for k, v in pairs(t) do | ||
if type(v) == "function" then | ||
local newfunc = v | ||
setfenv(newfunc, env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the new variable? newfunc
is still the same function as v
.
You are setting the env of v
. This might be useful for functions that use functions from the luacontroller env. But in general I think this is at least rather inconvenient. If you want to make the whole minetest
table accessible, like you suggested in the title comment, you will have to wrap nearly every function because otherwise it will not work, or crash because it's not a lua function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I misunderstood the function passing behavior in Lua here - after reading up on that some, I see I could clean this up a little. But the setfenv() is still needed (I can just do it on v) in order for the main purpose of these libraries - interacting with LuaC capabilities - to work. Shoving the entire "minetest" table in there was more of just sort of a wild suggestion and not really an intended use case.
mesecons_luacontroller/init.lua
Outdated
local function get_require(env) | ||
return function(name) | ||
if mesecon.luacontroller_libraries[name] then | ||
return mesecon.tablecopy_change_env(mesecon.luacontroller_libraries[name],env) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could make this more general by having library getter functions in mesecon.luacontroller_libraries
:
local function get_require(env) | |
return function(name) | |
if mesecon.luacontroller_libraries[name] then | |
return mesecon.tablecopy_change_env(mesecon.luacontroller_libraries[name],env) | |
end | |
end | |
end | |
local function get_require(env, pos) | |
return function(name) | |
if mesecon.luacontroller_libraries[name] then | |
return mesecon.luacontroller_libraries[name](env, vector.new(pos)) | |
end | |
end | |
end |
Note that I've also added pos
here. I think there was somewhen the request to be able to add some sort upgrades to the luacontroller, like a sensor node that is placed beneath the luacontroller, having the pos allows this.
Such a getter function could still change the env of their functions if it wants to. And it has to ensure there are no security flaws, but that's the case anyway, a simpler api without the deepcopy-with-env-change might make this even easier.
@@ -459,6 +459,16 @@ local function get_digiline_send(pos, itbl, send_warning) | |||
end | |||
end | |||
|
|||
mesecon.luacontroller_libraries = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have this mentioned somewhere, eg. in the code comment at the top of the file.
The envs need to be changed, otherwise the library functions are unable to access normal Luacontroller functionality (digiline_send(), interrupt(), event, pin, port, etc.) |
Yes I know. But if you use functions in |
Hmm... how about if I was to allow libraries to be registered either as a table or a function - if they're a table then it would do what ti does now, or if it's a function then it would be called as you suggested. This way you could have all sorts of advanced stuff like you suggested, whereas simple libraries like the ones I was more after don't need to specially handle the environment themselves. |
local function f1(c)
digiline_send(c, "bla")
end
local function f2(c)
f1(c.."2")
end
local function f3(p)
return minetest.get_node(p)
end
local get_node = minetest.get_node
local function f4(p)
return get_node(p)
end
local incl_both = false
mesecon.luacontroller_libraries["foo"] = {
f2 = f2,
f1 = incl_both and f1 or nil,
f3 = f3,
f4 = f4,
} In the luacontroller With the more general approach, it's more explicit, but not really more complex imo: mesecon.luacontroller_libraries["foo"] = function(env, pos)
local function f1(c)
env.digiline_send(c, "bla")
end
local function f2(c)
f1(c.."2")
end
local function f3(p)
return minetest.get_node(p)
end
local get_node = minetest.get_node
local function f4(p)
return get_node(p)
end
local incl_both = false
return {
f2 = f2,
f1 = incl_both and f1 or nil,
f3 = f3,
f4 = f4,
}
end You could allow both ways, but I wouldn't encourage using the one that sets the envs. Edit: Also, the standard luacontroller env functions use the normal env, so I could also reason with consistency. Edit2: The libraries you think of probably do something that could be done with the normal luacontroller functions (=> require instead of copy). Such things could be done by Btw. this PR fixes #428. |
I will admit that the current way is a little strange in regards to how the environment behaves, but it's the cleanest way I could come up with for libraries that just aim to do things that the LuaC could already do, just without copy/pasting a bunch - like the one for the touchscreen that just gives a bunch of functions that eventually call digiline_send(), or in #428 where the goal is just to supply (copies of) a table without any functions at all. Both of these are doable in the way you suggest, but with additional requirements (the mod registering it would need to supply a getter function). Give me a few minutes here and I'll see about coming up with a (hopefully) relatively clean way to just handle both. |
Libraries can now be registered as a function which will be called when the library is requested. This allows functionality such as libraries that behave differently depending on where the Luacontroller is (for example, a sensor of some sort that only works if the LuaC is next to it) as well as various initialization that the library may need to perform. Supplying a table is still supported and works as before.
Is this still relevant/active? I think this feature will open up a whole new ecosystem of pluggable libraries and i'm looking forward to that 👍 Can i help somehow here? Does this need further testing or documentation? |
I don't know - I pushed that last set of changes after the previous message and never heard anything about it after that. |
The string metatable sandbox should probably be mentioned in the documentation. Furthermore, |
fyi: this has been forked and implemented here: https://github.com/mt-mods/mesecons not sure if this is still relevant/feasible to merge here |
To elaborate on the string metatable point: Say the library function is called at mesecons/mesecons_luacontroller/init.lua Line 473 in 91e3b13
s:find with a pattern as an argument, for example, it will crash unexpectedly.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for the idea.
👎 for changing function environments. That will cause myriads of mysterious bugs.
Actually there are 2 kinds of libraries IMO:
- Controller-local, that only need to run “inside” the LuaC. These can be provided as code chunks and be run in the LuaC environment entirely.
- Controller-boundary, like digilines. These are, by definition, crossing a security boundary so simplistic things are non-solutions there. Time limit alone can wreak havoc on a perfect-looking solution.
if type(mesecon.luacontroller_libraries[name]) == "function" then | ||
return mesecon.luacontroller_libraries[name](env, pos) | ||
elseif type(mesecon.luacontroller_libraries[name]) == "table" then | ||
return mesecon.tablecopy_change_env(mesecon.luacontroller_libraries[name], env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repetitive mesecon.luacontroller_libraries[name]
This allows mods to provide their own libraries that can be accessed from within a Luacontroller, for example to make working with advanced digilines peripherals somewhat easier.
Libraries can be added to the mesecon.luacontroller_libraries table, and then the code running in the Luacontroller can use require() to request one. require() will return nil if the library is not present.
As some examples of where this could be useful: