feat(core): add protobuf asset support, npc spawns, and map zones - #1001
feat(core): add protobuf asset support, npc spawns, and map zones#1001karolak6612 wants to merge 13 commits into
Conversation
Integrated Protobuf-based client asset loading, introduced a dedicated NPC spawn system, and implemented map zones with persistent XML storage. Changes: - `CMakeLists.txt` — added Protobuf and LibLZMA dependencies and proto generation - `source/app/client_asset_detector.cpp` — implemented protobuf asset detection logic - `source/brushes/spawn/npc_spawn_brush.cpp` — implemented NPC-specific spawn brush [new] - `source/brushes/zone/zone_brush.cpp` — implemented map zone painting brush [new] - `source/io/map_xml_io.cpp` — implemented XML serialization for NPC spawns and zones - `source/io/otbm/header_serialization_otbm.cpp` — added OTBM 5/6 version support and new file tags - `source/io/xml_file_loader.cpp` — added generic XML inclusion and visiting utility [new] - `source/item_definitions/formats/protobuf/protobuf_item_parser.cpp` — implemented Protobuf appearance parser [new] - `source/map/map.cpp` — added `ZoneRegistry` and NPC spawn/zone management to Map - `source/rendering/core/sprite_archive.cpp` — implemented Protobuf sheet decoding and LZMA decompression - `source/rendering/core/texture_atlas.cpp` — added support for variable-sized sprite packing - `source/ui/tile_properties/zone_property_panel.cpp` — implemented zone management UI [new] - `vcpkg.json` — added `protobuf` and `liblzma` dependencies [minor]
|
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:
📝 WalkthroughWalkthroughAdds Protobuf-supported appearances/catalog parsing, LZMA-backed sprite-sheet decoding, variable-dimension atlas/image pipeline, NPC-spawn and zone data model + I/O/brushes/UI, OTBM v5–v6 support, and build integration for Protobuf and liblzma. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AssetLoader
participant ProtobufParser
participant SpriteArchive
participant AtlasManager
participant GraphicsAssembler
User->>AssetLoader: request load (mode=Protobuf)
AssetLoader->>ProtobufParser: parseCatalog(appearances / catalog-content.json)
ProtobufParser-->>AssetLoader: DatCatalog + sprite_dimensions, fragments
AssetLoader->>SpriteArchive: loadProtobuf(catalog-content.json)
SpriteArchive->>SpriteArchive: parse sheets, build lookup, decode LZMA sheets
SpriteArchive->>AtlasManager: readRGBA(sprite_id) -> (rgba, dimensions)
AtlasManager->>GraphicsAssembler: addSprite(rgba, width, height)
GraphicsAssembler->>User: graphics assembled, load complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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 significantly enhances the core functionality of the map editor by introducing support for modern Protobuf-based client assets. It also expands the map editing capabilities by adding a dedicated NPC spawn system and a zone management system, both of which include persistent XML storage and UI support. These changes required updates to the build system, rendering engine, and map serialization logic to accommodate the new data formats and features. 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
|
![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 comprehensive support for Protobuf-based client assets and implements a new NPC spawn and zone system for OTBM versions 5 and 6. Key architectural changes include the addition of a ProtobufItemParser, support for variable-sized sprites in the rendering pipeline, and a centralized XML loading utility. Feedback focuses on centralizing duplicated asset detection logic to adhere to DRY principles and ensuring that all string formatting utilizes std::format instead of wxString::Format to comply with the project's C++20/23 mandatory standards.
| if (item_definition_mode == ItemDefinitionMode::Protobuf) { | ||
| package_path = wxFileName(client_path.GetFullPath(), "package.json"); | ||
| if (!package_path.FileExists()) { | ||
| return false; | ||
| } | ||
|
|
||
| sprites_path = wxFileName(client_path.GetFullPath() + FileName::GetPathSeparator() + "assets", "catalog-content.json"); | ||
| if (!sprites_path.FileExists()) { | ||
| return false; | ||
| } | ||
|
|
||
| std::ifstream catalog_stream(sprites_path.GetFullPath().ToStdString(), std::ios::in | std::ios::binary); | ||
| if (!catalog_stream.is_open()) { | ||
| return false; | ||
| } | ||
|
|
||
| json::json catalog = json::json::parse(catalog_stream, nullptr, false); | ||
| if (catalog.is_discarded() || !catalog.is_array()) { | ||
| return false; | ||
| } | ||
|
|
||
| std::string appearances_file; | ||
| for (const auto& entry : catalog) { | ||
| if (!entry.is_object() || entry.value("type", std::string {}) != "appearances") { | ||
| continue; | ||
| } | ||
| appearances_file = entry.value("file", std::string {}); | ||
| if (!appearances_file.empty()) { | ||
| break; | ||
| } | ||
| } | ||
| if (appearances_file.empty()) { | ||
| return false; | ||
| } | ||
|
|
||
| metadata_path = wxFileName(sprites_path.GetPath(), wxString::FromUTF8(appearances_file)); | ||
| return metadata_path.FileExists(); | ||
| } |
There was a problem hiding this comment.
This block of code for detecting and parsing Protobuf client assets (package.json, catalog-content.json) appears to be a duplication of the logic already implemented in source/app/client_asset_detector.cpp.
To adhere to the DRY (Don't Repeat Yourself) principle, as mentioned in the repository style guide (rule #11), this logic should be centralized. Consider refactoring this into a shared helper function or modifying ClientAssetDetector to be usable here. This will improve maintainability by ensuring that any future changes to the asset detection logic only need to be made in one place.
References
- Rule Modernize LightDrawer with VBOs and GLEW #11 (DRY): Don't Repeat Yourself — search before coding, reuse existing utilities. The logic for parsing protobuf client metadata is duplicated from
client_asset_detector.cpp. (link) - Don't Repeat Yourself — search before coding, reuse existing utilities. This logic for parsing protobuf asset metadata is duplicated in
client_asset_detector.cpp.
| if (item_definition_mode == ItemDefinitionMode::Protobuf) { | ||
| message = "Could not locate the protobuf client package, catalog-content.json, or appearances file. Please navigate to the protobuf client root for %s.\n"; | ||
| message << "Attempted package file: %s\n"; | ||
| message << "Attempted catalog file: %s\n"; | ||
| message << "Attempted appearances file: %s\n"; | ||
| DialogUtil::PopupDialog("Error", wxString::Format(message, name, package_path.GetFullPath(), sprites_path.GetFullPath(), metadata_path.GetFullPath()), wxOK); | ||
| } else { | ||
| message = "Could not locate metadata and/or sprite files, please navigate to your client assets %s installation folder.\n"; | ||
| message << "Attempted metadata file: %s\n"; | ||
| message << "Attempted sprites file: %s\n"; | ||
| DialogUtil::PopupDialog("Error", wxString::Format(message, name, metadata_path.GetFullPath(), sprites_path.GetFullPath()), wxOK); | ||
| } |
There was a problem hiding this comment.
The use of wxString::Format is discouraged in the project's style guide, which mandates using std::format for all new and modified code (rule #84).
Please refactor this to use std::format for building the error messages. This will align the code with modern C++ standards and project conventions.
const auto message_text = std::format(
"Could not locate the protobuf client package, catalog-content.json, or appearances file. Please navigate to the protobuf client root for {}.\n"
"Attempted package file: {}\n"
"Attempted catalog file: {}\n"
"Attempted appearances file: {}\n",
name, nstr(package_path.GetFullPath()), nstr(sprites_path.GetFullPath()), nstr(metadata_path.GetFullPath())
);
DialogUtil::PopupDialog("Error", wxString::FromUTF8(message_text), wxOK);
} else {
const auto message_text = std::format(
"Could not locate metadata and/or sprite files, please navigate to your client assets {} installation folder.\n"
"Attempted metadata file: {}\n"
"Attempted sprites file: {}\n",
name, nstr(metadata_path.GetFullPath()), nstr(sprites_path.GetFullPath())
);
DialogUtil::PopupDialog("Error", wxString::FromUTF8(message_text), wxOK);References
- Rule 👻 Phantom: Modernize UI Event Handling and Layouts #84: Use
std::formatoversprintf/wxString::Format. The current code useswxString::Formatto build error messages. (link)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
source/rendering/drawers/entities/sprite_drawer.cpp (1)
90-99:⚠️ Potential issue | 🟡 MinorConsider using actual region dimensions for multi-tile sprite positioning instead of assuming TILE_SIZE.
The current code uses
cx * TILE_SIZEto position multi-tile sprite frames, which assumes each grid cell is always 32 pixels. However,region->pixel_widthandregion->pixel_heightcan vary. If frames have non-standard dimensions, this grid-based positioning may cause misalignment. Consider adjusting the positioning logic to account for the actual dimensions of each atlas region.Note: This pattern appears in
creature_drawer.cpp(lines 72, 95) anditem_drawer.cpp(line 205) as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/entities/sprite_drawer.cpp` around lines 90 - 99, The nested loops in sprite_drawer.cpp compute sprite tile positions using cx * TILE_SIZE and cy * TILE_SIZE which misaligns frames when AtlasRegion sizes vary; update the positioning so that when you fetch a region via spr->getAtlasRegion(...) you use that region's pixel_width and pixel_height to compute offsets (or maintain accumulators per row/column of actual widths/heights) before calling glBlitAtlasQuad(sprite_batch, screenx - xOffset, screeny - yOffset, region, color), and apply the same fix for the analogous loops in creature_drawer.cpp and item_drawer.cpp where TILE_SIZE is assumed.source/ui/tile_properties/spawn_property_panel.cpp (2)
35-38:⚠️ Potential issue | 🔴 CriticalMissing
titleargument will cause compilation error.The
SetItemmethod callsSetSpawn(nullptr, tile, map)with only 3 arguments, butSetSpawnnow requires 4 parameters includingconst wxString& title. This will fail to compile.🐛 Proposed fix
void SpawnPropertyPanel::SetItem(Item* /*item*/, Tile* tile, Map* map) { // Not used for items - SetSpawn(nullptr, tile, map); + SetSpawn(nullptr, tile, map, "Spawn Radius"); }Alternatively, provide a default parameter value in the header declaration:
void SetSpawn(Spawn* spawn, Tile* tile, Map* map, const wxString& title = "Spawn Radius");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/spawn_property_panel.cpp` around lines 35 - 38, SpawnPropertyPanel::SetItem currently calls SetSpawn(nullptr, tile, map) but SetSpawn now requires a fourth parameter (const wxString& title); update the call in SetItem to pass an appropriate title string (e.g. "Spawn Radius" or a localized wxString) as the fourth argument, or alternatively add a default value for the title parameter in the SetSpawn declaration (e.g. const wxString& title = "Spawn Radius") so existing call sites compile without changes; reference SetItem and SetSpawn when making the change.
56-72:⚠️ Potential issue | 🟠 MajorOnRadiusChange only updates
new_tile->spawn, not NPC spawns.When this panel is used for NPC spawns,
OnRadiusChangemodifiesnew_tile->spawn(line 64-65) rather thannew_tile->npc_spawn. This means radius changes won't persist for NPC spawns.🐛 Suggested approach
You'll need to track which spawn type is being edited, then update the appropriate field:
+ // Store which spawn type we're editing + bool is_npc_spawn = false; + // Set this in SetSpawn based on context + void SpawnPropertyPanel::OnRadiusChange(wxSpinEvent& event) { if (current_spawn && current_tile && current_map) { Editor* editor = g_gui.GetCurrentEditor(); if (!editor) { return; } std::unique_ptr<Tile> new_tile = TileOperations::deepCopy(current_tile, *current_map); - if (new_tile->spawn) { - new_tile->spawn->setSize(radius_spin->GetValue()); + Spawn* target_spawn = is_npc_spawn ? new_tile->npc_spawn.get() : new_tile->spawn.get(); + if (target_spawn) { + target_spawn->setSize(radius_spin->GetValue()); std::unique_ptr<Action> action = editor->actionQueue->createAction(ACTION_CHANGE_PROPERTIES); action->addChange(std::make_unique<Change>(std::move(new_tile))); editor->addAction(std::move(action)); g_gui.RefreshView(); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/spawn_property_panel.cpp` around lines 56 - 72, In SpawnPropertyPanel::OnRadiusChange, you currently always call new_tile->spawn->setSize(...); instead detect which spawn is being edited (the panel's current_spawn state or presence of current_tile->npc_spawn) and call setSize on the matching field (new_tile->spawn or new_tile->npc_spawn), guarding with null checks (e.g. if (new_tile->npc_spawn) new_tile->npc_spawn->setSize(radius_spin->GetValue()); else if (new_tile->spawn) new_tile->spawn->setSize(...)); keep the rest of the action creation (editor->actionQueue->createAction, action->addChange, editor->addAction) and view refresh unchanged so the correct modified Tile is queued.source/game/creatures.cpp (1)
387-394:⚠️ Potential issue | 🟠 MajorCreate a brush when a duplicate entry is upgraded from a brushless placeholder.
The duplicate path preserves
current->brush, but existing entries can legitimately havebrush == nullptr(for example afterloadFromXML(...)oraddMissingCreatureType(...)). BecauseensureCreatureBrush(...)now only runs on insert, those upgraded creatures/NPCs stay brushless and never become usable from the palette.🛠️ Proposed fix
if (creatureType) { CreatureType* current = (*this)[creatureType->name]; if (current) { CreatureType::preserve_assign_creature_fields(current, *creatureType); + if (!current->brush) { + ensureCreatureBrush(current); + } delete creatureType; } else { creature_map[as_lower_str(creatureType->name)] = creatureType; ensureCreatureBrush(creatureType); } }Also applies to: 418-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/game/creatures.cpp` around lines 387 - 394, When upgrading a duplicate (the branch where current is non-null) you preserve fields via CreatureType::preserve_assign_creature_fields(current, *creatureType) but never call ensureCreatureBrush, leaving current->brush possibly null; after deleting creatureType, call ensureCreatureBrush(current) (or at least call it whenever current->brush == nullptr) so upgraded entries created from brushless placeholders (e.g. from loadFromXML/addMissingCreatureType) get a brush; do the same change for the other duplicate-handling block around the 418-424 region.
🟡 Minor comments (11)
data/clients.toml-1225-1241 (1)
1225-1241:⚠️ Potential issue | 🟡 MinorNew client entry appears to be an incomplete template.
This new
[[clients]]entry fordataDirectory = '1287'has placeholder values:
version = 0otbmVersions = [](empty)datSignature = '0'andsprSignature = '0'description = ''(empty)name = 'New Client'If this is intentional scaffolding for users to customize, consider adding a comment or removing it from the default configuration to avoid confusion. If it's meant to be functional, the values need to be populated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/clients.toml` around lines 1225 - 1241, The new [[clients]] block with dataDirectory '1287' is an incomplete template—fields like version, otbmVersions, datSignature, sprSignature, description, and name are placeholders; either remove this block from the default config or turn it into a commented example and/or populate those fields with real values before committing. Locate the [[clients]] entry with dataDirectory = '1287' and either (A) delete the entire block if it should not ship as default, (B) convert it to a commented example and add a short comment explaining it’s a user template, or (C) replace placeholders (set a proper version, populate otbmVersions, set real signatures, and fill description/name) so the client entry is functional.source/util/nanovg_canvas.cpp-258-268 (1)
258-268:⚠️ Potential issue | 🟡 MinorUse
SPRITE_PIXELSconstant instead of hardcoding 32 for consistency.Lines 260-261 hardcode
32for tile positioning, but the codebase uses theSPRITE_PIXELSconstant elsewhere (e.g.,sprite_icon_generator.cpp). Replace the hardcoded values with the constant:Fix
- int part_x = (gs->width - sw - 1) * 32; - int part_y = (gs->height - sh - 1) * 32; + int part_x = (gs->width - sw - 1) * SPRITE_PIXELS; + int part_y = (gs->height - sh - 1) * SPRITE_PIXELS;(Ensure
app/definitions.his included forSPRITE_PIXELS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/util/nanovg_canvas.cpp` around lines 258 - 268, Replace the hardcoded tile size 32 used to compute part_x and part_y with the SPRITE_PIXELS constant to match the rest of the codebase; update the calculations in nanovg_canvas.cpp where part_x and part_y are defined (currently using (gs->width - sw - 1) * 32 and (gs->height - sh - 1) * 32) to use SPRITE_PIXELS instead, and ensure app/definitions.h is `#included` so SPRITE_PIXELS is available.source/rendering/drawers/overlays/marker_drawer.cpp-45-55 (1)
45-55:⚠️ Potential issue | 🟡 MinorSpawn overlays now collide on tiles that carry both spawn types.
If a tile has both
tile->spawnandtile->npc_spawn, this draws the sameSPRITE_SPAWNat the same coordinates twice, so the NPC marker hides the regular spawn marker. A dedicated NPC sprite, a small offset, or a combined-state overlay would keep both states visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/marker_drawer.cpp` around lines 45 - 55, The code draws SPRITE_SPAWN twice when a tile has both tile->spawn and tile->npc_spawn, causing the NPC marker to hide the regular spawn; update the marker drawing in marker_drawer.cpp so that when both tile->spawn and tile->npc_spawn are present you either (1) use a distinct sprite constant (e.g., SPRITE_NPC_SPAWN) for the NPC and leave SPRITE_SPAWN for the regular spawn, or (2) draw one of them with a small offset (adjust draw_x/draw_y) or a combined-state overlay so both are visible; implement the chosen approach by changing the calls to drawer->BlitSprite (referencing draw_x, draw_y, SPRITE_SPAWN, tile->npc_spawn, tile->spawn) and ensure selection coloring logic still applies to the NPC variant.source/item_definitions/core/asset_bundle_loader.cpp-37-64 (1)
37-64:⚠️ Potential issue | 🟡 MinorAdd a defensive
defaultbranch.An unsupported
ItemDefinitionModecurrently falls through thisswitchand reachesassemble()with an empty catalog/archive, which makes the failure mode much harder to diagnose than a direct error here.💡 Suggested guard
switch (request.mode) { case ItemDefinitionMode::Protobuf: { ProtobufItemParser protobuf_parser; if (!protobuf_parser.parseCatalog(definition_input, bundle.dat_catalog, error, warnings)) { return false; @@ } break; } + default: { + error = "Unsupported item definition mode."; + return false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/asset_bundle_loader.cpp` around lines 37 - 64, The switch on request.mode in the asset bundle loading logic (the switch handling ItemDefinitionMode::Protobuf / DatOtb / DatOnly / DatSrv) lacks a default branch; add a defensive default that handles unknown ItemDefinitionMode values by setting/augmenting the existing error string (or logging) with a clear message that the mode is unsupported and then return false so assemble() is never called with empty catalog/archive; reference the switch on request.mode and the ItemDefinitionMode enum when implementing this guard and ensure behavior matches the existing error/warning pattern used by parseCatalog and SpriteArchive::load/loadProtobuf.source/game/materials.cpp-87-94 (1)
87-94:⚠️ Potential issue | 🟡 MinorPopulate
visit_errorbefore aborting on an invalid<tileset>.
unserializeTileset()only reports throughwarnings. Returning its bool directly here meansloadMaterials()can fail with an emptyerrorstring, which makes malformedmaterials.xmlfailures hard to diagnose.Suggested change
if (child_name == "tileset") { - return unserializeTileset(child_node, visit_warnings); + if (!unserializeTileset(child_node, visit_warnings)) { + if (visit_error.empty()) { + visit_error = "Could not parse <tileset> entry in materials.xml."; + } + return false; + } + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/game/materials.cpp` around lines 87 - 94, The visitor currently returns the bool from unserializeTileset(child_node, visit_warnings) without setting visit_error, so failures produce no error message; update the visitor lambda so that when unserializeTileset(...) returns false it assigns a descriptive message to visit_error (include context like the child element name "tileset" and source_file or identifier) before returning false; reference the visitor lambda around unserializeTileset, the visit_error and visit_warnings variables, and the call to XmlFileLoader::visitElements so the change is made where the visitor handles the "tileset" child.source/map/tile.h-72-73 (1)
72-73:⚠️ Potential issue | 🟡 MinorFix
memsize()to account fornpc_spawnmemory.
size()andisContentEqual()have been correctly updated to account for bothnpc_spawnandzone_ids, and zone mutators are properly implemented. However,memsize()accounts forzone_ids.capacity()but omits the memory cost of theSpawnobject thatnpc_spawnpoints to. Add corresponding logic to compute and include the spawn's memory footprint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/tile.h` around lines 72 - 73, memsize() currently includes zone_ids.capacity() but omits the memory used by npc_spawn; update the memsize() implementation in the Tile class (the memsize() method) to check if npc_spawn is non-null and add the spawn object's footprint — prefer calling npc_spawn->memsize() if Spawn exposes a memsize() method, otherwise add sizeof(*npc_spawn) (and any dynamic allocations it owns if applicable) to the total along with the existing zone_ids.capacity() accounting.source/io/iomap_otbm.cpp-424-427 (1)
424-427:⚠️ Potential issue | 🟡 MinorError message mentions "OTBM %d" but passes incremented version.
The error message passes
static_cast<int>(map.getVersion().otbm) + 1, which means forMAP_OTBM_4it would display "OTBM 5" as the unsupported version. This could be confusing since the check is< MAP_OTBM_5. Verify this is the intended user-facing message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/iomap_otbm.cpp` around lines 424 - 427, The error text currently prints an incremented OTBM version via static_cast<int>(map.getVersion().otbm) + 1 inside the error(...) call even though the check is map.getVersion().otbm < MAP_OTBM_5; update the error message to accurately reflect the failing condition by either removing the +1 (so it prints the raw enum value from map.getVersion().otbm) or change the message to a clearer phrase like "maps older than OTBM 5" and reference the same symbols (map.getVersion().otbm, MAP_OTBM_5 and the error(...) invocation) so the displayed version matches the check.source/rendering/core/game_sprite.cpp-185-196 (1)
185-196:⚠️ Potential issue | 🟡 MinorUse maximum sprite dimensions across all parts for implicit offset calculation.
The
getDrawOffset()computes implicit offset from onlyspriteList.front()(sprite at index 0), but the codebase handles multi-part sprites with varying dimensions across parts. This is evident from theGetDrawHeight()method (lines 165-168) which explicitly iterates through all parts to computemax_width.Using only the first sprite's dimensions may not correctly represent the overall draw offset for complex multi-part sprites. Consider computing the implicit offset using the maximum dimensions across all relevant sprite parts, similar to how
draw_heightis calculated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/game_sprite.cpp` around lines 185 - 196, getDrawOffset() currently uses only spriteList.front() for implicit offsets; change it to compute implicit_offset_x/implicit_offset_y from the maximum width/height across all sprite parts (skip null entries), mirroring the logic used in GetDrawHeight()/draw_height calculation. Iterate spriteList, track max dimensions, compute implicit offsets as max(0, max_width - SPRITE_PIXELS) and max(0, max_height - SPRITE_PIXELS), then add those to drawoffset_x and drawoffset_y before returning. Ensure you reference GameSprite::getDrawOffset, spriteList, SPRITE_PIXELS, and drawoffset_x/drawoffset_y when locating and updating the code.source/ui/tile_properties/tile_properties_panel.cpp-234-237 (1)
234-237:⚠️ Potential issue | 🟡 MinorZone selection check is inconsistent with other selection patterns.
The zone selection at line 236 checks
!tile->getZones().empty()without anisSelected()check, unlike creature, spawn, and NPC spawn which all verify the selection state. This could cause unexpected panel switching when zones exist but aren't selected.Consider whether zone selection state should be tracked similarly to other selectable entities.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/tile_properties_panel.cpp` around lines 234 - 237, The zone-check currently triggers OnZoneSelected() whenever tile->getZones() is non-empty, causing inconsistent behavior vs. other cases that require isSelected(); change the condition to verify selection state like the others — e.g., inspect tile->getZones() and only call OnZoneSelected() when at least one zone object's isSelected() returns true (or use the tile/zone selection accessor if one exists) so the branch matches the isSelected() pattern used for creature/spawn/npc_spawn and avoid switching panels for unselected zones.source/ui/tile_properties/zone_property_panel.cpp-91-94 (1)
91-94:⚠️ Potential issue | 🟡 MinorMemory leak when zone already exists.
When
new_tile->hasZone(zone_id)is true, the function returns early without usingnew_tile. SinceTileOperations::deepCopyreturns aunique_ptr, it will be destroyed here, but the logic flow suggests checkinghasZonebefore deep copying would be more efficient.♻️ Suggested optimization
+ const uint16_t zone_id = current_map->zones.ensureZone(zone_name); + if (current_tile->hasZone(zone_id)) { + return; + } + - const uint16_t zone_id = current_map->zones.ensureZone(zone_name); auto new_tile = TileOperations::deepCopy(current_tile, *current_map); - if (new_tile->hasZone(zone_id)) { - return; - } new_tile->addZone(zone_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/zone_property_panel.cpp` around lines 91 - 94, The code deep-copies the tile before checking if the zone already exists, causing unnecessary work; move the check to avoid the deep copy: call current_tile->hasZone(zone_id) (using the same zone_id from current_map->zones.ensureZone(zone_name)) before invoking TileOperations::deepCopy so you only create new_tile when the zone is absent; ensure you still return early when hasZone is true and only allocate the unique_ptr when needed.source/palette/palette_creature.cpp-183-188 (1)
183-188:⚠️ Potential issue | 🟡 MinorKeep the NPC radius source of truth in sync.
Lines 184-186 make
npc_spawn_size_spinthe selected size for the NPC brush, butOnUpdateBrushSize()still only updatesspawn_size_spin. After a global brush-size change, this method returns stale data for NPC spawns.🔧 Proposed fix
int CreaturePalettePanel::GetSelectedBrushSize() const { if (npc_spawn_brush_button->GetValue()) { return npc_spawn_size_spin->GetValue(); } return spawn_size_spin->GetValue(); } void CreaturePalettePanel::OnUpdateBrushSize(BrushShape shape, int size) { - return spawn_size_spin->SetValue(size); + if (npc_spawn_brush_button->GetValue()) { + npc_spawn_size_spin->SetValue(size); + } else { + spawn_size_spin->SetValue(size); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/palette/palette_creature.cpp` around lines 183 - 188, GetSelectedBrushSize() reads npc_spawn_size_spin when npc_spawn_brush_button is active but OnUpdateBrushSize() only updates spawn_size_spin, causing stale NPC sizes after global changes; modify OnUpdateBrushSize() (or the central brush-size update path) to also set npc_spawn_size_spin to the same value whenever the global/selected brush size changes (or when npc_spawn_brush_button is active) so both spawn_size_spin and npc_spawn_size_spin stay in sync.
🧹 Nitpick comments (16)
source/item_definitions/core/item_definition_store.cpp (1)
54-57: KeepisValid()in sync with the new storage table.
passiveMetadataJson()indexespassive_metadata_.json_blobs, butisValid()does not check that vector’s size. Todayappend()keeps the tables aligned, but any future drift turns this accessor into an out-of-bounds read instead of returning the empty view.💡 Defensive follow-up
bool ItemDefinitionView::isValid() const { return store_ != nullptr && index_ < store_->identity_.server_ids.size() + && index_ < store_->passive_metadata_.json_blobs.size() && index_ < store_->visual_.client_ids.size() && index_ < store_->editor_.data.size(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/core/item_definition_store.cpp` around lines 54 - 57, The accessor ItemDefinitionView::passiveMetadataJson() can OOB-read passive_metadata_.json_blobs because isValid() doesn't verify that json_blobs is large enough; update the validity check so isValid() also ensures store_->passive_metadata_.json_blobs.size() > index_ (or otherwise that index_ is within bounds of that vector) so passiveMetadataJson() can safely use store_->passive_metadata_.json_blobs[index_]; alternatively, add an explicit bounds check inside passiveMetadataJson() before indexing—refer to isValid(), passiveMetadataJson(), passive_metadata_.json_blobs and append() when making the change.source/brushes/creature/creature_brush.cpp (1)
123-131: Redundant call tousesNpcSpawnSystemon line 131.The function
usesNpcSpawnSystemis called twice: once at line 123 (stored inuse_npc_spawn) and again at line 131. Sincemapandcreature_typehaven't changed, reuse the existinguse_npc_spawnvariable.♻️ Proposed fix to reuse existing variable
tile->creature = std::make_unique<Creature>(creature_type); - tile->creature->setSpawnTime(usesNpcSpawnSystem(map, creature_type) ? g_brush_manager.GetNpcSpawnTime() : g_gui.GetSpawnTime()); + tile->creature->setSpawnTime(use_npc_spawn ? g_brush_manager.GetNpcSpawnTime() : g_gui.GetSpawnTime());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/brushes/creature/creature_brush.cpp` around lines 123 - 131, Replace the redundant call to usesNpcSpawnSystem by reusing the already computed use_npc_spawn variable: where tile->creature->setSpawnTime currently calls usesNpcSpawnSystem(map, creature_type) choose between g_brush_manager.GetNpcSpawnTime() and g_gui.GetSpawnTime() based on use_npc_spawn instead; ensure use_npc_spawn (declared in the if) is in scope or hoist its declaration if necessary so tile->creature->setSpawnTime(...) uses use_npc_spawn to decide the spawn time.source/rendering/core/atlas_manager.h (1)
34-39: Consider removing the default sprite dimensions.Now that variable-size sprites are part of the API, defaulting back to
BASE_SLOT_SIZEcan hide missed call-site updates and silently pack the wrong size. Making dimensions mandatory, or accepting anImageDimensions, would fail fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/atlas_manager.h` around lines 34 - 39, The addSprite declaration currently defaults pixel_width and pixel_height to TextureAtlas::BASE_SLOT_SIZE which can silently accept old call-sites; change the signature of const AtlasRegion* addSprite(uint32_t sprite_id, const uint8_t* rgba_data, int pixel_width = TextureAtlas::BASE_SLOT_SIZE, int pixel_height = TextureAtlas::BASE_SLOT_SIZE); to require explicit dimensions (remove the default values) or replace the two int params with a single ImageDimensions (or similar struct) parameter so callers must supply the intended size; update all callers of addSprite and any related code in TextureAtlas that assumed BASE_SLOT_SIZE defaults to pass explicit sizes or an ImageDimensions instance.source/rendering/ui/selection_controller.cpp (1)
86-94: Extract this selection precedence into a helper.The
spawn -> npc_spawn -> creature -> top itemdecision tree now appears in four branches across three handlers. Centralizing it would keep the new NPC-spawn behavior consistent and reduce the chance of future drift.Also applies to: 136-139, 226-232, 290-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/selection_controller.cpp` around lines 86 - 94, Extract the repeated "spawn -> npc_spawn -> creature -> top item" precedence logic into a single helper (e.g., SelectionController::toggleSelectionAtTile or Editor::toggleSelectionForTile) that takes the tile and handles selection.start(), deciding which entity to target (spawn, npc_spawn, creature, top item), calling editor.selection.add/remove appropriately, calling selection.finish(), and then updateSelectionCount(); then replace the four duplicated branches (the blocks around editor.selection.start()/finish() seen in selection_controller.cpp including the snippets at the original 86-94 and the occurrences around lines 136-139, 226-232, 290-291) with calls to this new helper to centralize behavior and ensure consistent NPC-spawn handling.source/map/tile.cpp (2)
414-416: Consider usingstd::ranges::binary_searchfor sorted vectors.Since
zone_idsis kept sorted,std::ranges::binary_searchwould be more efficient than linearstd::ranges::find.♻️ Use binary search for sorted zone lookup
bool Tile::hasZone(uint16_t zone_id) const { - return std::ranges::find(zone_ids, zone_id) != zone_ids.end(); + return std::ranges::binary_search(zone_ids, zone_id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/tile.cpp` around lines 414 - 416, Tile::hasZone currently uses std::ranges::find on the sorted container zone_ids which is O(n); replace it with std::ranges::binary_search(zone_ids, zone_id) to get O(log n) lookup. Update the implementation of Tile::hasZone to call std::ranges::binary_search and ensure the translation unit includes the appropriate <algorithm> or <ranges> header if needed; leave the function signature and const-qualification unchanged and rely on the already-sorted invariant of zone_ids.
396-404: Consider using sorted insertion instead of sort-after-push.
addZonecurrently pushes, sorts, and deduplicates on every call. If zones are added frequently, consider usingstd::lower_boundfor sorted insertion with duplicate check to avoid repeated full sorts.♻️ Optional optimization using binary insertion
void Tile::addZone(uint16_t zone_id) { if (zone_id == 0) { return; } - zone_ids.push_back(zone_id); - std::ranges::sort(zone_ids); - zone_ids.erase(std::unique(zone_ids.begin(), zone_ids.end()), zone_ids.end()); + auto it = std::lower_bound(zone_ids.begin(), zone_ids.end(), zone_id); + if (it == zone_ids.end() || *it != zone_id) { + zone_ids.insert(it, zone_id); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/tile.cpp` around lines 396 - 404, The addZone function currently pushes then sorts+deduplicates on every call; change it to perform a binary search insertion: keep the early return for zone_id == 0, then use std::lower_bound on the member vector zone_ids to find the insertion point, check if the value at that iterator equals zone_id (if so do nothing), otherwise insert zone_id at that iterator position; remove the std::ranges::sort and std::unique calls so repeated full sorts are avoided (use Tile::addZone and zone_ids to find and update the right spot).source/io/iomap_otbm.cpp (1)
60-70: Avoidconst_castto iterate over a const Map.The
const_castto get a mutableMap&for iteration is a code smell. IfMapIteratorrequires non-const access, consider either makinghasZoneAssignmentstake a non-const reference, or better, adding a const-compatible iteration mechanism toMap.♻️ Proposed fix: take non-const Map reference
namespace { - bool hasZoneAssignments(const Map& map) { - Map& mutable_map = const_cast<Map&>(map); - for (MapIterator it = mutable_map.begin(); it != mutable_map.end(); ++it) { + bool hasZoneAssignments(Map& map) { + for (MapIterator it = map.begin(); it != map.end(); ++it) { if (const Tile* tile = it->get(); tile && !tile->getZones().empty()) { return true; } } return false; } }Then update the call site at line 424 to pass a non-const
map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/iomap_otbm.cpp` around lines 60 - 70, The helper hasZoneAssignments currently const_casts the Map to iterate with MapIterator (code smell); fix by removing the const_cast: either change hasZoneAssignments signature to take a non-const Map& and iterate with MapIterator directly (then update its callers to pass a non-const map), or add a const-compatible iterator API to Map (provide begin()/end() const or a Map::const_iterator) and use that in hasZoneAssignments to iterate without casting; reference the hasZoneAssignments function and MapIterator/Map so you can locate and update the implementation and its call sites.source/editor/persistence/editor_persistence.cpp (1)
124-129: Consider adding NPC spawn and zone files to the.saving.txtstatus file.The
.saving.txtfile records backup targets for crash recovery, but only includes the original four files (OTBM, house, spawn, waypoint). The newly addedbackup_spawn_npcandbackup_zoneare not written to this file, which means crash recovery won't be able to restore these auxiliary files if the editor crashes during save.Proposed fix to include all backup targets
std::ofstream f(n.c_str(), std::ios::trunc | std::ios::out); f << backup_otbm << std::endl << backup_house << std::endl << backup_spawn << std::endl - << backup_waypoint << std::endl; + << backup_waypoint << std::endl + << backup_spawn_npc << std::endl + << backup_zone << std::endl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/persistence/editor_persistence.cpp` around lines 124 - 129, The .saving.txt writer currently writes only backup_otbm, backup_house, backup_spawn, and backup_waypoint; update the block in editor_persistence.cpp that constructs and writes file "n" (the .saving.txt writer around variables n and f) to also output backup_spawn_npc and backup_zone so the crash-recovery status file contains all backup targets; locate the write sequence that does f << backup_otbm << backup_house << backup_spawn << backup_waypoint and append the two additional variables in the same style so they are persisted.source/app/client_version.cpp (1)
495-503: Consider closing the file stream explicitly before returning.The
catalog_streamis opened but if parsing fails (e.g.,catalog.is_discarded()), the stream remains open until it goes out of scope. While RAII handles cleanup, it's good practice to close early when done reading, especially before multiple early-return paths.Additionally,
json::json::parsewithallow_exceptions = falsewill return a discarded value on parse error, which is correctly handled. However, the check!catalog.is_array()will also fail for valid JSON that isn't an array—consider logging a more specific error for debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/client_version.cpp` around lines 495 - 503, The code opens catalog_stream and parses it into catalog with json::json::parse but returns early on errors without closing the stream; update the block around catalog_stream, json::json::parse, catalog.is_discarded(), and catalog.is_array() to call catalog_stream.close() before every early return (and after a successful parse when the stream is no longer needed), and replace the generic !catalog.is_array() return with a logged/diagnostic path that closes the stream and emits a specific message indicating the JSON was parsed but not an array (including sprites_path for context) before returning false.source/app/managers/version_manager.cpp (1)
157-170: Creatures loading silently proceeds if both monster/NPC files fail to import.If both
monsters_xml_path.FileExists()andnpcs_xml_path.FileExists()return true but bothimportXMLFromOTcalls fail, the code only adds warnings but continues. This is likely intentional for robustness, but it means the editor may proceed without any creature data loaded.Consider whether this should be an error condition or if the current warning-only approach is acceptable for your use case.
🤖 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 157 - 170, When both monsters_xml_path.FileExists() and npcs_xml_path.FileExists() are true but both g_creatures.importXMLFromOT calls fail, the code only appends warnings and continues; change this to detect the double-failure and treat it as a fatal load error: after the two importXMLFromOT attempts, check whether either succeeded (track booleans like monsters_ok and npcs_ok), and if both are false push a clear error into warnings (or set creatures_error) and return/abort the higher-level load operation instead of proceeding; update the logic around g_creatures.importXMLFromOT and the alternative g_creatures.loadFromXML branch so callers can handle the fatal failure consistently.source/rendering/core/image.cpp (1)
40-46: Potential logic issue:image_dimensionsmay be overwritten unexpectedly.When
preloaded_datais provided with explicitdimensions, but the code falls through togetRGBAData()(line 44),image_dimensionsis unconditionally overwritten withgetDimensions()on line 45. This seems intentional for the!preloaded_databranch, but the structure is slightly confusing.The current logic appears correct since: if
preloaded_datais non-null, line 44 is never reached due to theelsebranch. Consider adding a comment to clarify this flow for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/image.cpp` around lines 40 - 46, The code may look like it overwrites image_dimensions unexpectedly; update the block around ImageDimensions image_dimensions = dimensions.value_or(getDimensions()); and the if (preloaded_data) { ... } else { rgba = getRGBAData(); image_dimensions = getDimensions(); } to make the intent explicit: add a short clarifying comment explaining that image_dimensions is initialized from dimensions (or getDimensions()) but is intentionally refreshed only in the !preloaded_data path because preloaded_data already corresponds to the supplied dimensions; optionally move the image_dimensions initialization into the branches (use dimensions.value_or(getDimensions()) in the preloaded_data branch and getDimensions() in the else) to avoid confusion. Ensure references to ImageDimensions, image_dimensions, dimensions, preloaded_data, getRGBAData(), getDimensions(), and rgba are preserved.source/map/map_spawn_manager.cpp (1)
30-53: Consider extracting the NPC spawn bookkeeping/search logic into shared helpers.These blocks are almost line-for-line copies of the regular spawn paths. The radius math, counter maintenance, and stale-count safety bound are subtle enough that future fixes will drift unless both implementations stay perfectly in sync.
Also applies to: 79-123, 187-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/map_spawn_manager.cpp` around lines 30 - 53, The NPC spawn add/remove logic in MapSpawnManager::addNpcSpawn duplicates the same radius math and per-tile counter updates seen in other spawn paths; extract that repeated logic into a shared helper (e.g., a private MapSpawnManager method) that takes Map&, Tile* (or TileLocation), Spawn* (or spawn size) and an int delta (+1 for add, -1 for remove) and performs the bounds calculation using spawn->getSize(), iterates x/y, calls map.createTileL(x,y,z) and updates the per-tile counter (increaseNpcSpawnCount or its counterpart) with delta, enforcing the stale-count safety bound/clamp so counts never go negative; then replace the loops in MapSpawnManager::addNpcSpawn and the other duplicated blocks with calls to this helper and keep the final map.npc_spawns.addSpawn(tile) call in addNpcSpawn.source/map/map.cpp (2)
81-89:ensureZonedoes not verifyaddZonesucceeded.After calling
addZone(name, id), the return value is ignored. While the logic preceding it should guarantee success, defensive programming would check the return value or use an assertion.💡 Defensive check suggestion
uint16_t ZoneRegistry::ensureZone(const std::string& name) { if (const auto existing = findId(name)) { return *existing; } const uint16_t id = nextFreeId(); - addZone(name, id); + const bool added = addZone(name, id); + ASSERT(added); // Should always succeed after findId check return id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/map.cpp` around lines 81 - 89, The ensureZone function should verify that addZone succeeded instead of ignoring its return; modify ZoneRegistry::ensureZone to check the boolean result of addZone(name, id) (or assert on success) and handle failure (e.g., log/error/throw or assert) so callers cannot get an invalid state—refer to ensureZone, addZone, findId, and nextFreeId when locating the code and add a conditional that reacts to addZone returning false (or use an assert) before returning id.
91-97: Linear scan innextFreeIdmay be slow with many zones.The implementation scans from 1 upward checking each ID. For typical map editor use (dozens to hundreds of zones), this is fine. If zone counts grow significantly, consider caching the next free ID or using a more efficient allocation strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/map.cpp` around lines 91 - 97, nextFreeId currently does a linear scan from 1 using id_to_name.contains(candidate), which can become slow; replace it with an allocation strategy: add a ZoneRegistry member like next_candidate (or a free-ids min-heap/set) and modify methods that create and remove zones (e.g., ZoneRegistry::create/insert and ZoneRegistry::remove/unregister) to allocate the smallest available id by advancing next_candidate while id_to_name contains it (or by popping from the free-id set) and to return freed ids back into the free-id set (or update next_candidate = min(next_candidate, freed_id)); update nextFreeId to return next_candidate (or peek the free-id set) so allocation is O(log n) or amortized O(1) instead of a full linear scan, and ensure all places that previously relied on nextFreeId still call the updated function.source/brushes/zone/zone_brush.cpp (1)
59-63: Clearing all zones when no zone is selected may be unexpected behavior.When
undrawis called without a selected zone (e.g., if the user deselects the zone before right-clicking), all zones are removed from the tile. This "clear all" behavior might surprise users who expect undraw to do nothing when no specific zone is selected.Consider whether a no-op or a confirmation would be more appropriate when no zone is selected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/brushes/zone/zone_brush.cpp` around lines 59 - 63, The undraw path currently clears all zones when no zone is selected (g_brush_manager.GetSelectedZone() returning empty) by calling tile->clearZones(); change this to a no-op instead: in the undraw implementation in zone_brush.cpp, remove or guard the tile->clearZones() call so that if g_brush_manager.GetSelectedZone() is empty you simply return without modifying the tile (or alternatively prompt/confirm before clearing if UX requires), ensuring you only clear or modify zones when a concrete zone name (from GetSelectedZone()) is present.source/io/xml_file_loader.cpp (1)
44-46:visitedset is updated before traversal completes.Both
visitingandvisitedare inserted at line 44-45 before processing children. This means if a recursive include references this file again, it will be skipped as "duplicate" rather than "cyclic". The distinction is minor but affects the warning message accuracy. Consider inserting intovisitedonly after successful traversal (near line 69).💡 More precise tracking
state.visiting.insert(path_key); - state.visited.insert(path_key); for (pugi::xml_node child = root.first_child(); child; child = child.next_sibling()) { // ... processing ... } state.visiting.erase(path_key); + state.visited.insert(path_key); return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/xml_file_loader.cpp` around lines 44 - 46, The code currently inserts path_key into both state.visiting and state.visited before processing includes, which causes recursive re-references to be reported as duplicates rather than cycles; modify the traversal logic so that path_key is inserted into state.visiting at the start of traversal (as currently done) but only insert into state.visited after all child includes are successfully processed (i.e., move the state.visited.insert(path_key) to after traversal completes, around where successful completion is handled, and ensure state.visiting.erase(path_key) still runs on exit/cleanup to maintain correct cycle detection in functions handling include resolution).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cc4e7d7-75a1-4f5d-96bf-c93a2e571201
📒 Files selected for processing (105)
.gitignoreCMakeLists.txtdata/clients.tomlsource/CMakeLists.txtsource/app/client_asset_detector.cppsource/app/client_version.cppsource/app/client_version.hsource/app/managers/version_manager.cppsource/app/preferences/client_version_page.cppsource/app/settings.cppsource/app/settings.hsource/brushes/brush.cppsource/brushes/brush.hsource/brushes/creature/creature_brush.cppsource/brushes/managers/brush_manager.cppsource/brushes/managers/brush_manager.hsource/brushes/spawn/npc_spawn_brush.cppsource/brushes/spawn/npc_spawn_brush.hsource/brushes/zone/zone_brush.cppsource/brushes/zone/zone_brush.hsource/editor/action.cppsource/editor/operations/copy_operations.cppsource/editor/operations/draw_operations.cppsource/editor/operations/selection_operations.cppsource/editor/persistence/editor_persistence.cppsource/game/creatures.cppsource/game/materials.cppsource/io/iomap_otbm.cppsource/io/map_xml_io.cppsource/io/map_xml_io.hsource/io/otbm/header_serialization_otbm.cppsource/io/otbm/item_serialization_otbm.cppsource/io/otbm/otbm_types.hsource/io/otbm/tile_serialization_otbm.cppsource/io/xml_file_loader.cppsource/io/xml_file_loader.hsource/item_definitions/core/asset_bundle_loader.cppsource/item_definitions/core/item_definition_fragments.hsource/item_definitions/core/item_definition_recipe.cppsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/item_definition_store.cppsource/item_definitions/core/item_definition_store.hsource/item_definitions/core/item_definition_types.hsource/item_definitions/core/item_definitions_loader.cppsource/item_definitions/formats/dat/dat_catalog.hsource/item_definitions/formats/dat/dat_item_parser.cppsource/item_definitions/formats/protobuf/protobuf_item_parser.cppsource/item_definitions/formats/protobuf/protobuf_item_parser.hsource/item_definitions/formats/xml/xml_item_parser.cppsource/map/map.cppsource/map/map.hsource/map/map_region.cppsource/map/map_region.hsource/map/map_spawn_manager.cppsource/map/map_spawn_manager.hsource/map/tile.cppsource/map/tile.hsource/map/tile_operations.cppsource/map/tileset.cppsource/palette/palette_creature.cppsource/palette/palette_creature.hsource/rendering/core/atlas_manager.cppsource/rendering/core/atlas_manager.hsource/rendering/core/game_sprite.cppsource/rendering/core/game_sprite.hsource/rendering/core/graphics_assembler.cppsource/rendering/core/graphics_assembler.hsource/rendering/core/image.cppsource/rendering/core/image.hsource/rendering/core/normal_image.cppsource/rendering/core/normal_image.hsource/rendering/core/sprite_archive.cppsource/rendering/core/sprite_archive.hsource/rendering/core/sprite_preloader.cppsource/rendering/core/sprite_preloader.hsource/rendering/core/template_image.cppsource/rendering/core/template_image.hsource/rendering/core/texture_atlas.cppsource/rendering/core/texture_atlas.hsource/rendering/drawers/entities/item_drawer.cppsource/rendering/drawers/entities/sprite_drawer.cppsource/rendering/drawers/overlays/marker_drawer.cppsource/rendering/ui/brush_selector.cppsource/rendering/ui/brush_selector.hsource/rendering/ui/drawing_controller.cppsource/rendering/ui/map_menu_handler.cppsource/rendering/ui/map_menu_handler.hsource/rendering/ui/selection_controller.cppsource/rendering/ui/tooltip_drawer.cppsource/rendering/utilities/sprite_icon_generator.cppsource/rendering/utilities/tile_describer.cppsource/ui/dialog_helper.cppsource/ui/gui_ids.hsource/ui/map_popup_menu.cppsource/ui/tile_properties/spawn_creature_panel.cppsource/ui/tile_properties/spawn_creature_panel.hsource/ui/tile_properties/spawn_property_panel.cppsource/ui/tile_properties/spawn_property_panel.hsource/ui/tile_properties/tile_properties_panel.cppsource/ui/tile_properties/tile_properties_panel.hsource/ui/tile_properties/zone_property_panel.cppsource/ui/tile_properties/zone_property_panel.hsource/util/nanovg_canvas.cppsource/util/nvg_utils.hvcpkg.json
| /assets | ||
| /build-ninja-codex/vcpkg_installed | ||
| /build-ninja-codex | ||
| /protobuf |
There was a problem hiding this comment.
Avoid ignoring the entire /protobuf source directory
Line 130 currently ignores /protobuf wholesale. That can accidentally exclude .proto schema sources from git, which can break reproducibility for CI and other developers. Prefer ignoring only generated outputs (e.g., specific *.pb.* files or build-output paths) instead of the whole directory.
Suggested `.gitignore` adjustment
-/protobuf
+# Keep source .proto files tracked; ignore only generated artifacts (adjust paths as needed)
+/protobuf/**/*.pb.cc
+/protobuf/**/*.pb.h
+/protobuf/**/generated/📝 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.
| /protobuf | |
| # Keep source .proto files tracked; ignore only generated artifacts (adjust paths as needed) | |
| /protobuf/**/*.pb.cc | |
| /protobuf/**/*.pb.h | |
| /protobuf/**/generated/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 130, Do not ignore the entire /protobuf folder; update
.gitignore to stop excluding the source schema files by removing or replacing
the "/protobuf" entry and instead add patterns that only ignore generated
artifacts (for example patterns matching generated protobuf outputs like
"*.pb.go", "*.pb.cc", "*.pb.h", "*.pb.swift", or build/output subdirectories
under protobuf). Locate the "/protobuf" entry in .gitignore and change it to
explicit generated-file patterns or output paths so .proto sources remain
tracked.
| } else if (prop_name == "configType") { | ||
| client->setConfigType(ConfigTypeFromSelection(prop->GetValue().GetLong())); | ||
| RequestClientAssetDetection(*client); | ||
| } else if (prop_name == "clientPath") { |
There was a problem hiding this comment.
Don't let config-type detection rewrite a client after it was just saved.
Changing configType now starts an async detection pass. If the user hits Apply before that worker finishes, the old file fields get persisted first, and the late detection result then mutates the in-memory client afterward. Because those detected updates are not part of the original edit flow, the page can still show Saved while the stored config is already stale. Please either block Apply while detection is pending, or mark the client dirty when detected fields change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/app/preferences/client_version_page.cpp` around lines 937 - 940,
Changing configType triggers async RequestClientAssetDetection which can mutate
the in-memory client after Apply completes; fix by ensuring detected changes
mark the client as dirty so the UI blocks or re-saves them: update
RequestClientAssetDetection (and its detection-completion callback) to, before
mutating fields, set a persistent-modified flag on the Client object (e.g., call
client->SetModified(true) or emit the same ClientChanged/MarkDirty signal used
by the manual edit flow) and trigger the same UI update path that
enables/disables the Apply button (the code that OnApply()/ApplyEnabled()
consults), and keep the setConfigType call as-is (ConfigTypeFromSelection /
client->setConfigType) but ensure the detection code uses the same
dirty/notification mechanism so late detection cannot silently overwrite saved
state.
| size_t& search_hint = single_slot ? small_search_hint_ : large_search_hint_; | ||
| for (;;) { | ||
| const size_t search_limit = static_cast<size_t>(layer_count_) * SLOTS_PER_LAYER; | ||
| for (size_t linear_index = search_hint; linear_index < search_limit; ++linear_index) { | ||
| const int layer = static_cast<int>(linear_index / SLOTS_PER_LAYER); | ||
| const int slot_index = static_cast<int>(linear_index % SLOTS_PER_LAYER); | ||
| const int slot_y = slot_index / SLOTS_PER_ROW; | ||
| const int slot_x = slot_index % SLOTS_PER_ROW; | ||
|
|
||
| if (isAreaFree(layer, slot_x, slot_y, slot_width, slot_height)) { | ||
| search_hint = linear_index + 1; | ||
| return Placement { | ||
| .layer = layer, | ||
| .slot_x = slot_x, | ||
| .slot_y = slot_y, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| if (!addLayer()) { | ||
| return std::nullopt; | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrap the placement scan before growing the atlas.
Lines 195-217 only search from the current hint to search_limit. After a 2x1/2x2 region is freed below large_search_hint_, it is never revisited, so the atlas starts allocating new layers even though reusable space still exists.
♻️ Proposed fix
size_t& search_hint = single_slot ? small_search_hint_ : large_search_hint_;
+ auto try_range = [&](size_t begin, size_t end) -> std::optional<Placement> {
+ for (size_t linear_index = begin; linear_index < end; ++linear_index) {
+ const int layer = static_cast<int>(linear_index / SLOTS_PER_LAYER);
+ const int slot_index = static_cast<int>(linear_index % SLOTS_PER_LAYER);
+ const int slot_y = slot_index / SLOTS_PER_ROW;
+ const int slot_x = slot_index % SLOTS_PER_ROW;
+
+ if (isAreaFree(layer, slot_x, slot_y, slot_width, slot_height)) {
+ search_hint = linear_index + 1;
+ return Placement {
+ .layer = layer,
+ .slot_x = slot_x,
+ .slot_y = slot_y,
+ };
+ }
+ }
+ return std::nullopt;
+ };
+
for (;;) {
const size_t search_limit = static_cast<size_t>(layer_count_) * SLOTS_PER_LAYER;
- for (size_t linear_index = search_hint; linear_index < search_limit; ++linear_index) {
- const int layer = static_cast<int>(linear_index / SLOTS_PER_LAYER);
- const int slot_index = static_cast<int>(linear_index % SLOTS_PER_LAYER);
- const int slot_y = slot_index / SLOTS_PER_ROW;
- const int slot_x = slot_index % SLOTS_PER_ROW;
-
- if (isAreaFree(layer, slot_x, slot_y, slot_width, slot_height)) {
- search_hint = linear_index + 1;
- return Placement {
- .layer = layer,
- .slot_x = slot_x,
- .slot_y = slot_y,
- };
- }
- }
+ if (auto placement = try_range(search_hint, search_limit)) {
+ return placement;
+ }
+ if (search_hint != 0) {
+ if (auto placement = try_range(0, std::min(search_hint, search_limit))) {
+ return placement;
+ }
+ }
+ search_hint = 0;
if (!addLayer()) {
return std::nullopt;
}
}📝 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.
| size_t& search_hint = single_slot ? small_search_hint_ : large_search_hint_; | |
| for (;;) { | |
| const size_t search_limit = static_cast<size_t>(layer_count_) * SLOTS_PER_LAYER; | |
| for (size_t linear_index = search_hint; linear_index < search_limit; ++linear_index) { | |
| const int layer = static_cast<int>(linear_index / SLOTS_PER_LAYER); | |
| const int slot_index = static_cast<int>(linear_index % SLOTS_PER_LAYER); | |
| const int slot_y = slot_index / SLOTS_PER_ROW; | |
| const int slot_x = slot_index % SLOTS_PER_ROW; | |
| if (isAreaFree(layer, slot_x, slot_y, slot_width, slot_height)) { | |
| search_hint = linear_index + 1; | |
| return Placement { | |
| .layer = layer, | |
| .slot_x = slot_x, | |
| .slot_y = slot_y, | |
| }; | |
| } | |
| } | |
| if (!addLayer()) { | |
| return std::nullopt; | |
| } | |
| } | |
| size_t& search_hint = single_slot ? small_search_hint_ : large_search_hint_; | |
| auto try_range = [&](size_t begin, size_t end) -> std::optional<Placement> { | |
| for (size_t linear_index = begin; linear_index < end; ++linear_index) { | |
| const int layer = static_cast<int>(linear_index / SLOTS_PER_LAYER); | |
| const int slot_index = static_cast<int>(linear_index % SLOTS_PER_LAYER); | |
| const int slot_y = slot_index / SLOTS_PER_ROW; | |
| const int slot_x = slot_index % SLOTS_PER_ROW; | |
| if (isAreaFree(layer, slot_x, slot_y, slot_width, slot_height)) { | |
| search_hint = linear_index + 1; | |
| return Placement { | |
| .layer = layer, | |
| .slot_x = slot_x, | |
| .slot_y = slot_y, | |
| }; | |
| } | |
| } | |
| return std::nullopt; | |
| }; | |
| for (;;) { | |
| const size_t search_limit = static_cast<size_t>(layer_count_) * SLOTS_PER_LAYER; | |
| if (auto placement = try_range(search_hint, search_limit)) { | |
| return placement; | |
| } | |
| if (search_hint != 0) { | |
| if (auto placement = try_range(0, std::min(search_hint, search_limit))) { | |
| return placement; | |
| } | |
| } | |
| search_hint = 0; | |
| if (!addLayer()) { | |
| return std::nullopt; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/texture_atlas.cpp` around lines 195 - 217, The
placement scan only searches from the current search_hint to search_limit and
never revisits earlier slots, causing premature addLayer() calls; modify the
loop in the allocation routine that uses search_hint (set to small_search_hint_
or large_search_hint_) so it first scans from search_hint to search_limit and if
nothing is found then wraps and scans from 0 up to the original search_hint
before calling addLayer(); keep updating search_hint to linear_index+1 when
isAreaFree(...) finds a spot and return the Placement (layer, slot_x, slot_y) as
before, and leave addLayer() as the fallback only after the full wrapped scan
fails.
|
🤖 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. |
|
🤖 I'm sorry @karolak6612, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
source/ui/map_popup_menu.cpp (1)
165-170: Deduplicate NPC/Zone menu-item appends to reduce drift risk.Both branches append the same two entries. A small local helper keeps behavior aligned and easier to maintain.
Refactor sketch
+auto appendNpcAndZoneSelectors = [&]() { + if (topNpcSpawn) { + Append(MAP_POPUP_MENU_SELECT_NPC_SPAWN_BRUSH, "Select NPC Spawn", "Select the NPC spawn brush") + ->SetBitmap(IMAGE_MANAGER.GetBitmap(ICON_USER, wxSize(16, 16))); + } + if (hasZones) { + Append(MAP_POPUP_MENU_SELECT_ZONE_BRUSH, "Select Zone", "Select the zone brush for this tile") + ->SetBitmap(IMAGE_MANAGER.GetBitmap(ICON_MARKER, wxSize(16, 16))); + } +}; ... - if (topNpcSpawn) { ... } - if (hasZones) { ... } + appendNpcAndZoneSelectors(); ... - if (topNpcSpawn) { ... } - if (hasZones) { ... } + appendNpcAndZoneSelectors();Also applies to: 221-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/map_popup_menu.cpp` around lines 165 - 170, The two branches duplicate the same Append(...)->SetBitmap(...) pattern for menu items (MAP_POPUP_MENU_SELECT_NPC_SPAWN_BRUSH / MAP_POPUP_MENU_SELECT_ZONE_BRUSH with IMAGE_MANAGER.GetBitmap using ICON_USER / ICON_MARKER); extract a small local helper (lambda or private function) that accepts the id, label, help text and icon constant, calls Append(id, label, help)->SetBitmap(IMAGE_MANAGER.GetBitmap(icon, wxSize(16,16))), and replace the duplicated Append calls with calls to that helper (apply the same refactor to the other duplicated block around the MAP_POPUP_MENU entries at the second location).source/app/managers/version_manager.cpp (1)
38-40: Unused helper functiondisplayName.This function is defined but never called in the file. Consider removing it to avoid dead code, or use it in the warning/error messages where file paths are displayed (e.g., lines 241, 244, 248, 273).
🤖 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 38 - 40, The helper function displayName is unused; either remove this dead function or apply it when forming file-path strings in the module's logging calls (replace direct filename.GetFullPath().ToStdString() or fallback uses in the warning/error message sites with displayName(filename, fallback_name)) so file display logic is centralized; ensure you update call sites that currently format file paths to call displayName(const wxFileName&, const char*) or delete the function if no calls are needed.source/map/tile.cpp (2)
533-535: Consider batch-friendly zone addition for bulk operations.The current implementation sorts and deduplicates after each
addZonecall. If multiple zones are added in sequence (e.g., during tile copy or deserialization), this results in repeated sorting. Consider adding a batch variant or deferring deduplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/tile.cpp` around lines 533 - 535, The code currently sorts and deduplicates zone_ids on every addition (zone_ids.push_back(zone_id); std::ranges::sort(...); zone_ids.erase(std::unique(...), ...);), causing O(n log n) work per add; change to support batch-friendly additions by either adding a new method (e.g., addZonesBatch(const std::vector<ZoneId>&) or addZoneNoSort(ZoneId)) that only push_backs without sorting, and introduce a single finalizeZones() or dedupeZones() helper that runs std::ranges::sort(zone_ids) and zone_ids.erase(std::unique(...), ...) once after bulk inserts; alternatively add an optional bool parameter to addZone(ZoneId, bool deferDedup=false) to skip immediate sort when true and call dedupeZones() later; update callers (tile copy/deserialization paths) to use the batch/no-sort path and call finalize/dedupe when done.
528-536: Consider usingstd::ranges::binary_searchinhasZonesincezone_idsis kept sorted.Since
addZonemaintains sorted order,hasZonecould use binary search for O(log n) lookups instead of O(n) linear search. This is a minor optimization given that zone counts per tile are typically small.♻️ Optional optimization
bool Tile::hasZone(uint16_t zone_id) const { - return std::ranges::find(zone_ids, zone_id) != zone_ids.end(); + return std::ranges::binary_search(zone_ids, zone_id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/tile.cpp` around lines 528 - 536, hasZone currently does a linear search over zone_ids even though Tile::addZone keeps zone_ids sorted; replace the linear scan with std::ranges::binary_search(zone_ids, zone_id) in hasZone to make lookups O(log n). Keep the existing behavior (return true if found, false otherwise) and ensure you include <algorithm> or <ranges> as needed and reference zone_ids and the hasZone method when making the 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/item_definitions/core/item_definition_resolver.cpp`:
- Around line 85-87: The protobuf branch incorrectly reuses resolveDatOnly which
emits dat-only/DAT/XML diagnostics; update the Protobuf case so it calls a
Protobuf-aware resolver (e.g., implement and call resolveProtobuf or extend
resolveDatOnly to accept an ItemDefinitionMode parameter) instead of
resolveDatOnly; ensure the resolver used for ItemDefinitionMode::Protobuf
produces error/warning messages that reference protobuf imports/pipeline and
pass the same arguments (input, fragments, rows, error, warnings, missingReport)
so diagnostics are mode-accurate.
In `@source/item_definitions/core/item_definition_store.cpp`:
- Around line 54-57: The method ItemDefinitionView::passiveMetadataJson()
currently assumes store_->passive_metadata_.json_blobs has an entry at index_,
but ItemDefinitionView::isValid() does not verify that, risking OOB access;
update isValid() to also check that store_->passive_metadata_.json_blobs.size()
> index_ (and that store_ and index_ are valid as currently done) so
passiveMetadataJson() can safely rely on isValid(); locate the isValid() method
and add the additional bounds check against store_->passive_metadata_.json_blobs
before returning true.
In `@source/map/map.cpp`:
- Around line 92-98: ZoneRegistry::nextFreeId can wrap candidate from 65535 to 0
and return an invalid ID; modify nextFreeId to detect wrap-around by remembering
the start value (or checking candidate==0) and stop the loop if no free ID is
found, then handle that case by logging an error and returning a clear failure
(e.g., throw an exception or return an optional/invalid sentinel) instead of
returning 0; also update callers like ensureZone and addZone to handle that
failure path (do not call addZone with ID 0, check for the error and propagate
it).
---
Nitpick comments:
In `@source/app/managers/version_manager.cpp`:
- Around line 38-40: The helper function displayName is unused; either remove
this dead function or apply it when forming file-path strings in the module's
logging calls (replace direct filename.GetFullPath().ToStdString() or fallback
uses in the warning/error message sites with displayName(filename,
fallback_name)) so file display logic is centralized; ensure you update call
sites that currently format file paths to call displayName(const wxFileName&,
const char*) or delete the function if no calls are needed.
In `@source/map/tile.cpp`:
- Around line 533-535: The code currently sorts and deduplicates zone_ids on
every addition (zone_ids.push_back(zone_id); std::ranges::sort(...);
zone_ids.erase(std::unique(...), ...);), causing O(n log n) work per add; change
to support batch-friendly additions by either adding a new method (e.g.,
addZonesBatch(const std::vector<ZoneId>&) or addZoneNoSort(ZoneId)) that only
push_backs without sorting, and introduce a single finalizeZones() or
dedupeZones() helper that runs std::ranges::sort(zone_ids) and
zone_ids.erase(std::unique(...), ...) once after bulk inserts; alternatively add
an optional bool parameter to addZone(ZoneId, bool deferDedup=false) to skip
immediate sort when true and call dedupeZones() later; update callers (tile
copy/deserialization paths) to use the batch/no-sort path and call
finalize/dedupe when done.
- Around line 528-536: hasZone currently does a linear search over zone_ids even
though Tile::addZone keeps zone_ids sorted; replace the linear scan with
std::ranges::binary_search(zone_ids, zone_id) in hasZone to make lookups O(log
n). Keep the existing behavior (return true if found, false otherwise) and
ensure you include <algorithm> or <ranges> as needed and reference zone_ids and
the hasZone method when making the change.
In `@source/ui/map_popup_menu.cpp`:
- Around line 165-170: The two branches duplicate the same
Append(...)->SetBitmap(...) pattern for menu items
(MAP_POPUP_MENU_SELECT_NPC_SPAWN_BRUSH / MAP_POPUP_MENU_SELECT_ZONE_BRUSH with
IMAGE_MANAGER.GetBitmap using ICON_USER / ICON_MARKER); extract a small local
helper (lambda or private function) that accepts the id, label, help text and
icon constant, calls Append(id, label,
help)->SetBitmap(IMAGE_MANAGER.GetBitmap(icon, wxSize(16,16))), and replace the
duplicated Append calls with calls to that helper (apply the same refactor to
the other duplicated block around the MAP_POPUP_MENU entries at the second
location).
🪄 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: 4d33c843-e5f6-4cb4-b797-24c9b1ba7935
📒 Files selected for processing (45)
CMakeLists.txtdata/clients.tomlsource/CMakeLists.txtsource/app/client_asset_detector.cppsource/app/managers/version_manager.cppsource/app/settings.cppsource/app/settings.hsource/brushes/brush.hsource/brushes/managers/brush_manager.cppsource/brushes/managers/brush_manager.hsource/editor/action.cppsource/editor/persistence/editor_persistence.cppsource/io/otbm/item_serialization_otbm.cppsource/io/otbm/tile_serialization_otbm.cppsource/item_definitions/core/asset_bundle_loader.cppsource/item_definitions/core/item_definition_fragments.hsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/item_definition_store.cppsource/item_definitions/core/item_definition_store.hsource/item_definitions/core/item_definition_types.hsource/item_definitions/core/item_definitions_loader.cppsource/item_definitions/formats/dat/dat_item_parser.cppsource/item_definitions/formats/xml/xml_item_parser.cppsource/map/map.cppsource/map/map.hsource/map/map_region.cppsource/map/map_region.hsource/map/tile.cppsource/map/tile.hsource/map/tile_operations.cppsource/palette/palette_creature.cppsource/palette/palette_creature.hsource/protobuf/CMakeLists.txtsource/protobuf/appearances.protosource/rendering/drawers/entities/item_drawer.cppsource/rendering/drawers/overlays/marker_drawer.cppsource/rendering/ui/drawing_controller.cppsource/rendering/ui/map_menu_handler.cppsource/rendering/ui/map_menu_handler.hsource/ui/gui.cppsource/ui/gui_ids.hsource/ui/map_popup_menu.cppsource/ui/tool_options_surface.cppsource/ui/tool_options_surface.hvcpkg.json
✅ Files skipped from review due to trivial changes (9)
- vcpkg.json
- source/app/settings.h
- source/brushes/brush.h
- source/item_definitions/core/item_definition_types.h
- source/CMakeLists.txt
- source/map/map_region.cpp
- source/ui/gui_ids.h
- source/palette/palette_creature.h
- source/palette/palette_creature.cpp
🚧 Files skipped from review as they are similar to previous changes (17)
- source/item_definitions/core/item_definitions_loader.cpp
- source/app/settings.cpp
- source/rendering/drawers/overlays/marker_drawer.cpp
- source/map/tile_operations.cpp
- source/editor/action.cpp
- source/io/otbm/item_serialization_otbm.cpp
- source/item_definitions/core/item_definition_fragments.h
- source/rendering/ui/map_menu_handler.h
- source/item_definitions/core/item_definition_store.h
- source/rendering/ui/map_menu_handler.cpp
- source/app/client_asset_detector.cpp
- source/io/otbm/tile_serialization_otbm.cpp
- source/brushes/managers/brush_manager.h
- source/map/map_region.h
- source/editor/persistence/editor_persistence.cpp
- source/rendering/ui/drawing_controller.cpp
- source/map/tile.h
…lection - `source/app/client_version.cpp` — replaced JSON catalog parsing with `ClientAssetDetector`, switched to `<format>` for error messages - `source/app/managers/version_manager.cpp` — separated monster/NPC loading with combined failure handling, removed `displayName()` helper - `source/brushes/creature/creature_brush.cpp` — cached `usesNpcSpawnSystem()` result to avoid redundant call - `source/brushes/zone/zone_brush.cpp` — added zero zone_id guard, removed unconditional `clearZones()` on empty zone name - `source/editor/persistence/editor_persistence.cpp` — persisted `backup_spawn_npc` and `backup_zone` settings - `source/game/creatures.cpp` — ensured creature brushes are created when importing OT XML without existing brush - `source/game/materials.cpp` — added error reporting for tileset loading failures, switched to `<format>` - `source/io/iomap_otbm.cpp` — removed const-cast in `hasZoneAssignments()`, fixed OTBM version error message off-by-one - `source/io/xml_file_loader.cpp` — deferred marking visited state until after child processing to improve cycle detection - `source/item_definitions/core/asset_bundle_loader.cpp` — added default case with error for unsupported definition modes - `source/item_definitions/core/item_definition_resolver.cpp` — extracted `modeLabel()`/`sourceLabel()` helpers, improved warning messages - `source/item_definitions/core/item_definition_store.cpp` — added `json_blobs` size check to `ItemDefinitionView::isValid()` - `source/map/map.cpp` — added zone ID allocation failure handling, switched to `uint32_t` counter with exhaustion guard - `source/map/map_spawn_manager.cpp` — extracted duplicated spawn coverage logic into `applySpawnCoverage()` helper - `source/map/map_spawn_manager.h` — declared `applySpawnCoverage()` private static method - `source/map/tile.cpp` — optimized zone operations with sorted insertion/binary search, added `npc_spawn` to memory accounting - `source/rendering/core/atlas_manager.cpp` — passed explicit dimensions to `addSprite()` for white pixel - `source/rendering/core/atlas_manager.h` — removed default parameters from `addSprite()` signature - `source/rendering/core/game_sprite.cpp` — computed draw offset from max dimensions across all images instead of first - `source/rendering/core/image.cpp` — added comment explaining preloaded vs on-demand dimension handling - `source/rendering/core/texture_atlas.h` — removed default parameters from `addSprite()` signature - `source/rendering/drawers/entities/creature_drawer.cpp` — accumulated per-tile column/row dimensions for accurate multi-tile sprite positioning - `source/rendering/drawers/entities/item_drawer.cpp` — accumulated per-tile column/row dimensions for accurate multi-tile sprite positioning - `source/rendering/drawers/entities/sprite_drawer.cpp` — accumulated per-tile column/row dimensions for accurate multi-tile sprite positioning - `source/rendering/drawers/overlays/marker_drawer.cpp` — offset NPC spawn indicator when tile also has monster spawn - `source/rendering/ui/selection_controller.cpp` — extracted tile selection target resolution into `TileSelectionTarget` struct with helper functions, reduced duplication - `source/ui/map_popup_menu.cpp` — introduced `appendIconItem()` lambda to consolidate menu item creation - `source/ui/tile_properties/spawn_property_panel.cpp` — resolved correct spawn pointer (monster vs NPC) before editing radius - `source/ui/tile_properties/zone_property_panel.cpp` — added zero zone_id guard after `ensureZone()` call - `source/util/nanovg_canvas.cpp` — replaced hardcoded `32` with `SPRITE_PIXELS` constant, added `definitions.h` include
|
🤖 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
source/item_definitions/core/item_definition_resolver.cpp (1)
261-277:⚠️ Potential issue | 🟠 MajorPopulate
row.volumebefore derived-property finalization.
finalizeDerivedProperties()now promotesITEM_TYPE_NONE+volume > 0intoITEM_TYPE_CONTAINER, but this constructor never copiesdat.volumeinto the resolved row. In DatOnly/Protobuf mode that leaves volume at0, so container inference never fires and the resolved item also loses its DAT volume.🩹 Proposed fix
row.server_id = client_id; row.client_id = client_id; row.group = dat.group; row.type = dat.type; row.flags = dat.flags; + row.volume = dat.volume; row.way_speed = dat.way_speed; row.always_on_top_order = dat.always_on_top_order; if (dat.max_text_len.has_value()) { row.max_text_len = *dat.max_text_len;🤖 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 261 - 277, The loop that builds ResolvedItemDefinitionRow from fragments.dat fails to copy dat.volume into row, so finalizeDerivedProperties() cannot infer container type; update the constructor code in the loop (the block creating ResolvedItemDefinitionRow row for each entry in fragments.dat) to assign row.volume = dat.volume (i.e., copy dat.volume into the row) before any derived-property finalization and before rows.push_back(std::move(row)), ensuring finalizeDerivedProperties() sees the correct volume and container inference succeeds.source/ui/tile_properties/spawn_property_panel.cpp (1)
64-78:⚠️ Potential issue | 🔴 CriticalRe-query
current_tileafter action to avoid stale pointer identity checks.
RefreshView()only redraws the view and does not rebind the panel. Aftereditor->addAction()appliesACTION_CHANGE_PROPERTIES, the tile in the map is replaced with the new copy, butcurrent_tileandcurrent_spawnstill point to the old tile. On the next radius edit, both pointer comparisons fail (sincedeepCopycreates newSpawnobjects),edited_spawnremainsnullptr, and the update silently skips.Follow the pattern used in
zone_property_panel.cpp(lines 101–107): after queuing the action, re-query the tile from the editor's map:const Position position = new_tile->getPosition(); std::unique_ptr<Action> action = editor->actionQueue->createAction(ACTION_CHANGE_PROPERTIES); action->addChange(std::make_unique<Change>(std::move(new_tile))); editor->addAction(std::move(action)); current_tile = editor->map.getTile(position); g_gui.RefreshView();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/spawn_property_panel.cpp` around lines 64 - 78, After queuing and applying the ACTION_CHANGE_PROPERTIES change for new_tile in spawn_property_panel.cpp, re-query the tile from the editor's map and update current_tile before calling g_gui.RefreshView() so pointer identity checks against current_spawn remain valid; specifically, capture the tile Position via new_tile->getPosition(), call editor->addAction(...) as you already do, then set current_tile = editor->map.getTile(position) (so future comparisons like current_spawn == current_tile->npc_spawn.get() or current_tile->spawn.get() succeed) and only then call g_gui.RefreshView().source/rendering/drawers/entities/item_drawer.cpp (1)
157-163:⚠️ Potential issue | 🟠 MajorUse composite pixel bounds for light placement.
The draw path now uses variable
AtlasRegion::pixel_width/pixel_height, butregisterSpriteLight()still derives the light center fromsprite.width/height * TILE_SIZE. For oversized 1x1 assets, that leaves the light visibly shifted relative to the rendered sprite. Please base the light bounds onspr->GetSize()or the same accumulated extents used by the blit path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/entities/item_drawer.cpp` around lines 157 - 163, The light registration currently computes the light center using the old sprite.width/height * TILE_SIZE math, causing misalignment for variable-sized AtlasRegion images; update the call to registerSpriteLight to use the same composite pixel bounds as the blit path by deriving extents from spr->GetSize() (or the accumulated extents used by the blit path) and use those to compute screenx/screeny and the light rectangle passed to registerSpriteLight; keep using light_buffer, view, *spr and item->getLight() but replace the width/height * TILE_SIZE derivation with the spr->GetSize()/composite pixel bounds values so the light is centered on the actual rendered sprite.source/game/materials.cpp (1)
215-236:⚠️ Potential issue | 🟠 Major
materialsextensionlost nested<include>support.
loadExtensions()still feeds the raw<materialsextension>node intounserializeMaterials(), but this function no longer handlesincludechildren. SinceXmlFileLoader::visitElements()only expands includes for callers that use it, any existing extension XML that relies on<include file="...">will now silently skip those entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/game/materials.cpp` around lines 215 - 236, The materialsextension handler lost support for nested <include> because unserializeMaterials() no longer expands include children; update the code so material extension nodes are visited/expanded for includes before parsing: either call XmlFileLoader::visitElements(...) (or the appropriate include-expansion helper) on the materialsextension node in loadExtensions() before passing it to unserializeMaterials(), or restore include-handling inside unserializeMaterials() (detect childName == "include" and resolve/inline the referenced file) so that nested <include file="..."> entries are not skipped; reference functions: loadExtensions, unserializeMaterials, materialsextension and XmlFileLoader::visitElements.
🧹 Nitpick comments (10)
source/ui/map_popup_menu.cpp (1)
163-174: Avoid duplicated selector-entry blocks across both branches.The creature/spawn/NPC/zone append logic is repeated in two places, which increases drift risk when adding/changing entries later.
♻️ Suggested refactor
+const auto appendEntityBrushSelectors = [&]() { + if (topCreature) { + appendIconItem(MAP_POPUP_MENU_SELECT_CREATURE_BRUSH, "Select Creature", "Uses the current creature as a creature brush", ICON_DRAGON); + } + if (topSpawn) { + appendIconItem(MAP_POPUP_MENU_SELECT_SPAWN_BRUSH, "Select Spawn", "Select the spawn brush", ICON_FIRE); + } + if (topNpcSpawn) { + appendIconItem(MAP_POPUP_MENU_SELECT_NPC_SPAWN_BRUSH, "Select NPC Spawn", "Select the NPC spawn brush", ICON_USER); + } + if (hasZones) { + appendIconItem(MAP_POPUP_MENU_SELECT_ZONE_BRUSH, "Select Zone", "Select the zone brush for this tile", ICON_MARKER); + } +}; ... - if (topCreature) { ... } - if (topSpawn) { ... } - if (topNpcSpawn) { ... } - if (hasZones) { ... } + appendEntityBrushSelectors(); ... - if (topCreature) { ... } - if (topSpawn) { ... } - if (topNpcSpawn) { ... } - if (hasZones) { ... } + appendEntityBrushSelectors();Also applies to: 219-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/map_popup_menu.cpp` around lines 163 - 174, The popup menu construction repeats the same appendIconItem calls for creature/spawn/npc/zone entries in multiple branches; extract the repeated block into a single helper function (e.g., addTileBrushItems or appendTileBrushItems) and call it from both places instead of duplicating lines referencing MAP_POPUP_MENU_SELECT_CREATURE_BRUSH, MAP_POPUP_MENU_SELECT_SPAWN_BRUSH, MAP_POPUP_MENU_SELECT_NPC_SPAWN_BRUSH and MAP_POPUP_MENU_SELECT_ZONE_BRUSH; ensure the helper accepts the booleans or context needed (topCreature/topSpawn/topNpcSpawn/hasZones or the tile/context) and reuses appendIconItem with ICON_DRAGON, ICON_FIRE, ICON_USER, ICON_MARKER so behavior and labels remain identical.source/rendering/ui/selection_controller.cpp (1)
62-72: Consider addingSHOW_ITEMSvisibility check for consistency.Spawns and creatures respect their respective visibility settings (
SHOW_SPAWNS,SHOW_CREATURES), but items at line 71 don't checkConfig::SHOW_ITEMS. If this is intentional (e.g., items should always be selectable as the fallback), a brief comment would clarify. Otherwise, consider adding the check for consistency:♻️ Suggested change if visibility gating is desired
- if (Item* item = tile->getTopItem()) { + if (g_settings.getInteger(Config::SHOW_ITEMS)) { + if (Item* item = tile->getTopItem()) { - return TileSelectionTarget { .kind = TileSelectionTargetKind::Item, .item = item }; + return TileSelectionTarget { .kind = TileSelectionTargetKind::Item, .item = item }; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/selection_controller.cpp` around lines 62 - 72, The item branch (returning TileSelectionTarget with TileSelectionTargetKind::Item when Item* item = tile->getTopItem()) lacks the same visibility gating used for spawns/creatures; either wrap that branch with a check using g_settings.getInteger(Config::SHOW_ITEMS) before returning, or if items are intentionally always selectable add a brief comment explaining that exception; update the code near the tile->getTopItem() handling in selection_controller (TileSelectionTarget / TileSelectionTargetKind::Item) to reflect the chosen approach.source/ui/tile_properties/zone_property_panel.cpp (1)
101-107: Extract the repeated “apply tile change” flow into a helper.The mutation/action/remap sequence is duplicated across Add/Remove/Clear. A small helper would reduce drift and make future behavior changes safer.
Also applies to: 134-140, 159-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/zone_property_panel.cpp` around lines 101 - 107, Extract the repeated "apply tile change" flow into a single helper (e.g., applyTileChange or applyTileChangeAndSelect) that accepts the new tile mutation (std::unique_ptr<Tile> new_tile) and an Editor pointer/reference, captures position via new_tile->getPosition(), creates the action using editor->actionQueue->createAction(ACTION_CHANGE_PROPERTIES), calls action->addChange(std::make_unique<Change>(std::move(new_tile))), calls editor->addAction(std::move(action)), and then updates current_map = &editor->map and current_tile = editor->map.getTile(position); then replace the duplicated blocks in the Add/Remove/Clear handlers with calls to this helper (keeping use of Change, ACTION_CHANGE_PROPERTIES, editor->actionQueue, editor->addAction, current_map and current_tile).source/io/iomap_otbm.cpp (3)
525-528: Consider guarding against empty file paths.The
addExtFilelambda writes attributes even when the path is empty (e.g.,map.spawnnpcfileormap.zonefileare empty strings). This may result in empty string attributes in the OTBM file.Optional: Guard against empty paths
if (mapVersion.otbm >= MAP_OTBM_5) { - addExtFile(OTBM_ATTR_EXT_SPAWN_NPC_FILE, map.spawnnpcfile); - addExtFile(OTBM_ATTR_EXT_ZONE_FILE, map.zonefile); + if (!map.spawnnpcfile.empty()) { + addExtFile(OTBM_ATTR_EXT_SPAWN_NPC_FILE, map.spawnnpcfile); + } + if (!map.zonefile.empty()) { + addExtFile(OTBM_ATTR_EXT_ZONE_FILE, map.zonefile); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/iomap_otbm.cpp` around lines 525 - 528, Guard against empty file paths by checking map.spawnnpcfile and map.zonefile before calling addExtFile (or modifying the addExtFile lambda to no-op on empty input); specifically, only invoke addExtFile(OTBM_ATTR_EXT_SPAWN_NPC_FILE, map.spawnnpcfile) and addExtFile(OTBM_ATTR_EXT_ZONE_FILE, map.zonefile) when the respective string is non-empty (when mapVersion.otbm >= MAP_OTBM_5), so you avoid writing empty-string attributes into the OTBM file.
448-454: Same iterator comparison inconsistency.The iterator comparison pattern
map.npc_spawns.begin() != map.npc_spawns.end()should use.empty()for consistency with the zones check at line 462.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/iomap_otbm.cpp` around lines 448 - 454, Change the iterator-based check to use .empty() for consistency: replace the condition in the NPC-spawn save block so it reads "if (!map.spawnnpcfile.empty() || !map.npc_spawns.empty())" instead of checking map.npc_spawns.begin() != map.npc_spawns.end(); keep the rest of the block (g_gui.SetLoadDone, MapXMLIO::saveNpcSpawns call and error handling) unchanged to preserve behavior.
423-426: Performance concern withhasZoneAssignments()for older OTBM versions.The
hasZoneAssignments(map)call performs a full map iteration (lines 62–67) on every save attempt for OTBM < 5. Since it's the rightmost condition in an AND expression, it won't be short-circuited by earlier checks. For large maps, consider reordering conditions to evaluate cheaper checks first:if (map.getVersion().otbm < MAP_OTBM_5 && (!map.zones.empty() || map.npc_spawns.begin() != map.npc_spawns.end() || hasZoneAssignments(map))) {Additionally, the
Spawnsclass does not expose an.empty()method, so the iterator comparison pattern (begin() != end()) is the intended API. Consider adding an.empty()method toSpawnsfor consistency with standard container conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/iomap_otbm.cpp` around lines 423 - 426, Reorder the cheap checks in the OTBM version guard so hasZoneAssignments(map) is evaluated last: change the condition on map.getVersion().otbm to check map.zones.empty() and the npc spawns iterator comparison (map.npc_spawns.begin() != map.npc_spawns.end()) before calling hasZoneAssignments(map) to avoid a full map iteration on every save for OTBM < 5; additionally consider adding an Spawns::empty() method so callers can use map.npc_spawns.empty() instead of begin()!=end() for clarity and consistency.source/map/map_spawn_manager.cpp (2)
5-38:deltaparameter is misleading - only sign matters.The function signature suggests
deltacould be any value, but the implementation only checksdelta > 0ordelta < 0and performs a single increment/decrement regardless of the actual magnitude. Ifdeltais always ±1, consider using a boolean or enum for clarity, or document this constraint.Option 1: Use enum for intent clarity
enum class SpawnCountOperation { Add, Remove }; void MapSpawnManager::applySpawnCoverage(Map& map, Tile* tile, Spawn* spawn, bool create_missing_tiles, SpawnCountOperation op, bool npc) { // ... if (op == SpawnCountOperation::Add) { tile_location->increaseNpcSpawnCount(); } else if (tile_location->getNpcSpawnCount() > 0) { tile_location->decreaseNpcSpawnCount(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/map_spawn_manager.cpp` around lines 5 - 38, The delta parameter of MapSpawnManager::applySpawnCoverage is misleading because only its sign is used; change the API to make intent explicit by replacing int delta with a clearer type (e.g., a boolean like addOperation or an enum SpawnCountOperation { Add, Remove }) and update all callers accordingly; inside applySpawnCoverage switch on the new flag/enum instead of comparing delta, then call TileLocation::increaseNpcSpawnCount()/decreaseNpcSpawnCount() or TileLocation::increaseSpawnCount()/decreaseSpawnCount() based on npc and the operation, preserving the existing guard that prevents decrementing below zero.
172-231: Significant code duplication withgetSpawnList.
getNpcSpawnListis nearly identical togetSpawnList(lines 110-170), differing only in which spawn field is accessed (npc_spawnvsspawn) and which count is checked. Consider refactoring to a template or parameterized helper to reduce duplication and maintenance burden.Refactor sketch using a template approach
template<typename SpawnGetter, typename CountGetter> SpawnList MapSpawnManager::getSpawnListImpl(Map& map, Tile* where, SpawnGetter getSpawn, CountGetter getCount) { SpawnList list; if (!where) return list; TileLocation* tile_loc = where->getLocation(); if (!tile_loc || getCount(tile_loc) == 0) return list; uint32_t found = 0; if (auto* spawn = getSpawn(where)) { ++found; list.push_back(spawn); } // ... rest of algorithm with getSpawn(tile) and getCount(tile_loc) } // Usage: SpawnList MapSpawnManager::getSpawnList(Map& map, Tile* where) { return getSpawnListImpl(map, where, [](Tile* t) { return t->spawn.get(); }, [](TileLocation* loc) { return loc->getSpawnCount(); }); } SpawnList MapSpawnManager::getNpcSpawnList(Map& map, Tile* where) { return getSpawnListImpl(map, where, [](Tile* t) { return t->npc_spawn.get(); }, [](TileLocation* loc) { return loc->getNpcSpawnCount(); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/map_spawn_manager.cpp` around lines 172 - 231, getNpcSpawnList duplicates getSpawnList; extract the common logic into a single helper (e.g., getSpawnListImpl) parameterized by two callables so you can pass the correct accessors for spawn vs npc_spawn and the correct count getter. Implement getSpawnListImpl(Map&, Tile*, SpawnGetter getSpawn, CountGetter getCount) and replace getSpawnList and getNpcSpawnList to call it with lambdas that return t->spawn.get()/t->npc_spawn.get() and loc->getSpawnCount()/loc->getNpcSpawnCount(); preserve the existing algorithm (found counting, checkTile logic, radius limit) but replace direct uses of Tile::spawn/Tile::npc_spawn and TileLocation::getSpawnCount/getNpcSpawnCount with the passed callables.source/brushes/creature/creature_brush.cpp (1)
119-135: Spawn time source inconsistency between NPC and legacy paths.The spawn time is sourced differently:
- NPC spawns:
g_brush_manager.GetNpcSpawnTime()- Legacy spawns:
g_gui.GetSpawnTime()Per
source/ui/gui.cpp(context snippet 3),GUI::SetSpawnTimesets both values simultaneously, so they should typically be synchronized. However, the inconsistent getter sources (g_brush_managervsg_gui) could cause confusion if the values diverge in future code paths. Consider using consistent sources, or document the intentional distinction.Option: Use consistent source
- tile->creature->setSpawnTime(use_npc_spawn ? g_brush_manager.GetNpcSpawnTime() : g_gui.GetSpawnTime()); + tile->creature->setSpawnTime(use_npc_spawn ? g_brush_manager.GetNpcSpawnTime() : g_brush_manager.GetSpawnTime());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/brushes/creature/creature_brush.cpp` around lines 119 - 135, The code in CreatureBrush::draw_creature uses two different getters for spawn time (g_brush_manager.GetNpcSpawnTime() for NPC spawns and g_gui.GetSpawnTime() for legacy spawns) which can diverge; unify the source by always using a single getter when calling tile->creature->setSpawnTime (e.g., replace the ternary use_npc_spawn ? g_brush_manager.GetNpcSpawnTime() : g_gui.GetSpawnTime() with a single call to g_gui.GetSpawnTime()), keeping usesNpcSpawnSystem, Spawn creation, and tile->creature construction unchanged.source/rendering/drawers/entities/creature_drawer.cpp (1)
129-156: CachegetDrawOffset()before entering the atlas loops.
getDrawOffset()now walks the fullspriteList, so calling it inside the inner mount/body loops adds unnecessary work on a hot render path. Grab the offset once per sprite and reuse it here, likeitem_drawer.cppalready does.♻️ Proposed cleanup
+const auto [mount_offset_x, mount_offset_y] = mountSpr->getDrawOffset(); int mount_x_offset = 0; for (int cx = 0; cx != mountSpr->width; ++cx) { int mount_y_offset = 0; for (int cy = 0; cy != mountSpr->height; ++cy) { const AtlasRegion* region = mountSpr->getAtlasRegion(cx, cy, static_cast<int>(dir), 0, 0, mountOutfit, resolvedFrame); if (region) { sprite_drawer->glBlitAtlasQuad( sprite_batch, - screenx - mount_x_offset - mountSpr->getDrawOffset().first, - screeny - mount_y_offset - mountSpr->getDrawOffset().second, + screenx - mount_x_offset - mount_offset_x, + screeny - mount_y_offset - mount_offset_y, region, options.color ); }+const auto [sprite_offset_x, sprite_offset_y] = spr->getDrawOffset(); int sprite_x_offset = 0; for (int cx = 0; cx != spr->width; ++cx) { int sprite_y_offset = 0; for (int cy = 0; cy != spr->height; ++cy) { const AtlasRegion* region = spr->getAtlasRegion(cx, cy, static_cast<int>(dir), pattern_y, pattern_z, outfit, resolvedFrame); if (region) { sprite_drawer->glBlitAtlasQuad( sprite_batch, - screenx - sprite_x_offset - spr->getDrawOffset().first, - screeny - sprite_y_offset - spr->getDrawOffset().second, + screenx - sprite_x_offset - sprite_offset_x, + screeny - sprite_y_offset - sprite_offset_y, region, options.color ); }Also applies to: 175-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/entities/creature_drawer.cpp` around lines 129 - 156, Cache the result of mountSpr->getDrawOffset() in a local variable before the nested loops (both the mount/body blocks that call mountSpr->getDrawOffset()), then use that cached pair when computing the blit positions instead of calling getDrawOffset() inside the inner loop; update references in the glue points around sprite_drawer->glBlitAtlasQuad where you currently subtract mountSpr->getDrawOffset().first/second and remove the repeated calls to getDrawOffset() to avoid walking spriteList on the hot render path.
🤖 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 117-118: items_otb_path currently only checks the root layout
causing missing OTB when files live under items/; update the resolve for the OTB
to probe both nested and root locations by calling resolveVersionDataFile with
the same dual-path list used for items_xml_path (e.g., {"items/items.otb",
"items.otb"}) using base_data_path so items_otb_path will resolve to the nested
items/items.otb when present; adjust any related logic that expects an empty
path accordingly.
In `@source/game/creatures.cpp`:
- Around line 406-417: The loader currently silently skips <npc> entries when
the 'file' attribute is missing or when loading the referenced file fails (see
npcNode, attribute, XmlFileLoader::resolveRelative, npcDoc, npcResult); change
the code to log or report a warning/error instead of just continuing: when
attribute is missing, emit a clear warning including context (the parent
filename and any identifying info from npcNode), and when npcDoc.load_file
fails, log the resolved npcFile path and the parse error
(npcResult.description()) so users can see which <npc> entry was skipped and
why. Ensure messages reference the original filename and the
attribute.as_string() where available.
In `@source/io/xml_file_loader.cpp`:
- Around line 14-17: normalizedPathKey currently returns the normalized path
preserving original case, which allows "foo.xml" and "Foo.xml" to be treated as
different keys on case-insensitive filesystems; change normalizedPathKey (and
only it) so after calling normalized.Normalize(...) and
normalized.GetFullPath().ToStdString() you canonicalize the casing for
case-insensitive platforms (e.g. convert the returned key to a consistent case
such as lowercase) or otherwise use a platform/FS case-insensitivity check to
produce a case-normalized key; update references to normalizedPathKey only so
the duplicate/cycle guards receive the canonicalized key.
In `@source/rendering/core/game_sprite.cpp`:
- Around line 153-175: getCompositePixelSize currently only checks variant
(pattern/frame) 0 by calling getIndex(..., 0, 0, 0, 0) so spriteList entries for
other frames/patterns can be larger and get clipped; fix by scanning all
variants (or the whole spriteList) when computing bounds. Update
GameSprite::getCompositePixelSize to either add loops for pattern_x, pattern_y,
pattern_z and frame (using the sprite-set counts you have in the class) and call
getIndex(part_x, part_y, layer, pattern_x, pattern_y, pattern_z, frame) for each
combination, or simply iterate over all indices in spriteList, skip null
entries, obtain dimensions via spriteList[i]->getDimensions(), and compute
draw_x/draw_y from the decoded part_x/part_y/layer (or by using the same mapping
logic as getIndex) before updating max_width/max_height; ensure you still use
SPRITE_PIXELS, width, height and layers when computing draw positions.
In `@source/rendering/drawers/entities/creature_drawer.cpp`:
- Around line 129-156: The light centering still uses the old fixed-size
calculation (sprite.width/height * TILE_SIZE) so lights remain offset for
oversized mount frames; update the call to registerCreatureSpriteLight() to use
the actual rendered extents instead—either use sprite.GetSize() if that returns
pixel dimensions, or compute the total mount width/height by summing
column_widths and row_heights (the same accumulators used for rendering) and
pass those values (and the same mount_x_offset/mount_y_offset/draw offset logic)
so the light bounds/center match the rendered mount regions produced from
mountSpr and the column_widths/row_heights loops.
---
Outside diff comments:
In `@source/game/materials.cpp`:
- Around line 215-236: The materialsextension handler lost support for nested
<include> because unserializeMaterials() no longer expands include children;
update the code so material extension nodes are visited/expanded for includes
before parsing: either call XmlFileLoader::visitElements(...) (or the
appropriate include-expansion helper) on the materialsextension node in
loadExtensions() before passing it to unserializeMaterials(), or restore
include-handling inside unserializeMaterials() (detect childName == "include"
and resolve/inline the referenced file) so that nested <include file="...">
entries are not skipped; reference functions: loadExtensions,
unserializeMaterials, materialsextension and XmlFileLoader::visitElements.
In `@source/item_definitions/core/item_definition_resolver.cpp`:
- Around line 261-277: The loop that builds ResolvedItemDefinitionRow from
fragments.dat fails to copy dat.volume into row, so finalizeDerivedProperties()
cannot infer container type; update the constructor code in the loop (the block
creating ResolvedItemDefinitionRow row for each entry in fragments.dat) to
assign row.volume = dat.volume (i.e., copy dat.volume into the row) before any
derived-property finalization and before rows.push_back(std::move(row)),
ensuring finalizeDerivedProperties() sees the correct volume and container
inference succeeds.
In `@source/rendering/drawers/entities/item_drawer.cpp`:
- Around line 157-163: The light registration currently computes the light
center using the old sprite.width/height * TILE_SIZE math, causing misalignment
for variable-sized AtlasRegion images; update the call to registerSpriteLight to
use the same composite pixel bounds as the blit path by deriving extents from
spr->GetSize() (or the accumulated extents used by the blit path) and use those
to compute screenx/screeny and the light rectangle passed to
registerSpriteLight; keep using light_buffer, view, *spr and item->getLight()
but replace the width/height * TILE_SIZE derivation with the
spr->GetSize()/composite pixel bounds values so the light is centered on the
actual rendered sprite.
In `@source/ui/tile_properties/spawn_property_panel.cpp`:
- Around line 64-78: After queuing and applying the ACTION_CHANGE_PROPERTIES
change for new_tile in spawn_property_panel.cpp, re-query the tile from the
editor's map and update current_tile before calling g_gui.RefreshView() so
pointer identity checks against current_spawn remain valid; specifically,
capture the tile Position via new_tile->getPosition(), call
editor->addAction(...) as you already do, then set current_tile =
editor->map.getTile(position) (so future comparisons like current_spawn ==
current_tile->npc_spawn.get() or current_tile->spawn.get() succeed) and only
then call g_gui.RefreshView().
---
Nitpick comments:
In `@source/brushes/creature/creature_brush.cpp`:
- Around line 119-135: The code in CreatureBrush::draw_creature uses two
different getters for spawn time (g_brush_manager.GetNpcSpawnTime() for NPC
spawns and g_gui.GetSpawnTime() for legacy spawns) which can diverge; unify the
source by always using a single getter when calling tile->creature->setSpawnTime
(e.g., replace the ternary use_npc_spawn ? g_brush_manager.GetNpcSpawnTime() :
g_gui.GetSpawnTime() with a single call to g_gui.GetSpawnTime()), keeping
usesNpcSpawnSystem, Spawn creation, and tile->creature construction unchanged.
In `@source/io/iomap_otbm.cpp`:
- Around line 525-528: Guard against empty file paths by checking
map.spawnnpcfile and map.zonefile before calling addExtFile (or modifying the
addExtFile lambda to no-op on empty input); specifically, only invoke
addExtFile(OTBM_ATTR_EXT_SPAWN_NPC_FILE, map.spawnnpcfile) and
addExtFile(OTBM_ATTR_EXT_ZONE_FILE, map.zonefile) when the respective string is
non-empty (when mapVersion.otbm >= MAP_OTBM_5), so you avoid writing
empty-string attributes into the OTBM file.
- Around line 448-454: Change the iterator-based check to use .empty() for
consistency: replace the condition in the NPC-spawn save block so it reads "if
(!map.spawnnpcfile.empty() || !map.npc_spawns.empty())" instead of checking
map.npc_spawns.begin() != map.npc_spawns.end(); keep the rest of the block
(g_gui.SetLoadDone, MapXMLIO::saveNpcSpawns call and error handling) unchanged
to preserve behavior.
- Around line 423-426: Reorder the cheap checks in the OTBM version guard so
hasZoneAssignments(map) is evaluated last: change the condition on
map.getVersion().otbm to check map.zones.empty() and the npc spawns iterator
comparison (map.npc_spawns.begin() != map.npc_spawns.end()) before calling
hasZoneAssignments(map) to avoid a full map iteration on every save for OTBM <
5; additionally consider adding an Spawns::empty() method so callers can use
map.npc_spawns.empty() instead of begin()!=end() for clarity and consistency.
In `@source/map/map_spawn_manager.cpp`:
- Around line 5-38: The delta parameter of MapSpawnManager::applySpawnCoverage
is misleading because only its sign is used; change the API to make intent
explicit by replacing int delta with a clearer type (e.g., a boolean like
addOperation or an enum SpawnCountOperation { Add, Remove }) and update all
callers accordingly; inside applySpawnCoverage switch on the new flag/enum
instead of comparing delta, then call
TileLocation::increaseNpcSpawnCount()/decreaseNpcSpawnCount() or
TileLocation::increaseSpawnCount()/decreaseSpawnCount() based on npc and the
operation, preserving the existing guard that prevents decrementing below zero.
- Around line 172-231: getNpcSpawnList duplicates getSpawnList; extract the
common logic into a single helper (e.g., getSpawnListImpl) parameterized by two
callables so you can pass the correct accessors for spawn vs npc_spawn and the
correct count getter. Implement getSpawnListImpl(Map&, Tile*, SpawnGetter
getSpawn, CountGetter getCount) and replace getSpawnList and getNpcSpawnList to
call it with lambdas that return t->spawn.get()/t->npc_spawn.get() and
loc->getSpawnCount()/loc->getNpcSpawnCount(); preserve the existing algorithm
(found counting, checkTile logic, radius limit) but replace direct uses of
Tile::spawn/Tile::npc_spawn and TileLocation::getSpawnCount/getNpcSpawnCount
with the passed callables.
In `@source/rendering/drawers/entities/creature_drawer.cpp`:
- Around line 129-156: Cache the result of mountSpr->getDrawOffset() in a local
variable before the nested loops (both the mount/body blocks that call
mountSpr->getDrawOffset()), then use that cached pair when computing the blit
positions instead of calling getDrawOffset() inside the inner loop; update
references in the glue points around sprite_drawer->glBlitAtlasQuad where you
currently subtract mountSpr->getDrawOffset().first/second and remove the
repeated calls to getDrawOffset() to avoid walking spriteList on the hot render
path.
In `@source/rendering/ui/selection_controller.cpp`:
- Around line 62-72: The item branch (returning TileSelectionTarget with
TileSelectionTargetKind::Item when Item* item = tile->getTopItem()) lacks the
same visibility gating used for spawns/creatures; either wrap that branch with a
check using g_settings.getInteger(Config::SHOW_ITEMS) before returning, or if
items are intentionally always selectable add a brief comment explaining that
exception; update the code near the tile->getTopItem() handling in
selection_controller (TileSelectionTarget / TileSelectionTargetKind::Item) to
reflect the chosen approach.
In `@source/ui/map_popup_menu.cpp`:
- Around line 163-174: The popup menu construction repeats the same
appendIconItem calls for creature/spawn/npc/zone entries in multiple branches;
extract the repeated block into a single helper function (e.g.,
addTileBrushItems or appendTileBrushItems) and call it from both places instead
of duplicating lines referencing MAP_POPUP_MENU_SELECT_CREATURE_BRUSH,
MAP_POPUP_MENU_SELECT_SPAWN_BRUSH, MAP_POPUP_MENU_SELECT_NPC_SPAWN_BRUSH and
MAP_POPUP_MENU_SELECT_ZONE_BRUSH; ensure the helper accepts the booleans or
context needed (topCreature/topSpawn/topNpcSpawn/hasZones or the tile/context)
and reuses appendIconItem with ICON_DRAGON, ICON_FIRE, ICON_USER, ICON_MARKER so
behavior and labels remain identical.
In `@source/ui/tile_properties/zone_property_panel.cpp`:
- Around line 101-107: Extract the repeated "apply tile change" flow into a
single helper (e.g., applyTileChange or applyTileChangeAndSelect) that accepts
the new tile mutation (std::unique_ptr<Tile> new_tile) and an Editor
pointer/reference, captures position via new_tile->getPosition(), creates the
action using editor->actionQueue->createAction(ACTION_CHANGE_PROPERTIES), calls
action->addChange(std::make_unique<Change>(std::move(new_tile))), calls
editor->addAction(std::move(action)), and then updates current_map =
&editor->map and current_tile = editor->map.getTile(position); then replace the
duplicated blocks in the Add/Remove/Clear handlers with calls to this helper
(keeping use of Change, ACTION_CHANGE_PROPERTIES, editor->actionQueue,
editor->addAction, current_map and current_tile).
🪄 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: 01319f95-e604-4e7f-a987-c2e8c6f69434
📒 Files selected for processing (30)
source/app/client_version.cppsource/app/managers/version_manager.cppsource/brushes/creature/creature_brush.cppsource/brushes/zone/zone_brush.cppsource/editor/persistence/editor_persistence.cppsource/game/creatures.cppsource/game/materials.cppsource/io/iomap_otbm.cppsource/io/xml_file_loader.cppsource/item_definitions/core/asset_bundle_loader.cppsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/item_definition_store.cppsource/map/map.cppsource/map/map_spawn_manager.cppsource/map/map_spawn_manager.hsource/map/tile.cppsource/rendering/core/atlas_manager.cppsource/rendering/core/atlas_manager.hsource/rendering/core/game_sprite.cppsource/rendering/core/image.cppsource/rendering/core/texture_atlas.hsource/rendering/drawers/entities/creature_drawer.cppsource/rendering/drawers/entities/item_drawer.cppsource/rendering/drawers/entities/sprite_drawer.cppsource/rendering/drawers/overlays/marker_drawer.cppsource/rendering/ui/selection_controller.cppsource/ui/map_popup_menu.cppsource/ui/tile_properties/spawn_property_panel.cppsource/ui/tile_properties/zone_property_panel.cppsource/util/nanovg_canvas.cpp
✅ Files skipped from review due to trivial changes (2)
- source/brushes/zone/zone_brush.cpp
- source/map/map.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- source/util/nanovg_canvas.cpp
- source/rendering/core/atlas_manager.h
- source/rendering/drawers/overlays/marker_drawer.cpp
- source/rendering/drawers/entities/sprite_drawer.cpp
- source/rendering/core/image.cpp
- source/rendering/core/atlas_manager.cpp
- source/map/map_spawn_manager.h
- source/app/client_version.cpp
- source/editor/persistence/editor_persistence.cpp
- source/map/tile.cpp
Extracted per-sprite dimension computation into reusable metrics structs for creature and item drawers, replacing duplicated column/row accumulation loops. Consolidated spawn list scanning into a templated helper and replaced raw delta ints with a typed SpawnCoverageOperation enum. Added Windows case-insensitive file cache keys, warned on missing NPC file attributes, conditionally omitted empty zone/spawn file extensions, and refreshed stale tile references in property panels. - `source/app/managers/version_manager.cpp` — added `items/items.otb` as an alternative resolution path - `source/game/creatures.cpp` — pushed warnings for missing or unloadable NPC file references, included `<format>` - `source/game/materials.cpp` — extracted `handleMaterialNode` and `visitMaterialElements` templates, removed `unserializeMaterials` method - `source/game/materials.h` — removed `unserializeMaterials` declaration - `source/game/spawn.h` — added `[[nodiscard]] empty()` accessor - `source/io/iomap_otbm.cpp` — switched `npc_spawns` check to `.empty()`, gated `spawnnpcfile` and `zonefile` extensions on non-empty paths - `source/io/xml_file_loader.cpp` — lowercased normalized paths on Windows to fix cache key collisions - `source/map/map_spawn_manager.cpp` — collapsed `getSpawnList`/`getNpcSpawnList` into `getSpawnListImpl` template, replaced `int delta` with `SpawnCoverageOperation` enum - `source/map/map_spawn_manager.h` — added `SpawnCoverageOperation` enum, updated `applySpawnCoverage` signature - `source/rendering/core/game_sprite.cpp` — iterated all pattern and frame dimensions when computing composite pixel size - `source/rendering/drawers/entities/creature_drawer.cpp` — extracted `computeOutfitSpriteMetrics` and overloaded `registerCreatureSpriteLight`, removed duplicated mount/creature dimension loops, added `<numeric>` - `source/rendering/drawers/entities/item_drawer.cpp` — extracted `CompositeSpriteMetrics` and `computeSpriteMetrics`, moved light registration before offset adjustment, removed inline dimension loops, added `<numeric>` - `source/rendering/ui/selection_controller.cpp` — [minor] added comment explaining item selectability when hidden - `source/ui/map_popup_menu.cpp` — extracted `appendTileBrushItems` lambda to deduplicate context menu population - `source/ui/tile_properties/spawn_property_panel.cpp` — refreshed `current_tile` and `current_spawn` after radius change to avoid stale pointers - `source/ui/tile_properties/zone_property_panel.cpp` — extracted `ApplyTileChange` helper, collapsed add/remove/clear zone handlers - `source/ui/tile_properties/zone_property_panel.h` — declared `ApplyTileChange`, added `<memory>` include
|
🤖 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Pull Request introduces significant enhancements to the editor, including support for Protobuf-based client assets, a dedicated NPC spawn system, and a map zone management system with persistent storage. The implementation of OTBM 5/6 support and variable-sized sprite packing in the texture atlas are particularly well-executed, providing a solid foundation for modern client support.
🔍 General Feedback
- Architecture: The separation of NPC and monster spawns in OTBM 5/6 is handled cleanly, maintaining backward compatibility while allowing for cleaner data organization in newer versions.
- UI: The new
ZonePropertyPaneland updates to theToolOptionsSurfaceprovide intuitive controls for the new features. - Performance: The introduction of LZMA decompression for Protobuf sheets is a great addition, though I've suggested a minor optimization for the file reading process.
- Reliability: Suggested some additional error checking in the
TextureAtlasexpansion logic to prevent potential data loss in low-VRAM scenarios.
| return false; | ||
| } | ||
|
|
||
| std::vector<uint8_t> buffer((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>()); |
There was a problem hiding this comment.
🟡 Reading the entire file using std::istreambuf_iterator is significantly slower than using std::ifstream::read() with a pre-allocated buffer because it performs multiple small allocations and character-by-character operations.
| std::vector<uint8_t> buffer((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>()); | |
| file.seekg(0, std::ios::end); | |
| const std::streamsize size = file.tellg(); | |
| file.seekg(0, std::ios::beg); | |
| std::vector<uint8_t> buffer(static_cast<size_t>(size)); | |
| if (!file.read(reinterpret_cast<char*>(buffer.data()), size)) { | |
| spdlog::error("SpriteArchive: failed to read protobuf sprite sheet {}", sheet.path); | |
| return false; | |
| } |
| if (err != GL_NO_ERROR) { | ||
| spdlog::error("TextureAtlas: glCopyImageSubData failed (err={}). Texture data lost!", err); | ||
| // We can't easily recover here, but at least we know why sprites are black. | ||
| } |
There was a problem hiding this comment.
🟡 If glCopyImageSubData fails, the new texture resource is still assigned to texture_id_, but the original texture data was not copied. This results in a blank texture and data loss. Consider returning false early if the copy operation fails.
| if (err != GL_NO_ERROR) { | |
| spdlog::error("TextureAtlas: glCopyImageSubData failed (err={}). Texture data lost!", err); | |
| // We can't easily recover here, but at least we know why sprites are black. | |
| } | |
| if (err != GL_NO_ERROR) { | |
| spdlog::error("TextureAtlas: glCopyImageSubData failed (err={}). Texture data lost!", err); | |
| return false; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
source/ui/tile_properties/zone_property_panel.cpp (1)
85-89:⚠️ Potential issue | 🟠 MajorKeep the zone ID separate from the display label.
When
findName(zone_id)is empty, the list box stores"123"as the actual value.OnRemoveZone()later resolves that text viacurrent_map->zones.findId(...)and returns early, andOnAssignedZoneSelected()forwards the same text intog_brush_manager.SetSelectedZone(...). That makes unnamed zone ids impossible to remove from the UI and can turn a numeric fallback into a brand-new zone name. Store the canonicalzone_idalongside the label, or explicitly parse numeric fallbacks before remove/select.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/zone_property_panel.cpp` around lines 85 - 89, The listbox currently stores the display label only (assigned_zones->Append(...)) so unnamed zones end up with their numeric string as the stored value and later lookups (OnRemoveZone, OnAssignedZoneSelected calling current_map->zones.findId(...) and g_brush_manager.SetSelectedZone(...)) fail or create new names; change the UI to keep the canonical zone_id alongside the visible label — e.g. append the label to assigned_zones but attach the uint16_t zone_id as client data (or maintain a small index->zone_id vector) so OnRemoveZone and OnAssignedZoneSelected use that stored zone_id directly when calling current_map->zones.findId/findName or g_brush_manager.SetSelectedZone, rather than parsing the displayed string.source/io/xml_file_loader.cpp (1)
16-25:⚠️ Potential issue | 🟠 MajorNormalize include keys on macOS too.
The duplicate/cycle guards still treat
Foo.xmlandfoo.xmlas different files on default macOS case-insensitive volumes because only Windows keys are lowercased here.Possible fix
-#ifdef _WIN32 +#if defined(_WIN32) || defined(__WXMAC__) std::transform(path_key.begin(), path_key.end(), path_key.begin(), [](unsigned char ch) { return static_cast<char>(std::tolower(ch)); }); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/xml_file_loader.cpp` around lines 16 - 25, The duplicate guards treat differently-cased filenames as distinct on macOS; update normalizedPathKey to lowercase the path_key on macOS as well by extending the platform condition that currently checks _WIN32 to also include macOS (e.g. add defined(__APPLE__) or the equivalent platform check) so that path_key is transformed to lowercase on macOS too; modify the block around std::transform that references path_key in normalizedPathKey to run for both Windows and macOS.
🧹 Nitpick comments (1)
source/io/iomap_otbm.cpp (1)
423-426: Minor inconsistency in version guard condition.The check guards against saving NPC spawn/zone data but doesn't check the filename fields (
spawnnpcfile,zonefile). If a map has an empty collection but a non-empty filename, the save proceeds, external files are written (lines 448-468), but the OTBM header won't reference them (lines 525-532 are version-gated).While this edge case is unlikely in practice (filenames are typically set when loading from OTBM >= 5), consider including them for completeness:
Suggested change
- if (map.getVersion().otbm < MAP_OTBM_5 && (!map.npc_spawns.empty() || !map.zones.empty() || hasZoneAssignments(map))) { + if (map.getVersion().otbm < MAP_OTBM_5 && (!map.npc_spawns.empty() || !map.zones.empty() || hasZoneAssignments(map) || !map.spawnnpcfile.empty() || !map.zonefile.empty())) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/iomap_otbm.cpp` around lines 423 - 426, The version guard in iomap_otbm.cpp currently prevents saving NPC/zone collections for OTBM < 5 by checking map.npc_spawns, map.zones, and hasZoneAssignments(map) but omits the filename fields; update the condition that checks map.getVersion().otbm (the if using map.npc_spawns, map.zones, hasZoneAssignments(map)) to also check map.spawnnpcfile and map.zonefile (or whatever exact member names hold the external filenames) so that non-empty spawn/zone filename strings will also block saving for OTBM < 5 and avoid writing external files without header references.
🤖 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 234-265: The current logic skips trying creatures.xml when
monsters.xml or npcs.xml exist but fail to load; modify the control flow so that
after attempting g_creatures.importXMLFromOT for monsters_xml_path and
npcs_xml_path (variables monsters_ok and npcs_ok), if either import failed try
the single-file fallback by calling g_creatures.loadFromXML(creatures_xml_path,
true, creatures_error, warnings) before treating it as a fatal error; only set
error/DestroyLoadBar/UnloadVersion and return false if both split imports failed
AND either creatures.xml doesn't exist or its load also fails. Reference
monsters_xml_path, npcs_xml_path, creatures_xml_path, monsters_ok, npcs_ok,
g_creatures.importXMLFromOT and g_creatures.loadFromXML when locating where to
change the logic.
In `@source/rendering/drawers/entities/creature_drawer.cpp`:
- Around line 72-78: The light is mispositioned because draw_offset returned by
sprite.getDrawOffset() already accounts for oversized sprites, but the code
subtracts std::max(0, composite_size.GetWidth() - TILE_SIZE) and std::max(0,
composite_size.GetHeight() - TILE_SIZE) again; remove those redundant
subtractions when computing left and top (i.e., compute left = screen_x -
draw_offset.first and top = screen_y - draw_offset.second) so that
light_buffer.AddScreenLight(left + width / 2, top + height / 2, view, light)
uses the correct coordinates for oversized sprites.
In `@source/rendering/ui/selection_controller.cpp`:
- Around line 24-30: getPrimaryTileSelectionTarget() should not return
TileSelectionTargetKind::None for tiles that contain zones but lack
spawn/creature/item/top; restore a tile-level fallback so zone-only tiles are
selectable. Update getPrimaryTileSelectionTarget() (or the selection-checking
logic in HandleClick(), HandleRelease(), and HandlePropertiesClick()) to
return/interpret a Tile-level target (add or reuse an enum value like Tile or
TileFallback in TileSelectionTargetKind) when a tile has zones, then ensure
TilePropertiesPanel::UpdateFromEditor() receives that tile selection so its
OnZoneSelected() path can run. Keep the change limited to the selection target
determination and consumers that switch on TileSelectionTargetKind.
---
Duplicate comments:
In `@source/io/xml_file_loader.cpp`:
- Around line 16-25: The duplicate guards treat differently-cased filenames as
distinct on macOS; update normalizedPathKey to lowercase the path_key on macOS
as well by extending the platform condition that currently checks _WIN32 to also
include macOS (e.g. add defined(__APPLE__) or the equivalent platform check) so
that path_key is transformed to lowercase on macOS too; modify the block around
std::transform that references path_key in normalizedPathKey to run for both
Windows and macOS.
In `@source/ui/tile_properties/zone_property_panel.cpp`:
- Around line 85-89: The listbox currently stores the display label only
(assigned_zones->Append(...)) so unnamed zones end up with their numeric string
as the stored value and later lookups (OnRemoveZone, OnAssignedZoneSelected
calling current_map->zones.findId(...) and g_brush_manager.SetSelectedZone(...))
fail or create new names; change the UI to keep the canonical zone_id alongside
the visible label — e.g. append the label to assigned_zones but attach the
uint16_t zone_id as client data (or maintain a small index->zone_id vector) so
OnRemoveZone and OnAssignedZoneSelected use that stored zone_id directly when
calling current_map->zones.findId/findName or g_brush_manager.SetSelectedZone,
rather than parsing the displayed string.
---
Nitpick comments:
In `@source/io/iomap_otbm.cpp`:
- Around line 423-426: The version guard in iomap_otbm.cpp currently prevents
saving NPC/zone collections for OTBM < 5 by checking map.npc_spawns, map.zones,
and hasZoneAssignments(map) but omits the filename fields; update the condition
that checks map.getVersion().otbm (the if using map.npc_spawns, map.zones,
hasZoneAssignments(map)) to also check map.spawnnpcfile and map.zonefile (or
whatever exact member names hold the external filenames) so that non-empty
spawn/zone filename strings will also block saving for OTBM < 5 and avoid
writing external files without header references.
🪄 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: cefb2782-5e8b-49c3-bcb6-f6fbc8f820d0
📒 Files selected for processing (17)
source/app/managers/version_manager.cppsource/game/creatures.cppsource/game/materials.cppsource/game/materials.hsource/game/spawn.hsource/io/iomap_otbm.cppsource/io/xml_file_loader.cppsource/map/map_spawn_manager.cppsource/map/map_spawn_manager.hsource/rendering/core/game_sprite.cppsource/rendering/drawers/entities/creature_drawer.cppsource/rendering/drawers/entities/item_drawer.cppsource/rendering/ui/selection_controller.cppsource/ui/map_popup_menu.cppsource/ui/tile_properties/spawn_property_panel.cppsource/ui/tile_properties/zone_property_panel.cppsource/ui/tile_properties/zone_property_panel.h
💤 Files with no reviewable changes (1)
- source/game/materials.h
🚧 Files skipped from review as they are similar to previous changes (4)
- source/ui/map_popup_menu.cpp
- source/ui/tile_properties/spawn_property_panel.cpp
- source/map/map_spawn_manager.h
- source/game/creatures.cpp
…ction Added creatures.xml as a fallback data source when monsters.xml and npcs.xml fail to load, enabled macOS case-insensitive path handling, and introduced tile-level zone selection in the UI with improved zone ID tracking to handle unnamed zones correctly. Fixed sprite archive read reliability, texture atlas error propagation, and OTBM save validation for new file-based spawn/zone fields. - `source/app/managers/version_manager.cpp` — added creatures.xml fallback loading when both monsters.xml and npcs.xml fail, updated error message to reflect third option - `source/io/iomap_otbm.cpp` — extended OTBM version check to include `spawnnpcfile` and `zonefile` fields before rejecting NPC/zone data - `source/io/xml_file_loader.cpp` — enabled case-insensitive path normalization on macOS by extending preprocessor guard - `source/rendering/core/sprite_archive.cpp` — replaced stream iterator buffer read with explicit seekg/tellg size check and guarded read - `source/rendering/core/texture_atlas.cpp` — added early return on glCopyImageSubData failure to prevent using invalid texture ID - `source/rendering/drawers/entities/creature_drawer.cpp` — removed composite size offset adjustment from creature light positioning - `source/rendering/ui/selection_controller.cpp` — added Tile selection target kind with tile pointer, selection check, and add/remove handlers for zone-assigned tiles - `source/ui/tile_properties/zone_property_panel.cpp` — replaced zone name lookup with ID-based indexing using `assigned_zone_ids` vector, fixed remove and select handlers to work with unnamed zones - `source/ui/tile_properties/zone_property_panel.h` — added `<vector>` include and `assigned_zone_ids` member variable
|
🤖 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 significant new features including Protobuf-based client asset support, a dedicated NPC spawn system, and map zones with persistent XML storage. The implementation of the Protobuf parser and the LZMA decompression logic for sprite sheets is well-executed, and the addition of OTBM version 5 and 6 support correctly integrates with the existing map persistence layer.
🔍 General Feedback
- Architecture: The use of a generic
XmlFileLoadervisitor for XML inclusions and visiting is a great improvement to codebase modularity. - Support for OTBM 5/6: The transition to these new versions is cleanly handled, including backward compatibility and clear error messages when newer data is attempted to be saved in older formats.
- Memory & Performance: While the sprite sheet loading works well, consider long-term memory impacts if the number of sheets grows significantly. The implementation of variable-sized sprite packing in the
TextureAtlasis a robust addition for modern client support. - Maintainability: Code is generally well-structured and follows existing patterns. The introduction of
ZoneRegistryand its integration with the Map class is clean and easy to follow.
| return false; | ||
| } | ||
|
|
||
| dimensions = ImageDimensions { 32, 32 }; |
There was a problem hiding this comment.
🟡 Loading the entire compressed sprite sheet into a std::vector<uint8_t> can be memory-intensive if many sheets are processed sequentially or in parallel. Consider using a streaming approach or reading only the necessary chunks if memory pressure becomes an issue.
| dimensions = ImageDimensions { 32, 32 }; | |
| std::vector<uint8_t> buffer; | |
| buffer.reserve(static_cast<size_t>(size)); | |
| buffer.assign(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()); |
(Actually, reading into a pre-allocated vector is already what they do, maybe suggest a stream-based LZMA decoding if possible, but liblzma often works better with a full buffer for raw decoders. I'll just keep it as a note.)
| const FileName include_file = XmlFileLoader::resolveRelative(filename, wxString::FromUTF8(include_attribute.as_string())); | ||
| wxString include_error; | ||
| if (!visitFile(include_file, root_name, visitor, state, include_error, warnings)) { | ||
| warnings.push_back(std::format("Failed to load XML include {}: {}", include_file.GetFullPath().ToStdString(), include_error.ToStdString())); |
There was a problem hiding this comment.
🟡 If a nested XML file inclusion fails, it only adds a warning and continues. Depending on the criticality of the included file (e.g., a mandatory set of brush definitions), this might result in a partially loaded state that could cause unexpected behavior. Consider if some failures should be treated as fatal.
| warnings.push_back(std::format("Failed to load XML include {}: {}", include_file.GetFullPath().ToStdString(), include_error.ToStdString())); | |
| if (!visitFile(include_file, root_name, visitor, state, include_error, warnings)) { | |
| warnings.push_back(std::format("Failed to load XML include {}: {}", include_file.GetFullPath().ToStdString(), include_error.ToStdString())); | |
| // Consider if we should return false here if the include is mandatory. | |
| } |
|
@coderabbitai full review |
|
/gemini review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for protobuf-based client assets, including a new ProtobufItemParser and associated asset detection logic. It also adds new brush types (NpcSpawnBrush, ZoneBrush) and updates the map serialization to support NPC spawns and zones in OTBM format. The code changes are well-structured and align with the project's data-oriented design goals. I have no feedback to provide as the reviewer's comments were either validation or suggestions for refactoring that, while potentially beneficial, are not strictly necessary for the correctness or performance of the current implementation.
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
source/editor/operations/selection_operations.cpp (1)
112-130:⚠️ Potential issue | 🟠 MajorMove
zone_idstogether with the ground tile.This path now transfers
npc_spawn, but it still leaveszone_idsonnew_src_tile. When a zoned tile is moved, the old position keeps the zone membership and the destination loses it.Proposed fix
// Move house data & tile status if ground is transferred if (tmp_storage_tile->ground) { + tmp_storage_tile->zone_ids = new_src_tile->zone_ids; + new_src_tile->zone_ids.clear(); tmp_storage_tile->house_id = new_src_tile->house_id; new_src_tile->house_id = 0; tmp_storage_tile->setMapFlags(new_src_tile->getMapFlags()); new_src_tile->setMapFlags(TILESTATE_NONE); doborders = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/operations/selection_operations.cpp` around lines 112 - 130, When transferring a ground tile (inside the block checking if (tmp_storage_tile->ground)), also move the tile's zone membership: assign/move new_src_tile->zone_ids into tmp_storage_tile->zone_ids and then clear new_src_tile->zone_ids (e.g. new_src_tile->zone_ids.clear() or std::move depending on the container) so the old position no longer keeps the zones; keep this logic adjacent to the existing house_id/map flag transfers (refer to new_src_tile, tmp_storage_tile, zone_ids, house_id, setMapFlags).source/map/map_region.cpp (1)
45-54:⚠️ Potential issue | 🟠 Major
TileLocation::clone()drops NPC spawn bookkeeping.The new
npc_spawn_countis initialized and used bysize(), but it is not copied here. Cloned locations will report the wrongsize()/empty()and can lose NPC spawn presence on clone-based paths.Suggested fix
auto copy = std::make_unique<TileLocation>(); copy->position = position; copy->spawn_count = spawn_count; + copy->npc_spawn_count = npc_spawn_count; copy->waypoint_count = waypoint_count; copy->town_count = town_count;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/map_region.cpp` around lines 45 - 54, TileLocation::clone() is not copying the npc_spawn_count field, causing size()/empty() to be incorrect for cloned objects; update TileLocation::clone to also copy npc_spawn_count (e.g. set copy->npc_spawn_count = npc_spawn_count) alongside the other counts and preserve NPC spawn bookkeeping when cloning.data/clients.toml (1)
235-251:⚠️ Potential issue | 🟠 MajorRemove duplicate signatures from client 8.40 or update to match its unique assets.
Client 8.40 reuses the signatures from client 8.00 (
datSignature = '467FD7E6',sprSignature = '467F9E74'), breaking the pattern where every other version (8.10, 8.11, 8.20–8.31, 8.30, 8.41, etc.) has unique signatures. Since 8.40 has a differentdataDirectory('840' vs '800'), it likely contains different assets, and using 8.00's signatures will cause asset loading failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/clients.toml` around lines 235 - 251, The datSignature and sprSignature for the client entry name '8.40' (version 840, configType 'dat_otb') are duplicates of 8.00 and must be corrected; inspect the actual asset files in dataDirectory '840' and replace datSignature and sprSignature with the correct unique signature strings for that folder (or remove them if signatures are not used for this config), ensuring the entry's datSignature and sprSignature match the real assets so asset loading (functions referencing these fields) will succeed.source/rendering/core/image.cpp (1)
42-65:⚠️ Potential issue | 🟠 MajorKeep a non-zero fallback sprite size on decode failure.
If
getRGBAData()fails anddimensions/getDimensions()do not yield valid values, this path now builds a zero-length buffer and callsaddSprite(..., 0, 0). The previous fixed32x32fallback at least produced a usable placeholder; this version can turn the error path into another load failure. Please clamp invalid dimensions to a sane minimum before allocating/registering the magenta fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/image.cpp` around lines 42 - 65, When getRGBAData() returns null, ensure ImageDimensions (the image_dimensions used for allocation and atlas registration) is clamped to a sane minimum (e.g. 32x32) before allocating the magenta buffer and before calling atlas_mgr->addSprite; update the branch where rgba is null to validate image_dimensions.pixelCount(), image_dimensions.width and height (using ImageDimensions methods or fields) and set them to the minimum if zero or invalid so you don't allocate a zero-length buffer or call addSprite(..., 0, 0). Ensure the same clamped image_dimensions are passed into atlas_mgr->addSprite(sprite_id, rgba.get(), image_dimensions.width, image_dimensions.height).
♻️ Duplicate comments (12)
.gitignore (1)
130-130:⚠️ Potential issue | 🟠 MajorKeep
.protosources tracked; don’t ignore the entire/protobufdirectory.Ignoring
/protobufwholesale can exclude schema files needed for deterministic codegen and CI reproducibility. Keep source schemas tracked and ignore only generated artifacts.Suggested `.gitignore` adjustment
-/protobuf +# Keep source schemas tracked; ignore generated protobuf outputs only +/protobuf/**/*.pb.cc +/protobuf/**/*.pb.h +/protobuf/**/generated/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 130, Remove the blanket ignore for the /protobuf directory and ensure .proto schema files remain tracked; update the .gitignore entry that currently lists "/protobuf" so it does not exclude .proto sources and instead only ignores generated artifacts (e.g., generated code or build output under /protobuf such as files with generated suffixes or a dedicated build/gen subfolder), referencing the "/protobuf" entry and .proto files when making the change.source/rendering/core/template_image.cpp (1)
109-121:⚠️ Potential issue | 🔴 CriticalCompare base and mask dimensions before colorizing.
getDimensions()still resolves only the base sprite, but bothColorizeTemplatePixels(...)calls consume the mask buffer for that same pixel count. If the mask slot ever decodes to a different size, this can read past the shorter buffer. Bail out on a width/height mismatch before either call.Possible fix
auto rgbdata = parent->spriteList[sprite_index]->getRGBData(); auto template_rgbdata = parent->spriteList[mask_index]->getRGBData(); + const auto base_dimensions = parent->spriteList[sprite_index]->getDimensions(); + const auto mask_dimensions = parent->spriteList[mask_index]->getDimensions(); if (!rgbdata) { return nullptr; } if (!template_rgbdata) { return nullptr; } + if (base_dimensions.width != mask_dimensions.width || base_dimensions.height != mask_dimensions.height) { + spdlog::warn( + "TemplateImage (texture_id={}): base/mask dimension mismatch (base={}x{}, mask={}x{})", + texture_id, + base_dimensions.width, + base_dimensions.height, + mask_dimensions.width, + mask_dimensions.height + ); + return nullptr; + } clampTemplateLookValues(this); - GameSprite::ColorizeTemplatePixels(rgbdata.get(), template_rgbdata.get(), getDimensions().pixelCount(), lookHead, lookBody, lookLegs, lookFeet, false); + GameSprite::ColorizeTemplatePixels(rgbdata.get(), template_rgbdata.get(), base_dimensions.pixelCount(), lookHead, lookBody, lookLegs, lookFeet, false);Also applies to: 132-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/template_image.cpp` around lines 109 - 121, The code must verify the base sprite and mask sprite dimensions match before colorizing to avoid buffer overruns: fetch and compare the dimensions (width/height or pixelCount) of parent->spriteList[sprite_index] and parent->spriteList[mask_index] after obtaining rgbdata and template_rgbdata and before calling clampTemplateLookValues() or GameSprite::ColorizeTemplatePixels; if they differ, return nullptr (or otherwise bail out). Ensure you reference the same dimension API used by getDimensions()/pixelCount() and apply the same guard in both places mentioned (around the ColorizeTemplatePixels calls).source/io/xml_file_loader.cpp (1)
63-66:⚠️ Potential issue | 🟠 MajorDon't continue after a failed
<include>by default.This turns nested include failures into warnings and keeps traversing, so callers can report success with only a partial dataset loaded. For definition files like
items.xmlandmaterials.xml, failing fast is usually safer than silently degrading. Consider making include failures fatal by default, or adding an explicit policy so only truly optional includes are downgraded to warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/xml_file_loader.cpp` around lines 63 - 66, The current code treats a failed include (visitFile returning false) as a warning and continues, which allows partial loads; change this so include failures are fatal by default by replacing the warnings.push_back path with immediate failure propagation (return false) so the caller sees the error (use include_error to surface the message). Alternatively, if some includes must remain optional, add and check an explicit flag/attribute (e.g., optionalInclude or an "optional" attribute on the include element) and only push to warnings when that flag is set; locate and modify the handling around visitFile, include_file, include_error and the warnings vector in xml_file_loader.cpp to implement this behavior.source/map/tile_operations.cpp (1)
179-184:⚠️ Potential issue | 🟡 MinorConsume
src->zone_idsinmerge().
TileOperations::merge()moves every other tile-owned payload out ofsrc, but zones are only copied intodest. That leavessrcin a partially moved-from state that still reports its old zones.Proposed fix
if (src->npc_spawn) { dest->npc_spawn = std::move(src->npc_spawn); } for (uint16_t zone_id : src->zone_ids) { dest->addZone(zone_id); } + src->zone_ids.clear(); if (src->invalidZones) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/tile_operations.cpp` around lines 179 - 184, TileOperations::merge() currently copies src->zone_ids into dest via addZone, leaving src in a partially moved-from state; change this to consume src->zone_ids by moving its contents into dest and clearing src->zone_ids (e.g., transfer/move the underlying container or use move-iterators into dest) so src no longer reports the old zones — update the merge implementation to move src->zone_ids instead of copying and ensure src->zone_ids is emptied afterwards.CMakeLists.txt (1)
33-34:⚠️ Potential issue | 🟠 MajorConfig-mode Protobuf generation mismatch still present.
At Line 77,
protobuf_generate_cpp(...)is still used afterfind_package(Protobuf REQUIRED)at Line 33. In config-mode package flows this can break unless compatibility is explicitly enabled. This duplicates the earlier finding and still needs resolution.For modern CMake + Protobuf config-mode packages (e.g., vcpkg/Conan), is `protobuf_generate_cpp(...)` available without setting `protobuf_MODULE_COMPATIBLE`, or should `protobuf_generate(...)` be used instead?Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 33 - 34, The CMake file is still calling protobuf_generate_cpp(...) after find_package(Protobuf REQUIRED), which can fail with config-mode Protobuf packages; update the package usage to enable compatibility or switch to the modern generator: either call find_package(Protobuf REQUIRED CONFIG PATHS ...) with setting protobuf_MODULE_COMPATIBLE TRUE before the find_package to allow protobuf_generate_cpp, or replace the call to protobuf_generate_cpp(...) with the newer protobuf_generate(TARGET ...) / protobuf_generate() API provided by the config-mode package (i.e., use protobuf_generate or the target-based generators from the Protobuf CMake package) and adjust references accordingly; ensure you reference the symbols protobuf_generate_cpp, protobuf_generate, and protobuf_MODULE_COMPATIBLE when making the change.source/app/client_asset_detector.cpp (1)
346-360:⚠️ Potential issue | 🟠 MajorRequire the referenced appearances asset to exist before returning success.
This still accepts any non-empty
filefield fromcatalog-content.json. If the catalog points at a missing or renamed appearances blob, detection succeeds and stores an unusablemetadata_file_name, so the real failure moves to a later load step. Only setresult.metadata_file_nameafter verifying the referenced file exists, and stop on the first valid match.Possible fix
for (const auto& entry : catalog) { if (!entry.is_object()) { continue; } if (entry.value("type", std::string {}) == "appearances") { const auto filename = entry.value("file", std::string {}); - if (!filename.empty()) { - result.metadata_file_name = filename; + if (filename.empty()) { + continue; + } + const wxFileName metadata_path(client_path.GetFullPath() + FileName::GetPathSeparator() + wxString::FromUTF8(filename)); + if (metadata_path.FileExists()) { + result.metadata_file_name = filename; + break; } } } if (!result.metadata_file_name.has_value()) { - result.warnings.emplace_back("Client asset detection failed: no appearances entry was found in catalog-content.json."); + result.warnings.emplace_back("Client asset detection failed: no readable appearances asset was found in catalog-content.json."); return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/client_asset_detector.cpp` around lines 346 - 360, The loop over catalog currently accepts any non-empty entry.value("file", ...) and sets result.metadata_file_name without verifying the blob exists; change it so that for each appearances entry you extract the filename (entry.value("file", ...)), verify the referenced file actually exists (e.g., via the same blob lookup or std::filesystem::exists used elsewhere in this module), only then assign result.metadata_file_name and break out of the loop on first valid match; if no verified file is found keep result.metadata_file_name unset and emit the existing warning before returning.source/game/creatures.cpp (1)
372-383:⚠️ Potential issue | 🟡 MinorKeep monster import diagnostics consistent with the new NPC path.
Broken
<monster file="...">entries are still silently skipped here. That leaves partial creature imports very hard to diagnose, especially now that the NPC branch already reports the same failures.Suggested fix
pugi::xml_attribute attribute; if (!(attribute = monsterNode.attribute("file"))) { + warnings.push_back(std::format( + "Skipping <monster> entry in {} because it is missing a file attribute.", + filename.GetFullPath().ToStdString() + )); continue; } FileName monsterFile = XmlFileLoader::resolveRelative(filename, wxString::FromUTF8(attribute.as_string())); pugi::xml_document monsterDoc; pugi::xml_parse_result monsterResult = monsterDoc.load_file(monsterFile.GetFullPath().mb_str()); if (!monsterResult) { + warnings.push_back(std::format( + "Skipping <monster file=\"{}\"> in {} because {} could not be loaded: {}.", + attribute.as_string(), + filename.GetFullPath().ToStdString(), + monsterFile.GetFullPath().ToStdString(), + monsterResult.description() + )); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/game/creatures.cpp` around lines 372 - 383, The monster import currently silently skips entries when the "file" attribute is missing or when XmlFileLoader::resolveRelative/monsterDoc.load_file fails; update the import logic in creatures.cpp (around monsterNode/monsterFile/monsterDoc/monsterResult) to emit a diagnostic log including the original filename, the resolved monsterFile path, and the XML parse error (monsterResult.description()) or a clear message if the attribute is missing, matching the NPC import's error reporting style so broken <monster file="..."> entries are visible during import.source/rendering/core/sprite_archive.h (1)
52-58:⚠️ Potential issue | 🟠 MajorUnbounded decoded pixel cache remains unaddressed.
The
ProtobufSheet::decoded_pixelsbuffer is lazily loaded vialoadSheetPixels()and retained for the lifetime of theSpriteArchive. After sprites are copied into the atlas, these decompressed RGBA buffers become redundant but are never evicted. For large protobuf clients with many sheets, this can consume significant memory.Consider adding a mechanism to release decoded pixels after atlas population, such as
ProtobufSheet::releaseDecodedPixels()or an LRU cache with bounded capacity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_archive.h` around lines 52 - 58, The ProtobufSheet decoded pixel buffers (ProtobufSheet::decoded_pixels) are kept for the lifetime of SpriteArchive and never freed; add a ProtobufSheet::releaseDecodedPixels() method that resets decoded_pixels (and ensures thread-safety if accessed concurrently), call this method after a sheet's pixels have been copied into the atlas during atlas population (e.g., in the function that iterates sheets and calls loadSheetPixels()), and/or implement a bounded LRU-style eviction around loadSheetPixels() to cap total decoded memory; update any callers (loadSheetPixels, atlas population code in SpriteArchive) to invoke releaseDecodedPixels() once the RGBA data is no longer needed.source/io/map_xml_io.cpp (1)
282-287:⚠️ Potential issue | 🔴 CriticalGuard
createTile()before writingnpc_spawn.After
map.createTile(...), the code immediately dereferencestilewithout checking if creation succeeded. Compare withloadSpawns()at lines 75-82 which properly guards this case. A failed tile creation would cause a null pointer dereference.🐛 Proposed fix
if (!tile) { tile = map.createTile(spawnPosition.x, spawnPosition.y, spawnPosition.z); } + if (!tile) { + spdlog::warn("MapXMLIO: Failed to create NPC spawn tile at {}:{}:{}", spawnPosition.x, spawnPosition.y, spawnPosition.z); + continue; + } + tile->npc_spawn = std::make_unique<Spawn>(radius); map.addNpcSpawn(tile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/map_xml_io.cpp` around lines 282 - 287, After calling map.createTile(spawnPosition.x, spawnPosition.y, spawnPosition.z) ensure you guard the returned pointer before writing to it: check if tile is non-null and only then assign tile->npc_spawn = std::make_unique<Spawn>(radius) and call map.addNpcSpawn(tile); if createTile fails, skip the npc_spawn creation/adding (mirroring the guard used in loadSpawns()). Update the code paths around createTile, tile, npc_spawn, Spawn, and map.addNpcSpawn to avoid dereferencing a null tile.source/rendering/core/sprite_archive.cpp (2)
143-152:⚠️ Potential issue | 🟠 MajorReject protobuf sheet IDs above
MAX_SPRITES.The code computes
sprite_countfromsheet.last_idvalues without validating againstMAX_SPRITES. A malformedcatalog-content.jsonwith very largelastspriteidvalues could cause excessive memory allocation forsheet_lookup.🛡️ Proposed fix
sprite_count = std::max(sprite_count, sheet.last_id); sheets.push_back(std::move(sheet)); } if (sheets.empty()) { error = "No protobuf sprite sheets were found in catalog-content.json."; return nullptr; } + if (sprite_count > MAX_SPRITES) { + error = wxString::FromUTF8(std::format( + "Protobuf sprite catalog references sprite id {} which exceeds MAX_SPRITES={}.", + sprite_count, + MAX_SPRITES)); + return nullptr; + } + std::vector<int32_t> sheet_lookup(static_cast<size_t>(sprite_count) + 1, -1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_archive.cpp` around lines 143 - 152, The code uses sheet.last_id to set sprite_count and then allocates sheet_lookup without checking MAX_SPRITES, which can lead to huge allocations; in the parsing loop that updates sprite_count (reference: sprite_count and sheet.last_id) validate that sheet.last_id is <= MAX_SPRITES and reject the input (set error and return nullptr) if it exceeds the limit, and also ensure the computed sprite_count used to size sheet_lookup is clamped to MAX_SPRITES before allocation to prevent excessive memory usage.
347-352:⚠️ Potential issue | 🟠 MajorReturn failure for out-of-range protobuf sprite IDs.
When
sprite_id >= protobuf_sheet_lookup_.size(), the code returnstruewith an empty image. This silently hides broken asset references (DAT/catalog mismatches) instead of surfacing the error.🐛 Proposed fix
- if (sprite_id == 0 || sprite_id >= protobuf_sheet_lookup_.size()) { + if (sprite_id == 0) { dimensions = {}; target = std::make_unique<uint8_t[]>(dimensions.pixelCount() * 4); return true; } + if (sprite_id >= protobuf_sheet_lookup_.size()) { + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_archive.cpp` around lines 347 - 352, In SpriteArchive::readProtobufRgba, treat out-of-range protobuf sprite IDs as an error: keep the existing empty-image behavior for sprite_id == 0 (set dimensions = {} and allocate an empty target), but when sprite_id >= protobuf_sheet_lookup_.size() set dimensions = {} clear/reset target (e.g. target.reset() or set to nullptr) and return false instead of true so callers can detect broken DAT/catalog references; update the branch that currently combines the two checks to distinguish sprite_id == 0 vs >= protobuf_sheet_lookup_.size() and return false for the latter.source/item_definitions/formats/protobuf/protobuf_item_parser.cpp (1)
361-384:⚠️ Potential issue | 🟠 MajorBound the derived client-id range before sizing
catalog.entries.Lines 382-383 clamp
catalog.item_countandcatalog.creature_counttouint16_tmax, but line 384 uses the unclampeditem_count + creature_countfor resizing. Additionally, the outfit loop at line 404 uses unclampeditem_count. A malformed appearances file with very large IDs can cause huge allocation or out-of-bounds access.🛡️ Proposed fix
+ // Validate total range before allocation + const uint64_t total_entries = uint64_t{item_count} + uint64_t{creature_count} + 1; + constexpr uint64_t kMaxEntries = 200000; // Reasonable upper bound + if (total_entries > kMaxEntries) { + error = wxString::FromUTF8(std::format( + "Protobuf appearances file has too many entries ({} items + {} creatures).", + item_count, creature_count)); + return false; + } + catalog = {}; catalog.format = DAT_FORMAT_1057; catalog.is_extended = true; catalog.has_transparency = true; catalog.has_frame_durations = true; catalog.has_frame_groups = true; catalog.item_count = static_cast<uint16_t>(std::min<uint32_t>(item_count, std::numeric_limits<uint16_t>::max())); catalog.creature_count = static_cast<uint16_t>(std::min<uint32_t>(creature_count, std::numeric_limits<uint16_t>::max())); - catalog.entries.resize(static_cast<size_t>(item_count + creature_count) + 1); + catalog.entries.resize(static_cast<size_t>(total_entries));And for the outfit loop:
for (const auto& outfit : appearances.outfit()) { if (!outfit.has_id()) { warnings.push_back("Skipping protobuf outfit appearance without an id."); continue; } - const uint32_t client_id = item_count + outfit.id(); + const uint64_t client_id = uint64_t{item_count} + uint64_t{outfit.id()}; + if (client_id >= catalog.entries.size()) { + warnings.push_back(std::format("Skipping protobuf outfit appearance {} - derived id out of range.", outfit.id())); + continue; + } - auto& entry = catalog.entries[client_id]; + auto& entry = catalog.entries[static_cast<size_t>(client_id)];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/formats/protobuf/protobuf_item_parser.cpp` around lines 361 - 384, The code computes item_count and creature_count from appearances but then clamps them into catalog.item_count/catalog.creature_count; however catalog.entries.resize(...) and the outfit loop still use the unclamped item_count/creature_count which can lead to huge allocations or OOBs. Fix by bounding the derived ranges before sizing/iteration: replace usage of raw item_count/creature_count in entries.resize and the outfit loop with the clamped values (catalog.item_count and catalog.creature_count, cast to size_t as needed) or explicitly clamp item_count/creature_count to std::numeric_limits<uint16_t>::max() before the resize and loops so all sizing/iteration uses the safe, limited range (refer to appearances.object(), appearances.outfit(), catalog.entries, catalog.item_count, catalog.creature_count, item_count, creature_count, and the outfit loop).
🧹 Nitpick comments (14)
source/ui/gui.cpp (1)
95-98: Both spawn radius settings are updated together.The function writes the same size value to both
CURRENT_SPAWN_RADIUSandCURRENT_NPC_SPAWN_RADIUS. This couples creature spawn and NPC spawn radii together, meaning users cannot independently configure different default radii for each spawn type.If this is intentional for UX simplicity, consider adding a brief comment explaining the design choice. If independent control is desired later, this would need refactoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/gui.cpp` around lines 95 - 98, The code currently writes the same brush size into both Config::CURRENT_SPAWN_RADIUS and Config::CURRENT_NPC_SPAWN_RADIUS via gui.GetBrushSize(), coupling creature and NPC spawn radii; either (a) if independent control is required, replace the single call with two distinct values obtained from appropriate GUI getters (e.g. add/use gui.GetCreatureSpawnRadius() and gui.GetNpcSpawnRadius()) and call g_settings.setInteger(Config::CURRENT_SPAWN_RADIUS, creatureSize) and g_settings.setInteger(Config::CURRENT_NPC_SPAWN_RADIUS, npcSize), or (b) if the coupling is intentional, add a short explanatory comment above the two g_settings.setInteger calls clarifying the deliberate UX decision. Ensure you reference and update the functions/fields: gui.GetBrushSize()/new getters, g_settings.setInteger, Config::CURRENT_SPAWN_RADIUS, and Config::CURRENT_NPC_SPAWN_RADIUS.source/rendering/drawers/entities/creature_drawer.cpp (1)
30-58: LGTM - OutfitSpriteMetrics and computeOutfitSpriteMetrics.The implementation correctly computes per-column and per-row dimensions for outfit sprites, mirroring the pattern in
item_drawer.cpp.Note:
OutfitSpriteMetricsis nearly identical toCompositeSpriteMetricsinitem_drawer.cpp. Consider extracting a shared struct to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/entities/creature_drawer.cpp` around lines 30 - 58, OutfitSpriteMetrics and computeOutfitSpriteMetrics duplicate CompositeSpriteMetrics logic; extract a shared struct (e.g., SpriteMetrics) into a common header and replace both OutfitSpriteMetrics and CompositeSpriteMetrics with that single type, update computeOutfitSpriteMetrics to return SpriteMetrics, and change the corresponding usages in item_drawer.cpp (functions that previously used CompositeSpriteMetrics) to use SpriteMetrics so both computeOutfitSpriteMetrics and the composite sprite computations share the same struct definition and fields (column_widths, row_heights, total_width, total_height, left_offset, top_offset).source/brushes/spawn/npc_spawn_brush.cpp (1)
38-44: Consider defensive null-check forparameterin release builds.
ASSERT(parameter)only fires in debug builds. In release mode, a nullparameterwould cause undefined behavior when dereferenced on line 43. If callers can guarantee non-null parameters, this is acceptable; otherwise, consider adding a runtime guard.🛡️ Optional defensive fix
void NpcSpawnBrush::draw(BaseMap* map, Tile* tile, void* parameter) { (void)map; ASSERT(tile); ASSERT(parameter); + if (!tile || !parameter) { + return; + } if (tile->npc_spawn == nullptr) { tile->npc_spawn = std::make_unique<Spawn>(std::max(1, *static_cast<int*>(parameter))); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/brushes/spawn/npc_spawn_brush.cpp` around lines 38 - 44, NpcSpawnBrush::draw uses ASSERT(parameter) which is removed in release builds, so dereferencing *static_cast<int*>(parameter) can UB; add a runtime null-check for parameter at the top of NpcSpawnBrush::draw and handle it (e.g., return early or use a sane default count) before using it to construct Spawn, ensuring tile->npc_spawn assignment only occurs when parameter is non-null; reference NpcSpawnBrush::draw, parameter, tile->npc_spawn and Spawn when locating the code.source/protobuf/CMakeLists.txt (1)
11-14: Usingfile(GLOB)for source files may miss new.protofiles.CMake won't automatically re-configure when new
.protofiles are added to these directories. Consider explicitly listing the proto files or documenting that a manual re-configure is needed after adding new protos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/protobuf/CMakeLists.txt` around lines 11 - 14, The CMake snippet uses file(GLOB ProtoFiles ...) which can miss newly added .proto files because CMake won't re-run configuration automatically; replace the glob with an explicit list by changing the file(GLOB ProtoFiles ...) block to a set(ProtoFiles ...) that enumerates each .proto filename (or maintain a generated list file checked into VCS) so new protos are picked up without relying on unpredictable glob behavior, and/or add a clear comment in CMakeLists.txt documenting that contributors must re-run CMake when adding .proto files if you choose to keep the glob; reference the existing file(GLOB call and the ProtoFiles variable when making the change.source/ui/tile_properties/zone_property_panel.h (1)
33-34: Consider documenting lifetime expectations for raw pointer members.
current_tileandcurrent_mapare raw pointers with no explicit ownership semantics. While this is typical for UI panels referencing editor state, ensure callers understand these pointers become stale when the tile/map is deleted or the selection changes. A brief comment noting the caller's responsibility to callSetTile(nullptr, nullptr)before destruction would clarify the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/zone_property_panel.h` around lines 33 - 34, Add a concise lifetime contract comment in ZonePropertyPanel near the raw pointer members current_tile and current_map explaining they are non-owning, may become invalid when the referenced Tile or Map is deleted or selection changes, and that callers must clear them (call SetTile(nullptr, nullptr)) before destroying the referenced objects or the panel; reference the member names current_tile, current_map and the setter SetTile to make the expected responsibility explicit.source/rendering/core/graphics_assembler.cpp (1)
49-69: Consider extracting the dimension resolution logic to reduce duplication.The same
sprite_archive && sprite_archive->isProtobuf() ? ... : toImageDimensions(...)logic appears at lines 58 and 64. A small lambda or helper would improve readability.♻️ Proposed refactor
NormalImage* GraphicsAssembler::ensureImage(GraphicManager& manager, const DatCatalog& catalog, const std::shared_ptr<SpriteArchive>& sprite_archive, uint32_t sprite_id) { if (sprite_id >= manager.image_space.size()) { return nullptr; } + auto getDimensions = [&]() { + return sprite_archive && sprite_archive->isProtobuf() + ? sprite_archive->spriteDimensions(sprite_id) + : toImageDimensions(catalog.spriteDimension(sprite_id)); + }; + auto& slot = manager.image_space[sprite_id]; if (!slot) { auto image = std::make_unique<NormalImage>(); image->id = sprite_id; - const auto dimensions = sprite_archive && sprite_archive->isProtobuf() ? sprite_archive->spriteDimensions(sprite_id) : toImageDimensions(catalog.spriteDimension(sprite_id)); + const auto dimensions = getDimensions(); image->pixel_width = dimensions.width; image->pixel_height = dimensions.height; slot = std::move(image); } else if (slot->isNormalImage()) { auto* image = static_cast<NormalImage*>(slot.get()); - const auto dimensions = sprite_archive && sprite_archive->isProtobuf() ? sprite_archive->spriteDimensions(sprite_id) : toImageDimensions(catalog.spriteDimension(sprite_id)); + const auto dimensions = getDimensions(); image->pixel_width = dimensions.width; image->pixel_height = dimensions.height; } return static_cast<NormalImage*>(slot.get()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/graphics_assembler.cpp` around lines 49 - 69, Extract the duplicated dimension-resolution expression inside GraphicsAssembler::ensureImage into a small helper (either a local lambda or a private helper method) that takes (const std::shared_ptr<SpriteArchive>& sprite_archive, const DatCatalog& catalog, uint32_t sprite_id) and returns the resolved dimensions; replace both occurrences of the ternary expression (sprite_archive && sprite_archive->isProtobuf() ? sprite_archive->spriteDimensions(sprite_id) : toImageDimensions(catalog.spriteDimension(sprite_id))) with a call to that helper, and keep the existing assignments to image->pixel_width and image->pixel_height for NormalImage unchanged.source/brushes/zone/zone_brush.cpp (1)
33-52: Consider adding trace logging whendynamic_castfails.Silent return when
concrete_mapis null may complicate debugging. A trace-level log would help identify misconfiguration without impacting performance.Also, the
std::string { zone_name }construction on line 47 allocates a new string. IfZoneRegistry::ensureZoneacceptedstd::string_view, this allocation could be avoided, though this is a minor optimization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/brushes/zone/zone_brush.cpp` around lines 33 - 52, ZoneBrush::draw silently returns when the dynamic_cast<Map*>(map) fails and when ensureZone is called with std::string{zone_name} causing an unnecessary allocation; modify the function to log a trace-level message when the dynamic_cast to Map (concrete_map) returns null (e.g., processLogger.trace or equivalent) to aid debugging, and if possible update ZoneRegistry::ensureZone to take a std::string_view (and adjust its callers) so you can pass zone_name directly without allocating a new std::string when calling concrete_map->zones.ensureZone(zone_name).source/ui/tile_properties/zone_property_panel.cpp (2)
45-46: Use= defaultfor empty destructor.The destructor body is empty and can be defaulted.
♻️ Suggested change
-ZonePropertyPanel::~ZonePropertyPanel() { -} +ZonePropertyPanel::~ZonePropertyPanel() = default;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/zone_property_panel.cpp` around lines 45 - 46, The destructor ZonePropertyPanel::~ZonePropertyPanel() is defined with an empty body; replace the manual empty definition with a defaulted destructor to signal the intent and enable potential compiler optimizations—change the out-of-class definition to use = default (or move the defaulting into the class declaration for class ZonePropertyPanel) so the destructor is defaulted rather than an empty manual implementation.
165-179: Empty zone name passed toSetSelectedZonefor unnamed zones.When selecting a zone that has no registered name (only a numeric ID fallback),
zone_namewill be empty (fromfindNamereturning empty string), butSetSelectedZone("")is called. This may cause the brush to have no active zone selected even though the user clicked on a valid zone entry.Consider setting the zone by ID or using the display string when the name is empty:
♻️ Proposed fix
const uint16_t zone_id = assigned_zone_ids[static_cast<size_t>(selection)]; const std::string zone_name = current_map ? current_map->zones.findName(zone_id) : std::string {}; zone_input->SetValue(wxstr(zone_name.empty() ? std::to_string(zone_id) : zone_name)); - g_brush_manager.SetSelectedZone(zone_name); + g_brush_manager.SetSelectedZone(zone_name.empty() ? std::string{} : zone_name);Note: This may be intentional if the brush should only work with named zones, but consider whether selecting an unnamed zone in the list should still activate it for painting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/tile_properties/zone_property_panel.cpp` around lines 165 - 179, OnAssignedZoneSelected currently calls g_brush_manager.SetSelectedZone(zone_name) even when zone_name is empty for unnamed zones; update ZonePropertyPanel::OnAssignedZoneSelected to compute a display string (use std::to_string(zone_id) when zone_name.empty()) and pass that display string to both zone_input->SetValue and g_brush_manager.SetSelectedZone (or call the brush manager overload that accepts an ID if available), ensuring assigned_zone_ids and selection logic remain unchanged and that unnamed zones activate the brush using their numeric ID.source/io/map_xml_io.cpp (1)
365-407: Consider using RAII guard pattern for creature reset consistency.
saveSpawns()uses aResetSavedGuardRAII pattern (lines 180-189) for exception safety when resetting creatures, butsaveNpcSpawns()uses a manual loop. While not critical, using the same pattern would ensure creatures are reset even if an exception occurs during serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/io/map_xml_io.cpp` around lines 365 - 407, The manual collection and final loop resetting saved_creatures in saveNpcSpawns() should be replaced with the same RAII pattern used in saveSpawns(): declare a ResetSavedGuard (the guard that resets creatures on destruction) scoped to cover the region where you call creature->save(), push creatures into the guard instead of saved_creatures, and remove the explicit for-loop that calls creature->reset(); ensure you still call creature->save() where currently done and reference the same Creature* instances (creature, saved_creatures) so exception safety matches saveSpawns().source/item_definitions/formats/protobuf/protobuf_item_parser.cpp (1)
386-396: Missing bounds check for object ID in catalog.entries.While the vector is sized for
item_count + creature_count + 1, the code at line 392 accessescatalog.entries[object.id()]directly. If an object has an ID greater thanitem_count(which is the max observed), this would still be in bounds, but if the appearances file is malformed with gaps or out-of-order IDs exceeding the vector size, this could cause issues.🛡️ Add bounds check
for (const auto& object : appearances.object()) { if (!object.has_id()) { warnings.push_back("Skipping protobuf object appearance without an id."); continue; } + if (object.id() >= catalog.entries.size()) { + warnings.push_back(std::format("Skipping protobuf object appearance {} - id out of range.", object.id())); + continue; + } + auto& entry = catalog.entries[object.id()];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/item_definitions/formats/protobuf/protobuf_item_parser.cpp` around lines 386 - 396, The loop over appearances.object() accesses catalog.entries[object.id()] without verifying object.id() is within the entries vector; update the loop in protobuf_item_parser.cpp to check that object.has_id() is true and that object.id() is less than catalog.entries.size() before indexing, and if out of bounds either resize catalog.entries appropriately or push a warning and continue; ensure you reference the same symbols (appearances.object(), object.id(), catalog.entries, and fillEntry) so the bounds check happens before calling fillEntry or indexing into catalog.entries.source/rendering/core/sprite_archive.cpp (1)
316-327: LZMA decompression expects exactlyLZMA_STREAM_END.The check
if (ret != LZMA_STREAM_END)will fail onLZMA_OKwhich could occur if the decompressed data exactly fills the buffer. While unlikely given the fixed sheet size, consider also acceptingLZMA_OKwhenavail_out == 0:♻️ More robust check
const lzma_ret ret = lzma_code(&stream, LZMA_RUN); lzma_end(&stream); - if (ret != LZMA_STREAM_END) { + if (ret != LZMA_STREAM_END && !(ret == LZMA_OK && stream.avail_out == 0)) { spdlog::error("SpriteArchive: failed to decode protobuf sprite sheet {} (lzma ret={})", sheet.path, static_cast<int>(ret)); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_archive.cpp` around lines 316 - 327, The decompression currently treats any return other than LZMA_STREAM_END as failure; change the check after lzma_code so that lzma_code's return is accepted as success if it is LZMA_STREAM_END OR it is LZMA_OK with the output buffer fully consumed (i.e., stream.avail_out == 0). Update the failure condition that checks ret (from the lzma_code call) to allow (ret == LZMA_OK && stream.avail_out == 0) as a valid successful case when filling decompressed (kSheetBytes + kBmpHeaderPadding) via stream.avail_out.source/map/map_spawn_manager.cpp (1)
51-71: Potential infinite loop iftarget_countexceeds actual spawns.The loop
while (found < target_count)relies on themax_radiusbreak condition (line 68-70) to terminate when spawns aren't found. However,max_radiusis computed from map dimensions which could be very large. Iftarget_countfromtile_loc->getSpawnCount()is incorrect (e.g., corrupted data or desync), this could iterate for a very long time.Consider adding an iteration limit or using the spawn's actual radius bounds:
♻️ Proposed safeguard
const int max_radius = std::max(map.getWidth(), map.getHeight()); + constexpr int kMaxIterations = 1000; // Reasonable upper bound + int iterations = 0; while (found < target_count) { + if (++iterations > kMaxIterations) { + spdlog::warn("getSpawnListImpl: Exceeded max iterations, spawn count may be inconsistent"); + break; + } for (int x = start_x; x <= end_x; ++x) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/map_spawn_manager.cpp` around lines 51 - 71, The while-loop guarded by "found < target_count" can spin for too long; add an explicit iteration cap and/or use the spawn's actual radius instead of map-derived max_radius to ensure termination. Concretely, in the loop around check_tile(...) (variables start_x/start_y/end_x/end_y and helper check_tile), introduce a loop counter (e.g., iterations) and a MAX_ITERATIONS constant (or derive max from tile_loc->getSpawnRadius() if available) and break with a logged warning when iterations exceed the cap; ensure found/target_count logic still governs normal exit and include the tile identifier in the log for diagnostics.source/protobuf/appearances.proto (1)
1-3: Package directory mismatch flagged by Buf linter.The protobuf package
canary.protobuf.appearancesdoesn't match the file's directory structure (source/protobuf/). While this works for code generation, it violates protobuf best practices where package paths should mirror directory structure.This is a low-priority style issue. If you want to address it, either:
- Move the file to
source/canary/protobuf/appearances/appearances.proto, or- Change the package to match the current location (e.g.,
source.protobuf)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/protobuf/appearances.proto` around lines 1 - 3, The proto package declaration in appearances.proto ("package canary.protobuf.appearances") does not match the file's directory, triggering the Buf linter; fix by either moving appearances.proto into a matching directory structure (e.g., place the file under source/canary/protobuf/appearances/ and keep package canary.protobuf.appearances) or change the package line in appearances.proto to reflect the current path (e.g., package source.protobuf) and update any import or generated code references that rely on the package name; locate the package statement in appearances.proto to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41c28244-a79b-4164-b774-849e8be5b7a0
📒 Files selected for processing (113)
.gitignoreCMakeLists.txtdata/clients.tomlsource/CMakeLists.txtsource/app/client_asset_detector.cppsource/app/client_version.cppsource/app/client_version.hsource/app/managers/version_manager.cppsource/app/preferences/client_version_page.cppsource/app/settings.cppsource/app/settings.hsource/brushes/brush.cppsource/brushes/brush.hsource/brushes/creature/creature_brush.cppsource/brushes/managers/brush_manager.cppsource/brushes/managers/brush_manager.hsource/brushes/spawn/npc_spawn_brush.cppsource/brushes/spawn/npc_spawn_brush.hsource/brushes/zone/zone_brush.cppsource/brushes/zone/zone_brush.hsource/editor/action.cppsource/editor/operations/copy_operations.cppsource/editor/operations/draw_operations.cppsource/editor/operations/selection_operations.cppsource/editor/persistence/editor_persistence.cppsource/game/creatures.cppsource/game/materials.cppsource/game/materials.hsource/game/spawn.hsource/io/iomap_otbm.cppsource/io/map_xml_io.cppsource/io/map_xml_io.hsource/io/otbm/header_serialization_otbm.cppsource/io/otbm/item_serialization_otbm.cppsource/io/otbm/otbm_types.hsource/io/otbm/tile_serialization_otbm.cppsource/io/xml_file_loader.cppsource/io/xml_file_loader.hsource/item_definitions/core/asset_bundle_loader.cppsource/item_definitions/core/item_definition_fragments.hsource/item_definitions/core/item_definition_recipe.cppsource/item_definitions/core/item_definition_resolver.cppsource/item_definitions/core/item_definition_store.cppsource/item_definitions/core/item_definition_store.hsource/item_definitions/core/item_definition_types.hsource/item_definitions/core/item_definitions_loader.cppsource/item_definitions/formats/dat/dat_catalog.hsource/item_definitions/formats/dat/dat_item_parser.cppsource/item_definitions/formats/protobuf/protobuf_item_parser.cppsource/item_definitions/formats/protobuf/protobuf_item_parser.hsource/item_definitions/formats/xml/xml_item_parser.cppsource/map/map.cppsource/map/map.hsource/map/map_region.cppsource/map/map_region.hsource/map/map_spawn_manager.cppsource/map/map_spawn_manager.hsource/map/tile.cppsource/map/tile.hsource/map/tile_operations.cppsource/map/tileset.cppsource/palette/palette_creature.cppsource/palette/palette_creature.hsource/protobuf/CMakeLists.txtsource/protobuf/appearances.protosource/rendering/core/atlas_manager.cppsource/rendering/core/atlas_manager.hsource/rendering/core/game_sprite.cppsource/rendering/core/game_sprite.hsource/rendering/core/graphics_assembler.cppsource/rendering/core/graphics_assembler.hsource/rendering/core/image.cppsource/rendering/core/image.hsource/rendering/core/normal_image.cppsource/rendering/core/normal_image.hsource/rendering/core/sprite_archive.cppsource/rendering/core/sprite_archive.hsource/rendering/core/sprite_preloader.cppsource/rendering/core/sprite_preloader.hsource/rendering/core/template_image.cppsource/rendering/core/template_image.hsource/rendering/core/texture_atlas.cppsource/rendering/core/texture_atlas.hsource/rendering/drawers/entities/creature_drawer.cppsource/rendering/drawers/entities/item_drawer.cppsource/rendering/drawers/entities/sprite_drawer.cppsource/rendering/drawers/overlays/marker_drawer.cppsource/rendering/ui/brush_selector.cppsource/rendering/ui/brush_selector.hsource/rendering/ui/drawing_controller.cppsource/rendering/ui/map_menu_handler.cppsource/rendering/ui/map_menu_handler.hsource/rendering/ui/selection_controller.cppsource/rendering/ui/tooltip_drawer.cppsource/rendering/utilities/sprite_icon_generator.cppsource/rendering/utilities/tile_describer.cppsource/ui/dialog_helper.cppsource/ui/gui.cppsource/ui/gui_ids.hsource/ui/map_popup_menu.cppsource/ui/tile_properties/spawn_creature_panel.cppsource/ui/tile_properties/spawn_creature_panel.hsource/ui/tile_properties/spawn_property_panel.cppsource/ui/tile_properties/spawn_property_panel.hsource/ui/tile_properties/tile_properties_panel.cppsource/ui/tile_properties/tile_properties_panel.hsource/ui/tile_properties/zone_property_panel.cppsource/ui/tile_properties/zone_property_panel.hsource/ui/tool_options_surface.cppsource/ui/tool_options_surface.hsource/util/nanovg_canvas.cppsource/util/nvg_utils.hvcpkg.json
💤 Files with no reviewable changes (1)
- source/game/materials.h
| [[clients]] | ||
| configType = 'dat_otb' | ||
| datSignature = '0' | ||
| dataDirectory = '1287' | ||
| description = '' | ||
| extended = false | ||
| frameDurations = false | ||
| frameGroups = false | ||
| metadataFile = 'Tibia.dat' | ||
| name = 'New Client' | ||
| otbId = 0 | ||
| otbMajor = 1 | ||
| otbmVersions = [] | ||
| sprSignature = '0' | ||
| spritesFile = 'Tibia.spr' | ||
| transparency = false | ||
| version = 0 No newline at end of file |
There was a problem hiding this comment.
New client entry appears to be a template/placeholder.
The new client 'New Client' has version = 0, empty otbmVersions = [], zeroed signatures, and an empty description. This looks like an incomplete template entry that may have been added accidentally.
Consider removing this entry or completing the configuration if it's intended for a specific client version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/clients.toml` around lines 1225 - 1241, The TOML contains a placeholder
client block with name 'New Client', configType 'dat_otb', version = 0, empty
otbmVersions = [], zeroed signatures (datSignature/sprSignature = '0') and no
description; remove this entire [[clients]] entry or replace it with a completed
configuration for the intended client (populate version, otbmVersions,
signatures, description, and proper file names) so the client list no longer
contains an incomplete template.
| if (tile->npc_spawn && tile->npc_spawn->isSelected()) { | ||
| copied_tile->npc_spawn = tile->npc_spawn->deepCopy(); | ||
| } | ||
| copied_tile->zone_ids = tile->zone_ids; |
There was a problem hiding this comment.
Only transfer zones when the tile itself is selected.
zone_ids are copied into the buffer for every selected tile, even when only an item/spawn on that tile is selected. That makes partial copy/cut pull zone metadata unintentionally, and cut() also leaves the same zones behind on the source tile.
Proposed fix
if (tile->ground && tile->ground->isSelected()) {
copied_tile->house_id = tile->house_id;
copied_tile->setMapFlags(tile->getMapFlags());
+ copied_tile->zone_ids = tile->zone_ids;
}
@@
- copied_tile->zone_ids = tile->zone_ids; if (tile->ground && tile->ground->isSelected()) {
copied_tile->house_id = newtile->house_id;
newtile->house_id = 0;
copied_tile->setMapFlags(tile->getMapFlags());
newtile->setMapFlags(TILESTATE_NONE);
+ copied_tile->zone_ids = newtile->zone_ids;
+ newtile->zone_ids.clear();
}
@@
- copied_tile->zone_ids = newtile->zone_ids;Also applies to: 122-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/editor/operations/copy_operations.cpp` around lines 57 - 60, The code
currently copies tile->zone_ids into copied_tile for every buffered selection;
change this so zone_ids are copied only when the tile itself is selected (not
when only an item/spawn on it is selected). Concretely, wrap the zone transfer
(copied_tile->zone_ids = tile->zone_ids) with a check that the tile is selected
(use the tile selection predicate used elsewhere, e.g. tile->isSelected() or the
same selection check used for items) in the block where npc_spawn is handled,
and make the identical change at the other occurrence (the block around lines
122-125). Also ensure the cut() path uses the same conditional so zones are not
left behind/removed incorrectly.
| Tile* tile = editor.map.getTile(offset); | ||
| std::unique_ptr<Tile> new_tile; | ||
| if (tile) { | ||
| new_tile = TileOperations::deepCopy(tile, editor.map); | ||
| } else { | ||
| new_tile = editor.map.allocator(editor.map.createTileL(offset)); | ||
| } | ||
|
|
||
| if (dodraw) { | ||
| brush->draw(&editor.map, new_tile.get(), nullptr); | ||
| } else { | ||
| brush->undraw(&editor.map, new_tile.get()); | ||
| } | ||
| action->addChange(std::make_unique<Change>(std::move(new_tile))); | ||
| batch->addAndCommitAction(std::move(action)); |
There was a problem hiding this comment.
Don't create a tile when erasing a zone from empty space.
When dodraw == false and editor.map.getTile(offset) is nullptr, this branch still allocates a fresh tile and commits it. That makes a no-op erase create empty tiles on the map.
Suggested fix
Tile* tile = editor.map.getTile(offset);
- std::unique_ptr<Tile> new_tile;
- if (tile) {
- new_tile = TileOperations::deepCopy(tile, editor.map);
- } else {
- new_tile = editor.map.allocator(editor.map.createTileL(offset));
- }
+ if (!tile && !dodraw) {
+ return;
+ }
+
+ std::unique_ptr<Tile> new_tile = tile
+ ? TileOperations::deepCopy(tile, editor.map)
+ : editor.map.allocator(editor.map.createTileL(offset));📝 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.
| Tile* tile = editor.map.getTile(offset); | |
| std::unique_ptr<Tile> new_tile; | |
| if (tile) { | |
| new_tile = TileOperations::deepCopy(tile, editor.map); | |
| } else { | |
| new_tile = editor.map.allocator(editor.map.createTileL(offset)); | |
| } | |
| if (dodraw) { | |
| brush->draw(&editor.map, new_tile.get(), nullptr); | |
| } else { | |
| brush->undraw(&editor.map, new_tile.get()); | |
| } | |
| action->addChange(std::make_unique<Change>(std::move(new_tile))); | |
| batch->addAndCommitAction(std::move(action)); | |
| Tile* tile = editor.map.getTile(offset); | |
| if (!tile && !dodraw) { | |
| return; | |
| } | |
| std::unique_ptr<Tile> new_tile = tile | |
| ? TileOperations::deepCopy(tile, editor.map) | |
| : editor.map.allocator(editor.map.createTileL(offset)); | |
| if (dodraw) { | |
| brush->draw(&editor.map, new_tile.get(), nullptr); | |
| } else { | |
| brush->undraw(&editor.map, new_tile.get()); | |
| } | |
| action->addChange(std::make_unique<Change>(std::move(new_tile))); | |
| batch->addAndCommitAction(std::move(action)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/editor/operations/draw_operations.cpp` around lines 381 - 395, The
code currently allocates a new Tile when editor.map.getTile(offset) is nullptr
even for erases (dodraw == false), which creates empty tiles; change the
allocation logic so you only create a new_tile when there is an existing tile or
when dodraw is true: use Tile* tile = editor.map.getTile(offset); if (tile)
new_tile = TileOperations::deepCopy(tile, editor.map); else if (dodraw) new_tile
= editor.map.allocator(editor.map.createTileL(offset)); else return/skip so you
do not call brush->undraw, action->addChange or batch->addAndCommitAction for a
no-op erase.
| if (map.getVersion().otbm < MAP_OTBM_5 && (!map.spawnnpcfile.empty() || !map.zonefile.empty() || !map.npc_spawns.empty() || !map.zones.empty() || hasZoneAssignments(map))) { | ||
| error("OTBM %d cannot store NPC spawn or zone data. Save this map as OTBM 5 or 6.", static_cast<int>(map.getVersion().otbm)); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Don't use default auxiliary filenames as proof that OTBM 5+ data exists.
loadMapFromDisk() populates map.spawnnpcfile / map.zonefile with default names even when no NPC spawn or zone XML exists, so this guard can reject re-saving an unchanged legacy OTBM 4 map. Gate this on actual data presence, not on filename fields.
Suggested fix
- if (map.getVersion().otbm < MAP_OTBM_5 && (!map.spawnnpcfile.empty() || !map.zonefile.empty() || !map.npc_spawns.empty() || !map.zones.empty() || hasZoneAssignments(map))) {
+ const bool hasNpcSpawnData = !map.npc_spawns.empty();
+ const bool hasZoneData = !map.zones.empty() || hasZoneAssignments(map);
+ if (map.getVersion().otbm < MAP_OTBM_5 && (hasNpcSpawnData || hasZoneData)) {
error("OTBM %d cannot store NPC spawn or zone data. Save this map as OTBM 5 or 6.", static_cast<int>(map.getVersion().otbm));
return false;
}📝 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.
| if (map.getVersion().otbm < MAP_OTBM_5 && (!map.spawnnpcfile.empty() || !map.zonefile.empty() || !map.npc_spawns.empty() || !map.zones.empty() || hasZoneAssignments(map))) { | |
| error("OTBM %d cannot store NPC spawn or zone data. Save this map as OTBM 5 or 6.", static_cast<int>(map.getVersion().otbm)); | |
| return false; | |
| } | |
| const bool hasNpcSpawnData = !map.npc_spawns.empty(); | |
| const bool hasZoneData = !map.zones.empty() || hasZoneAssignments(map); | |
| if (map.getVersion().otbm < MAP_OTBM_5 && (hasNpcSpawnData || hasZoneData)) { | |
| error("OTBM %d cannot store NPC spawn or zone data. Save this map as OTBM 5 or 6.", static_cast<int>(map.getVersion().otbm)); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/io/iomap_otbm.cpp` around lines 423 - 426, The guard wrongly treats
non-empty map.spawnnpcfile/map.zonefile as proof of OTBM 5+ data because
loadMapFromDisk may fill those with default names; update the conditional in
iomap_otbm.cpp so it only rejects saving as OTBM < MAP_OTBM_5 when actual
NPC/zone data exists (use map.npc_spawns, map.zones and hasZoneAssignments(map)
or an explicit check for real XML data), removing the filename checks
(map.spawnnpcfile/map.zonefile) from the condition that references
map.getVersion().otbm and MAP_OTBM_5/6.
| const auto dimensions = image->getDimensions(); | ||
|
|
||
| int part_x = (gs->width - sw - 1) * 32; | ||
| int part_y = (gs->height - sh - 1) * 32; | ||
| int part_x = (gs->width - sw - 1) * SPRITE_PIXELS; | ||
| int part_y = (gs->height - sh - 1) * SPRITE_PIXELS; | ||
|
|
||
| for (int sy = 0; sy < 32; ++sy) { | ||
| for (int sx = 0; sx < 32; ++sx) { | ||
| for (int sy = 0; sy < dimensions.height; ++sy) { | ||
| for (int sx = 0; sx < dimensions.width; ++sx) { | ||
| int dy = part_y + sy; | ||
| int dx = part_x + sx; | ||
| int di = (dy * w + dx) * 4; | ||
| int si = (sy * 32 + sx) * 4; | ||
| int si = (sy * dimensions.width + sx) * 4; |
There was a problem hiding this comment.
Potential out-of-bounds write when sprite dimensions exceed allocated buffer.
The composite buffer is sized based on gs->GetSize() (lines 231-232), but the inner loop iterates based on each individual sprite's image->getDimensions(). If any sprite's dimensions exceed the expected part boundaries (calculated using SPRITE_PIXELS), the destination indices dx and dy could exceed the buffer bounds.
Consider adding bounds checking before writing to the composite buffer:
🛡️ Proposed fix
for (int sy = 0; sy < dimensions.height; ++sy) {
for (int sx = 0; sx < dimensions.width; ++sx) {
int dy = part_y + sy;
int dx = part_x + sx;
+ if (dx < 0 || dx >= w || dy < 0 || dy >= h) {
+ continue;
+ }
int di = (dy * w + dx) * 4;
int si = (sy * dimensions.width + sx) * 4;📝 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.
| const auto dimensions = image->getDimensions(); | |
| int part_x = (gs->width - sw - 1) * 32; | |
| int part_y = (gs->height - sh - 1) * 32; | |
| int part_x = (gs->width - sw - 1) * SPRITE_PIXELS; | |
| int part_y = (gs->height - sh - 1) * SPRITE_PIXELS; | |
| for (int sy = 0; sy < 32; ++sy) { | |
| for (int sx = 0; sx < 32; ++sx) { | |
| for (int sy = 0; sy < dimensions.height; ++sy) { | |
| for (int sx = 0; sx < dimensions.width; ++sx) { | |
| int dy = part_y + sy; | |
| int dx = part_x + sx; | |
| int di = (dy * w + dx) * 4; | |
| int si = (sy * 32 + sx) * 4; | |
| int si = (sy * dimensions.width + sx) * 4; | |
| const auto dimensions = image->getDimensions(); | |
| int part_x = (gs->width - sw - 1) * SPRITE_PIXELS; | |
| int part_y = (gs->height - sh - 1) * SPRITE_PIXELS; | |
| for (int sy = 0; sy < dimensions.height; ++sy) { | |
| for (int sx = 0; sx < dimensions.width; ++sx) { | |
| int dy = part_y + sy; | |
| int dx = part_x + sx; | |
| if (dx < 0 || dx >= w || dy < 0 || dy >= h) { | |
| continue; | |
| } | |
| int di = (dy * w + dx) * 4; | |
| int si = (sy * dimensions.width + sx) * 4; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/util/nanovg_canvas.cpp` around lines 260 - 270, The loops that copy
each sprite (using image->getDimensions(), sy/sx) compute target coords
part_x/part_y using SPRITE_PIXELS then write into the composite buffer via
dx/dy/di, but there is no bounds check so large sprite dimensions can write out
of range; fix by clamping or skipping writes whose computed dx or dy fall
outside the composite buffer width/height (check 0 <= dx < w and 0 <= dy < h
before computing di and writing), and ensure any partial overlaps are handled by
only copying the valid sub-region for the sprite.
| const auto dimensions = gs.spriteList[spriteIdx]->getDimensions(); | ||
|
|
||
| // Right-to-left, bottom-to-top arrangement (standard RME rendering order) | ||
| int part_x = (gs.width - w - 1) * 32; | ||
| int part_y = (gs.height - h - 1) * 32; | ||
|
|
||
| for (int sy = 0; sy < 32; ++sy) { | ||
| for (int sx = 0; sx < 32; ++sx) { | ||
| for (int sy = 0; sy < dimensions.height; ++sy) { | ||
| for (int sx = 0; sx < dimensions.width; ++sx) { |
There was a problem hiding this comment.
Composite placement still assumes 32px slots.
The copy loop now respects getDimensions(), but part_x/part_y are still derived from * 32. For multi-part variable-sized sprites, the preview composite will be shifted or cropped even though the output buffer size is now dynamic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/util/nvg_utils.h` around lines 51 - 58, The composite placement uses a
hardcoded 32px multiplier for part_x/part_y causing misalignment for
variable-sized parts; change the computation to use the sprite dimensions
returned by getDimensions() instead of 32 (e.g., multiply by dimensions.width
and dimensions.height or otherwise use dimensions.*) so part_x and part_y are
calculated from the actual part size; update the code around
gs.spriteList[spriteIdx]->getDimensions(), part_x and part_y to use those values
(and ensure the rest of the copy loop that iterates over dimensions.width/height
remains consistent).
Added defensive checks, bounds validation, and proper error propagation across I/O, rendering, and map systems to prevent crashes and silent failures. - `CMakeLists.txt` — enabled `protobuf_MODULE_COMPATIBLE` for compatible protobuf module loading - `source/app/client_asset_detector.cpp` — added metadata file existence guard and missing `break` after appearances type match - `source/editor/operations/selection_operations.cpp` — added `std::move` and `clear` on `zone_ids` when transferring selection - `source/game/creatures.cpp` — added warning logs when skipping monster entries with missing file attribute or failed load - `source/io/map_xml_io.cpp` — added null tile check after `createTile` for NPC spawn with warning log - `source/io/xml_file_loader.cpp` — changed XML include errors to propagate `include_error` instead of warning-only, added `state.visiting.erase` and early return - `source/map/map_region.cpp` — added missing `npc_spawn_count` copy in `TileLocation::clone()` - `source/map/map_spawn_manager.cpp` — added iteration counter with cap warning for spawn lookup, added `spdlog` include - `source/map/tile_operations.cpp` — added `src->zone_ids.clear()` after copying `zone_ids` to dest - `source/rendering/core/image.cpp` — added zero-dimension validation before fallback magenta texture creation - `source/rendering/core/sprite_archive.cpp` — added `MAX_SPRITES` validation, split `sprite_id==0` handling in `readProtobufRgba`, added decoded pixels release and `last_decoded_sheet_index_` tracking - `source/rendering/core/sprite_archive.h` — added `releaseDecodedPixels()` method and `last_decoded_sheet_index_` member to `ProtobufSheet` - `source/rendering/core/template_image.cpp` — added base/mask dimension mismatch checks, switched to `base_dimensions.pixelCount()` for colorize calls
|
🤖 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. |
Removed the `last_decoded_sheet_index_` cache and `releaseDecodedPixels()` helper, eliminating conditional pixel release logic that was likely ineffective or unnecessary. - `source/rendering/core/sprite_archive.cpp` — removed conditional release of previously decoded sheet pixels and cleared the cache index assignment - `source/rendering/core/sprite_archive.h` — removed `releaseDecodedPixels()` method from `ProtobufSheet` struct and `last_decoded_sheet_index_` member variable
… calculations Extracted per-frame sprite metric computations into cached methods on GameSprite, eliminating redundant traversal of atlas regions across creature, item, and sprite drawers. Geometry cache now tracks composite size and draw offset with invalidation support. - `source/rendering/core/game_sprite.cpp` — added `resolvePlainSpriteIndex()` and `resolveOutfitSpriteIndex()` helpers, introduced `rebuildGeometryCache()`, `rebuildPlainLayoutMetrics()`, and `rebuildOutfitLayoutMetrics()` with cache invalidation via `invalidateMetricCaches()` - `source/rendering/core/game_sprite.h` — declared `SpriteLayoutMetrics` struct, public getter methods for plain/outfit layout metrics, private cache key structs and rebuild functions, added mutable cache state members - `source/rendering/core/graphics_assembler.cpp` — set `image->parent` pointer when installing sprite entries to enable parent invalidation - `source/rendering/core/normal_image.cpp` — extracted dimension update into `updateDimensions()` helper that invalidates parent metric caches when size changes - `source/rendering/core/sprite_preloader.cpp` — added dimension change detection during preload fulfillment with parent metric cache invalidation - `source/rendering/drawers/entities/creature_drawer.cpp` — removed local `OutfitSpriteMetrics` struct and `computeOutfitSpriteMetrics()`, switched to `getOutfitLayoutMetrics()`, updated light registration to use `SpriteLayoutMetrics` - `source/rendering/drawers/entities/item_drawer.cpp` — removed local `CompositeSpriteMetrics` struct and `computeSpriteMetrics()`, switched to `getPlainLayoutMetrics()`, simplified light registration signature - `source/rendering/drawers/entities/sprite_drawer.cpp` — removed inline column/row width computation, switched to `getPlainLayoutMetrics()`, cached draw offset before use
|
🤖 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 significant features for the editor, including support for Protobuf-based client assets, a dedicated NPC spawn system, and map zones. While the overall implementation is solid and well-structured, I identified a critical out-of-bounds vulnerability in the sprite sheet reading logic and a potential issue with 16-bit item ID truncation that could affect modern client versions.
🔍 General Feedback
- Protobuf Integration: Excellent work on the Protobuf item parser and LZMA decompression logic. It's great to see support for newer client formats.
- Map Zones: The implementation of named zones with persistent XML storage is a very welcome feature.
- Architectural consistency: The new
XmlFileLoaderand the refactoredMaterialsloading are much cleaner and maintainable. - Safety: Please ensure that the bounds checks are added in
SpriteArchiveandNanoVGCanvasas mentioned in the comments to prevent crashes or security issues.
|
|
||
| const uint32_t sprite_offset = sprite_id - sheet.first_id; | ||
| const int columns = kSheetDimension / source_width; | ||
| const int sprite_row = static_cast<int>(sprite_offset / static_cast<uint32_t>(columns)); |
There was a problem hiding this comment.
If sprite_offset is large enough that sprite_row * source_height equals kSheetDimension, the source_offset will start exactly at the end of the decoded_pixels buffer, and any row >= 0 will cause an out-of-bounds read. A 384x384 sheet can fit at most 144 32x32 sprites (12x12). If sprite_offset is 144 or greater, it will overflow.
You should add a check during catalog loading or here to ensure the sprite fits within the sheet dimensions.
| const int sprite_row = static_cast<int>(sprite_offset / static_cast<uint32_t>(columns)); | |
| const uint32_t sprite_offset = sprite_id - sheet.first_id; | |
| const int columns = kSheetDimension / source_width; | |
| const int rows = kSheetDimension / source_height; | |
| if (sprite_offset >= static_cast<uint32_t>(columns * rows)) { | |
| spdlog::error("SpriteArchive: sprite {} offset {} out of bounds for sheet {}", sprite_id, sprite_offset, sheet.path); | |
| return false; | |
| } | |
| const int sprite_row = static_cast<int>(sprite_offset / static_cast<uint32_t>(columns)); |
| catalog.has_transparency = true; | ||
| catalog.has_frame_durations = true; | ||
| catalog.has_frame_groups = true; | ||
| catalog.item_count = static_cast<uint16_t>(std::min<uint32_t>(item_count, std::numeric_limits<uint16_t>::max())); |
There was a problem hiding this comment.
Truncating item_count and creature_count to uint16_t will cause data loss for items with IDs > 65,535. While the RME architecture might be 16-bit internally for many structures, this parser should at least warn if IDs are being truncated, or RME should be updated to use 32-bit ClientItemId.
| catalog.item_count = static_cast<uint16_t>(std::min<uint32_t>(item_count, std::numeric_limits<uint16_t>::max())); | |
| const uint32_t final_item_count = std::min<uint32_t>(item_count, std::numeric_limits<uint16_t>::max()); | |
| if (item_count > std::numeric_limits<uint16_t>::max()) { | |
| warnings.push_back(std::format("Protobuf appearances file contains {} items, but only {} are supported by ClientItemId.", item_count, final_item_count)); | |
| } | |
| catalog.item_count = static_cast<uint16_t>(final_item_count); |
| for (int sx = 0; sx < dimensions.width; ++sx) { | ||
| int dy = part_y + sy; | ||
| int dx = part_x + sx; | ||
| int di = (dy * w + dx) * 4; |
There was a problem hiding this comment.
Unlike nvg_utils.h, this function lacks safety checks for the composite buffer write. While gs->GetSize() should provide enough space, it's safer to have them here as well.
| int di = (dy * w + dx) * 4; | |
| int dy = part_y + sy; | |
| int dx = part_x + sx; | |
| if (dx < 0 || dx >= w || dy < 0 || dy >= h) { | |
| continue; | |
| } | |
| int di = (dy * w + dx) * 4; |
| npcNode.append_attribute("name") = creature->getName().c_str(); | ||
| npcNode.append_attribute("x") = x; | ||
| npcNode.append_attribute("y") = y; | ||
| npcNode.append_attribute("z") = spawnPosition.z; |
There was a problem hiding this comment.
The loadNpcSpawns function correctly uses spawnPosition.z (the center of the spawn circle) and only reads x and y offsets from individual npc nodes. Adding z to individual npc nodes is redundant and inconsistent with how regular monster/npc spawns are saved in spawn.xml.
| npcNode.append_attribute("z") = spawnPosition.z; | |
| npcNode.append_attribute("name") = creature->getName().c_str(); | |
| npcNode.append_attribute("x") = x; | |
| npcNode.append_attribute("y") = y; | |
| npcNode.append_attribute("spawntime") = creature->getSpawnTime(); |
| const auto dimensions = gs.spriteList[spriteIdx]->getDimensions(); | ||
|
|
||
| // Right-to-left, bottom-to-top arrangement (standard RME rendering order) | ||
| int part_x = (gs.width - w - 1) * 32; |
There was a problem hiding this comment.
It's better to use the SPRITE_PIXELS constant consistently throughout the project, as done in nanovg_canvas.cpp.
| int part_x = (gs.width - w - 1) * 32; | |
| // Right-to-left, bottom-to-top arrangement (standard RME rendering order) | |
| int part_x = (gs.width - w - 1) * SPRITE_PIXELS; | |
| int part_y = (gs.height - h - 1) * SPRITE_PIXELS; |
Increased atlas initial capacity from 16 to 32 layers and PBO ring from 2 to 4 buffers to reduce stalls during large pans. Extracted per-floor view bounds calculation to replace shared view mutation, and throttled sprite uploads to 128 per frame. - `source/rendering/core/atlas_manager.cpp` — doubled INITIAL_LAYERS from 16 to 32 for larger sprite capacity - `source/rendering/core/pixel_buffer_object.h` — increased BUFFER_COUNT from 2 to 4 for better burst handling - `source/rendering/core/render_view.cpp` — extracted floor bounds computation into getBoundsForFloor() with margin_pixels and offset logic - `source/rendering/core/render_view.h` — added getBoundsForFloor() declaration - `source/rendering/core/sprite_preloader.cpp` — capped result processing at MAX_UPLOADS_PER_FRAME, added queue size check inside enqueue loop - `source/rendering/core/sprite_preloader.h` — added MAX_UPLOADS_PER_FRAME = 128 constant - `source/rendering/core/texture_atlas.cpp` — changed layer growth from fixed +4 to exponential step based on current allocated_layers_ - `source/rendering/drawers/map_layer_drawer.cpp` — moved RegisterGroundLightOcclusion into tile draw lambda with lazy floor_light_start evaluation - `source/rendering/drawers/tiles/tile_renderer.cpp` — [minor] added blank line before closing namespace brace - `source/rendering/map_drawer.cpp` — computed per-floor RenderView bounds instead of mutating shared view, passed explicit draw_view to DrawMapLayer - `source/rendering/map_drawer.h` — updated DrawMapLayer signature to accept const RenderView& parameter
…ches Consolidated redundant Refresh() calls across UI components, replaced single-entry layout caches with 8-slot LRU caches, added byte-size limits to sprite preloader queues, and eliminated dynamic_cast usage in sprite retrieval. Reduced atlas initial layers from 32 to 16 to bound startup memory. - `source/game/animation_timer.cpp` — added early returns skipping refresh when zoom > 2.0, canvas hidden, or tab inactive; included gui.h for tab check - `source/rendering/core/atlas_manager.cpp` — reduced INITIAL_LAYERS from 32 to 16 to bound startup memory - `source/rendering/core/game_sprite.cpp` — replaced single-entry layout caches with 8-slot LRU deques, extracted build methods returning metrics by value, added null/empty guards in isSimpleAndLoaded() - `source/rendering/core/game_sprite.h` — added PlainLayoutCacheEntry/OutfitLayoutCacheEntry structs, replaced cache validity flags and single entries with std::deque members - `source/rendering/core/graphics.cpp` — added getGameSprite() returning typed GameSprite* with bounds checks, avoiding dynamic_cast - `source/rendering/core/graphics.h` — declared [[nodiscard]] getGameSprite() method - `source/rendering/core/render_view.cpp` — simplified getBoundsForFloor() to integer arithmetic, removed <cmath> dependency, added fixed offsets for underground floors - `source/rendering/core/sprite_archive.cpp` — released previous sheet's decoded pixels when switching sheets to reduce memory pressure - `source/rendering/core/sprite_archive.h` — added releaseDecodedPixels() helper and last_decoded_sheet_index_ tracker - `source/rendering/core/sprite_batch.cpp` — promoted used_sections from local to member variable, cleared between flush cycles - `source/rendering/core/sprite_batch.h` — added used_sections_ member to avoid per-flush allocation - `source/rendering/core/sprite_preloader.cpp` — added byte-size limits to result queue and per-frame uploads, tracked queued_result_bytes, extracted resultByteSize() helper - `source/rendering/core/sprite_preloader.h` — increased MAX_UPLOADS_PER_FRAME to 2048, added MAX_RESULT_QUEUE_SIZE (4096), MAX_RESULT_QUEUE_BYTES (64MB), MAX_UPLOAD_BYTES_PER_FRAME (16MB) constants and queued_result_bytes member - `source/rendering/core/texture_atlas.cpp` — fixed layer growth to increment by 4 instead of doubling, decremented total_sprite_count_ in freeSlot() - `source/rendering/drawers/cursors/live_cursor_drawer.cpp` — cached atlas_manager pointer, avoided mutating cursor.color by using local draw_color, added null guards - `source/rendering/drawers/entities/creature_drawer.cpp` — replaced dynamic_cast with getGameSprite() - `source/rendering/drawers/entities/item_drawer.cpp` — replaced dynamic_cast with getGameSprite() - `source/rendering/drawers/entities/sprite_drawer.cpp` — replaced dynamic_cast with getGameSprite() - `source/rendering/drawers/overlays/preview_drawer.cpp` — replaced full tile grid iteration with visitLeaves() sparse traversal, extracted drawPreviewTile lambda, precomputed source coordinates and offset - `source/rendering/map_drawer.cpp` — removed updateTime() call from Draw() - `source/rendering/ui/map_display.cpp` — simplified Refresh() to wxGLCanvas::Refresh(false), removed rate-limiting throttle - `source/rendering/ui/navigation_controller.cpp` — removed redundant Refresh() calls from arrow/drag handlers, consolidated into single call at end of ChangeFloor() - `source/rendering/ui/tooltip_drawer.cpp` — replaced dynamic_cast with getGameSprite() - `source/rendering/ui/zoom_controller.cpp` — added early return when zoom unchanged, added minimap sync when scroll delta is zero, cached map_window pointer - `source/ui/gui.cpp` — RefreshView() now refreshes only current tab instead of all tabs, added tabbook null guard - `source/ui/map_window.cpp` — added early returns in Scroll/ScrollRelative when position unchanged, moved Refresh() calls into Scroll/ScrollRelative, removed redundant calls from scroll event handlers
|
🤖 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. |
Refactored texture garbage collector to use incremental time-budgeted sweeps instead of a single blocking pass, eliminating periodic frame stalls. Increased GC call frequency from once-per-second to every frame on the active map tab since sweeps are now internally budgeted. - `source/rendering/core/texture_garbage_collector.cpp` — split single-pass GC into resumable loops over images and sprites with a 1ms time budget checked every 128 items, added `<chrono>` import - `source/rendering/core/texture_garbage_collector.h` — added `resident_image_cursor`, `resident_sprite_cursor`, and `sweep_in_progress` state fields - `source/rendering/ui/map_display.cpp` — removed 1-second cooldown on `PerformGarbageCollection()`, deleted `m_last_gc_time` member initialization - `source/rendering/ui/map_display.h` — removed `m_last_gc_time` member variable
|
🤖 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 significant features including Protobuf-based client asset support, NPC spawn management, and map zones. The implementation is comprehensive and follows existing architectural patterns.
🔍 General Feedback
- Protobuf Support: The integration of Protobuf and LZMA for modern client assets is well-implemented and provides necessary support for newer game versions.
- Map Zones: The new zone system is well-integrated into the Map class and persistence layer.
- UI Improvements: The new property panels and tool options for NPC spawns and zones provide a much-needed user interface for these features.
- Performance: Good optimizations were added to the animation timer to skip unnecessary refreshes.
| delete creatureType; | ||
| if (!current->brush) { | ||
| ensureCreatureBrush(current); | ||
| } |
There was a problem hiding this comment.
🟢 The explicit check for !current->brush is redundant as ensureCreatureBrush already performs this check at its beginning. This pattern is repeated in several places in this file.
| } | |
| ensureCreatureBrush(current); |
|
|
||
| const wxString previous_value = !zone_name_combo->GetValue().IsEmpty() | ||
| ? zone_name_combo->GetValue() | ||
| : wxstr(std::string { g_brush_manager.GetSelectedZone() }); |
There was a problem hiding this comment.
🟡 Wrapping g_brush_manager.GetSelectedZone() in std::string { ... } is redundant here. wxstr (or wxString::FromUTF8) can take the return value directly.
| : wxstr(std::string { g_brush_manager.GetSelectedZone() }); | |
| : wxstr(g_brush_manager.GetSelectedZone()); |
| for (const auto& spawnPos : map.npc_spawns) { | ||
| const Tile* tile = map.getTile(spawnPos); | ||
| if (!tile || !tile->npc_spawn) { | ||
| continue; |
There was a problem hiding this comment.
🟡 Using the same tag name <npc> for both the spawn center and the NPC instances within it is unconventional and might be confusing. Standard OT formats usually use <spawn> for the group and <npc> or <monster> for the instances.
| continue; | |
| pugi::xml_node rootNode = doc.append_child("npcs"); | |
| std::vector<Creature*> saved_creatures; | |
| for (const auto& spawnPos : map.npc_spawns) { | |
| const Tile* tile = map.getTile(spawnPos); | |
| if (!tile || !tile->npc_spawn) { | |
| continue; | |
| } | |
| const Position spawnPosition = spawnPos; | |
| pugi::xml_node spawnNode = rootNode.append_child("spawn"); | |
| spawnNode.append_attribute("centerx") = spawnPosition.x; |
| editor.map.setHouseFilename(nstr(_name.GetFullName())); | ||
| _name.SetName(filename.GetName() + "-waypoint"); | ||
| editor.map.setWaypointFilename(nstr(_name.GetFullName())); | ||
| _name.SetName(filename.GetName() + "-npc"); |
There was a problem hiding this comment.
🟡 Hardcoding the filename suffixes -npc and -zones directly in EditorPersistence makes them less configurable. It would be better to have these as constants or retrieved from a central configuration.
| _name.SetName(filename.GetName() + "-npc"); | |
| _name.SetName(filename.GetName() + "-npc"); | |
| editor.map.setSpawnNpcFilename(nstr(_name.GetFullName())); | |
| _name.SetName(filename.GetName() + "-zones"); | |
| editor.map.setZoneFilename(nstr(_name.GetFullName())); |
| const uint16_t zone_id = current_map->zones.ensureZone(zone_name); | ||
| if (zone_id == 0) { | ||
| return; | ||
| } | ||
| auto new_tile = TileOperations::deepCopy(current_tile, *current_map); | ||
| if (new_tile->hasZone(zone_id)) { | ||
| return; | ||
| } | ||
| new_tile->addZone(zone_id); | ||
|
|
||
| ApplyTileChange(std::move(new_tile), true); |
There was a problem hiding this comment.
🟡 Performance: g_gui.RefreshPalettes() is an expensive operation. It should only be triggered if a new zone was actually added to the ZoneRegistry. If ensureZone returned an existing ID, the palette doesn't need refreshing.
| const uint16_t zone_id = current_map->zones.ensureZone(zone_name); | |
| if (zone_id == 0) { | |
| return; | |
| } | |
| auto new_tile = TileOperations::deepCopy(current_tile, *current_map); | |
| if (new_tile->hasZone(zone_id)) { | |
| return; | |
| } | |
| new_tile->addZone(zone_id); | |
| ApplyTileChange(std::move(new_tile), true); | |
| const bool is_new_zone = !current_map->zones.findId(zone_name).has_value(); | |
| const uint16_t zone_id = current_map->zones.ensureZone(zone_name); | |
| if (zone_id == 0) { | |
| return; | |
| } | |
| auto new_tile = TileOperations::deepCopy(current_tile, *current_map); | |
| if (new_tile->hasZone(zone_id)) { | |
| return; | |
| } | |
| new_tile->addZone(zone_id); | |
| ApplyTileChange(std::move(new_tile), is_new_zone); |
| stream.next_out = decompressed.get(); | ||
| stream.avail_out = kSheetBytes + kBmpHeaderPadding; | ||
|
|
||
| const lzma_ret ret = lzma_code(&stream, LZMA_RUN); |
There was a problem hiding this comment.
🟢 For raw decoding of a single block where the entire compressed stream is expected to be in the input buffer, LZMA_FINISH is generally preferred over LZMA_RUN to ensure all data is flushed and the stream is properly terminated.
| const lzma_ret ret = lzma_code(&stream, LZMA_RUN); | |
| const lzma_ret ret = lzma_code(&stream, LZMA_FINISH); |
| } | ||
|
|
||
| const Position spawnPosition = spawnPos; | ||
| pugi::xml_node spawnNode = rootNode.append_child("npc"); |
There was a problem hiding this comment.
🟡 Consistency: Using the same tag name <npc> for both the spawn center and the NPC instances within it is unconventional and might be confusing. Standard OT formats usually use <spawn> for the group and <npc> or <monster> for the instances.
| pugi::xml_node spawnNode = rootNode.append_child("npc"); | |
| pugi::xml_node spawnNode = rootNode.append_child("spawn"); |
Implemented a dedicated Zone palette with per-tile assignment support and integrated Canary-specific spawn metadata including weights and custom attributes. Refactored palette management to include creature searching and independent configuration for NPC versus Monster spawns. - `data/clients.toml` — updated Canary client configuration to use protobuf and new asset paths - `data/menubar.xml` — added Zone palette entry to the View menu - `source/CMakeLists.txt` — added Zone palette source files to the build - `source/brushes/brush_enums.h` — added `TILESET_ZONES` category - `source/game/creature.cpp` — updated deep copy to include spawn weights and attributes - `source/game/creature.h` — added storage and accessors for spawn weights and custom string attributes - `source/game/creatures.cpp` — changed default monster tileset to "Monsters" - `source/game/materials.cpp` — explicitly created and populated "Monsters" and "NPCs" tilesets - `source/io/iomap_otbm.cpp` — added Canary metadata validation for OTBM saving and improved zone file handling - `source/io/map_xml_io.cpp` — refactored zone I/O to support per-tile assignments and implemented custom spawn attribute serialization - `source/io/map_xml_io.h` — [minor] updated class documentation - `source/map/map.cpp` — added `clearZoneAssignments` utility - `source/map/map.h` — declared zone assignment cleanup helper - `source/palette/house/house_palette.cpp` — refactored to inherit from `PalettePanel` for consistent window management - `source/palette/house/house_palette.h` — updated inheritance and added overrides for standard palette interface - `source/palette/palette_creature.cpp` — added search functionality and separated NPC/Monster spawn control synchronization - `source/palette/palette_creature.h` — added search control member and synchronization helpers - `source/palette/palette_window.cpp` — integrated Zone and House palettes into the main palette container - `source/palette/palette_window.h` — added House and Zone palette member pointers - `source/palette/palette_zone.cpp` — implemented new Zone palette UI for managing and selecting map zones - `source/palette/palette_zone.h` — defined `ZonePalettePanel` class interface - `source/rendering/drawers/overlays/marker_drawer.cpp` — removed legacy zone overlay rendering - `source/rendering/drawers/tiles/tile_color_calculator.cpp` — implemented tile color modulation for zone highlighting - `source/rendering/drawers/tiles/tile_renderer.cpp` — removed redundant zone sprite blitting - `source/rendering/ui/brush_selector.cpp` — updated zone selection logic to target the Zone palette - `source/ui/gui.cpp` — branched spawn radius settings between NPCs and Monsters - `source/ui/main_menubar.cpp` — added menu event handler for the Zone palette - `source/ui/main_menubar.h` — added `SELECT_ZONE` menu ID and handler declaration - `source/ui/map/map_properties_window.cpp` — added support for OTBM 5/6 and external NPC/Zone file configuration - `source/ui/map/map_properties_window.h` — added controls for external NPC and zone filenames - `source/ui/menubar/menubar_action_manager.cpp` — registered the Zone palette selection action with icons - `source/ui/menubar/palette_menu_handler.cpp` — refactored palette selection handlers and added Zone palette support - `source/ui/menubar/palette_menu_handler.h` — added Zone palette menu handler declaration - `source/ui/tile_properties/zone_property_panel.cpp` — improved zone selection state management - `source/ui/tool_options_surface.cpp` — implemented context-aware spawn controls and improved zone combo box synchronization - `source/ui/tool_options_surface.h` — added helpers for context-aware spawn control detection
|
🤖 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 significant features including Protobuf-based client asset loading, NPC spawn management, and persistent map zones. While the architecture follows existing patterns well, there are critical performance and security concerns in the core implementation that need to be addressed before merging.
🔍 General Feedback
- Security & Stability: Identified a critical out-of-bounds read vulnerability in the
SpriteArchivesheet decoding logic and a potential performance-related hang inMapSpawnManager. - Resource Management: The
TextureAtlasplacement strategy could lead to VRAM fragmentation over long sessions; suggested a more robust reuse strategy. - Maintainability: The integration of NPC spawns and zones is well-modularized and extends the existing XML I/O system cleanly.
- Code Quality: Overall code style is consistent with the project's standards, with good use of modern C++ features and spdlog for debugging.
| const int max_radius = std::max(map.getWidth(), map.getHeight()); | ||
| const int max_iterations = std::max(1, max_radius + 1); |
There was a problem hiding this comment.
🔴 Using map width/height as max_radius for spawn lookup is highly inefficient and could lead to significant performance hangs, especially on large maps (e.g., 30k x 30k) if found < target_count due to data inconsistencies. Since spawns have a restricted maximum radius, the search should be bounded by MAX_SPAWN_RADIUS.
| const int max_radius = std::max(map.getWidth(), map.getHeight()); | |
| const int max_iterations = std::max(1, max_radius + 1); | |
| const int max_radius = std::max(1, g_settings.getInteger(Config::MAX_SPAWN_RADIUS)); | |
| const int max_iterations = max_radius + 1; |
|
|
||
| uint32_t pixel_offset = 0; | ||
| std::memcpy(&pixel_offset, decompressed.get() + 10, sizeof(uint32_t)); | ||
| if (pixel_offset >= kSheetBytes + kBmpHeaderPadding) { |
There was a problem hiding this comment.
🔴 This check is insufficient to prevent out-of-bounds reads during the pixel copy loop. pixel_offset must be small enough such that pixel_offset + kSheetBytes does not exceed the size of the decompressed buffer. Currently, if pixel_offset is just below kSheetBytes + kBmpHeaderPadding, the subsequent std::memcpy at line 349 will access illegal memory.
| if (pixel_offset >= kSheetBytes + kBmpHeaderPadding) { | |
| if (pixel_offset + kSheetBytes > kSheetBytes + kBmpHeaderPadding) { |
| size_t& search_hint = single_slot ? small_search_hint_ : large_search_hint_; | ||
| for (;;) { | ||
| const size_t search_limit = static_cast<size_t>(layer_count_) * SLOTS_PER_LAYER; | ||
| for (size_t linear_index = search_hint; linear_index < search_limit; ++linear_index) { |
There was a problem hiding this comment.
🟠 The large_search_hint_ only increases and never resets, which can lead to significant VRAM fragmentation and leak as new layers are added even if older layers have enough free slots (since large sprites don't use free_slots_).
Consider either wrapping the search around to 0 once search_limit is reached, or resetting the hint when a layer is removed or periodically.
| size_t& search_hint = single_slot ? small_search_hint_ : large_search_hint_; | |
| for (;;) { | |
| const size_t search_limit = static_cast<size_t>(layer_count_) * SLOTS_PER_LAYER; | |
| for (size_t linear_index = search_hint; linear_index < search_limit; ++linear_index) { | |
| size_t& search_hint = single_slot ? small_search_hint_ : large_search_hint_; | |
| for (int pass = 0; pass < 2; ++pass) { | |
| const size_t search_limit = static_cast<size_t>(layer_count_) * SLOTS_PER_LAYER; | |
| for (size_t linear_index = search_hint; linear_index < search_limit; ++linear_index) { |
Introduced support for importing monster and NPC definitions from Lua scripts and upgraded the Zone palette with a color-coded data view and jump-to functionality. Added spawn density and weight controls for monster placement and improved OTBM serialization resilience. - `data/clients.toml` — updated sample client to use protobuf and updated supported OTBM versions - `source/app/client_version.cpp` — added persistence and cloning for monster/NPC Lua paths - `source/app/client_version.h` — added getters and setters for creature Lua paths - `source/app/managers/version_manager.cpp` — implemented automated Lua creature directory resolution and loading - `source/app/preferences/client_version_page.cpp` — added monster and NPC Lua path properties to client editor - `source/app/settings.cpp` — initialized default values for monster spawn density and weight - `source/app/settings.h` — added `SPAWN_MONSTER_DENSITY` and `MONSTER_DEFAULT_WEIGHT` config keys - `source/assets/png/avoidable.png` — added new icon asset - `source/assets/png/house_exit.png` — added new icon asset - `source/assets/png/mini_borderize.png` — added new icon asset - `source/assets/png/mini_change.png` — added new icon asset - `source/assets/png/mini_copy.png` — added new icon asset - `source/assets/png/mini_cut.png` — added new icon asset - `source/assets/png/mini_delete.png` — added new icon asset - `source/assets/png/mini_draw.png` — added new icon asset - `source/assets/png/mini_erase.png` — added new icon asset - `source/assets/png/mini_fill.png` — added new icon asset - `source/assets/png/mini_move.png` — added new icon asset - `source/assets/png/mini_paste.png` — added new icon asset - `source/assets/png/mini_randomize.png` — added new icon asset - `source/assets/png/mini_remote.png` — added new icon asset - `source/assets/png/mini_replace.png` — added new icon asset - `source/assets/png/mini_rotate.png` — added new icon asset - `source/assets/png/mini_select.png` — added new icon asset - `source/assets/png/mini_switch.png` — added new icon asset - `source/assets/png/mini_unselect.png` — added new icon asset - `source/assets/png/monsters.png` — added new icon asset - `source/assets/png/moveable.png` — added new icon asset - `source/assets/png/nologout_zone.png` — added new icon asset - `source/assets/png/nopvp_zone.png` — added new icon asset - `source/assets/png/npcs.png` — added new icon asset - `source/assets/png/pickupable.png` — added new icon asset - `source/assets/png/pickupable_moveable.png` — added new icon asset - `source/assets/png/position_go.png` — added new icon asset - `source/assets/png/protected_zone.png` — added new icon asset - `source/assets/png/toolbar_hooks.png` — added new icon asset - `source/assets/png/toolbar_moveables.png` — added new icon asset - `source/assets/png/toolbar_pickupables.png` — added new icon asset - `source/assets/png/zone_brush.png` — added new icon asset - `source/assets/png/zone_brush_small.png` — added new icon asset - `source/brushes/creature/creature_brush.cpp` — applied default spawn weight when drawing monsters - `source/brushes/spawn/spawn_brush.cpp` — implemented automated monster population within spawn area based on density - `source/clients.toml` — updated supported OTBM versions for client 13.20 - `source/game/creatures.cpp` — implemented Lua parser for monster/NPC definitions and directory importing - `source/game/creatures.h` — declared new Lua creature import methods - `source/io/filehandle.h` — added `setReadOffset` to support random access in binary nodes - `source/io/iomap_otbm.cpp` — ensured valid NPC spawn file path during save - `source/io/otbm/tile_serialization_otbm.cpp` — added recovery for unknown tile attributes and fixed inline item attribute reading - `source/item_definitions/formats/protobuf/protobuf_item_parser.cpp` — applied draw offsets from appearance flags - `source/palette/palette_zone.cpp` — refactored zone list to `wxDataViewListCtrl` with colors and jump-to functionality - `source/palette/palette_zone.h` — updated UI members and event handlers for the new zone list - `source/rendering/core/sprite_archive.cpp` — implemented LRU pruning for the decoded protobuf sheet cache - `source/rendering/core/sprite_archive.h` — added tracking fields for sheet cache management - `source/rendering/drawers/tiles/tile_color_calculator.cpp` — improved zone tinting with averaged colors and alpha blending - `source/rendering/drawers/tiles/tile_color_calculator.h` — exposed `GetZoneColor` method - `source/ui/map/map_properties_window.cpp` — improved client version selection logic to handle missing matches - `source/ui/map_popup_menu.cpp` — updated zone brush icon to use a PNG asset - `source/ui/tool_options_surface.cpp` — added controls for monster spawn density and weight - `source/ui/tool_options_surface.h` — updated UI members to support new spawn settings
|
🤖 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. |
|
🤖 I'm sorry @karolak6612, but I was unable to process your request. Please see the logs for more details. |
2 participants
Integrated Protobuf-based client asset loading, introduced a dedicated NPC spawn system, and implemented map zones with persistent XML storage.
Changes:
CMakeLists.txt— added Protobuf and LibLZMA dependencies and proto generationsource/app/client_asset_detector.cpp— implemented protobuf asset detection logicsource/brushes/spawn/npc_spawn_brush.cpp— implemented NPC-specific spawn brush [new]source/brushes/zone/zone_brush.cpp— implemented map zone painting brush [new]source/io/map_xml_io.cpp— implemented XML serialization for NPC spawns and zonessource/io/otbm/header_serialization_otbm.cpp— added OTBM 5/6 version support and new file tagssource/io/xml_file_loader.cpp— added generic XML inclusion and visiting utility [new]source/item_definitions/formats/protobuf/protobuf_item_parser.cpp— implemented Protobuf appearance parser [new]source/map/map.cpp— addedZoneRegistryand NPC spawn/zone management to Mapsource/rendering/core/sprite_archive.cpp— implemented Protobuf sheet decoding and LZMA decompressionsource/rendering/core/texture_atlas.cpp— added support for variable-sized sprite packingsource/ui/tile_properties/zone_property_panel.cpp— implemented zone management UI [new]vcpkg.json— addedprotobufandliblzmadependencies [minor]Summary by CodeRabbit
New Features
Improvements