(feat/sol): Switch Lua C API to SOL#390
Closed
iThorgrim wants to merge 45 commits into
Closed
Conversation
- Introduces ObjectRef and derived classes for safe handling of game objects in Lua. - Ensures that Lua scripts do not use raw pointers, preventing use-after-free errors. - Implements caching and resolution logic for object handles.
- Replace function reference with event ID for better clarity - Remove unnecessary extern "C" block for Lua includes - Streamline event addition and removal processes
… to use sol::protected_function
…ility - Replaced multiple Push calls with a single CallAll function - Improved handling of addon messages in ServerHooks - Streamlined event handling in SpellHooks, TicketHooks, and VehicleHooks
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR migrates the ALE Lua scripting engine from the raw Lua 5.1 C API (lua_State*, manual stack manipulation, luaL_ref-based callback storage, lmarshal serialization) to sol2/sol-based bindings throughout the codebase, affecting the core engine, event manager, handle/reference system, binding maps, all hooks, and all Lua method implementations. Legacy helper files (ALETemplate.h, ALEIncludes.h, HookHelpers.h, lmarshal.cpp/h, ALECompat.cpp/h) are removed and replaced with sol2-native equivalents (ALEBind.h/cpp, ALEHandles.h/cpp, ALEScriptCache.h/cpp, ALERegistry.h). ChangesCore sol2 engine migration
Hook dispatch migration
Lua method binding migration
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant LuaScript
participant ALEEngine as ALE (sol::state)
participant BindingMap
participant EventKey
participant EngineHook as Hook (e.g. CreatureHooks)
LuaScript->>ALEEngine: Register(regtype, event_id, callback)
ALEEngine->>BindingMap: Insert(key, sol::protected_function, shots)
BindingMap-->>ALEEngine: bindingId
ALEEngine-->>LuaScript: cancel callable
EngineHook->>ALEEngine: CallAll(*Bindings, key, args...)
ALEEngine->>BindingMap: GetCallbacksFor(key)
BindingMap-->>ALEEngine: vector<sol::protected_function>
loop each callback
ALEEngine->>LuaScript: Call(callback, args...)
LuaScript-->>ALEEngine: sol::protected_function_result
end
ALEEngine-->>EngineHook: aggregated result
sequenceDiagram
participant Loader
participant ALEScriptCache
participant FileSystem
participant SolState as sol::state
Loader->>ALEScriptCache: Load(lua, filepath, isMoonScript)
ALEScriptCache->>FileSystem: GetFileMTime(filepath)
alt cache hit (mtime matches)
ALEScriptCache->>ALEScriptCache: LoadCompiled from cached bytecode
else cache miss
ALEScriptCache->>SolState: compile chunk in throwaway state
ALEScriptCache->>ALEScriptCache: store bytecode + mtime
ALEScriptCache->>ALEScriptCache: LoadCompiled(lua, filepath)
end
ALEScriptCache-->>Loader: sol::protected_function
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/LuaEngine/methods/LootMethods.cpp (1)
76-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude quest loot in item lookups and looted-state updates.
AddItem(..., needsQuest=true)andRemoveItemhandlequest_items, butHasItemandSetItemLootedonly check regular loot, so quest loot reports incorrectly.Proposed fix
bool HasItem(Loot* loot, sol::optional<uint32> itemId, sol::optional<uint32> countArg) { uint32 itemid = itemId.value_or(0); uint32 count = countArg.value_or(0); - bool has_item = false; - if (itemid) - { - for (LootItem const& lootitem : loot->items) - { - if (lootitem.itemid == itemid && (count == 0 || lootitem.count == count)) - { - has_item = true; - break; - } - } - } - else - { - for (LootItem const& lootitem : loot->items) - { - if (lootitem.itemid != 0) - { - has_item = true; - break; - } - } - } + auto contains = [itemid, count](std::vector<LootItem> const& container) + { + for (LootItem const& lootitem : container) + if (itemid ? (lootitem.itemid == itemid && (count == 0 || lootitem.count == count)) : lootitem.itemid != 0) + return true; - return has_item; + return false; + }; + + return contains(loot->items) || contains(loot->quest_items); @@ void SetItemLooted(Loot* loot, uint32 itemid, uint32 count, sol::optional<bool> lootedArg) { bool looted = lootedArg.value_or(true); - for (auto& lootItem : loot->items) + auto mark = [itemid, count, looted](std::vector<LootItem>& container) { - if (lootItem.itemid == itemid && (count == 0 || lootItem.count == count)) + for (auto& lootItem : container) { - lootItem.is_looted = looted; - break; + if (lootItem.itemid == itemid && (count == 0 || lootItem.count == count)) + { + lootItem.is_looted = looted; + return true; + } } - } + + return false; + }; + + if (!mark(loot->items)) + mark(loot->quest_items); }Also applies to: 323-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/LuaEngine/methods/LootMethods.cpp` around lines 76 - 105, The HasItem and SetItemLooted logic in LootMethods currently ignores quest_items, so quest loot is not reflected in item lookups or looted-state updates. Update the HasItem function to search both loot->items and loot->quest_items (matching itemId/count semantics), and update SetItemLooted to mark the corresponding quest item entries as looted as well as regular loot. Use the existing AddItem(..., needsQuest=true) and RemoveItem behavior as the reference for handling quest loot consistently.src/LuaEngine/methods/CreatureMethods.cpp (1)
608-639: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix random target selection’s count semantics.
For
SELECT_TARGET_RANDOM,positionis documented as the number of top threat entries to choose from, but Line 608 rejectsposition == targetList.size()and Line 636 samples0..positioninclusively.Proposed fix
- if (position >= targetList.size()) - return nullptr; + if (targetType == SELECT_TARGET_RANDOM) + { + if (position > targetList.size()) + return nullptr; + } + else if (position >= targetList.size()) + return nullptr; @@ - if (position) - std::advance(itr, urand(0, position)); + if (position) + std::advance(itr, urand(0, position - 1));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/LuaEngine/methods/CreatureMethods.cpp` around lines 608 - 639, The random target selection logic in CreatureMethods::selectTarget uses incorrect count semantics for SELECT_TARGET_RANDOM: it rejects position equal to the list size and samples an inclusive upper bound, which makes the documented “number of top threat entries” behavior off by one. Update the bounds check and the urand/advance logic in the SELECT_TARGET_RANDOM branch so position is treated as a count of eligible entries, matching the behavior used by the other target selection cases in this method.src/LuaEngine/LuaEngine.cpp (1)
694-699: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not clear map-level bindings with an instance-id key.
MapEventBindingsare registered withentryon Line 581, whileFreeInstanceId()builds the key frominstanceId; this can miss intended cleanup and accidentally remove a map binding whose map id equals the freed instance id.Proposed fix
for (int i = 1; i < Hooks::INSTANCE_EVENT_COUNT; ++i) { auto key = EntryKey<Hooks::InstanceEvents>((Hooks::InstanceEvents)i, instanceId); - MapEventBindings->Clear(key); InstanceEventBindings->Clear(key); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/LuaEngine/LuaEngine.cpp` around lines 694 - 699, `FreeInstanceId()` is clearing `MapEventBindings` with an instance-id-based `EntryKey`, which can target the wrong map binding and miss the intended cleanup. Update the cleanup loop in `FreeInstanceId()` so `InstanceEventBindings` still uses the instance key, but `MapEventBindings` is cleared using the map’s own identifier/context instead of `instanceId`. Use the existing `MapEventBindings` registration path in `MapEvent`/`entry` as the reference for the correct key shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/LuaEngine/ALEBind.cpp`:
- Around line 25-29: ToLuaDynamic is returning the base
CreatureRef/GameObjectRef before it has a chance to preserve PetRef and
TransportRef, which hides the derived Lua APIs. Update the dynamic conversion
order in ToLuaDynamic in ALEBind.cpp so concrete Pet and Transport checks are
performed before the broader Creature/GameObject branches, and keep the existing
object-type handling intact for the fallback cases.
In `@src/LuaEngine/ALEBind.h`:
- Line 362: Update the Lua-facing type alias for AuctionHouseObject in
ALETypeName so stale-handle errors report the current exposed name instead of
the removed compatibility name. Change the specialization for
ALETypeName<AuctionHouseObject> in ALEBind.h to use the active Lua-facing
identifier consistently with the rest of the binding names, and verify any
related diagnostics or script-facing messages reference the same symbol.
In `@src/LuaEngine/ALEInstanceAI.cpp`:
- Around line 50-71: EncodeTable currently recurses through nested Lua tables
without detecting cycles, so a self-referential table can loop forever during
Save(). Update EncodeTable and the callers in ALEInstanceAI::Save/EncodeValue to
track visited table identities (or otherwise reject already-seen tables) before
descending into nested tables. If a cycle is found, stop saving and surface a
clear save error instead of continuing recursion.
- Around line 112-120: The string decoding path is allocating based on the
persisted length before verifying enough bytes remain, which can let malformed
blobs request huge allocations. Update the string-read logic that uses
reader.Read to first validate the remaining unread bytes against the declared
length and only allocate/copy after that check passes. Apply the same guard in
both affected decode sites, using the existing reader helper and the length
field as the key symbols to locate the change.
In `@src/LuaEngine/ALEScriptCache.cpp`:
- Around line 22-35: The cache stamp in GetFileModTime and the
timestampCache/CacheEntry logic only uses std::time_t second-level mtime, so
edits within the same tick can reuse stale bytecode. Update the cache keying in
ALEScriptCache to use a higher-fidelity stamp, such as higher-resolution
modified time plus file size or a source hash, and make the reuse check in the
bytecode cache compare that richer metadata instead of std::time_t alone.
In `@src/LuaEngine/hooks/ServerHooks.cpp`:
- Around line 60-61: The channel identifier passed to Lua is inconsistent
between player hooks and addon messages; update the addon-message path in
ServerHooks::... to use the same channel identifier contract as PlayerHooks.cpp,
which means preserving custom channel context as a negative DB id instead of
always calling GetChannelId(). Adjust the else-if branch that builds target with
sol::make_object so Lua handlers receive the same channel context format for
both code paths.
In `@src/LuaEngine/HttpManager.cpp`:
- Around line 238-246: The failure path in HttpManager::ProcessResponse is being
skipped because the callback guard only allows res->statusCode >= 0, so Lua
never receives request failures enqueued with statusCode == -1. Update the
response handling around sALE->CallFunction so failed requests also invoke the
Lua callback, and pass through the failure status and any available body/headers
data consistently rather than dropping the response.
- Around line 169-172: The redirected request result from DoRequest in
HttpManager::FinishRequest flow is being dereferenced without checking for
failure, so guard the res pointer before accessing res->status, res->body, and
res->headers. In the request-handling logic around DoRequest and FinishRequest,
return or handle the error path when the redirected request does not produce a
valid response, and only call FinishRequest with response data after confirming
res is non-null/valid.
In `@src/LuaEngine/LuaEngine.cpp`:
- Around line 99-106: Guard the script path before accessing its first character
in LuaEngine::LuaEngine. The current lua_folderpath[0] check is unsafe when
ALEConfig::GetInstance().GetScriptPath() returns an empty string; add an
empty-check before the '~' expansion logic so the HOME replacement only runs
when lua_folderpath is non-empty. Keep the fix localized around lua_folderpath
and the non-Windows path handling.
In `@src/LuaEngine/LuaEngine.h`:
- Around line 144-149: Make dispatch depth exception-safe by ensuring every
EnterDispatch() is paired with LeaveDispatch() even when argument conversion or
the Lua callback throws. Add an RAII/scope guard around the callback path that
increments event_level on construction and decrements it on destruction, so the
logic in LeaveDispatch() still runs and ALEHandleEpoch::Bump() can resume when
depth returns to zero.
In `@src/LuaEngine/methods/AuraMethods.cpp`:
- Around line 51-54: GetCasterLevel currently dereferences aura->GetCaster()
without checking for null, which can crash when the caster is missing. Update
GetCasterLevel in AuraMethods.cpp to validate the Aura caster before accessing
GetLevel, and return a safe fallback or surface a Lua-visible error when
aura->GetCaster() is nullptr.
In `@src/LuaEngine/methods/BattleGroundMethods.cpp`:
- Around line 147-150: GetMinPlayers in BattleGroundMethods.cpp is using the
max-per-team value, so it returns the maximum total instead of the minimum
total. Update GetMinPlayers(Battleground* bg) to derive the total from the
battleground’s minimum per-team player count rather than GetMaxPlayersPerTeam,
and keep the logic consistent with the existing GetMaxPlayers helper.
In `@src/LuaEngine/methods/CreatureMethods.cpp`:
- Around line 1061-1064: `AttackStart` currently dereferences `creature->AI()`
unconditionally, which can crash when the creature has no enabled AI. Update the
`AttackStart(Creature* creature, Unit* target)` method in `CreatureMethods.cpp`
to first guard with `IsAIEnabled` (matching the pattern used by other AI entry
points in this file) before calling `AI()->AttackStart(target)`, and skip the
call or return early when AI is unavailable.
In `@src/LuaEngine/methods/GameObjectMethods.cpp`:
- Around line 221-249: In AddLoot, validate the entire variadic arg list before
any DB or in-memory changes are made, since the current loop in
GameObjectMethods.cpp can partially save items to CharacterDatabase and go->loot
before a later invalid pair throws. Update AddLoot to first parse/validate all
entry/amount pairs (including supporting the documented default amount case for
AddLoot(entry)), then only after validation create items, save them, and add
them to loot, using the AddLoot and Item::CreateItem flow as the main
touchpoints.
In `@src/LuaEngine/methods/GemPropertiesEntryMethods.cpp`:
- Around line 25-26: The documentation for GemPropertiesEntryMethods is stale
because it still describes pushing the ID onto the Lua stack, but the method now
returns a uint32 through sol. Update the comment for the relevant method in
GemPropertiesEntryMethods so it accurately states the direct return behavior and
removes Lua-stack wording, keeping the description aligned with the current
return path.
In `@src/LuaEngine/methods/ItemTemplateMethods.cpp`:
- Around line 199-205: Guard the item display lookup in GetIcon before
dereferencing the result: sItemDisplayInfoStore.LookupEntry(display_id) can
return null for invalid or custom DisplayInfoID values, so add a nullptr check
around displayInfo and handle the missing case safely instead of accessing
displayInfo->inventoryIcon. Use the GetIcon and LookupEntry(display_id) symbols
to locate the fix, and return a safe fallback when no ItemDisplayInfoEntry is
found.
- Around line 61-69: GetName in ItemTemplateMethods currently uses the
Lua-provided locale directly to index itemLocale->Name, which can go out of
bounds. Add a guard before the array access to validate loc_idx against
TOTAL_LOCALES and only use the localized name when the index is within range and
the entry is non-empty; otherwise keep the default itemTemplate->Name1.
---
Outside diff comments:
In `@src/LuaEngine/LuaEngine.cpp`:
- Around line 694-699: `FreeInstanceId()` is clearing `MapEventBindings` with an
instance-id-based `EntryKey`, which can target the wrong map binding and miss
the intended cleanup. Update the cleanup loop in `FreeInstanceId()` so
`InstanceEventBindings` still uses the instance key, but `MapEventBindings` is
cleared using the map’s own identifier/context instead of `instanceId`. Use the
existing `MapEventBindings` registration path in `MapEvent`/`entry` as the
reference for the correct key shape.
In `@src/LuaEngine/methods/CreatureMethods.cpp`:
- Around line 608-639: The random target selection logic in
CreatureMethods::selectTarget uses incorrect count semantics for
SELECT_TARGET_RANDOM: it rejects position equal to the list size and samples an
inclusive upper bound, which makes the documented “number of top threat entries”
behavior off by one. Update the bounds check and the urand/advance logic in the
SELECT_TARGET_RANDOM branch so position is treated as a count of eligible
entries, matching the behavior used by the other target selection cases in this
method.
In `@src/LuaEngine/methods/LootMethods.cpp`:
- Around line 76-105: The HasItem and SetItemLooted logic in LootMethods
currently ignores quest_items, so quest loot is not reflected in item lookups or
looted-state updates. Update the HasItem function to search both loot->items and
loot->quest_items (matching itemId/count semantics), and update SetItemLooted to
mark the corresponding quest item entries as looted as well as regular loot. Use
the existing AddItem(..., needsQuest=true) and RemoveItem behavior as the
reference for handling quest loot consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 958b6cc2-d18d-4d38-8404-479722a1c07b
📒 Files selected for processing (82)
CMakeLists.txtsrc/LuaEngine/ALEBind.cppsrc/LuaEngine/ALEBind.hsrc/LuaEngine/ALECompat.cppsrc/LuaEngine/ALECompat.hsrc/LuaEngine/ALECreatureAI.hsrc/LuaEngine/ALEDBCRegistry.hsrc/LuaEngine/ALEEventMgr.cppsrc/LuaEngine/ALEEventMgr.hsrc/LuaEngine/ALEHandles.cppsrc/LuaEngine/ALEHandles.hsrc/LuaEngine/ALEIncludes.hsrc/LuaEngine/ALEInstanceAI.cppsrc/LuaEngine/ALEScriptCache.cppsrc/LuaEngine/ALEScriptCache.hsrc/LuaEngine/ALETemplate.hsrc/LuaEngine/ALEUtility.cppsrc/LuaEngine/ALEUtility.hsrc/LuaEngine/BindingMap.hsrc/LuaEngine/HookHelpers.hsrc/LuaEngine/HttpManager.cppsrc/LuaEngine/HttpManager.hsrc/LuaEngine/LuaEngine.cppsrc/LuaEngine/LuaEngine.hsrc/LuaEngine/LuaFunctions.cppsrc/LuaEngine/hooks/AllCreatureHooks.cppsrc/LuaEngine/hooks/BattleGroundHooks.cppsrc/LuaEngine/hooks/CreatureHooks.cppsrc/LuaEngine/hooks/GameObjectHooks.cppsrc/LuaEngine/hooks/GossipHooks.cppsrc/LuaEngine/hooks/GroupHooks.cppsrc/LuaEngine/hooks/GuildHooks.cppsrc/LuaEngine/hooks/InstanceHooks.cppsrc/LuaEngine/hooks/ItemHooks.cppsrc/LuaEngine/hooks/PacketHooks.cppsrc/LuaEngine/hooks/PlayerHooks.cppsrc/LuaEngine/hooks/ServerHooks.cppsrc/LuaEngine/hooks/SpellHooks.cppsrc/LuaEngine/hooks/TicketHooks.cppsrc/LuaEngine/hooks/VehicleHooks.cppsrc/LuaEngine/lmarshal.cppsrc/LuaEngine/lmarshal.hsrc/LuaEngine/methods/ALEQueryMethods.cppsrc/LuaEngine/methods/ALERegistry.hsrc/LuaEngine/methods/AchievementMethods.cppsrc/LuaEngine/methods/AuraMethods.cppsrc/LuaEngine/methods/BattleGroundMethods.cppsrc/LuaEngine/methods/BattleGroundMethods.hsrc/LuaEngine/methods/ChatHandlerMethods.cppsrc/LuaEngine/methods/CorpseMethods.cppsrc/LuaEngine/methods/CreatureMethods.cppsrc/LuaEngine/methods/GameObjectMethods.cppsrc/LuaEngine/methods/GemPropertiesEntryMethods.cppsrc/LuaEngine/methods/GlobalMethods.cppsrc/LuaEngine/methods/GroupMethods.cppsrc/LuaEngine/methods/GuildMethods.cppsrc/LuaEngine/methods/ItemMethods.cppsrc/LuaEngine/methods/ItemTemplateMethods.cppsrc/LuaEngine/methods/ItemTemplateMethods.hsrc/LuaEngine/methods/LootMethods.cppsrc/LuaEngine/methods/MapMethods.cppsrc/LuaEngine/methods/ObjectMethods.cppsrc/LuaEngine/methods/PetMethods.cppsrc/LuaEngine/methods/PlayerMethods.cppsrc/LuaEngine/methods/QuestMethods.cppsrc/LuaEngine/methods/RollMethods.cppsrc/LuaEngine/methods/SpellEntryMethods.cppsrc/LuaEngine/methods/SpellInfoMethods.cppsrc/LuaEngine/methods/SpellMethods.cppsrc/LuaEngine/methods/TicketMethods.cppsrc/LuaEngine/methods/TicketMethods.hsrc/LuaEngine/methods/TransportMethods.cppsrc/LuaEngine/methods/UnitMethods.cppsrc/LuaEngine/methods/VehicleMethods.cppsrc/LuaEngine/methods/WorldObjectMethods.cppsrc/LuaEngine/methods/WorldPacketMethods.cppsrc/lualib/luajit/CMakeLists.txtsrc/lualib/sol/LICENSE.txtsrc/lualib/sol/README.mdsrc/lualib/sol/config.hppsrc/lualib/sol/forward.hppsrc/lualib/sol/sol.hpp
💤 Files with no reviewable changes (10)
- src/LuaEngine/lmarshal.cpp
- src/LuaEngine/lmarshal.h
- src/LuaEngine/HookHelpers.h
- src/LuaEngine/ALETemplate.h
- src/LuaEngine/ALECompat.h
- src/LuaEngine/methods/ItemTemplateMethods.h
- src/LuaEngine/ALECompat.cpp
- src/LuaEngine/ALEIncludes.h
- src/LuaEngine/ALEUtility.h
- src/LuaEngine/methods/BattleGroundMethods.h
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
1 participant
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.
Summary
This PR is a complete rewrite of ALE's binding layer: the entire engine moves from the raw Lua C API (
lua_State*, manual stack manipulation,luaL_Regtables) to sol3 (v3.3.0, vendored single-header). No legacy binding code remains.The goal is readability: a contributor who has never touched the Lua C API can now read, fix and add a binding. Every method is a plain named C++ function with typed parameters, no stack indices, no
CHECKVAL/CHECKOBJmacros, no manual push/return counting:The Lua-facing API keeps the Eluna style (
RegisterPlayerEvent(id, fn), same method names, same events), so most scripts keep working, see Breaking changes below for the exceptions.Highlights
Safe object handles
Raw pointers never cross into Lua anymore. Every game object is wrapped in a handle (
PlayerRef,CreatureRef,ItemRef, ...) that stores itsObjectGuidplus enough context to find it again, and is re-resolved through the core accessors on every use:PlayerRef : UnitRef : WorldObjectRef : ObjectRef), so a Player can be passed to any function expecting a Unit, and polymorphic returns arrive in Lua as their most-derived type;Map/Group/Guildresolve through their managers; transient types with no lookup path (Aura,Spell,Vehicle,Loot, ...) use a scoped handle that is only valid during the event that provided it, matching the old engine's guarantees, but enforced instead of assumed.New:
Petis now a real Lua type inheriting the wholeCreature/Unitchain (the legacy engine definedPetMethods[]but never registered it).One binding style everywhere
Each
methods/<Type>Methods.cppcontains the documented named functions and, at the bottom, a single registration function listing every binding:ALEBind(new) does all conversions: handles resolve to live pointers on the way in, returned pointers are wrapped back into handles (nullptr becomes nil), and everything else goes through sol's usual conversions. Each methods file is its own translation unit, so binding-heavy template instantiation parallelizes across cores instead of funneling through one giant TU.Legacy code removed
ALETemplate.h,ALECompat.h/.cpp,ALEIncludes.h,HookHelpers.h, deleted.lmarshal(lua-marshal C library), deleted; instance data persistence now uses a small bounds-checked binary serializer inALEInstanceAI.cpp."long long"/"unsigned long long"userdata, replaced by a realObjectGuidusertype (GetCounter,GetEntry,IsPlayer,tostring,==).LuaFunctions.cppshrinks from ~2000 lines of registration tables to a slimRegisterFunctionscalling one register function per type..out, bytecode cache all preserved) and the HTTP manager were rewritten on sol primitives (sol::protected_function, RAII references).Legacy bugs fixed along the way
HttpManagerleaked the work item and the callback reference on every early-exit path of the worker loop.BindingMapkept danglingBindingList*pointers in its id lookup table.RemoveEventByIdread itsall_eventsflag from the wrong stack slot.GetMapEntrancereturned 5 values with only 4 pushed (stack garbage as 5th).(
GetStat,GetClassAsString,AddAura, ...), they now return nil explicitly.C++ exception; sol is now configured (SOL_EXCEPTIONS_SAFE_PROPAGATION=0) to convert them into regular Lua errors carrying the real message.Breaking changes (Lua scripts)
GetGUID()return anObjectGuidvalue with methods (:GetCounter(),:GetEntry(),:IsPlayer(),tostring(guid),guid1 == guid2) instead of a boxed 64-bit number.GetGUIDLow()still returns a number.CreateLongLong/CreateULongLongreturn plain Lua numbers.Register*Eventreturns a cancel callable instead of a numeric reference; call it to unregister the handler.CreateLuaEventids are plain numbers as before but 64-bit.WorldDBQuery& co. was removed (build the string in Lua).AuctionHouseEntryis gone, the legacy type had zero methods and nothing ever produced a value of it.pcall) instead of being undefined behaviour.Co-Authored-By: Claude Fable 5 [email protected]
Summary by CodeRabbit
New Features
Bug Fixes
Refactor