cuberite icon indicating copy to clipboard operation
cuberite copied to clipboard

Investigate sol2 viability to replace ToLua++

Open madmaxoft opened this issue 7 years ago • 19 comments

The sol2 library ( https://github.com/ThePhD/sol2 ) seems like a very workable piece of code that provides a lot of features and bonuses over ToLua++. We should investigate whether we could replace our ToLua++ with it.

Points to research:

  • [x] Is it easy enough to fit into our cmake system? They provide something they call a single-header, is it usable in our scenario?
  • [x] Are the bindings it generates visible in the Lua code? (i.e. will APIDump still work?) If they use __index metamethods to provide the bindings, it's a show-stopper.
  • [ ] Can (most of) our big clumsy manual bindings be dropped in favor of simple sol2 bindings?
  • [ ] How large an increase in compile time will this replacement cost us? That's a fairly common complaint about the lib. We've got 112 classes, 1551 functions, 51 variables and 1706 constants in current API.

Any more points? @peterbell10 @tigerw ?

madmaxoft avatar Aug 01 '18 18:08 madmaxoft

I've had a read through some of the docs and tried using the library for a few bindings and I've got some answers but still lots of questions.

  1. It seems that the single-header is already in the repo and doesn't require any fancy build steps so all we need to do is set the right include path.
  2. I found this example showing you can bind custom __index metamethods so they must not be using it themselves. https://github.com/ThePhD/sol2/blob/develop/examples/index_and_newindex_usertype.cpp
  3. I ran into an issue that sol2 won't allow numeric values to be coerced into strings as lua_tostring would. This could be worked around but it suggests then need for caution before just dropping the current manual bindings.

Another thing I would like to know is how customisable the error messages are when binding functions. Their stack API lets you pass an error handler to sol::stack::check but I can't see a way to get the same functionality for automatically bound functions.

peterbell10 avatar Aug 02 '18 14:08 peterbell10

As for 2, ideally make a short sample program that binds one member function in one class, then try dumping all the members that Lua reports:

for i, v in pairs(a_ClassObj) do
	print(i, tostring(v));
end
print("meta:")
for i, v in pairs(getmetatable(a_ClassObj) or {}) do
	print(i, tostring(v));
end

I've added a new question, compilation time increase. Perhaps try simulating our API size (with some synthetic data of similar object counts) and see how long it compiles; compare that to recompiling bindings (touch src/Bindings/AllToLua.pkg and all the ManualBindings*.cpp files).

madmaxoft avatar Aug 02 '18 15:08 madmaxoft

I don't quite understand your point 3, what does that mean to us?

madmaxoft avatar Aug 02 '18 15:08 madmaxoft

The prefabs specifically rely on this coercion, if you rewrite the cLuaState::GetStackValue for AString then they fail to load completely. Whether this was intentional or not though...

peterbell10 avatar Aug 02 '18 15:08 peterbell10

Could you give me an example, I still don't see where the coercion is used.

madmaxoft avatar Aug 02 '18 15:08 madmaxoft

This was where I was seeing errors: https://github.com/cuberite/cuberite/blob/57690b81a24a29d70cb6f4196a6e0f521a3cb61b/src/Generating/PrefabPiecePool.cpp#L497-L498

"Direction" in the cubeset files are numbers which get coerced into strings when getting them from the stack by calling lua_tostring.

peterbell10 avatar Aug 02 '18 15:08 peterbell10

Okay, I bound a new type with sol2:

sol::state_view sv(tolua_S);
struct S { int member; };
sv.new_usertype<S>("S",
	"foo", [] { FLOG("Hello World"); },
	"bar", [] { return 20; },
	"member", &S::member,
	"print", [](AString s) { FLOG("{}", s); }
);

And after running your lua code I get this output:

__name  sol.cManualBindings::Bind::S.user
__eq    function: 0B514598
print   function: 0B514530
__type  table: 1397B398
__gc    function: 0B513E48
bar     function: 0B514460
__pairs function: 0B514A10
new     function: 0B513F18
foo     function: 0B514738
meta:
__type  table: 1397B398
__index function: 0AF0D0F8
__newindex      function: 0AF0CE00

peterbell10 avatar Aug 02 '18 15:08 peterbell10

So the code can see functions, but not variables (and probably no constants, either). That's only half-way bad :P I wonder how exactly they implement the variables / constants, if it could be hacked in our way...

madmaxoft avatar Aug 02 '18 15:08 madmaxoft

What does APIDump expect? I see that tolua++ uses ".get" and ".set" tables to implement properties.

peterbell10 avatar Aug 02 '18 16:08 peterbell10

Exactly that, but anything that is visible can be made to work. What I expect is that they actually implement variables and constants through their __newindex / __index metamethods. That isn't usable in APIDump, because it wouldn't know what names to query for in the first place.

madmaxoft avatar Aug 02 '18 16:08 madmaxoft

I think if we were using a newer lua version then __pairs would let you enumerate the variables. Would upgrading lua be feasible?

peterbell10 avatar Aug 02 '18 17:08 peterbell10

I've asked ThePhD directly about enumerating the variable names, he said there's no such thing built into sol2, we'd need to make something of our own. As for constants, they should be registered using sol::var: http://sol2.readthedocs.io/en/latest/api/var.html

I'll try to do something with the variables.

madmaxoft avatar Aug 03 '18 21:08 madmaxoft

With a bit of work, it should actually possible to mix tolua++ with sol2. We could just bind the variables with tolua_variable and bind the functions with sol2. It wouldn't be pretty but it should work.

peterbell10 avatar Aug 03 '18 21:08 peterbell10

I don't think mixing the two is a good idea.

Well, my template-fu is not up to the task, I need someone more skilled in the fine art of templates to do this. My line of thought is this: Instead of writing:

lua.new_usertype<cClass>("cClass",
  "var", &cClass::var
)

We could do:

template <typename Class, typename... Args>
void registerClass(sol::state & a_Lua, const char * a_ClassName, Args &&... a_Args)
{
	auto variableNames = collectVariableNames(std::forward<Args>(a_Args)...);
	a_Lua.new_usertype<Class>(a_ClassName, std::forward<Args>(a_Args)...);
	a_Lua[a_ClassName].set("__vars", variableNames);
}

registerClass<cClass>("cClass",
  "var", &cClass::var
);

The problem is I can't seem to code collectVariableNames, I tried using a simple recursive template, eating up the arguments by pairs, but I don't know how to exactly specify that a parameter is a pointer-to-member-variable. Perhaps there's a better way of doing this, but I can't see it.

madmaxoft avatar Aug 04 '18 07:08 madmaxoft

Here's a small repo for testing: https://github.com/madmaxoft/TestSol2

madmaxoft avatar Aug 04 '18 07:08 madmaxoft

Consider using std::is_member_object_pointer as that will handle const members as well.

peterbell10 avatar Aug 04 '18 08:08 peterbell10

Could you make a PR againt the test repo?

madmaxoft avatar Aug 04 '18 08:08 madmaxoft

I think we've been going about this the wrong way. We should use the current tolua + APIDump to dump all the API functions, constants and variables, then use that output to build an API description that would be both compiled into Cuberite and used in new APIDump to create the docs. So instead of writing C++ bindings and having APIDump query them, use a common API description to generate the C++ bindings and the docs.

madmaxoft avatar Apr 12 '20 11:04 madmaxoft

As tolua++ has been archived quite a while ago this needs to be done.

MightyFilipns avatar Jan 25 '25 19:01 MightyFilipns