Open
Conversation
📝 WalkthroughWalkthroughAdds a complete imbuement UI subsystem, new SD/inverted progress bar widgets and styles, client-game bindings and protocol support for imbuement window actions, and multiple Lua controllers to drive item/scroll selection, imbuement application and clearing. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as ImbuementUI
participant Controller as ImbuementItem/Scroll
participant Game as GameClient
participant Protocol as ProtocolGame
participant Server
User->>UI: open imbuement window
UI->>Controller: populate panels (select/scroll/imbue)
User->>Controller: choose slot / select imbuement
Controller->>Controller: validate requirements (items, gold)
User->>Controller: confirm apply
Controller->>Game: call selectImbuementItem(itemId,pos,stackpos) / selectImbuementScroll()
Game->>Protocol: sendImbuementWindowAction(type, ... )
Protocol->>Server: transmit opcode 0xB2 payload
Server->>Protocol: respond GameServerInventoryImbuements
Protocol->>Game: parseImbuementWindow -> invoke Lua callbacks
Game->>UI: update UI state (onImbuementItem/onImbuementScroll)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@data/styles/10-progressbars.otui`:
- Around line 42-68: The file defines ProgressBarSD twice with different bases,
causing the first definition to be overridden; rename one of the ProgressBarSD
classes to a distinct name (e.g., ProgressBarSD_UIProgressBar or
ProgressBarSD_SD) and update all usages/inheritance to reference the new name so
each variant keeps its intended base (references to UIProgressBarSD vs
UIProgressBar), ensure derived classes like HealthBarSD inherit from the correct
renamed variant, and keep other classes (ProgressBarSDInv, InvManaBar,
InvVerticalBar, VerticalBar) unchanged unless they need to point to the renamed
symbol.
In `@modules/corelib/ui/uiprogressbarsd.lua`:
- Around line 4-15: The constructor UIProgressBarSD.create currently initializes
progressbar.min/progressbar.max causing self.minimum/self.maximum to remain nil
and break setValue clamps; change the initialization to set progressbar.minimum
and progressbar.maximum (and keep progressbar.value) so setValue and any
style-driven percent calculations have valid bounds; update any places relying
on min/max to use minimum/maximum or set both aliases if needed to preserve
compatibility.
In `@modules/corelib/uiprogressbarsd.lua`:
- Around line 4-16: In UIProgressBarSD.create(), the progress bar initializes
progressbar.min/progressbar.max which leaves self.minimum/self.maximum nil (used
by setValue); change the initialization to set progressbar.minimum = 0 and
progressbar.maximum = 100 (keep progressbar.value = 0 and the other setup calls
as-is) so clamps in setValue work correctly.
In `@modules/corelib/uiprogressbarsdinverted.lua`:
- Around line 4-15: The create function sets progressbar.min/progressbar.max
which leaves progressbar.minimum/progressbar.maximum nil and causes setValue
(which clamps against self.minimum/self.maximum) to fail; change
UIProgressBarSDInverted.create to initialize progressbar.minimum and
progressbar.maximum (and set progressbar.value after them) instead of
progressbar.min/progressbar.max so setValue/clamping works correctly, leaving
other initializations (bgBorder*, setFocusable, setOn, insertLuaCall("onSetup"))
unchanged.
In `@modules/game_imbui/classes/imbuementitem.lua`:
- Around line 19-29: The loop in ImbuementItem.setup uses `#activeSlots` which
fails if activeSlots is 0-based; change the iteration to use the provided slots
count instead (iterate from 0 to slots-1) when populating
self.activeSlots["slot"..i] = activeSlots[i] or {}; update the loop in
ImbuementItem.setup to use the function parameter slots (or self.slots) as the
upper bound so slot1/slot2 aren't dropped when indices start at 0.
In `@modules/game_imbui/classes/imbuementselection.lua`:
- Around line 52-92: In ImbuementSelection.onChooseItemMouseRelease the branch
for scroll selection incorrectly calls g_game.selectImbuementItem; change it so
when self.isSelectingScroll is true you call g_game.selectImbuementScroll() (no
arguments) and when false call g_game.selectImbuementItem(itemId, pos,
stackPos); keep the rest of the cleanup (self.pickItem:ungrabMouse(),
g_mouse.popCursor('target')) as-is.
In `@modules/game_imbui/t_imbui.lua`:
- Around line 185-209: The toggleMenu function uses a Lua 5.2-only goto (label
::continue::) which breaks under LuaJIT; refactor Imbuement:toggleMenu to remove
the goto by inverting the condition and using nested conditionals: iterate
pairs(self), skip non-userdata or key == 'window' with an if check (e.g., if
type(value) == 'userdata' and key ~= 'window' then ... end), then inside handle
the key == menu branch to show and adjust self.window:setHeight(...) for the
four menu cases and an else branch to hide the value; ensure the loop simply
continues naturally without any goto or labels.
In `@modules/game_imbuing/imbuing.lua`:
- Around line 161-175: The setProtection function accesses selectedImbue['cost']
and ['protectionCost'] without ensuring selectedImbue is defined; guard against
nil by checking selectedImbue at the top of the protection-true branch (or
early-return if protection cannot be enabled) before using its fields, and only
update emptyImbue.cost, emptyImbue.successRate, emptyImbue.cost:setColor and
protectionBtn:setImageClip when selectedImbue is non-nil; reference the
setProtection function, selectedImbue, emptyImbue and protectionBtn to locate
and apply the guard and handle the insufficient-gold check (bankGold +
inventoryGold) safely.
In `@src/client/protocolgameparse.cpp`:
- Around line 5746-5832: The code in ProtocolGame::parseImbuementWindow reads a
windowType for client versions >= 1510 but only handles values 0,1,2 and will
fall through into the legacy parser on any other value; add a default guard
immediately after reading const uint8_t windowType = msg->getU8() that detects
when windowType is not 0,1,2, logs a warning (using the project's logger)
including the unexpected windowType and returns early to avoid corrupting the
message stream and desync with the legacy parsing path.
🧹 Nitpick comments (4)
modules/game_imbuing/imbuing.lua (1)
64-140: Consider extracting the “hasAllItems/hasEnoughGold + UI state” logic into a helper.
This logic is repeated inonOptionChange,setProtection, andonResourcesBalanceChange, which makes future tweaks easy to miss.src/client/protocolcodes.h (1)
224-224: Keep the custom opcode in the 51–99 block for readability.
Line 224 introduces a value in the custom range but places it after higher-valued opcodes; consider moving it into the “NOTE: add any custom opcodes in this range” section to keep ordering/scanning consistent.src/client/protocolgamesend.cpp (1)
1469-1482: Avoid magic opcode/type values and validate action types.
Line 1472 hard-codes0xB2and Line 1473 uses raw type codes. Please confirm this opcode is valid for all supported protocol versions; if it’s 15+ only, guard older versions. Also consider defining a named opcode inProto::ClientOpcodesand validating the action type to avoid malformed packets coming from Lua.♻️ Suggested hardening
void ProtocolGame::sendImbuementWindowAction(const uint8_t type, const uint16_t itemId, const Position& pos, const uint8_t stackpos) { + // If this is protocol 15+ only, guard older clients. + if (g_game.getClientVersion() < 1510) { + g_logger.warn("Imbuement window action not supported for client version {}", g_game.getClientVersion()); + return; + } + + if (type != 1 && type != 2) { + g_logger.warn("Invalid imbuement window action type: {}", type); + return; + } const auto& msg = std::make_shared<OutputMessage>(); - msg->addU8(0xB2); // same opcode as parseImbuementWindow on server - msg->addU8(type); // 1 = SELECT_ITEM, 2 = SCROLL + msg->addU8(Proto::ClientImbuementWindowAction); + msg->addU8(type);modules/game_imbui/t_imbui.lua (1)
17-74: PreferResourceTypesconstants ingetPlayerBalance.Line 31–32 uses numeric resource IDs; aligning with
ResourceTypes.BANK_BALANCE/ResourceTypes.GOLD_EQUIPPED(already used at Line 216–217) avoids drift if enums change.♻️ Suggested change
- local bankGold = player:getResourceBalance(1) or 0 -- BANK_BALANCE - local inventoryGold = player:getResourceBalance(0) or 0 -- GOLD_EQUIPPED + local bankGold = player:getResourceBalance(ResourceTypes.BANK_BALANCE) or 0 + local inventoryGold = player:getResourceBalance(ResourceTypes.GOLD_EQUIPPED) or 0
data/styles/10-progressbars.otui
Outdated
Comment on lines
+42
to
+68
| ProgressBarSD < UIProgressBarSD | ||
| height: 22 | ||
| tooltip-delayed: true | ||
|
|
||
| ProgressBarSD < UIProgressBar | ||
| height: 22 | ||
| tooltip-delayed: true | ||
|
|
||
| HealthBarSD < ProgressBarSD | ||
|
|
||
| ProgressBarSDInv < UIProgressBarSDInverted | ||
|
|
||
| InvManaBar < UIProgressBarSDInverted | ||
| size: 10 30 | ||
| image-border: 1 | ||
|
|
||
| InvVerticalBar < UIVerticalProgressBarSDInverted | ||
| size: 10 30 | ||
| image-border: 1 | ||
|
|
||
| VerticalBar < UIVerticalProgressBarSD | ||
| size: 10 30 | ||
| image-border: 1 | ||
|
|
||
| ProgressBarSD < UIProgressBarSD | ||
| height: 22 | ||
| tooltip-delayed: true No newline at end of file |
There was a problem hiding this comment.
Duplicate ProgressBarSD definitions override each other.
The class is declared multiple times with different bases, so only the last definition survives. If you need SD styling for both UIProgressBarSD and UIProgressBar, use distinct class names (and update references).
🔧 One possible consolidation
-ProgressBarSD < UIProgressBar
+ProgressBarSDPlain < UIProgressBar
height: 22
tooltip-delayed: true
@@
-ProgressBarSD < UIProgressBarSD
- height: 22
- tooltip-delayed: true🤖 Prompt for AI Agents
In `@data/styles/10-progressbars.otui` around lines 42 - 68, The file defines
ProgressBarSD twice with different bases, causing the first definition to be
overridden; rename one of the ProgressBarSD classes to a distinct name (e.g.,
ProgressBarSD_UIProgressBar or ProgressBarSD_SD) and update all
usages/inheritance to reference the new name so each variant keeps its intended
base (references to UIProgressBarSD vs UIProgressBar), ensure derived classes
like HealthBarSD inherit from the correct renamed variant, and keep other
classes (ProgressBarSDInv, InvManaBar, InvVerticalBar, VerticalBar) unchanged
unless they need to point to the renamed symbol.
Comment on lines
+4
to
+15
| function UIProgressBarSD.create() | ||
| local progressbar = UIProgressBarSD.internalCreate() | ||
| progressbar:setFocusable(false) | ||
| progressbar:setOn(true) | ||
| progressbar.min = 0 | ||
| progressbar.max = 100 | ||
| progressbar.value = 0 | ||
| progressbar.bgBorderLeft = 0 | ||
| progressbar.bgBorderRight = 0 | ||
| progressbar.bgBorderTop = 0 | ||
| progressbar.bgBorderBottom = 0 | ||
| return progressbar |
There was a problem hiding this comment.
Initialize minimum/maximum instead of min/max to avoid nil clamps.
setValue clamps using self.minimum/self.maximum; with current init those are nil until setters run, so a style-driven percent can throw.
🐛 Proposed fix
- progressbar.min = 0
- progressbar.max = 100
+ progressbar.minimum = 0
+ progressbar.maximum = 100📝 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.
Suggested change
| function UIProgressBarSD.create() | |
| local progressbar = UIProgressBarSD.internalCreate() | |
| progressbar:setFocusable(false) | |
| progressbar:setOn(true) | |
| progressbar.min = 0 | |
| progressbar.max = 100 | |
| progressbar.value = 0 | |
| progressbar.bgBorderLeft = 0 | |
| progressbar.bgBorderRight = 0 | |
| progressbar.bgBorderTop = 0 | |
| progressbar.bgBorderBottom = 0 | |
| return progressbar | |
| function UIProgressBarSD.create() | |
| local progressbar = UIProgressBarSD.internalCreate() | |
| progressbar:setFocusable(false) | |
| progressbar:setOn(true) | |
| progressbar.minimum = 0 | |
| progressbar.maximum = 100 | |
| progressbar.value = 0 | |
| progressbar.bgBorderLeft = 0 | |
| progressbar.bgBorderRight = 0 | |
| progressbar.bgBorderTop = 0 | |
| progressbar.bgBorderBottom = 0 | |
| return progressbar |
🤖 Prompt for AI Agents
In `@modules/corelib/ui/uiprogressbarsd.lua` around lines 4 - 15, The constructor
UIProgressBarSD.create currently initializes progressbar.min/progressbar.max
causing self.minimum/self.maximum to remain nil and break setValue clamps;
change the initialization to set progressbar.minimum and progressbar.maximum
(and keep progressbar.value) so setValue and any style-driven percent
calculations have valid bounds; update any places relying on min/max to use
minimum/maximum or set both aliases if needed to preserve compatibility.
Comment on lines
+4
to
+16
| function UIProgressBarSD.create() | ||
| local progressbar = UIProgressBarSD.internalCreate() | ||
| progressbar:setFocusable(false) | ||
| progressbar:setOn(true) | ||
| progressbar.min = 0 | ||
| progressbar.max = 100 | ||
| progressbar.value = 0 | ||
| progressbar.bgBorderLeft = 0 | ||
| progressbar.bgBorderRight = 0 | ||
| progressbar.bgBorderTop = 0 | ||
| progressbar.bgBorderBottom = 0 | ||
| progressbar:insertLuaCall("onSetup") | ||
| progressbar:insertLuaCall("onGeometryChange") |
There was a problem hiding this comment.
Initialize minimum/maximum instead of min/max to avoid nil clamps.
setValue clamps via self.minimum/self.maximum; with current init those are nil until setters run.
🐛 Proposed fix
- progressbar.min = 0
- progressbar.max = 100
+ progressbar.minimum = 0
+ progressbar.maximum = 100📝 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.
Suggested change
| function UIProgressBarSD.create() | |
| local progressbar = UIProgressBarSD.internalCreate() | |
| progressbar:setFocusable(false) | |
| progressbar:setOn(true) | |
| progressbar.min = 0 | |
| progressbar.max = 100 | |
| progressbar.value = 0 | |
| progressbar.bgBorderLeft = 0 | |
| progressbar.bgBorderRight = 0 | |
| progressbar.bgBorderTop = 0 | |
| progressbar.bgBorderBottom = 0 | |
| progressbar:insertLuaCall("onSetup") | |
| progressbar:insertLuaCall("onGeometryChange") | |
| function UIProgressBarSD.create() | |
| local progressbar = UIProgressBarSD.internalCreate() | |
| progressbar:setFocusable(false) | |
| progressbar:setOn(true) | |
| progressbar.minimum = 0 | |
| progressbar.maximum = 100 | |
| progressbar.value = 0 | |
| progressbar.bgBorderLeft = 0 | |
| progressbar.bgBorderRight = 0 | |
| progressbar.bgBorderTop = 0 | |
| progressbar.bgBorderBottom = 0 | |
| progressbar:insertLuaCall("onSetup") | |
| progressbar:insertLuaCall("onGeometryChange") |
🤖 Prompt for AI Agents
In `@modules/corelib/uiprogressbarsd.lua` around lines 4 - 16, In
UIProgressBarSD.create(), the progress bar initializes
progressbar.min/progressbar.max which leaves self.minimum/self.maximum nil (used
by setValue); change the initialization to set progressbar.minimum = 0 and
progressbar.maximum = 100 (keep progressbar.value = 0 and the other setup calls
as-is) so clamps in setValue work correctly.
Comment on lines
+4
to
+15
| function UIProgressBarSDInverted.create() | ||
| local progressbar = UIProgressBarSDInverted.internalCreate() | ||
| progressbar:setFocusable(false) | ||
| progressbar:setOn(true) | ||
| progressbar.min = 0 | ||
| progressbar.max = 100 | ||
| progressbar.value = 0 | ||
| progressbar.bgBorderLeft = 0 | ||
| progressbar.bgBorderRight = 0 | ||
| progressbar.bgBorderTop = 0 | ||
| progressbar.bgBorderBottom = 0 | ||
| progressbar:insertLuaCall("onSetup") |
There was a problem hiding this comment.
Initialize minimum/maximum instead of min/max to avoid nil clamps.
setValue clamps via self.minimum/self.maximum; with current init those are nil until setters run.
🐛 Proposed fix
- progressbar.min = 0
- progressbar.max = 100
+ progressbar.minimum = 0
+ progressbar.maximum = 100📝 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.
Suggested change
| function UIProgressBarSDInverted.create() | |
| local progressbar = UIProgressBarSDInverted.internalCreate() | |
| progressbar:setFocusable(false) | |
| progressbar:setOn(true) | |
| progressbar.min = 0 | |
| progressbar.max = 100 | |
| progressbar.value = 0 | |
| progressbar.bgBorderLeft = 0 | |
| progressbar.bgBorderRight = 0 | |
| progressbar.bgBorderTop = 0 | |
| progressbar.bgBorderBottom = 0 | |
| progressbar:insertLuaCall("onSetup") | |
| function UIProgressBarSDInverted.create() | |
| local progressbar = UIProgressBarSDInverted.internalCreate() | |
| progressbar:setFocusable(false) | |
| progressbar:setOn(true) | |
| progressbar.minimum = 0 | |
| progressbar.maximum = 100 | |
| progressbar.value = 0 | |
| progressbar.bgBorderLeft = 0 | |
| progressbar.bgBorderRight = 0 | |
| progressbar.bgBorderTop = 0 | |
| progressbar.bgBorderBottom = 0 | |
| progressbar:insertLuaCall("onSetup") |
🤖 Prompt for AI Agents
In `@modules/corelib/uiprogressbarsdinverted.lua` around lines 4 - 15, The create
function sets progressbar.min/progressbar.max which leaves
progressbar.minimum/progressbar.maximum nil and causes setValue (which clamps
against self.minimum/self.maximum) to fail; change
UIProgressBarSDInverted.create to initialize progressbar.minimum and
progressbar.maximum (and set progressbar.value after them) instead of
progressbar.min/progressbar.max so setValue/clamping works correctly, leaving
other initializations (bgBorder*, setFocusable, setOn, insertLuaCall("onSetup"))
unchanged.
Comment on lines
+19
to
+29
| function ImbuementItem.setup(itemId, tier, slots, activeSlots, availableImbuements, needItems) | ||
| self.itemId = itemId | ||
| self.tier = tier | ||
| self.slots = slots | ||
|
|
||
| self.activeSlots = {} | ||
| for i = 0, #activeSlots do | ||
| self.activeSlots["slot"..i] = activeSlots[i] or {} | ||
| end | ||
| self.availableImbuements = availableImbuements or {} | ||
| self.needItems = needItems or {} |
There was a problem hiding this comment.
#activeSlots can drop slots when indices start at 0.
Line 25 uses the length operator; if activeSlots is 0-based (slot0–slot2), # is undefined and may return 0, leaving slot1/slot2 empty. Iterate by slots count instead.
🐛 Suggested change
- self.activeSlots = {}
- for i = 0, `#activeSlots` do
- self.activeSlots["slot"..i] = activeSlots[i] or {}
- end
+ self.activeSlots = {}
+ activeSlots = activeSlots or {}
+ for i = 0, (slots or 0) - 1 do
+ self.activeSlots["slot"..i] = activeSlots[i] or {}
+ end🤖 Prompt for AI Agents
In `@modules/game_imbui/classes/imbuementitem.lua` around lines 19 - 29, The loop
in ImbuementItem.setup uses `#activeSlots` which fails if activeSlots is 0-based;
change the iteration to use the provided slots count instead (iterate from 0 to
slots-1) when populating self.activeSlots["slot"..i] = activeSlots[i] or {};
update the loop in ImbuementItem.setup to use the function parameter slots (or
self.slots) as the upper bound so slot1/slot2 aren't dropped when indices start
at 0.
Comment on lines
+52
to
+92
| function ImbuementSelection.onChooseItemMouseRelease(widget, mousePosition, mouseButton) | ||
| local item = nil | ||
| if mouseButton == MouseLeftButton then | ||
| local clickedWidget = modules.game_interface.getRootPanel():recursiveGetChildByPos(mousePosition, false) | ||
| if clickedWidget then | ||
| if clickedWidget:getClassName() == 'UIGameMap' then | ||
| local tile = clickedWidget:getTile(mousePosition) | ||
| if tile then | ||
| local thing = tile:getTopMoveThing() | ||
| if thing and thing:isItem() then | ||
| item = thing | ||
| end | ||
| end | ||
| elseif clickedWidget:getClassName() == 'UIItem' and not clickedWidget:isVirtual() then | ||
| item = clickedWidget:getItem() | ||
| end | ||
| end | ||
| end | ||
|
|
||
| if item and item:isPickupable() then | ||
| local pos = item:getPosition() | ||
| local itemId = item:getId() | ||
| local stackPos = item:getStackPos() | ||
|
|
||
| if self.isSelectingScroll then | ||
| g_game.selectImbuementItem(itemId, pos, stackPos) | ||
| else | ||
| g_game.selectImbuementItem(itemId, pos, stackPos) | ||
| end | ||
|
|
||
| self.pickItem:ungrabMouse() | ||
| g_mouse.popCursor('target') | ||
|
|
||
| return true | ||
| else | ||
| modules.game_textmessage.displayFailureMessage(tr('Sorry, not possible.')) | ||
| end | ||
|
|
||
| self.pickItem:ungrabMouse() | ||
| g_mouse.popCursor('target') | ||
| return true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether scroll-specific and item selection APIs exist
rg -n "selectImbuementScroll" .
echo "---"
rg -n "selectImbuementItem" .Repository: opentibiabr/otclient
Length of output: 993
🏁 Script executed:
#!/bin/bash
# Read the specific file and lines mentioned in the review
cat -n modules/game_imbui/classes/imbuementselection.lua | sed -n '52,92p'Repository: opentibiabr/otclient
Length of output: 1641
Scroll selection uses wrong API.
Both if self.isSelectingScroll branches call g_game.selectImbuementItem() identically (lines 76–79). The scroll-specific API g_game.selectImbuementScroll() exists but is never invoked, causing scroll selections to be treated as item selections and failing the intended flow.
Fix
if self.isSelectingScroll then
- g_game.selectImbuementItem(itemId, pos, stackPos)
+ g_game.selectImbuementScroll()
else
g_game.selectImbuementItem(itemId, pos, stackPos)
endNote: selectImbuementScroll() takes no parameters, unlike selectImbuementItem().
🤖 Prompt for AI Agents
In `@modules/game_imbui/classes/imbuementselection.lua` around lines 52 - 92, In
ImbuementSelection.onChooseItemMouseRelease the branch for scroll selection
incorrectly calls g_game.selectImbuementItem; change it so when
self.isSelectingScroll is true you call g_game.selectImbuementScroll() (no
arguments) and when false call g_game.selectImbuementItem(itemId, pos,
stackPos); keep the rest of the cleanup (self.pickItem:ungrabMouse(),
g_mouse.popCursor('target')) as-is.
Comment on lines
+185
to
+209
| function Imbuement:toggleMenu(menu) | ||
| for key, value in pairs(self) do | ||
| if type(value) ~= 'userdata' or key == 'window' then | ||
| goto continue | ||
| end | ||
|
|
||
| if key == menu then | ||
| value:show() | ||
| -- Ajustar tamanho da janela baseado no menu | ||
| if menu == 'selectItemOrScroll' then | ||
| self.window:setHeight(388) | ||
| elseif menu == 'scrollImbue' then | ||
| self.window:setHeight(655) | ||
| elseif menu == 'selectImbue' then | ||
| self.window:setHeight(528) | ||
| elseif menu == 'clearImbue' then | ||
| self.window:setHeight(502) | ||
| end | ||
| else | ||
| value:hide() | ||
| end | ||
|
|
||
| ::continue:: | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Lua version references in the codebase
rg -n "LuaJIT|LUA_VERSION|lua5|lua-5|lua51|lua52|lua53|lua54" -iRepository: opentibiabr/otclient
Length of output: 3139
🏁 Script executed:
# Also check for common version indicators in build/config files
fd -t f "(package|config|setup|version|CMakeLists|Makefile|build)" | head -20Repository: opentibiabr/otclient
Length of output: 637
🏁 Script executed:
# Verify the actual file and code at lines 185-209
cat -n modules/game_imbui/t_imbui.lua | sed -n '185,209p'Repository: opentibiabr/otclient
Length of output: 865
🏁 Script executed:
# Search for all uses of goto in the codebase to see if this is a pattern
rg "goto|::.*::" --multiline-dotallRepository: opentibiabr/otclient
Length of output: 50377
Remove goto — incompatible with Lua 5.1/LuaJIT.
Line 188 uses goto which is a syntax error in Lua 5.1/LuaJIT (goto was introduced in Lua 5.2). The client uses LuaJIT, so this module will fail to load. Refactor using nested conditionals instead.
🔧 Suggested change (no `goto`)
function Imbuement:toggleMenu(menu)
for key, value in pairs(self) do
- if type(value) ~= 'userdata' or key == 'window' then
- goto continue
- end
-
- if key == menu then
- value:show()
- -- Ajustar tamanho da janela baseado no menu
- if menu == 'selectItemOrScroll' then
- self.window:setHeight(388)
- elseif menu == 'scrollImbue' then
- self.window:setHeight(655)
- elseif menu == 'selectImbue' then
- self.window:setHeight(528)
- elseif menu == 'clearImbue' then
- self.window:setHeight(502)
- end
- else
- value:hide()
- end
-
- ::continue::
+ if type(value) == 'userdata' and key ~= 'window' then
+ if key == menu then
+ value:show()
+ -- Ajustar tamanho da janela baseado no menu
+ if menu == 'selectItemOrScroll' then
+ self.window:setHeight(388)
+ elseif menu == 'scrollImbue' then
+ self.window:setHeight(655)
+ elseif menu == 'selectImbue' then
+ self.window:setHeight(528)
+ elseif menu == 'clearImbue' then
+ self.window:setHeight(502)
+ end
+ else
+ value:hide()
+ end
+ end
end
end🤖 Prompt for AI Agents
In `@modules/game_imbui/t_imbui.lua` around lines 185 - 209, The toggleMenu
function uses a Lua 5.2-only goto (label ::continue::) which breaks under
LuaJIT; refactor Imbuement:toggleMenu to remove the goto by inverting the
condition and using nested conditionals: iterate pairs(self), skip non-userdata
or key == 'window' with an if check (e.g., if type(value) == 'userdata' and key
~= 'window' then ... end), then inside handle the key == menu branch to show and
adjust self.window:setHeight(...) for the four menu cases and an else branch to
hide the value; ensure the loop simply continues naturally without any goto or
labels.
Comment on lines
161
to
175
| function setProtection(value) | ||
| protection = value | ||
| if protection then | ||
| emptyImbue.cost:setText(selectedImbue['cost'] + selectedImbue['protectionCost']) | ||
| emptyImbue.cost:setText(comma_value(selectedImbue['cost'] + selectedImbue['protectionCost'])) | ||
| emptyImbue.successRate:setText('100%') | ||
| emptyImbue.successRate:setColor('green') | ||
| protectionBtn:setImageClip(torect('66 0 66 66')) | ||
|
|
||
| -- Verificar se há gold suficiente para o custo total com proteção | ||
| if (bankGold + inventoryGold) < (selectedImbue['cost'] + selectedImbue['protectionCost']) then | ||
| emptyImbue.cost:setColor('red') | ||
| else | ||
| emptyImbue.cost:setColor('white') | ||
| end | ||
| else |
There was a problem hiding this comment.
Guard selectedImbue when enabling protection.
If the protection toggle is used before an imbuement is selected, selectedImbue[...] will error.
🐛 Proposed fix
function setProtection(value)
protection = value
if protection then
+ if not selectedImbue then return end
emptyImbue.cost:setText(comma_value(selectedImbue['cost'] + selectedImbue['protectionCost']))
emptyImbue.successRate:setText('100%')📝 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.
Suggested change
| function setProtection(value) | |
| protection = value | |
| if protection then | |
| emptyImbue.cost:setText(selectedImbue['cost'] + selectedImbue['protectionCost']) | |
| emptyImbue.cost:setText(comma_value(selectedImbue['cost'] + selectedImbue['protectionCost'])) | |
| emptyImbue.successRate:setText('100%') | |
| emptyImbue.successRate:setColor('green') | |
| protectionBtn:setImageClip(torect('66 0 66 66')) | |
| -- Verificar se há gold suficiente para o custo total com proteção | |
| if (bankGold + inventoryGold) < (selectedImbue['cost'] + selectedImbue['protectionCost']) then | |
| emptyImbue.cost:setColor('red') | |
| else | |
| emptyImbue.cost:setColor('white') | |
| end | |
| else | |
| function setProtection(value) | |
| protection = value | |
| if protection then | |
| if not selectedImbue then return end | |
| emptyImbue.cost:setText(comma_value(selectedImbue['cost'] + selectedImbue['protectionCost'])) | |
| emptyImbue.successRate:setText('100%') | |
| emptyImbue.successRate:setColor('green') | |
| protectionBtn:setImageClip(torect('66 0 66 66')) | |
| -- Verificar se há gold suficiente para o custo total com proteção | |
| if (bankGold + inventoryGold) < (selectedImbue['cost'] + selectedImbue['protectionCost']) then | |
| emptyImbue.cost:setColor('red') | |
| else | |
| emptyImbue.cost:setColor('white') | |
| end | |
| else |
🤖 Prompt for AI Agents
In `@modules/game_imbuing/imbuing.lua` around lines 161 - 175, The setProtection
function accesses selectedImbue['cost'] and ['protectionCost'] without ensuring
selectedImbue is defined; guard against nil by checking selectedImbue at the top
of the protection-true branch (or early-return if protection cannot be enabled)
before using its fields, and only update emptyImbue.cost,
emptyImbue.successRate, emptyImbue.cost:setColor and protectionBtn:setImageClip
when selectedImbue is non-nil; reference the setProtection function,
selectedImbue, emptyImbue and protectionBtn to locate and apply the guard and
handle the insufficient-gold check (bankGold + inventoryGold) safely.
Comment on lines
+5746
to
+5832
| if (g_game.getClientVersion() >= 1510) { | ||
| const uint8_t windowType = msg->getU8(); | ||
|
|
||
| // 0 = CHOICE (select item or scroll) | ||
| if (windowType == 0) { | ||
| msg->getU8(); // unknown | ||
| msg->getU16(); // padding | ||
| msg->getU32(); // padding | ||
| g_lua.callGlobalField("g_game", "onOpenImbuementWindow"); | ||
| return; | ||
| } | ||
|
|
||
| // 1 = SELECT_ITEM | ||
| if (windowType == 1) { | ||
| msg->getU8(); // unknown | ||
|
|
||
| const uint16_t itemId = msg->getU16(); | ||
| const auto& item = Item::create(itemId); | ||
| if (!item || item->getId() == 0) { | ||
| throw Exception("ProtocolGame::parseImbuementWindow: unable to create item with invalid id {}", itemId); | ||
| } | ||
|
|
||
| uint8_t tier = 0; | ||
| if (item->getClassification() > 0) { | ||
| tier = msg->getU8(); | ||
| } | ||
|
|
||
| const uint8_t slots = msg->getU8(); | ||
| std::unordered_map<int, std::tuple<Imbuement, uint32_t, uint32_t>> activeSlots; | ||
| for (auto i = 0; i < slots; i++) { | ||
| const uint8_t firstByte = msg->getU8(); | ||
| if (firstByte == 0x01) { | ||
| Imbuement imbuement = getImbuementInfo(msg); | ||
| const uint32_t duration = msg->getU32(); | ||
| const uint32_t removalCost = msg->getU32(); | ||
| activeSlots[i] = std::make_tuple(imbuement, duration, removalCost); | ||
| } | ||
| } | ||
|
|
||
| const uint16_t imbuementsSize = msg->getU16(); | ||
| std::vector<Imbuement> imbuements; | ||
| imbuements.reserve(imbuementsSize); | ||
| for (auto i = 0; i < imbuementsSize; ++i) { | ||
| imbuements.push_back(getImbuementInfo(msg)); | ||
| } | ||
|
|
||
| const uint32_t neededItemsListCount = msg->getU32(); | ||
| std::vector<ItemPtr> neededItemsList; | ||
| neededItemsList.reserve(neededItemsListCount); | ||
| for (uint32_t i = 0; i < neededItemsListCount; ++i) { | ||
| const uint16_t needItemId = msg->getU16(); | ||
| const uint16_t count = msg->getU16(); | ||
| const auto& needItem = Item::create(needItemId); | ||
| needItem->setCount(count); | ||
| neededItemsList.push_back(needItem); | ||
| } | ||
| g_lua.callGlobalField("g_game", "onImbuementItem", itemId, tier, slots, activeSlots, imbuements, neededItemsList); | ||
| return; | ||
| } | ||
|
|
||
| // 2 = SCROLL | ||
| if (windowType == 2) { | ||
| msg->getU8(); // unknown | ||
| msg->getU8(); // unknown | ||
| msg->getU8(); // unknown | ||
|
|
||
| const uint16_t imbuementsSize = msg->getU16(); | ||
| std::vector<Imbuement> imbuements; | ||
| imbuements.reserve(imbuementsSize); | ||
| for (auto i = 0; i < imbuementsSize; ++i) { | ||
| imbuements.push_back(getImbuementInfo(msg)); | ||
| } | ||
|
|
||
| const uint32_t neededItemsListCount = msg->getU32(); | ||
| std::vector<ItemPtr> neededItemsList; | ||
| neededItemsList.reserve(neededItemsListCount); | ||
| for (uint32_t i = 0; i < neededItemsListCount; ++i) { | ||
| const uint16_t needItemId = msg->getU16(); | ||
| const uint16_t count = msg->getU16(); | ||
| const auto& needItem = Item::create(needItemId); | ||
| needItem->setCount(count); | ||
| neededItemsList.push_back(needItem); | ||
| } | ||
| g_lua.callGlobalField("g_game", "onImbuementScroll", imbuements, neededItemsList); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard unknown imbuement window types to avoid parser desync.
For 1510+, an unexpected windowType will fall through to the legacy parser after consuming one byte, which can corrupt the stream and disconnect. Add a default branch that logs and returns (or skips) before the legacy path executes.
🔧 Suggested fix (add a default guard)
// 2 = SCROLL
if (windowType == 2) {
msg->getU8(); // unknown
msg->getU8(); // unknown
msg->getU8(); // unknown
@@
g_lua.callGlobalField("g_game", "onImbuementScroll", imbuements, neededItemsList);
return;
}
+ g_logger.warning("ProtocolGame::parseImbuementWindow: unknown window type {}", windowType);
+ return;
}🤖 Prompt for AI Agents
In `@src/client/protocolgameparse.cpp` around lines 5746 - 5832, The code in
ProtocolGame::parseImbuementWindow reads a windowType for client versions >=
1510 but only handles values 0,1,2 and will fall through into the legacy parser
on any other value; add a default guard immediately after reading const uint8_t
windowType = msg->getU8() that detects when windowType is not 0,1,2, logs a
warning (using the project's logger) including the unexpected windowType and
returns early to avoid corrupting the message stream and desync with the legacy
parsing path.
majestyotbr
changed the title
59 tasks
|
At least rename the Lua files |
matzinhozz
added a commit
to matzinhozz/otclient
that referenced
this pull request
Jan 27, 2026
- Integra sistema de imbuement para protocolo 15+ - Adiciona novos componentes de progress bar (UIProgressBarSD, UIProgressBarSDInverted) - Adiciona módulo game_imbui com classes para seleção de item/scroll - Adiciona métodos selectImbuementItem e selectImbuementScroll em Game - Resolve conflitos mantendo funcionalidades existentes (Wheel of Destiny, Weapon Proficiency) - Mantém lógica de updateBackground do HEAD para esconder imagem quando width == 1
matzinhozz
added a commit
to matzinhozz/otclient
that referenced
this pull request
Feb 1, 2026
Collaborator
|
Adding this to my TODO list this week. Sorry for the delay in testing. Expect modifications. |
matzinhozz
added a commit
to matzinhozz/otclient
that referenced
this pull request
Mar 16, 2026
Made-with: Cursor
|
This PR is stale because it has been open 45 days with no activity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Nessa pr foi adicionado sistema de imbui para o protocolo 15+. Imbue scroll funcionando 100%
Eu tambem deixei o imbue do cliente 14 com a aparência semelhante ao cliente oficial
Behavior
Actual
Imagens do antigo cliente 14
Expected
Modificação para versão 14
imbue.14.after.mp4
Modificação para versão 15
imbue.15.mp4
Type of change
How Has This Been Tested
o arquivo protocolgameparse eu fiz bem "porco" apenas para o sistema da versao 15 funcionar, então, para os programadores de plantão que poderem ajudar a melhora-lo, fiquem a vontade
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.