feat(ui): add missing items report dialog - #1006
Conversation
- Added MissingItemReport struct and MissingItemsDialog for tracking and displaying item definition mismatches between DAT/OTB/XML files - Refactored ItemDefinitionResolver to collect missing items instead of failing on missing DAT entries, added isDatItemEmptyOrInvalid filter - Integrated missing items collection into AssetBundle, ItemDefinitionsLoader, and VersionManager load pipelines - Added SHOW_MISSING_ITEMS_WARNING config setting (default false) to control detailed warning output during data load - Added "Missing Items Report..." menu item under File -> Reload with handler in FileMenuHandler to display dialog - Added preferences checkbox in EditorPage to toggle missing items warning display - Registered MISSING_ITEMS_REPORT action in menubar_action_manager and added enum/handler to main_menubar
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of the map editor's data loading process by introducing a comprehensive system for detecting and reporting item definition mismatches. Instead of failing when inconsistencies are found between DAT, OTB, and XML files, the editor now collects these issues and provides a user-friendly dialog to review them. This allows users to identify and address data integrity problems without interrupting the loading process, while also providing configurable options to control the visibility of these warnings. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCollects and stores categorized missing-item discrepancies during item resolution, exposes them via VersionManager and AssetBundle, adds a "Missing Items Report..." menu item/action that opens a dialog displaying categorized lists, and adds a preference to optionally show missing-item warnings on load. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Menu as Menu System
participant FileHandler as FileMenuHandler
participant VersionMgr as VersionManager
participant Dialog as MissingItemsDialog
User->>Menu: Click "Missing Items Report..."
Menu->>FileHandler: OnMissingItemsReport()
FileHandler->>VersionMgr: getLastMissingItems()
VersionMgr-->>FileHandler: MissingItemReport
FileHandler->>VersionMgr: lastLoadHasOtb()
VersionMgr-->>FileHandler: bool
FileHandler->>Dialog: Show(parent, report, hasOtb)
Dialog->>Dialog: Build UI (tabs, lists, counts)
Dialog-->>User: Display modal dialog
User->>Dialog: Copy / Save / Close
Dialog-->>User: End modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Missing Items Report system to identify and display discrepancies between items.otb, tibia.dat, and items.xml. It includes a new UI dialog, a preference setting for load-time warnings, and logic to track missing definitions during item resolution. Feedback focuses on strict adherence to the project's C++20/23 style guide, specifically requiring the use of .contains() for container checks, std::format for string formatting, and FromDIP() for High DPI compatibility. Additionally, a large UI initialization function needs to be refactored to comply with function length limits.
| CenterOnParent(); | ||
| } | ||
|
|
||
| void MissingItemsDialog::BuildUI() { |
There was a problem hiding this comment.
The BuildUI function is approximately 120 lines long, which exceeds the 50-line limit defined in the style guide. Please split this function into smaller, logical components (e.g., CreateDatPage, CreateOtbPage, etc.) to improve maintainability and adhere to SRP.
References
- SRP LIMITS — Function > 50 lines → split. (link)
| text += wxString::Format("Server ID: %u, Client ID: %u, Name: %s, Description: %s\n", | ||
| entry.server_id, entry.client_id, entry.name, entry.description); |
There was a problem hiding this comment.
Using wxString::Format with std::string objects (like entry.name) using the %s specifier is unsafe and can lead to undefined behavior or garbage output if .c_str() is not called. Switching to std::format is safer and required by the style guide.
text += std::format("Server ID: {}, Client ID: {}, Name: {}, Description: {}\n",
entry.server_id, entry.client_id, entry.name, entry.description);References
- std::format over sprintf/wxString::Format (link)
| if (isDatItemEmptyOrInvalid(dat, input.dat_catalog, client_id)) { | ||
| continue; | ||
| } | ||
| if (dat_ids_referenced_by_otb.find(client_id) == dat_ids_referenced_by_otb.end()) { |
There was a problem hiding this comment.
Since the project mandates C++20/23, use .contains() for checking existence in associative containers instead of find() == end(). This is more idiomatic and readable.
| if (dat_ids_referenced_by_otb.find(client_id) == dat_ids_referenced_by_otb.end()) { | |
| if (!dat_ids_referenced_by_otb.contains(client_id)) { |
References
- Every line of code you write or modify MUST use C++20/23 features. Pre-C++17 patterns are technical debt — eliminate on contact. (link)
| if (server_id < 100) { | ||
| continue; | ||
| } | ||
| if (fragments.otb.find(server_id) == fragments.otb.end()) { |
There was a problem hiding this comment.
Use .contains() for existence checks in maps to adhere to C++20 standards as required by the style guide.
| if (fragments.otb.find(server_id) == fragments.otb.end()) { | |
| if (!fragments.otb.contains(server_id)) { |
References
- C++20/23 is MANDATORY. Every new and modified file MUST use modern C++20/23 features. (link)
| if (isDatItemEmptyOrInvalid(dat, input.dat_catalog, client_id)) { | ||
| continue; | ||
| } | ||
| if (dat_ids_used_by_xml.find(client_id) == dat_ids_used_by_xml.end()) { |
There was a problem hiding this comment.
Use .contains() for existence checks in sets to adhere to C++20 standards.
| if (dat_ids_used_by_xml.find(client_id) == dat_ids_used_by_xml.end()) { | |
| if (!dat_ids_used_by_xml.contains(client_id)) { |
References
- C++20/23 is MANDATORY. Every new and modified file MUST use modern C++20/23 features. (link)
| } | ||
| auto* headerText = newd wxStaticText(this, wxID_ANY, headerTextContent, | ||
| wxDefaultPosition, wxDefaultSize, wxALIGN_LEFT); | ||
| mainSizer->Add(headerText, 0, wxALL | wxEXPAND, 10); |
There was a problem hiding this comment.
All hardcoded pixel values, including sizer borders, must be wrapped in FromDIP() to ensure proper scaling on High DPI displays.
mainSizer->Add(headerText, 0, wxALL | wxEXPAND, FromDIP(10));References
- USE: FromDIP() for any pixel values. BANNED: Hardcoded wxPoint/wxSize pixels. (link)
| wxString::Format("%u", entry.server_id), | ||
| wxString::Format("%u", entry.client_id), |
There was a problem hiding this comment.
The style guide explicitly bans wxString::Format in favor of std::format. Please update all instances in this file.
std::format("{}", entry.server_id),
std::format("{}", entry.client_id),References
- std::format over sprintf/wxString::Format (link)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a valuable missing items report feature to help identify discrepancies between DAT, OTB, and XML data files. The implementation correctly integrates into the loading pipeline and provides a modern user interface for viewing and exporting reports.
🔍 General Feedback
- The use of
std::formatand other C++23 features is appropriate given the project's configuration. - The filtering heuristic in
isDatItemEmptyOrInvalidis a clever way to reduce noise from placeholder entries in data files. - Critical Fix Needed: In the report generation logic,
wxString::Formatis used withstd::stringand%s, which will lead to crashes. This must be corrected to usestd::formator.c_str().
| text += wxString::Format("Server ID: %u, Client ID: %u, Name: %s, Description: %s\n", | ||
| entry.server_id, entry.client_id, entry.name, entry.description); | ||
| } |
There was a problem hiding this comment.
🔴 Same issue here: wxString::Format with %s will fail with std::string. Use std::format instead.
| text += wxString::Format("Server ID: %u, Client ID: %u, Name: %s, Description: %s\n", | |
| entry.server_id, entry.client_id, entry.name, entry.description); | |
| } | |
| for (const auto& entry : report.xml_no_otb) { | |
| text += wxString::FromUTF8(std::format("Server ID: {}, Client ID: {}, Name: {}, Description: {}\n", | |
| entry.server_id, entry.client_id, entry.name, entry.description)); | |
| } |
| } | ||
| } | ||
|
|
||
| if (rows.empty() && missingReport && missingReport->missing_in_dat.empty()) { |
There was a problem hiding this comment.
🟡 This check is inconsistent with resolveDatOnly. In resolveDatOnly, the function returns false if rows is empty, regardless of whether missingReport is provided. If no items are resolved, the editor will likely be in an unusable state, so it should probably always return an error if rows.empty().
| if (rows.empty() && missingReport && missingReport->missing_in_dat.empty()) { | |
| if (rows.empty()) { |
| text += wxString::Format("Server ID: %u, Client ID: %u, Name: %s, Description: %s\n", | ||
| entry.server_id, entry.client_id, entry.name, entry.description); | ||
| } |
There was a problem hiding this comment.
🔴 wxString::Format with the %s format specifier does not correctly handle std::string objects and will likely cause a crash or undefined behavior. Since the project uses C++23, it is safer and more idiomatic to use std::format and wxString::FromUTF8.
| text += wxString::Format("Server ID: %u, Client ID: %u, Name: %s, Description: %s\n", | |
| entry.server_id, entry.client_id, entry.name, entry.description); | |
| } | |
| for (const auto& entry : report.missing_in_dat) { | |
| text += wxString::FromUTF8(std::format("Server ID: {}, Client ID: {}, Name: {}, Description: {}\n", | |
| entry.server_id, entry.client_id, entry.name, entry.description)); | |
| } |
| SetMinSize(wxSize(600, 400)); | ||
| } | ||
|
|
||
| wxString MissingItemsDialog::GenerateReportText(bool hasOtb) const { |
There was a problem hiding this comment.
🟡 The hasOtb parameter shadows the class member hasOtb. Since it is always called with the member value, this parameter is redundant and can be removed.
| wxString MissingItemsDialog::GenerateReportText(bool hasOtb) const { | |
| wxString MissingItemsDialog::GenerateReportText() const { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
source/ui/menubar/menubar_action_manager.cpp (1)
173-173: Consider adding enable/disable state management for MISSING_ITEMS_REPORT.The action is registered but
UpdateStatedoesn't include anEnableItemcall forMISSING_ITEMS_REPORT. Other data-dependent actions likeDEBUG_VIEW_DAT(line 298) are conditionally enabled withmb->EnableItem(DEBUG_VIEW_DAT, loaded). The Missing Items Report should likely only be accessible when a version is loaded, sincegetLastMissingItems()data is populated during version loading.♻️ Proposed fix to add state management
Add this line near line 298 in
UpdateState:mb->EnableItem(MISSING_ITEMS_REPORT, loaded);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/menubar/menubar_action_manager.cpp` at line 173, Add state management for MISSING_ITEMS_REPORT in the UpdateState method: ensure the menu item created by MAKE_ACTION_ICON(MISSING_ITEMS_REPORT, ..., OnMissingItemsReport) is enabled/disabled based on whether a version is loaded (same condition used for DEBUG_VIEW_DAT). Call mb->EnableItem(MISSING_ITEMS_REPORT, loaded) in UpdateState so the Missing Items Report (which reads getLastMissingItems()) is only accessible when data is present.source/app/managers/version_manager.h (1)
33-41: Consider clearinglast_missing_itemson load failure to avoid stale data.Looking at the context snippet from
version_manager.cpp(lines 87-125), ifbundle_loader.load()orbundle_loader.install()fails,UnloadVersion()is called butlast_missing_itemsis not cleared. This meansgetLastMissingItems()could return stale data from a previous successful load after a failed reload attempt.While the UI check for empty vectors prevents showing an incorrect dialog when no version is loaded, this edge case could still cause confusion if users check the report after a failed reload.
♻️ Option 1: Clear in UnloadVersion
In
version_manager.cpp, add toUnloadVersion():last_missing_items = MissingItemReport{}; last_load_has_otb = true;♻️ Option 2: Clear at start of LoadDataFiles
At the beginning of
LoadDataFiles():last_missing_items = MissingItemReport{};Also applies to: 47-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/managers/version_manager.h` around lines 33 - 41, getLastMissingItems can return stale data after a failed reload because last_missing_items (and last_load_has_otb) are not cleared when bundle_loader.load() or bundle_loader.install() fails; fix by clearing last_missing_items = MissingItemReport{} and resetting last_load_has_otb (e.g. true or default) either at the start of LoadDataFiles() or inside UnloadVersion() so any failed load leaves no leftover report — update the code paths around bundle_loader.load()/bundle_loader.install() and UnloadVersion/LoadDataFiles to ensure last_missing_items and last_load_has_otb are reset on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/item_definitions/core/item_definition_resolver.cpp`:
- Around line 105-125: resolveDatOnly() currently treats only a completely
missing fragments.dat entry as "missing", while resolveDatOtb() treats both
absent and unusable/empty DAT entries as missing; update resolveDatOnly() to use
the same "missing or unusable DAT" predicate as resolveDatOtb(): when
fragments.dat contains effective_client_id but the DAT entry is unusable (e.g.,
has no drawable/content), populate MissingItemEntry (server_id, client_id,
otb.name, otb.description) into missingReport->missing_in_dat or set the same
error string and return false when missingReport is null; also ensure
dat_ids_referenced_by_otb and row.flags are only updated for DAT entries
considered usable (match the logic/path used in resolveDatOtb()) so placeholder
client IDs are reported consistently.
- Around line 14-21: The fast-path in isDatItemEmptyOrInvalid incorrectly treats
any DatItemFragment with default metadata as empty before checking whether the
real definition comes from the sprite/catalog; update isDatItemEmptyOrInvalid to
only return true for the "all-default" case when there is no corresponding entry
in the DatCatalog/sprite source for that item (use the DatCatalog pointer and
client_id or the fragment's sprite/index info to look up if a definition
exists), otherwise fall through and consider the item valid; specifically, add a
catalog lookup (e.g., via an existing lookup method on DatCatalog) when dat
fields are defaults and only return true if that lookup fails.
---
Nitpick comments:
In `@source/app/managers/version_manager.h`:
- Around line 33-41: getLastMissingItems can return stale data after a failed
reload because last_missing_items (and last_load_has_otb) are not cleared when
bundle_loader.load() or bundle_loader.install() fails; fix by clearing
last_missing_items = MissingItemReport{} and resetting last_load_has_otb (e.g.
true or default) either at the start of LoadDataFiles() or inside
UnloadVersion() so any failed load leaves no leftover report — update the code
paths around bundle_loader.load()/bundle_loader.install() and
UnloadVersion/LoadDataFiles to ensure last_missing_items and last_load_has_otb
are reset on failure.
In `@source/ui/menubar/menubar_action_manager.cpp`:
- Line 173: Add state management for MISSING_ITEMS_REPORT in the UpdateState
method: ensure the menu item created by MAKE_ACTION_ICON(MISSING_ITEMS_REPORT,
..., OnMissingItemsReport) is enabled/disabled based on whether a version is
loaded (same condition used for DEBUG_VIEW_DAT). Call
mb->EnableItem(MISSING_ITEMS_REPORT, loaded) in UpdateState so the Missing Items
Report (which reads getLastMissingItems()) is only accessible when data is
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae4bcedc-1041-44f2-ac8f-cc6a76174a18
📒 Files selected for processing (22)
data/menubar.xmlsource/CMakeLists.txtsource/app/managers/version_manager.cppsource/app/managers/version_manager.hsource/app/preferences/editor_page.cppsource/app/preferences/editor_page.hsource/app/settings.cppsource/app/settings.hsource/item_definitions/core/asset_bundle.hsource/item_definitions/core/asset_bundle_loader.cppsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/item_definition_resolver.hsource/item_definitions/core/item_definitions_loader.cppsource/item_definitions/core/item_definitions_loader.hsource/item_definitions/core/missing_item_report.hsource/ui/dialogs/missing_items_dialog.cppsource/ui/dialogs/missing_items_dialog.hsource/ui/main_menubar.cppsource/ui/main_menubar.hsource/ui/menubar/file_menu_handler.cppsource/ui/menubar/file_menu_handler.hsource/ui/menubar/menubar_action_manager.cpp
| const auto dat_it = fragments.dat.find(effective_client_id); | ||
| if (dat_it == fragments.dat.end()) { | ||
| error = wxString::FromUTF8(std::format("Missing DAT definition for client id {} (server id {}).", effective_client_id, server_id)); | ||
| return false; | ||
| // DAT item truly doesn't exist - collect as missing | ||
| if (missingReport) { | ||
| MissingItemEntry entry; | ||
| entry.server_id = server_id; | ||
| entry.client_id = effective_client_id; | ||
| entry.name = otb.name; | ||
| entry.description = otb.description; | ||
| missingReport->missing_in_dat.push_back(std::move(entry)); | ||
| } else { | ||
| error = wxString::FromUTF8(std::format("Missing DAT definition for client id {} (server id {}).", effective_client_id, server_id)); | ||
| return false; | ||
| } | ||
| // Continue processing other items | ||
| continue; | ||
| } | ||
|
|
||
| // DAT item exists (even if empty) - this is a valid OTB mapping | ||
| dat_ids_referenced_by_otb.insert(effective_client_id); | ||
| row.flags |= dat_it->second.flags & ~flagMask(ItemFlag::Moveable); |
There was a problem hiding this comment.
Use the same “missing or unusable DAT” predicate in both resolve paths.
Right now resolveDatOtb() only treats an absent DAT fragment as missing, while resolveDatOnly() silently skips XML rows whose DAT slot exists but has no drawable content. That leaves placeholder client IDs out of missing_in_dat entirely, even though they are still missing usable DAT definitions from the editor’s point of view.
Also applies to: 220-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 105 -
125, resolveDatOnly() currently treats only a completely missing fragments.dat
entry as "missing", while resolveDatOtb() treats both absent and unusable/empty
DAT entries as missing; update resolveDatOnly() to use the same "missing or
unusable DAT" predicate as resolveDatOtb(): when fragments.dat contains
effective_client_id but the DAT entry is unusable (e.g., has no
drawable/content), populate MissingItemEntry (server_id, client_id, otb.name,
otb.description) into missingReport->missing_in_dat or set the same error string
and return false when missingReport is null; also ensure
dat_ids_referenced_by_otb and row.flags are only updated for DAT entries
considered usable (match the logic/path used in resolveDatOtb()) so placeholder
client IDs are reported consistently.
Added tracking and UI reporting for OTB entries that lack corresponding XML overrides. - `source/item_definitions/core/missing_item_report.h` — added `otb_no_xml` vector to MissingItemReport struct - `source/item_definitions/core/item_definition_resolver.cpp` — collect OTB entries missing XML overrides during resolution, skipping entries with no client ID, no DAT definition, or existing XML entry - `source/app/managers/version_manager.cpp` — include `otb_no_xml` count in missing items total and add warning log section listing each OTB-no-XML entry - `source/ui/dialogs/missing_items_dialog.h` — added `listOtbNoXml` and `countOtbNoXml` member pointers - `source/ui/dialogs/missing_items_dialog.cpp` — added fourth notebook tab for OTB-no-XML entries with server ID, client ID, name, description columns; updated dialog show condition to include `otb_no_xml` - `source/ui/menubar/file_menu_handler.cpp` — updated missing items report menu check to include `otb_no_xml` emptiness test
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
source/item_definitions/core/item_definition_resolver.cpp (2)
193-197: Consider harmonizing the empty-rows check between resolve methods.In
resolveDatOtb(line 193), failing returns false only whenrowsis empty ANDmissingReport->missing_in_datis also empty (allowing the load to succeed if items were collected for reporting). However,resolveDatOnly(line 296) always fails whenrowsis empty.If the intent is graceful degradation when collecting missing items, consider applying the same pattern to
resolveDatOnly:- if (rows.empty()) { + if (rows.empty() && (!missingReport || missingReport->missing_in_dat.empty())) { error = "No item definitions were resolved from DAT/XML."; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 193 - 197, resolveDatOnly currently fails whenever rows.empty(); harmonize it with resolveDatOtb by only treating empty rows as an error when there is no missingReport data: in resolveDatOnly check if rows.empty() && missingReport && missingReport->missing_in_dat.empty(), set error = "No item definitions were resolved from DAT/OTB/XML." and return false, otherwise return true so the loader can succeed when rows are empty but missingReport has collected items; reference resolveDatOnly, rows, missingReport, and missing_in_dat when applying the change.
279-293: Semantic mismatch: reusingmissing_in_otbfor DAT-only mode.In DAT-only mode, this populates
missing_in_otbwith DAT items not referenced by XML. However,MissingItemReport::missing_in_otbis documented as "In DAT but not in OTB" (seemissing_item_report.h:21). The UI correctly adjusts labels (e.g., "DAT not in XML" vs "DAT not in OTB"), but the semantic overloading of this field could cause confusion in logging, debugging, or future maintenance.Consider either:
- Adding a separate field (e.g.,
missing_in_xml) for DAT-only mode, or- Documenting this dual-use behavior clearly in
MissingItemReport🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 279 - 293, The code is populating MissingItemReport::missing_in_otb for DAT-only mode which semantically means "in DAT but not in OTB"; add a new field MissingItemReport::missing_in_xml (and corresponding vector of MissingItemEntry) and change the loop in item_definition_resolver.cpp that iterates fragments.dat (currently using missingReport->missing_in_otb) to push entries into missing_in_xml when fragments.xml is non-empty/when operating in DAT-only mode instead; update any consumers/serializers/logging that read MissingItemReport to handle the new missing_in_xml field (or alternatively add clear doc comments to MissingItemReport::missing_in_otb and adjust all readers to interpret it conditionally if you prefer documentation over schema change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/ui/dialogs/missing_items_dialog.cpp`:
- Around line 173-208: GenerateReportText currently omits the "otb_no_xml"
section when hasOtb is true, so add a new block inside GenerateReportText
guarded by the same hasOtb branch that appends a header for report.otb_no_xml
(with its size) and iterates report.otb_no_xml, appending each entry's details
(Server ID, Client ID, Name, Description) using wxString::Format—mirror the
style used for report.xml_no_otb and the other sections so the OTB-only entries
are included in the exported report.
---
Nitpick comments:
In `@source/item_definitions/core/item_definition_resolver.cpp`:
- Around line 193-197: resolveDatOnly currently fails whenever rows.empty();
harmonize it with resolveDatOtb by only treating empty rows as an error when
there is no missingReport data: in resolveDatOnly check if rows.empty() &&
missingReport && missingReport->missing_in_dat.empty(), set error = "No item
definitions were resolved from DAT/OTB/XML." and return false, otherwise return
true so the loader can succeed when rows are empty but missingReport has
collected items; reference resolveDatOnly, rows, missingReport, and
missing_in_dat when applying the change.
- Around line 279-293: The code is populating MissingItemReport::missing_in_otb
for DAT-only mode which semantically means "in DAT but not in OTB"; add a new
field MissingItemReport::missing_in_xml (and corresponding vector of
MissingItemEntry) and change the loop in item_definition_resolver.cpp that
iterates fragments.dat (currently using missingReport->missing_in_otb) to push
entries into missing_in_xml when fragments.xml is non-empty/when operating in
DAT-only mode instead; update any consumers/serializers/logging that read
MissingItemReport to handle the new missing_in_xml field (or alternatively add
clear doc comments to MissingItemReport::missing_in_otb and adjust all readers
to interpret it conditionally if you prefer documentation over schema change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9746107a-d9ba-4197-ab7f-72d8f99d52d3
📒 Files selected for processing (6)
source/app/managers/version_manager.cppsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/missing_item_report.hsource/ui/dialogs/missing_items_dialog.cppsource/ui/dialogs/missing_items_dialog.hsource/ui/menubar/file_menu_handler.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- source/ui/menubar/file_menu_handler.cpp
- source/item_definitions/core/missing_item_report.h
source/app/managers/version_manager.cpp — Clear last_missing_items and reset last_load_has_otb on bundle load/install failure to prevent stale data source/item_definitions/core/item_definition_resolver.cpp — Prioritize catalog sprite validation over default metadata checks in isDatItemEmptyOrInvalid; replace .find() == .end() with .contains(); remove empty rows check gating missing_in_dat source/ui/dialogs/missing_items_dialog.cpp — Wrap all hardcoded pixel values with FromDIP() for DPI awareness; replace wxString::Format with wxString::FromUTF8(std::format()) for integer formatting; remove hasOtb parameter from GenerateReportText and always show OTB no XML section source/ui/dialogs/missing_items_dialog.h — Remove hasOtb parameter from GenerateReportText declaration source/ui/menubar/menubar_action_manager.cpp — Enable MISSING_ITEMS_REPORT menu item when project is loaded
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
source/item_definitions/core/item_definition_resolver.cpp (3)
285-300: Semantic mismatch:missing_in_otbused for "DAT not in XML" in DatOnly mode.In
resolveDatOnly(), unreferenced DAT items are added tomissing_in_otb, but this mode has no OTB at all. This reuses the bucket in a semantically different way, which could confuse users viewing the report (the UI labels will say "OTB" when OTB isn't used).The UI does adapt labels based on
hasOtb, but the underlying data structure naming is misleading. This is acceptable as-is given the UI handles it, but consider a comment clarifying this dual-purpose usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 285 - 300, The code is populating missingReport->missing_in_otb with DAT entries in resolveDatOnly(), which semantically misuses the "OTB" bucket when no OTB exists; update the code around the loop that inspects fragments.dat (the block that checks isDatItemEmptyOrInvalid, dat_ids_used_by_xml and pushes MissingItemEntry into missing_in_otb) to include a clear comment that missing_in_otb is intentionally reused as a generic "not referenced in XML" bucket in DatOnly mode (or alternatively create and push to a new descriptive field); reference resolveDatOnly(), missing_in_otb, MissingItemEntry, fragments.dat and dat_ids_used_by_xml so reviewers can locate and apply the comment or rename/refactor accordingly.
156-174: Potential false positives inxml_no_otbcollection.The loop collects XML entries that reference non-existent OTB server IDs. However, when an XML entry specifies a
client_idoverride that differs from the server_id, this entry could be legitimately not in OTB while still being valid as a client-id mapping override for an existing OTB entry.Additionally, the skip logic at lines 159-165 filters
server_id <= 0andserver_id < 100, but these checks seem redundant sinceserver_id < 100already coversserver_id <= 0.Simplify redundant checks
for (const auto& [server_id, xml] : fragments.xml) { - // Skip XML entries with invalid IDs - if (server_id <= 0) { - continue; - } // Skip server-side fluid types and special items (IDs < 100) if (server_id < 100) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 156 - 174, The xml_no_otb list currently reports XML entries when their server_id isn't in fragments.otb, but this produces false positives for entries that specify a client_id override that maps to an existing OTB entry; update the loop that iterates fragments.xml to first skip server_id < 100 (remove the redundant server_id <= 0 check) and also skip adding a MissingItemEntry when xml.client_id.has_value() and fragments.otb.contains(xml.client_id.value()) (i.e., treat client_id overrides that resolve to an existing OTB entry as valid), while otherwise constructing MissingItemEntry (server_id, client_id via xml.client_id.value_or(0), name, description) and pushing to missingReport->xml_no_otb.
111-131: Inconsistent "missing or unusable DAT" handling between resolve paths.In
resolveDatOtb(), when DAT exists but is empty/invalid (according toisDatItemEmptyOrInvalid), the code still processes it and adds todat_ids_referenced_by_otb(line 130). However, inresolveDatOnly()(lines 248-253), empty/invalid DAT items are silently skipped and not reported.This creates an asymmetry:
resolveDatOtb: Empty DAT items are treated as valid mappingsresolveDatOnly: Empty DAT items silently skip XML entries without reportingConsider whether empty/invalid DAT entries should be reported in
missing_in_datwhen they exist but are unusable, for consistency between both paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 111 - 131, The two resolver paths are inconsistent: resolveDatOtb currently treats DAT entries that exist but are empty/invalid as valid (it inserts into dat_ids_referenced_by_otb and proceeds), while resolveDatOnly skips and reports them; to fix, update resolveDatOtb to call isDatItemEmptyOrInvalid for the found DAT entry and handle unusable entries the same way as resolveDatOnly — i.e., if isDatItemEmptyOrInvalid(effective_client_id) is true, add a MissingItemEntry (server_id, client_id, otb.name, otb.description) to missingReport->missing_in_dat or set error and return false when missingReport is null, and do NOT insert into dat_ids_referenced_by_otb or apply flags; keep resolveDatOnly behavior unchanged so both paths report unusable DATs consistently.source/app/managers/version_manager.cpp (1)
140-185: Consider bounded output for very large missing item lists.When
SHOW_MISSING_ITEMS_WARNINGis enabled and there are thousands of missing items, the warnings list could become extremely large, potentially impacting performance or usability of any UI that displays these warnings.Consider adding a cap (e.g., first N items per category) with a note like "...and X more" to keep the warning output manageable while still providing useful diagnostic information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/managers/version_manager.cpp` around lines 140 - 185, The warnings builder in version_manager.cpp (inside the block guarded by g_settings.getBoolean(Config::SHOW_MISSING_ITEMS_WARNING)) unboundedly pushes every missing entry from last_missing_items.* into warnings; cap output per-category (e.g., int MAX_SHOW = 50) by iterating only the first MIN(size, MAX_SHOW) entries for missing_in_dat, missing_in_otb, xml_no_otb, and otb_no_xml, and after each category append a summary line like "...and X more" when size > MAX_SHOW; preserve the original top-level total_missing count and the existing header lines in warnings, and reference the existing symbols last_missing_items, warnings, and Config::SHOW_MISSING_ITEMS_WARNING when making the change.source/ui/dialogs/missing_items_dialog.h (1)
10-19: Consider forward declarations or moving includes to cpp.Several
<wx/*.h>headers (clipboard, filedlg, textfile, stream, txtstrm, wfstream) are only needed for implementation details in the cpp file, not for the class declaration. Moving these to the cpp file would reduce header dependencies and potentially improve compile times.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/dialogs/missing_items_dialog.h` around lines 10 - 19, The header includes heavy implementation-only wx headers which should be moved to the .cpp: remove `#include` lines for <wx/clipbrd.h>, <wx/filedlg.h>, <wx/textfile.h>, <wx/stream.h>, <wx/txtstrm.h>, and <wx/wfstream.h> from source/ui/dialogs/missing_items_dialog.h and add minimal forward declarations (e.g., forward-declare any wx classes referenced in the class declaration of MissingItemsDialog such as wxClipboard, wxFileDialog, wxTextFile, wxInputStream/ wxOutputStream or other specific types used) so the header only depends on declarations; then include the removed wx headers in missing_items_dialog.cpp where the actual clipboard/file/stream/text operations are implemented. Ensure all symbols used in inline methods or member variables remain declared in the header via forward declarations or by keeping only the necessary lightweight wx headers (like <wx/wx.h>) if required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/ui/dialogs/missing_items_dialog.cpp`:
- Around line 217-223: MissingItemsDialog::OnCopyToClipboard currently silently
skips when wxTheClipboard->Open() returns false; update this method to show an
error message to the user when wxTheClipboard->Open() fails (e.g., call
wxMessageBox with a descriptive error and wxICON_ERROR), then return early; keep
the successful branch behavior (SetData, Close, success message) unchanged and
ensure you only call wxTheClipboard->SetData(...) when Open() succeeded.
- Around line 92-117: The code creates page3, sizer3, countXmlNoOtb and
listXmlNoOtb unconditionally but only adds page3 to notebook when hasOtb,
causing a leak; fix by moving the entire creation/population block for page3
(the newd calls and the for loop that appends items and sets the label) inside
the if (hasOtb) branch so those widgets are only created when
notebook->AddPage(...) will be called, or alternatively delete page3 (and its
owned children) when hasOtb is false; reference symbols: page3, sizer3,
countXmlNoOtb, listXmlNoOtb, notebook, report.xml_no_otb.
---
Nitpick comments:
In `@source/app/managers/version_manager.cpp`:
- Around line 140-185: The warnings builder in version_manager.cpp (inside the
block guarded by g_settings.getBoolean(Config::SHOW_MISSING_ITEMS_WARNING))
unboundedly pushes every missing entry from last_missing_items.* into warnings;
cap output per-category (e.g., int MAX_SHOW = 50) by iterating only the first
MIN(size, MAX_SHOW) entries for missing_in_dat, missing_in_otb, xml_no_otb, and
otb_no_xml, and after each category append a summary line like "...and X more"
when size > MAX_SHOW; preserve the original top-level total_missing count and
the existing header lines in warnings, and reference the existing symbols
last_missing_items, warnings, and Config::SHOW_MISSING_ITEMS_WARNING when making
the change.
In `@source/item_definitions/core/item_definition_resolver.cpp`:
- Around line 285-300: The code is populating missingReport->missing_in_otb with
DAT entries in resolveDatOnly(), which semantically misuses the "OTB" bucket
when no OTB exists; update the code around the loop that inspects fragments.dat
(the block that checks isDatItemEmptyOrInvalid, dat_ids_used_by_xml and pushes
MissingItemEntry into missing_in_otb) to include a clear comment that
missing_in_otb is intentionally reused as a generic "not referenced in XML"
bucket in DatOnly mode (or alternatively create and push to a new descriptive
field); reference resolveDatOnly(), missing_in_otb, MissingItemEntry,
fragments.dat and dat_ids_used_by_xml so reviewers can locate and apply the
comment or rename/refactor accordingly.
- Around line 156-174: The xml_no_otb list currently reports XML entries when
their server_id isn't in fragments.otb, but this produces false positives for
entries that specify a client_id override that maps to an existing OTB entry;
update the loop that iterates fragments.xml to first skip server_id < 100
(remove the redundant server_id <= 0 check) and also skip adding a
MissingItemEntry when xml.client_id.has_value() and
fragments.otb.contains(xml.client_id.value()) (i.e., treat client_id overrides
that resolve to an existing OTB entry as valid), while otherwise constructing
MissingItemEntry (server_id, client_id via xml.client_id.value_or(0), name,
description) and pushing to missingReport->xml_no_otb.
- Around line 111-131: The two resolver paths are inconsistent: resolveDatOtb
currently treats DAT entries that exist but are empty/invalid as valid (it
inserts into dat_ids_referenced_by_otb and proceeds), while resolveDatOnly skips
and reports them; to fix, update resolveDatOtb to call isDatItemEmptyOrInvalid
for the found DAT entry and handle unusable entries the same way as
resolveDatOnly — i.e., if isDatItemEmptyOrInvalid(effective_client_id) is true,
add a MissingItemEntry (server_id, client_id, otb.name, otb.description) to
missingReport->missing_in_dat or set error and return false when missingReport
is null, and do NOT insert into dat_ids_referenced_by_otb or apply flags; keep
resolveDatOnly behavior unchanged so both paths report unusable DATs
consistently.
In `@source/ui/dialogs/missing_items_dialog.h`:
- Around line 10-19: The header includes heavy implementation-only wx headers
which should be moved to the .cpp: remove `#include` lines for <wx/clipbrd.h>,
<wx/filedlg.h>, <wx/textfile.h>, <wx/stream.h>, <wx/txtstrm.h>, and
<wx/wfstream.h> from source/ui/dialogs/missing_items_dialog.h and add minimal
forward declarations (e.g., forward-declare any wx classes referenced in the
class declaration of MissingItemsDialog such as wxClipboard, wxFileDialog,
wxTextFile, wxInputStream/ wxOutputStream or other specific types used) so the
header only depends on declarations; then include the removed wx headers in
missing_items_dialog.cpp where the actual clipboard/file/stream/text operations
are implemented. Ensure all symbols used in inline methods or member variables
remain declared in the header via forward declarations or by keeping only the
necessary lightweight wx headers (like <wx/wx.h>) if required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cbf1e16-f26d-4a14-8c8e-d1ba525be0fb
📒 Files selected for processing (5)
source/app/managers/version_manager.cppsource/item_definitions/core/item_definition_resolver.cppsource/ui/dialogs/missing_items_dialog.cppsource/ui/dialogs/missing_items_dialog.hsource/ui/menubar/menubar_action_manager.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- source/ui/menubar/menubar_action_manager.cpp
- Capped missing items warning output to 50 entries per category with "...and N more" suffix to prevent excessive log output - Fixed resolveDatOtb to accept DAT items with empty mappings and added client_id override validation for XML entries - Removed empty DAT item filtering in resolveDatOnly mode to load all items unconditionally - Made "XML no OTB" tab in MissingItemsDialog conditional on hasOtb flag instead of always building it - Moved wxWidgets includes from missing_items_dialog.h to .cpp, removed unused header includes - Improved clipboard error handling in OnCopyToClipboard with proper open failure check and error message
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
This pull request introduces a comprehensive "Missing Items Report" feature to help identify and resolve discrepancies between DAT, OTB, and XML data files. The implementation is well-structured, integrating cleanly into the existing asset loading pipeline and providing a user-friendly dialog for detailed inspection.
🔍 General Feedback
- Well-Integrated: The collection of missing items is handled within the
ItemDefinitionResolver, ensuring that the report is always accurate based on the current loading mode. - UI Polished: The new dialog provides useful features like clipboard export and saving to a file, which are great for debugging data file issues.
- Performance Consideration: I identified one nested loop in the OTB resolution logic that could become a bottleneck with large data sets; optimizing this with a temporary lookup set is recommended.
- Consistency: The changes follow established patterns in the codebase, such as the use of
wxNotebookandwxDataViewListCtrlfor reports.
| AssetBundle bundle; | ||
| AssetBundleLoader bundle_loader; | ||
| if (!bundle_loader.load(asset_request, bundle, error, warnings)) { | ||
| error = "Couldn't load canonical asset bundle: " + error; |
There was a problem hiding this comment.
🟡 Resetting last_load_has_otb to true on failure might be inconsistent if the failed load was in DatOnly mode. It would be better to keep the value determined at the start of LoadDataFiles or set it to a more neutral state.
| error = "Couldn't load canonical asset bundle: " + error; | |
| // Clear stale data on failure | |
| last_missing_items = {}; | |
| UnloadVersion(); |
| bool foundInOtb = false; | ||
| for (const auto& [otbServerId, otb] : fragments.otb) { | ||
| if (otb.client_id == xml.client_id.value()) { | ||
| foundInOtb = true; |
There was a problem hiding this comment.
🟡 The nested loop here results in O(N*M) complexity, where N is the number of XML entries and M is the number of OTB entries. Given that these collections can contain thousands of items, this may cause a noticeable delay during data loading.
Consider creating a temporary std::unordered_set<ClientItemId> of all client_id values present in fragments.otb before this loop to allow for O(1) lookups.
// Pre-collect all client IDs present in OTB for fast lookup
std::unordered_set<ClientItemId> otb_client_ids;
for (const auto& [id, otb] : fragments.otb) {
otb_client_ids.insert(otb.client_id);
}
// Collect XML entries that reference non-existent OTB server IDs
for (const auto& [server_id, xml] : fragments.xml) {
// ...
if (xml.client_id.has_value()) {
if (otb_client_ids.contains(xml.client_id.value())) {
continue;
}
}
// ...
}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/item_definitions/core/item_definition_resolver.cpp (1)
227-239:⚠️ Potential issue | 🟠 MajorAvoid duplicate
server_ids in DatOnly mode.This seeds one row for every DAT id and later rewrites
row.server_idfrom XML. If an XML entry mapsserver_idto a differentclient_idand that originalserver_idalso exists in DAT,rowsends up with two definitions for the same server id. Please suppress or collapse the default DAT row before applying that remap so the resolved table stays one-definition-per-server-id.Also applies to: 286-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 227 - 239, The current loop over fragments.dat creates a default ResolvedItemDefinitionRow per DAT id (in rows and client_to_row), but later XML remapping can assign a different client_id to an existing server_id, producing duplicate server_id entries; fix by ensuring DAT defaults are suppressed/merged when a later XML remap reassigns server_id: do not blindly push_back a DAT row for client_id if another entry will claim the same server_id, instead insert into rows via client_to_row only after checking/creating by server_id (use server_id as the canonical key), and when applying the XML remap update or move the existing rows entry (and client_to_row mapping) rather than adding a new one so rows remains one-definition-per-server-id (update code around the fragments.dat loop and the XML remap handling to consult and modify client_to_row and rows by server_id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/app/managers/version_manager.cpp`:
- Around line 147-148: The warning strings currently use a static header and
menu hint that don't reflect the active mode or actual action location; update
the two places that push warnings (the lines that call warnings.push_back with
std::format("Missing item definitions detected...") and the subsequent hint
line) to: 1) choose the header text based on the missing set identifier (e.g.,
use "Not referenced by items.xml" when missing_in_otb represents dat-only items,
otherwise indicate "missing from items.otb"), using the existing variable
missing_in_otb and total_missing to build the message; and 2) change the hint
line to reference the correct menu path/action ("File -> Reload") or
conditionally show the correct menu path matching where the new action was
added; apply the same conditional wording change to the other occurrence noted
(the second push_back around the same pattern).
---
Outside diff comments:
In `@source/item_definitions/core/item_definition_resolver.cpp`:
- Around line 227-239: The current loop over fragments.dat creates a default
ResolvedItemDefinitionRow per DAT id (in rows and client_to_row), but later XML
remapping can assign a different client_id to an existing server_id, producing
duplicate server_id entries; fix by ensuring DAT defaults are suppressed/merged
when a later XML remap reassigns server_id: do not blindly push_back a DAT row
for client_id if another entry will claim the same server_id, instead insert
into rows via client_to_row only after checking/creating by server_id (use
server_id as the canonical key), and when applying the XML remap update or move
the existing rows entry (and client_to_row mapping) rather than adding a new one
so rows remains one-definition-per-server-id (update code around the
fragments.dat loop and the XML remap handling to consult and modify
client_to_row and rows by server_id).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d88a6f8a-b9e9-4073-9c71-c1643df2caaf
📒 Files selected for processing (4)
source/app/managers/version_manager.cppsource/item_definitions/core/item_definition_resolver.cppsource/ui/dialogs/missing_items_dialog.cppsource/ui/dialogs/missing_items_dialog.h
🚧 Files skipped from review as they are similar to previous changes (1)
- source/ui/dialogs/missing_items_dialog.h
| warnings.push_back(std::format("Missing item definitions detected ({} entries total).", total_missing)); | ||
| warnings.push_back("Go to File -> Missing Items Report... for detailed view."); |
There was a problem hiding this comment.
Keep the warning text aligned with the active mode and actual menu path.
missing_in_otb is reused as “not referenced by items.xml” in DatOnly mode, but this header is always rendered as items.otb. The hint line also points to File -> Missing Items Report..., while this PR adds the action under File -> Reload.
Suggested wording fix
- warnings.push_back("Go to File -> Missing Items Report... for detailed view.");
+ warnings.push_back("Go to File -> Reload -> Missing Items Report... for detailed view.");
...
- warnings.push_back(std::format("--- tibia.dat items not in items.otb ({}) ---", last_missing_items.missing_in_otb.size()));
+ warnings.push_back(std::format("--- tibia.dat items not in {} ({}) ---",
+ last_load_has_otb ? "items.otb" : "items.xml",
+ last_missing_items.missing_in_otb.size()));Also applies to: 166-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/app/managers/version_manager.cpp` around lines 147 - 148, The warning
strings currently use a static header and menu hint that don't reflect the
active mode or actual action location; update the two places that push warnings
(the lines that call warnings.push_back with std::format("Missing item
definitions detected...") and the subsequent hint line) to: 1) choose the header
text based on the missing set identifier (e.g., use "Not referenced by
items.xml" when missing_in_otb represents dat-only items, otherwise indicate
"missing from items.otb"), using the existing variable missing_in_otb and
total_missing to build the message; and 2) change the hint line to reference the
correct menu path/action ("File -> Reload") or conditionally show the correct
menu path matching where the new action was added; apply the same conditional
wording change to the other occurrence noted (the second push_back around the
same pattern).
Reorganized resolveDatOnly into clear 3-step process with improved comments - Moved dat_ids_used_by_xml insertion to duplicate-handling block to avoid tracking unused entries - Consolidated empty DAT item checks into the reporting phase instead of the XML override phase - Removed redundant server_id <= 0 check and invalid ID guard - Simplified missing item collection by deferring empty/invalid filtering to the final loop
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
The pull request introduces a new "Missing Items Report" dialog to identify discrepancies between Tibia's data files (OTB, DAT, and XML). This feature provides essential diagnostic information for map designers to ensure all items are correctly defined. The implementation follows the project's existing architectural patterns and uses modern C++ features consistently.
🔍 General Feedback
-
Good Categorization: Using
wxNotebookto separate discrepancies by their source (DAT missing in OTB/XML vs. OTB/XML missing in DAT) makes the report very easy to navigate. -
DPI Awareness: Proper use of
FromDIPin the new UI components ensures compatibility with high-resolution displays. - Configuration Control: Adding a preference to toggle automatic warnings while keeping the detailed report available via the menu is a thoughtful UX choice.
-
Performance Consideration: One nested loop in the loading phase was identified as a potential performance bottleneck (
$O(N \cdot M)$ complexity) and has been suggested for optimization using a lookup set. - Code Cleanliness: A minor unused include in the new dialog file was noted for removal.
| // Collect XML entries that reference non-existent OTB server IDs | ||
| for (const auto& [server_id, xml] : fragments.xml) { | ||
| // Skip server-side fluid types and special items (IDs < 100) | ||
| if (server_id < 100) { |
There was a problem hiding this comment.
🟡 This nested loop results in fragments.otb can be large, this may cause a noticeable delay during data loading.
Consider pre-building a std::unordered_set<ClientItemId> of all client IDs present in OTB to make this check
| if (server_id < 100) { | |
| // Pre-build a set of client IDs referenced by OTB for fast lookup | |
| std::unordered_set<ClientItemId> otb_client_ids; | |
| otb_client_ids.reserve(fragments.otb.size()); | |
| for (const auto& [id, otb] : fragments.otb) { | |
| otb_client_ids.insert(otb.client_id); | |
| } | |
| // Collect XML entries that reference non-existent OTB server IDs | |
| for (const auto& [server_id, xml] : fragments.xml) { | |
| // Skip server-side fluid types and special items (IDs < 100) | |
| if (server_id < 100) { | |
| continue; | |
| } | |
| // If XML entry has a client_id override that maps to an existing OTB entry, it's valid | |
| if (xml.client_id.has_value()) { | |
| if (otb_client_ids.contains(xml.client_id.value())) { | |
| continue; | |
| } | |
| } |
| #include <format> | ||
| #include <wx/clipbrd.h> | ||
| #include <wx/filedlg.h> | ||
| #include <wx/fdrepdlg.h> |
There was a problem hiding this comment.
🟢 The wx/fdrepdlg.h include (Find/Replace dialog) appears to be unused in this dialog.
| #include <wx/fdrepdlg.h> | |
| #include <wx/filedlg.h> | |
| #include <wx/stream.h> |
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
This pull request introduces a valuable new feature: a "Missing Items Report" dialog that identifies discrepancies between items.otb, tibia.dat, and items.xml files. The implementation is comprehensive, integrating the report collection into the existing data loading pipeline and providing a user-friendly UI for viewing and exporting the results.
🔍 General Feedback
- Well-Structured: The addition of the
MissingItemReportstruct and its integration intoItemDefinitionResolveris clean and follows the project's architecture. - User Control: Adding a preference setting to toggle detailed warnings on load is a good decision, as it prevents clutter for users who don't need it while keeping the information accessible.
- Performance: I've suggested an optimization for a nested loop in the report generation to ensure it scales well with large item definitions.
- Maintainability: The use of modern C++ features (like
std::format) is consistent with recent changes in the codebase.
| // Collect XML entries that reference non-existent OTB server IDs | ||
| for (const auto& [server_id, xml] : fragments.xml) { | ||
| // Skip server-side fluid types and special items (IDs < 100) | ||
| if (server_id < 100) { | ||
| continue; | ||
| } | ||
| // If XML entry has a client_id override that maps to an existing OTB entry, it's valid | ||
| if (xml.client_id.has_value()) { | ||
| bool foundInOtb = false; | ||
| for (const auto& [otbServerId, otb] : fragments.otb) { | ||
| if (otb.client_id == xml.client_id.value()) { | ||
| foundInOtb = true; | ||
| break; | ||
| } | ||
| } | ||
| if (foundInOtb) { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The nested loop here is O(N*M) where N is the number of XML entries and M is the number of OTB entries. This can be significantly slow when both collections are large.
I suggest pre-calculating a set of all OTB client IDs before starting the XML loop to reduce the complexity to O(N+M).
| // Collect XML entries that reference non-existent OTB server IDs | |
| for (const auto& [server_id, xml] : fragments.xml) { | |
| // Skip server-side fluid types and special items (IDs < 100) | |
| if (server_id < 100) { | |
| continue; | |
| } | |
| // If XML entry has a client_id override that maps to an existing OTB entry, it's valid | |
| if (xml.client_id.has_value()) { | |
| bool foundInOtb = false; | |
| for (const auto& [otbServerId, otb] : fragments.otb) { | |
| if (otb.client_id == xml.client_id.value()) { | |
| foundInOtb = true; | |
| break; | |
| } | |
| } | |
| if (foundInOtb) { | |
| continue; | |
| } | |
| } | |
| // Collect XML entries that reference non-existent OTB server IDs | |
| std::unordered_set<ClientItemId> all_otb_client_ids; | |
| for (const auto& [id, otb] : fragments.otb) { | |
| all_otb_client_ids.insert(otb.client_id); | |
| } | |
| for (const auto& [server_id, xml] : fragments.xml) { | |
| // Skip server-side fluid types and special items (IDs < 100) | |
| if (server_id < 100) { | |
| continue; | |
| } | |
| // If XML entry has a client_id override that maps to an existing OTB entry, it's valid | |
| if (xml.client_id.has_value()) { | |
| if (all_otb_client_ids.contains(xml.client_id.value())) { | |
| continue; | |
| } | |
| } |
| wxString::FromUTF8(std::format("{}", entry.client_id)), | ||
| wxString::FromUTF8(entry.name.empty() ? "unknown" : entry.name), | ||
| wxString::FromUTF8(entry.description) | ||
| }); |
There was a problem hiding this comment.
🟢 The button label "Ignore" is a bit unusual for closing a report dialog. "Close" or "OK" would be more idiomatic for a report/view-only dialog.
| }); | |
| auto* ignoreButton = newd wxButton(this, wxID_ANY, "Close"); |
✅ Actions performedFull review triggered. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive 'Missing Items Report' feature, allowing users to identify discrepancies between items.otb, tibia.dat, and items.xml. Key changes include the implementation of a new reporting dialog, menu actions, and logic within the ItemDefinitionResolver to track missing definitions. Feedback focuses on adhering to the project's mandatory C++20/23 standards, specifically recommending the use of designated initializers, ranges, and views to modernize the implementation and the removal of unused includes.
| #include "item_definitions/core/item_definition_resolver.h" | ||
| #include "item_definitions/formats/dat/dat_catalog.h" | ||
|
|
||
| #include <spdlog/spdlog.h> |
There was a problem hiding this comment.
The include <spdlog/spdlog.h> is not used in this file and should be removed to keep the dependencies clean. If you intended to use it for debugging, please ensure it's removed before merging.
| #include <spdlog/spdlog.h> | |
| #include <algorithm> |
| if (catalog) { | ||
| const auto* entry = catalog->entry(client_id); | ||
| if (entry) { | ||
| bool hasValidSprite = (entry->numsprites > 0 && entry->sprite_ids.size() > 0 && entry->sprite_ids[0] != 0); |
There was a problem hiding this comment.
Use !entry->sprite_ids.empty() instead of entry->sprite_ids.size() > 0. This is more idiomatic in C++ and follows the project's goal of using modern C++ patterns.
| bool hasValidSprite = (entry->numsprites > 0 && entry->sprite_ids.size() > 0 && entry->sprite_ids[0] != 0); | |
| bool hasValidSprite = (entry->numsprites > 0 && !entry->sprite_ids.empty() && entry->sprite_ids[0] != 0); |
| MissingItemEntry entry; | ||
| entry.server_id = server_id; | ||
| entry.client_id = effective_client_id; | ||
| entry.name = otb.name; | ||
| entry.description = otb.description; | ||
| missingReport->missing_in_dat.push_back(std::move(entry)); |
There was a problem hiding this comment.
Use designated initializers for aggregate initialization. This is a core C++20 feature and is explicitly required by the repository style guide (Rule 94) for better readability and safety.
| MissingItemEntry entry; | |
| entry.server_id = server_id; | |
| entry.client_id = effective_client_id; | |
| entry.name = otb.name; | |
| entry.description = otb.description; | |
| missingReport->missing_in_dat.push_back(std::move(entry)); | |
| missingReport->missing_in_dat.push_back({ | |
| .server_id = server_id, | |
| .client_id = effective_client_id, | |
| .name = otb.name, | |
| .description = otb.description | |
| }); |
References
- Designated initializers are mandatory for new code in this repository. (link)
| bool foundInOtb = false; | ||
| for (const auto& [otbServerId, otb] : fragments.otb) { | ||
| if (otb.client_id == xml.client_id.value()) { | ||
| foundInOtb = true; | ||
| break; | ||
| } | ||
| } | ||
| if (foundInOtb) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This manual search loop can be replaced with std::ranges::any_of for a more concise and modern C++20 approach, adhering to the project's mandatory C++20/23 requirement.
if (std::ranges::any_of(fragments.otb, [&](const auto& p) { return p.second.client_id == xml.client_id.value(); })) {
continue;
}| #include "ui/tool_options_window.h" | ||
| #include "item_definitions/core/asset_bundle_loader.h" | ||
| #include "item_definitions/core/item_definition_store.h" | ||
| #include "app/settings.h" |
There was a problem hiding this comment.
Include <ranges> to support modern C++20 range views and adaptors used in the loading logic.
| #include "app/settings.h" | |
| #include "app/settings.h" | |
| #include <ranges> |
| size_t show_count = std::min(last_missing_items.missing_in_dat.size(), MAX_SHOW); | ||
| for (size_t i = 0; i < show_count; ++i) { | ||
| const auto& entry = last_missing_items.missing_in_dat[i]; | ||
| warnings.push_back(std::format(" Server ID: {}, Client ID: {}, Name: '{}'", | ||
| entry.server_id, entry.client_id, entry.name.empty() ? "unknown" : entry.name)); | ||
| } |
There was a problem hiding this comment.
Use std::views::take to simplify the loop and avoid manual index management. This aligns with the repository's mandatory C++20/23 standard (Rule 17).
for (const auto& entry : last_missing_items.missing_in_dat | std::views::take(MAX_SHOW)) {
warnings.push_back(std::format(" Server ID: {}, Client ID: {}, Name: '{}'",
entry.server_id, entry.client_id, entry.name.empty() ? "unknown" : entry.name));
}References
- Every line of code modified MUST use C++20/23 features. (link)
| #include <format> | ||
| #include <wx/clipbrd.h> | ||
| #include <wx/filedlg.h> | ||
| #include <wx/fdrepdlg.h> |
There was a problem hiding this comment.
The include <wx/fdrepdlg.h> (Find/Replace dialog) is not used in this file and should be removed.
#include <wx/filedlg.h>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
source/app/managers/version_manager.cpp (1)
147-148:⚠️ Potential issue | 🟡 MinorFix the warning copy for
DatOnlymode and the real menu path.Line 148 still points users to
File -> Missing Items Report..., but this action lives underFile -> Reload. Line 166 also hardcodesitems.otb, even thoughmissing_in_otbmeans “not referenced by items.xml” whenlast_load_has_otbisfalse.📝 Proposed fix
- warnings.push_back("Go to File -> Missing Items Report... for detailed view."); + warnings.push_back("Go to File -> Reload -> Missing Items Report... for detailed view."); ... - warnings.push_back(std::format("--- tibia.dat items not in items.otb ({}) ---", last_missing_items.missing_in_otb.size())); + warnings.push_back(std::format("--- tibia.dat items not referenced by {} ({}) ---", + last_load_has_otb ? "items.otb" : "items.xml", + last_missing_items.missing_in_otb.size()));Also applies to: 165-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/managers/version_manager.cpp` around lines 147 - 148, The warning text is wrong for DatOnly loads and the menu path is incorrect; update the two warnings created via warnings.push_back(...) so the menu path reads "File -> Reload -> Missing Items Report..." and, when last_load_has_otb is false (DatOnly mode), change the second message and any mention of "items.otb" to reflect that the missing entries are not referenced by items.xml (e.g., "Not referenced by items.xml" or a DatOnly-specific phrase) using the existing missing_in_otb and last_load_has_otb variables to choose the correct wording instead of hardcoding "items.otb".
🧹 Nitpick comments (1)
source/app/managers/version_manager.cpp (1)
20-20: Include<format>directly in this translation unit.The new
std::formatcalls below currently depend on a transitive include. Please make that dependency explicit here instead of relying on another header to pull it in.🔧 Proposed fix
`#include` <spdlog/spdlog.h> +#include <format> `#include` "ui/gui.h"Also applies to: 147-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/managers/version_manager.cpp` at line 20, Add a direct `#include` <format> to this translation unit so the std::format calls used in this file are not relying on transitive includes; locate the top-of-file includes around the existing `#include` "app/settings.h" and insert the <format> include, and also ensure the same explicit include is present where the other std::format usages occur (the section referenced around the code that formats version strings / messages between lines ~147-203).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/app/preferences/editor_page.cpp`:
- Line 74: Update the helper string that currently reads "File -> Missing Items
Report..." to point to the actual menu location under File -> Reload (e.g.,
change to "File -> Reload -> Missing Items Report..." or "File -> Reload" as
appropriate) in source/app/preferences/editor_page.cpp where the summary string
is defined so the UI hint matches the actual action placement.
In `@source/app/settings.h`:
- Line 88: You added SHOW_MISSING_ITEMS_WARNING into the middle of the settings
enum which shifts subsequent enum values and can corrupt persisted settings;
move the SHOW_MISSING_ITEMS_WARNING identifier out of the middle and append it
to the stable end of the enum (or just before the enum sentinel/explicit last
key used for stability) so existing enum ordering/values for other keys remain
unchanged; update only the enum declaration (the symbol
SHOW_MISSING_ITEMS_WARNING) and do not renumber or reorder any other existing
enum members.
In `@source/item_definitions/core/item_definition_resolver.cpp`:
- Around line 151-163: In resolveDatOtb(), the code currently skips creating an
xml_no_otb row whenever xml.client_id matches any fragments.otb[].client_id;
instead, only skip when the OTB entry matches the XML by server_id. Change the
fast-path so it checks fragments.otb entries against xml.server_id (or remove
the continue) rather than xml.client_id: locate the loop over fragments.otb and
the if (otb.client_id == xml.client_id.value()) condition and replace it with a
comparison to otb.server_id == xml.server_id (or simply stop skipping based on
client_id), ensuring XML entries whose server_id is absent from OTB are still
emitted as xml_no_otb rows.
---
Duplicate comments:
In `@source/app/managers/version_manager.cpp`:
- Around line 147-148: The warning text is wrong for DatOnly loads and the menu
path is incorrect; update the two warnings created via warnings.push_back(...)
so the menu path reads "File -> Reload -> Missing Items Report..." and, when
last_load_has_otb is false (DatOnly mode), change the second message and any
mention of "items.otb" to reflect that the missing entries are not referenced by
items.xml (e.g., "Not referenced by items.xml" or a DatOnly-specific phrase)
using the existing missing_in_otb and last_load_has_otb variables to choose the
correct wording instead of hardcoding "items.otb".
---
Nitpick comments:
In `@source/app/managers/version_manager.cpp`:
- Line 20: Add a direct `#include` <format> to this translation unit so the
std::format calls used in this file are not relying on transitive includes;
locate the top-of-file includes around the existing `#include` "app/settings.h"
and insert the <format> include, and also ensure the same explicit include is
present where the other std::format usages occur (the section referenced around
the code that formats version strings / messages between lines ~147-203).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0699263-7ac3-45b9-8506-1b0f6d127640
📒 Files selected for processing (22)
data/menubar.xmlsource/CMakeLists.txtsource/app/managers/version_manager.cppsource/app/managers/version_manager.hsource/app/preferences/editor_page.cppsource/app/preferences/editor_page.hsource/app/settings.cppsource/app/settings.hsource/item_definitions/core/asset_bundle.hsource/item_definitions/core/asset_bundle_loader.cppsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/item_definition_resolver.hsource/item_definitions/core/item_definitions_loader.cppsource/item_definitions/core/item_definitions_loader.hsource/item_definitions/core/missing_item_report.hsource/ui/dialogs/missing_items_dialog.cppsource/ui/dialogs/missing_items_dialog.hsource/ui/main_menubar.cppsource/ui/main_menubar.hsource/ui/menubar/file_menu_handler.cppsource/ui/menubar/file_menu_handler.hsource/ui/menubar/menubar_action_manager.cpp
| missing_items_warn_chkbox = PreferencesLayout::AddCheckBoxRow( | ||
| safety_section, | ||
| "Show missing items warning on load", | ||
| "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Missing Items Report...", |
There was a problem hiding this comment.
Update menu path in helper text to match actual UI location.
At Line 74, the text says File -> Missing Items Report..., but this action is added under File -> Reload. This can mislead users when they try to find the dialog.
Suggested text fix
- "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Missing Items Report...",
+ "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Reload -> Missing Items Report...",📝 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.
| "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Missing Items Report...", | |
| "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Reload -> Missing Items Report...", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/app/preferences/editor_page.cpp` at line 74, Update the helper string
that currently reads "File -> Missing Items Report..." to point to the actual
menu location under File -> Reload (e.g., change to "File -> Reload -> Missing
Items Report..." or "File -> Reload" as appropriate) in
source/app/preferences/editor_page.cpp where the summary string is defined so
the UI hint matches the actual action placement.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
source/app/settings.h (1)
88-88:⚠️ Potential issue | 🟠 MajorDo not insert this config key in the middle of the enum.
At Line 88, this shifts persisted indices for all following keys and can corrupt settings mapping. Move it to the stable tail section (before
LAST).Suggested fix
WARN_FOR_DUPLICATE_ID, - SHOW_MISSING_ITEMS_WARNING, USE_UPDATER, @@ SEARCH_RESULTS_LIMIT, SHOW_INVALID_TILES, SHOW_INVALID_ZONES, + SHOW_MISSING_ITEMS_WARNING, LAST,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/settings.h` at line 88, The new enum entry SHOW_MISSING_ITEMS_WARNING was inserted in the middle of the settings enum which shifts persisted indices; move the SHOW_MISSING_ITEMS_WARNING identifier out of the main body and place it in the stable tail section just before the LAST enum value (i.e., ensure SHOW_MISSING_ITEMS_WARNING is declared immediately before LAST) so existing enum ordering and persisted mapping remain unchanged.source/app/preferences/editor_page.cpp (1)
71-76:⚠️ Potential issue | 🟡 MinorUpdate helper text to the real menu path.
Line 74 still says
File -> Missing Items Report..., but the action is underFile -> Reload. Please align the hint text to avoid user confusion.Suggested fix
- "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Missing Items Report...", + "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Reload -> Missing Items Report...",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/preferences/editor_page.cpp` around lines 71 - 76, Update the helper text passed to PreferencesLayout::AddCheckBoxRow for missing_items_warn_chkbox to reference the correct menu path; replace the current hint "Display a summary of missing item definitions after loading a map. Items are always tracked and available via File -> Missing Items Report..." with wording that points to "File -> Reload" (or equivalent) so the user sees the accurate location; ensure the change is applied where the call uses g_settings.getBoolean(Config::SHOW_MISSING_ITEMS_WARNING) so only the descriptive string is updated.source/app/managers/version_manager.cpp (1)
147-148:⚠️ Potential issue | 🟡 MinorKeep the warning text aligned with the active mode and actual menu path.
In
DatOnlymode this section is about DAT items not referenced byitems.xml, but the header still saysitems.otb. The hint line also points users to a menu path this PR doesn't add.Suggested wording fix
- warnings.push_back("Go to File -> Missing Items Report... for detailed view."); + warnings.push_back("Go to File -> Reload -> Missing Items Report... for detailed view."); ... - warnings.push_back(std::format("--- tibia.dat items not in items.otb ({}) ---", last_missing_items.missing_in_otb.size())); + warnings.push_back(std::format("--- tibia.dat items not in {} ({}) ---", + last_load_has_otb ? "items.otb" : "items.xml", + last_missing_items.missing_in_otb.size()));Also applies to: 165-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/managers/version_manager.cpp` around lines 147 - 148, The warning messages added via warnings.push_back(...) are incorrect for DatOnly mode: update the header and hint text to reference DAT items and the correct menu path for this PR (not "items.otb" nor "File -> Missing Items Report..."). Locate the two occurrences (the warnings.push_back calls around total_missing at the block that checks for DatOnly mode and the similar block at lines 165-166) and change the first message to mention "DAT item definitions" (or "DAT items not referenced by items.xml") and the second message to point to the actual menu path added by this change; ensure the messages remain formatted with total_missing where used.source/item_definitions/core/item_definition_resolver.cpp (1)
151-163:⚠️ Potential issue | 🟠 MajorDon't treat a reused client id as proof that the XML server id exists in OTB.
This method only emits rows from
fragments.otb. If anitems.xmlentry has no matching OTBserver_idbut happens to reuse another OTB item'sclient_id, this shortcut suppresses thexml_no_otbreport and the mismatch disappears entirely.Suggested change
- // If XML entry has a client_id override that maps to an existing OTB entry, it's valid - if (xml.client_id.has_value()) { - bool foundInOtb = false; - for (const auto& [otbServerId, otb] : fragments.otb) { - if (otb.client_id == xml.client_id.value()) { - foundInOtb = true; - break; - } - } - if (foundInOtb) { - continue; - } - } if (!fragments.otb.contains(server_id)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_resolver.cpp` around lines 151 - 163, The current logic treats any match on otb.client_id as proof that the XML entry exists in OTB, which hides xml_no_otb cases; change the check so a match only counts if the OTB entry’s server_id corresponds to the XML entry’s server_id (i.e., when xml.client_id has_value(), search fragments.otb for an entry where otb.client_id == xml.client_id.value() AND otb.server_id == xml.server_id, or simply require an otb entry with otb.server_id == xml.server_id regardless of client_id), and only then continue; update the code that references xml.client_id, fragments.otb and otb.client_id/otb.server_id accordingly so a reused client_id alone does not suppress the xml_no_otb report.
🧹 Nitpick comments (3)
source/ui/main_menubar.h (1)
164-165: StabilizeActionIDnumeric values to avoid future command-id drift.Line 164 inserts a new enum member in the middle, which renumbers trailing IDs. Since command IDs are derived from these values, prefer explicit fixed values (or append-only IDs) to reduce accidental breakage later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/main_menubar.h` around lines 164 - 165, The ActionID enum is being altered by inserting MISSING_ITEMS_REPORT mid-list which shifts all subsequent numeric IDs (including EXTENSIONS), so stabilize command IDs by giving explicit fixed values: assign explicit integer values to MISSING_ITEMS_REPORT and any affected trailing members (or alternatively make the enum append-only by adding new IDs only at the end), update the ActionID enum definition (referencing the ActionID enum and members MISSING_ITEMS_REPORT and EXTENSIONS) so existing command IDs do not change and ensure any new IDs do not collide with existing values.source/item_definitions/core/item_definitions_loader.cpp (1)
36-39: Reset caller-provided report before collecting new results.When
missingReportis reused by a caller, old entries may bleed into a new load. Clear it at the start ofassemble(...).💡 Proposed fix
fragments = {}; rows.clear(); + if (missingReport) { + *missingReport = {}; + } seedVersionInfoFromClient(input, fragments);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definitions_loader.cpp` around lines 36 - 39, In assemble(...), reset the caller-provided report to avoid leaking previous entries by clearing missingReport at the start of the function (e.g., call missingReport.clear() alongside fragments = {} and rows.clear()) before you call seedVersionInfoFromClient(input, fragments) or begin collecting results; this ensures missingReport is empty when assemble begins.source/ui/dialogs/missing_items_dialog.h (1)
17-19: Don't expose a borrowed report through a public constructor.
reportis stored by reference, soMissingItemsDialog(parent, BuildReport())becomes dangling as soon as construction finishes. Storing the report by value, or making the constructor private and forcing callers throughShow(), would make this API much harder to misuse.Also applies to: 29-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/dialogs/missing_items_dialog.h` around lines 17 - 19, The public constructor MissingItemsDialog(wxWindow* parent, const MissingItemReport& report, bool hasOtb = true) is unsafe because the dialog stores the report by reference; change the API so the dialog owns the report (take MissingItemReport by value and store it by value) or make the constructor(s) private and force creation via the static Show(wxWindow*, const MissingItemReport&, bool) method which should copy/move the report into the dialog; apply the same change to the other constructor overloads mentioned (lines with the same MissingItemsDialog signature) so no public constructor accepts a borrowed reference to MissingItemReport.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/menubar.xml`:
- Around line 51-54: The <item name="Missing Items Report..."
action="MISSING_ITEMS_REPORT"/> element is currently outside the <menu
name="Reload"> block; move that <item> so it is nested inside the Reload menu
(i.e., place the Missing Items Report item before the closing tag of the <menu
name="Reload"> element) so the navigation becomes File -> Reload -> Missing
Items Report; ensure attributes (name, action, help) are preserved.
In `@source/ui/dialogs/missing_items_dialog.cpp`:
- Around line 230-232: The code currently calls wxTheClipboard->SetData(new
wxTextDataObject(GenerateReportText())) then always shows wxMessageBox("Report
copied to clipboard."); change this to check the boolean return of
wxTheClipboard->SetData and only show the success message when it returns true;
if SetData returns false, show an error/warning message instead and ensure you
still call wxTheClipboard->Close(); reference the SetData call, the
wxTextDataObject(GenerateReportText()) construction, and the wxMessageBox
invocation when implementing the conditional flow.
In `@source/ui/menubar/file_menu_handler.cpp`:
- Around line 98-105: The code currently binds missing to
g_version.getLastMissingItems() by const reference which may observe invalidated
state if g_version changes while the dialog is open; create a local value-copy
(e.g. auto missingCopy = g_version.getLastMissingItems()) and also snapshot any
other g_version-derived values used by the dialog (e.g. auto hasOtb =
g_version.lastLoadHasOtb()), then call MissingItemsDialog::Show(g_gui.root,
missingCopy, hasOtb) so the dialog operates on a stable snapshot rather than a
reference into mutable global state.
---
Duplicate comments:
In `@source/app/managers/version_manager.cpp`:
- Around line 147-148: The warning messages added via warnings.push_back(...)
are incorrect for DatOnly mode: update the header and hint text to reference DAT
items and the correct menu path for this PR (not "items.otb" nor "File ->
Missing Items Report..."). Locate the two occurrences (the warnings.push_back
calls around total_missing at the block that checks for DatOnly mode and the
similar block at lines 165-166) and change the first message to mention "DAT
item definitions" (or "DAT items not referenced by items.xml") and the second
message to point to the actual menu path added by this change; ensure the
messages remain formatted with total_missing where used.
In `@source/app/preferences/editor_page.cpp`:
- Around line 71-76: Update the helper text passed to
PreferencesLayout::AddCheckBoxRow for missing_items_warn_chkbox to reference the
correct menu path; replace the current hint "Display a summary of missing item
definitions after loading a map. Items are always tracked and available via File
-> Missing Items Report..." with wording that points to "File -> Reload" (or
equivalent) so the user sees the accurate location; ensure the change is applied
where the call uses g_settings.getBoolean(Config::SHOW_MISSING_ITEMS_WARNING) so
only the descriptive string is updated.
In `@source/app/settings.h`:
- Line 88: The new enum entry SHOW_MISSING_ITEMS_WARNING was inserted in the
middle of the settings enum which shifts persisted indices; move the
SHOW_MISSING_ITEMS_WARNING identifier out of the main body and place it in the
stable tail section just before the LAST enum value (i.e., ensure
SHOW_MISSING_ITEMS_WARNING is declared immediately before LAST) so existing enum
ordering and persisted mapping remain unchanged.
In `@source/item_definitions/core/item_definition_resolver.cpp`:
- Around line 151-163: The current logic treats any match on otb.client_id as
proof that the XML entry exists in OTB, which hides xml_no_otb cases; change the
check so a match only counts if the OTB entry’s server_id corresponds to the XML
entry’s server_id (i.e., when xml.client_id has_value(), search fragments.otb
for an entry where otb.client_id == xml.client_id.value() AND otb.server_id ==
xml.server_id, or simply require an otb entry with otb.server_id ==
xml.server_id regardless of client_id), and only then continue; update the code
that references xml.client_id, fragments.otb and otb.client_id/otb.server_id
accordingly so a reused client_id alone does not suppress the xml_no_otb report.
---
Nitpick comments:
In `@source/item_definitions/core/item_definitions_loader.cpp`:
- Around line 36-39: In assemble(...), reset the caller-provided report to avoid
leaking previous entries by clearing missingReport at the start of the function
(e.g., call missingReport.clear() alongside fragments = {} and rows.clear())
before you call seedVersionInfoFromClient(input, fragments) or begin collecting
results; this ensures missingReport is empty when assemble begins.
In `@source/ui/dialogs/missing_items_dialog.h`:
- Around line 17-19: The public constructor MissingItemsDialog(wxWindow* parent,
const MissingItemReport& report, bool hasOtb = true) is unsafe because the
dialog stores the report by reference; change the API so the dialog owns the
report (take MissingItemReport by value and store it by value) or make the
constructor(s) private and force creation via the static Show(wxWindow*, const
MissingItemReport&, bool) method which should copy/move the report into the
dialog; apply the same change to the other constructor overloads mentioned
(lines with the same MissingItemsDialog signature) so no public constructor
accepts a borrowed reference to MissingItemReport.
In `@source/ui/main_menubar.h`:
- Around line 164-165: The ActionID enum is being altered by inserting
MISSING_ITEMS_REPORT mid-list which shifts all subsequent numeric IDs (including
EXTENSIONS), so stabilize command IDs by giving explicit fixed values: assign
explicit integer values to MISSING_ITEMS_REPORT and any affected trailing
members (or alternatively make the enum append-only by adding new IDs only at
the end), update the ActionID enum definition (referencing the ActionID enum and
members MISSING_ITEMS_REPORT and EXTENSIONS) so existing command IDs do not
change and ensure any new IDs do not collide with existing values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df52f8e1-3054-4b3b-bd2c-63d94bd0f2d9
📒 Files selected for processing (22)
data/menubar.xmlsource/CMakeLists.txtsource/app/managers/version_manager.cppsource/app/managers/version_manager.hsource/app/preferences/editor_page.cppsource/app/preferences/editor_page.hsource/app/settings.cppsource/app/settings.hsource/item_definitions/core/asset_bundle.hsource/item_definitions/core/asset_bundle_loader.cppsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/item_definition_resolver.hsource/item_definitions/core/item_definitions_loader.cppsource/item_definitions/core/item_definitions_loader.hsource/item_definitions/core/missing_item_report.hsource/ui/dialogs/missing_items_dialog.cppsource/ui/dialogs/missing_items_dialog.hsource/ui/main_menubar.cppsource/ui/main_menubar.hsource/ui/menubar/file_menu_handler.cppsource/ui/menubar/file_menu_handler.hsource/ui/menubar/menubar_action_manager.cpp
| <menu name="Reload"> | ||
| <item name="Reload" hotkey="F5" action="RELOAD_DATA" help="Reloads all data files."/> | ||
| <item name="Reload Data Files" hotkey="F5" action="RELOAD_DATA" help="Reloads all data files."/> | ||
| </menu> | ||
| <item name="Missing Items Report..." action="MISSING_ITEMS_REPORT" help="View missing item definitions between data files."/> |
There was a problem hiding this comment.
Missing Items Report... is placed outside the Reload submenu.
If the intended navigation is File -> Reload -> Missing Items Report..., Line 54 should be nested inside the Reload menu block.
💡 Proposed fix
<menu name="Reload">
<item name="Reload Data Files" hotkey="F5" action="RELOAD_DATA" help="Reloads all data files."/>
+ <item name="Missing Items Report..." action="MISSING_ITEMS_REPORT" help="View missing item definitions between data files."/>
</menu>
- <item name="Missing Items Report..." action="MISSING_ITEMS_REPORT" help="View missing item definitions between data files."/>📝 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.
| <menu name="Reload"> | |
| <item name="Reload" hotkey="F5" action="RELOAD_DATA" help="Reloads all data files."/> | |
| <item name="Reload Data Files" hotkey="F5" action="RELOAD_DATA" help="Reloads all data files."/> | |
| </menu> | |
| <item name="Missing Items Report..." action="MISSING_ITEMS_REPORT" help="View missing item definitions between data files."/> | |
| <menu name="Reload"> | |
| <item name="Reload Data Files" hotkey="F5" action="RELOAD_DATA" help="Reloads all data files."/> | |
| <item name="Missing Items Report..." action="MISSING_ITEMS_REPORT" help="View missing item definitions between data files."/> | |
| </menu> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/menubar.xml` around lines 51 - 54, The <item name="Missing Items
Report..." action="MISSING_ITEMS_REPORT"/> element is currently outside the
<menu name="Reload"> block; move that <item> so it is nested inside the Reload
menu (i.e., place the Missing Items Report item before the closing tag of the
<menu name="Reload"> element) so the navigation becomes File -> Reload ->
Missing Items Report; ensure attributes (name, action, help) are preserved.
| wxTheClipboard->SetData(newd wxTextDataObject(GenerateReportText())); | ||
| wxTheClipboard->Close(); | ||
| wxMessageBox("Report copied to clipboard.", "Clipboard", wxOK | wxICON_INFORMATION, this); |
There was a problem hiding this comment.
❓ Verification inconclusive
In wxWidgets, what is the documented failure behavior of wxClipboard::SetData(wxDataObject*), and should callers check its boolean return value before showing a success message?
wxClipboard::SetData(wxDataObject*) is documented to replace (clear) the previous clipboard contents, and—if the call succeeds—the clipboard takes ownership of the wxDataObject* (so the caller must not delete it). (git.amebis.si)
Its failure behavior is the normal wxWidgets one for bool-returning APIs: it returns a “special” value (false) to indicate failure (wxWidgets does not use exceptions for this). (wxwidgets.org) Additionally, clipboard operations can fail depending on platform/selection mode (e.g., using PRIMARY selection on platforms that don’t support it makes “all clipboard operations” fail). (git.amebis.si)
So yes: callers should check the returned bool (and also check Open() first) before showing a “copied to clipboard” success message, otherwise you may report success when nothing was actually placed on the clipboard. (git.amebis.si)
Citations:
- 1: https://git.amebis.si/Amebis/wxWidgets/src/commit/f2d7fdf7b0a57a7e0bf033530148343751b5346b/interface/wx/clipbrd.h
- 2: https://wxwidgets.org/develop/coding-guidelines/?utm_source=openai
- 3: https://git.amebis.si/Amebis/wxWidgets/src/commit/f2d7fdf7b0a57a7e0bf033530148343751b5346b/interface/wx/clipbrd.h
- 4: https://git.amebis.si/Amebis/wxWidgets/src/commit/f2d7fdf7b0a57a7e0bf033530148343751b5346b/interface/wx/clipbrd.h
Script executed:
# Locate and examine the file
fd "missing_items_dialog.cpp" --type fRepository: karolak6612/remeres-map-editor-redux
Repository: karolak6612/remeres-map-editor-redux
Exit code: 0
stdout:
source/ui/dialogs/missing_items_dialog.cpp
Script executed:
# Read the relevant section of the file to confirm the code snippet and context
sed -n '225,240p' source/ui/dialogs/missing_items_dialog.cppRepository: karolak6612/remeres-map-editor-redux
Repository: karolak6612/remeres-map-editor-redux
Exit code: 0
stdout:
wxMessageBox("Failed to open clipboard. Another application may be using it.",
"Clipboard Error", wxOK | wxICON_ERROR, this);
return;
}
wxTheClipboard->SetData(newd wxTextDataObject(GenerateReportText()));
wxTheClipboard->Close();
wxMessageBox("Report copied to clipboard.", "Clipboard", wxOK | wxICON_INFORMATION, this);
}
void MissingItemsDialog::OnSaveReport(wxCommandEvent& WXUNUSED(evt)) {
wxFileDialog saveDlg(this, "Save Report As", "", "missing_items_report.txt",
"Text files (*.txt)|*.txt", wxFD_SAVE | wxFD_OVERWRITE_PROMPT);
if (saveDlg.ShowModal() == wxID_OK) {
wxString path = saveDlg.GetPath();
Check SetData() return value before showing success message.
Open() only acquires the clipboard. SetData() can still fail even after a successful Open() (e.g., on platforms that don't support the requested selection mode). Show the success message only if SetData() returns true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/dialogs/missing_items_dialog.cpp` around lines 230 - 232, The code
currently calls wxTheClipboard->SetData(new
wxTextDataObject(GenerateReportText())) then always shows wxMessageBox("Report
copied to clipboard."); change this to check the boolean return of
wxTheClipboard->SetData and only show the success message when it returns true;
if SetData returns false, show an error/warning message instead and ensure you
still call wxTheClipboard->Close(); reference the SetData call, the
wxTextDataObject(GenerateReportText()) construction, and the wxMessageBox
invocation when implementing the conditional flow.
| const auto& missing = g_version.getLastMissingItems(); | ||
| if (missing.missing_in_dat.empty() && missing.missing_in_otb.empty() && missing.xml_no_otb.empty() && missing.otb_no_xml.empty()) { | ||
| wxMessageBox("No missing item definitions were detected for the current client version.", | ||
| "Missing Items Report", wxOK | wxICON_INFORMATION, g_gui.root); | ||
| return; | ||
| } | ||
| MissingItemsDialog::Show(g_gui.root, missing, g_version.lastLoadHasOtb()); | ||
| } |
There was a problem hiding this comment.
Avoid passing mutable global report by reference into the dialog.
Line 98 binds to g_version state by reference. If missing-items state is replaced while the modal is open, the dialog can observe invalidated data. Snapshot the report before calling Show(...).
💡 Proposed fix
- const auto& missing = g_version.getLastMissingItems();
+ const auto missing = g_version.getLastMissingItems();
if (missing.missing_in_dat.empty() && missing.missing_in_otb.empty() && missing.xml_no_otb.empty() && missing.otb_no_xml.empty()) {
wxMessageBox("No missing item definitions were detected for the current client version.",
"Missing Items Report", wxOK | wxICON_INFORMATION, g_gui.root);
return;
}
MissingItemsDialog::Show(g_gui.root, missing, g_version.lastLoadHasOtb());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/menubar/file_menu_handler.cpp` around lines 98 - 105, The code
currently binds missing to g_version.getLastMissingItems() by const reference
which may observe invalidated state if g_version changes while the dialog is
open; create a local value-copy (e.g. auto missingCopy =
g_version.getLastMissingItems()) and also snapshot any other g_version-derived
values used by the dialog (e.g. auto hasOtb = g_version.lastLoadHasOtb()), then
call MissingItemsDialog::Show(g_gui.root, missingCopy, hasOtb) so the dialog
operates on a stable snapshot rather than a reference into mutable global state.
Replaced index-based loops with std::views::take for slicing missing item entries across version_manager.cpp and item_definition_resolver.cpp source/app/managers/version_manager.cpp — switched to ranges::views::take for truncating displayed warnings; added dynamic label for missing_in_otb based on load mode source/app/settings.cpp — moved SHOW_MISSING_ITEMS_WARNING to end of config section to preserve enum stability source/app/settings.h — repositioned SHOW_MISSING_ITEMS_WARNING enum value to end of list source/item_definitions/core/item_definition_resolver.cpp — replaced std::move with aggregate initialization for MissingItemEntry; used std::ranges::any_of for OTB lookup; added early continue to skip duplicate server_id checks; removed unused spdlog import source/ui/dialogs/missing_items_dialog.cpp — renamed button from "Ignore" to "Close"; removed unused wx/fdrepdlg.h import
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
This PR introduces a valuable feature for identifying and reporting missing item definitions across different data sources (DAT, OTB, and XML). The implementation is well-integrated into the existing UI and loading pipeline.
🔍 General Feedback
- Feature Completeness: The inclusion of both an automatic warning system on load and a detailed manual report dialog is excellent for user experience.
-
Performance: Most logic is sound, but some
$O(N \times M)$ lookups during item resolution should be optimized to ensure responsiveness with large data sets. - Code Quality: The code follows established patterns in the repository. Refactoring the warning generation logic would improve maintainability.
| if (!dat_ids_referenced_by_otb.contains(client_id)) { | ||
| missingReport->missing_in_otb.push_back({ | ||
| .server_id = 0, | ||
| .client_id = client_id, | ||
| .name = "", | ||
| .description = "" | ||
| }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 This nested search has fragments.otb can be large, it is more efficient to pre-calculate a set of client IDs from OTB and use it for lookups.
| if (!dat_ids_referenced_by_otb.contains(client_id)) { | |
| missingReport->missing_in_otb.push_back({ | |
| .server_id = 0, | |
| .client_id = client_id, | |
| .name = "", | |
| .description = "" | |
| }); | |
| } | |
| } | |
| // Pre-index OTB client IDs for faster lookup | |
| std::unordered_set<ClientItemId> otb_client_ids; | |
| for (const auto& [id, otb] : fragments.otb) { | |
| if (otb.client_id != 0) { | |
| otb_client_ids.insert(otb.client_id); | |
| } | |
| } | |
| // Collect XML entries that reference non-existent OTB server IDs | |
| for (const auto& [server_id, xml] : fragments.xml) { | |
| // Skip server-side fluid types and special items (IDs < 100) | |
| if (server_id < 100) { | |
| continue; | |
| } | |
| // If XML server_id already exists in OTB, it's valid | |
| if (fragments.otb.contains(server_id)) { | |
| continue; | |
| } | |
| // If XML entry has a client_id override that maps to an existing OTB entry, it's valid | |
| if (xml.client_id.has_value() && otb_client_ids.contains(xml.client_id.value())) { | |
| continue; | |
| } | |
| missingReport->xml_no_otb.push_back({ | |
| .server_id = server_id, | |
| .client_id = xml.client_id.value_or(0), | |
| .name = xml.name, | |
| .description = xml.description | |
| }); | |
| } |
| auto* copyButton = newd wxButton(this, wxID_ANY, "Copy to Clipboard"); | ||
| copyButton->Bind(wxEVT_BUTTON, &MissingItemsDialog::OnCopyToClipboard, this); | ||
| buttonSizer->Add(copyButton, 0, wxALL, FromDIP(5)); | ||
|
|
There was a problem hiding this comment.
🟢 GenerateReportText is called when copying to clipboard or saving to a file. For very large reports, using a std::string with reserve() or a std::ostringstream would be more efficient than repeated += operations which may cause many reallocations.
| wxString MissingItemsDialog::GenerateReportText() const { | |
| std::basic_ostringstream<wxChar> oss; | |
| oss << "=== Missing Item Definitions Report ===\n\n"; | |
| if (hasOtb) { | |
| oss << wxString::FromUTF8(std::format("--- Items in items.otb missing from tibia.dat ({}) ---\n", report.missing_in_dat.size())); | |
| } else { | |
| oss << wxString::FromUTF8(std::format("--- Items in items.xml missing from tibia.dat ({}) ---\n", report.missing_in_dat.size())); | |
| } | |
| for (const auto& entry : report.missing_in_dat) { | |
| oss << wxString::FromUTF8(std::format("Server ID: {}, Client ID: {}, Name: {}, Description: {}\\n", | |
| entry.server_id, entry.client_id, entry.name, entry.description)); | |
| } | |
| oss << "\n"; | |
| // ... apply same pattern to other sections ... | |
| return oss.str(); | |
| } |
| last_missing_items.missing_in_otb.size() + | ||
| last_missing_items.xml_no_otb.size() + | ||
| last_missing_items.otb_no_xml.size(); | ||
| if (total_missing > 0) { |
There was a problem hiding this comment.
🟡 The logic for generating these warnings is quite verbose and repeated across different item categories. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
| if (total_missing > 0) { | |
| auto add_missing_warnings = [&](const std::string& title, const std::vector<MissingItemEntry>& entries) { | |
| if (entries.empty()) return; | |
| warnings.push_back(std::format("--- {} ({}) ---", title, entries.size())); | |
| for (const auto& entry : entries | std::views::take(MAX_SHOW)) { | |
| if (entry.server_id > 0) { | |
| warnings.push_back(std::format(" Server ID: {}, Client ID: {}, Name: '{}'", | |
| entry.server_id, entry.client_id, entry.name.empty() ? "unknown" : entry.name)); | |
| } else { | |
| warnings.push_back(std::format(" Client ID: {}", entry.client_id)); | |
| } | |
| } | |
| if (entries.size() > MAX_SHOW) { | |
| warnings.push_back(std::format(" ...and {} more", entries.size() - MAX_SHOW)); | |
| } | |
| warnings.push_back(""); | |
| }; | |
| add_missing_warnings("Items missing from tibia.dat", last_missing_items.missing_in_dat); | |
| add_missing_warnings(missing_in_otb_label, last_missing_items.missing_in_otb); | |
| add_missing_warnings("items.xml entries missing from items.otb", last_missing_items.xml_no_otb); | |
| add_missing_warnings("items.otb entries missing from items.xml", last_missing_items.otb_no_xml); |
1 participant
Summary by CodeRabbit
New Features
Preferences
UI Changes