Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request migrates the outfit and mount system from C++-based static XML data to a Lua-based dynamic system with storage-backed state. The change eliminates static mounts.xml and outfits.xml files, removes C++ implementations (src/mounts.* and src/outfit.*), consolidates database tables into player_storage, introduces new Lua modules for outfit/mount management, adds network packet handlers and admin commands, and updates the configuration system accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as ProtocolGame
participant OutfitSystem as Outfit System<br/>(Lua)
participant PlayerStorage as Player Storage
participant Database as player_storage
Client->>Server: 0xD3 (Set Outfit)
Server->>OutfitSystem: set_outfit.lua onReceive
OutfitSystem->>OutfitSystem: Validate outfit wearable<br/>(access, premium, unlocked, addons)
alt Mount Selected
OutfitSystem->>OutfitSystem: Validate mount ownership<br/>via canRideMount
OutfitSystem->>PlayerStorage: setCurrentMount(mountId)
PlayerStorage->>Database: Update player_storage<br/>[currentMount]
end
OutfitSystem->>PlayerStorage: setRandomizeMount(bool)
PlayerStorage->>Database: Update player_storage<br/>[randomizeMount]
OutfitSystem->>PlayerStorage: setCurrentOutfit(outfit)
PlayerStorage->>Database: Store outfit data
Server-->>Client: Outfit applied
sequenceDiagram
participant Client
participant Server as ProtocolGame
participant MountSystem as Mount System<br/>(Lua)
participant Outfit as Outfit System
participant PlayerStorage as Player Storage
Client->>Server: 0xD4 (Toggle Mount)
Server->>MountSystem: toggle_mount.lua onReceive
alt AllowToggleMount = true
MountSystem->>MountSystem: Check cooldown<br/>(getLastMountToggle)
MountSystem->>MountSystem: Check protection zone<br/>exemptions
MountSystem->>MountSystem: Check mount ownership<br/>(hasMount)
MountSystem->>Outfit: mount/dismount<br/>applies outfit + speed
MountSystem->>PlayerStorage: setLastMountToggle(now)
MountSystem->>PlayerStorage: setWasMounted(state)
else AllowToggleMount = false
MountSystem->>Server: Early exit
end
Server-->>Client: Mount state updated
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 12
🧹 Nitpick comments (11)
src/script.cpp (1)
59-71: LGTM! Consider standardizing the path access for consistency.The relative path logging improves readability for error, loaded, and reloaded messages. However, there's a minor inconsistency: Line 38 uses
it->path().lexically_relative(dir)while these lines useit->lexically_relative(dir)(without explicit.path()). Both forms work, but standardizing on one approach would improve maintainability.🔎 Optional: Standardize to explicit `.path()` for consistency
- std::cout << "> " << it->lexically_relative(dir).string() << " [error]" << std::endl; + std::cout << "> " << it->path().lexically_relative(dir).string() << " [error]" << std::endl;- std::cout << "> " << it->lexically_relative(dir).string() << " [loaded]" << std::endl; + std::cout << "> " << it->path().lexically_relative(dir).string() << " [loaded]" << std::endl;- std::cout << "> " << it->lexically_relative(dir).string() << " [reloaded]" << std::endl; + std::cout << "> " << it->path().lexically_relative(dir).string() << " [reloaded]" << std::endl;data/scripts/systems/outfits/config.lua (1)
1-4: Consider adding documentation comments.The configuration is clear, but adding brief comments explaining when/where these settings are used would improve maintainability.
🔎 Suggested enhancement
+-- Global outfit system configuration Outfits = { + -- Allow players to change outfits (affects outfit window access) AllowChangeOutfit = true, + -- Cooldown between mount/dismount actions in milliseconds ToggleMountCooldown = 3000, -- in milliseconds }data/migrations/37.lua (1)
19-19: VariableresultIdis redeclared in the same scope.The variable
resultIdis already declared on line 6 and is being redeclared on line 19. While Lua allows this, it shadows the previous variable and could be confusing. Consider using a different variable name for clarity.🔎 Suggested fix
- local resultId = db.storeQuery("SELECT `player_id`, `mount_id` FROM `player_mounts`") - if resultId then + local mountResultId = db.storeQuery("SELECT `player_id`, `mount_id` FROM `player_mounts`") + if mountResultId then repeat - local playerId = result.getNumber(resultId, "player_id") - local mountId = result.getNumber(resultId, "mount_id") + local playerId = result.getNumber(mountResultId, "player_id") + local mountId = result.getNumber(mountResultId, "mount_id") local storageKey = PlayerStorageKeys.mountsBase + mountId table.insert(rows, {playerId = playerId, key = storageKey, value = 1}) - until not result.next(resultId) - result.free(resultId) + until not result.next(mountResultId) + result.free(mountResultId) enddata/scripts/systems/outfits/data/mounts.lua (2)
213-224:legacyIdfield is not exposed inGame.getMounts()return value.The mount data includes a
legacyIdfield, but neitherGame.getMounts()norGame.getMountByLookType()includes it in the returned object. IflegacyIdis needed by consumers (e.g., for protocol compatibility), consider including it in the returned data. If it's not needed externally, consider removing it from the data definition to avoid confusion.
1-3: Inconsistent indentation style.Line 1 uses spaces for indentation while subsequent lines use tabs. Consider standardizing on one style throughout the file for consistency.
data/scripts/systems/outfits/events.lua (1)
8-8: RedundantgetPlayer()call.Since
creature:isPlayer()already returned true,creatureis already the player. ThegetPlayer()call is unnecessary.🔎 Suggested fix
- local player = creature:getPlayer() - if toZone == ZONE_PROTECTION then - if not player:getGroup():getAccess() and player:isMounted() then - player:toggleMount(false) - player:setWasMounted(true) + if toZone == ZONE_PROTECTION then + if not creature:getGroup():getAccess() and creature:isMounted() then + creature:toggleMount(false) + creature:setWasMounted(true) end - elseif player:getWasMounted() then - player:toggleMount(true) - player:setWasMounted(false) + elseif creature:getWasMounted() then + creature:toggleMount(true) + creature:setWasMounted(false) enddata/scripts/systems/outfits/data/outfits.lua (2)
227-241: Consider sorting results for consistent outfit display order.
Game.getOutfitsusespairs()which doesn't guarantee iteration order. This may cause outfits to appear in different orders each time the outfit window is opened. If consistent ordering matters for UX, consider sorting the results bylookTypeornamebefore returning.🔎 Suggested addition
function Game.getOutfits(sex) local result = {} for lookType, outfit in pairs(outfits) do if outfit.sex == sex and outfit.enabled then table.insert(result, { lookType = lookType, name = outfit.name, sex = outfit.sex, premium = outfit.premium, unlocked = outfit.unlocked, }) end end -- Sort by lookType for consistent ordering table.sort(result, function(a, b) return a.lookType < b.lookType end) return result end
1-4: Inconsistent indentation style.Line 1 uses spaces for indentation while subsequent lines use tabs. Consider standardizing on one style throughout the file for consistency.
data/scripts/systems/outfits/network/set_outfit.lua (1)
75-78: Redundant client ID check.If
Game.getItemTypeByClientId(clientId)returns a valid item type, then by definitionit:getClientId()should equalclientId. The second part of the condition appears redundant.🔎 Suggested simplification
local it = Game.getItemTypeByClientId(clientId) - if not it or it:getClientId() ~= clientId then + if not it then return enddata/scripts/systems/outfits/core/windows.lua (2)
1-7: DuplicatecanWearOutfitlogic - consider consolidation.This local
canWearOutfitfunction has different logic fromPlayer.canWearOutfitin outfits.lua. The local version doesn't check for group access bypass, while the Player method does. This inconsistency could cause confusion about which outfits are available.Consider using
Player.canWearOutfitdirectly ingetAvailableOutfitsto ensure consistent behavior.
109-127: Inconsistent indentation - mixing spaces and tabs.Lines 110-127 use 4-space indentation while the rest of the file uses tabs. This inconsistency affects code readability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
.gitignoredata/XML/mounts.xmldata/XML/outfits.xmldata/cpplinter.luadata/events/scripts/player.luadata/lib/compat/compat.luadata/lib/core/creature.luadata/lib/core/network_message.luadata/lib/core/storages.luadata/migrations/37.luadata/migrations/38.luadata/scripts/systems/outfits/config.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/events.luadata/scripts/systems/outfits/network/request_outfit_window.luadata/scripts/systems/outfits/network/set_outfit.luaschema.sqlsrc/CMakeLists.txtsrc/creature.hsrc/game.cppsrc/game.hsrc/iologindata.cppsrc/lua/CMakeLists.txtsrc/lua/api.cppsrc/lua/api.hsrc/lua/modules.cppsrc/lua/modules/game.cppsrc/lua/modules/outfit.cppsrc/lua/modules/player.cppsrc/lua/register.hsrc/lua/script.hsrc/mounts.cppsrc/mounts.hsrc/otserv.cppsrc/outfit.cppsrc/outfit.hsrc/player.cppsrc/player.hsrc/protocolgame.cppsrc/protocolgame.hsrc/script.cppsrc/signals.cpp
💤 Files with no reviewable changes (23)
- data/XML/outfits.xml
- src/lua/script.h
- src/creature.h
- src/lua/modules/game.cpp
- src/lua/modules/outfit.cpp
- src/lua/register.h
- src/lua/api.cpp
- src/outfit.cpp
- src/outfit.h
- src/mounts.h
- src/signals.cpp
- src/game.h
- src/mounts.cpp
- data/lib/compat/compat.lua
- src/lua/modules.cpp
- data/XML/mounts.xml
- src/protocolgame.h
- src/CMakeLists.txt
- src/lua/CMakeLists.txt
- src/otserv.cpp
- src/game.cpp
- src/player.h
- src/player.cpp
🧰 Additional context used
🧬 Code graph analysis (6)
data/scripts/systems/outfits/core/windows.lua (3)
data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfits(227-241)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMounts(213-224)src/networkmessage.h (1)
NetworkMessage(15-74)
data/events/scripts/player.lua (1)
data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMountByLookType(226-238)
data/scripts/systems/outfits/network/set_outfit.lua (2)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)src/tile.h (1)
Tile(118-178)
data/scripts/systems/outfits/core/outfits.lua (1)
data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfitByLookType(243-256)
data/lib/core/network_message.lua (1)
src/items.h (1)
ItemType(259-418)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMounts(213-224)Game.getMountByLookType(226-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (18)
.gitignore (1)
187-187: Narrowing the ignore pattern to root-level config.lua is the right call.The
/prefix correctly restricts the match to the repository root, preventing accidental ignoring ofconfig.luafiles in subdirectories. This is a safer, more selective approach that aligns with the Lua-based system restructuring.src/script.cpp (1)
37-39: LGTM! Improved log readability.The change to display relative paths for disabled scripts improves log readability without affecting functionality.
data/lib/core/storages.lua (1)
52-56: LGTM!The new storage keys for outfits and mounts are well-organized and use distinct ranges that don't conflict with existing keys.
data/lib/core/creature.lua (1)
203-205: LGTM!The new
hasStorageValuehelper provides a clean way to check for storage value existence. The implementation is consistent with the existingremoveStorageValuepattern.src/lua/api.h (1)
3-3: LGTM!The change from
outfit.htoenums.his appropriate for this refactoring. The API now depends onOutfit_tstruct (viaenums.h) rather than theOutfitclass, aligning with the migration to Lua-based outfit management.schema.sql (1)
378-378: LGTM - Schema version bump is correct.The database version update to '38' aligns with the migration changes. However, ensure that the migration logic in
data/migrations/38.luaproperly handles any existing outfit/mount data before these schema changes are applied.data/events/scripts/player.lua (1)
154-156: LGTM!The API migration from
Game.getMountIdByLookType()toGame.getMountByLookType()is handled correctly. The new function returns a mount object (or nil), and the condition properly checks for both the object's existence and ownership viamount.id.data/scripts/systems/outfits/network/request_outfit_window.lua (1)
4-4: The concern is invalid. Both files are loaded in the same recursive script loading phase fromdata/scripts/, and files are sorted alphabetically. Sinceconfig.lua(atsystems/outfits/config.lua) comes beforenetwork/request_outfit_window.lua(atsystems/outfits/network/request_outfit_window.lua) in sort order, the config is guaranteed to be loaded first. Additionally, the handler function only executes at runtime when a packet arrives, well after all scripts have been loaded.Likely an incorrect or invalid review comment.
data/lib/core/network_message.lua (1)
18-39: LGTM!The
addOutfitmethod correctly serializes outfit and mount data according to the protocol structure, properly handling the conditional fields based onlookTypeandlookMountvalues.src/protocolgame.cpp (1)
723-724: LGTM!The outfit-related opcodes (0xD2 and 0xD3) are correctly commented out, allowing them to fall through to the default case which dispatches to
parsePlayerNetworkMessage. This enables the Lua-side handlers to process these packets as part of the migration.data/scripts/systems/outfits/core/outfits.lua (1)
1-12: LGTM - Storage-backed outfit and addon management.The storage-based approach using bitmasks for addons is well-implemented. The bitwise operations for adding/removing individual addons are correct.
data/scripts/systems/outfits/core/windows.lua (1)
48-107: LGTM - Outfit window construction is well-implemented.The network message building correctly handles the protocol, including the conditional mount color bytes and proper iteration over available outfits/mounts/familiars.
data/scripts/systems/outfits/core/mounts.lua (2)
95-98: Clarify thegetWasMounted()guard intoggleMount.The condition
not self:getWasMounted()prevents toggling unlesswasMountedis explicitly set to true elsewhere. This creates a dependency that isn't obvious from this code alone. Consider adding a comment explaining whenwasMountedshould be set, or verify this is the intended behavior.
75-93: LGTM - Mount/dismount implementation.The
mountanddismountfunctions correctly update the outfit's lookMount and adjust player speed based on mount speed.src/lua/modules/player.cpp (2)
1540-1588: LGTM - Clean C++ bindings for outfit get/set operations.The new outfit binding functions follow the established patterns in the codebase:
- Proper null checks with
lua_pushnilon failure- Consistent use of
tfs::lua::pushOutfitandtfs::lua::getOutfit- Returns boolean on successful set operations
2462-2465: LGTM - Method registration is correct.The registration properly maps the Lua method names to the C++ implementation functions.
data/cpplinter.lua (2)
424-436: LGTM - Type annotations for new outfit/mount API.The type annotations correctly document the new Player methods for outfit and mount management, providing good IDE support for the migrated functionality.
69-71: LGTM - Game-level mount/outfit lookup type annotations.The type annotations for
getOutfitByLookTypeandgetMountByLookTypeare correctly added.
There was a problem hiding this comment.
Pull request overview
This PR migrates outfit and mount management from C++ to Lua, removing the dedicated C++ classes and replacing them with Lua-based implementations using player storage. The changes include deleting XML configuration files, removing C++ network message handlers, and implementing new Lua scripts for outfit/mount management.
Key changes:
- Removed C++ classes
MountsandOutfitsalong with their XML loading - Migrated outfit/mount data to player storage using storage keys
- Implemented Lua-based network packet handlers and outfit/mount windows
- Updated database schema to remove dedicated tables and use storage-based approach
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mounts.{h,cpp} | Deleted mount management C++ class |
| src/outfit.{h,cpp} | Deleted outfit management C++ class |
| src/player.{h,cpp} | Removed mount/outfit methods from Player class |
| src/protocolgame.{h,cpp} | Removed C++ packet handlers for outfits |
| data/scripts/systems/outfits/ | New Lua implementation for outfits and mounts |
| schema.sql | Updated database schema and version to 38 |
| data/migrations/37.lua | Migration script to convert existing data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ranisalt
force-pushed
the
refactor/lua-outfits-mounts
branch
from
4d421f2 to
38d08c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
data/migrations/38.lua (1)
1-3: Don’t returnfalsefrom a no‑op migration; it will be treated as a failed upgrade
onUpdateDatabase()unconditionally returningfalsemeans that if38.luais ever executed by the migration runner, the upgrade will abort even though there is nothing to do here. Now that37.luacontains the real outfits/mounts migration to version 38 andschema.sqlseedsdb_versionto38, this file should either:
- return
true(no-op success), or- be removed / renumbered in line with how the migration runner maps versions to files.
Otherwise, older databases that need to advance through version 38 risk failing the migration sequence.
🧹 Nitpick comments (1)
data/migrations/37.lua (1)
2-54: Outfits/mounts →player_storagemigration looks correct; a couple of minor nitsThe migration cleanly:
- Reads all outfits/mounts,
- Maps them into
player_storagewithPlayerStorageKeys.outfitsBase/mountsBase, and- Drops the obsolete tables/columns using synchronous
db.querywith proper error propagation.That addresses the earlier async‑migration/data‑loss concern.
Two minor points you may want to double‑check:
- For very large databases, the single bulk
INSERT ... VALUES (...)could grow big; if you expect millions of rows, consider batching it in chunks (purely an operational nicety).- Ensure
PlayerStorageKeys.outfitsBase/mountsBaseare reserved ranges that don’t collide with any existing storage keys.The version string in the
37.luais slightly confusing but not functionally problematic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
.gitignoredata/XML/mounts.xmldata/XML/outfits.xmldata/cpplinter.luadata/lib/compat/compat.luadata/lib/core/creature.luadata/lib/core/network_message.luadata/lib/core/storages.luadata/migrations/37.luadata/migrations/38.luadata/scripts/events/player.luadata/scripts/systems/outfits/config.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/events.luadata/scripts/systems/outfits/network/request_outfit_window.luadata/scripts/systems/outfits/network/set_outfit.luaschema.sqlsrc/CMakeLists.txtsrc/creature.hsrc/game.cppsrc/game.hsrc/iologindata.cppsrc/lua/CMakeLists.txtsrc/lua/api.cppsrc/lua/api.hsrc/lua/modules.cppsrc/lua/modules/game.cppsrc/lua/modules/outfit.cppsrc/lua/modules/player.cppsrc/lua/register.hsrc/lua/script.hsrc/mounts.cppsrc/mounts.hsrc/otserv.cppsrc/outfit.cppsrc/outfit.hsrc/player.cppsrc/player.hsrc/protocolgame.cppsrc/protocolgame.hsrc/script.cppsrc/signals.cpp
💤 Files with no reviewable changes (23)
- data/XML/outfits.xml
- data/lib/compat/compat.lua
- src/mounts.h
- src/lua/api.cpp
- src/lua/CMakeLists.txt
- src/signals.cpp
- src/lua/modules.cpp
- src/protocolgame.h
- src/lua/modules/outfit.cpp
- src/outfit.cpp
- src/outfit.h
- src/lua/register.h
- src/CMakeLists.txt
- src/creature.h
- src/lua/modules/game.cpp
- data/XML/mounts.xml
- src/game.cpp
- src/mounts.cpp
- src/game.h
- src/otserv.cpp
- src/player.cpp
- src/lua/script.h
- src/player.h
🚧 Files skipped from review as they are similar to previous changes (10)
- data/scripts/systems/outfits/network/request_outfit_window.lua
- data/lib/core/creature.lua
- data/lib/core/network_message.lua
- data/scripts/systems/outfits/network/set_outfit.lua
- .gitignore
- src/lua/api.h
- data/scripts/systems/outfits/config.lua
- src/script.cpp
- data/cpplinter.lua
- data/lib/core/storages.lua
🧰 Additional context used
🧬 Code graph analysis (5)
data/scripts/events/player.lua (1)
data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMountByLookType(226-238)
data/scripts/systems/outfits/core/outfits.lua (1)
data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfitByLookType(243-256)
data/scripts/systems/outfits/core/windows.lua (3)
data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfits(227-241)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMounts(213-224)src/networkmessage.h (1)
NetworkMessage(15-74)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMounts(213-224)Game.getMountByLookType(226-238)
src/lua/modules/player.cpp (1)
src/lua/api.cpp (6)
pushOutfit(317-332)pushOutfit(317-317)getOutfit(161-182)getOutfit(161-161)pushBoolean(294-294)pushBoolean(294-294)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu
- GitHub Check: format
- GitHub Check: docker-image
- GitHub Check: windows
- GitHub Check: Analyze (c-cpp)
- GitHub Check: test
🔇 Additional comments (36)
src/iologindata.cpp (1)
92-110: SELECT column list now matches dropped mount schemaRemoving
currentmount/randomizemountfrom bothloadPlayerByIdandloadPlayerByNamealigns the queries with the updatedplayerstable and the currentloadPlayerfield usage. No further issues spotted here.src/protocolgame.cpp (1)
723-725: Outfit opcodes 0xD2/0xD3 are now delegated via the default handler—verify Lua wiringBy commenting out the explicit
case 0xD2/0xD3entries, these packets will now fall through to thedefaultbranch and be handled byg_game.parsePlayerNetworkMessage, which is consistent with moving outfit logic to Lua.Please double‑check that your Lua
PacketHandlers/Player:onNetworkMessagetable actually defines handlers for 0xD2 (outfit window) and 0xD3 (set outfit), so these packets don’t become effective no‑ops.schema.sql (1)
377-377: Confirmdb_version = 38stays in sync with the server’s database version constant and migration layoutSeeding
server_config.db_versionto38is consistent with the new schema and the outfits/mounts migration. Just make sure:
- The core/database version constant in C++ is also
38, and- The migration runner expects the current layout (logic in
37.luaand the no‑op38.lua),so that older databases advance cleanly to 38 without skipping or re‑running the outfits/mounts migration.
src/lua/modules/player.cpp (1)
1540-1588: New Lua outfit APIs are consistent with existing binding helpers
getCurrentOutfit/getDefaultOutfitand their setters correctly:
- Resolve the
PlayerviagetSharedPtr,- Use the shared
pushOutfit/getOutfithelpers for marshalling, and- Follow the usual pattern of returning
nilwhen the player userdata is invalid andtrueon successful setters.Assuming
Player::setCurrentOutfit/setDefaultOutfitthemselves handle notifying the client, this binding layer looks solid.Also applies to: 2463-2466
data/scripts/events/player.lua (1)
120-129: Podium mount validation viaGame.getMountByLookTypelooks good—just confirmhasMountsemanticsSwitching to:
local mount = Game.getMountByLookType(outfit.lookMount) if not mount or not self:hasMount(mount.id) then outfit.lookMount = 0 endcorrectly clears podium mounts that are either unknown or not owned by the player, aligning with the new lookType‑based mount API.
Please just verify that
self:hasMount()is defined to take the sameidthatGame.getMountByLookTypereturns (i.e., no mismatch between lookType vs internal mount id).data/scripts/systems/outfits/events.lua (1)
1-18: Zone-change mount toggle correctly gated on leaving protection zonesThe new
onChangeZoneevent cleanly:
- Dismounts non‑staff players when entering
ZONE_PROTECTION, remembering prior mounted state, and- Only remounts when leaving
ZONE_PROTECTIONandwasMountedis set.This addresses the earlier concern about unintended remounting when moving between non‑protection tiles.
data/scripts/systems/outfits/data/mounts.lua (3)
1-211: LGTM with minor observation.The mounts registry is well-structured. Note that entries [421], [437], and [438] all share the name "Rented Horse" - this may be intentional for different rental variants, but verify if these should have distinct names for clarity.
213-224: LGTM!The function correctly iterates the mounts registry and returns a clean array of mount objects.
226-238: LGTM!The lookup function correctly handles missing entries by returning nil and provides consistent field structure.
data/scripts/systems/outfits/data/outfits.lua (3)
1-225: LGTM!The outfits registry is well-organized with clear separation of male and female outfits. The fields (name, sex, premium, unlocked, enabled) provide comprehensive metadata for outfit management.
227-241: LGTM!The function correctly filters outfits by sex and enabled status, returning appropriate fields for each outfit.
243-256: LGTM!The lookup function correctly retrieves outfit details by lookType. Note that it does not filter by the
enabledflag, which appears intentional for direct lookups.data/scripts/systems/outfits/core/windows.lua (7)
1-7: LGTM!The wearability logic correctly distinguishes between unlocked (available by default, premium-gated) and locked (must be explicitly owned) outfits.
9-30: LGTM!The function correctly provides GM privileges with full access and properly filters outfits for regular players based on wearability and addon ownership.
32-42: LGTM!The function correctly filters mounts to only those owned by the player.
44-46: LGTM!Placeholder for future familiar support. This is appropriate for the current migration scope.
48-107: LGTM!The outfit window construction is thorough, handling edge cases like empty outfits, default lookType selection, and mount state restoration via the wasMounted flag.
109-210: LGTM!The podium window logic correctly handles empty podiums by copying player outfit/mount, validates wearability, and constructs the appropriate network message. Line 200 correctly checks
playerOutfit.lookMount ~= 0for mount availability.
212-212: LGTM!Appropriate alias for backward compatibility or API clarity.
data/scripts/systems/outfits/core/outfits.lua (8)
1-3: LGTM!The function correctly stores outfit addons with a sensible default of 0.
5-9: LGTM!The bitmask operation correctly adds the addon bit to the existing addons.
11-13: LGTM!Safe getter with appropriate default value.
15-24: LGTM!The function now correctly validates both outfit ownership and addon requirements using bitmask comparison. The past review concern has been addressed.
26-28: LGTM!The function correctly checks if the addon bit(s) are set in the player's addon bitmask.
30-32: LGTM!Simple and correct storage removal.
34-38: LGTM!The bitmask operation correctly removes the specified addon bit while preserving others.
40-63: LGTM!The wearability checks are comprehensive and correct:
- GM access bypass
- Outfit existence check
- Premium requirement validation
- Unlock status verification
- Addon ownership validation using proper bitmask comparison
The addon check at line 58 correctly passes the bitmask to
hasOutfitAddon, which handles it appropriately. The past review concern appears to be outdated.data/scripts/systems/outfits/core/mounts.lua (9)
1-3: LGTM!Simple and correct mount ownership storage.
5-7: LGTM!Correct ownership verification.
9-15: LGTM!Excellent defensive logic that automatically dismounts the player if their current mount is removed.
17-35: LGTM!The mount state management functions are well-implemented with appropriate storage handling.
53-55: LGTM!Correct mounted status check.
57-73: LGTM!The random mount selection logic is correct and handles the edge case of no available mounts.
75-93: LGTM!The mount and dismount logic correctly manages outfit state and speed changes.
95-152: Verify cooldown logic intent.The cooldown check at line 96 includes
and not self:getWasMounted(), which allows cooldown bypass whenwasMountedis true. This may be intentional to permit outfit window state restoration without cooldown penalties, but the logic should be verified against intended behavior.The rest of the toggle logic correctly handles:
- Protection zone restrictions
- Mount availability checks
- Random mount selection
- Premium requirements
- Condition checks
- Mount/dismount operations
If the cooldown should apply regardless of
wasMountedstate, consider:if os.mtime() - self:getLastMountToggle() < Outfits.ToggleMountCooldown then return false end
154-157: LGTM!Good deprecation strategy with a clear warning message and temporary compatibility shim.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
data/cpplinter.luadata/migrations/37.luadata/scripts/events/player.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/events.luadata/scripts/systems/outfits/network/set_outfit.luadata/scripts/talkactions/commands/reload.luasrc/configmanager.cppsrc/configmanager.hsrc/const.hsrc/lua/modules/configmanager.cppsrc/lua/modules/globals.cppvc18/atlas.vcxprojvc18/atlas.vcxproj.filters
💤 Files with no reviewable changes (8)
- data/scripts/talkactions/commands/reload.lua
- src/configmanager.cpp
- src/lua/modules/configmanager.cpp
- vc18/atlas.vcxproj
- src/lua/modules/globals.cpp
- vc18/atlas.vcxproj.filters
- src/configmanager.h
- src/const.h
🚧 Files skipped from review as they are similar to previous changes (3)
- data/scripts/events/player.lua
- data/scripts/systems/outfits/network/set_outfit.lua
- data/scripts/systems/outfits/core/outfits.lua
🧰 Additional context used
🧬 Code graph analysis (2)
data/scripts/systems/outfits/core/windows.lua (2)
data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfits(227-241)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMounts(213-224)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMountByLookType(226-238)Game.getMounts(213-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (c-cpp)
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: format
- GitHub Check: ubuntu
- GitHub Check: docker-image
🔇 Additional comments (8)
data/scripts/systems/outfits/data/mounts.lua (1)
1-238: LGTM! Well-structured mount registry.The mount data registry and public API functions are well-implemented:
- Encapsulates the mounts table and provides controlled access via public functions
- Returns copies of mount data rather than direct references
- Clean separation of data and API
data/scripts/systems/outfits/events.lua (1)
1-18: LGTM! Zone change handling is correct.The event handler properly manages mounting state when players enter/leave protection zones, with correct checks for both zone transitions.
data/scripts/systems/outfits/data/outfits.lua (1)
1-256: LGTM! Well-organized outfit registry.The outfit data registry follows the same clean pattern as the mounts registry:
- Comprehensive outfit data organized by gender
- Public API with appropriate filtering (sex, enabled status)
- Proper encapsulation of the outfits table
data/scripts/systems/outfits/core/windows.lua (2)
48-108: LGTM! Outfit window implementation is solid.The
sendOutfitWindowfunction properly:
- Validates available outfits before opening the window
- Handles default outfit fallbacks
- Constructs the network message with all required data
- Manages mounted state correctly
110-212: LGTM! Podium window implementation is thorough.The
sendPodiumWindowfunction correctly:
- Validates podium and tile existence
- Copies player outfit/mount when podium is empty
- Validates outfit wearability before display
- Constructs detailed network message with all podium state
data/scripts/systems/outfits/core/mounts.lua (1)
23-158: Mount management implementation is solid.The mounting system properly handles:
- Storage-backed mount ownership
- Cooldown enforcement
- Protection zone restrictions
- Premium mount checks
- Mount randomization
- Speed adjustments on mount/dismount
data/cpplinter.lua (1)
70-72: LGTM! Type definitions properly updated.The type definitions correctly reflect the new API surface for the Lua-based outfit/mount system, including:
- New Game API functions for lookType-based queries
- Player methods for outfit/mount management
- Podium interaction support
Also applies to: 425-437
data/migrations/37.lua (1)
13-13: Both storage key constants are properly defined and initialized.
PlayerStorageKeys.outfitsBase(600000) andPlayerStorageKeys.mountsBase(610000) are defined indata/lib/core/storages.luaand explicitly loaded early during server initialization viadata/lib/core/core.lua. These constants are available globally when the migration executes.
| return | ||
| end | ||
|
|
||
| player:onPodiumEdit(item, outfit, direction, isVisible) |
There was a problem hiding this comment.
I don't think this will work
| end | ||
|
|
||
| player:setOutfit(outfit) | ||
| player:setRandomizeMount(randomizeMount) |
There was a problem hiding this comment.
Here you need to update mount + speed (g_game.playerChangeOutfit)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
data/cpplinter.lua (1)
70-72: Consider type consistency for mount return type.
getOutfitByLookTypereturnsOutfit_t(defined astable<string, integer>) whilegetMountByLookTypereturns a generictable. For consistency and better type safety, consider defining aMount_ttype alias similar toOutfit_tor havinggetMountByLookTypereturn a more specific type.🔎 Suggested type alias
Add near line 8:
---@alias Outfit_t table<string, integer> ---@alias MagicEffect_t table<string, integer> +---@alias Mount_t table<string, integer>Then update line 72:
----@field getMountByLookType fun(lookType: number): table +---@field getMountByLookType fun(lookType: number): Mount_t
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
data/cpplinter.luadata/scripts/systems/outfits/events.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- data/scripts/systems/outfits/events.lua
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: format
- GitHub Check: Analyze (c-cpp)
- GitHub Check: ubuntu
- GitHub Check: windows
- GitHub Check: docker-image
- GitHub Check: test
🔇 Additional comments (2)
data/cpplinter.lua (2)
425-437: LGTM! New outfit and mount API methods are well-defined.The type annotations for the new Player methods (
getCurrentOutfit,setCurrentOutfit,getDefaultOutfit,setDefaultOutfit, mount control methods, andsendPodiumWindow) are properly specified and align with the migration of outfit/mount management to Lua.
2061-2097: Config key indices verified and match correctly.The ALLOW_CHANGEOUTFIT removal has been consistently applied across both the Lua and C++ implementations. All 37 boolean config keys (indices 0–36) match exactly between
data/cpplinter.luaand theboolean_config_tenum insrc/configmanager.h, with no orphaned references remaining in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
data/cpplinter.luadata/scripts/systems/outfits/events/change_zone.luadata/scripts/systems/outfits/events/logout.lua
🧰 Additional context used
🧬 Code graph analysis (1)
data/scripts/systems/outfits/events/logout.lua (1)
src/creatureevent.cpp (1)
CreatureEvent(157-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: docker-image
- GitHub Check: format
- GitHub Check: test
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (5)
data/cpplinter.lua (3)
1878-1884: LGTM! Syntax error from previous review is now fixed.The RELOAD_TYPE constants are correctly formatted without trailing commas, as standalone variable assignments in Lua do not allow trailing commas. This properly addresses the critical syntax error flagged in the previous review.
2061-2097: LGTM! Table formatting is correct.The trailing commas in the
configKeystable are valid Lua syntax since they appear within a table constructor. This is different from the RELOAD_TYPE constants, which were standalone assignments where trailing commas are invalid.
70-72: LGTM! Type annotations are well-defined.The new type annotations for outfit and mount management methods follow the correct LuaLS annotation syntax and provide clear documentation of the API surface for:
- Look-type based accessors for outfits and mounts
- Player-specific outfit and mount state management
- Podium interaction methods
Also applies to: 425-441
data/scripts/systems/outfits/events/change_zone.lua (2)
1-6: LGTM! Event setup and filtering are correct.The event creation and early return pattern for non-players is properly implemented and follows the standard event handling conventions.
8-14: Code logic is sound; mount methods are properly implemented in Lua.The zone transition logic correctly handles mount state preservation. The methods
isMounted(),getWasMounted(), andsetWasMounted()are implemented indata/scripts/systems/outfits/core/mounts.luaand work as expected. WhileisMounted(),getWasMounted(), andsetWasMounted()are missing from cpplinter.lua type annotations (unliketoggleMountand other mount methods which are documented), the methods are properly exposed and functional, making this a minor annotation gap rather than a functional issue.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
data/scripts/systems/outfits/core/mounts.lua (1)
1-21: Ensure per-player mount state tables are cleaned up on logout.
lastMountToggleandwasMountedare module‑local tables keyed byplayer:getId(). If there is noonLogout(or equivalent) cleanup elsewhere, these entries can accumulate and/or leak state across sessions. Earlier reviews already suggested a logout cleanup event for these tables.If you haven’t added such a handler in
events/cleanup.luaor similar, consider doing so (e.g., clearing both entries for the logging‑out player ID).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
data/scripts/events/creature.luadata/scripts/lib/event_callbacks.luadata/scripts/systems/outfits/commands/add_addon.luadata/scripts/systems/outfits/commands/add_mount.luadata/scripts/systems/outfits/commands/add_outfit.luadata/scripts/systems/outfits/commands/remove_addon.luadata/scripts/systems/outfits/commands/remove_mount.luadata/scripts/systems/outfits/commands/remove_outfit.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/events/change_zone.luadata/scripts/systems/outfits/events/cleanup.luadata/scripts/systems/outfits/network/set_outfit.luasrc/events.cppsrc/events.hsrc/game.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- data/scripts/systems/outfits/events/change_zone.lua
- data/scripts/systems/outfits/core/windows.lua
- data/scripts/systems/outfits/core/outfits.lua
🧰 Additional context used
🧬 Code graph analysis (10)
data/scripts/systems/outfits/commands/remove_mount.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMount(254-261)
data/scripts/systems/outfits/commands/add_mount.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMount(254-261)
data/scripts/systems/outfits/commands/add_outfit.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(273-280)
src/game.cpp (3)
src/events.cpp (2)
onRemoved(411-433)onRemoved(411-411)src/house.cpp (2)
onRemoved(536-543)onRemoved(536-536)src/item.cpp (2)
onRemoved(216-221)onRemoved(216-216)
data/scripts/systems/outfits/commands/add_addon.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(273-280)
data/scripts/systems/outfits/network/set_outfit.lua (2)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)src/tile.h (1)
Tile(118-178)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMountByLookType(226-238)Game.getMounts(213-224)
data/scripts/systems/outfits/commands/remove_outfit.lua (3)
src/talkaction.h (1)
TalkAction(21-58)src/player.cpp (1)
Player(38-38)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(273-280)
src/events.h (1)
src/events.cpp (2)
onRemoved(411-433)onRemoved(411-411)
data/scripts/systems/outfits/commands/remove_addon.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(273-280)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: format
- GitHub Check: docker-image
- GitHub Check: ubuntu
- GitHub Check: windows
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (13)
src/events.h (1)
44-44: LGTM!The new
onRemoveddeclaration follows the established pattern of other creature event functions in this namespace and correctly matches the implementation insrc/events.cpp.src/events.cpp (3)
26-26: LGTM!The new
onRemovedhandler field follows the established pattern inCreatureHandlers.
45-45: LGTM!The meta-event loading is consistent with the other creature handlers.
411-433: LGTM!The
onRemovedimplementation correctly follows the established pattern used by other creature event handlers:
- Early return when handler is unregistered (-1)
- Proper script environment reservation with stack overflow handling
- Correct creature metatable binding using
setCreatureMetatable- Appropriate void function call with single argument
data/scripts/lib/event_callbacks.lua (1)
32-32: LGTM!The new
onCreatureRemovedcallback follows the established pattern for void creature events (similar toonCreatureHear,onCreatureChangeZone).data/scripts/events/creature.lua (1)
43-48: LGTM!The new
Creature:onRemoved()method correctly delegates to the event callback system and provides a sensible default return value oftruewhen no handler is registered.data/scripts/systems/outfits/events/cleanup.lua (1)
1-13: LGTM!The cleanup handler correctly:
- Filters to only process player creatures
- Clears mount state (
setLastMountToggle,setWasMounted) on player removal- Returns
nilfor non-players (allowing the event chain to continue) andtruefor playersdata/scripts/systems/outfits/commands/remove_mount.lua (1)
1-32: LGTM!The
/removemountcommand implementation is well-structured with proper validation:
- Parameter count validation
- Online player check
- Mount existence validation via
Game.getMount- Ownership verification before removal
- Consistent use of
mount.lookTypefor mount operationsdata/scripts/systems/outfits/commands/add_addon.lua (1)
1-21: The rest of the command logic is correct.Parameter parsing, target validation, outfit lookup by sex, addon value validation (1 or 2), duplicate addon check, and the final
addOutfitAddoncall are all properly implemented.Also applies to: 27-43
data/scripts/systems/outfits/commands/add_mount.lua (1)
1-32: LGTM!The
/addmountcommand implementation follows the established pattern correctly:
- Proper parameter validation
- Target player validation
- Mount existence check via
Game.getMount- Correctly uses
target:hasMountto verify the target doesn't already have the mount- Consistent use of
mount.lookTypefor mount operationssrc/game.cpp (1)
599-607: Newcreature::onRemovedhook placement looks correct.Calling
tfs::events::creature::onRemoved(creature);after internal removal (including summons) but before returning keeps the core removal semantics intact while exposing a clean “fully removed” hook for Lua. No issues spotted with ordering or recursion.data/scripts/systems/outfits/data/mounts.lua (1)
213-261: Mount registry and lookup helpers are consistent and safe.The mounts table plus
Game.getMounts,Game.getMountByLookType,Game.getMountByName, andGame.getMountform a clean, read‑only API over the data. Name lookups are case‑insensitive and, where names are duplicated, all variants share the same attributes, so behavior remains deterministic enough for admin tooling.data/scripts/systems/outfits/data/outfits.lua (1)
227-280: Outfit lookup API is clean and aligns with the mounts module.
Game.getOutfits,Game.getOutfitByLookType,Game.getOutfitByName, andGame.getOutfitare straightforward and consistent, with case‑insensitive name lookup and an explicitenabledflag. From a code perspective this looks solid; if you ever need deterministic ordering in the UI, you can sort the result ofgetOutfits, but it isn’t required for correctness.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data/lib/core/storages.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/events/change_zone.lua
🚧 Files skipped from review as they are similar to previous changes (2)
- data/scripts/systems/outfits/core/windows.lua
- data/scripts/systems/outfits/events/change_zone.lua
🧰 Additional context used
🧬 Code graph analysis (1)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMountByLookType(226-238)Game.getMounts(213-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: format
- GitHub Check: docker-image
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (7)
data/lib/core/storages.lua (1)
51-56: LGTM! Storage keys properly defined.The new storage keys for mount and outfit management are well-organized and follow the established pattern.
data/scripts/systems/outfits/core/mounts.lua (6)
23-38: LGTM! Mount collection management is correct.The mount ownership tracking via storage is properly implemented. The
removeMountfunction correctly dismounts the player if they're currently riding the mount being removed.
55-65: LGTM! Randomize mount flag correctly implemented.
67-82: LGTM! Mount permission checks are comprehensive.The function correctly validates admin access, mount existence, premium requirements, and ownership.
84-86: LGTM! Mounted state check is correct.
108-124: LGTM! Random mount selection logic is sound.The function correctly filters owned mounts and handles the case where no mounts are available.
185-188: LGTM! Deprecation shim is appropriate.The deprecation warning clearly communicates the change, and returning
lookTypeprovides backward compatibility during the migration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
data/scripts/systems/outfits/core/mounts.lua (3)
91-97: Add mount parameter validation.The function uses
mount.lookTypeandmount.speedwithout validating the mount parameter. If called with an invalid object, this could cause runtime errors.🔎 Suggested validation
function Player.mount(self, mount) + if not mount or type(mount) ~= "table" or not mount.lookType or not mount.speed then + return false + end local outfit = self:getDefaultOutfit() outfit.lookMount = mount.lookType self:setOutfit(outfit) self:changeSpeed(mount.speed) + return true end
1-21: Critical: Memory leak remains unfixed.The
lastMountToggleandwasMountedtables still accumulate player ID entries without cleanup on logout, causing unbounded memory growth. This critical issue was raised in previous reviews but has not been addressed.🔎 Required fix - add cleanup handler
Add at the end of this file:
local cleanMountState = CreatureEvent("cleanMountState") function cleanMountState.onLogout(player) local playerId = player:getId() lastMountToggle[playerId] = nil wasMounted[playerId] = nil return true end cleanMountState:register()
129-186: Critical: wasMounted state never updated, breaking cooldown.The
setWasMountedmethod exists but is never called in this function. This means:
getWasMounted()always returnsfalse- The cooldown check on line 130 doesn't work as intended
- The cooldown will incorrectly block all actions within the cooldown period
This critical issue was raised in previous reviews but remains unfixed.
🔎 Required fix
Update the wasMounted state when mounting/dismounting:
self:mount(currentMount) + self:setWasMounted(true) else if not self:isMounted() then return false end self:dismount() + self:setWasMounted(false) end self:setOutfit(self:getDefaultOutfit())Also consider whether the cooldown logic should check
self:getWasMounted()instead ofnot self:getWasMounted()on line 130, depending on whether you want to prevent rapid dismount-remount cycles vs. general rapid toggling.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
data/scripts/systems/outfits/commands/add_addon.luadata/scripts/systems/outfits/commands/add_outfit.luadata/scripts/systems/outfits/commands/remove_addon.luadata/scripts/systems/outfits/commands/remove_outfit.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/network/set_outfit.lua
🚧 Files skipped from review as they are similar to previous changes (2)
- data/scripts/systems/outfits/commands/add_addon.lua
- data/scripts/systems/outfits/commands/add_outfit.lua
🧰 Additional context used
🧬 Code graph analysis (4)
data/scripts/systems/outfits/commands/remove_outfit.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(273-280)
data/scripts/systems/outfits/commands/remove_addon.lua (3)
src/talkaction.h (1)
TalkAction(21-58)src/player.cpp (1)
Player(38-38)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(273-280)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMountByLookType(226-238)Game.getMounts(213-224)
data/scripts/systems/outfits/network/set_outfit.lua (2)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)src/tile.h (1)
Tile(118-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (c-cpp)
- GitHub Check: ubuntu
- GitHub Check: windows
- GitHub Check: docker-image
- GitHub Check: test
- GitHub Check: format
🔇 Additional comments (5)
data/scripts/systems/outfits/commands/remove_addon.lua (1)
1-43: LGTM! Previous ownership check issue has been resolved.The command logic is now correct. The target ownership validation at line 22 properly checks
target:hasOutfit(outfit.lookType), which addresses the previous review concern. The implementation correctly validates:
- Parameter count and format
- Target player online status
- Outfit existence for the target's gender
- Target owns the base outfit
- Addon value is valid (1 or 2)
- Target has the specific addon
All error messages are clear and the command flow is appropriate for a GM administrative action.
data/scripts/systems/outfits/commands/remove_outfit.lua (1)
1-32: LGTM! Previous undefined variable issue has been resolved.The command implementation is now correct. The error message at line 18 properly uses
split[2]instead of the previously undefinedlookTypevariable, which resolves the runtime error concern from the previous review. The implementation correctly validates:
- Parameter count and format
- Target player online status
- Outfit existence for the target's gender
- Target owns the outfit before removal
The command follows the same reliable pattern as the other outfit management commands and is properly configured for GAMEMASTER access.
data/scripts/systems/outfits/network/set_outfit.lua (1)
18-45: Validation logic looks good.The customize window flow now properly validates outfit ownership via
canWearOutfitand mount permissions viacanRideMountbefore applying changes. The conditional mount application (lines 25-31) correctly handles the case when no mount is selected.data/scripts/systems/outfits/core/mounts.lua (2)
23-38: Mount ownership operations are well implemented.The functions correctly manage mount storage, and
removeMountproperly dismounts the player if they're currently riding the mount being removed.
48-56: Good ownership validation.The addition of the
hasMountcheck (lines 50-52) properly validates mount ownership before setting the current mount, addressing previous security concerns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
data/scripts/systems/outfits/network/set_outfit.lua (2)
46-53: Store window branch is incomplete.This branch parses the message data but performs no action. The player's outfit state may be inconsistent since fields are modified at lines 10-16 but never applied or validated. Consider either implementing the store functionality or adding an explicit early return with a TODO comment.
Would you like me to help draft the implementation or open an issue to track this?
86-89: Critical:Positionobjects don't have astackposfield.This validation will always fail or error because
itemPosition.stackposisnil. The stackpos is a message-level concept, not a property of Position objects.🔎 Suggested fix
- local itemPosition = item:getPosition() - if itemPosition.stackpos ~= stackpos then - return - end + -- Verify item is at expected stack position on the tile + local thingAtPos = tile:getThing(stackpos) + if not thingAtPos or thingAtPos ~= item then + return + end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
data/scripts/systems/outfits/network/set_outfit.lua
🧰 Additional context used
🧬 Code graph analysis (1)
data/scripts/systems/outfits/network/set_outfit.lua (1)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: format
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: docker-image
- GitHub Check: test
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (5)
data/scripts/systems/outfits/network/set_outfit.lua (5)
1-6: LGTM!Handler initialization and config gating are correctly implemented.
8-16: LGTM!Outfit field parsing is clean, and validation is properly deferred to each outfitType branch before applying changes.
36-45: LGTM!Validation of outfit and mount permissions before applying is correctly implemented.
91-96: LGTM!Item type validation is thorough. The
it:getClientId() ~= clientIdcheck is technically redundant sinceitwas retrieved by thatclientId, but it doesn't cause any issues.
97-102: LGTM!Unknown type warning and handler registration are correctly implemented.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
data/scripts/systems/outfits/core/mounts.lua (1)
150-207: Critical:setWasMountedis never called - cooldown logic will not work correctly.Despite a past review comment indicating this was addressed,
setWasMountedis still never called in this function. This means:
getWasMounted()will always returnfalsefor all players- The cooldown check on Line 151 will always evaluate with
not false = true, so the cooldown will always be enforced (blocking both mounting and dismounting within the cooldown period)- The intended behavior (allowing immediate dismounts while blocking rapid remounts) cannot work
🔎 Proposed fix - update wasMounted state
self:setOutfit(self:getDefaultOutfit()) self:setLastMountToggle(os.mtime()) + self:setWasMounted(mounted) return true endAdditionally, verify the cooldown logic intent. If the goal is to:
- Allow immediate dismounting (no cooldown)
- Block rapid remounting (cooldown applies)
Then the condition should check
self:getWasMounted()(withoutnot):- if os.mtime() - self:getLastMountToggle() < Outfits.ToggleMountCooldown and not self:getWasMounted() then + if os.mtime() - self:getLastMountToggle() < Outfits.ToggleMountCooldown and self:getWasMounted() then return false endThis would enforce cooldown only when the player was previously mounted (preventing rapid remounting), but allow immediate dismounting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
data/lib/core/player.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.lua
💤 Files with no reviewable changes (1)
- data/lib/core/player.lua
🧰 Additional context used
🧬 Code graph analysis (2)
data/scripts/systems/outfits/core/outfits.lua (1)
data/scripts/systems/outfits/data/outfits.lua (2)
Game.getOutfits(227-241)Game.getOutfitByLookType(243-256)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMounts(213-224)Game.getMountByLookType(226-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: docker-image
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: test
- GitHub Check: format
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (26)
data/scripts/systems/outfits/core/outfits.lua (11)
1-7: LGTM!The function correctly checks if the outfit already exists and adds it idempotently with zero addons initially. The storage-based approach is appropriate for this Lua migration.
9-14: LGTM!The function correctly retrieves sex-appropriate outfits and adds them using the idempotent
addOutfitmethod.
16-24: LGTM!The function correctly validates outfit existence and uses bitwise operations to add addon flags. The bit-shift formula
1 << (addon - 1)properly maps addon numbers to bitmask values.
36-42: LGTM!The function correctly returns the addon bitmask or 0 if the outfit doesn't exist. This safe default prevents errors when querying non-existent outfits.
44-47: LGTM!The function correctly checks outfit existence by verifying the storage value is set (not nil and not -1).
49-56: LGTM!The function correctly uses bitwise AND to verify all requested addon bits are present. The check
(currentAddons & addons) == addonsensures the player has at least the specified addons.
58-61: LGTM!The function correctly checks if a specific addon bit is set using the formula
1 << (addon - 1). This properly handles individual addon numbers (1 or 2).
63-65: LGTM!The function correctly removes the outfit storage entry, which effectively removes the outfit and all its addons.
67-71: LGTM!The function correctly uses bitwise AND with NOT (
& ~(1 << (addon - 1))) to clear the specific addon bit while preserving others. The implementation safely handles non-existent outfits throughgetOutfitAddons.
73-96: LGTM!The function correctly validates all wearability requirements:
- Admin access bypasses all checks
- Outfit existence in the registry
- Premium status requirement
- Unlocked status or player ownership
- Addon bitmask validation using bitwise AND
The logic flow is sound and comprehensive.
26-34: Consider restrictingaddAddonToAllOutfitsto only the player's sex, consistent withaddAllOutfits.This function adds outfits and addons for both
sex = 0andsex = 1, whereas the similaraddAllOutfitsfunction (line 10) respects the player's actual sex viaGame.getOutfits(self:getSex()). This inconsistency causes the player to receive outfits they shouldn't have access to based on their sex. Align this function to useself:getSex()instead of looping through both sexes, unless the broader-than-intended grant is intentional.data/scripts/systems/outfits/core/mounts.lua (15)
23-25: LGTM!The function correctly uses storage-backed state to track mount ownership, consistent with the Lua migration approach.
27-32: LGTM!The function correctly grants all available mounts. This is appropriate for admin/GM commands to grant full mount access.
34-37: LGTM!The function correctly checks mount ownership by verifying the storage value is set.
39-45: LGTM!The function correctly handles the edge case where the removed mount is currently active, automatically dismounting the player to prevent invalid state.
47-55: LGTM!The function correctly removes all mounts and ensures the player is dismounted. The final dismount check (lines 52-54) serves as a safety net in case the current mount wasn't in the registry.
57-63: LGTM!The function correctly retrieves the current mount ID or returns nil if none is set.
65-73: LGTM!The function correctly validates mount ownership before setting it as current, preventing invalid mount IDs from being stored.
75-78: LGTM!The function correctly checks the randomize mount flag from storage.
80-85: LGTM!The function correctly manages the randomize mount flag by setting storage to 1 or removing it.
87-102: LGTM!The function correctly validates all mount riding requirements: admin access, mount existence, premium status, and ownership.
104-106: LGTM!The function correctly checks mounted state by verifying
lookMountis non-zero.
108-118: LGTM!The function correctly validates the mount parameter before applying it, preventing runtime errors from invalid mount objects. The implementation properly updates the outfit and applies the speed bonus.
120-130: LGTM!The function correctly dismounts by clearing
lookMountand reversing the speed bonus. The nil check for the mount lookup is a good safety measure.
132-148: LGTM!The helper function correctly builds a list of owned mounts and selects one randomly. The nil check for empty lists prevents errors.
209-212: LGTM!The deprecation shim correctly warns about the deprecated API while maintaining backward compatibility by returning the input unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
data/scripts/systems/outfits/core/mounts.lua (1)
151-213: Critical:wasMountedstate is never updated, breaking cooldown logic.
setWasMountedis never called intoggleMount, sogetWasMounted()always returnsfalse. Combined with the condition on line 155 (not self:getWasMounted()), this means ALL toggle attempts within the cooldown period are blocked—not just rapid re-mounts.🔎 Proposed fix
self:setOutfit(self:getDefaultOutfit()) self:setLastMountToggle(os.mtime()) + self:setWasMounted(mounted) return true endAdditionally, verify the intended cooldown behavior. If the goal is to prevent rapid re-mounting while allowing immediate dismounting, the condition on line 155 should be:
if os.mtime() - lastMountToggle < Outfits.ToggleMountCooldown and self:getWasMounted() then(checking
self:getWasMounted()instead ofnot self:getWasMounted()).
🧹 Nitpick comments (3)
data/scripts/systems/outfits/data/outfits.lua (1)
1-6: Inconsistent indentation: tabs vs spaces.Line 3 uses spaces for indentation while lines 4+ use tabs. Consider normalizing to a consistent style throughout the file.
data/scripts/systems/outfits/data/mounts.lua (1)
1-3: Inconsistent indentation: tabs vs spaces.Line 2 uses spaces for indentation while lines 3+ use tabs. Consider normalizing to a consistent style.
data/scripts/systems/outfits/core/mounts.lua (1)
47-56: Minor inefficiency: potential double dismount call.If the player is mounted on a mount being removed in the loop (line 41-43 in
removeMount),dismount()will be called there. Then lines 53-55 calldismount()again. Whiledismount()handles this gracefully (sinceisMounted()will return false), it's slightly wasteful.🔎 Suggested optimization
function Player.removeAllMounts(self) + if self:isMounted() then + self:dismount() + end + local mounts = Game.getMounts() for _, mount in ipairs(mounts) do - self:removeMount(mount.lookType) - end - - if self:isMounted() then - self:dismount() + self:removeStorageValue(PlayerStorageKeys.mountsBase + mount.lookType) end end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config.lua.distdata/scripts/network/toggle_mount.luadata/scripts/systems/outfits/config.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/events/change_zone.luadata/scripts/systems/outfits/network/set_outfit.luadata/scripts/systems/outfits/network/toggle_mount.lua
💤 Files with no reviewable changes (2)
- config.lua.dist
- data/scripts/network/toggle_mount.lua
🚧 Files skipped from review as they are similar to previous changes (4)
- data/scripts/systems/outfits/network/set_outfit.lua
- data/scripts/systems/outfits/core/outfits.lua
- data/scripts/systems/outfits/config.lua
- data/scripts/systems/outfits/events/change_zone.lua
🧰 Additional context used
🧬 Code graph analysis (2)
data/scripts/systems/outfits/network/toggle_mount.lua (1)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMounts(213-224)Game.getMountByLookType(226-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: format
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: docker-image
- GitHub Check: test
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (8)
data/scripts/systems/outfits/data/outfits.lua (2)
227-241: LGTM!The
getOutfitsfunction correctly filters by sex and enabled status, returning a clean result set with all necessary fields.
243-279: LGTM!The lookup functions provide a clean API:
getOutfitByLookTypefor direct ID lookup.getOutfitByNamefor case-insensitive name search with sex filtering.getOutfitas a convenient dispatcher.The asymmetry where
sexis ignored for numeric lookups is correct sincelookTypeis unique per outfit entry.data/scripts/systems/outfits/network/toggle_mount.lua (1)
1-12: LGTM!The packet handler is clean and appropriately delegates all business logic to
player:toggleMount(). The config check provides a simple toggle for disabling the feature.data/scripts/systems/outfits/data/mounts.lua (1)
213-238: LGTM!The
getMountsandgetMountByLookTypefunctions are well-implemented, providing clean access to the mount registry data.data/scripts/systems/outfits/core/mounts.lua (4)
23-45: LGTM!The mount add/remove/has functions correctly use storage values for persistence. The
removeMountfunction properly handles dismounting when the current mount is removed.
66-75: LGTM!The validation in
setCurrentMountcorrectly bypasses ownership checks for players with group access while enforcing ownership for regular players.
109-131: LGTM!The
mountanddismountfunctions include proper validation and correctly handle speed adjustments.
215-218: LGTM!The deprecation warning provides a helpful migration path for existing code while maintaining backward compatibility.
| do | ||
| local lastMountToggle = {} | ||
| function Player.getLastMountToggle(self) | ||
| return lastMountToggle[self:getId()] or 0 | ||
| end | ||
|
|
||
| function Player.setLastMountToggle(self, time) | ||
| lastMountToggle[self:getId()] = time | ||
| end | ||
| end | ||
|
|
||
| do | ||
| local wasMounted = {} | ||
| function Player.getWasMounted(self) | ||
| return wasMounted[self:getId()] or false | ||
| end | ||
|
|
||
| function Player.setWasMounted(self, mounted) | ||
| wasMounted[self:getId()] = mounted or nil | ||
| end | ||
| end |
There was a problem hiding this comment.
Memory leak: player state tables are never cleaned up on logout.
The lastMountToggle and wasMounted tables accumulate entries keyed by player ID but are never cleared when players log out. This causes unbounded memory growth over the server's lifetime.
🔎 Recommended fix - add cleanup event at end of file
local cleanMountState = CreatureEvent("cleanMountState")
function cleanMountState.onLogout(player)
local playerId = player:getId()
-- Access the closures through the public API to clear state
player:setLastMountToggle(nil)
player:setWasMounted(nil)
return true
end
cleanMountState:register()Note: Since the tables are scoped within do blocks, you'll need to either:
- Use the setter methods with
nilto clear entries (if they support it), or - Move the tables to module scope, or
- Add explicit clear methods.
🤖 Prompt for AI Agents
In data/scripts/systems/outfits/core/mounts.lua around lines 1 to 21 the
player-state tables lastMountToggle and wasMounted are never cleaned up, causing
a memory leak; add a CreatureEvent handler registered for logout that calls the
public setters to clear those entries (e.g. in onLogout call
player:setLastMountToggle(nil) and player:setWasMounted(nil) and return true),
then register the event so each player's entries are removed on logout.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
data/scripts/systems/outfits/network/set_outfit.lua (2)
51-58: Store window outfit type is incomplete.The store window branch (
outfitType == 1) parses mount color bytes but performs no action. The TODO comment at line 58 acknowledges this. Without proper handling, the outfit changes parsed at lines 11-16 are never validated or applied for this path.If store outfit preview is not yet implemented, consider adding an explicit early return or more detailed TODO comment explaining the intended behavior.
91-94: Critical: Position objects don't have a stackpos field.The validation
itemPosition.stackpos ~= stackposwill fail because Position objects returned byitem:getPosition()don't contain astackposfield in Lua. This check will either cause a runtime error or always evaluate incorrectly.The stackpos needs to be validated differently, either by comparing the item against what's at that stack position on the tile, or by verifying the item's identity through other means.
🔎 Suggested fix (Option 1: Validate via tile)
- local itemPosition = item:getPosition() - if itemPosition.stackpos ~= stackpos then - return - end + -- Verify item is at expected stack position + local thingAtPos = tile:getThing(stackpos) + if not thingAtPos or thingAtPos:getId() ~= item:getId() then + return + end🔎 Alternative fix (Option 2: Remove if stackpos not critical)
- local itemPosition = item:getPosition() - if itemPosition.stackpos ~= stackpos then - return - end + -- Verify item position matches + local itemPosition = item:getPosition() + if itemPosition.x ~= position.x or itemPosition.y ~= position.y or itemPosition.z ~= position.z then + return + end
🧹 Nitpick comments (2)
data/scripts/systems/outfits/network/set_outfit.lua (2)
26-31: Consider clearing mount color fields when dismounting for consistency.When
lookMount == 0(dismounting), the mount color fields retain their previous values from the current outfit. While this shouldn't cause functional issues sincelookMountis 0, it creates an inconsistent state where the outfit object has mount colors but no mount.🔎 Suggested consistency fix
- outfit.lookMount = lookMount - if lookMount ~= 0 then -- only update colors if a mount with colors is selected - outfit.lookMountHead = lookMountHead - outfit.lookMountBody = lookMountBody - outfit.lookMountLegs = lookMountLegs - outfit.lookMountFeet = lookMountFeet - end + outfit.lookMount = lookMount + outfit.lookMountHead = lookMountHead + outfit.lookMountBody = lookMountBody + outfit.lookMountLegs = lookMountLegs + outfit.lookMountFeet = lookMountFeet
96-98: Simplify redundant clientId validation.The check
it:getClientId() ~= clientIdis redundant sinceitwas just retrieved viaGame.getItemTypeByClientId(clientId). If the lookup succeeds, the clientId will always match.🔎 Suggested simplification
local it = Game.getItemTypeByClientId(clientId) - if not it or it:getClientId() ~= clientId or it:getId() ~= item:getId() then + if not it or it:getId() ~= item:getId() then return end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
data/scripts/systems/outfits/network/set_outfit.lua
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: docker-image
- GitHub Check: ubuntu
- GitHub Check: windows
- GitHub Check: format
- GitHub Check: test
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
data/scripts/systems/outfits/network/set_outfit.lua (1)
10-16: Outfit fields should be validated before modification or explicitly copied.The code modifies outfit fields (lines 11-16) before performing validation (line 36 for outfitType 0, line 73 for outfitType 2). If
player:getCurrentOutfit()returns a reference to the player's actual outfit rather than a defensive copy, modifications persist even when validation fails and the function returns early. Refactor to either:
- Validate the outfit's lookType before line 11, or
- Explicitly create a copy of the outfit before modification to guarantee unsafe changes don't persist
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/events.cpp (1)
106-140: Missing initialization ofplayerHandlers.onLogout.The
load_player_from_scripts()function loads all player event handlers but doesn't initializeplayerHandlers.onLogout. This needs to be added to properly wire the Lua event handler.🔎 Proposed fix
Add the initialization after line 139:
playerHandlers.onInventoryUpdate = scriptInterface.getMetaEvent("Player", "onInventoryUpdate"); playerHandlers.onNetworkMessage = scriptInterface.getMetaEvent("Player", "onNetworkMessage"); playerHandlers.onSpellCheck = scriptInterface.getMetaEvent("Player", "onSpellCheck"); + playerHandlers.onLogout = scriptInterface.getMetaEvent("Player", "onLogout"); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
data/scripts/events/player.luadata/scripts/lib/event_callbacks.luadata/scripts/systems/outfits/events/cleanup.luasrc/events.cppsrc/events.hsrc/player.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/events.h
🔇 Additional comments (8)
src/player.cpp (1)
2205-2214: Verify the new Lua event integration is safe and consistent.The addition of
tfs::events::player::onLogout(asPlayer())introduces the new Lua-based event system. Please verify:
Error handling: Ensure that errors or exceptions in the Lua event handler don't prevent the logout from completing. The player kick should proceed even if the event fails.
Consistency: Confirm that other logout code paths (e.g., normal logout in
Player::onRemoveCreatureat line 1146) also invoke this event if appropriate, or document whykickPlayeris the only location that needs it.Blocking operations: Verify that the Lua event handler doesn't perform blocking I/O or long-running operations that could hang the logout process.
data/scripts/lib/event_callbacks.lua (1)
62-62: LGTM!The new
onPlayerLogoutevent callback is correctly registered following the established pattern for event definitions.data/scripts/systems/outfits/events/cleanup.lua (1)
1-9: LGTM!The logout cleanup handler correctly resets per-player mount state. This ensures that session-specific mount data doesn't persist across logins.
data/scripts/events/player.lua (3)
104-104: LGTM!The updated podium window handling delegates to the new
sendPodiumWindowmethod, aligning with the Lua-based outfit system.
126-129: LGTM!The mount validation now correctly uses
mount.lookTypefor the possession check, addressing the concern from the previous review.
272-277: LGTM!The new
onLogouthandler correctly delegates to theEvent.onPlayerLogoutcallback if defined, following the established event delegation pattern.src/events.cpp (2)
103-103: LGTM!The
onLogouthandler field is correctly added to thePlayerHandlersstruct following the established pattern.
1382-1404: LGTM!The
onLogoutfunction implementation correctly follows the established pattern for event handlers: validates the handler is registered, reserves the script environment, sets the script ID, pushes the player to Lua, and invokes the callback.
ranisalt
force-pushed
the
refactor/lua-outfits-mounts
branch
from
9def96c to
c63fc4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
data/scripts/systems/outfits/commands/add_addon.luadata/scripts/systems/outfits/commands/add_mount.luadata/scripts/systems/outfits/commands/add_outfit.luadata/scripts/systems/outfits/commands/remove_addon.luadata/scripts/systems/outfits/commands/remove_mount.luadata/scripts/systems/outfits/commands/remove_outfit.luadata/scripts/systems/outfits/config.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/events/change_zone.luadata/scripts/systems/outfits/events/cleanup.luadata/scripts/systems/outfits/network/request_outfit_window.luadata/scripts/systems/outfits/network/set_outfit.luadata/scripts/systems/outfits/network/toggle_mount.luasrc/events.cppsrc/game.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- data/scripts/systems/outfits/events/cleanup.lua
- data/scripts/systems/outfits/commands/remove_addon.lua
- data/scripts/systems/outfits/network/toggle_mount.lua
- src/events.cpp
- data/scripts/systems/outfits/core/mounts.lua
- data/scripts/systems/outfits/core/outfits.lua
- data/scripts/systems/outfits/network/set_outfit.lua
- data/scripts/systems/outfits/commands/add_outfit.lua
- data/scripts/systems/outfits/commands/remove_mount.lua
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-26T22:34:15.530Z
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 89
File: .gitignore:187-187
Timestamp: 2025-12-26T22:34:15.530Z
Learning: In the atlas-kit/atlas repository, only the root `/config.lua` file should be ignored (contains server credentials), while module config files like `data/scripts/systems/beds/config.lua` should be tracked as they contain game mechanics settings.
Applied to files:
data/scripts/systems/outfits/config.lua
🧬 Code graph analysis (5)
data/scripts/systems/outfits/network/request_outfit_window.lua (1)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)
data/scripts/systems/outfits/commands/add_addon.lua (3)
src/talkaction.h (1)
TalkAction(21-58)src/player.cpp (1)
Player(38-38)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(287-293)
data/scripts/systems/outfits/commands/remove_outfit.lua (3)
src/talkaction.h (1)
TalkAction(21-58)src/player.cpp (1)
Player(38-38)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(287-293)
data/scripts/systems/outfits/commands/add_mount.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMount(266-272)
data/scripts/systems/outfits/core/windows.lua (4)
data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfits(241-255)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMounts(225-236)src/networkmessage.h (1)
NetworkMessage(15-74)src/items.h (1)
ItemType(259-418)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: format
- GitHub Check: test
- GitHub Check: docker-image
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (8)
data/scripts/systems/outfits/core/windows.lua (4)
7-13: LGTM!The access control logic correctly handles both unlocked (globally available) outfits and player-owned outfits, with proper premium checks.
15-36: LGTM!The function properly segregates staff access (granting full addons) from regular players, and correctly filters outfits by wearability.
145-148: TheItem:getPodium()method is properly defined in the Item class (src/item.h, lines 483-484) and correctly overridden in the Podium class (src/podium.h, lines 14-18). The method is actively used throughout the codebase and accessible from Lua bindings.
82-82: ThegetWasMounted()method is properly defined indata/scripts/systems/outfits/core/mounts.lua(line 25) and correctly used on line 82 ofdata/scripts/systems/outfits/core/windows.lua. The implementation stores per-player mount state in a table and returns false by default, which aligns with the documented protocol comment on line 67.data/scripts/systems/outfits/data/outfits.lua (4)
15-239: LGTM!The outfit data structure is comprehensive, well-organized, and consistent. All entries include the required fields with appropriate values.
241-255: LGTM!The function correctly filters by sex and enabled status, returning a clean list of available outfits.
257-270: LGTM!The function provides efficient O(1) lookup by lookType with proper nil handling for missing entries.
287-293: LGTM with dependency on fix above.The dispatcher pattern cleanly handles both numeric lookType and string name lookups. However, ensure the type validation issue in
getOutfitByNameis addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/protocolgame.cpp (1)
733-735: Opcode handoff for outfits (0xD2/0xD3) looks correctCommenting out the 0xD2/0xD3 cases lets those packets fall through to the
defaultbranch and be handled viaGame::parsePlayerNetworkMessage(Lua side), which is consistent with the migration away from C++ outfit handling.Optionally, you might extend these comments to note “handled by Lua via onNetworkMessage” to make the intent explicit for future readers.
data/cpplinter.lua (1)
430-441: Consider renamingmountIdparameters tolookTypefor clarity.Lines 435, 438-440 use
mountIdas the parameter name for mount-related methods, but the PR migrates the system to use lookType as the mount identifier. While the parameter name may just be historical, it creates ambiguity about what value should be passed.🔎 Suggested parameter name updates
----@field mount fun(self: Player, mountId: number) +---@field mount fun(self: Player, lookType: number) ---@field dismount fun(self: Player) ---@field sendPodiumWindow fun(self: Player, item: Item) ----@field addMount fun(self: Player, mountId: number) ----@field removeMount fun(self: Player, mountId: number) ----@field hasMount fun(self: Player, mountId: number): boolean +---@field addMount fun(self: Player, lookType: number) +---@field removeMount fun(self: Player, lookType: number) +---@field hasMount fun(self: Player, lookType: number): boolean
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
data/cpplinter.luadata/lib/compat/compat.luadata/scripts/events/player.luadata/scripts/talkactions/commands/reload.luasrc/CMakeLists.txtsrc/const.hsrc/creature.hsrc/game.cppsrc/game.hsrc/lua/CMakeLists.txtsrc/lua/api.cppsrc/lua/api.hsrc/lua/modules.cppsrc/lua/modules/game.cppsrc/lua/modules/globals.cppsrc/lua/register.hsrc/player.cppsrc/protocolgame.cppsrc/protocolgame.hsrc/signals.cppvc18/atlas.vcxprojvc18/atlas.vcxproj.filters
💤 Files with no reviewable changes (17)
- data/scripts/talkactions/commands/reload.lua
- src/signals.cpp
- src/lua/api.cpp
- data/lib/compat/compat.lua
- src/lua/modules/game.cpp
- src/const.h
- vc18/atlas.vcxproj.filters
- src/lua/register.h
- vc18/atlas.vcxproj
- src/lua/modules/globals.cpp
- src/protocolgame.h
- src/creature.h
- src/lua/CMakeLists.txt
- src/game.h
- src/player.cpp
- src/CMakeLists.txt
- src/lua/modules.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
data/scripts/events/player.lua (1)
data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMountByLookType(238-250)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: format
- GitHub Check: docker-image
- GitHub Check: test
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (8)
src/lua/api.h (1)
3-3: LGTM! The include change is correct.
Outfit_tis properly defined inenums.h(line 526), and there are no dangling references to the removedgetOutfitClassorpushOutfit(Outfit*)APIs. This refactoring appropriately simplifies header dependencies by switching from theOutfitclass to theOutfit_tstruct for C++/Lua boundary communication.src/game.cpp (1)
595-603: No behavioral change inGame::removeCreaturetailThe only modification here is whitespace before the final
return true;; function behavior and summon cleanup are unchanged and look correct.data/scripts/events/player.lua (2)
103-103: LGTM: Updated to new podium window API.The change from
sendEditPodiumtosendPodiumWindowaligns with the new Lua-based podium interaction system.
125-126: No action required. The code correctly passesmount.lookTypetohasMount(). Throughout the modern mount system (windows.lua, add_mount.lua, remove_mount.lua, and other recent files),mount.lookTypeis consistently used as the mount identifier with all mount-related Player methods. Although the parameter is namedmountIdfor legacy compatibility, the implementation stores and retrieves mounts indexed by this numeric identifier, which in the current system islookType.data/cpplinter.lua (4)
70-72: LGTM: New lookType-based mount and outfit APIs.The addition of
getOutfitByLookTypeandgetMountByLookTypeproperly replaces the old ID-based system with lookType identifiers.
425-428: LGTM: Outfit management API additions.The new outfit accessors (
getCurrentOutfit,setCurrentOutfit,getDefaultOutfit,setDefaultOutfit) are properly typed and follow consistent naming conventions.
2052-2088: ALLOW_CHANGEOUTFIT was never a ConfigManager boolean key and does not require reindexing of configKeys.ALLOW_CHANGEOUTFIT exists only as a Lua module configuration in the Outfits system (
data/scripts/systems/outfits/config.lua), not as a ConfigManager key. The C++enum boolean_config_tand the corresponding LuaconfigKeystable both contain 37 sequential entries (indices 0–36) with no gaps, and these match correctly between the C++ enum definition and the Lua table. No removal or reindexing of configKeys occurred.Likely an incorrect or invalid review comment.
1869-1875: No action required—RELOAD_TYPE constants are referenced by name throughout the codebase.All reload type references use named constants (e.g.,
RELOAD_TYPE_SCRIPTS,RELOAD_TYPE_MOVEMENTS) rather than hardcoded numeric values. The reindexing from the removal ofRELOAD_TYPE_CREATURESCRIPTSis transparent to all callers and poses no compatibility risk.
| self:addItemId(outfit.lookTypeEx) | ||
| end | ||
|
|
||
| -- mount |
There was a problem hiding this comment.
Wouldn't it be more organized to have a dedicated function for addMount? function NetworkMessage:addMount(mount)
There was a problem hiding this comment.
Absolutely!
Edit: actually no, the packets inconsistently send the mount with or without colors with lookMount is 0, I'll just inline the mount part
- Deleted the Mounts class and related methods for managing mounts. - Removed outfit management from Player class, including outfit-related methods and attributes. - Eliminated outfit loading from XML and outfit-related network message handling. - Cleaned up includes and references to outfits and mounts across various files.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 65 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- - When mounted is false: dismounts. | ||
| -- Enforces cooldown when mounting, protection-zone restriction, premium/ownership rules, and CONDITION_OUTFIT. | ||
| function Player.toggleMount(self, mounted) | ||
|
|
| return false | ||
| print("> Updating database to version 38 (revert outfits/mounts to storages)") | ||
|
|
||
| if not db.query("START TRANSACTION") then |
…its and mounts migration
…n in database update
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
data/scripts/systems/outfits/core/mounts.lua (1)
11-31: Memory leak: session state tables not cleaned on logout.The
lastMountToggleandwasMountedtables accumulate entries keyed by player ID but are never cleared when players log out. Over time, this causes unbounded memory growth.Add an
onLogouthandler to clear these entries:🔎 Recommended fix
local cleanMountState = CreatureEvent("cleanMountState") function cleanMountState.onLogout(player) player:setLastMountToggle(nil) player:setWasMounted(nil) return true end cleanMountState:register()data/scripts/systems/outfits/network/set_outfit.lua (1)
115-118: Critical: Position objects don't have astackposfield.Line 116 accesses
itemPosition.stackpos, but Position objects returned byitem:getPosition()do not have astackposattribute. This will either always fail or cause a runtime error.🔎 Suggested fix
Since
stackposwas already retrieved from the message at line 86 andtilewas retrieved at line 105, compare against the tile's thing index:- local itemPosition = item:getPosition() - if itemPosition.stackpos ~= stackpos then + if tile:getThingIndex(item) ~= stackpos then return endNote: Line 180 in
windows.luashows the correct pattern:local stackpos = tile:getThingIndex(item).
🧹 Nitpick comments (3)
data/migrations/37.lua (1)
11-94: Consider streaming approach for very large datasets.The migration loads all rows into memory before insertion. For servers with millions of outfit/mount records, this could cause memory pressure. The current batched insertion helps with query size, but the
rowstable still holds everything.For most deployments this is fine, but if you anticipate very large datasets, consider processing each source query's results in streaming batches rather than accumulating all rows first.
data/scripts/systems/outfits/data/mounts.lua (1)
1061-1072: Consider cachinggetMounts()result.
Game.getMounts()creates a new array and copies all mount data on every call. If this is called frequently (e.g., in loops or UI updates), consider caching the result:🔎 Optional optimization
+local cachedMounts = nil function Game.getMounts() + if cachedMounts then + return cachedMounts + end + local result = {} for lookType, mount in pairs(mounts) do table.insert(result, { lookType = lookType, name = mount.name, speed = mount.speed, premium = mount.premium }) end + cachedMounts = result return result enddata/scripts/systems/outfits/network/set_outfit.lua (1)
75-82: Consider clarifying the store window placeholder.The store window branch (outfitType 1) parses mount color bytes but performs no action. While the comment at line 82 hints at this, a more explicit TODO would help maintainability.
🔎 Suggested improvement
- -- open store? + -- TODO: Implement store outfit preview window
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
data/migrations/37.luadata/scripts/systems/outfits/commands/add_addon.luadata/scripts/systems/outfits/commands/add_mount.luadata/scripts/systems/outfits/commands/add_outfit.luadata/scripts/systems/outfits/commands/remove_addon.luadata/scripts/systems/outfits/commands/remove_mount.luadata/scripts/systems/outfits/commands/remove_outfit.luadata/scripts/systems/outfits/config.luadata/scripts/systems/outfits/core/mounts.luadata/scripts/systems/outfits/core/outfits.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/network/request_outfit_window.luadata/scripts/systems/outfits/network/set_outfit.luadata/scripts/systems/outfits/network/toggle_mount.lua
🚧 Files skipped from review as they are similar to previous changes (4)
- data/scripts/systems/outfits/commands/add_mount.lua
- data/scripts/systems/outfits/data/outfits.lua
- data/scripts/systems/outfits/commands/remove_outfit.lua
- data/scripts/systems/outfits/core/outfits.lua
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
📚 Learning: 2026-01-02T22:55:10.339Z
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
Applied to files:
data/scripts/systems/outfits/network/request_outfit_window.luadata/scripts/systems/outfits/core/windows.luadata/scripts/systems/outfits/network/toggle_mount.luadata/scripts/systems/outfits/config.luadata/scripts/systems/outfits/commands/add_outfit.luadata/scripts/systems/outfits/commands/remove_mount.luadata/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/network/set_outfit.luadata/scripts/systems/outfits/core/mounts.lua
📚 Learning: 2025-12-26T22:34:25.721Z
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 89
File: .gitignore:187-187
Timestamp: 2025-12-26T22:34:25.721Z
Learning: In the atlas-kit/atlas repository, only the root `/config.lua` file should be ignored (contains server credentials), while module config files like `data/scripts/systems/beds/config.lua` should be tracked as they contain game mechanics settings.
Applied to files:
data/scripts/systems/outfits/config.lua
🧬 Code graph analysis (10)
data/scripts/systems/outfits/network/request_outfit_window.lua (1)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)
data/migrations/37.lua (2)
src/database.h (1)
DBTransaction(183-224)src/database.cpp (1)
DBInsert(227-227)
data/scripts/systems/outfits/commands/remove_addon.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(1607-1613)
data/scripts/systems/outfits/core/windows.lua (4)
data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfits(1561-1575)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMounts(1061-1072)src/networkmessage.h (1)
NetworkMessage(15-74)src/items.h (1)
ItemType(259-418)
data/scripts/systems/outfits/network/toggle_mount.lua (1)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)
data/scripts/systems/outfits/commands/add_outfit.lua (2)
src/talkaction.h (1)
TalkAction(21-58)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(1607-1613)
data/scripts/systems/outfits/commands/remove_mount.lua (3)
src/talkaction.h (1)
TalkAction(21-58)src/player.cpp (1)
Player(36-36)data/scripts/systems/outfits/data/mounts.lua (1)
Game.getMount(1102-1108)
data/scripts/systems/outfits/network/set_outfit.lua (2)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)src/tile.h (1)
Tile(118-178)
data/scripts/systems/outfits/commands/add_addon.lua (3)
src/talkaction.h (1)
TalkAction(21-58)src/player.cpp (1)
Player(36-36)data/scripts/systems/outfits/data/outfits.lua (1)
Game.getOutfit(1607-1613)
data/scripts/systems/outfits/core/mounts.lua (1)
data/scripts/systems/outfits/data/mounts.lua (2)
Game.getMounts(1061-1072)Game.getMountByLookType(1074-1086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: format
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: docker-image
- GitHub Check: ubuntu
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (16)
data/migrations/37.lua (1)
6-9: LGTM on transaction handling.The
DBTransactionRAII pattern correctly handles rollback on early returns. The destructor will callrollback()ifcommit()wasn't called, ensuring data integrity on any failure path.Also applies to: 90-92, 96-108
data/scripts/systems/outfits/data/mounts.lua (1)
1074-1108: LGTM on mount lookup API.The API provides clean lookup functions with consistent nil-on-not-found semantics. Returning copies of mount data prevents accidental mutation of the source table.
data/scripts/systems/outfits/core/mounts.lua (3)
174-180: Verify: Cooldown only applies after forced dismount.The current logic only enforces the cooldown when
wasMountedistrue(set bychange_zone.luaafter forced dismount in protection zones). Normal mounting attempts bypass the cooldown entirely.If the intent is:
- No cooldown for normal mounting: ✅ current behavior
- Cooldown only for rapid remounting after being forced off in PZ: ✅ current behavior
Just confirming this matches the design intent, as it differs from typical mount cooldown implementations.
229-232: Verify:setOutfit(getDefaultOutfit())after mounting may be redundant or incorrect.Line 230 calls
self:setOutfit(self:getDefaultOutfit())after both mounting and dismounting. Themount()function already callsself:setOutfit(outfit)withlookMountset.If
getDefaultOutfit()returns the current outfit state (including the just-setlookMount), this is redundant. If it returns a stored "base" outfit without the mount, this would clear the mount just applied.Please verify that this call is intentional and doesn't overwrite the mount state.
235-239: LGTM on deprecation warning.Clean deprecation approach - returns the input unchanged while warning callers to update their code.
data/scripts/systems/outfits/network/request_outfit_window.lua (1)
1-14: LGTM!Clean packet handler with appropriate config check. The silent return when disabled is acceptable for this use case.
data/scripts/systems/outfits/network/toggle_mount.lua (1)
1-17: LGTM!Clean packet handler that properly delegates to
Player.toggleMount()for all validation and business logic.data/scripts/systems/outfits/commands/remove_mount.lua (1)
1-46: LGTM!Well-structured command with thorough validation and clear error messages. The use of
Game.getMount()provides flexibility for both lookType and name inputs.data/scripts/systems/outfits/commands/remove_addon.lua (1)
1-58: LGTM!Well-structured command with complete validation chain. The outfit lookup correctly uses
target:getSex()for sex-specific outfit resolution.data/scripts/systems/outfits/commands/add_outfit.lua (1)
1-46: LGTM! Past issues have been addressed.The command implementation is now correct. The target player ownership check (line 24) and the outfit addition (line 29) both properly reference the target, and the code no longer uses undefined variables. The validation flow, error handling, and success messaging are all appropriate.
data/scripts/systems/outfits/config.lua (1)
1-24: LGTM! Clear and well-documented configuration.The configuration module provides sensible defaults for the outfit/mount system. The inline documentation clearly maps network opcodes to their handlers, and the flag meanings are explicit. The 3-second mount toggle cooldown is reasonable for preventing spam.
data/scripts/systems/outfits/commands/add_addon.lua (1)
1-58: LGTM! Command implementation is correct.The addon command properly validates all requirements: parameter count, target online status, outfit existence, base outfit ownership (line 24 correctly checks the target), addon value range, and existing addon ownership. The error handling and success messaging are appropriate.
data/scripts/systems/outfits/network/set_outfit.lua (1)
40-74: LGTM! Customize outfit flow is now correct.The mount handling properly addresses previous concerns:
- Mount colors are only assigned when a mount is selected (lines 48-53)
- Mount state is cleared when dismounting (lines 69-71)
- Outfit and mount validations are in place (lines 58, 62-66)
The flow correctly handles both mounting and dismounting scenarios.
data/scripts/systems/outfits/core/windows.lua (3)
6-68: LGTM! Helper functions implement correct logic.The helper functions properly handle:
- Outfit unlocking logic based on premium and ownership status
- Staff access with GM outfits and full addon access
- Sex-specific outfit filtering
- Mount ownership checks
83-141: LGTM! Outfit window correctly implements packet structure.The implementation properly handles:
- Manual mount serialization (lines 106-110) after
addOutfit, consistent with the learning that mount data is not automatically serialized- Preserved mount checkbox state using
getWasMounted()for PZ scenarios (line 96)- Proper packet structure with all required fields
Based on learnings, the manual mount serialization is intentional design.
158-260: LGTM! Podium window implementation is correct.The podium window properly handles:
- All necessary validations (podium, tile, item type, outfit availability)
- Correct stackpos retrieval using
tile:getThingIndex(item)(line 180)- Smart outfit/mount copying from player when podium is empty
- Manual mount serialization (lines 219-223) consistent with the learning
- Correct mount checkbox logic (line 249) checking
playerOutfit.lookMount
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
data/scripts/systems/outfits/network/set_outfit.lua (1)
115-118: Fix stackpos validation logic.Position objects don't have a
stackposfield. This validation will either always fail or error at runtime. Thestackposis a separate concept from the position coordinates.🔎 Suggested fix
Verify the item's stack position by comparing against the tile's thing at that index:
local itemPosition = item:getPosition() - if itemPosition.stackpos ~= stackpos then - return - end + -- Verify item is at expected stack position + local thingAtPos = tile:getThing(stackpos) + if not thingAtPos or thingAtPos ~= item then + return + enddata/scripts/systems/outfits/data/outfits.lua (1)
1576-1589: Add type validation for thenameparameter.The function calls
name:lower()at line 1578 without validating thatnameis a string, which will raise a runtime error ifnameisnilor a non-string type.🔎 Proposed fix
function Game.getOutfitByName(name, sex) + if type(name) ~= "string" then + return nil + end for lookType, outfit in pairs(outfits) do if outfit.name:lower() == name:lower() and outfit.sex == sex then return { lookType = lookType, name = outfit.name, sex = outfit.sex, premium = outfit.premium, unlocked = outfit.unlocked } end end return nil end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
data/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/network/set_outfit.lua
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
📚 Learning: 2026-01-02T22:55:10.339Z
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
Applied to files:
data/scripts/systems/outfits/data/mounts.luadata/scripts/systems/outfits/network/set_outfit.luadata/scripts/systems/outfits/data/outfits.lua
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: docker-image
- GitHub Check: ubuntu
- GitHub Check: windows
- GitHub Check: format
- GitHub Check: test
- GitHub Check: Analyze (c-cpp)
ranisalt
force-pushed
the
refactor/lua-outfits-mounts
branch
from
7469117 to
dcfa9d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
data/scripts/systems/outfits/network/set_outfit.lua (1)
47-53: Consider clearing mount colors when no mount is selected.When
lookMount == 0, the code doesn't update the mount color fields (lines 49-52), leaving them with values fromgetCurrentOutfit(). While this is harmless (the client won't render the colors without a mount), explicitly clearing them would make the outfit state cleaner.🔎 Optional refactor
outfit.lookMount = lookMount -if lookMount ~= 0 then -- only update colors if a mount with colors is selected +if lookMount ~= 0 then outfit.lookMountHead = lookMountHead outfit.lookMountBody = lookMountBody outfit.lookMountLegs = lookMountLegs outfit.lookMountFeet = lookMountFeet +else + outfit.lookMountHead = 0 + outfit.lookMountBody = 0 + outfit.lookMountLegs = 0 + outfit.lookMountFeet = 0 enddata/scripts/systems/outfits/data/outfits.lua (1)
1603-1624: Efficient caching with potential mutability concern.The pre-computed cache for enabled outfits is a good performance optimization. However,
Game.getOutfits(sex)returns a direct reference to the cached table (line 1622), allowing callers to potentially mutate it.If cache immutability is desired, consider one of these approaches:
Option 1: Return a shallow copy
function Game.getOutfits(sex) local result = {} for i, outfit in ipairs(cachedOutfits[sex]) do result[i] = outfit end return result endOption 2: Use a metatable to make it read-only (more complex but zero-copy)
Otherwise, document that callers must not modify the returned table.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data/lib/core/storages.luadata/migrations/37.luadata/scripts/systems/outfits/data/outfits.luadata/scripts/systems/outfits/network/set_outfit.lua
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
📚 Learning: 2026-01-02T22:55:10.339Z
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
Applied to files:
data/lib/core/storages.luadata/scripts/systems/outfits/network/set_outfit.luadata/scripts/systems/outfits/data/outfits.lua
🧬 Code graph analysis (2)
data/migrations/37.lua (2)
src/database.h (1)
DBTransaction(183-224)src/database.cpp (1)
DBInsert(227-227)
data/scripts/systems/outfits/network/set_outfit.lua (2)
data/lib/core/packet_handler.lua (1)
PacketHandler(23-29)src/tile.h (1)
Tile(118-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: format
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: test
- GitHub Check: docker-image
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (10)
data/lib/core/storages.lua (1)
51-55: LGTM! Clean storage key definitions.The new storage keys are well-organized, properly documented, and don't conflict with existing ranges. The values align with the migration script and provide clear separation between current mount state (60000-60001) and per-outfit/mount storage bases (600000+, 610000+).
data/migrations/37.lua (3)
1-6: Clear constant definitions with helpful documentation.The local constants correctly match the storage keys defined in
data/lib/core/storages.lua, and the comment provides clear guidance for future maintainers.
7-74: Well-structured data collection with proper resource management.The coroutine approach efficiently gathers outfit, mount, and player mount state data from legacy tables. Each block properly:
- Queries the source data
- Transforms it to the new storage format
- Frees result sets after iteration
The WHERE clause on line 48 optimizes by filtering rows, while the
if > 0checks at lines 55 and 63 ensure only meaningful values are migrated.
84-91: Code location does not match the review comment.The code snippet shown in the review does not correspond to lines 84-91 of
data/migrations/37.lua. Line 84-91 contain onlyreturn tx.commit(). The actual migration usesdb.storeQuery()with proper result checks rather than a coroutine pattern. Additionally,DBInsert::execute()safely handles empty inserts by returningtruewhen no rows have been added, so the hypothetical concern about fresh databases is not a problem.Likely an incorrect or invalid review comment.
data/scripts/systems/outfits/network/set_outfit.lua (2)
1-39: Well-documented packet structure and clean parsing logic.The extensive comments at lines 1-24 provide clear documentation of the packet format for all three outfit types. The handler correctly checks the feature flag and parses the common outfit fields before branching.
83-126: Comprehensive podium validation with proper security checks.The podium edit path includes thorough validation:
- Outfit ownership and mount capability (lines 97-103)
- Tile and item existence (lines 105-113)
- Stack position verification to prevent client-server desync (lines 115-119)
- Item type consistency checks (lines 121-124)
This layered approach effectively prevents malicious clients from editing arbitrary items or using unauthorized outfits/mounts.
data/scripts/systems/outfits/data/outfits.lua (4)
15-1559: Comprehensive outfit data with consistent structure.The outfit registry contains 100+ entries spanning both male and female variants, each with properly structured metadata. The lookType keys are well-distributed and the field structure (name, sex, premium, unlocked, enabled) is consistent throughout.
1561-1574: Clean lookup function with proper nil handling.The function correctly handles missing lookTypes and returns a defensive copy of the outfit data rather than the internal table reference.
1576-1593: Robust name-based lookup with proper validation.The function includes type validation for the
nameparameter (addressing previous review feedback) and performs case-insensitive matching, which is user-friendly for name-based queries.
1595-1601: Convenient dispatcher with implicit type handling.The function provides a flexible interface that accepts either lookType (number) or name (string), delegating to the appropriate lookup function. Type mismatches are handled gracefully by the underlying functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
data/migrations/37.lua (1)
18-18: Consider using unique variable names for clarity.The variable
resultIdis reused across three separatedo...endblocks (lines 18, 33, 48). While this is technically safe due to the separate scopes and properresult.free()calls, using unique names likeoutfitResultId,mountResultId, andplayerResultIdwould improve code clarity and reduce cognitive load during maintenance.🔎 Suggested refactor
do - local resultId = db.storeQuery("SELECT `player_id`, `outfit_id`, `addons` FROM `player_outfits`") - if resultId then + local outfitResultId = db.storeQuery("SELECT `player_id`, `outfit_id`, `addons` FROM `player_outfits`") + if outfitResultId then repeat - local playerId = result.getNumber(resultId, "player_id") - local outfitId = result.getNumber(resultId, "outfit_id") - local addons = result.getNumber(resultId, "addons") + local playerId = result.getNumber(outfitResultId, "player_id") + local outfitId = result.getNumber(outfitResultId, "outfit_id") + local addons = result.getNumber(outfitResultId, "addons") local storageKey = OUTFITS_BASE + outfitId query:addRow(string.format("%d, %d, %d", playerId, storageKey, addons)) - until not result.next(resultId) - result.free(resultId) + until not result.next(outfitResultId) + result.free(outfitResultId) end endApply similar changes to the other two blocks.
Also applies to: 33-33, 48-49
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
data/migrations/37.lua
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: docker-image
- GitHub Check: ubuntu
- GitHub Check: windows
- GitHub Check: test
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (3)
data/migrations/37.lua (3)
17-30: Migration logic correctly preserves all outfit and mount data.The three data migration blocks properly:
- Migrate outfit unlocks with addon state from
player_outfits(lines 17-30)- Migrate mount unlocks from
player_mounts(lines 32-44)- Migrate current mount selection and randomization preference from
playerscolumns (lines 46-66)Storage key calculations correctly map IDs to their respective ranges (600000+ for outfits, 610000+ for mounts), and the NULL/zero checks appropriately filter out invalid values.
Also applies to: 32-44, 46-66
72-82: Cleanup operations are safely ordered and idempotent.The DROP operations correctly:
- Execute after data migration completes
- Use
IF EXISTSfor idempotency- Check for errors on each operation
- Drop tables before dropping columns (avoiding potential FK issues)
68-70: This concern is not applicable; DBInsert::execute() already handles empty data gracefully.The
execute()method insrc/database.cpp(lines 260–271) checksif (values.empty())and returnstrueimmediately, skipping execution of any incomplete INSERT statement. No rows being added to the query is a safe, expected scenario.
…n onUpdateDatabase
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
data/migrations/37.lua (1)
48-49: Consider filtering zero values in the WHERE clause.The query selects rows where
currentmount IS NOT NULL OR randomizemount IS NOT NULL, but only inserts rows where values are> 0(lines 56, 60). If the schema allows0as a default value, this fetches unnecessary rows.For better efficiency, consider:
🔎 Suggested optimization
local resultId = db.storeQuery( - "SELECT `id`, `currentmount`, `randomizemount` FROM `players` WHERE `currentmount` IS NOT NULL OR `randomizemount` IS NOT NULL") + "SELECT `id`, `currentmount`, `randomizemount` FROM `players` WHERE `currentmount` > 0 OR `randomizemount` > 0")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
data/migrations/37.lua
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/lib/core/network_message.lua:18-29
Timestamp: 2026-01-02T22:55:10.339Z
Learning: In data/lib/core/network_message.lua, the NetworkMessage:addOutfit(outfit) method intentionally does NOT serialize mount data (lookMount, lookMountHead, etc.). Mount data should be serialized manually where needed, as this separation was chosen to avoid confusion and maintain clear separation of concerns.
Learnt from: ranisalt
Repo: atlas-kit/atlas PR: 114
File: data/scripts/systems/outfits/network/set_outfit.lua:48-48
Timestamp: 2026-01-03T16:01:49.114Z
Learning: In data/scripts/systems/outfits/network/set_outfit.lua, mount colors (lookMountHead, lookMountBody, lookMountLegs, lookMountFeet) are only updated when lookMount ~= 0. This is intentional: when no mount is selected, the code preserves existing color values so players don't have to re-pick colors when they later select a mount again.
🧬 Code graph analysis (1)
data/migrations/37.lua (2)
src/database.h (1)
DBTransaction(183-224)src/database.cpp (1)
DBInsert(227-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: format
- GitHub Check: test
- GitHub Check: docker-image
- GitHub Check: ubuntu
- GitHub Check: windows
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
data/migrations/37.lua (1)
15-71: No changes required.The DBInsert class is designed to safely handle empty inserts. The
execute()method returnstrueimmediately if no rows were added viaaddRow(), without attempting to execute invalid SQL. Therefore, the migration will succeed on fresh databases or test environments where the source tables are empty.
…d randomized mounts
|
Blocked because podium functionality is super tightly coupled with the engine and many parts can't be migrated to Lua for now. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
data/cpplinter.lua (1)
72-72: Consider defining aMount_ttype alias forgetMountByLookType's return.
getOutfitByLookTypereturns the typedOutfit_t, butgetMountByLookTypereturns the generictable. If the mount table has a stable structure, a---@alias Mount_t table<string, integer>(parallel toOutfit_ton line 7) would give callers IDE-level field completion and type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/cpplinter.lua` at line 72, The comment notes getMountByLookType is annotated to return a generic table while getOutfitByLookType returns a typed Outfit_t; add a Mount_t alias and use it in the function annotation. Define a new alias similar to Outfit_t (e.g.,---@alias Mount_t table<string, integer>) near the top where Outfit_t is declared, then change the getMountByLookType annotation to---@field getMountByLookType fun(lookType: number): Mount_t so callers get IDE completion and type safety for mount fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@data/cpplinter.lua`:
- Line 72: The comment notes getMountByLookType is annotated to return a generic
table while getOutfitByLookType returns a typed Outfit_t; add a Mount_t alias
and use it in the function annotation. Define a new alias similar to Outfit_t
(e.g.,---@alias Mount_t table<string, integer>) near the top where Outfit_t is
declared, then change the getMountByLookType annotation to---@field
getMountByLookType fun(lookType: number): Mount_t so callers get IDE completion
and type safety for mount fields.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores