feat: migrate core Lua functions to Sol2#3816
Draft
beats-dh wants to merge 4 commits into
Draft
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces sol2 as a dependency and systematically migrates Lua-C++ bindings from the raw Lua C API to sol2-based bindings across multiple game function modules, including updates to public interfaces and internal state management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replaces manual Lua C API bindings for bank, config, and game functions with sol2-based bindings, simplifying code and improving maintainability. Updates CMake to require sol2 and links it in the build. Removes legacy Lua stack manipulation and introduces more idiomatic C++/sol2 code for exposing functions and objects to Lua scripts. Updates function signatures and object handling to use sol::object and sol::state_view, and modernizes the way enums and tables are exposed to Lua.
beats-dh
force-pushed
the
fix/lua-sol2-bindings
branch
from
f4cb397 to
c61c6b4
Compare
beats-dh
changed the title
beats-dh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lua/functions/core/game/bank_functions.cpp`:
- Around line 122-133: The current deposit flow calls
g_game().removeMoney(player, amount) && bank->credit(amount) (and similarly for
destination->credit) which can remove money then fail to credit; change this to
perform the steps explicitly: call g_game().removeMoney(player, amount) and if
that returns false return false immediately; otherwise attempt credit on the
Bank instance (or destination returned by getBank(destinationObj.value())); if
credit succeeds return true, but if credit fails call g_game().addMoney(player,
amount) to rollback the removal, log/report the failure (e.g., via
Lua::reportErrorFunc or process logger), and return false. Update both the
default Bank creation path (Bank(player) / bank->credit) and the custom
destination path (getBank(...) / destination->credit) accordingly.
🧹 Nitpick comments (6)
src/lua/scripts/lua_environment.hpp (1)
56-67: Avoid exposing mutable internal state through public accessors.The
getSolState()method currently returns a non-const reference tosolState, which theoretically allows external callers to mutate the optional. Although the method is currently unused in the codebase, returning a const reference aligns with the encapsulation pattern—all state mutations are properly handled via internalemplace()andreset()calls in the initialization and cleanup paths.♻️ Suggested change
- std::optional<sol::state_view> &getSolState() { - return solState; - } + const std::optional<sol::state_view> &getSolState() const { + return solState; + }src/lua/scripts/lua_environment.cpp (1)
87-89: ResetsolStatebefore callinglua_close.The
solStateis a view overluaState. Callinglua_close(luaState)on line 87 invalidates the underlying Lua state, butsolState.reset()is called on line 89 afterward. While this may not cause immediate issues sincereset()just clears the optional, it's safer and clearer to reset the view before destroying what it references.Suggested reorder
+ solState.reset(); lua_close(luaState); luaState = nullptr; - solState.reset();src/lua/functions/core/game/config_functions.cpp (1)
22-32: Consider validating enum keys before casting.The bindings cast
uint32_tdirectly toConfigKey_twithout validating that the value corresponds to a valid enum member. If Lua scripts pass invalid keys, this could lead to undefined behavior ing_configManager().Example validation pattern
configManager.set_function("getString", [](uint32_t key) { + auto enumKey = magic_enum::enum_cast<ConfigKey_t>(key); + if (!enumKey.has_value()) { + Lua::reportErrorFunc("Invalid config key"); + return std::string{}; + } - return g_configManager().getString(static_cast<ConfigKey_t>(key)); + return g_configManager().getString(enumKey.value()); });src/lua/functions/core/game/bank_functions.cpp (1)
39-49: Polymorphic return type may confuse callers.The
balancefunction returns either auint64_t(balance value) orfalse(on error). Lua scripts need to check the type of the return value. Consider documenting this behavior or using a more explicit error signaling pattern.src/lua/functions/core/game/game_functions.cpp (2)
123-143: Consider validating position table fields.Using
get_or(0)for position coordinates means malformed position tables silently default to(0, 0, 0). This could lead to unexpected behavior. Consider validating required fields or using a consistent position parsing helper.
671-703: Consider reusingachievementToTablefor consistency.The
getAchievementInfoByIdandgetAchievementInfoByNamefunctions duplicate the table construction logic that's already captured in theachievementToTablelambda defined on lines 705-714.Suggested consolidation
game.set_function("getAchievementInfoById", [](uint16_t id, sol::this_state s) -> sol::object { const Achievement achievement = g_game().getAchievementById(id); if (achievement.id == 0) { Lua::reportErrorFunc("Achievement id is wrong"); return sol::object(sol::lua_nil); } - sol::state_view L = s; - sol::table t = L.create_table(); - t["id"] = achievement.id; - t["name"] = achievement.name; - t["description"] = achievement.description; - t["points"] = achievement.points; - t["grade"] = achievement.grade; - t["secret"] = achievement.secret; - return t; + return achievementToTable(sol::state_view(s), achievement); });Note: This requires moving
achievementToTabledefinition before its usage or restructuring the bindings.
Comment on lines
+122
to
+133
| if (!destinationObj) { | ||
| auto bank = std::make_shared<Bank>(player); | ||
| return g_game().removeMoney(player, amount) && bank->credit(amount); | ||
| } | ||
|
|
||
| const auto &destination = getBank(destinationObj.value()); | ||
| if (!destination) { | ||
| Lua::reportErrorFunc("Destination is nullptr"); | ||
| return false; | ||
| } | ||
| return g_game().removeMoney(player, amount) && destination->credit(amount); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for addMoney, giveMoney and related money manipulation functions
rg -n "addMoney|giveMoney" --type cpp -A 2Repository: opentibiabr/canary
Length of output: 2263
🏁 Script executed:
# Get the full context of the deposit function (lines 109-133)
sed -n '109,133p' src/lua/functions/core/game/bank_functions.cppRepository: opentibiabr/canary
Length of output: 819
🏁 Script executed:
# Search for removeMoney function signature
rg -n "removeMoney" --type cpp -B 2 -A 5 | head -50Repository: opentibiabr/canary
Length of output: 5519
🏁 Script executed:
# Search for credit function in Bank class
rg -n "credit" --type cpp -B 2 -A 5 | head -50Repository: opentibiabr/canary
Length of output: 3135
Add rollback logic to prevent money loss on deposit failure.
The deposit logic uses g_game().removeMoney(player, amount) && bank->credit(amount). If removeMoney succeeds but credit fails, the player loses money without it being credited to the bank. Since both functions must succeed together, add explicit error handling with rollback using addMoney():
Suggested safer pattern
- return g_game().removeMoney(player, amount) && bank->credit(amount);
+ if (!g_game().removeMoney(player, amount)) {
+ return false;
+ }
+ if (!bank->credit(amount)) {
+ // Rollback: return money to player
+ g_game().addMoney(player, amount);
+ return false;
+ }
+ return true;Apply the same fix to line 132 for the custom destination case.
🤖 Prompt for AI Agents
In `@src/lua/functions/core/game/bank_functions.cpp` around lines 122 - 133, The
current deposit flow calls g_game().removeMoney(player, amount) &&
bank->credit(amount) (and similarly for destination->credit) which can remove
money then fail to credit; change this to perform the steps explicitly: call
g_game().removeMoney(player, amount) and if that returns false return false
immediately; otherwise attempt credit on the Bank instance (or destination
returned by getBank(destinationObj.value())); if credit succeeds return true,
but if credit fails call g_game().addMoney(player, amount) to rollback the
removal, log/report the failure (e.g., via Lua::reportErrorFunc or process
logger), and return false. Update both the default Bank creation path
(Bank(player) / bank->credit) and the custom destination path (getBank(...) /
destination->credit) accordingly.
Contributor
|
✅ Updated vcpkg baseline to 2026.01.16 ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@vcpkg.json`:
- Line 20: The vcpkg manifest currently lists only "sol2" which is header-only
and requires an explicit Lua implementation; update vcpkg.json to include either
the "lua" or "luajit" port alongside "sol2" in the dependencies array (e.g., add
"lua" or "luajit" next to "sol2") and, if your chosen Lua/LuaJIT was built with
C++ linkage, ensure the appropriate compile definition (SOL_USING_CXX_LUA or
SOL_USING_CXX_LUA_JIT) is applied in your build configuration.
| "parallel-hashmap", | ||
| "protobuf", | ||
| "pugixml", | ||
| "sol2", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Sol2 LuaJIT Lua compatibility requirements vcpkg port
💡 Result:
- sol2 (Sol3) is header-only and does not ship Lua/LuaJIT; the vcpkg
sol2port has no dependency onluaorluajit(only build helpers), so your vcpkg port/manifest must add the Lua implementation you want (typicallyluajitorlua). [1] - LuaJIT compatibility (what sol2 expects):
- sol2 supports Lua 5.1/5.2/5.3+ and LuaJIT (2.0.4+ and 2.1.x betas), using its own compatibility header to provide 5.2/5.3-style APIs when building against Lua 5.1 / LuaJIT. [2][3]
- If you already provide your own Lua compatibility layer, you can disable sol2’s by defining
SOL_NO_COMPAT=1before includingsol.hpp. [3]
- vcpkg pieces you typically combine:
- Install/use
sol2+luajit(LuaJIT is Lua-5.1 API/ABI oriented) orsol2+lua(vanilla Lua). The vcpkgluajitport is available (with some platform limits), andluais a separate port. [4][5]
- Install/use
- Linkage note (only if you built Lua/LuaJIT as C++): sol2 has config defines like
SOL_USING_CXX_LUA/SOL_USING_CXX_LUA_JITto stop wrapping Lua headers inextern "C", but you should only use these if your Lua/LuaJIT was built with C++ linkage. [6]
Sources:
[1] vcpkg.link sol2 port metadata (dependencies) (vcpkg.link)
[2] sol2 “features” / supported Lua & LuaJIT versions (sol2.readthedocs.io)
[3] sol2 compatibility.hpp docs + SOL_NO_COMPAT (sol2.readthedocs.io)
[4] vcpkg luajit package page (vcpkg.io)
[5] vcpkg lua port page (vcpkg.link)
[6] sol2 config/safety docs (SOL_USING_CXX_LUA(_JIT)) (sol2.readthedocs.io)
Citations:
- 1: https://vcpkg.link/ports/sol2.json
- 2: https://sol2.readthedocs.io/en/latest/features.html?utm_source=openai
- 3: https://sol2.readthedocs.io/en/latest/api/compatibility.html?utm_source=openai
- 4: https://vcpkg.io/en/package/luajit.html
- 5: https://vcpkg.link/ports/lua?utm_source=openai
- 6: https://sol2.readthedocs.io/en/latest/safety.html?utm_source=openai
🌐 Web query:
Sol2 library Lua version requirements compile definitions
💡 Result:
- Supported Lua versions (sol2/sol3): sol2 supports Lua 5.1+ (including Lua 5.2/5.3/5.4) and LuaJIT 2.0+ (MoonJIT included). [1]
- Selecting a Lua version in sol2’s CMake build: when building Lua as part of sol2, set
SOL2_LUA_VERSION(e.g.,5.1,5.2,5.3,5.4,LuaJIT, or a specific version like5.4.4). [2] - Common compile definitions (preprocessor macros) you may need:
SOL_ALL_SAFETIES_ON=1to enable sol’s safer checking paths (otherwise safety features default to off). [4]SOL_NO_COMPAT=1to disable sol’s internal Lua 5.2/5.3-compat layer (useful if you already provide your own compat layer or hit redefinition conflicts). [5]- Lua integration-related macros mentioned by sol2:
SOL_USING_CXX_LUA,SOL_USING_CXX_LUAJIT,SOL_USE_LUA_HPP. [3]
- Lua version detection in code: sol2 adapts based on the detected Lua environment and exposes Lua-version-specific compile-time checks/macros (e.g., guidance mentions checking
SOL_LUA_VERSION_I_for version-specific code paths). [3]
Sources: [1] [2] [3] [4] [5]
Ensure lua or luajit is explicitly added to dependencies.
The vcpkg sol2 port is header-only with no direct dependency on lua or luajit—you must add your chosen Lua implementation as a separate dependency in vcpkg.json. Sol2 supports Lua 5.1+ and LuaJIT 2.0+, so either port will work. Compile definitions like SOL_USING_CXX_LUA or SOL_USING_CXX_LUA_JIT are only needed if your Lua/LuaJIT was built with C++ linkage.
🤖 Prompt for AI Agents
In `@vcpkg.json` at line 20, The vcpkg manifest currently lists only "sol2" which
is header-only and requires an explicit Lua implementation; update vcpkg.json to
include either the "lua" or "luajit" port alongside "sol2" in the dependencies
array (e.g., add "lua" or "luajit" next to "sol2") and, if your chosen
Lua/LuaJIT was built with C++ linkage, ensure the appropriate compile definition
(SOL_USING_CXX_LUA or SOL_USING_CXX_LUA_JIT) is applied in your build
configuration.
Contributor
|
✅ Updated vcpkg baseline to 2026.01.16 ( |
|
Contributor
|
This PR is stale because it has been open 45 days with no activity. |
|
Contributor
|
Ao usar !deposit all, ou valor, !balance. |
Contributor
|
Contributor
|
Quando spamma talkaction de banco. |
Contributor
|
This PR is stale because it has been open 45 days with no activity. |
Contributor
|
This PR is stale because it has been open 45 days with no activity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This Pull Request implements the migration of the core Lua bindings (
Game,Bank, andConfignamespaces) to the Sol2 library. This feature modernizes the codebase by replacing the legacy manual Lua C API with Sol2's high-level, type-safe C++ bindings.Features
sol2as a project dependency invcpkg.jsonand updated CMake configuration (BaseConfig.cmake,CanaryLib.cmake) to link against it.GameFunctions(game_functions.cpp) to use Sol2, covering all game-related logic (items, monsters, players, etc.).BankFunctions(bank_functions.cpp) to use Sol2 for bank and transfer operations.ConfigFunctions(config_functions.cpp) to use Sol2, including automatic enum exposure for configuration keys.Benefits
Compatibility
Creature,Item,ItemClassification) to objects returned by Sol2, ensuring existing scripts continue to work without modification.pairs()andunpack().Summary by CodeRabbit