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

Investigate sol2 viability to replace ToLua++ #4276

Open
madmaxoft opened this Issue Aug 1, 2018 · 17 comments

Comments

Projects
None yet
2 participants
@madmaxoft
Copy link
Member

madmaxoft commented Aug 1, 2018

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:

  • Is it easy enough to fit into our cmake system? They provide something they call a single-header, is it usable in our scenario?
  • 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 ?

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 2, 2018

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.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 2, 2018

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

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 2, 2018

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

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 2, 2018

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...

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 2, 2018

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

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 2, 2018

This was where I was seeing errors:

!a_LuaState.GetNamedValue("Direction", DirectionStr) ||
!cPiece::cConnector::StringToDirection(DirectionStr, Direction)

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

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 2, 2018

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
@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 2, 2018

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...

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 2, 2018

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

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 2, 2018

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.

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 2, 2018

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

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 3, 2018

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.

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 3, 2018

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.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 4, 2018

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

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 4, 2018

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

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 4, 2018

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

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 4, 2018

Could you make a PR againt the test repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment