Fix lua native bindings and lua script reload (#1575)

* fix: script manager and lua scripts: change the api so that the m_scripts array is only modified in a specific safe location: before it gets ticked.
* fix script manager: don't expose the script vector directly, for multithreading safety
* fix lua manager usage: don't iterate the module array without locking, nor un/load module from a script thread
* lua script: only do actual loading of lua modules in script mgr
* lua native bindin: fix pointer parameters, out C style parameters are returned as multiple return values lua-style

src/hooks/protections/script_event_handler.cpp

@@ -60,7 +60,7 @@ namespace big
 
 		auto plyr = g_player_service->get_by_id(player->m_player_id);
 
-		if (g_lua_manager && g_lua_manager->get_modules().size() > 0)
+		if (g_lua_manager && g_lua_manager->get_module_count() > 0)
 		{
 			std::vector<int32_t> script_event_args;
 

src/lua/lua_manager.cpp

@@ -6,6 +6,8 @@ namespace big
 {
 	lua_manager::lua_manager()
 	{
+		m_schedule_reload_modules = false;
+
 		load_all_modules();
 
 		g_lua_manager = this;
@@ -56,8 +58,31 @@ namespace big
 		m_modules.push_back(std::make_shared<lua_module>(module_name));
 	}
 
+	void lua_manager::queue_load_module(const std::string& module_name, std::function<void(std::weak_ptr<lua_module>)> on_module_loaded)
+	{
+		m_modules_load_queue.push({module_name, on_module_loaded});
+	}
+
+	void lua_manager::load_modules_from_queue()
+	{
+		while (m_modules_load_queue.size())
+		{
+			auto& module_load_info = m_modules_load_queue.front();
+
+			const auto id = rage::joaat(module_load_info.m_name);
+
+			load_module(module_load_info.m_name);
+			auto loaded_module = get_module(id);
+			module_load_info.m_on_module_loaded(loaded_module);
+
+			m_modules_load_queue.pop();
+		}
+	}
+
 	std::weak_ptr<lua_module> lua_manager::get_module(rage::joaat_t module_id)
 	{
+		std::lock_guard guard(m_module_lock);
+
 		for (const auto& module : m_modules)
 			if (module->module_id() == module_id)
 				return module;
@@ -65,17 +90,6 @@ namespace big
 		return {};
 	}
 
-	const std::vector<std::shared_ptr<lua_module>>& lua_manager::get_modules() const
-	{
-		return m_modules;
-	}
-
-	void lua_manager::reload_all_modules()
-	{
-		unload_all_modules();
-		load_all_modules();
-	}
-
 	void lua_manager::handle_error(const sol::error& error, const sol::state_view& state)
 	{
 		LOG(WARNING) << state["!module_name"].get<std::string_view>() << ": " << error.what();

src/lua/lua_manager.hpp

@@ -7,16 +7,31 @@ namespace big
 	{
 		std::mutex m_module_lock;
 
+	public:
+		bool m_schedule_reload_modules;
+
 	public:
 		lua_manager();
 		~lua_manager();
 
+		void load_all_modules();
+		void unload_all_modules();
+
+		inline auto get_module_count() const
+		{
+			return m_modules.size();
+		}
+
 		void draw_gui(rage::joaat_t tab_hash);
+
 		void unload_module(rage::joaat_t module_id);
 		void load_module(const std::string& module_name);
+
+		void queue_load_module(const std::string& module_name, std::function<void(std::weak_ptr<lua_module>)> on_module_loaded);
+		void load_modules_from_queue();
+
 		std::weak_ptr<lua_module> get_module(rage::joaat_t module_id);
-		const std::vector<std::shared_ptr<lua_module>>& get_modules() const;
-		void reload_all_modules();
+
 		void handle_error(const sol::error& error, const sol::state_view& state);
 
 		template<template_str hash_str, typename Return = void, typename... Args>
@@ -24,9 +39,11 @@ namespace big
 		{
 			constexpr auto hash = rage::joaat(hash_str.value);
 
-			for (auto& modules : get_modules())
+			std::lock_guard guard(m_module_lock);
+
+			for (auto& module : m_modules)
 			{
-				if (auto vec = modules->m_event_callbacks.find(hash); vec != modules->m_event_callbacks.end())
+				if (auto vec = module->m_event_callbacks.find(hash); vec != module->m_event_callbacks.end())
 				{
 					for (auto& cb : vec->second)
 					{
@@ -56,11 +73,25 @@ namespace big
 				return std::nullopt;
 		}
 
-	private:
-		void load_all_modules();
-		void unload_all_modules();
+		inline void for_each_module(auto func)
+		{
+			std::lock_guard guard(m_module_lock);
 
+			for (auto& module : m_modules)
+			{
+				func(module);
+			}
+		}
+
+	private:
 		std::vector<std::shared_ptr<lua_module>> m_modules;
+
+		struct module_load_info
+		{
+			std::string m_name;
+			std::function<void(std::weak_ptr<lua_module>)> m_on_module_loaded;
+		};
+		std::queue<module_load_info> m_modules_load_queue;
 	};
 
 	inline lua_manager* g_lua_manager;

src/lua/natives/natives_gen.py

@@ -5,6 +5,8 @@ natives_hpp = open("../../natives.hpp", "r")
 cpp_print_buf = ""
 hpp_print_buf = ""
 
+hpp_lua_native_wrappers_print_buf = ""
+
 
 def print_cpp(text):
     global cpp_print_buf
@@ -16,39 +18,157 @@ def print_hpp(text):
     hpp_print_buf += text + "\n"
 
 
+def print_hpp_native_wrapper(text):
+    global hpp_lua_native_wrappers_print_buf
+    hpp_lua_native_wrappers_print_buf += text + "\n"
+
+
 class Arg:
     def __init__(self, name, type_):
         self.name = name
-        self.type_ = type_
+        self.type_ = type_.replace("BOOL", "bool")
+        self.is_pointer_arg = "*" in type_ and "const char" not in type_
+        self.type_no_star = self.type_.replace("*", "")
 
     def __str__(self) -> str:
         return str(self.type_) + " " + str(self.name)
 
 
 class NativeFunc:
-    def __init__(self, name, args, return_type):
+    def __init__(self, namespace, name, args, return_type):
+        self.namespace = namespace
         self.name = name
         self.args = args
-        self.return_type = return_type
+        self.return_type = return_type.replace("BOOL", "bool")
+
+        self.out_params = []
+        if self.return_type != "void":
+            retvalArg = Arg("retval", self.return_type)
+            # Any* case: this is incorrect but
+            # we'd need a custom lua usertype and write code for it inside the native function wrappers,
+            # it also only affect some of the DATAFILE natives.
+            retvalArg.is_pointer_arg = False
+            self.out_params.append(retvalArg)
+        for arg in self.args:
+            if arg.is_pointer_arg:
+                self.out_params.append(arg)
 
     def __str__(self) -> str:
+        s = ""
+
+        returning_multiple_values = False
+        tuple_type = ""
+        fixed_return = self.return_type
+        if len(self.out_params) > 1:
+            fixed_return = "std::tuple<"
+            for out_param in self.out_params:
+                if out_param.is_pointer_arg:
+                    fixed_return += out_param.type_no_star + ", "
+                else:
+                    fixed_return += out_param.type_ + ", "
+            fixed_return = fixed_return[:-2] + ">"
+            returning_multiple_values = True
+            tuple_type = fixed_return
+        elif len(self.out_params) == 1:
+            if self.out_params[0].is_pointer_arg:
+                fixed_return = self.out_params[0].type_no_star
+            else:
+                fixed_return = self.out_params[0].type_
+
+        fixed_params = ""
         if len(self.args) > 0:
-            return (
-                str(self.return_type)
-                + " "
-                + str(self.name)
-                + "( "
-                + str(self.args[0])
-                + " )"
-            )
+            for arg in self.args:
+                if not arg.is_pointer_arg:
+                    fixed_params += arg.type_ + " " + arg.name + ", "
+                else:
+                    fixed_params += arg.type_no_star + " " + arg.name + ", "
+            fixed_params = fixed_params[:-2]
+
+        s += (
+            fixed_return
+            + " "
+            + "LUA_NATIVE_"
+            + self.namespace
+            + "_"
+            + self.name
+            + "( "
+            + fixed_params
+            + " )"
+        )
+
+        s += "\n"
+        s += "\t{\n"
+
+        call_native = "\t\t"
+        if len(self.out_params) > 0:
+            if returning_multiple_values:
+                tuple_declaration = tuple_type + " return_values;"
+                s += "\t\t" + tuple_declaration + "\n"
+                if self.return_type != "void":
+                    call_native += "std::get<0>(return_values) = "
+                    if self.return_type == "bool":
+                        call_native += "(bool)"
+            elif self.return_type != "void":
+                call_native += "auto retval = "
+                if self.return_type == "bool":
+                    call_native += "(bool)"
+
+            call_native += self.namespace + "::" + self.name + "("
         else:
-            return str(self.return_type) + " " + str(self.name) + "( )"
+            call_native += self.namespace + "::" + self.name + "("
+
+        if len(self.args) > 0:
+            for arg in self.args:
+                if arg.is_pointer_arg:
+                    if arg.type_ == "bool*":
+                        call_native += "(BOOL*)"
+                    call_native += "&"
+
+                call_native += arg.name + ", "
+            call_native = call_native[:-2]
+
+        call_native += ");"
+
+        s += call_native
+
+        if returning_multiple_values:
+            assign_return_values = "\n"
+            if self.return_type != "void":
+                i = 1
+            else:
+                i = 0
+            for arg in self.args:
+                if arg.is_pointer_arg:
+                    assign_return_values += (
+                        "\t\tstd::get<"
+                        + str(i)
+                        + ">(return_values) = "
+                        + arg.name
+                        + ";\n"
+                    )
+                    i += 1
+            s += assign_return_values
+
+        return_statement = ""
+        if len(self.out_params) > 0:
+            if returning_multiple_values:
+                return_statement = "return return_values;"
+            elif self.return_type != "void":
+                return_statement = "return retval;"
+            else:
+                for arg in self.args:
+                    if arg.is_pointer_arg:
+                        return_statement = "return " + arg.name + ";"
+            s += "\n\t\t" + return_statement
+
+        s += "\n\t}"
+
+        return s
 
 
 def get_natives_func_from_natives_hpp_file(natives_hpp):
     functions_per_namespaces = {}
     current_namespace = ""
-    func_name = ""
     start_parsing = False
     for line in natives_hpp.readlines():
         if "namespace SYSTEM" not in line and not start_parsing:
@@ -69,26 +189,62 @@ def get_natives_func_from_natives_hpp_file(natives_hpp):
             words.pop(0)
 
             func_name = ""
-            return_type = [x for x in words if "(" not in x][0]
-
             for word in words:
-                # function name + args
                 if "(" in word:
                     if func_name == "":
-                        func_name_and_args = word.split("(")
-                        func_name = func_name_and_args[0]
+                        func_name = word.split("(")[0]
+
                     continue
 
-            functions_per_namespaces[current_namespace].append(
-                NativeFunc(func_name, [], return_type)
+            args = []
+            args_start = line.split("(")[1]
+            if args_start[0] == ")":
+                # no args
+                pass
+            else:
+                args_str = args_start.rstrip()[:-1]
+                i = 0
+                for arg in args_str.split(","):
+                    arg_type = arg[: arg.rfind(" ")].strip()
+                    arg_name = arg[arg.rfind(" ") :].strip()
+                    args.append(Arg(arg_name, arg_type))
+                    i += 1
+
+            return_type = (
+                line[: line.find(func_name)].replace("NATIVE_DECL", "").strip()
             )
 
+            native_func = NativeFunc(current_namespace, func_name, args, return_type)
+
+            functions_per_namespaces[current_namespace].append(native_func)
+
     return functions_per_namespaces
 
 
 functions_per_namespaces = get_natives_func_from_natives_hpp_file(natives_hpp)
 
 
+def generate_lua_native_wrapper_binding_hpp_file(functions_per_namespaces):
+    print_hpp_native_wrapper("#pragma once")
+    print_hpp_native_wrapper('#include "natives.hpp"')
+    print_hpp_native_wrapper("")
+    print_hpp_native_wrapper("namespace lua::native")
+    print_hpp_native_wrapper("{")
+
+    i = 0
+    for namespace_name, native_funcs in functions_per_namespaces.items():
+        for native_func in native_funcs:
+            i += 1
+            print_hpp_native_wrapper("\t" + str(native_func))
+            print_hpp_native_wrapper("")
+    print_hpp_native_wrapper("}")
+
+    print(f"Wrote lua native wrappers for {i} native functions")
+
+
+generate_lua_native_wrapper_binding_hpp_file(functions_per_namespaces)
+
+
 def generate_native_binding_cpp_and_hpp_file(functions_per_namespaces):
     generated_function_name = "void init_native_binding(sol::state& L)"
 
@@ -101,7 +257,7 @@ def generate_native_binding_cpp_and_hpp_file(functions_per_namespaces):
     print_hpp("}")
 
     print_cpp('#include "lua_native_binding.hpp"')
-    print_cpp('#include "natives.hpp"')
+    print_cpp('#include "lua_native_wrappers_binding.hpp"')
     print_cpp("")
     print_cpp("namespace lua::native")
     print_cpp("{")
@@ -126,18 +282,20 @@ def generate_native_binding_cpp_and_hpp_file(functions_per_namespaces):
                 + '.set_function("'
                 + native_func.name
                 + '", '
-                + namespace_name
-                + "::"
+                + "LUA_NATIVE_"
+                + native_func.namespace
+                + "_"
                 + native_func.name
                 + ");"
             )
 
         print_cpp("")
 
-    print(f"Wrote binding for {i} native functions")
     print_cpp("\t}")
     print_cpp("}")
 
+    print(f"Wrote binding for {i} native functions")
+
 
 generate_native_binding_cpp_and_hpp_file(functions_per_namespaces)
 
@@ -160,5 +318,15 @@ def write_hpp_code(hpp_print_buf):
     f.close()
 
 
+def write_lua_native_wrappers_hpp_code(hpp_lua_native_wrappers_print_buf):
+    file_name = "lua_native_wrappers_binding.hpp"
+    if os.path.exists(file_name):
+        os.remove(file_name)
+    f = open(file_name, "a")
+    f.write(hpp_lua_native_wrappers_print_buf)
+    f.close()
+
+
 write_cpp_code(cpp_print_buf)
 write_hpp_code(hpp_print_buf)
+write_lua_native_wrappers_hpp_code(hpp_lua_native_wrappers_print_buf)

src/script.cpp

@@ -5,7 +5,7 @@
 
 namespace big
 {
-	script::script(const func_t func, const std::string_view name, const bool toggleable, const std::optional<std::size_t> stack_size) :
+	script::script(const func_t func, const std::string& name, const bool toggleable, const std::optional<std::size_t> stack_size) :
 	    script(func, stack_size)
 	{
 		m_name       = name;
@@ -17,7 +17,8 @@ namespace big
 	    m_toggleable(false),
 	    m_script_fiber(nullptr),
 	    m_main_fiber(nullptr),
-	    m_func(func)
+	    m_func(func),
+	    m_should_be_deleted(false)
 	{
 		m_script_fiber = CreateFiber(
 		    stack_size.has_value() ? stack_size.value() : 0,

src/script.hpp

@@ -5,15 +5,17 @@ namespace big
 {
 	class script
 	{
-		std::string_view m_name;
+		std::string m_name;
 		bool m_enabled;
 		bool m_toggleable;
 
 	public:
 		using func_t = std::function<void(void)>;
 
+		bool m_should_be_deleted;
+
 	public:
-		explicit script(const func_t func, const std::string_view name, const bool toggleable = true, const std::optional<std::size_t> stack_size = std::nullopt);
+		explicit script(const func_t func, const std::string& name, const bool toggleable = true, const std::optional<std::size_t> stack_size = std::nullopt);
 		explicit script(const func_t func, const std::optional<std::size_t> stack_size = std::nullopt);
 		~script();
 

src/script_mgr.cpp

@@ -6,34 +6,31 @@
 #include "gta_util.hpp"
 #include "invoker.hpp"
 #include "pointers.hpp"
+#include "lua/lua_manager.hpp"
 
 namespace big
 {
 	script* script_mgr::add_script(std::unique_ptr<script> script)
 	{
 		std::lock_guard lock(m_mutex);
+
 		auto* ret = script.get();
-		m_scripts.push_back(std::move(script));
+		m_scripts_to_add.push_back(std::move(script));
 		return ret;
 	}
 
 	void script_mgr::remove_script(script* scr)
 	{
 		std::lock_guard lock(m_mutex);
-		std::erase_if(m_scripts, [scr](auto& iter) {
-			return iter.get() == scr;
-		});
+
+		scr->m_should_be_deleted = true;
 	}
 
 	void script_mgr::remove_all_scripts()
 	{
 		std::lock_guard lock(m_mutex);
-		m_scripts.clear();
-	}
 
-	script_list& script_mgr::scripts()
-	{
-		return m_scripts;
+		m_scripts.clear();
 	}
 
 	void script_mgr::tick()
@@ -46,8 +43,40 @@ namespace big
 		static bool ensure_main_fiber = (ConvertThreadToFiber(nullptr), true);
 
 		std::lock_guard lock(m_mutex);
-		for (auto const& script : m_scripts)
+
+		if (g_lua_manager->m_schedule_reload_modules)
+		{
+			g_lua_manager->unload_all_modules();
+		}
+
+		std::erase_if(m_scripts, [](std::unique_ptr<script>& iter) {
+			return iter->m_should_be_deleted;
+		});
+
+		if (g_lua_manager->m_schedule_reload_modules)
+		{
+			g_lua_manager->load_all_modules();
+			g_lua_manager->m_schedule_reload_modules = false;
+		}
+		else
+		{
+			g_lua_manager->load_modules_from_queue();
+		}
+
+		if (m_scripts_to_add.size())
+		{
+			for (auto& script_to_add : m_scripts_to_add)
+			{
+				m_scripts.push_back(std::move(script_to_add));
+			}
+
+			m_scripts_to_add.clear();
+		}
+
+		for (const auto& script : m_scripts)
+		{
 			if (script->is_enabled())
 				script->tick();
+		}
 	}
 }

src/script_mgr.hpp

@@ -16,7 +16,15 @@ namespace big
 		void remove_script(script* script);
 		void remove_all_scripts();
 
-		script_list& scripts();
+		inline void for_each_script(auto func)
+		{
+			std::lock_guard lock(m_mutex);
+
+			for (const auto& script : m_scripts)
+			{
+				func(script);
+			}
+		}
 
 		void tick();
 
@@ -26,6 +34,7 @@ namespace big
 	private:
 		std::recursive_mutex m_mutex;
 		script_list m_scripts;
+		script_list m_scripts_to_add;
 	};
 
 	inline script_mgr g_script_mgr;

src/views/debug/view_debug_scripts.cpp

@@ -10,8 +10,8 @@ namespace big
 		{
 			components::sub_title("DEBUG_SCRIPTS_SUB_TITLE"_T);
 
-			for (const auto& script : g_script_mgr.scripts())
-			{
+			g_script_mgr.for_each_script([](const auto& script) {
+
 				if (script->is_toggleable())
 				{
 					if (ImGui::Checkbox(script->name(), script->toggle_ptr()))
@@ -19,7 +19,8 @@ namespace big
 						g_notification_service->push(std::string(script->name()).append(" script"), script->is_enabled() ? "Resumed" : "Halted");
 					}
 				}
-			}
+			});
+
 			ImGui::EndTabItem();
 		}
 	}

src/views/settings/view_lua_scripts.cpp

@@ -8,7 +8,6 @@
 namespace big
 {
 	static std::weak_ptr<lua_module> selected_module{};
-	static bool show = true;
 
 	void view::lua_scripts()
 	{
@@ -17,21 +16,11 @@ namespace big
 
 		if (ImGui::BeginListBox("##empty", ImVec2(200, 200)))
 		{
-			auto& modules = g_lua_manager->get_modules();
-
-			if (show && modules.size() > 0)
-			{
-				for (const auto& module : modules)
-				{
-					if (ImGui::Selectable(module->module_name().c_str(),
-					        !selected_module.expired() && selected_module.lock().get() == module.get()))
-						selected_module = module;
-				}
-			}
-			else
-			{
-				ImGui::Text("No scripts loaded");
-			}
+			g_lua_manager->for_each_module([](auto& module) {
+				if (ImGui::Selectable(module->module_name().c_str(),
+					!selected_module.expired() && selected_module.lock().get() == module.get()))
+				selected_module = module;
+			});
 
 			ImGui::EndListBox();
 		}
@@ -46,22 +35,23 @@ namespace big
 			ImGui::Text("Memory Patches Registered: %d", selected_module.lock()->m_registered_patches.size());
 			ImGui::Text("GUI Tabs Registered: %d", selected_module.lock()->m_gui.size());
 
-			components::button("Reload", [] {
+			if (components::button("Reload"))
+			{
 				auto name = selected_module.lock()->module_name();
 				auto id   = selected_module.lock()->module_id();
 
 				g_lua_manager->unload_module(id);
-				g_lua_manager->load_module(name);
-				selected_module = g_lua_manager->get_module(id);
-			});
+				g_lua_manager->queue_load_module(name, [](std::weak_ptr<big::lua_module> loaded_module) {
+					selected_module = loaded_module;
+				});
+			}
 		}
 
 		ImGui::EndGroup();
 
-		components::button("Reload All", [] {
-			show = false;
-			g_lua_manager->reload_all_modules();
-			show = true;
-		});
+		if (components::button("Reload All"))
+		{
+			g_lua_manager->m_schedule_reload_modules = true;
+		}
 	}
 }