Conversation
|
|
Where is the credits to the real developers? 
|
|
Qual server tá usando para validar? |
Canary and crystal |
📝 WalkthroughWalkthroughAdds a complete Weapon Proficiency feature: new top-bar widget, full proficiency UI and data layers, inventory hooks, client protocol send/parse and Lua bindings, ThingType/protobuf proficiency flags, and C++/Lua plumbing to request/apply proficiencies and update the UI. Changes
Sequence DiagramssequenceDiagram
participant UI as Proficiency UI
participant Lua as modules.game_proficiency
participant Game as Game (C++)
participant Protocol as ProtocolGame
participant Server as Server
UI->>Lua: User clicks "Apply" with selected perks
Lua->>Game: g_game.sendWeaponProficiencyApplyLua(itemId, levels, perkPositions)
Game->>Protocol: sendWeaponProficiencyApply(itemId, pairedPerks)
Protocol->>Server: ClientWeaponProficiency (action=3, itemId, perks[])
Server-->>Protocol: WeaponProficiencyInfo
Protocol->>Lua: onWeaponProficiency(itemId, experience, perks, marketCategory)
Lua->>UI: refresh UI and top-bar progress
sequenceDiagram
participant Inventory as Inventory Module
participant Lua as modules.game_proficiency
participant Game as Game (C++)
participant Protocol as ProtocolGame
participant StatsBar as StatsBar Widget
Inventory->>Inventory: Equip weapon in left slot
Inventory->>Lua: call updateTopBarProficiency()
Lua->>Game: g_game.sendWeaponProficiencyAction(actionType, itemId)
Game->>Protocol: sendWeaponProficiencyAction(actionType, itemId)
Protocol->>Server: ClientWeaponProficiency (action=0/2)
Server-->>Protocol: WeaponProficiencyExperience
Protocol->>Lua: onWeaponProficiencyExperience(itemId, experience, hasUnusedPerk)
Lua->>StatsBar: updateTopBarProficiency() -> update progress display
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@modules/game_proficiency/const.lua`:
- Line 38: Rename the misspelled constant AUGMENT_CHAIN_LENGHT to
AUGMENT_CHAIN_LENGTH in modules/game_proficiency/const.lua and update every
usage/reference in the codebase to the new symbol; ensure the value (5) stays
the same and run tests or search to catch any remaining references to
AUGMENT_CHAIN_LENGHT so there are no undefined constant errors.
In `@modules/game_proficiency/proficiency.lua`:
- Around line 1009-1011: The variable cacheId is declared twice in the same
function (first at the earlier assignment and again around the later block),
causing redundancy and potential confusion; remove the second local declaration
and reuse the existing cacheId variable by replacing the later "local cacheId =
marketItem.originalId or itemId" with a simple assignment "cacheId =
marketItem.originalId or itemId" (or eliminate the later assignment entirely if
not needed), keeping references like self.selectedItemId and any subsequent
proficiency lookups using the original cacheId.
In `@src/client/protocolgameparse.cpp`:
- Around line 170-179: Wrap the Proto::GameServerWeaponProficiencyExperience
handler in a client-version check using g_game.getClientVersion() to avoid older
clients parsing the new payload; specifically, inside the case for
Proto::GameServerWeaponProficiencyExperience add: if (g_game.getClientVersion()
>= <SUMMER_UPDATE_2025_VERSION>) { /* existing reads: msg->getU16(),
msg->getU32(), msg->getU8() and
g_lua.callGlobalField("g_game","onWeaponProficiencyExperience",...) */ } so that
on older clients the message is ignored (no msg reads) and stream parsing stays
in sync—replace <SUMMER_UPDATE_2025_VERSION> with the actual client version
number for the Summer Update 2025 release.
In `@src/framework/luaengine/luavaluecasts.h`:
- Around line 605-623: The rawGeti calls pass arguments in the wrong order and
wrongly adjust negative stack indexes; swap the arguments so the first parameter
is the table stack-position and the second is the element key, and stop
subtracting 1 from negative indexes. Replace the two calls g_lua.rawGeti(index <
0 ? index - 1 : index, 1) and g_lua.rawGeti(index < 0 ? index - 1 : index, 2)
with g_lua.rawGeti(index, 1) and g_lua.rawGeti(index, 2) respectively (leaving
the surrounding luavalue_cast(-1, firstValue)/luavalue_cast(-1, secondValue),
pop(), and assignments to pair.first/pair.second unchanged).
🧹 Nitpick comments (13)
src/client/const.h (2)
471-471: Consider using English for code comments.The comment
// game proficiency apenas para 1510+is in Portuguese. For consistency with the rest of the codebase and to help international contributors, consider translating to English:// game proficiency for 1510+ only.Suggested change
- GameProficiency = 19, // game proficiency apenas para 1510+ + GameProficiency = 19, // game proficiency for 1510+ only
870-875: Minor indentation inconsistency.Line 870 has inconsistent leading whitespace (3 spaces instead of 4). The addition of
IMBUEMENT_WINDOW_SCROLL = 2extends the enum correctly.Suggested fix
- enum Imbuement_Window_t : uint8_t + enum Imbuement_Window_t : uint8_tmodules/game_inventory/inventory.lua (1)
155-164: Consider documenting the magic number0insendWeaponProficiencyAction.The first parameter
0passed tosendWeaponProficiencyAction(0, item:getId())is unclear. Consider using a named constant or adding a comment to clarify its meaning (e.g., action type, request mode).💡 Suggested improvement
-- Request proficiency data for new weapon if item and g_game.sendWeaponProficiencyAction then - g_game.sendWeaponProficiencyAction(0, item:getId()) + -- Action type 0 = request proficiency data + g_game.sendWeaponProficiencyAction(0, item:getId()) endOr define a constant in the proficiency module's
const.luafile.modules/game_proficiency/proficiency.otmod (1)
1-13: Website URL points to a different repository.The
websitefield points tohttps://github.com/mehah/otclient, but this PR is targetingopentibiabr/otclient. Consider updating the URL to match the target repository for consistency.💡 Suggested fix
Module name: game_proficiency description: Weapon Proficiency System author: OTClient Team - website: https://github.com/mehah/otclient + website: https://github.com/opentibiabr/otclient sandboxed: truesrc/client/protocolgameparse.cpp (1)
6342-6370: HardenparseWeaponProficiencyInfo: reserve vector, use correct index types, and confirm marketData sentinel logic.
perksCountisuint8_t; usingint iis minor but avoidable.perks.reserve(perksCount)avoids reallocations.!marketData.name.empty()as the only “market data exists” check could misclassify items if category is set but name is empty (depends on yourMarketDatainvariants).Proposed tweak
const uint8_t perksCount = msg->getU8(); std::vector<std::pair<uint8_t, uint8_t>> perks; - for (int i = 0; i < perksCount; ++i) { + perks.reserve(perksCount); + for (size_t i = 0; i < perksCount; ++i) { const uint8_t level = msg->getU8(); const uint8_t perkPosition = msg->getU8(); perks.emplace_back(level, perkPosition); } @@ if (itemType) { const auto& marketData = itemType->getMarketData(); - if (!marketData.name.empty()) { + // Verify the correct "market data is present" condition for your data model: + if (!marketData.name.empty()) { marketCategory = marketData.category; } }modules/game_proficiency/proficiency.otui (2)
356-372: Consider usingtr()for translatable dropdown options.The ComboBox options are added with hardcoded English strings. For i18n consistency with other UI elements in this file (e.g., line 226 uses
tr('Weapon Proficiency')), consider wrapping these options:self:addOption(tr("Weapons: Axes"), nil, true) -- etc.
470-481: Hardcoded warning text should usetr()for localization.Line 479 contains a user-facing warning message that should be translatable:
- text: Perks are inactive because you don't meet the requirements to use this weapon. + !text: tr("Perks are inactive because you don't meet the requirements to use this weapon.")modules/game_proficiency/proficiency_data.lua (4)
54-89: Consider adding error logging for failed JSON parsing.The pcall correctly catches parsing errors, but the error is silently discarded. Adding a log message would help debugging:
if not status then + g_logger.error("Failed to parse proficiencies.json: " .. tostring(result)) return false end
165-202: Partial match search may have performance implications with large proficiency datasets.This loop iterates through all entries in
nameIndexfor each item lookup, checking two suffix patterns. For a large item catalog, this could become slow.Consider:
- Building a reverse index during
loadProficiencyJsonthat maps item name suffixes to proficiency IDs- Caching resolved proficiency IDs per item after first lookup
However, if the proficiency count is small (< 100 entries), this is likely acceptable.
323-327: Redundant ternary pattern can be simplified.function ProficiencyData:getContentById(id) local content = self.content[id] - return content and content or nil + return content endThe expression
content and content or nilis equivalent to justcontent.
486-493: Last resort fallback returns arbitrary proficiency ID.The iteration
for id, _ in pairs(self.content)returns an arbitrary entry since table iteration order in Lua is undefined. This could lead to inconsistent behavior.Consider using a deterministic fallback (e.g., always return 6 or the lowest ID):
-- Last resort: return first available proficiency ID -for id, _ in pairs(self.content) do - return id -end +-- Always return consistent fallback return 6modules/game_proficiency/proficiency.lua (2)
626-644: Indentation inconsistency suggests code formatting issue.Lines 627-630 and 636-643 have inconsistent indentation that breaks the visual structure:
searchText.onTextChange = function(widget, text) WeaponProficiency.searchFilter = text WeaponProficiency:refreshItemList() end -- This should be indented properly endThis appears to work but makes the code harder to maintain. Consider reformatting for consistency.
120-154: Retry logic with scheduleEvent could lead to infinite recursion if StatsBar never loads.The
initTopBarProficiencyfunction schedules itself recursively if conditions aren't met. While there's no explicit retry limit, the function will eventually succeed when the UI loads or fail silently if the game ends.Consider adding a retry counter similar to
autoSelectItem(line 494) to prevent potential issues:local initRetries = 0 local maxRetries = 10 function initTopBarProficiency() if initRetries >= maxRetries then g_logger.warning("Failed to initialize proficiency top bar after max retries") return end initRetries = initRetries + 1 -- ... rest of logic end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (113)
data/images/game/proficiency/augment-icons.pngis excluded by!**/*.pngdata/images/game/proficiency/backdrop_weaponmastery_highlight.pngis excluded by!**/*.pngdata/images/game/proficiency/big-icon-lock-grey.pngis excluded by!**/*.pngdata/images/game/proficiency/bonus-select-bg-progress.pngis excluded by!**/*.pngdata/images/game/proficiency/bonus-select-bg.pngis excluded by!**/*.pngdata/images/game/proficiency/border-weaponmasterytreeicons-active.pngis excluded by!**/*.pngdata/images/game/proficiency/border-weaponmasterytreeicons-inactive.pngis excluded by!**/*.pngdata/images/game/proficiency/full-bonus-select-bg.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-lock-grey.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-0.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-1-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-1-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-1.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-2-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-2-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-2.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-3-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-3-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-3.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-4-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-4-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-4.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-5-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-5-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-5.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-6-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-6-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-6.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-7-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-7-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-masterylevel-7.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-star-dark.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-star-tiny-gold.pngis excluded by!**/*.pngdata/images/game/proficiency/icon-star-tiny-silver.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-0.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-1.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-2.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-3.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-4.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-5.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-6.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-7.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-8.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-9-off.pngis excluded by!**/*.pngdata/images/game/proficiency/icons-9.pngis excluded by!**/*.pngdata/images/game/proficiency/proficiency-progress.pngis excluded by!**/*.pngdata/images/game/proficiency/progress-bg.pngis excluded by!**/*.pngdata/images/game/proficiency/star-progress-bg.pngis excluded by!**/*.pngdata/images/game/proficiency/star-progress.pngis excluded by!**/*.pngdata/images/game/topbar/backgroundTop.pngis excluded by!**/*.pngdata/images/game/topbar/boost-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/boost.pngis excluded by!**/*.pngdata/images/game/topbar/bottom-topbar.pngis excluded by!**/*.pngdata/images/game/topbar/compact/compact-health-cond-mana.pngis excluded by!**/*.pngdata/images/game/topbar/compact/large-container-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/compact/large-container.pngis excluded by!**/*.pngdata/images/game/topbar/compact/small-container-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/compact/small-container.pngis excluded by!**/*.pngdata/images/game/topbar/condition-topbar.pngis excluded by!**/*.pngdata/images/game/topbar/healthMana-topbar.pngis excluded by!**/*.pngdata/images/game/topbar/highlight-proficiency-button.pngis excluded by!**/*.pngdata/images/game/topbar/highlight-proficiency-large-button.pngis excluded by!**/*.pngdata/images/game/topbar/icon-combopoint-empty.pngis excluded by!**/*.pngdata/images/game/topbar/icon-combopoint-filled.pngis excluded by!**/*.pngdata/images/game/topbar/icon-proficiencytree-off.pngis excluded by!**/*.pngdata/images/game/topbar/icon-proficiencytree-on.pngis excluded by!**/*.pngdata/images/game/topbar/icon-serene-off.pngis excluded by!**/*.pngdata/images/game/topbar/icon-serene-on.pngis excluded by!**/*.pngdata/images/game/topbar/icons.pngis excluded by!**/*.pngdata/images/game/topbar/large/large-container-mana-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/large/large-container-mana.pngis excluded by!**/*.pngdata/images/game/topbar/large/large-container-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/large/large-container.pngis excluded by!**/*.pngdata/images/game/topbar/large/small-container-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/large/small-container.pngis excluded by!**/*.pngdata/images/game/topbar/left-bg.pngis excluded by!**/*.pngdata/images/game/topbar/marker_left.pngis excluded by!**/*.pngdata/images/game/topbar/marker_top.pngis excluded by!**/*.pngdata/images/game/topbar/parallel/parallel-health-mana-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/parallel/parallel-health-mana.pngis excluded by!**/*.pngdata/images/game/topbar/proficiency-bg.pngis excluded by!**/*.pngdata/images/game/topbar/proficiency-button.pngis excluded by!**/*.pngdata/images/game/topbar/proficiency-large-button.pngis excluded by!**/*.pngdata/images/game/topbar/proficiency-progress.pngis excluded by!**/*.pngdata/images/game/topbar/progress/mana-progressbar-large-100-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/mana-progressbar-large-100.pngis excluded by!**/*.pngdata/images/game/topbar/progress/manashield-progressbar-large-100-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/manashield-progressbar-large-100.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-10-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-10.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-100-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-100.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-30-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-30.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-4-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-4.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-60-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-60.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-95-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/progressbar-large-95.pngis excluded by!**/*.pngdata/images/game/topbar/progress/shieldmana-progressbar-large-100-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/shieldmana-progressbar-large-100.pngis excluded by!**/*.pngdata/images/game/topbar/progress/wide-mana-progressbar-large-100-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/wide-progressbar-large-10-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/wide-progressbar-large-100-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/wide-progressbar-large-30-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/wide-progressbar-large-4-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/wide-progressbar-large-60-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progress/wide-progressbar-large-95-vertical.pngis excluded by!**/*.pngdata/images/game/topbar/progressbar-large-100.pngis excluded by!**/*.pngdata/images/game/topbar/right-bg.pngis excluded by!**/*.pngdata/images/game/topbar/topBarBG.pngis excluded by!**/*.pngdata/images/options/button_proficiency.pngis excluded by!**/*.png
📒 Files selected for processing (22)
data/json/SkillwheelStringsJsonLibrary.jsondata/json/proficiencies.jsondata/styles/30-statsbar.otuimodules/game_interface/widgets/statsbar.luamodules/game_inventory/inventory.luamodules/game_proficiency/const.luamodules/game_proficiency/proficiency.luamodules/game_proficiency/proficiency.otmodmodules/game_proficiency/proficiency.otuimodules/game_proficiency/proficiency_data.luasrc/client/const.hsrc/client/game.cppsrc/client/game.hsrc/client/luafunctions.cppsrc/client/protocolcodes.hsrc/client/protocolgame.hsrc/client/protocolgameparse.cppsrc/client/protocolgamesend.cppsrc/client/thingtype.cppsrc/client/thingtype.hsrc/framework/luaengine/luavaluecasts.hsrc/protobuf/appearances.proto
🧰 Additional context used
🧬 Code graph analysis (9)
src/framework/luaengine/luavaluecasts.h (2)
src/framework/luaengine/luavaluecasts.cpp (16)
push_luavalue(27-31)push_luavalue(27-27)push_luavalue(40-44)push_luavalue(40-40)push_luavalue(55-59)push_luavalue(55-55)push_luavalue(70-74)push_luavalue(70-70)push_luavalue(83-87)push_luavalue(83-83)push_luavalue(90-102)push_luavalue(90-90)push_luavalue(128-140)push_luavalue(128-128)push_luavalue(166-174)push_luavalue(166-166)src/framework/luaengine/luainterface.cpp (4)
isTable(1292-1296)isTable(1292-1292)rawGeti(1091-1095)rawGeti(1091-1091)
src/client/game.h (2)
src/client/game.cpp (6)
sendWeaponProficiencyAction(1871-1877)sendWeaponProficiencyAction(1871-1871)sendWeaponProficiencyApply(1879-1885)sendWeaponProficiencyApply(1879-1879)sendWeaponProficiencyApplyLua(1887-1900)sendWeaponProficiencyApplyLua(1887-1887)src/client/protocolgamesend.cpp (4)
sendWeaponProficiencyAction(1592-1603)sendWeaponProficiencyAction(1592-1592)sendWeaponProficiencyApply(1605-1621)sendWeaponProficiencyApply(1605-1605)
modules/game_interface/widgets/statsbar.lua (1)
meta.lua (1)
g_game.getClientVersion(1049-1049)
src/client/luafunctions.cpp (2)
src/client/game.cpp (4)
sendWeaponProficiencyAction(1871-1877)sendWeaponProficiencyAction(1871-1871)sendWeaponProficiencyApplyLua(1887-1900)sendWeaponProficiencyApplyLua(1887-1887)src/client/protocolgamesend.cpp (2)
sendWeaponProficiencyAction(1592-1603)sendWeaponProficiencyAction(1592-1592)
src/client/game.cpp (2)
src/client/protocolgamesend.cpp (4)
sendWeaponProficiencyAction(1592-1603)sendWeaponProficiencyAction(1592-1592)sendWeaponProficiencyApply(1605-1621)sendWeaponProficiencyApply(1605-1605)src/client/game.h (1)
canPerformGameAction(337-340)
modules/game_proficiency/proficiency_data.lua (3)
meta.lua (2)
g_resources.fileExists(3711-3711)g_resources.readFileContents(3752-3752)modules/game_proficiency/proficiency.lua (1)
WeaponProficiency:createItemCache(659-728)modules/corelib/table.lua (1)
table.size(162-169)
src/client/protocolgame.h (2)
src/client/game.cpp (4)
sendWeaponProficiencyAction(1871-1877)sendWeaponProficiencyAction(1871-1871)sendWeaponProficiencyApply(1879-1885)sendWeaponProficiencyApply(1879-1879)src/client/protocolgamesend.cpp (4)
sendWeaponProficiencyAction(1592-1603)sendWeaponProficiencyAction(1592-1592)sendWeaponProficiencyApply(1605-1621)sendWeaponProficiencyApply(1605-1605)
src/client/thingtype.cpp (1)
src/client/tile.h (1)
m_flags(200-200)
modules/game_proficiency/proficiency.lua (1)
modules/game_proficiency/proficiency_data.lua (11)
ProficiencyData:getCurrentLevelByExp(713-732)ProficiencyData:getLevelPercent(680-696)ProficiencyData:getMaxExperienceByLevel(707-710)ProficiencyData:getProficiencyIdForItem(374-493)ProficiencyData:getPerkLaneCount(496-502)ProficiencyData:getMaxExperience(673-677)ProficiencyData:getTotalPercent(699-704)ProficiencyData:getContentById(324-327)ProficiencyData:getImageSourceAndClip(537-576)ProficiencyData:getAugmentIconClip(637-643)ProficiencyData:getBonusNameAndTooltip(579-634)
⏰ 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: ubuntu-24.04-linux-debug
- GitHub Check: windows-2022-windows-release
- GitHub Check: docker-build
🔇 Additional comments (26)
src/client/const.h (1)
1068-1068: LGTM!The new
ThingFlagAttrProficiencyflag correctly usesstatic_cast<uint64_t>for the bit shift and follows the established pattern for high-bit flags.src/protobuf/appearances.proto (1)
186-195: LGTM!The protobuf changes correctly:
- Remove tag 61 from the reserved set and assign it to the new
proficiencyfield- Define
AppearanceFlagProficiencymessage following the same pattern as similar flags (e.g.,AppearanceFlagLenshelp)- Use
optionalfor backward compatibilitysrc/framework/luaengine/luavaluecasts.h (1)
590-599: LGTM!The
push_luavalueimplementation forstd::paircorrectly creates a 2-element array table withfirstat index 1 andsecondat index 2, following the same pattern as other container types.modules/game_proficiency/const.lua (2)
1-56: LGTM!The constants file is well-organized with clear categorization of perk types, augments, and experience tables. The
ExperienceTablestructure with different progression rates forregular,knight, andcrossbowroles is a clean design.
324-333: Good defensive pattern for optional globals.The
UnknownCategoriestable correctly uses theorfallback pattern (WEAPON_AXE or 2) to handle cases where the weapon type constants might not be defined, ensuring the module remains functional even if those globals are missing.src/client/protocolcodes.h (1)
325-325: LGTM! No conflicts found.The new
ClientWeaponProficiency = 179opcode is correctly added to theClientOpcodesenum. Opcode 179 is already referenced inprotocolgamesend.cppfor weapon proficiency actions, and the client/server opcode namespaces are separate, so no conflict exists withGameServerCloseChannel = 179in theGameServerOpcodesenum.src/client/thingtype.h (1)
189-190: LGTM!The new
getProficiencyId()accessor andm_proficiencyIdmember follow the existing patterns in the class (e.g.,getSkillWheelGemQualityId). The getter is const-correct, and the member is properly zero-initialized.Also applies to: 263-263
modules/game_interface/widgets/statsbar.lua (3)
20-23: LGTM!Height increase from 35 to 52 pixels for Large and Default dimensions accommodates the new proficiency widget in the stats bar.
512-513: LGTM!Properly initializes the proficiency top bar after constructing the stats bar.
688-709: LGTM!The
initProficiencyTopBar()function is well-structured with appropriate defensive checks:
- Null check for stats bar
- Widget existence check before visibility toggle
- Version gating (>= 1500) for the proficiency feature
- Module and function existence check before calling
updateTopBarProficiency()The function is called from both
constructStatsBar()andOnGameStart()to ensure proper initialization in all scenarios.src/client/thingtype.cpp (1)
369-375: LGTM!The proficiency flag handling correctly:
- Checks for the presence of the proficiency data with
has_proficiency()- Guards the feature with
g_game.getFeature(Otc::GameProficiency)- Sets the member and flag appropriately
Both
ThingFlagAttrProficiency(defined in const.h:1068) andOtc::GameProficiency(defined in const.h:471) are properly defined. This follows the pattern used for other appearance flags in this function.src/client/protocolgame.h (2)
152-153: LGTM!The new method declarations are well-designed:
sendWeaponProficiencyActionappropriately uses a default parameter foritemIdsince it's only needed for specific action types (0 and 2).sendWeaponProficiencyApplyuses a clean pair structure for perk data.Both signatures correctly match their implementations in
protocolgamesend.cpp.
370-371: LGTM!The parser method is correctly placed under the 15.x protocol section and follows the established pattern for message parsers.
src/client/game.h (1)
394-398: LGTM!The weapon proficiency API follows the established pattern used by similar features (e.g., imbuing). The
sendWeaponProficiencyApplyLuawrapper correctly adapts the pair-based API for Lua's table semantics.One observation: the implementation uses
std::min(levels.size(), perkPositions.size())which silently truncates mismatched arrays. This is acceptable behavior, but consider whether a debug log would help diagnose Lua-side issues if callers accidentally pass arrays of different lengths.src/client/luafunctions.cpp (1)
388-389: LGTM!The Lua bindings are correctly set up:
sendWeaponProficiencyActionmaps directly to the C++ method.sendWeaponProficiencyApplycorrectly maps tosendWeaponProficiencyApplyLua, which provides a Lua-friendly interface using parallel arrays instead ofstd::pair. This appropriately hides the internal implementation from Lua callers.src/client/protocolgamesend.cpp (1)
1592-1621: LGTM! Clean implementation of weapon proficiency protocol methods.The new protocol methods follow the established patterns in this file. The conditional itemId serialization for specific action types and the perk array serialization are implemented correctly.
Minor observation: The comment on lines 1615-1616 documents the 0-indexed expectation clearly, which is helpful for maintenance.
data/styles/30-statsbar.otui (2)
1300-1372: Well-structured proficiency top bar widget.The widget definition is clean with:
- Proper anchor relationships between child elements
- Defensive onClick handler that checks module existence
- Clear visual hierarchy with button, background, progress bar, label, and icon
- Default hidden state (
visible: false) appropriate for version-gated features
509-515: The anchor ordering is not an issue. The OTUI parser supports forward references to elements defined later in the file. This pattern is used extensively throughoutdata/styles/30-statsbar.otui(at lines 406, 418, 512, 528, 588, 604, 625, 632, 659, 1066, 1081, 1095, 1255, 1268, 1281) without causing layout problems, confirming that anchors are resolved at runtime rather than parse time.modules/game_proficiency/proficiency.otui (3)
1-6: LGTM! Clean base widget definitions.The
MiniStarand widget hierarchy are well-defined with appropriate sizing and image sources.
7-44: ItemBox widget properly configured for interactive item display.Good use of:
change-cursor-image: trueandcursor: pointerfor UX feedbacktooltip-delayed: trueto prevent tooltip spamfocusable: truewith$focusstate for keyboard navigationphantom: trueon child widgets to allow click-through
413-416: Dynamic ItemBox creation is appropriate for this use case.Creating 45 ItemBox widgets at setup time is reasonable for the fixed-size grid. The naming convention
widget_+ index allows easy lookup.modules/game_proficiency/proficiency_data.lua (1)
750-774: Robust vocation restriction handling with multiple format support.Good defensive coding:
- Handles both table and number formats for
restrictVocation- Provides fallback for environments without
bit32- Clear comments explaining the bitmask logic
modules/game_proficiency/proficiency.lua (4)
260-296: Perk conversion logic handles edge cases well.The 0-indexed to 1-indexed conversion (lines 276-277) and the filtering of invalid values (lines 274-275) are correctly implemented. The cache preservation logic (lines 284-296) for empty server responses is a good defensive measure.
1762-1778: Vocation filter bitmask calculation is correct but could use a helper.The logic duplicates what's in
proficiency_data.lua(lines 752-773). Consider extracting to a shared utility function to maintain DRY principle.However, given this is filter logic vs. profession type determination, the duplication may be acceptable for clarity.
1897-1904: Perk array conversion correctly handles 1-indexed to 0-indexed transformation.The conversion from Lua 1-indexed format to server 0-indexed format is correctly implemented:
levelIndex - 1andperkIndex - 1for the protocol- Sorting by level ensures consistent ordering
1852-1855: No action required; empty arrays are properly handled by the protocol implementation.The Lua call
g_game.sendWeaponProficiencyApply(self.selectedItemId, {}, {})correctly passes two empty arrays tosendWeaponProficiencyApplyLuainsrc/client/game.cpp. The C++ implementation converts the parallel arrays to a vector of pairs usingstd::min(levels.size(), perkPositions.size()), which results in an empty perks vector when both input arrays are empty. The protocol message correctly encodes this asperksCount = 0, which is a valid message format that the server should handle.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Get first element (index 1 in Lua) | ||
| g_lua.rawGeti(index < 0 ? index - 1 : index, 1); | ||
| K firstValue; | ||
| if (!luavalue_cast(-1, firstValue)) { | ||
| g_lua.pop(); | ||
| } else { | ||
| return false; | ||
| } | ||
| if (g_lua.next(index < 0 ? index - 1 : index)) { | ||
| V value; | ||
| if (!luavalue_cast(-1, value)) | ||
| pair.second = value; | ||
| pair.first = firstValue; | ||
| g_lua.pop(); | ||
|
|
||
| // Get second element (index 2 in Lua) | ||
| g_lua.rawGeti(index < 0 ? index - 1 : index, 2); | ||
| V secondValue; | ||
| if (!luavalue_cast(-1, secondValue)) { | ||
| g_lua.pop(); | ||
| } else { | ||
| return false; | ||
| } | ||
| pair.second = secondValue; | ||
| g_lua.pop(); |
There was a problem hiding this comment.
Critical: Arguments to rawGeti are swapped.
The rawGeti function signature is rawGeti(int n, int index) where n is the table key and index is the stack position. The current code passes these in the wrong order and also applies an incorrect stack adjustment.
Current behavior with index = -1:
rawGeti(index < 0 ? index - 1 : index, 1)becomesrawGeti(-2, 1)— this tries to get element-2from table at stack position1, which is incorrect.
Proposed fix
if (g_lua.isTable(index)) {
// Get first element (index 1 in Lua)
- g_lua.rawGeti(index < 0 ? index - 1 : index, 1);
+ g_lua.rawGeti(1, index);
K firstValue;
if (!luavalue_cast(-1, firstValue)) {
g_lua.pop();
return false;
}
pair.first = firstValue;
g_lua.pop();
// Get second element (index 2 in Lua)
- g_lua.rawGeti(index < 0 ? index - 1 : index, 2);
+ g_lua.rawGeti(2, index);
V secondValue;
if (!luavalue_cast(-1, secondValue)) {
g_lua.pop();
return false;
}
pair.second = secondValue;
g_lua.pop();🤖 Prompt for AI Agents
In `@src/framework/luaengine/luavaluecasts.h` around lines 605 - 623, The rawGeti
calls pass arguments in the wrong order and wrongly adjust negative stack
indexes; swap the arguments so the first parameter is the table stack-position
and the second is the element key, and stop subtracting 1 from negative indexes.
Replace the two calls g_lua.rawGeti(index < 0 ? index - 1 : index, 1) and
g_lua.rawGeti(index < 0 ? index - 1 : index, 2) with g_lua.rawGeti(index, 1) and
g_lua.rawGeti(index, 2) respectively (leaving the surrounding luavalue_cast(-1,
firstValue)/luavalue_cast(-1, secondValue), pop(), and assignments to
pair.first/pair.second unchanged).
|
working fine for me. |
| bool luavalue_cast(int index, std::unordered_map<K, V, H>& map); | ||
|
|
||
| // pair | ||
| template<class K, class V> |
| int push_luavalue(const std::pair<K, V>& pair) | ||
| { | ||
| g_lua.createTable(2, 0); | ||
| push_internal_luavalue(pair.first); |
| K value; | ||
| if (!luavalue_cast(-1, value)) | ||
| pair.first = value; | ||
| // Get first element (index 1 in Lua) |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@modules/game_proficiency/proficiency.lua`:
- Around line 1817-1824: The pcall around self:applyPendingSelections() swallows
errors (variable err) without logging; modify the error path in the block (and
similarly in onOkClick) to log the error before clearing pendingSelections and
calling updateApplyButtonState — include the err and a stack traceback (or
tostring(err)) for context and use the module's existing logging mechanism (or
print/warn if none) so failures in applyPendingSelections are visible while
still clearing pendingSelections and updating the button state.
- Around line 955-958: In refreshItemList the cache key uses "local cacheId =
marketItem.marketData.showAs or displayId" which treats showAs==0 as false and
yields 0; change the logic to explicitly test for nil (e.g. if
marketItem.marketData and marketItem.marketData.showAs ~= nil then cacheId =
marketItem.marketData.showAs else cacheId = displayId end) so that a 0 showAs
uses displayId correctly; update the code around cacheId, displayId and
marketItem.marketData.showAs (the same block that calls itemWidget:setItemId) to
use the nil-check instead of Lua's falsy or pattern.
- Around line 735-741: The sort key construction inside the table.sort
comparator uses "a.marketData.showAs or a.displayId or a.originalId" which
treats showAs==0 as truthy and causes cache lookup mismatches; update the
idA/idB selection in the comparator (the table.sort anonymous function) to
explicitly treat showAs==0 as invalid (e.g. check for nil and nonzero) and fall
back to displayId or originalId so WeaponProficiency.cacheList is looked up
using the actual originalId keys populated by onWeaponProficiency/selectItem.
- Around line 1756-1771: Replace the fragile math.pow usage and handle
playerVocation==0: ensure playerVocation is >=1 (skip or continue if not), then
compute an integer bit mask instead of a float for vocBit; when bit32 is
available use bit32.lshift(1, playerVocation - 1) to produce an integer mask,
otherwise build the mask with integer multiplication (local vocBit = 1; for i=1,
playerVocation-1 do vocBit = vocBit * 2 end) and use that vocBit in the
bit32.band or modulo/shift checks on restrictVocation (variables:
restrictVocation, playerVocation, vocBit, filteredItems, bit32).
- Around line 627-644: The nested closures and if-blocks around
searchText.onTextChange and clearButton.onClick have inconsistent indentation
that obscures which "end" corresponds to which block; re-indent the block
containing searchText.onTextChange (the function assigned to
searchText.onTextChange), its enclosing if that checks for searchText, and the
clearButton.onClick function and its enclosing if so that each function body and
its corresponding "end" align visually with their opening statement (e.g., align
the "end" for searchText.onTextChange directly under the "function(widget,
text)" line, the "end" for the if that found searchText under that if, and
similarly align clearButton.onClick's function end and the if that found
clearButton); ensure WeaponProficiency:refreshItemList(),
WeaponProficiency.searchFilter assignments, and searchWidget:setText('') remain
inside the correct closures (searchText.onTextChange and clearButton.onClick
respectively) and close the blocks in the order: function end, enclosing if end,
then outer scopes.
- Around line 120-154: initTopBarProficiency currently reschedules itself
forever via scheduleEvent when modules.game_interface.StatsBar or statsBar is
nil; add a bounded retry counter (e.g., maxRetries = 15) to the function so
repeated calls increment an attempts parameter and stop rescheduling after the
cap to avoid an infinite loop. Modify initTopBarProficiency to accept an
optional attempts argument (default 0), increment it on each reschedule, and
only call scheduleEvent(initTopBarProficiency, 500, attempts+1) (or equivalent)
when attempts < maxRetries; apply this check in both places that currently call
scheduleEvent (the StatsBarModule nil branch and the statsBar nil branch) and
ensure normal behavior continues when the module becomes available.
In `@src/client/protocolgameparse.cpp`:
- Around line 6340-6369: parseWeaponProficiencyInfo currently parses an opcode
unguarded which can desync older clients; add the same version guard used for
GameServerWeaponProficiencyExperience by checking getClientVersion() >= 1510
before parsing or calling parseWeaponProficiencyInfo (or bail/ignore the
opcode), and apply that same check at the opcode dispatch site that invokes
parseWeaponProficiencyInfo; also replace the magic default 32 with a named
constant (e.g., constexpr uint16_t MarketCategoryWeaponsAll = 32) and use that
constant when initializing marketCategory to improve clarity; keep references to
parseWeaponProficiencyInfo, GameServerWeaponProficiencyExperience,
getClientVersion(), and g_things when making the changes.
🧹 Nitpick comments (5)
modules/game_proficiency/proficiency.lua (5)
4-6:__indexassignment is unused — nosetmetatablecall exists.
WeaponProficiency.__index = WeaponProficiencyon line 6 has no effect becauseWeaponProficiencyis never used as a metatable viasetmetatable. All methods are called directly on the table (e.g.,WeaponProficiency:selectItem(...)orself:refreshItemList()), so the__indexfield is dead code.
1779-1813:applyOneHandedFilterandapplyTwoHandedFilterare dead code.These two functions are never called. The 1H/2H filtering is handled inline inside
refreshItemList(lines 884–915), which also covers the "both active" case. Consider removing these unused functions to avoid maintenance confusion.
1417-1418: Redundant re-declaration oficonPerksandiconPerksGrey.These locals were already declared on lines 1385–1386 with identical
getChildByIdcalls. The secondlocaldeclarations shadow the first and waste a widget lookup. Remove lines 1417–1418 and reuse the existing variables.Proposed fix
-- Handle augment overlay icons visibility for spell augments - local iconPerks = bonusIcon:getChildById('iconPerks') - local iconPerksGrey = bonusIcon:getChildById('iconPerks-grey') if perkData.Type == PERK_SPELL_AUGMENT then
1116-1119: Server request fires on every item selection — consider debouncing.
sendWeaponProficiencyActionis called every time an item is clicked (line 1118). Rapid clicks through the item list could burst many requests. A simple debounce (e.g., cancel a previously scheduled event and re-schedule with a short delay) would reduce unnecessary traffic.
333-337: Re-sorting all categories on every experience tick may be expensive during active combat.Each call to
onWeaponProficiencyExperiencetriggerstable.sorton all 7 category lists. If the server sends frequent XP updates, consider deferring the re-sort (e.g., mark categories dirty and sort lazily when the list is next displayed).
| function initTopBarProficiency() | ||
| -- Delay initialization to ensure StatsBar is fully loaded | ||
| scheduleEvent(function() | ||
| -- Access StatsBar through modules.game_interface | ||
| local StatsBarModule = modules.game_interface and modules.game_interface.StatsBar | ||
| if not StatsBarModule then | ||
| scheduleEvent(initTopBarProficiency, 500) | ||
| return | ||
| end | ||
|
|
||
| local statsBar = StatsBarModule.getCurrentStatsBarWithPosition and StatsBarModule.getCurrentStatsBarWithPosition() | ||
| if statsBar then | ||
| local profWidget = statsBar:recursiveGetChildById('proficiencyTopBar') | ||
| if profWidget then | ||
| local shouldShow = g_game.getClientVersion() >= 1500 | ||
| profWidget:setVisible(shouldShow) | ||
|
|
||
| if shouldShow then | ||
| -- Request proficiency data for equipped weapon | ||
| local player = g_game.getLocalPlayer() | ||
| if player then | ||
| local leftSlotItem = player:getInventoryItem(InventorySlotLeft) | ||
| if leftSlotItem and g_game.sendWeaponProficiencyAction then | ||
| local itemId = leftSlotItem:getId() | ||
| g_game.sendWeaponProficiencyAction(0, itemId) | ||
| end | ||
| end | ||
| updateTopBarProficiency() | ||
| end | ||
| end | ||
| else | ||
| scheduleEvent(initTopBarProficiency, 500) | ||
| end | ||
| end, 500) -- 500ms delay | ||
| end |
There was a problem hiding this comment.
Unbounded retry loop in initTopBarProficiency can schedule events indefinitely.
If StatsBarModule is nil (line 126) or statsBar is nil (line 151), the function re-schedules itself every 500 ms with no retry limit. If the StatsBar module fails to load or is disabled, this runs forever.
Add a retry counter with a reasonable cap (e.g., 10–20 attempts).
Proposed fix
-function initTopBarProficiency()
- -- Delay initialization to ensure StatsBar is fully loaded
- scheduleEvent(function()
+function initTopBarProficiency(retryCount)
+ retryCount = retryCount or 0
+ if retryCount >= 15 then return end
+ scheduleEvent(function()
-- Access StatsBar through modules.game_interface
local StatsBarModule = modules.game_interface and modules.game_interface.StatsBar
if not StatsBarModule then
- scheduleEvent(initTopBarProficiency, 500)
+ initTopBarProficiency(retryCount + 1)
return
end
local statsBar = StatsBarModule.getCurrentStatsBarWithPosition and StatsBarModule.getCurrentStatsBarWithPosition()
if statsBar then
-- ... existing logic ...
else
- scheduleEvent(initTopBarProficiency, 500)
+ initTopBarProficiency(retryCount + 1)
end
end, 500)
end🤖 Prompt for AI Agents
In `@modules/game_proficiency/proficiency.lua` around lines 120 - 154,
initTopBarProficiency currently reschedules itself forever via scheduleEvent
when modules.game_interface.StatsBar or statsBar is nil; add a bounded retry
counter (e.g., maxRetries = 15) to the function so repeated calls increment an
attempts parameter and stop rescheduling after the cap to avoid an infinite
loop. Modify initTopBarProficiency to accept an optional attempts argument
(default 0), increment it on each reschedule, and only call
scheduleEvent(initTopBarProficiency, 500, attempts+1) (or equivalent) when
attempts < maxRetries; apply this check in both places that currently call
scheduleEvent (the StatsBarModule nil branch and the statsBar nil branch) and
ensure normal behavior continues when the module becomes available.
| searchText.onTextChange = function(widget, text) | ||
| WeaponProficiency.searchFilter = text | ||
| WeaponProficiency:refreshItemList() | ||
| end | ||
| end | ||
|
|
||
| -- Setup clear search button | ||
| local clearButton = WeaponProficiency.window:recursiveGetChildById('clearSearchButton') | ||
| if clearButton then | ||
| clearButton.onClick = function() | ||
| local searchWidget = WeaponProficiency.window:recursiveGetChildById('searchText') | ||
| if searchWidget then | ||
| searchWidget:setText('') | ||
| WeaponProficiency.searchFilter = nil | ||
| WeaponProficiency:refreshItemList() | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Misleading indentation in nested closures obscures block structure.
The end keywords for the onTextChange closure (lines 630), its enclosing if (line 631), the onClick closure (line 643), and the outer if blocks (lines 642, 644) are indented inconsistently, making it hard to see which end closes which block.
Proposed fix
if searchText then
searchText.onTextChange = function(widget, text)
WeaponProficiency.searchFilter = text
- WeaponProficiency:refreshItemList()
-end
+ WeaponProficiency:refreshItemList()
+ end
end
-- Setup clear search button
local clearButton = WeaponProficiency.window:recursiveGetChildById('clearSearchButton')
if clearButton then
clearButton.onClick = function()
local searchWidget = WeaponProficiency.window:recursiveGetChildById('searchText')
if searchWidget then
searchWidget:setText('')
WeaponProficiency.searchFilter = nil
WeaponProficiency:refreshItemList()
- end
- end
-end
+ end
+ end
+ end🤖 Prompt for AI Agents
In `@modules/game_proficiency/proficiency.lua` around lines 627 - 644, The nested
closures and if-blocks around searchText.onTextChange and clearButton.onClick
have inconsistent indentation that obscures which "end" corresponds to which
block; re-indent the block containing searchText.onTextChange (the function
assigned to searchText.onTextChange), its enclosing if that checks for
searchText, and the clearButton.onClick function and its enclosing if so that
each function body and its corresponding "end" align visually with their opening
statement (e.g., align the "end" for searchText.onTextChange directly under the
"function(widget, text)" line, the "end" for the if that found searchText under
that if, and similarly align clearButton.onClick's function end and the if that
found clearButton); ensure WeaponProficiency:refreshItemList(),
WeaponProficiency.searchFilter assignments, and searchWidget:setText('') remain
inside the correct closures (searchText.onTextChange and clearButton.onClick
respectively) and close the blocks in the order: function end, enclosing if end,
then outer scopes.
| table.sort(itemList, function(a, b) | ||
| -- Use showAs (marketData.showAs) for cache lookup - this is what the server uses | ||
| local idA = a.marketData.showAs or a.displayId or a.originalId | ||
| local idB = b.marketData.showAs or b.displayId or b.originalId | ||
|
|
||
| local expA = WeaponProficiency.cacheList[idA] and WeaponProficiency.cacheList[idA].exp or 0 | ||
| local expB = WeaponProficiency.cacheList[idB] and WeaponProficiency.cacheList[idB].exp or 0 |
There was a problem hiding this comment.
Cache key mismatch in sort: marketData.showAs can be 0 (truthy in Lua), producing a wrong lookup.
In Lua, 0 is truthy, so a.marketData.showAs or a.displayId or a.originalId evaluates to 0 when showAs is 0 — not to a.displayId as likely intended. The proficiency cache is keyed by originalId (set in onWeaponProficiency and selectItem), so items with showAs == 0 won't find their cache entry, and their experience will appear as 0 during sorting.
Proposed fix — guard against zero showAs
- local idA = a.marketData.showAs or a.displayId or a.originalId
- local idB = b.marketData.showAs or b.displayId or b.originalId
+ local showAsA = a.marketData.showAs
+ local idA = (showAsA and showAsA ~= 0) and showAsA or a.displayId or a.originalId
+ local showAsB = b.marketData.showAs
+ local idB = (showAsB and showAsB ~= 0) and showAsB or b.displayId or b.originalId🤖 Prompt for AI Agents
In `@modules/game_proficiency/proficiency.lua` around lines 735 - 741, The sort
key construction inside the table.sort comparator uses "a.marketData.showAs or
a.displayId or a.originalId" which treats showAs==0 as truthy and causes cache
lookup mismatches; update the idA/idB selection in the comparator (the
table.sort anonymous function) to explicitly treat showAs==0 as invalid (e.g.
check for nil and nonzero) and fall back to displayId or originalId so
WeaponProficiency.cacheList is looked up using the actual originalId keys
populated by onWeaponProficiency/selectItem.
| local displayId = marketItem.displayId or marketItem.originalId | ||
| -- Use showAs (displayId) as cache key - this is what the server uses | ||
| local cacheId = marketItem.marketData.showAs or displayId | ||
| itemWidget:setItemId(displayId) |
There was a problem hiding this comment.
Same showAs == 0 truthy issue for cache key in refreshItemList.
Line 957 has the same pattern: local cacheId = marketItem.marketData.showAs or displayId. When showAs is 0, cacheId becomes 0 instead of displayId, causing a cache miss and showing 0 stars for the item.
Proposed fix
- local cacheId = marketItem.marketData.showAs or displayId
+ local showAs = marketItem.marketData.showAs
+ local cacheId = (showAs and showAs ~= 0) and showAs or displayId🤖 Prompt for AI Agents
In `@modules/game_proficiency/proficiency.lua` around lines 955 - 958, In
refreshItemList the cache key uses "local cacheId = marketItem.marketData.showAs
or displayId" which treats showAs==0 as false and yields 0; change the logic to
explicitly test for nil (e.g. if marketItem.marketData and
marketItem.marketData.showAs ~= nil then cacheId = marketItem.marketData.showAs
else cacheId = displayId end) so that a 0 showAs uses displayId correctly;
update the code around cacheId, displayId and marketItem.marketData.showAs (the
same block that calls itemWidget:setItemId) to use the nil-check instead of
Lua's falsy or pattern.
| -- restrictVocation is a bitmask: bit N is set if vocation N can use the item | ||
| -- playerVocation is 1-based (1=Knight, 2=Paladin, etc.) | ||
| -- The bitmask uses 2^(vocation-1) for each vocation | ||
| local vocBit = math.pow(2, playerVocation - 1) | ||
| if bit32 then | ||
| -- Use bit32 library if available | ||
| if bit32.band(restrictVocation, vocBit) ~= 0 then | ||
| table.insert(filteredItems, item) | ||
| end | ||
| else | ||
| -- Fallback: use modulo arithmetic for bitwise AND | ||
| local shifted = math.floor(restrictVocation / vocBit) | ||
| if shifted % 2 == 1 then | ||
| table.insert(filteredItems, item) | ||
| end | ||
| end |
There was a problem hiding this comment.
math.pow returns a float, risking incorrect bitwise comparisons; edge case when playerVocation == 0.
math.pow(2, playerVocation - 1) produces a double. When playerVocation is 0, vocBit becomes 0.5, and the subsequent bit32.band / modulo check silently produces wrong results. Even for valid vocations, relying on float-to-int coercion inside bit32.band is fragile.
Proposed fix
+ if playerVocation <= 0 then
+ -- No vocation - can't filter, include item
+ table.insert(filteredItems, item)
+ else
- local vocBit = math.pow(2, playerVocation - 1)
- if bit32 then
- -- Use bit32 library if available
- if bit32.band(restrictVocation, vocBit) ~= 0 then
+ local vocBit = bit32 and bit32.lshift(1, playerVocation - 1) or (2 ^ (playerVocation - 1))
+ if bit32 then
+ if bit32.band(restrictVocation, vocBit) ~= 0 then
table.insert(filteredItems, item)
end
- else
- -- Fallback: use modulo arithmetic for bitwise AND
- local shifted = math.floor(restrictVocation / vocBit)
+ else
+ local shifted = math.floor(restrictVocation / vocBit)
if shifted % 2 == 1 then
table.insert(filteredItems, item)
end
end
+ end🤖 Prompt for AI Agents
In `@modules/game_proficiency/proficiency.lua` around lines 1756 - 1771, Replace
the fragile math.pow usage and handle playerVocation==0: ensure playerVocation
is >=1 (skip or continue if not), then compute an integer bit mask instead of a
float for vocBit; when bit32 is available use bit32.lshift(1, playerVocation -
1) to produce an integer mask, otherwise build the mask with integer
multiplication (local vocBit = 1; for i=1, playerVocation-1 do vocBit = vocBit *
2 end) and use that vocBit in the bit32.band or modulo/shift checks on
restrictVocation (variables: restrictVocation, playerVocation, vocBit,
filteredItems, bit32).
| local success, err = pcall(function() | ||
| self:applyPendingSelections() | ||
| end) | ||
| if not success then | ||
| -- Clear pending selections on error to allow closing | ||
| self.pendingSelections = {} | ||
| self:updateApplyButtonState() | ||
| end |
There was a problem hiding this comment.
Errors from applyPendingSelections are silently swallowed.
The err value returned by pcall is captured but never logged. If applyPendingSelections fails, the error is lost, making debugging difficult. The same pattern exists in onOkClick (line 1831).
Proposed fix
local success, err = pcall(function()
self:applyPendingSelections()
end)
if not success then
+ g_logger.error("Failed to apply proficiency selections: " .. tostring(err))
-- Clear pending selections on error to allow closing
self.pendingSelections = {}
self:updateApplyButtonState()
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local success, err = pcall(function() | |
| self:applyPendingSelections() | |
| end) | |
| if not success then | |
| -- Clear pending selections on error to allow closing | |
| self.pendingSelections = {} | |
| self:updateApplyButtonState() | |
| end | |
| local success, err = pcall(function() | |
| self:applyPendingSelections() | |
| end) | |
| if not success then | |
| g_logger.error("Failed to apply proficiency selections: " .. tostring(err)) | |
| -- Clear pending selections on error to allow closing | |
| self.pendingSelections = {} | |
| self:updateApplyButtonState() | |
| end |
🤖 Prompt for AI Agents
In `@modules/game_proficiency/proficiency.lua` around lines 1817 - 1824, The pcall
around self:applyPendingSelections() swallows errors (variable err) without
logging; modify the error path in the block (and similarly in onOkClick) to log
the error before clearing pendingSelections and calling updateApplyButtonState —
include the err and a stack traceback (or tostring(err)) for context and use the
module's existing logging mechanism (or print/warn if none) so failures in
applyPendingSelections are visible while still clearing pendingSelections and
updating the button state.
| void ProtocolGame::parseWeaponProficiencyInfo(const InputMessagePtr& msg) | ||
| { | ||
| msg->getU16(); // itemId | ||
| msg->getU32(); // experience | ||
|
|
||
| const uint8_t size = msg->getU8(); | ||
| for (auto j = 0; j < size; ++j) { | ||
| msg->getU8(); // proficiencyLevel | ||
| msg->getU8(); // perkPosition | ||
| // Opcode 0xC4 (196) - Weapon Proficiency Info | ||
| // Sent by server in response to sendWeaponProficiencyAction | ||
| // Structure: uint16 itemId, uint32 experience, uint8 perksCount, [perksCount * {uint8 level, uint8 perkPosition}] | ||
|
|
||
| const uint16_t itemId = msg->getU16(); | ||
| const uint32_t experience = msg->getU32(); | ||
| const uint8_t perksCount = msg->getU8(); | ||
|
|
||
| std::vector<std::pair<uint8_t, uint8_t>> perks; | ||
| for (int i = 0; i < perksCount; ++i) { | ||
| const uint8_t level = msg->getU8(); | ||
| const uint8_t perkPosition = msg->getU8(); | ||
| perks.emplace_back(level, perkPosition); | ||
| } | ||
|
|
||
| // Get market category for the item (for sorting in UI) | ||
| uint16_t marketCategory = 32; // Default: WeaponsAll | ||
| if (g_things.isValidDatId(itemId, ThingCategoryItem)) { | ||
| const auto& itemType = g_things.getThingType(itemId, ThingCategoryItem); | ||
| if (itemType) { | ||
| const auto& marketData = itemType->getMarketData(); | ||
| if (!marketData.name.empty()) { | ||
| marketCategory = marketData.category; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| g_lua.callGlobalField("g_game", "onWeaponProficiency", itemId, experience, perks, marketCategory); |
There was a problem hiding this comment.
Missing version gating in parseWeaponProficiencyInfo, inconsistent with the experience handler.
GameServerWeaponProficiencyExperience (Line 173) is gated behind getClientVersion() >= 1510, but parseWeaponProficiencyInfo and its dispatch site (Line 482) have no version check. If an older client somehow receives this opcode, the differing payload structure will cause a stream desync. For consistency and defensive correctness, consider adding the same guard here.
Also, the magic number 32 (Line 6358) for the default market category should ideally be a named constant for clarity.
Suggested version gate and named constant
void ProtocolGame::parseWeaponProficiencyInfo(const InputMessagePtr& msg)
{
+ if (g_game.getClientVersion() < 1510) {
+ return;
+ }
+
// Opcode 0xC4 (196) - Weapon Proficiency Info- uint16_t marketCategory = 32; // Default: WeaponsAll
+ static constexpr uint16_t kDefaultMarketCategoryWeaponsAll = 32;
+ uint16_t marketCategory = kDefaultMarketCategoryWeaponsAll;🤖 Prompt for AI Agents
In `@src/client/protocolgameparse.cpp` around lines 6340 - 6369,
parseWeaponProficiencyInfo currently parses an opcode unguarded which can desync
older clients; add the same version guard used for
GameServerWeaponProficiencyExperience by checking getClientVersion() >= 1510
before parsing or calling parseWeaponProficiencyInfo (or bail/ignore the
opcode), and apply that same check at the opcode dispatch site that invokes
parseWeaponProficiencyInfo; also replace the magic default 32 with a named
constant (e.g., constexpr uint16_t MarketCategoryWeaponsAll = 32) and use that
constant when initializing marketCategory to improve clarity; keep references to
parseWeaponProficiencyInfo, GameServerWeaponProficiencyExperience,
getClientVersion(), and g_things when making the changes.
|
Adding this to my TODO list this week. Sorry for the delay in testing. Expect modifications. |
libergod
requested review from
libergod
and removed request for
InnerCircleTFS
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as outdated.
This comment was marked as outdated.
Acho que voce esta utilizando outro modulo. Nao existe nenhum arquivo chamado weapon_proficiency.lua nesse PR |
|
You’re right, I had an old module in the mods folder. My apologies. |
6 participants
Description
This PR adds the Proficiency Module for Client 15.00+
Behavior
Actual
Type of change
Test Configuration:
Checklist
Summary by CodeRabbit