feat: complete Cyclopedia Houses integration and improve Character/Magical Archives UX persistence#1676
Open
Juanzitooh wants to merge 17 commits intoopentibiabr:mainfrom
Open
feat: complete Cyclopedia Houses integration and improve Character/Magical Archives UX persistence#1676Juanzitooh wants to merge 17 commits intoopentibiabr:mainfrom
Juanzitooh wants to merge 17 commits intoopentibiabr:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive state persistence for the game cyclopedia UI system, managing session-level window selection and per-tab UI state caching. It significantly expands the house management module with new parsing callbacks, filtering/sorting capabilities, and minimap preview functionality. The character tab gains a new titles browser with filtering. Multiple tabs (bestiary, bosstiary, items, charms, map, magical archives) receive state persistence for searches, filters, and selections. The magical archives module is substantially rebuilt with spell catalog, filtering, and interactive UI. Game protocol infrastructure is extended with new C++ request methods and Lua bindings for character titles and house auction operations. Changes
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/client/protocolgameparse.cpp (1)
4556-4652:⚠️ Potential issue | 🟠 MajorThe new graceful-fail path stops at the 6-byte house header.
After the header check, each state branch still does unchecked
getString()/getU32()/getU64()reads. A short or partially rolled-outAVAILABLE/TRANSFER/MOVEOUTentry will still throw to the outerparseMessagecatch and drop the rest of the packet instead of takingfailParse().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/protocolgameparse.cpp` around lines 4556 - 4652, The header-only unread-size check lets later msg->getString()/getU32()/getU64() calls in the switch branches throw; update the parsing in parseMessage (the loop handling houses in protocolgameparse.cpp) so each branch (Otc::CYCLOPEDIA_HOUSE_STATE_AVAILABLE, _RENTED, _TRANSFER, _MOVEOUT) verifies msg->getUnreadSize() is large enough before every multi-byte read (strings, u32, u64) and calls failParse(...) then returns on insufficient bytes; specifically guard reads around bidName->getString()/getU32()/getU64(), ownerName/paidUntil/transferTime/transferValue reads, and optional reads controlled by isBidder/isOwner/isNewOwner to prevent exceptions from propagating to parseMessage.modules/game_cyclopedia/tab/bosstiary/bosstiary.lua (1)
426-460:⚠️ Potential issue | 🟠 MajorLive filtering no longer matches the restored filtering rules.
BosstiarySearchText()recomputes visibility from search text only, whilechangeBosstiaryFilter()recomputes from a single checkbox only. After this PR, reopen usesapplyBosstiaryState()and combines both dimensions, so the list a user sees live can change after reopening the tab.Also applies to: 463-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_cyclopedia/tab/bosstiary/bosstiary.lua` around lines 426 - 460, BosstiarySearchText currently sets creature.visible based only on search text and unlocked state, which diverges from the combined filtering logic used by applyBosstiaryState/changeBosstiaryFilter and causes restored views to differ; update BosstiarySearchText to reuse the same combined visibility computation (either call applyBosstiaryState or extract the shared visibility function) so that visibility considers both the search text and the current filter/checkbox state before calling ReadjustPages() and saveBosstiaryViewState(getBosstiaryCurrentState()).modules/game_cyclopedia/tab/house/house.lua (1)
1762-1795:⚠️ Potential issue | 🟠 MajorReapply the current state filter when toggling Houses/Guildhalls.
This function persists
filterMode, but then rewritesdata.visibleusing onlydata.gh. If the state dropdown is already on "Auctioned" or "Rented", the list immediately falls out of sync with the selected state until the user changes the dropdown again.modules/game_cyclopedia/tab/character/character.lua (1)
723-733:⚠️ Potential issue | 🟠 MajorAvoid persisting transient default list-mode changes.
Cyclopedia.characterItemListFilter()is also used by the Item Summary entry path to force the initial"list"layout. SavingitemsListModeunconditionally here overwrites the user's saved grid preference as soon as the page is opened, so the restore block at Lines 505-509 never sees the previous mode again.
🧹 Nitpick comments (4)
tools/generate_cyclopedia_houses_lua.py (3)
112-116: Replace developer-specific default path with a portable alternative.The hardcoded path
/home/jp/Documentos/github/canary/...is developer-specific and will not work for other contributors. Consider using a relative path or requiring the--xmlargument explicitly.♻️ Proposed fix for portable default path
parser.add_argument( "--xml", - default="/home/jp/Documentos/github/canary/data-otservbr-global/world/otservbr-house.xml", + required=True, help="Path to canary houses XML file.", )Alternatively, use a path relative to the repository root if there's a standard location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/generate_cyclopedia_houses_lua.py` around lines 112 - 116, The default value for the "--xml" argparse option is a developer-specific absolute path; update the parser.add_argument call for "--xml" in tools/generate_cyclopedia_houses_lua.py (the parser.add_argument("--xml", ...) invocation) to remove the hardcoded path—either set a portable relative default (e.g., a repo-relative file like "data/world/otservbr-house.xml") or make the argument required by removing the default and adding required=True so contributors must supply the path; ensure the help text reflects the new behavior.
47-50: Usedefusedxmlto mitigate XML attack vectors.As flagged by static analysis, using
xml.etree.ElementTreeto parse untrusted XML is vulnerable to XXE (XML External Entity) and entity expansion attacks. While this script processes local server data files, usingdefusedxmlis a low-cost safeguard.🛡️ Proposed fix using defusedxml
-import xml.etree.ElementTree as ET +import defusedxml.ElementTree as ETThen add
defusedxmlto your project's development dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/generate_cyclopedia_houses_lua.py` around lines 47 - 50, Replace usage of xml.etree.ElementTree in the generate function with defusedxml's safe parser: import defusedxml.ElementTree as ET (or use defusedxml.ElementTree.parse) and update the call in generate(xml_path) that currently does ET.parse(xml_path) to use the defusedxml parser, keeping the existing root.tag check; also add defusedxml to the project's development dependencies so the safe library is installed.
38-39: Consider escaping additional characters inlua_quote.The current implementation only escapes backslashes and double quotes. House names containing newlines (
\n), tabs (\t), or other control characters would produce invalid Lua syntax.♻️ Proposed fix for more robust Lua string escaping
def lua_quote(value: str) -> str: - return value.replace("\\", "\\\\").replace('"', '\\"') + value = value.replace("\\", "\\\\") + value = value.replace('"', '\\"') + value = value.replace("\n", "\\n") + value = value.replace("\r", "\\r") + value = value.replace("\t", "\\t") + return value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/generate_cyclopedia_houses_lua.py` around lines 38 - 39, The lua_quote function currently only escapes backslashes and double quotes, which breaks Lua strings containing control characters; update lua_quote to also replace control characters (at minimum \n, \r, \t, \f, \v) with their escaped sequences and escape any other non-printable characters (e.g., by using hex escapes) so generated strings produce valid Lua syntax; modify the implementation of lua_quote to perform these additional replacements (or canonicalize via a safe serializer) while preserving existing backslash and quote escapes.modules/game_cyclopedia/game_cyclopedia.lua (1)
58-77: Nested tab state is replaced wholesale.
getTabState()andsaveTabState()only merge one level deep. If a caller saves a partial nested patch likefilters = { unlocked = true }, sibling fields infiltersdisappear and nested defaults never come back on load. Since this is now the shared tab-state API, a recursive merge would make the contract much safer.♻️ Possible fix
+local function mergeTable(base, patch) + base = type(base) == "table" and copyTable(base) or {} + if type(patch) ~= "table" then + return base + end + + for key, value in pairs(patch) do + if type(value) == "table" then + base[key] = mergeTable(base[key], value) + else + base[key] = value + end + end + + return base +end + function Cyclopedia.getTabState(tabName, defaults) @@ - local base = copyTable(defaults) - if type(savedState) ~= "table" then - return base - end - - for key, value in pairs(savedState) do - if type(value) == "table" then - base[key] = copyTable(value) - else - base[key] = value - end - end - return base + return mergeTable(defaults, savedState) end @@ - for key, value in pairs(statePatch) do - if type(value) == "table" then - current[key] = copyTable(value) - else - current[key] = value - end - end + sessionState.tabStates[tabName] = mergeTable(current, statePatch) endAlso applies to: 80-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_cyclopedia/game_cyclopedia.lua` around lines 58 - 77, getTabState currently only merges savedState one level deep so nested tables (e.g., filters.unlocked) overwrite siblings and defaults; change Cyclopedia.getTabState to perform a deep/recursive merge of savedState into the copied defaults (use a helper like deepMerge(dst, src) that recurses on table values) so table fields are merged rather than replaced, and apply the same recursive-merge logic in the corresponding Cyclopedia.saveTabState routine to ensure symmetric behavior when persisting nested updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/game_cyclopedia/tab/bestiary/bestiary.lua`:
- Around line 57-67: The code always forces Cyclopedia.Bestiary.Stage =
STAGES.CATEGORY on initialization which prevents restoring a previous
STAGES.SEARCH and can leave a stale pendingBestiaryRestoreCategory; update
showBestiary initialization (where getBestiaryViewState() is used) to branch on
viewState.stage and restore either CATEGORY or SEARCH accordingly, when
restoring SEARCH set UI.SearchEdit to viewState.searchText (persist the
searchText in the saved view state) and clear pendingBestiaryRestoreCategory
when entering SEARCH; also modify the save-path (the code around where
STAGES.SEARCH is set on lines ~459-462) to store searchText into the viewState
and explicitly clear selectedCategory when switching to STAGES.SEARCH so
reopening can rebuild the last search instead of reopening a stale category.
In `@modules/game_cyclopedia/tab/bosstiary/bosstiary.lua`:
- Around line 118-129: The range checks for creature.kills overlap because the
upper bounds are inclusive; update the comparisons in bosstiary.lua so bucket
boundaries are exclusive on the upper end: replace the Prowess branch condition
(currently "creature.kills >= CONFIG[creature.category].PROWESS and
creature.kills <= CONFIG[creature.category].EXPERTISE") with a check that the
kills are >= PROWESS and < EXPERTISE, and replace the Expertise branch condition
(currently "...>= CONFIG[...].EXPERTISE and creature.kills <=
CONFIG[...].MASTERY") with kills >= EXPERTISE and < MASTERY, leaving the final
Mastery branch as >= MASTERY; this ensures kills == EXPERTISE goes to the
Expertise bucket and kills == MASTERY reaches Mastery (referencing variables:
creature.kills, CONFIG[category].PROWESS/EXPERTISE/MASTERY, and
filters.ProwessCheck/ExpertiseCheck/MasteryCheck).
In `@modules/game_cyclopedia/tab/character/character.lua`:
- Around line 485-503: The saved search text from getCharacterViewState is being
passed directly into Cyclopedia.characterItemsSearch which ends up using
string.find without the plain-text flag, so patterns like [, %, ^, * will error;
update the search matching in Cyclopedia.characterItemsSearch (the code that
calls string.find(name, text:lower())) to use the plain-text mode
(string.find(name, text:lower(), 1, true)) or otherwise escape the input before
matching so persisted searches are treated as literal text.
In `@modules/game_cyclopedia/tab/house/house.lua`:
- Around line 800-823: In applyHouseVisibilityByViewState(), change the
condition for stateType == 2 so it only marks houses visible when their state
equals CyclopediaHouseStates.Available (instead of using not data.rented which
includes Transfer/MoveOut); update the branch that currently does data.visible =
baseVisible and not data.rented to use data.visible = baseVisible and data.state
== CyclopediaHouseStates.Available, keeping other branches (stateType == 3 and
default) unchanged and referencing Cyclopedia.House.Data, viewState.filterMode,
stateType, data.rented, data.inTransfer, and CyclopediaHouseStates.Available to
locate the code to edit.
- Around line 2282-2293: The UI currently shows a disabled placeholder for
widget.data.movingOut so users cannot undo a scheduled move-out; update this
block to check the parsed flag (canCancelMoveOut from loadHouseList) and, when
true, render an enabled "Cancel Move Out" button (keep id "moveOutButton") with
an onClick handler that dispatches the existing CancelMoveOut auction action
handled by the auction message table (reuse the same message/API used elsewhere
to cancel move-outs); when canCancelMoveOut is false keep the disabled grey
"Move Out Pending" state. Ensure you reference widget.data.movingOut,
canCancelMoveOut, the moveOutButton id, and the CancelMoveOut handler when
wiring this change.
- Around line 764-798: The applyHouseSortByType function is missing sortType ==
3 and misorders types 4 and 5; add a branch for sortType == 3 that sorts
Cyclopedia.House.Data by the numeric rent field (ascending or the intended
direction) and implement sortType == 4 to sort by current/highest bid value (use
hasBid and the numeric bidValue or highestBid field, treating missing bids as
zero and breaking ties by name), and implement sortType == 5 to sort by auction
end time (use an auctionEnd or auction_end timestamp field, treating
missing/never-ending auctions as far future/last and breaking ties by name);
update comparisons in applyHouseSortByType (referencing Cyclopedia.House.Data
and the function applyHouseSortByType) to handle nils safely and ensure
deterministic ordering.
- Around line 1889-1914: The reloadHouseList path doesn't handle the case where
Cyclopedia.House.Data exists but visibleEntries == 0, so the old
selection/preview/buttons remain; update the reloadHouseList logic to explicitly
detect visibleEntries == 0 and treat it like the empty-list branch: clear
Cyclopedia.House.lastSelectedHouse, clear any preview/action UI state, ensure no
house is selected (use selectHouseWithoutPersist(nil) or the same clearing
routine used for empty lists) before proceeding, referencing
UI.ListBase.AuctionList, Cyclopedia.House.ViewState.selectedHouseId,
Cyclopedia.House.lastSelectedHouse and selectHouseWithoutPersist to locate where
to add this branch.
In `@modules/game_cyclopedia/tab/items/items.lua`:
- Around line 717-745: Restoring the saved category calls child:onClick(), which
triggers Cyclopedia.selectItemCategory() and resets item filters, so the saved
vocation/level/hand/classification get overwritten; to fix, capture the current
Cyclopedia.Items filter state (LevelFilter, VocFilter, h1Filter/h2Filter,
ClassificationFilter) before invoking child:onClick(), then after the onClick()
completes reapply those captured values to the UI (use
UI.LevelButton:setChecked, UI.VocationButton:setChecked,
UI.H1Button/H2Button:setChecked and UI.ItemFilter:setOption) so the persisted
filters survive the category restore; apply the same capture-and-reapply pattern
to the code path around Cyclopedia.selectItemCategory() (lines 1113-1123) to
prevent its defaults from clobbering saved filters.
In `@modules/game_cyclopedia/tab/magicalArchives/magicalArchives.lua`:
- Around line 190-200: The saved auto-aim state must be loaded before any code
that can trigger an initial spell selection; move or call loadAimAtTargetData()
so it runs before loadSpellsData() and setupSearchUI() (both of which can focus
a spell and call selectSpellDetails()), ensuring selectSpellDetails() sees the
persisted auto-aim value when rendering directional spell checkboxes.
- Around line 1211-1225: The cooldown fields are read from non-existent
spell.cooldown/groupCooldown; instead use the schema fields spell.exhaustion for
the rune cooldown and spell.group (a map) for group cooldown. Replace the
fallback reads in the rune UI block so runeCooldownValue uses spell.exhaustion
(or default 2000) and runeGroupCooldownValue uses spell.group[spell.id] (or
spell.exhaustion or 2000 if that lookup is nil); keep the same math.floor and
setText logic. Reference: recursiveGetChildById, runeCooldownValue,
runeGroupCooldownValue, spell.exhaustion, and spell.group[spell.id].
- Around line 283-293: assignSpellToActionBar currently passes currentSpell into
modules.game_actionbar.assignSpell but the action-bar flow in
ActionAssignmentWindows.lua expects an action button and immediately calls
button:getParent(); update assignSpellToActionBar to invoke the action-bar flow
with a real action-slot/button object (not the spell), e.g. retrieve the
currently selected action button from the actionbar (use or add a getter like
modules.game_actionbar.getSelectedButton()) and pass that button to
modules.game_actionbar.assignSpell, or alternatively add a new API on
modules.game_actionbar that accepts a spell (e.g. assignSpellToSlot(spell,
slot)) and call that from assignSpellToActionBar so the action-window code
receives the expected button and button:getParent() will succeed.
- Around line 843-860: searchSpells currently rebuilds filteredSpells from
allSpells and uses pattern matching, discarding existing
vocation/level/group/learnt filters and mis-handling magic chars; change it to
either iterate the already-filtered list (filteredSpells or a copy) or, if you
must iterate allSpells, apply the same predicate functions (passesVocation,
passesLevel, passesGroup, passesLearnt or whatever filter helpers are used by
applyAllFilters) before inserting, and switch string matching to plain mode (use
string.find(..., 1, true) or :find(searchText, 1, true)) for both spell.name and
spell.words; keep calling updateSpellListUI() afterward.
In `@modules/gamelib/game.lua`:
- Around line 165-178: In g_game.requestSelectCharacterTitle, allow titleId == 0
to clear the selection by changing the validation to reject only negative
values: replace the check "if titleId <= 0" with "if titleId < 0" (after
normalizing with normalizePositiveNumber), keeping the subsequent client version
guard (g_game.getClientVersion) and the call to
g_game.requestSetCharacterTitle(titleId) as-is so the 0 sentinel is passed
through to requestSetCharacterTitle.
In `@modules/gamelib/spells.lua`:
- Around line 378-397: The clientId sentinel 0 is treated as a valid icon
because Spells.getSpellIconId checks "spellData.clientId >= 0", preventing
fallback to spellData.id; change the clientId check in Spells.getSpellIconId to
require a positive value (e.g., type(spellData.clientId) == "number" and
spellData.clientId > 0) so clientId == 0 is treated as missing and the function
will fall back to checking spellData.id (keep the existing spellData.id check
as-is).
- Around line 324-342: The fast-path uses the original input when indexing
SpellInfo even though getSpellProfileByName() normalizes/trims the name; fix
getSpellByName to trim/normalize the name once and reuse it for both
getSpellProfileByName and the SpellInfo lookup (e.g. compute a local
normalizedName = <trim name> or call the same normalizer, then call
Spells.getSpellProfileByName(normalizedName) and index
SpellInfo[profile][normalizedName] instead of SpellInfo[profile][name]); update
the code in function Spells.getSpellByName to use normalizedName for the profile
lookup and the direct SpellInfo fast-path.
---
Outside diff comments:
In `@modules/game_cyclopedia/tab/bosstiary/bosstiary.lua`:
- Around line 426-460: BosstiarySearchText currently sets creature.visible based
only on search text and unlocked state, which diverges from the combined
filtering logic used by applyBosstiaryState/changeBosstiaryFilter and causes
restored views to differ; update BosstiarySearchText to reuse the same combined
visibility computation (either call applyBosstiaryState or extract the shared
visibility function) so that visibility considers both the search text and the
current filter/checkbox state before calling ReadjustPages() and
saveBosstiaryViewState(getBosstiaryCurrentState()).
In `@src/client/protocolgameparse.cpp`:
- Around line 4556-4652: The header-only unread-size check lets later
msg->getString()/getU32()/getU64() calls in the switch branches throw; update
the parsing in parseMessage (the loop handling houses in protocolgameparse.cpp)
so each branch (Otc::CYCLOPEDIA_HOUSE_STATE_AVAILABLE, _RENTED, _TRANSFER,
_MOVEOUT) verifies msg->getUnreadSize() is large enough before every multi-byte
read (strings, u32, u64) and calls failParse(...) then returns on insufficient
bytes; specifically guard reads around bidName->getString()/getU32()/getU64(),
ownerName/paidUntil/transferTime/transferValue reads, and optional reads
controlled by isBidder/isOwner/isNewOwner to prevent exceptions from propagating
to parseMessage.
---
Nitpick comments:
In `@modules/game_cyclopedia/game_cyclopedia.lua`:
- Around line 58-77: getTabState currently only merges savedState one level deep
so nested tables (e.g., filters.unlocked) overwrite siblings and defaults;
change Cyclopedia.getTabState to perform a deep/recursive merge of savedState
into the copied defaults (use a helper like deepMerge(dst, src) that recurses on
table values) so table fields are merged rather than replaced, and apply the
same recursive-merge logic in the corresponding Cyclopedia.saveTabState routine
to ensure symmetric behavior when persisting nested updates.
In `@tools/generate_cyclopedia_houses_lua.py`:
- Around line 112-116: The default value for the "--xml" argparse option is a
developer-specific absolute path; update the parser.add_argument call for
"--xml" in tools/generate_cyclopedia_houses_lua.py (the
parser.add_argument("--xml", ...) invocation) to remove the hardcoded
path—either set a portable relative default (e.g., a repo-relative file like
"data/world/otservbr-house.xml") or make the argument required by removing the
default and adding required=True so contributors must supply the path; ensure
the help text reflects the new behavior.
- Around line 47-50: Replace usage of xml.etree.ElementTree in the generate
function with defusedxml's safe parser: import defusedxml.ElementTree as ET (or
use defusedxml.ElementTree.parse) and update the call in generate(xml_path) that
currently does ET.parse(xml_path) to use the defusedxml parser, keeping the
existing root.tag check; also add defusedxml to the project's development
dependencies so the safe library is installed.
- Around line 38-39: The lua_quote function currently only escapes backslashes
and double quotes, which breaks Lua strings containing control characters;
update lua_quote to also replace control characters (at minimum \n, \r, \t, \f,
\v) with their escaped sequences and escape any other non-printable characters
(e.g., by using hex escapes) so generated strings produce valid Lua syntax;
modify the implementation of lua_quote to perform these additional replacements
(or canonicalize via a safe serializer) while preserving existing backslash and
quote escapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27c071c9-c2dd-40c8-83ef-5c2d1c71e0ca
⛔ Files ignored due to path filters (1)
data/images/game/cyclopedia/ui/archive-menu.pngis excluded by!**/*.png
📒 Files selected for processing (26)
modules/game_cyclopedia/game_cyclopedia.luamodules/game_cyclopedia/game_cyclopedia.otmodmodules/game_cyclopedia/houses_data.luamodules/game_cyclopedia/tab/bestiary/bestiary.luamodules/game_cyclopedia/tab/bosstiary/bosstiary.luamodules/game_cyclopedia/tab/character/character.luamodules/game_cyclopedia/tab/character/character.otuimodules/game_cyclopedia/tab/charms/charms.luamodules/game_cyclopedia/tab/house/house.luamodules/game_cyclopedia/tab/items/items.luamodules/game_cyclopedia/tab/magicalArchives/magicalArchives.luamodules/game_cyclopedia/tab/magicalArchives/magicalArchives.otuimodules/game_cyclopedia/tab/map/map.luamodules/game_cyclopedia/tab/map/map.otuimodules/game_cyclopedia/utils.luamodules/gamelib/const.luamodules/gamelib/game.luamodules/gamelib/spells.luasrc/client/game.cppsrc/client/game.hsrc/client/luafunctions.cppsrc/client/protocolcodes.hsrc/client/protocolgame.hsrc/client/protocolgameparse.cppsrc/client/protocolgamesend.cpptools/generate_cyclopedia_houses_lua.py
💤 Files with no reviewable changes (1)
- modules/game_cyclopedia/utils.lua
Comment on lines
+57
to
67
| local viewState = getBestiaryViewState() | ||
|
|
||
| UI.ListBase.CategoryList:setVisible(true) | ||
| UI.ListBase.CreatureList:setVisible(false) | ||
| UI.ListBase.CreatureInfo:setVisible(false) | ||
|
|
||
| Cyclopedia.Bestiary.Stage = STAGES.CATEGORY | ||
| pendingBestiaryRestoreCategory = viewState.selectedCategory | ||
| UI.SearchEdit:setText(viewState.searchText or "") | ||
| Cyclopedia.BestiarySearchText(UI.SearchEdit:getText() or "") | ||
| controllerCyclopedia.ui.CharmsBase:setVisible(true) |
There was a problem hiding this comment.
viewState.stage is saved here, but it isn't actually restorable.
Line 63 always forces STAGES.CATEGORY, while Lines 459-462 save STAGES.SEARCH after blanking the query and leaving any previously saved selectedCategory untouched. Reopening after a search therefore cannot rebuild the last search view and can even auto-open a stale category instead.
Persist the data needed to rerun the search, clear selectedCategory when entering search, and branch on viewState.stage during showBestiary() initialization.
Also applies to: 459-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_cyclopedia/tab/bestiary/bestiary.lua` around lines 57 - 67, The
code always forces Cyclopedia.Bestiary.Stage = STAGES.CATEGORY on initialization
which prevents restoring a previous STAGES.SEARCH and can leave a stale
pendingBestiaryRestoreCategory; update showBestiary initialization (where
getBestiaryViewState() is used) to branch on viewState.stage and restore either
CATEGORY or SEARCH accordingly, when restoring SEARCH set UI.SearchEdit to
viewState.searchText (persist the searchText in the saved view state) and clear
pendingBestiaryRestoreCategory when entering SEARCH; also modify the save-path
(the code around where STAGES.SEARCH is set on lines ~459-462) to store
searchText into the viewState and explicitly clear selectedCategory when
switching to STAGES.SEARCH so reopening can rebuild the last search instead of
reopening a stale category.
Comment on lines
+118
to
+129
| if visible then | ||
| if creature.kills < 1 then | ||
| visible = filters.NoKillsCheck ~= false | ||
| elseif creature.kills < CONFIG[creature.category].PROWESS then | ||
| visible = filters.FewKillsCheck ~= false | ||
| elseif creature.kills >= CONFIG[creature.category].PROWESS and creature.kills <= CONFIG[creature.category].EXPERTISE then | ||
| visible = filters.ProwessCheck ~= false | ||
| elseif creature.kills >= CONFIG[creature.category].EXPERTISE and creature.kills <= CONFIG[creature.category].MASTERY then | ||
| visible = filters.ExpertiseCheck ~= false | ||
| elseif creature.kills >= CONFIG[creature.category].MASTERY then | ||
| visible = filters.MasteryCheck ~= false | ||
| end |
There was a problem hiding this comment.
Exact Expertise/Mastery thresholds land in the previous bucket.
At Line 123 and Line 125 the upper bounds are inclusive, so kills == EXPERTISE still uses ProwessCheck, and kills == MASTERY never reaches MasteryCheck.
🐛 Suggested boundary fix
- elseif creature.kills >= CONFIG[creature.category].PROWESS and creature.kills <= CONFIG[creature.category].EXPERTISE then
+ elseif creature.kills >= CONFIG[creature.category].PROWESS and creature.kills < CONFIG[creature.category].EXPERTISE then
visible = filters.ProwessCheck ~= false
- elseif creature.kills >= CONFIG[creature.category].EXPERTISE and creature.kills <= CONFIG[creature.category].MASTERY then
+ elseif creature.kills >= CONFIG[creature.category].EXPERTISE and creature.kills < CONFIG[creature.category].MASTERY then
visible = filters.ExpertiseCheck ~= false
elseif creature.kills >= CONFIG[creature.category].MASTERY then
visible = filters.MasteryCheck ~= false
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.
Suggested change
| if visible then | |
| if creature.kills < 1 then | |
| visible = filters.NoKillsCheck ~= false | |
| elseif creature.kills < CONFIG[creature.category].PROWESS then | |
| visible = filters.FewKillsCheck ~= false | |
| elseif creature.kills >= CONFIG[creature.category].PROWESS and creature.kills <= CONFIG[creature.category].EXPERTISE then | |
| visible = filters.ProwessCheck ~= false | |
| elseif creature.kills >= CONFIG[creature.category].EXPERTISE and creature.kills <= CONFIG[creature.category].MASTERY then | |
| visible = filters.ExpertiseCheck ~= false | |
| elseif creature.kills >= CONFIG[creature.category].MASTERY then | |
| visible = filters.MasteryCheck ~= false | |
| end | |
| if visible then | |
| if creature.kills < 1 then | |
| visible = filters.NoKillsCheck ~= false | |
| elseif creature.kills < CONFIG[creature.category].PROWESS then | |
| visible = filters.FewKillsCheck ~= false | |
| elseif creature.kills >= CONFIG[creature.category].PROWESS and creature.kills < CONFIG[creature.category].EXPERTISE then | |
| visible = filters.ProwessCheck ~= false | |
| elseif creature.kills >= CONFIG[creature.category].EXPERTISE and creature.kills < CONFIG[creature.category].MASTERY then | |
| visible = filters.ExpertiseCheck ~= false | |
| elseif creature.kills >= CONFIG[creature.category].MASTERY then | |
| visible = filters.MasteryCheck ~= false | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_cyclopedia/tab/bosstiary/bosstiary.lua` around lines 118 - 129,
The range checks for creature.kills overlap because the upper bounds are
inclusive; update the comparisons in bosstiary.lua so bucket boundaries are
exclusive on the upper end: replace the Prowess branch condition (currently
"creature.kills >= CONFIG[creature.category].PROWESS and creature.kills <=
CONFIG[creature.category].EXPERTISE") with a check that the kills are >= PROWESS
and < EXPERTISE, and replace the Expertise branch condition (currently "...>=
CONFIG[...].EXPERTISE and creature.kills <= CONFIG[...].MASTERY") with kills >=
EXPERTISE and < MASTERY, leaving the final Mastery branch as >= MASTERY; this
ensures kills == EXPERTISE goes to the Expertise bucket and kills == MASTERY
reaches Mastery (referencing variables: creature.kills,
CONFIG[category].PROWESS/EXPERTISE/MASTERY, and
filters.ProwessCheck/ExpertiseCheck/MasteryCheck).
Comment on lines
+485
to
+503
| local viewState = getCharacterViewState() | ||
| local savedFilters = viewState.itemsFilters or {} | ||
| local hasSavedFilters = false | ||
|
|
||
| for filterId, enabled in pairs(savedFilters) do | ||
| local filterWidget = UI.CharacterItems.filters[filterId] | ||
| if filterWidget and enabled == true then | ||
| filterWidget:setChecked(true) | ||
| hasSavedFilters = true | ||
| elseif filterWidget and enabled == false then | ||
| filterWidget:setChecked(false) | ||
| end | ||
| end | ||
|
|
||
| if not hasSavedFilters and UI.CharacterItems.filters.inventory then | ||
| UI.CharacterItems.filters.inventory:setChecked(true) | ||
| end | ||
|
|
||
| Cyclopedia.characterItemsSearch(viewState.itemsSearchText or "") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /dev/shm/repo && find . -name "*.lua" -path "*/game_cyclopedia/*" | head -20Repository: opentibiabr/otclient
Length of output: 127
🏁 Script executed:
cd /dev/shm/repo && find . -name "character.lua" | grep -i cyclopediaRepository: opentibiabr/otclient
Length of output: 127
🏁 Script executed:
cd /dev/shm/repo && rg "characterItemsSearch" --type lua -A 10 -B 2 | head -100Repository: opentibiabr/otclient
Length of output: 127
🏁 Script executed:
cd /dev/shm/repo && rg "loadCharacterItems" --type lua -A 20 | head -150Repository: opentibiabr/otclient
Length of output: 127
🏁 Script executed:
find . -name "character.lua" | grep -i cyclopediaRepository: opentibiabr/otclient
Length of output: 117
🏁 Script executed:
rg "characterItemsSearch" --type lua -A 10 -B 2Repository: opentibiabr/otclient
Length of output: 2152
🏁 Script executed:
rg "loadCharacterItems" --type lua -A 20 | head -150Repository: opentibiabr/otclient
Length of output: 7721
🏁 Script executed:
rg "function characterItemsSearch" --type lua -A 30Repository: opentibiabr/otclient
Length of output: 46
🏁 Script executed:
rg "function Cyclopedia.characterItemsSearch" --type lua -A 50Repository: opentibiabr/otclient
Length of output: 4317
🏁 Script executed:
cat -n ./modules/game_cyclopedia/tab/character/character.lua | sed -n '450,550p'Repository: opentibiabr/otclient
Length of output: 4572
🏁 Script executed:
rg "string.find" ./modules/game_cyclopedia/tab/character/character.lua -B 2 -A 2Repository: opentibiabr/otclient
Length of output: 389
🏁 Script executed:
cat -n ./modules/game_cyclopedia/tab/character/character.lua | sed -n '360,380p'Repository: opentibiabr/otclient
Length of output: 768
🏁 Script executed:
rg "string.find.*plain" ./modules/game_cyclopedia/ --type luaRepository: opentibiabr/otclient
Length of output: 46
🏁 Script executed:
rg "saveCharacterViewState\|getCharacterViewState" ./modules/game_cyclopedia/tab/character/character.lua -B 2 -A 2 | head -50Repository: opentibiabr/otclient
Length of output: 46
🏁 Script executed:
rg "itemsSearchText" ./modules/game_cyclopedia/ --type lua -B 3 -A 3Repository: opentibiabr/otclient
Length of output: 1858
🏁 Script executed:
cat -n ./modules/game_cyclopedia/tab/character/character.lua | sed -n '340,365p'Repository: opentibiabr/otclient
Length of output: 1137
Don't replay persisted item searches with Lua pattern matching.
loadCharacterItems() restores arbitrary saved text via Cyclopedia.characterItemsSearch(...) at line 503, but the search function uses string.find(name, text:lower()) without the plain text flag. Special characters like [, %, ^, or * in a saved search query will fail as Lua pattern syntax, breaking the Character Items tab every time it opens until the corrupted state is cleared. Add the plain text flag to string.find() or escape the search input before matching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_cyclopedia/tab/character/character.lua` around lines 485 - 503,
The saved search text from getCharacterViewState is being passed directly into
Cyclopedia.characterItemsSearch which ends up using string.find without the
plain-text flag, so patterns like [, %, ^, * will error; update the search
matching in Cyclopedia.characterItemsSearch (the code that calls
string.find(name, text:lower())) to use the plain-text mode (string.find(name,
text:lower(), 1, true)) or otherwise escape the input before matching so
persisted searches are treated as literal text.
Comment on lines
+764
to
+798
| local function applyHouseSortByType(sortType) | ||
| if not Cyclopedia.House.Data then | ||
| return | ||
| end | ||
|
|
||
| if sortType == 1 then | ||
| table.sort(Cyclopedia.House.Data, function(a, b) | ||
| return a.name < b.name | ||
| end) | ||
| elseif sortType == 2 then | ||
| table.sort(Cyclopedia.House.Data, function(a, b) | ||
| return a.sqm < b.sqm | ||
| end) | ||
| elseif sortType == 4 then | ||
| table.sort(Cyclopedia.House.Data, function(a, b) | ||
| if a.hasBid and not b.hasBid then | ||
| return true | ||
| elseif not a.hasBid and b.hasBid then | ||
| return false | ||
| else | ||
| return false | ||
| end | ||
| end) | ||
| elseif sortType == 5 then | ||
| table.sort(Cyclopedia.House.Data, function(a, b) | ||
| if a.hasBid and not b.hasBid then | ||
| return true | ||
| elseif not a.hasBid and b.hasBid then | ||
| return false | ||
| else | ||
| return false | ||
| end | ||
| end) | ||
| end | ||
| end |
There was a problem hiding this comment.
The new sort helper doesn't implement three of the five labels correctly.
sortType == 3 ("Sort by rent") is missing entirely, and both sortType == 4 and sortType == 5 only bucket on hasBid instead of ordering by bid value or auction end. Users will get effectively arbitrary ordering for those options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_cyclopedia/tab/house/house.lua` around lines 764 - 798, The
applyHouseSortByType function is missing sortType == 3 and misorders types 4 and
5; add a branch for sortType == 3 that sorts Cyclopedia.House.Data by the
numeric rent field (ascending or the intended direction) and implement sortType
== 4 to sort by current/highest bid value (use hasBid and the numeric bidValue
or highestBid field, treating missing bids as zero and breaking ties by name),
and implement sortType == 5 to sort by auction end time (use an auctionEnd or
auction_end timestamp field, treating missing/never-ending auctions as far
future/last and breaking ties by name); update comparisons in
applyHouseSortByType (referencing Cyclopedia.House.Data and the function
applyHouseSortByType) to handle nils safely and ensure deterministic ordering.
Comment on lines
+800
to
+823
| local function applyHouseVisibilityByViewState() | ||
| if not Cyclopedia.House.Data then | ||
| return | ||
| end | ||
|
|
||
| local viewState = Cyclopedia.House.ViewState or {} | ||
| local onlyGuildHall = viewState.filterMode == "guildhalls" | ||
| local stateType = tonumber(viewState.stateType) or 1 | ||
|
|
||
| for _, data in ipairs(Cyclopedia.House.Data) do | ||
| local baseVisible = false | ||
| if onlyGuildHall then | ||
| baseVisible = data.gh and true or false | ||
| else | ||
| baseVisible = not data.gh | ||
| end | ||
| if stateType == 3 then | ||
| data.visible = baseVisible and (data.rented or data.inTransfer) | ||
| elseif stateType == 2 then | ||
| data.visible = baseVisible and not data.rented | ||
| else | ||
| data.visible = baseVisible | ||
| end | ||
| end |
There was a problem hiding this comment.
"Auctioned" should only match available houses now.
With the new Transfer and MoveOut states, stateType == 2 using not data.rented also includes houses that are being transferred or moved out. Check data.state == CyclopediaHouseStates.Available instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_cyclopedia/tab/house/house.lua` around lines 800 - 823, In
applyHouseVisibilityByViewState(), change the condition for stateType == 2 so it
only marks houses visible when their state equals
CyclopediaHouseStates.Available (instead of using not data.rented which includes
Transfer/MoveOut); update the branch that currently does data.visible =
baseVisible and not data.rented to use data.visible = baseVisible and data.state
== CyclopediaHouseStates.Available, keeping other branches (stateType == 3 and
default) unchanged and referencing Cyclopedia.House.Data, viewState.filterMode,
stateType, data.rented, data.inTransfer, and CyclopediaHouseStates.Available to
locate the code to edit.
Comment on lines
+843
to
+860
| function searchSpells(searchText) | ||
| if not searchText or searchText == '' then | ||
| applyAllFilters() | ||
| return | ||
| end | ||
|
|
||
| searchText = searchText:lower() | ||
| filteredSpells = {} | ||
|
|
||
| for _, spell in ipairs(allSpells) do | ||
| local spellName = spell.name or "" | ||
| local spellWords = spell.words or "" | ||
| if spellName:lower():find(searchText) or spellWords:lower():find(searchText) then | ||
| table.insert(filteredSpells, spell) | ||
| end | ||
| end | ||
|
|
||
| updateSpellListUI() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file to ensure it exists and examine its context
find . -type f -name "magicalArchives.lua"Repository: opentibiabr/otclient
Length of output: 129
🏁 Script executed:
# Look for the searchSpells function and surrounding context to understand the filter pipeline
rg -A 50 "function searchSpells" modules/game_cyclopedia/tab/magicalArchives/ 2>/dev/null | head -100Repository: opentibiabr/otclient
Length of output: 5110
🏁 Script executed:
# Search for other filter-related functions mentioned (passes*, applyAllFilters, etc.)
rg "function (applyAllFilters|passes|filter)" modules/game_cyclopedia/tab/magicalArchives/ 2>/dev/nullRepository: opentibiabr/otclient
Length of output: 661
🏁 Script executed:
# Check where filteredSpells is used to understand the filter pipeline
rg "filteredSpells" modules/game_cyclopedia/tab/magicalArchives/ 2>/dev/nullRepository: opentibiabr/otclient
Length of output: 764
🏁 Script executed:
# Get the applyAllFilters function to see how it manages filtering
rg -B 5 -A 30 "^function applyAllFilters" modules/game_cyclopedia/tab/magicalArchives/magicalArchives.luaRepository: opentibiabr/otclient
Length of output: 1182
🏁 Script executed:
# Search for where passes* functions are called
rg "passes(Vocation|Level|SpellGroup|LearntSpells)" modules/game_cyclopedia/tab/magicalArchives/magicalArchives.lua -A 2 -B 2Repository: opentibiabr/otclient
Length of output: 1433
🏁 Script executed:
# Get the full context around the filter functions to understand the pipeline
rg -B 3 -A 15 "^function passes" modules/game_cyclopedia/tab/magicalArchives/magicalArchives.luaRepository: opentibiabr/otclient
Length of output: 1896
🏁 Script executed:
# Verify the pattern matching issue by checking Lua string.find behavior with special characters
python3 << 'EOF'
# Demonstrate the issue with Lua pattern matching
# In Lua, string:find(pattern) treats the pattern as a pattern with special chars
# Let's show what happens with problematic inputs
test_cases = [
("Fire [", "Invalid pattern: unclosed bracket"),
("100%", "Valid but % is escape char"),
("spell[1]", "[ ] are character class syntax"),
]
print("Lua pattern matching issues with user input:")
print("=" * 60)
for test_input, explanation in test_cases:
print(f"Input: '{test_input}'")
print(f"Issue: {explanation}")
print()
print("\nSolution: Use plain=true parameter")
print("string:find(text, pattern, 1, true) treats pattern as literal")
EOFRepository: opentibiabr/otclient
Length of output: 431
🏁 Script executed:
# Check if there's any error handling around the find() call in searchSpells
rg -B 5 -A 10 "function searchSpells" modules/game_cyclopedia/tab/magicalArchives/magicalArchives.lua | grep -A 10 "find"Repository: opentibiabr/otclient
Length of output: 46
🏁 Script executed:
# Check the full line count of the file to understand its size
wc -l modules/game_cyclopedia/tab/magicalArchives/magicalArchives.luaRepository: opentibiabr/otclient
Length of output: 132
🏁 Script executed:
# Get more complete view of searchSpells to understand full scope
sed -n '843,900p' modules/game_cyclopedia/tab/magicalArchives/magicalArchives.luaRepository: opentibiabr/otclient
Length of output: 2078
Apply search filter on top of already-filtered results.
searchSpells() rebuilds from allSpells and discards vocation/level/group/learnt filters as soon as the user types. Additionally, it uses Lua pattern matching without the plain=true parameter, causing special characters like %, [, and ] to behave unexpectedly or error. Either filter the already-filtered set or rerun all passes* checks here alongside the text search using plain string matching (string:find(text, searchText, 1, true)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_cyclopedia/tab/magicalArchives/magicalArchives.lua` around lines
843 - 860, searchSpells currently rebuilds filteredSpells from allSpells and
uses pattern matching, discarding existing vocation/level/group/learnt filters
and mis-handling magic chars; change it to either iterate the already-filtered
list (filteredSpells or a copy) or, if you must iterate allSpells, apply the
same predicate functions (passesVocation, passesLevel, passesGroup, passesLearnt
or whatever filter helpers are used by applyAllFilters) before inserting, and
switch string matching to plain mode (use string.find(..., 1, true) or
:find(searchText, 1, true)) for both spell.name and spell.words; keep calling
updateSpellListUI() afterward.
Comment on lines
+1211
to
+1225
| -- Cooldown | ||
| local runeCooldownValue = UI:recursiveGetChildById('runeCooldownValue') | ||
| if runeCooldownValue then | ||
| local cooldown = spell.cooldown or 2000 | ||
| local seconds = math.floor(cooldown / 1000) | ||
| runeCooldownValue:setText(seconds .. 's') | ||
| end | ||
|
|
||
| -- Group Cooldown | ||
| local runeGroupCooldownValue = UI:recursiveGetChildById('runeGroupCooldownValue') | ||
| if runeGroupCooldownValue then | ||
| local groupCooldown = spell.groupCooldown or spell.cooldown or 2000 | ||
| local seconds = math.floor(groupCooldown / 1000) | ||
| runeGroupCooldownValue:setText(seconds .. 's') | ||
| end |
There was a problem hiding this comment.
Rune cooldowns are reading fields that the spell schema doesn't provide.
SpellInfo.Default stores cooldowns in exhaustion and group = {[id] = cooldown} (see modules/gamelib/spells.lua:34-48), but this branch falls back to spell.cooldown and spell.groupCooldown. As written, the rune tab will show 2s for most entries regardless of the real cooldown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_cyclopedia/tab/magicalArchives/magicalArchives.lua` around lines
1211 - 1225, The cooldown fields are read from non-existent
spell.cooldown/groupCooldown; instead use the schema fields spell.exhaustion for
the rune cooldown and spell.group (a map) for group cooldown. Replace the
fallback reads in the rune UI block so runeCooldownValue uses spell.exhaustion
(or default 2000) and runeGroupCooldownValue uses spell.group[spell.id] (or
spell.exhaustion or 2000 if that lookup is nil); keep the same math.floor and
setText logic. Reference: recursiveGetChildById, runeCooldownValue,
runeGroupCooldownValue, spell.exhaustion, and spell.group[spell.id].
Comment on lines
+165
to
+178
| function g_game.requestSelectCharacterTitle(titleId) | ||
| titleId = normalizePositiveNumber(titleId) | ||
| if titleId <= 0 then | ||
| return | ||
| end | ||
|
|
||
| if g_game.getClientVersion and g_game.getClientVersion() < 1412 then | ||
| return | ||
| end | ||
|
|
||
| if g_game.requestSetCharacterTitle then | ||
| g_game.requestSetCharacterTitle(titleId) | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how titleId is used in the codebase to determine if 0 is a valid "no title" value
rg -n -C3 'titleId' --type lua | head -80Repository: opentibiabr/otclient
Length of output: 4132
🏁 Script executed:
# Search for all calls to requestSelectCharacterTitle
rg -n 'requestSelectCharacterTitle' --type lua -B2 -A2Repository: opentibiabr/otclient
Length of output: 942
🏁 Script executed:
# Search for selectedTitleId initialization and handling
rg -n 'selectedTitleId' --type lua -B1 -A1 | head -100Repository: opentibiabr/otclient
Length of output: 1377
🏁 Script executed:
# Look for protocol definitions or constants related to titles
rg -n 'Titles\|TITLE\|titleId.*0\|0.*title' --type lua --type json | head -50Repository: opentibiabr/otclient
Length of output: 46
🏁 Script executed:
# Find characterTitlesCurrentTitle initialization and usage
rg -n 'characterTitlesCurrentTitle' --type lua -B2 -A2Repository: opentibiabr/otclient
Length of output: 1418
🏁 Script executed:
# Look for where character titles are set from server/protocol
rg -n 'characterTitlesCache\|setCharacterTitles\|currentTitle' --type lua -B1 -A1 | head -80Repository: opentibiabr/otclient
Length of output: 46
🏁 Script executed:
# Verify if the protocol/network code accepts titleId=0
rg -n 'requestSetCharacterTitle\|SetCharacterTitle' --type lua -B2 -A5 | head -60Repository: opentibiabr/otclient
Length of output: 46
Allow titleId = 0 to clear title selection.
The function currently returns early when titleId <= 0, but the codebase uses 0 as the sentinel value for "no character title selected" (initialized at characterTitlesCurrentTitle = 0 and displayed as "(No character title selected)" in the UI). This prevents users from clearing their title selection. Change the condition from if titleId <= 0 to if titleId < 0 to allow 0 while still rejecting invalid negative values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/gamelib/game.lua` around lines 165 - 178, In
g_game.requestSelectCharacterTitle, allow titleId == 0 to clear the selection by
changing the validation to reject only negative values: replace the check "if
titleId <= 0" with "if titleId < 0" (after normalizing with
normalizePositiveNumber), keeping the subsequent client version guard
(g_game.getClientVersion) and the call to
g_game.requestSetCharacterTitle(titleId) as-is so the 0 sentinel is passed
through to requestSetCharacterTitle.
Comment on lines
324
to
+342
| function Spells.getSpellByName(name) | ||
| return SpellInfo[Spells.getSpellProfileByName(name)][name] | ||
| if type(name) ~= "string" or name == "" then | ||
| return nil | ||
| end | ||
|
|
||
| local profile = Spells.getSpellProfileByName(name) | ||
| if profile and SpellInfo[profile] and SpellInfo[profile][name] then | ||
| return SpellInfo[profile][name] | ||
| end | ||
|
|
||
| for _, data in pairs(SpellInfo) do | ||
| for _, spell in pairs(data) do | ||
| if spell.name == name then | ||
| return spell | ||
| end | ||
| end | ||
| end | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Reuse the normalized spell name in the fast-path lookup.
getSpellProfileByName() trims the input, but this branch still indexes SpellInfo[profile][name] with the original string. Inputs like "Light Healing " now resolve a profile and still return nil, so the normalization below never actually helps here.
💡 Suggested fix
function Spells.getSpellByName(name)
- if type(name) ~= "string" or name == "" then
+ if type(name) ~= "string" then
return nil
end
- local profile = Spells.getSpellProfileByName(name)
- if profile and SpellInfo[profile] and SpellInfo[profile][name] then
- return SpellInfo[profile][name]
+ local normalizedName = name:trim()
+ if normalizedName == "" then
+ return nil
+ end
+
+ local profile = Spells.getSpellProfileByName(normalizedName)
+ if profile and SpellInfo[profile] and SpellInfo[profile][normalizedName] then
+ return SpellInfo[profile][normalizedName]
end
for _, data in pairs(SpellInfo) do
for _, spell in pairs(data) do
- if spell.name == name then
+ if spell.name == normalizedName then
return spell
end
end
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.
Suggested change
| function Spells.getSpellByName(name) | |
| return SpellInfo[Spells.getSpellProfileByName(name)][name] | |
| if type(name) ~= "string" or name == "" then | |
| return nil | |
| end | |
| local profile = Spells.getSpellProfileByName(name) | |
| if profile and SpellInfo[profile] and SpellInfo[profile][name] then | |
| return SpellInfo[profile][name] | |
| end | |
| for _, data in pairs(SpellInfo) do | |
| for _, spell in pairs(data) do | |
| if spell.name == name then | |
| return spell | |
| end | |
| end | |
| end | |
| return nil | |
| function Spells.getSpellByName(name) | |
| if type(name) ~= "string" then | |
| return nil | |
| end | |
| local normalizedName = name:trim() | |
| if normalizedName == "" then | |
| return nil | |
| end | |
| local profile = Spells.getSpellProfileByName(normalizedName) | |
| if profile and SpellInfo[profile] and SpellInfo[profile][normalizedName] then | |
| return SpellInfo[profile][normalizedName] | |
| end | |
| for _, data in pairs(SpellInfo) do | |
| for _, spell in pairs(data) do | |
| if spell.name == normalizedName then | |
| return spell | |
| end | |
| end | |
| end | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/gamelib/spells.lua` around lines 324 - 342, The fast-path uses the
original input when indexing SpellInfo even though getSpellProfileByName()
normalizes/trims the name; fix getSpellByName to trim/normalize the name once
and reuse it for both getSpellProfileByName and the SpellInfo lookup (e.g.
compute a local normalizedName = <trim name> or call the same normalizer, then
call Spells.getSpellProfileByName(normalizedName) and index
SpellInfo[profile][normalizedName] instead of SpellInfo[profile][name]); update
the code in function Spells.getSpellByName to use normalizedName for the profile
lookup and the direct SpellInfo fast-path.
Comment on lines
+378
to
+397
| function Spells.getSpellIconId(spellName) | ||
| if not spellName then | ||
| return 0 | ||
| end | ||
|
|
||
| local spellData = Spells.getSpellByName(spellName) | ||
| if not spellData then | ||
| return 0 | ||
| end | ||
|
|
||
| if type(spellData.clientId) == "number" and spellData.clientId >= 0 then | ||
| return spellData.clientId | ||
| end | ||
|
|
||
| if type(spellData.id) == "number" and spellData.id >= 0 then | ||
| return spellData.id | ||
| end | ||
|
|
||
| return 0 | ||
| end |
There was a problem hiding this comment.
Treat clientId == 0 as missing so the fallback can run.
A lot of entries above use clientId = 0 as the missing-sprite sentinel while still having a valid spell id — Ultimate Healing is one example. With the current >= 0 check, getSpellIconId() returns 0 and never falls back, so those spells render without an icon.
🛠️ Suggested fix
- if type(spellData.clientId) == "number" and spellData.clientId >= 0 then
+ if type(spellData.clientId) == "number" and spellData.clientId > 0 then
return spellData.clientId
end
if type(spellData.id) == "number" and spellData.id >= 0 then
return spellData.id📝 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 Spells.getSpellIconId(spellName) | |
| if not spellName then | |
| return 0 | |
| end | |
| local spellData = Spells.getSpellByName(spellName) | |
| if not spellData then | |
| return 0 | |
| end | |
| if type(spellData.clientId) == "number" and spellData.clientId >= 0 then | |
| return spellData.clientId | |
| end | |
| if type(spellData.id) == "number" and spellData.id >= 0 then | |
| return spellData.id | |
| end | |
| return 0 | |
| end | |
| function Spells.getSpellIconId(spellName) | |
| if not spellName then | |
| return 0 | |
| end | |
| local spellData = Spells.getSpellByName(spellName) | |
| if not spellData then | |
| return 0 | |
| end | |
| if type(spellData.clientId) == "number" and spellData.clientId > 0 then | |
| return spellData.clientId | |
| end | |
| if type(spellData.id) == "number" and spellData.id >= 0 then | |
| return spellData.id | |
| end | |
| return 0 | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/gamelib/spells.lua` around lines 378 - 397, The clientId sentinel 0
is treated as a valid icon because Spells.getSpellIconId checks
"spellData.clientId >= 0", preventing fallback to spellData.id; change the
clientId check in Spells.getSpellIconId to require a positive value (e.g.,
type(spellData.clientId) == "number" and spellData.clientId > 0) so clientId ==
0 is treated as missing and the function will fall back to checking spellData.id
(keep the existing spellData.id check as-is).
59 tasks
Contributor
|
Houses > Rented is not working correctly: 
Sort by rent filter/ bid filter: This filter not working properly too: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR delivers the Cyclopedia feature set from the
imp/cyclopediabranch, covering protocol-to-UI integration for Houses and UX hardening for Character/Magical Archives.Main changes:
g_game.request*House*wrappers.getTabState/saveTabState/resetTabState).titleIdfallback behavior).Motivation/context:
Dependencies:
toggleCyclopediaHouseAuction = true.spells.lua/SpellInfo).Behavior
Actual
Cyclopedia had partial/inconsistent Houses integration, context reset between tabs, Character Titles/Magical Archives UX gaps, and high RAM growth while browsing Character Appearances.
Expected
Cyclopedia Houses works end-to-end with real payloads, tab context persists across navigation, Character Titles and Magical Archives interactions are stable/persisted, and Character Appearances uses lower memory during browsing.
Fixes
(issue)
Type of change
How Has This Been Tested
Test Configuration:
imp/cyclopedia)Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements