Skip to content
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

Lua scripts support #127

Merged
merged 26 commits into from Sep 10, 2016
Merged

Lua scripts support #127

merged 26 commits into from Sep 10, 2016

Conversation

smbas
Copy link
Contributor

@smbas smbas commented Aug 25, 2016

A basic support for the Lua scripts used in The Witcher. There is the ability to bind C++ to Lua and call Lua functions from C++.
No actual bindings are done, so.

The code is a bit raw, I think. So needs you feedback.

@smbas
Copy link
Contributor Author

smbas commented Aug 25, 2016

It doesn't build the toluapp directory for some reason. Did I miss something?
The builds on my machines are successful (on both Ubuntu 14.04 and Linux Mint 18).

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 25, 2016

It seems like you forgot to add the tolua directory to the SUBDIRS variables in the top-level Makefile.am: https://github.com/smbas/xoreos/blob/feature-witcher-lua-scripts/Makefile.am#L92

If you're building with CMake, that's not necessary, but the autotools build system needs it.

const int execResult = lua_dobuffer(_luaState, data, dataSize, path.c_str());
if (execResult != 0) {
warning("Failed to execute Lua file: %s", TypeMan.setFileType(path, kFileTypeLUC).c_str());
}
Copy link
Member

@DrMcCoy DrMcCoy Aug 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you're leaking both the original stream as well as the memStream here.

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 25, 2016

Nice job in getting it to compile now. :)

Could you split up the last commit (the "BUILD: Proper Lua includes") and rebase the fixes into the original commits? I.e. the Makefile.am changes should into the first commit, the one that adds the toluapp directory in the first place, and the include fixes into the commits that add those files.

That involves an interactive git rebase. If you don't know how to do that, just say so and I'll take care of it once the PR is ready to be merged.

/** Was the script subsystem successfully initialized? */
bool ready() const;

/** Execute a sctipt file. */
Copy link
Member

@DrMcCoy DrMcCoy Aug 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: s/sctipt/script/

Copy link
Member

@DrMcCoy DrMcCoy Aug 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you fixed the typo in a908f9b. If you could instead move the fix directly here (by using an interactive git rebase), i.e. have it look like you never made the typo at all, that would be better.

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 25, 2016

Apart from these issues here, your code is pretty clean and nice. :)

@clone2727 will add a few comments about the Lua parts later.

One thing, though, that's kinda missing in my eyes: actually using any of this in the Witcher engine. I.e. running any of the Lua scripts from within the game, even if it just results in a lot of warnings about missing functions, classes, what-have-you. As is, I can't really evaluate if any of this actually works.

I'm not sure yet how the Lua scripts are even called from within the engine, to be honest. I guess there are hooks for specific events? There's also the RunClientLua() NWScript engine function, which, I guess, would call ScriptManager::executeString()? These are mostly simple single-liners, though, and obviously not the brunt of the Lua functionality in the game. They also reference things that must have been set-up previously, either from full-fledged script or from the engine code. I for one have no clue. :)

@smbas
Copy link
Contributor Author

smbas commented Aug 25, 2016

Done. Hope I haven't missed anything.
I definitely need to build it using the autotools before pushing :)

Firstly, I don't know at the moment what scripts are called from the game. What I know is that there's the script "startup.luc", which call the others. And yes, RunClientLua() executes a string of Lua code.

To be able to execute Lua scripts we need to bind a lot of classes and functions. Without the bindings (or stubs at least) the execution will be interrupted immediately. Maybe there is a better way other than write stubs for every class and functions to make the scripts run, just skipping the actual logic. I haven't find any such way yet.

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 25, 2016

Done. Hope I haven't missed anything.

Looks good. :)

I don't know at the moment what scripts are called from the game

Ah, okay.

Maybe there is a better way other than write stubs for every class and functions to make the scripts run

Is there a way to add a generic "this is missing, just print its name" binding? Or I don't suppose you have lists of what is missing, so stubs could be added via a huge table, like what the NWScript system is doing?

I.e. basically what I would like to see, if possible, is kinda a mirror to the "TODO" warnings for unimplemented engine functions that the NWScript system does.

@smbas
Copy link
Contributor Author

smbas commented Aug 25, 2016

I'll work on stubs this weekend. It's a boring task, I said. Currently I just execute the startup script and see what's missing. Than write an appropriate stub. The problem here is that most stubs need to return a valid value in order to continue the execution of a script.

Also there are situations with an access to a nested variable or function. In this case Lua just throw the error "Can't access field ?". What field, who knows. You have just a question mark.
Happily there is a decompiler of Lua scripts. It makes such process a bit easier.

Currently, I would make something like this to bind some class to Lua:

class Control {
public:
    static void registerLuaBindings();

private:
    static void luaSomeMethod(lua_State *state);
};

But what about stubs? I think about creating a huge class LuaBindings, which have a lot of private classes that look like the above. Or better to create just plain functions with such signature, for example, addAbility_CDefs(). A name of a function + A name of a class. What do you think?

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 25, 2016

EDIT: Grml, why does GitHub disable Markdown for email replies...

I'll work on stubs this weekend.

Thanks. :)

Currently I start the execution of startup script and see what's missing. Than write an appropriate stub.

Yeah, that's what I feared. For NWScript, I was glad that the nwscript.nss (or nwscriptdefn.nss in The Witcher) files existed, so I could automate that.

Problem here is that most stubs need to return a valid value in order to continue the execution of a script.

Well, the NWScript engine functions that need to return something return an "empty" value, which evaluates to 0 for int, "" for string, etc. If the scripts actually need a real value, that won't work, yeah.

I think about creating a huge class LuaBindings, which have a lot of private classes that look like the above.

Hmm, yes, for stubbing, that's probably okay.

But I guess in the future, we probably want to implement them as real C++ classes with methods? Is that possible?

Otherwise, if we're creating them as plain functions, I'd rather have them in one C++ class with non-static methods even, so that when they're getting implemented, they can easily access game data. Similar to the Functions class in src/engines/witcher/script/functions.h. That works by using boost::function and boost::bind() to create a functor that binds the object to the function pointer as its this.

(Btw, I guess Witcher's script directory should be renamed to nwscript, then. And another directory, lua, would then contain the Lua side of things.)

@smbas
Copy link
Contributor Author

smbas commented Aug 26, 2016

But I guess in the future, we probably want to implement them as real C++ classes with methods? Is that possible?

Currently to do this you must create proxies to the actual methods of an object. As Lua only accepts functions with the signature: int func(lua_State *). So now we can create bindings only as static methods or free functions.

Maybe there more robust solution using C++ templates. Don't know about binding member functions, but I tried to bind a function with a custom signature that way. It looked like: Variables func(const Variables &).

@smbas
Copy link
Contributor Author

smbas commented Sep 1, 2016

Add stubs to allow execution of the global and startup scripts without errors. Not so a big win, because currently only the code from the global scope is executed. There are a lot of function that must be called from the engine.

Also I have implemented RunClientLua. But currently it's just throws exceptions that the global variable g_GuiInGame not found. It seems that the original engine has CGuiInGame::OnInitialize method, that call the appropriate code to initialize the GUI.

@@ -128,7 +128,7 @@ typedef LUA_UACNUMBER l_uacNumber;
** type for virtual-machine instructions
** must be an unsigned with (at least) 4 bytes (see details in lopcodes.h)
*/
typedef unsigned long Instruction;
typedef unsigned int Instruction;
Copy link
Contributor

@clone2727 clone2727 Sep 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a change to make it a fixed-width integer, it should just be uint32_t.

Copy link
Member

@DrMcCoy DrMcCoy Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, uint32_t would make more sense.

@DrMcCoy
Copy link
Member

DrMcCoy commented Sep 6, 2016

Even though it doesn't functionally do much yet, it's nice to see how the code is supposed to be called and supposed to work. :)

It seems that the original engine has CGuiInGame::OnInitialize method, that call the appropriate code to initialize the GUI

Okay, interesting. So there's a lot of things that still need more research.

As for this PR, it looks really quite good. If you change these two things @clone2727 flagged, I'd say it's fit to be merged. :)

Two questions though:

  1. How do you wish to be credited in the AUTHORS file?
  2. Do you plan to extend upon this? Research more Witcher Lua stuff, implement more for xoreos, etc.? It's fine if you don't want to or can't for any reason, but I certainly would appreciate it.

@smbas
Copy link
Contributor Author

smbas commented Sep 10, 2016

Done.

  1. How do you wish to be credited in the AUTHORS file?

As you do for all contributors: name, username and email.

  1. Do you plan to extend upon this? Research more Witcher Lua stuff, implement more for xoreos, etc.? It's fine if you don't want to or can't for any reason, but I certainly would appreciate it.

Yes, but currently I don't know how often I can do this. Next I want to research more about GUI system of The Witcher. The most part of it is implemented using Lua.

@DrMcCoy DrMcCoy merged commit 325f1ee into xoreos:master Sep 10, 2016
@DrMcCoy
Copy link
Member

DrMcCoy commented Sep 10, 2016

Okay, merged, thanks! :D

Yes, but currently I don't know how often I can do this.

Sure, no pressure.

Next I want to research more about GUI system of The Witcher.

Neat! Looking forward to this, then. :)

@DrMcCoy
Copy link
Member

DrMcCoy commented Sep 14, 2016

Hmm, there's a lot of memory issues valgrind finds in the Lua code, though: https://gist.github.com/DrMcCoy/4f2c93770c2f1a290965fa83c35e8b38 (a 64-bit build on amd64). Looks like it's all use after free?

@smbas
Copy link
Contributor Author

smbas commented Sep 14, 2016

Quick look at the Valgrind log tells that the problem seems to be the following. The Lua state is closed in the ScriptManager::deinit(). Then its destructor destroys the TableRefs that are stored in _luaObjectInstances. This includes the "unrefing" from the already freed Lua state.

Normally, it cannot happen. On the exit all Lua instances must be freed and the map _luaObjectInstances must be empty. You can find a warning about that in the log: https://github.com/xoreos/xoreos/blob/master/src/aurora/lua/scriptman.cpp#L57

Two possible quick solutions:

  1. Temporary disable (comment) the RegisterSubst/UnRegisterSubst bindings.
  2. Clear the _objectLuaInstances before closing the Lua state.

The right way of fixing it will be to provide the delete() method for objects that calls ``ScriptManager::unsetLuaInstanceForObject()`. But when I ran the game Lua wasn't complain about the absence of this method. It's not unusual so, because the most part of the Lua code isn't executed yet (only the code that are located in the global scope). Moreover there are probably some cleanup methods that must be called manually from the engine itself.

Also I think about storing some sort of a weak pointer to a Lua state in the TableRef. But I have no idea how to do it at the moment.

DrMcCoy added a commit that referenced this pull request Sep 14, 2016
So that instances left unfreed do not try to access an already
destroyed Lua state.

See also
#127 (comment)
@DrMcCoy
Copy link
Member

DrMcCoy commented Sep 14, 2016

Ah, okay, that makes sense. :)

Clear the _objectLuaInstances before closing the Lua state.

Yeah, that's what I added now. And with that , the valgrind issues are gone.

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

Successfully merging this pull request may close these issues.

None yet

3 participants