Expose script loading to the Lua API

[Gopmyc]

Description

Adds Resources.GetScript(path), which loads a .lua once and returns whatever the script returns, handing the same instance to every call site.

local VectorUtils = Resources.GetScript("Scripts/Utils/VectorUtils.lua")

• Paths go through PathParser::GetRealPath, so : resolves against engine assets.
• Rejects paths escaping both asset roots, and extensions outside GetValidExtensions().
• Cycles are cut with an error instead of a stack overflow.
• The cache lives in the Lua registry: out of _G, and dropped when Reload() destroys the state.
• Behaviours are untouched, still re-executed per instance so each actor keeps its own table.

Uses safe_script_file(..., script_pass_on_error, load_mode::text) rather than sol2's require_file: Lua is built as C, so SOL_PROPAGATE_EXCEPTIONS is off and require_file fails through lua_error — a longjmp that cannot be caught and skips C++ destructors. Its cache is also only written after execution, so it cannot detect cycles.

The second commit removes dofile, loadfile and load, which resolved against the executable's working directory and accepted bytecode.

Related Issue(s)

Fixes #838

Review Guidance

• Breaking. load compiles strings, not files, so nothing here replaces it. The two commits are independent if you would rather take only the first.
• .luarc.json keeps "basic": "enable" since print, pairs and friends come from it. runtime.builtin is per-library, so the LSP still completes the three removed globals.
• Overlaps Enabled common lua modules #751, which opens sol::lib::package on the same lines.
• A file used both as a Behaviour and through GetScript runs twice and yields two unrelated tables. Intended: Behaviours need per-actor state and their own injected owner.
• Shared state does not survive RemoveBehaviour, which reloads the whole state.
• The containment check is lexical, so a symlink inside the assets folder pointing outside would pass. Scenes.Load and the other Resources.Get* do not check containment at all today.

Screenshots/GIFs

N/A

AI Usage Disclosure

N/A

Checklist

• My code follows the project's code style guidelines
• When applicable, I have commented my code, particularly in hard-to-understand areas
• When applicable, I have updated the documentation accordingly
• My changes don't generate new warnings or errors
• I have reviewed and take responsibility for all code in this PR (including any AI-assisted contributions)

[adriengivry]

Not sure I understand the point of creating a custom script loading system, instead of relying on the built-in package library which is much more permissive.

On my side I tested this:

diff --git a/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp b/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
index 74d8b038..df1738f9 100644
--- a/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
+++ b/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
@@ -116,7 +116,7 @@ namespace
                        "    \"table\": \"disable\",\n"
                        "    \"io\": \"disable\",\n"
                        "    \"os\": \"disable\",\n"
-                       "    \"package\": \"disable\",\n"
+                       "    \"package\": \"enable\",\n"
                        "    \"coroutine\": \"disable\"\n"
                        "  }}\n"
                        "}}\n",
@@ -345,7 +345,17 @@ void OvCore::Scripting::LuaScriptEngine::CreateContext()
        OVASSERT(m_context.luaState == nullptr, "A Lua context already exists!");

        m_context.luaState = std::make_unique<sol::state>();
-       m_context.luaState->open_libraries(sol::lib::base, sol::lib::math);
+       m_context.luaState->open_libraries(sol::lib::base, sol::lib::math, sol::lib::package);
+
+       // Allow require() to find Lua files in the project's asset directory.
+       auto& lua = *m_context.luaState;
+
+       std::string packagePath = (m_context.projectAssetsPath / "?.lua").string();
+
+       packagePath += ";";
+       packagePath += lua["package"]["path"].get<std::string>();
+
+       lua["package"]["path"] = packagePath;

        BindLuaActor(*m_context.luaState);
        BindLuaComponents(*m_context.luaState);

And I was able to create these scripts:

Assets/Scripts/Truc.lua

---@class Truc
local Truc = {}

function Truc:SayHello()
	Debug.Log("Hello")
end

return Truc

Assets/Toto.lua

---@class Toto
local Toto = {}

function Toto:SayHi()
	Debug.Log("Hi")
end

return Toto

Assets/Test.lua (Behaviour)

---@class Test : Behaviour
local Test = {}

function Test:OnStart()
	local toto = require("Toto")
	toto:SayHi()

	local truc = require("Scripts.Truc")
	truc:SayHello()
end

return Test

And it produced the expected output when Test.lua is attached to an actor:

[Gopmyc]

Not sure I understand the point of creating a custom script loading system, instead of relying on the built-in package library which is much more permissive.

On my side I tested this:

diff --git a/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp b/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
index 74d8b038..df1738f9 100644
--- a/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
+++ b/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
@@ -116,7 +116,7 @@ namespace
                        "    \"table\": \"disable\",\n"
                        "    \"io\": \"disable\",\n"
                        "    \"os\": \"disable\",\n"
-                       "    \"package\": \"disable\",\n"
+                       "    \"package\": \"enable\",\n"
                        "    \"coroutine\": \"disable\"\n"
                        "  }}\n"
                        "}}\n",
@@ -345,7 +345,17 @@ void OvCore::Scripting::LuaScriptEngine::CreateContext()
        OVASSERT(m_context.luaState == nullptr, "A Lua context already exists!");

        m_context.luaState = std::make_unique<sol::state>();
-       m_context.luaState->open_libraries(sol::lib::base, sol::lib::math);
+       m_context.luaState->open_libraries(sol::lib::base, sol::lib::math, sol::lib::package);
+
+       // Allow require() to find Lua files in the project's asset directory.
+       auto& lua = *m_context.luaState;
+
+       std::string packagePath = (m_context.projectAssetsPath / "?.lua").string();
+
+       packagePath += ";";
+       packagePath += lua["package"]["path"].get<std::string>();
+
+       lua["package"]["path"] = packagePath;

        BindLuaActor(*m_context.luaState);
        BindLuaComponents(*m_context.luaState);

And I was able to create these scripts:

Assets/Scripts/Truc.lua

---@class Truc
local Truc = {}

function Truc:SayHello()
	Debug.Log("Hello")
end

return Truc

Assets/Toto.lua

---@class Toto
local Toto = {}

function Toto:SayHi()
	Debug.Log("Hi")
end

return Toto

Assets/Test.lua (Behaviour)

---@class Test : Behaviour
local Test = {}

function Test:OnStart()
	local toto = require("Toto")
	toto:SayHi()

	local truc = require("Scripts.Truc")
	truc:SayHello()
end

return Test

And it produced the expected output when Test.lua is attached to an actor:

Good point on the ergonomics, require("Scripts.Truc") is definitely what people expect. My worry isn't really the syntax though, it's what the stock loader accepts once package is open.

A few things come with it in the vendored 5.4:

• package.loadlib will load any dynamic library and hand you back a function from it (loadlib.c:682-692), and searcher_C / searcher_Croot are in the default searchers (loadlib.c:701-708). On Windows LUA_CPATH_DEFAULT is !\?.dll;!\..\lib\lua\5.4\?.dll;!\loadall.dll;.\?.dll (luaconf.h:217-221), so that's the editor directory and the CWD.
• LUA_PATH_DEFAULT has !\?.lua and .\?.lua in it too (luaconf.h:209-215). Since the diff prepends instead of replacing, those stay in the chain, so a module can still get picked up from next to the exe. That's the behaviour the issue was trying to get rid of for dofile and loadfile.
• searcher_Lua goes through luaL_loadfile (loadlib.c:541), which passes mode NULL, and checkmode does nothing when the mode is null (ldo.c:983-1003). So bytecode loads fine, and the undumper isn't hardened against crafted input.
• ll_require only writes LOADED[name] after the loader returns (loadlib.c:648-668), so a circular require just recurses until it hits the C stack limit. You get an error, but it doesn't tell you which file.

None of that is unfixable. Replace package.path instead of prepending it, clear cpath, nil loadlib, drop searchers 3 and 4, and put a text-mode searcher in place of searcher_Lua. That's maybe 30 lines, but you have to know that searchers 3 and 4 are the C ones and that loadfile defaults to "bt". The version in this PR is longer to read, but at least it states what it allows in one spot.

Couple of smaller things: require can't express the : engine prefix, and if you add both roots to package.path then a module called Utils becomes ambiguous between them.

I've got this branch built and running here, with a set of test scripts that check instance identity and single execution, .. and backslash spellings landing on the same cache entry, nested and circular loads, and each of the rejection paths. I can share them if you want, they'd probably be a decent way to compare the two approaches directly.

One thing I wasn't sure about from your test: did you check that the LSP resolves it, or just the runtime? The generated .luarc.json doesn't set Lua.runtime.path, so I don't know whether luals maps require("Toto") to Assets/Toto.lua on its own.

Anyway, if require is the API you'd rather have, I'm happy to rework this instead of dropping it. The restrictions fit into a custom searcher, so you keep require() and package.loaded and the policy stays visible. Something like this (untested):

sol::table searchers = (*m_context.luaState)["package"]["searchers"];
searchers[2] = /* resolve under the asset roots, load in text mode */;

Just say which way you'd prefer.

[Gopmyc]

Not sure I understand the point of creating a custom script loading system, instead of relying on the built-in package library which is much more permissive.
On my side I tested this:

diff --git a/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp b/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
index 74d8b038..df1738f9 100644
--- a/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
+++ b/Sources/OvCore/src/OvCore/Scripting/Lua/LuaScriptEngine.cpp
@@ -116,7 +116,7 @@ namespace
                        "    \"table\": \"disable\",\n"
                        "    \"io\": \"disable\",\n"
                        "    \"os\": \"disable\",\n"
-                       "    \"package\": \"disable\",\n"
+                       "    \"package\": \"enable\",\n"
                        "    \"coroutine\": \"disable\"\n"
                        "  }}\n"
                        "}}\n",
@@ -345,7 +345,17 @@ void OvCore::Scripting::LuaScriptEngine::CreateContext()
        OVASSERT(m_context.luaState == nullptr, "A Lua context already exists!");

        m_context.luaState = std::make_unique<sol::state>();
-       m_context.luaState->open_libraries(sol::lib::base, sol::lib::math);
+       m_context.luaState->open_libraries(sol::lib::base, sol::lib::math, sol::lib::package);
+
+       // Allow require() to find Lua files in the project's asset directory.
+       auto& lua = *m_context.luaState;
+
+       std::string packagePath = (m_context.projectAssetsPath / "?.lua").string();
+
+       packagePath += ";";
+       packagePath += lua["package"]["path"].get<std::string>();
+
+       lua["package"]["path"] = packagePath;

        BindLuaActor(*m_context.luaState);
        BindLuaComponents(*m_context.luaState);

And I was able to create these scripts:
Assets/Scripts/Truc.lua

---@class Truc
local Truc = {}

function Truc:SayHello()
	Debug.Log("Hello")
end

return Truc

Assets/Toto.lua

---@class Toto
local Toto = {}

function Toto:SayHi()
	Debug.Log("Hi")
end

return Toto

Assets/Test.lua (Behaviour)

---@class Test : Behaviour
local Test = {}

function Test:OnStart()
	local toto = require("Toto")
	toto:SayHi()

	local truc = require("Scripts.Truc")
	truc:SayHello()
end

return Test

And it produced the expected output when Test.lua is attached to an actor:

Good point on the ergonomics, require("Scripts.Truc") is definitely what people expect. My worry isn't really the syntax though, it's what the stock loader accepts once package is open.

A few things come with it in the vendored 5.4:

• package.loadlib will load any dynamic library and hand you back a function from it (loadlib.c:682-692), and searcher_C / searcher_Croot are in the default searchers (loadlib.c:701-708). On Windows LUA_CPATH_DEFAULT is !\?.dll;!\..\lib\lua\5.4\?.dll;!\loadall.dll;.\?.dll (luaconf.h:217-221), so that's the editor directory and the CWD.
• LUA_PATH_DEFAULT has !\?.lua and .\?.lua in it too (luaconf.h:209-215). Since the diff prepends instead of replacing, those stay in the chain, so a module can still get picked up from next to the exe. That's the behaviour the issue was trying to get rid of for dofile and loadfile.
• searcher_Lua goes through luaL_loadfile (loadlib.c:541), which passes mode NULL, and checkmode does nothing when the mode is null (ldo.c:983-1003). So bytecode loads fine, and the undumper isn't hardened against crafted input.
• ll_require only writes LOADED[name] after the loader returns (loadlib.c:648-668), so a circular require just recurses until it hits the C stack limit. You get an error, but it doesn't tell you which file.

None of that is unfixable. Replace package.path instead of prepending it, clear cpath, nil loadlib, drop searchers 3 and 4, and put a text-mode searcher in place of searcher_Lua. That's maybe 30 lines, but you have to know that searchers 3 and 4 are the C ones and that loadfile defaults to "bt". The version in this PR is longer to read, but at least it states what it allows in one spot.

Couple of smaller things: require can't express the : engine prefix, and if you add both roots to package.path then a module called Utils becomes ambiguous between them.

I've got this branch built and running here, with a set of test scripts that check instance identity and single execution, .. and backslash spellings landing on the same cache entry, nested and circular loads, and each of the rejection paths. I can share them if you want, they'd probably be a decent way to compare the two approaches directly.

One thing I wasn't sure about from your test: did you check that the LSP resolves it, or just the runtime? The generated .luarc.json doesn't set Lua.runtime.path, so I don't know whether luals maps require("Toto") to Assets/Toto.lua on its own.

Anyway, if require is the API you'd rather have, I'm happy to rework this instead of dropping it. The restrictions fit into a custom searcher, so you keep require() and package.loaded and the policy stays visible. Something like this (untested):

sol::table searchers = (*m_context.luaState)["package"]["searchers"];
searchers[2] = /* resolve under the asset roots, load in text mode */;

Just say which way you'd prefer.

One more angle, on the API side rather than the sandbox.

Everything else in the engine addresses a script by path. Behaviour.name is
Scripts/Foo.lua, that's what GetScriptPath produces and what the inspector shows,
and AssetRef carries paths too. Resources.GetScript("Scripts/Foo.lua") is the same
string. With require the same file ends up with two names depending on how it gets
loaded, Scripts/Foo.lua as a Behaviour and Scripts.Foo as a module, and you'd be
mixing Resources.GetTexture("Textures/foo.png") with require("Scripts.Utils") in the
same file. Scripts would be the only asset type not addressed like the rest

There's also the failure mode. require raises, so a typo in a module name at the top of
a Behaviour propagates into LoadScript, registration fails, errorCount goes up and
IsOk() blocks play mode. Resources.GetScript returns nil and logs, same as GetModel
and GetTexture already do for a missing asset. A missing texture doesn't stop you
entering play, and a missing module probably shouldn't either

[adriengivry]

For this one I still think we should go the require route. We shouldn't treat scripts as resources. require is simple, already implemented, and only requires us to add the project's folder to the search path. Regarding the best way to add the project to the search path, not sure what's the best way to go. What I proposed worked, maybe there is a better, safer way.