Skip to content
This repository was archived by the owner on Dec 29, 2024. It is now read-only.

(wip) Add the ability to push a global module to lua, close #197 #198

Closed
wants to merge 6 commits into from

Conversation

Fran6nd
Copy link

@Fran6nd Fran6nd commented Mar 9, 2024

Pull request of a possible implementation to fix #197.

It allows to push a global as follow:
push_module('my_module_name', [[function_name_1, function_1], [function_name_2, function_2]])
then we can use it in lua as follow:
my_module_name. function_name_1()
my_module_name. function_name_2()

It is not registered as a library as since the latest lua release it is a bit tricky.
This PR is WIP and intended for discussion. Yet no unit test and my understanding of Godot_luaAPI is still limited so things might be not ready to merge!

Commits will be squashed of course before any merge.

Thank you for Godot_luaAPI which is an awesome tool!

@Trey2k
Copy link
Member

Trey2k commented Mar 10, 2024

So my only concern with the current implementation is functionally this would behave the same as pushing an object or dictionary as a global with all the module methods.

Ideally we would implment it as an actual library that can be obtained via require if we are going to separate it. My only concern then us in unsure of a use case where you would not want the module already made available for the user buy they have access to require.

@Fran6nd
Copy link
Author

Fran6nd commented Mar 10, 2024

I do agree that making it available from require would be the best. I am working on it but for now I am unsuccessful.

My main issue is that we have to use luaL_requiref(L, module_name, push_fodule_function, true);.
However push_module_function needs to shaped as follow:
static int LuaState:: push_module_function(lua_State* L)
So we can't make it as a member of luaState.
So we need to like register all instances of luaState, then for each instance we do need to register all libraries by name and then having a function called for any instance of luaState able to load the right library into the right instance of luaState?

So libraries could look as follow:
static std::vector<std::tuple<LuaState * state, lua_State * L, std::vector<std::pair<String, Array>>>> gdLibraries;
And so static int LuaState:: push_module_function(lua_State* L) could do the job once called?

I did a promising prototyping tonight but gonna try further tomorrow.

But of course I am wondering if there would be any way to perform it in a less tricky way? As this would add really a lot of complexity. My initial version is much simpler though.

Anyway thank you for your answer.

@Fran6nd
Copy link
Author

Fran6nd commented Mar 11, 2024

It is pretty dirty but here is what I came up with.
You can only require Godot made libraries as the default require is disabled. I did not understand how. So I pushed my own which have the benefit of being sandboxed.
And I also think that:
LuaAPI::~LuaAPI() { lua_close(lState); }
could be moved into the destructor of luaState?
What do you think?

EDIT: I do not prevent yet the dev to register multiple libraries with the same name. So only the first one will be loaded.

@Trey2k
Copy link
Member

Trey2k commented Mar 11, 2024

It is pretty dirty but here is what I came up with. You can only require Godot made libraries as the default require is disabled. I did not understand how.

It's provided via the package library in lua

@Fran6nd
Copy link
Author

Fran6nd commented Mar 11, 2024

Thank you,

It is pretty dirty but here is what I came up with. You can only require Godot made libraries as the default require is disabled. I did not understand how.

It's provided via the package library in lua

Even by doing lua.bind_libraries(["base", "table", "string", "package"]) I still get this error using require:

ERROR 2: [LUA_ERRRUN - runtime error ]
[string "..."]:2: attempt to call a nil value (global 'require')
stack traceback:

So it remains mysterious to me!

@Trey2k
Copy link
Member

Trey2k commented Mar 12, 2024

Even by doing lua.bind_libraries(["base", "table", "string", "package"]) I still get this error using require:

ERROR 2: [LUA_ERRRUN - runtime error ]
[string "..."]:2: attempt to call a nil value (global 'require')
stack traceback:

So it remains mysterious to me!

I am curious as to what version of the addon you are using. I just tested on my end and this code runs without error.

extends Node

var lua: LuaAPI = LuaAPI.new()

func _ready():
	lua.bind_libraries(["base", "package"])
	
	var err: LuaError = lua.do_string("""
	require "base"
	print("test")
	""")
	if err is LuaError:
		print("ERROR %d: %s" % [err.type, err.message])
		return

@Fran6nd
Copy link
Author

Fran6nd commented Mar 14, 2024

I ran your code with the latest gdextension available on the GitHub page, and still have the issue.

Same on a brand new git clone, then submodule init, update from the repo.
Running on a Mac M1 (ARM).

⚠️Just tried on my windows laptop where it works fine. So something during compilation?
Gonna do further investigations...

extends Node

var lua: LuaAPI = LuaAPI.new()

func _ready():
	var err: LuaError = lua.bind_libraries(["base", "package"," sqfdq", "dsfds"])
	if err is LuaError:
		print("ERROR %d: %s" % [err.type, err.message])
		return	
	err = lua.do_string("""
	--require "base"
	print("test")
	""")
	print(err)
	if err is LuaError:
		print("ERROR %d: %s" % [err.type, err.message])
		return

This code give me that:

Godot Engine v4.3.dev4.official.df78c0636 - https://godotengine.org
OpenGL API 4.1 Metal - 88 - Compatibility - Using Device: Apple - Apple M1

ERROR 2: Library "base" does not exist.
--- Debugging process stopped ---

So it is like the problem is about all modules....
The failing points:

bool loadLuaLibrary(lua_State *L, String libraryName)

from:

lua_libraries.gen.cpp

@Trey2k
Copy link
Member

Trey2k commented Mar 14, 2024

Same on a brand new git clone, then submodule init, update from the repo. Running on a Mac M1 (ARM).

this is probably because of #193, we have #194 we should fix it but it was causing some other issues.

@Fran6nd
Copy link
Author

Fran6nd commented Mar 18, 2024

Hello sir! As I have been trying to use the #194 patch without success. Compilation ok but unable to load the project, I have done my own workaround Fix loadLuaLibrary for MAc OS which seems to compile fine and I have been able to test.
It is really simple and kinda ugly... But seems to do the job?
Furthermore, now require ' ' from package module can now load a gd made module.
However to keep being able to sandbox it I am thinking about some kind of a luaAPI.alllow_only_gd_packages()?

@Fran6nd Fran6nd force-pushed the main branch 2 times, most recently from b085238 to 2e1e283 Compare March 18, 2024 12:52
@SumianVoice
Copy link

Not sure if I'm misunderstanding something but this seems to be the same use case as push_variant as used on a table of functions or a node?

extends Node
var lua: LuaAPI = LuaAPI.new()
func message():
	print("hello world")
func _ready():
	lua.push_variant("MyNode", self)
	
	var err: LuaError = lua.do_string("""
	MyNode.message()
	""")

As described in the OP, it seems this does what you want? You could obviously obscure some functions by packing them into an array like:

	lua.push_variant("MyModule", [function_1, function_2])

And this would work the same way. Why are you specifically wanting to use require? I thought that for files you want to only selectively run.

For the stated use case, push_variant works as intended for it.

@Fran6nd
Copy link
Author

Fran6nd commented May 4, 2024

Hello Sir, sorry for the delay.
My point is to allow pushing like "toolboxes" to lua to allow the user to use what they want.
I also want to be able to push a module accessed like:

robot = require 'robot'
robot.head.tilt_right()
robot.motor.stop()

So I want to be able to have nested named tables of functions.
Not sure if that's clear?
Using 'require' is to do not push useless stuff to lua to avoid memory issues (In my case I am having to push a lot of tools). And it makes code more readable imo.
Anyway, maybe my use case is too a corner case?
Regarding the fix for Mac OS, my own (in the commit list) is working really well.

EDIT: Currently it works as follow:

GDSCRIPT
var lib = [["head", ["tilt_right", rb_tilt_r]], ["motor", ["stop", rb_stop]]]
lua.bind_gd_library("robot", lib)

@radiantgurl
Copy link
Contributor

radiantgurl commented May 22, 2024

This looks completely unnecessary to my point of view, as all you need to do is

api.push_variant("module_name",{
   "function_name_1":function_1,
   "function_name_2":function_2
})

and it is wayyy straight forward.

@Fran6nd
Copy link
Author

Fran6nd commented Sep 1, 2024

As you prefer... I agree to the fact that it is a lot of troubles for a luxury.
I needed this for one of my project but maybe no need to have it upstream?
Closing...

@Fran6nd Fran6nd closed this Sep 1, 2024
@radiantgurl
Copy link
Contributor

radiantgurl commented Sep 1, 2024

GDScript recently started to allow lua-like dictionary creation.

api.push_variant("module_name",{
   function_name_1=function_1,
   function_name_2=function_2
})

is now valid too.

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

Successfully merging this pull request may close these issues.

Feature Resuest: Pushing modules to lua
4 participants