feat(data): load XML data palletes, tilesets, brushes, etc.#32
Conversation
Refactored client data loading to support version-specific directories and added pulse animations to the UI. Removed obsolete sample data files. - `ImguiMapEditor/Brushes/BrushRegistry.h` — added `getBorderTemplateCount()` - `ImguiMapEditor/CMakeLists.txt` — updated copy command to use `../data` instead of `sample_data` - `ImguiMapEditor/Domain/ClientVersion.cpp` — updated `getItemMetadataPath()` to support versioned data directories - `ImguiMapEditor/Domain/ClientVersion.h` — added data directory normalization and auto-detection - `ImguiMapEditor/Services/ClientVersionRegistry.cpp` — added automatic data directory detection during registry loading - `ImguiMapEditor/Services/Map/MapLoadingService.cpp` — updated resource loading to use version-specific paths - `ImguiMapEditor/Services/Map/MapLoadingService.h` — updated `tryLoadCreatures` and `tryLoadItems` signatures - `ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp` — implemented `renderBrushCard` and added pulse animation for selected items - `ImguiMapEditor/UI/Widgets/TilesetGridWidget.h` — added `renderBrushCard` method declaration - `ImguiMapEditor/sample_data/background.jpg` — [deleted] - `ImguiMapEditor/sample_data/borders/borders.xml` — [deleted] - `ImguiMapEditor/sample_data/brushes/brushes.xml` — [deleted] - `ImguiMapEditor/sample_data/creatures/creatures.xml` — [deleted] - `ImguiMapEditor/sample_data/items/items.xml` — [deleted] - `ImguiMapEditor/sample_data/materials.xml` — [deleted] - `ImguiMapEditor/sample_data/palettes.xml` — [deleted]
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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 introduces a more flexible and robust system for loading XML-based game data, such as palettes, tilesets, and brushes. By shifting from a hardcoded global data path to a version-aware approach, the editor can now correctly resolve resources specific to different client versions. Additionally, the UI has been polished to provide better visual cues when interacting with brushes, and redundant sample data files have been removed to streamline the repository. 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 the 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 counterproductive. 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
|
Reviewer's GuideRefactors tileset grid brush rendering into a reusable card renderer with improved visuals and pulse handling, introduces version-specific data directory logic for loading XML resources (materials, tilesets, palettes, creatures, items) and item metadata, normalizes client version data directories including auto-detection, wires new callbacks in MainWindow, and updates build/runtime data copying while cleaning up minor APIs and includes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
Type: Enhancement
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The logic for resolving
data_dir(handlingdata/prefixes, buildingcurrent_path()/data/..., existence checks) is now duplicated acrossClientVersion::setDataDirectory,getItemMetadataPath, andMapLoadingService::loadClientData; consider extracting a shared helper to centralize this behavior and avoid future divergence. - In
MapLoadingService::loadClientData, if adata_diris configured but the corresponding directory does not exist, the code now skips materials/tilesets/palettes loading entirely; if that is not intentional, you may want to retain a fallback to the previous default (current_path()/data) to avoid regressions when the configuration is wrong or missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for resolving `data_dir` (handling `data/` prefixes, building `current_path()/data/...`, existence checks) is now duplicated across `ClientVersion::setDataDirectory`, `getItemMetadataPath`, and `MapLoadingService::loadClientData`; consider extracting a shared helper to centralize this behavior and avoid future divergence.
- In `MapLoadingService::loadClientData`, if a `data_dir` is configured but the corresponding directory does not exist, the code now skips materials/tilesets/palettes loading entirely; if that is not intentional, you may want to retain a fallback to the previous default (`current_path()/data`) to avoid regressions when the configuration is wrong or missing.
## Individual Comments
### Comment 1
<location path="ImguiMapEditor/Services/Map/MapLoadingService.cpp" line_range="571-580" />
<code_context>
- if (!tryLoadCreatures(map_dir, client_path)) {
+ // Compute version data path for creatures.xml and items.xml lookup
+ std::string data_dir_for_resources = version_info->getDataDirectory();
+ std::filesystem::path version_data_path;
+ if (!data_dir_for_resources.empty()) {
+ auto candidate = std::filesystem::current_path() / "data" / data_dir_for_resources;
+ if (std::filesystem::exists(candidate)) {
+ version_data_path = candidate;
+ }
+ }
+
+ if (!tryLoadCreatures(map_dir, client_path, version_data_path)) {
spdlog::warn("No creature data loaded. Spawns may look incorrect.");
}
- if (!tryLoadItems(map_dir, client_path)) {
+ if (!tryLoadItems(map_dir, client_path, version_data_path)) {
spdlog::warn("No items.xml loaded. Item names may be missing.");
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Data directory handling for creatures/items is inconsistent with tileset loading and may fail when `dataDirectory` contains a `data/` prefix.
Here `version_data_path` is built as `current_path() / "data" / data_dir_for_resources` without the normalization you use later for `app_data_path` (stripping a leading `data/`). If `dataDirectory` is `"data/1098"`, creatures/items are looked up under `.../data/data/1098` while tilesets/palettes use `.../data/1098`, so creature/item loading can silently fail for older configs or user-provided values. Please apply the same normalization logic here (or via a shared helper) so both paths are resolved consistently.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR introduces a significant improvement in how client data (palettes, tilesets, brushes, etc.) are managed by moving them from sample_data to a more structured data directory and adding auto-detection for version-specific data.
🔍 General Feedback
The transition to a more flexible data loading system is a great step. However, there are some concerns regarding the reliability of path resolution using current_path() and a potential bug in the fallback logic when a specific client's data directory is missing but a generic data/ directory exists.
Key Points:
- 🔴 Path Resolution: Reliance on
std::filesystem::current_path()to find application assets can lead to issues if the working directory is not the application root. - 🔴 Fallback Bug: In
MapLoadingService::loadClientData, an empty metadata path can cause the fallback logic to incorrectly identify thedata/directory as a valid metadata file. - 🟡 Path Normalization: The normalization of
data_directory_is somewhat fragile and could be more robust. - 🟢 Decoupling: The addition of callbacks in
MainWindowfor map operations is a positive change for decoupling the UI from the application logic.
![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 refactors client version and map loading services to support version-specific data directories for items, creatures, tilesets, and palettes. It also introduces automatic data directory detection and refactors the TilesetGridWidget to use a card-based rendering system with fallback text labels and pulse animations. The review feedback highlights several key improvement opportunities, including resolving path resolution inconsistencies in MapLoadingService, optimizing repeated ImGui::CalcTextSize calls in TilesetGridWidget to prevent performance bottlenecks, and ensuring the pulseBrush_ pointer is safely cleared when pulse animations expire to avoid stale pointer bugs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Compute version data path for creatures.xml and items.xml lookup | ||
| std::string data_dir_for_resources = version_info->getDataDirectory(); | ||
| std::filesystem::path version_data_path; | ||
| if (!data_dir_for_resources.empty()) { | ||
| auto candidate = std::filesystem::current_path() / "data" / data_dir_for_resources; | ||
| if (std::filesystem::exists(candidate)) { | ||
| version_data_path = candidate; | ||
| } | ||
| } |
There was a problem hiding this comment.
The path resolution logic for version_data_path here does not handle the data/ prefix stripping fallback, unlike the logic used for app_data_path later in the function. If the configured data directory contains a data/ prefix, creatures.xml and items.xml will fail to load from the version data folder, while tilesets and palettes will succeed.
We should resolve this path robustly once at the beginning of the function and reuse it to eliminate duplicate code and prevent this bug.
| // Compute version data path for creatures.xml and items.xml lookup | |
| std::string data_dir_for_resources = version_info->getDataDirectory(); | |
| std::filesystem::path version_data_path; | |
| if (!data_dir_for_resources.empty()) { | |
| auto candidate = std::filesystem::current_path() / "data" / data_dir_for_resources; | |
| if (std::filesystem::exists(candidate)) { | |
| version_data_path = candidate; | |
| } | |
| } | |
| // Compute version data path for creatures.xml and items.xml lookup | |
| std::string data_dir = version_info->getDataDirectory(); | |
| std::filesystem::path version_data_path; | |
| if (!data_dir.empty()) { | |
| auto candidate = std::filesystem::current_path() / "data" / data_dir; | |
| if (std::filesystem::exists(candidate)) { | |
| version_data_path = candidate; | |
| } else { | |
| // Try stripping "data/" prefix if present | |
| std::filesystem::path dir_path(data_dir); | |
| if (dir_path.has_parent_path() && dir_path.parent_path().filename() == "data") { | |
| auto stripped = std::filesystem::current_path() / "data" / dir_path.filename(); | |
| if (std::filesystem::exists(stripped)) { | |
| version_data_path = stripped; | |
| spdlog::warn("Data directory '{}' had 'data/' prefix, stripped to '{}'", | |
| data_dir, dir_path.filename().string()); | |
| } | |
| } | |
| } | |
| } |
| if (textSize.x > MAX_TEXT_WIDTH) { | ||
| while (label.size() > 1 && ImGui::CalcTextSize((label + "...").c_str()).x > MAX_TEXT_WIDTH) { | ||
| label.pop_back(); | ||
| } | ||
| label += "..."; | ||
| textSize = ImGui::CalcTextSize(label.c_str()); | ||
| } |
There was a problem hiding this comment.
Calling ImGui::CalcTextSize repeatedly in a linear loop inside the render function (which runs every frame) is highly inefficient and can cause a severe performance bottleneck when rendering many cards with fallback labels.
We can optimize this by estimating the truncation length first based on the average character width, which reduces the number of layout calculations to almost always 1 or 2 iterations.
if (textSize.x > MAX_TEXT_WIDTH && !label.empty()) {
float avgCharWidth = textSize.x / label.size();
size_t estimatedLength = static_cast<size_t>(MAX_TEXT_WIDTH / avgCharWidth);
if (estimatedLength > 3) {
label = label.substr(0, estimatedLength - 3);
} else {
label = label.substr(0, 1);
}
while (label.size() > 1 && ImGui::CalcTextSize((label + "...").c_str()).x > MAX_TEXT_WIDTH) {
label.pop_back();
}
label += "...";
textSize = ImGui::CalcTextSize(label.c_str());
}| // Use version-specific data directory for tilesets, palettes, brushes, etc. | ||
| // dataDirectory should be just the folder name (e.g., "1098"), not a path. | ||
| // Handle both "1098" and "data/1098" gracefully. | ||
| std::string data_dir = version_info->getDataDirectory(); | ||
| std::filesystem::path app_data_path; | ||
|
|
||
| bool tilesets_loaded = tileset_service_.loadMaterials(app_data_path); | ||
| if (!tilesets_loaded) { | ||
| spdlog::warn("materials.xml could not be loaded. Falling back to direct tileset/palette loading."); | ||
| tilesets_loaded = tileset_service_.loadTilesets(app_data_path); | ||
| if (!tilesets_loaded) { | ||
| spdlog::warn("No tilesets found. The palette will be empty."); | ||
| if (!data_dir.empty()) { | ||
| auto candidate = std::filesystem::current_path() / "data" / data_dir; | ||
| if (std::filesystem::exists(candidate)) { | ||
| app_data_path = candidate; | ||
| } else { | ||
| // Maybe data_dir already contains "data/" prefix — try stripping it | ||
| std::filesystem::path dir_path(data_dir); | ||
| if (dir_path.has_parent_path() && dir_path.parent_path().filename() == "data") { | ||
| auto stripped = std::filesystem::current_path() / "data" / dir_path.filename(); | ||
| if (std::filesystem::exists(stripped)) { | ||
| app_data_path = stripped; | ||
| spdlog::warn("Data directory '{}' had 'data/' prefix, stripped to '{}'", | ||
| data_dir, dir_path.filename().string()); | ||
| } | ||
| } | ||
| } | ||
| if (!tileset_service_.loadPalettes(app_data_path)) { | ||
| spdlog::warn("No palettes loaded. Ribbon palette buttons will be empty."); | ||
| } |
There was a problem hiding this comment.
Now that version_data_path is resolved robustly at the top of the function, we can completely eliminate this duplicate path resolution block and simply reuse version_data_path as app_data_path.
| // Use version-specific data directory for tilesets, palettes, brushes, etc. | |
| // dataDirectory should be just the folder name (e.g., "1098"), not a path. | |
| // Handle both "1098" and "data/1098" gracefully. | |
| std::string data_dir = version_info->getDataDirectory(); | |
| std::filesystem::path app_data_path; | |
| bool tilesets_loaded = tileset_service_.loadMaterials(app_data_path); | |
| if (!tilesets_loaded) { | |
| spdlog::warn("materials.xml could not be loaded. Falling back to direct tileset/palette loading."); | |
| tilesets_loaded = tileset_service_.loadTilesets(app_data_path); | |
| if (!tilesets_loaded) { | |
| spdlog::warn("No tilesets found. The palette will be empty."); | |
| if (!data_dir.empty()) { | |
| auto candidate = std::filesystem::current_path() / "data" / data_dir; | |
| if (std::filesystem::exists(candidate)) { | |
| app_data_path = candidate; | |
| } else { | |
| // Maybe data_dir already contains "data/" prefix — try stripping it | |
| std::filesystem::path dir_path(data_dir); | |
| if (dir_path.has_parent_path() && dir_path.parent_path().filename() == "data") { | |
| auto stripped = std::filesystem::current_path() / "data" / dir_path.filename(); | |
| if (std::filesystem::exists(stripped)) { | |
| app_data_path = stripped; | |
| spdlog::warn("Data directory '{}' had 'data/' prefix, stripped to '{}'", | |
| data_dir, dir_path.filename().string()); | |
| } | |
| } | |
| } | |
| if (!tileset_service_.loadPalettes(app_data_path)) { | |
| spdlog::warn("No palettes loaded. Ribbon palette buttons will be empty."); | |
| } | |
| // Use version-specific data directory for tilesets, palettes, brushes, etc. | |
| const std::filesystem::path& app_data_path = version_data_path; |
| if (pulseElapsed >= PULSE_DURATION) { | ||
| pulseBrushName_.clear(); | ||
| pulseStartTime_ = -1.0f; | ||
| isPulsing = false; | ||
| } |
There was a problem hiding this comment.
When the pulse duration expires, pulseBrush_ is not cleared (unlike in the original implementation). This leaves a dangling/stale pointer to the brush in the member variable, which can cause state bugs or crashes if the brush is reloaded or destroyed.
if (pulseElapsed >= PULSE_DURATION) {
pulseBrush_ = nullptr;
pulseBrushName_.clear();
pulseStartTime_ = -1.0f;
isPulsing = false;
}| if (pulseElapsed >= PULSE_DURATION) { | ||
| pulseBrushName_.clear(); | ||
| pulseStartTime_ = -1.0f; | ||
| isPulsing = false; | ||
| } |
There was a problem hiding this comment.
When the pulse duration expires, pulseBrush_ is not cleared here either. We should reset it to nullptr to avoid holding a stale pointer.
if (pulseElapsed >= PULSE_DURATION) {
pulseBrush_ = nullptr;
pulseBrushName_.clear();
pulseStartTime_ = -1.0f;
isPulsing = false;
}
There was a problem hiding this comment.
📋 Review Summary (Continued)
Adding missing comments from the previous review session.
Key Points:
- 🔴 Bug in MapLoadingService: Fixed a potential issue where a directory could be loaded as a metadata file.
- 🔴 Architecture: Further emphasized the need to avoid
current_path(). - 🟡 Normalization: Suggested improvements for data directory normalization.
There was a problem hiding this comment.
📋 Review Summary (Final)
I have reviewed the changes in PR #32. Here is a consolidated list of the remaining points that could not be added as inline comments:
🔴 Critical Issues
-
Fallback Bug in
MapLoadingService::loadClientData(Line 490):
IfgetItemMetadataPath()returns an empty path (e.g., when the specific data directory is missing), the fallback logic uses thedata/directory itself as the file path. Since the directory exists, it passes validation but will fail during loading.
Suggestion: Fall back to a default filename likeitems.otboritems.srvbefore constructing the fallback path. -
Unreliable Path Resolution:
The extensive use ofstd::filesystem::current_path()to locate application assets (inClientVersion.cpp,ClientVersion.h, etc.) is fragile. If the application is launched from a different working directory, these paths will fail.
Suggestion: Use paths relative to the executable or a centralized path service.
🟡 Minor Issues
- Fragile Normalization in
setDataDirectory(ClientVersion.h):
The current logic only handles a single level of the "data/" prefix and is case-sensitive. - Redundant Validation in
ClientVersion::validateFiles:
The fallback check invalidateFilesignores thedata_directory_configuration, which is inconsistent with how the primary path is resolved.
🟢 Positive Changes
- UI Decoupling: The use of callbacks for map operations in
MainWindowis a good architectural improvement. - Auto-detection: The added logic for auto-detecting data directories based on client version simplifies configuration for the user.
- Namespace fixes: Fixed potential namespace issues in
MapLoadingService.cpp. - Cleanup: Fixed a premature namespace closure in
Tile.cpp.
Overall, the PR is a great improvement but needs a few fixes for robustness.
github-actions
Bot
added
Type: Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImguiMapEditor/CMakeLists.txt`:
- Around line 539-542: The CMake step that runs COMMAND ${CMAKE_COMMAND} -E
copy_directory from "${CMAKE_CURRENT_SOURCE_DIR}/../data" to
"$<TARGET_FILE_DIR:${PROJECT_NAME}>/data" assumes a repo-level data/ dir that
may be missing; update ImguiMapEditor/CMakeLists to guard this by checking if
the source exists (using EXISTS on "${CMAKE_CURRENT_SOURCE_DIR}/../data") and
either emit a clear message(FATAL_ERROR "...data directory not found...") or run
a fallback target that creates/downloads/populates the data directory before
attempting the copy; reference the existing COMMAND, CMAKE_CURRENT_SOURCE_DIR,
and PROJECT_NAME symbols so the guard is colocated with the copy_directory
invocation.
In `@ImguiMapEditor/Domain/ClientVersion.cpp`:
- Around line 48-49: The current unresolved-lookup return of {} drops the
metadata filename needed by downstream fallback, so instead of returning an
empty path in the failure branch, return a path constructed from
metadata_path.filename() (e.g., std::filesystem::path{metadata_path.filename()}
or equivalent) so client_path_ filename context is preserved; update the return
site in ClientVersion.cpp (the branch that currently does "return {}") to return
that filename-only path while leaving parent components empty so downstream code
can build ".../data/<filename>" properly.
In `@ImguiMapEditor/Domain/ClientVersion.h`:
- Around line 127-132: autoDetectDataDirectory currently uses
std::filesystem::current_path() which is unreliable; change it to resolve a
stable base path (either accept a base_path parameter or compute the executable
directory) before appending "data"/std::to_string(version_). Specifically update
the autoDetectDataDirectory function (and callers) to take a
std::filesystem::path base_path or use a platform-safe method to get the
executable path (e.g., read the running binary path via platform APIs
/proc/self/exe on Linux or GetModuleFileName on Windows) then test
std::filesystem::exists(base_path / "data" / std::to_string(version_)) and set
data_directory_ accordingly instead of using current_path(); reference the
symbols autoDetectDataDirectory, data_directory_, and version_ when making the
change.
- Around line 116-124: Update the inline comment for setDataDirectory in
ClientVersion.h to accurately describe its normalization: it reduces paths of
the form .../data/<version> (including absolute paths) to just the final segment
(assigning data_directory_), not only stripping a literal leading "data/"
prefix; mention that autoDetectDataDirectory() uses
std::filesystem::current_path() so detection and MapLoadingService loading
depend on the process working directory. Reference setDataDirectory,
autoDetectDataDirectory, data_directory_, and MapLoadingService in the comment
to make the behavior and dependency clear.
In `@ImguiMapEditor/Services/Map/MapLoadingService.cpp`:
- Around line 570-578: The lookup for creatures.xml/items.xml uses
data_dir_for_resources directly so it fails when getDataDirectory() returns a
path already prefixed with "data/" (or "data\\"); normalize that value before
building version_data_path. In the block that computes version_data_path,
canonicalize or strip a leading "data" component from data_dir_for_resources
(e.g., remove a leading "data/" or "data\\", or parse it as a
std::filesystem::path and use its filename or relative tail) so the candidate
path resolution (current_path() / "data" / data_dir_for_resources) matches the
same normalization used by the tileset/material resolution; update the code that
references data_dir_for_resources and version_data_path accordingly (symbols:
getDataDirectory, data_dir_for_resources, version_data_path).
In `@ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp`:
- Around line 386-399: Extract the duplicated pulse animation logic into a
private helper on TilesetGridWidget: add a declaration std::pair<bool,float>
computePulseState(const Brushes::IBrush* brush); to the class header, implement
computePulseState to use pulseBrushName_, pulseStartTime_, PULSE_DURATION and
ImGui::GetTime() to compute and return {isPulsing, pulseElapsed} while resetting
pulseBrushName_ and pulseStartTime_ when the duration elapses, and replace the
two identical blocks (the ones using isPulsing, pulseElapsed and
brush->getName() == pulseBrushName_) with calls to computePulseState(brush) to
unpack the returned pair.
- Around line 16-18: Remove the anonymous-namespace PULSE_DURATION constant and
replace its uses with the class-scoped constant
TilesetGridWidget::PULSE_DURATION; locate any references in
TilesetGridWidget.cpp that currently resolve to the local anonymous constant
(mentions in the review correspond to usages around the pulse/timing logic) and
change them to use TilesetGridWidget::PULSE_DURATION (or bare PULSE_DURATION if
inside TilesetGridWidget methods), ensuring only the single static constexpr
float TilesetGridWidget::PULSE_DURATION (the declaration in the header) remains.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0a11d05-a22f-40ba-b27b-a277e0cc0c8f
⛔ Files ignored due to path filters (1)
ImguiMapEditor/sample_data/background.jpgis excluded by!**/*.jpg
📒 Files selected for processing (20)
ImguiMapEditor/Brushes/BrushRegistry.hImguiMapEditor/CMakeLists.txtImguiMapEditor/Domain/ClientVersion.cppImguiMapEditor/Domain/ClientVersion.hImguiMapEditor/Domain/Tile.cppImguiMapEditor/Presentation/MainWindow.hImguiMapEditor/Services/ClientVersionRegistry.cppImguiMapEditor/Services/Map/MapLoadingService.cppImguiMapEditor/Services/Map/MapLoadingService.hImguiMapEditor/UI/Dialogs/EditTownsDialog.cppImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cppImguiMapEditor/UI/PreferencesDialog.cppImguiMapEditor/UI/Widgets/TilesetGridWidget.cppImguiMapEditor/UI/Widgets/TilesetGridWidget.hImguiMapEditor/sample_data/borders/borders.xmlImguiMapEditor/sample_data/brushes/brushes.xmlImguiMapEditor/sample_data/creatures/creatures.xmlImguiMapEditor/sample_data/items/items.xmlImguiMapEditor/sample_data/materials.xmlImguiMapEditor/sample_data/palettes.xml
💤 Files with no reviewable changes (6)
- ImguiMapEditor/sample_data/materials.xml
- ImguiMapEditor/sample_data/palettes.xml
- ImguiMapEditor/sample_data/borders/borders.xml
- ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cpp
- ImguiMapEditor/Domain/Tile.cpp
- ImguiMapEditor/sample_data/creatures/creatures.xml
| # Copy entire data directory tree (includes version folders, tilesets/, creatures/, items/, etc.) | ||
| COMMAND ${CMAKE_COMMAND} -E copy_directory | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/sample_data" | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/../data" | ||
| "$<TARGET_FILE_DIR:${PROJECT_NAME}>/data" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify ../data exists relative to ImguiMapEditor/ and contains version subdirectories
if [ -d "data" ]; then
echo "✓ data/ directory exists"
echo "Contents:"
fd -d 2 -t d . data/
else
echo "✗ data/ directory not found relative to repository root"
exit 1
fiRepository: Open-Tibia-Tools/tibia-imgui-map-editor
Length of output: 139
Guard the CMake copy_directory source or ensure data/ is present
- The path
${CMAKE_CURRENT_SOURCE_DIR}/../data(used inImguiMapEditor/CMakeLists.txt:539-542) resolves to${repo_root}/data, but this checkout has nodata/directory. - Add an
EXISTScheck (and a clearFATAL_ERRORif missing) or make the build generate/download the requireddataresources.
# Copy entire data directory tree (includes version folders, tilesets/, creatures/, items/, etc.)
COMMAND ${CMAKE_COMMAND} -E copy_directory
"${CMAKE_CURRENT_SOURCE_DIR}/../data"
"$<TARGET_FILE_DIR:${PROJECT_NAME}>/data"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/CMakeLists.txt` around lines 539 - 542, The CMake step that
runs COMMAND ${CMAKE_COMMAND} -E copy_directory from
"${CMAKE_CURRENT_SOURCE_DIR}/../data" to
"$<TARGET_FILE_DIR:${PROJECT_NAME}>/data" assumes a repo-level data/ dir that
may be missing; update ImguiMapEditor/CMakeLists to guard this by checking if
the source exists (using EXISTS on "${CMAKE_CURRENT_SOURCE_DIR}/../data") and
either emit a clear message(FATAL_ERROR "...data directory not found...") or run
a fallback target that creates/downloads/populates the data directory before
attempting the copy; reference the existing COMMAND, CMAKE_CURRENT_SOURCE_DIR,
and PROJECT_NAME symbols so the guard is colocated with the copy_directory
invocation.
| // 3. No fallback — client_path_ is not used for items.otb | ||
| return {}; |
There was a problem hiding this comment.
Preserve metadata filename on unresolved lookup.
Returning an empty path here drops filename context needed by downstream fallback code (e.g., checks that build .../data/<filename> from metadata_path.filename()). This can mask missing metadata and weaken validation behavior.
Suggested fix
- // 3. No fallback — client_path_ is not used for items.otb
- return {};
+ // 3. Preserve filename fallback for downstream resolution (./data/<filename>)
+ switch (data_source_) {
+ case ItemDataSource::SRV:
+ return std::filesystem::path("items.srv");
+ case ItemDataSource::OTB:
+ return std::filesystem::path("items.otb");
+ case ItemDataSource::DAT:
+ default:
+ return {};
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/Domain/ClientVersion.cpp` around lines 48 - 49, The current
unresolved-lookup return of {} drops the metadata filename needed by downstream
fallback, so instead of returning an empty path in the failure branch, return a
path constructed from metadata_path.filename() (e.g.,
std::filesystem::path{metadata_path.filename()} or equivalent) so client_path_
filename context is preserved; update the return site in ClientVersion.cpp (the
branch that currently does "return {}") to return that filename-only path while
leaving parent components empty so downstream code can build
".../data/<filename>" properly.
| void setDataDirectory(const std::string &dir) { | ||
| // Normalize: strip "data/" or "data\\" prefix if user typed it | ||
| std::filesystem::path p(dir); | ||
| if (p.has_parent_path() && p.parent_path().filename() == "data") { | ||
| data_directory_ = p.filename().string(); | ||
| } else { | ||
| data_directory_ = dir; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how setDataDirectory is called to understand expected input patterns
rg -nP 'setDataDirectory\(' -A2 -B2Repository: Open-Tibia-Tools/tibia-imgui-map-editor
Length of output: 4373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant implementation area in ClientVersion.h
echo "---- ClientVersion.h (around setDataDirectory) ----"
sed -n '90,170p' ImguiMapEditor/Domain/ClientVersion.h
# Find all mentions of autoDetectDataDirectory
echo "---- autoDetectDataDirectory references ----"
rg -n "autoDetectDataDirectory" ImguiMapEditor/Domain/ClientVersion.h ImguiMapEditor -S --hidden
# Show where getDataDirectory / dataDirectory property is used (brief)
echo "---- getDataDirectory usages ----"
rg -n "getDataDirectory\(" ImguiMapEditor -S --hidden
echo "---- dataDirectory property persistence ----"
sed -n '1,220p' ImguiMapEditor/Services/ClientVersionPersistence.cppRepository: Open-Tibia-Tools/tibia-imgui-map-editor
Length of output: 12395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the UI/editor code that feeds into setDataDirectory(...)
echo "---- ClientPropertyEditor.cpp (setDataDirectory call sites) ----"
sed -n '200,520p' ImguiMapEditor/UI/Dialogs/ClientConfiguration/ClientPropertyEditor.cpp
# Find where eb.dataDir is declared/filled (best-effort search)
echo "---- Search for 'dataDir' in ClientPropertyEditor.cpp ----"
rg -n "dataDir" ImguiMapEditor/UI/Dialogs/ClientConfiguration/ClientPropertyEditor.cpp
# Inspect UI controller where data_dir_buf_ is set from cv.getDataDirectory()
echo "---- ClientConfigurationController.cpp (data_dir_buf_ usage) ----"
sed -n '300,430p' ImguiMapEditor/Presentation/Dialogs/ClientConfigurationController.cpp
# Inspect StartupController client_info.data_directory usage
echo "---- StartupController.cpp (client_info.data_directory) ----"
sed -n '260,470p' ImguiMapEditor/Controllers/StartupController.cpp
# Inspect MapLoadingService usage to understand expected format
echo "---- MapLoadingService.cpp (data_dir_for_resources/data_dir) ----"
sed -n '520,680p' ImguiMapEditor/Services/Map/MapLoadingService.cppRepository: Open-Tibia-Tools/tibia-imgui-map-editor
Length of output: 37469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' ImguiMapEditor/Services/ClientVersionRegistry.cppRepository: Open-Tibia-Tools/tibia-imgui-map-editor
Length of output: 3729
Fix setDataDirectory comment to match its actual normalization behavior
ImguiMapEditor/Domain/ClientVersion.h (setDataDirectory, ~116-124) strips to the last path segment whenever the immediate parent directory is literally named data—so it’s not “prefix-only” (it also handles absolute selections like .../data/<version>), which aligns with how the UI/help text feeds the value and how MapLoadingService expects dataDirectory (folder name like "<version>"). Update the inline comment to describe this .../data/<version> behavior; the proposed “prefix-only” restriction would break valid absolute-folder inputs.
Also, autoDetectDataDirectory() depends on std::filesystem::current_path() for locating data/<version>, so detection (and similarly MapLoadingService loading) will be sensitive to the app’s working directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/Domain/ClientVersion.h` around lines 116 - 124, Update the
inline comment for setDataDirectory in ClientVersion.h to accurately describe
its normalization: it reduces paths of the form .../data/<version> (including
absolute paths) to just the final segment (assigning data_directory_), not only
stripping a literal leading "data/" prefix; mention that
autoDetectDataDirectory() uses std::filesystem::current_path() so detection and
MapLoadingService loading depend on the process working directory. Reference
setDataDirectory, autoDetectDataDirectory, data_directory_, and
MapLoadingService in the comment to make the behavior and dependency clear.
| void autoDetectDataDirectory() { | ||
| auto candidate = std::filesystem::current_path() / "data" / std::to_string(version_); | ||
| if (std::filesystem::exists(candidate)) { | ||
| data_directory_ = std::to_string(version_); | ||
| } | ||
| } |
There was a problem hiding this comment.
std::filesystem::current_path() is unreliable for directory detection.
current_path() depends on the process's working directory at launch, which varies by how the executable is invoked (CLI, double-click, debugger, etc.). If launched from a different directory, auto-detection will fail silently or detect the wrong path.
🔧 Use executable-relative or application base path instead
void autoDetectDataDirectory() {
- auto candidate = std::filesystem::current_path() / "data" / std::to_string(version_);
+ // Assuming a getApplicationBasePath() or similar exists that returns executable directory
+ auto candidate = getApplicationBasePath() / "data" / std::to_string(version_);
if (std::filesystem::exists(candidate)) {
data_directory_ = std::to_string(version_);
}
}Alternatively, if the application already stores a base path elsewhere (e.g., in a configuration service), pass it as a parameter rather than relying on current_path().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/Domain/ClientVersion.h` around lines 127 - 132,
autoDetectDataDirectory currently uses std::filesystem::current_path() which is
unreliable; change it to resolve a stable base path (either accept a base_path
parameter or compute the executable directory) before appending
"data"/std::to_string(version_). Specifically update the autoDetectDataDirectory
function (and callers) to take a std::filesystem::path base_path or use a
platform-safe method to get the executable path (e.g., read the running binary
path via platform APIs /proc/self/exe on Linux or GetModuleFileName on Windows)
then test std::filesystem::exists(base_path / "data" / std::to_string(version_))
and set data_directory_ accordingly instead of using current_path(); reference
the symbols autoDetectDataDirectory, data_directory_, and version_ when making
the change.
| // Compute version data path for creatures.xml and items.xml lookup | ||
| std::string data_dir_for_resources = version_info->getDataDirectory(); | ||
| std::filesystem::path version_data_path; | ||
| if (!data_dir_for_resources.empty()) { | ||
| auto candidate = std::filesystem::current_path() / "data" / data_dir_for_resources; | ||
| if (std::filesystem::exists(candidate)) { | ||
| version_data_path = candidate; | ||
| } | ||
| } |
There was a problem hiding this comment.
Normalize data_dir_for_resources consistently before resolving version resource files.
This lookup path doesn’t handle "data/<version>"-prefixed directory values, while the later tileset/material resolution does. That inconsistency can skip versioned creatures.xml/items.xml even when the directory is valid.
Suggested fix
std::string data_dir_for_resources = version_info->getDataDirectory();
std::filesystem::path version_data_path;
if (!data_dir_for_resources.empty()) {
auto candidate = std::filesystem::current_path() / "data" / data_dir_for_resources;
if (std::filesystem::exists(candidate)) {
version_data_path = candidate;
+ } else {
+ std::filesystem::path dir_path(data_dir_for_resources);
+ if (dir_path.has_parent_path() && dir_path.parent_path().filename() == "data") {
+ auto stripped = std::filesystem::current_path() / "data" / dir_path.filename();
+ if (std::filesystem::exists(stripped)) {
+ version_data_path = stripped;
+ }
+ }
}
}🧰 Tools
🪛 Clang (14.0.6)
[note] 573-573: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 575-575: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
🪛 Cppcheck (2.20.0)
[style] 573-573: The function 'hasChanges' is never used.
(unusedFunction)
[style] 578-578: The function 'clearChanges' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/Services/Map/MapLoadingService.cpp` around lines 570 - 578,
The lookup for creatures.xml/items.xml uses data_dir_for_resources directly so
it fails when getDataDirectory() returns a path already prefixed with "data/"
(or "data\\"); normalize that value before building version_data_path. In the
block that computes version_data_path, canonicalize or strip a leading "data"
component from data_dir_for_resources (e.g., remove a leading "data/" or
"data\\", or parse it as a std::filesystem::path and use its filename or
relative tail) so the candidate path resolution (current_path() / "data" /
data_dir_for_resources) matches the same normalization used by the
tileset/material resolution; update the code that references
data_dir_for_resources and version_data_path accordingly (symbols:
getDataDirectory, data_dir_for_resources, version_data_path).
| namespace { | ||
| constexpr float PULSE_DURATION = 1.5f; | ||
| } // namespace |
There was a problem hiding this comment.
Critical: Duplicate PULSE_DURATION constant with conflicting value.
The anonymous namespace defines PULSE_DURATION = 1.5f, but the class header declares static constexpr float PULSE_DURATION = 2.0f at line 271. This creates two constants with different values, and the code currently uses the 1.5f version. Remove the anonymous namespace constant and use the class member instead.
🔧 Proposed fix
-namespace {
-constexpr float PULSE_DURATION = 1.5f;
-} // namespace
-
`#include` "../../Brushes/BrushController.h"Then update lines 150, 394, and 558 to use TilesetGridWidget::PULSE_DURATION or just PULSE_DURATION if within class scope.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 17-17: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp` around lines 16 - 18, Remove
the anonymous-namespace PULSE_DURATION constant and replace its uses with the
class-scoped constant TilesetGridWidget::PULSE_DURATION; locate any references
in TilesetGridWidget.cpp that currently resolve to the local anonymous constant
(mentions in the review correspond to usages around the pulse/timing logic) and
change them to use TilesetGridWidget::PULSE_DURATION (or bare PULSE_DURATION if
inside TilesetGridWidget methods), ensuring only the single static constexpr
float TilesetGridWidget::PULSE_DURATION (the declaration in the header) remains.
| bool isPulsing = isSelected && !pulseBrushName_.empty() && | ||
| brush->getName() == pulseBrushName_; | ||
| float pulseElapsed = 0.0f; | ||
| if (isPulsing) { | ||
| float currentTime = ImGui::GetTime(); | ||
| if (pulseStartTime_ < 0) | ||
| pulseStartTime_ = currentTime; | ||
| pulseElapsed = currentTime - pulseStartTime_; | ||
| if (pulseElapsed >= PULSE_DURATION) { | ||
| pulseBrushName_.clear(); | ||
| pulseStartTime_ = -1.0f; | ||
| isPulsing = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Extract duplicated pulse animation logic into a helper method.
The pulse animation computation at lines 386-399 and 550-563 is identical. Extract this into a private helper method to eliminate duplication and improve maintainability.
♻️ Proposed refactoring
Add this private helper method declaration to the header:
std::pair<bool, float> computePulseState(const Brushes::IBrush *brush);Add this implementation:
std::pair<bool, float> TilesetGridWidget::computePulseState(const Brushes::IBrush *brush) {
bool isPulsing = !pulseBrushName_.empty() && brush && brush->getName() == pulseBrushName_;
float pulseElapsed = 0.0f;
if (isPulsing) {
float currentTime = ImGui::GetTime();
if (pulseStartTime_ < 0) {
pulseStartTime_ = currentTime;
}
pulseElapsed = currentTime - pulseStartTime_;
if (pulseElapsed >= PULSE_DURATION) {
pulseBrushName_.clear();
pulseStartTime_ = -1.0f;
isPulsing = false;
}
}
return {isPulsing, pulseElapsed};
}Then replace both occurrences:
- bool isPulsing = isSelected && !pulseBrushName_.empty() &&
- brush->getName() == pulseBrushName_;
- float pulseElapsed = 0.0f;
- if (isPulsing) {
- float currentTime = ImGui::GetTime();
- if (pulseStartTime_ < 0)
- pulseStartTime_ = currentTime;
- pulseElapsed = currentTime - pulseStartTime_;
- if (pulseElapsed >= PULSE_DURATION) {
- pulseBrushName_.clear();
- pulseStartTime_ = -1.0f;
- isPulsing = false;
- }
- }
+ auto [isPulsing, pulseElapsed] = computePulseState(brush);
+ isPulsing = isPulsing && isSelected;Also applies to: 550-563
🧰 Tools
🪛 Clang (14.0.6)
[note] 386-386: +1
(clang)
[note] 389-389: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 391-391: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 394-394: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[warning] 388-388: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 390-390: variable 'currentTime' is not initialized
(cppcoreguidelines-init-variables)
[warning] 391-391: statement should be inside braces
(readability-braces-around-statements)
[warning] 396-396: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
🪛 Cppcheck (2.20.0)
[style] 390-390: The function 'getSpriteCount' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp` around lines 386 - 399,
Extract the duplicated pulse animation logic into a private helper on
TilesetGridWidget: add a declaration std::pair<bool,float>
computePulseState(const Brushes::IBrush* brush); to the class header, implement
computePulseState to use pulseBrushName_, pulseStartTime_, PULSE_DURATION and
ImGui::GetTime() to compute and return {isPulsing, pulseElapsed} while resetting
pulseBrushName_ and pulseStartTime_ when the duration elapses, and replace the
two identical blocks (the ones using isPulsing, pulseElapsed and
brush->getName() == pulseBrushName_) with calls to computePulseState(brush) to
unpack the returned pair.
Consolidated version-specific data path resolution logic into a central `resolveDataPath` helper and refactored TilesetGridWidget pulse animation logic for improved maintainability. Updated build system to conditionally handle data directory copying to prevent missing directory errors. - `ImguiMapEditor/CMakeLists.txt` — replaced absolute directory copy with a conditional `EXISTS` check for the data directory - `ImguiMapEditor/Domain/ClientVersion.cpp` — delegated path resolution to `resolveDataPath` - `ImguiMapEditor/Domain/ClientVersion.h` — added `resolveDataPath` helper method to encapsulate path resolution logic - `ImguiMapEditor/Services/Map/MapLoadingService.cpp` — utilized `resolveDataPath` to simplify resource loading paths - `ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp` — extracted pulse animation state to `computePulseState`, optimized label truncation - `ImguiMapEditor/UI/Widgets/TilesetGridWidget.h` — declared `computePulseState` helper
github-actions
Bot
added
the
Area: Build
There was a problem hiding this comment.
📋 Review Summary
This PR introduces significant improvements to data directory management and version-specific asset loading. By resolving data paths relative to the application's data folder and supporting version-specific subdirectories, it lays the groundwork for better multi-client support.
However, there are some critical issues that need to be addressed:
- Incomplete Modal Dialog Implementation: The PR adds callbacks to
MainWindowand includesNewMapPanel.h, but the methodsshowNewMapDialog()andshowOpenSecDialog()are not defined in the.cppfile, which will cause linker errors. Furthermore, theNewMapDialogcomponent is not instantiated or rendered withinMainWindow. - Path Resolution Robustness: Several locations still rely on
std::filesystem::current_path(), which can be unreliable depending on how the application is launched. - Redundant Normalization: The normalization logic in
ClientVersionis spread across both the setter and the resolver, which could be consolidated.
🔍 General Feedback
- Correctness: Stray characters in
Tile.cppandCMakeLists.txtwere correctly identified and removed. - Maintainability: The refactoring of
TilesetGridWidget's brush card rendering and pulse state computation improves code readability. - Architecture: Moving towards version-specific data directories is a great architectural decision that solves many asset conflict issues.
Please address the missing definitions in MainWindow.cpp to ensure the project remains buildable.
| // Resolve the version-specific data directory to an absolute path. | ||
| // Returns empty path if data_directory_ is not set or doesn't exist. | ||
| // Handles both "1098" and "data/1098" formats. | ||
| std::filesystem::path resolveDataPath() const { |
There was a problem hiding this comment.
🟡 Relying on std::filesystem::current_path() for data folder resolution can be brittle if the application is launched from a different directory. It is generally safer to use the executable's directory or a configured base path.
| std::filesystem::path dir_path(data_directory_); | ||
| if (dir_path.has_parent_path() && dir_path.parent_path().filename() == "data") { | ||
| auto stripped = std::filesystem::current_path() / "data" / dir_path.filename(); | ||
| if (std::filesystem::exists(stripped)) return stripped; |
There was a problem hiding this comment.
🟡 While this check handles the case where data_directory_ was manually set to a path containing "data/", the setDataDirectory() method already performs this normalization. It might be cleaner to ensure data_directory_ is always normalized upon assignment to avoid redundant checks during path resolution.
| auto otb = data_dir / "items.otb"; | ||
| if (std::filesystem::exists(otb)) return otb; | ||
| break; | ||
| } |
There was a problem hiding this comment.
🟢 Good change. Removing the fallback to client_path_ for items.otb ensures that only the intended editor-specific metadata is used, preventing potential issues with incompatible OTB files found in client directories.
| if (!tilesets_loaded) { | ||
| spdlog::warn("materials.xml could not be loaded. Falling back to direct tileset/palette loading."); | ||
| tilesets_loaded = tileset_service_.loadTilesets(app_data_path); | ||
| // Load tilesets, palettes, brushes from version data directory |
There was a problem hiding this comment.
🟢 Using version_data_path here correctly points the editor to the version-specific assets, which is a major improvement for multi-version support.
| auto stripped = std::filesystem::current_path() / "data" / dir_path.filename(); | ||
| if (std::filesystem::exists(stripped)) return stripped; | ||
| } | ||
| return {}; |
There was a problem hiding this comment.
🟡 This secondary stripping logic seems redundant since setDataDirectory() already handles this normalization. Ensuring data_directory_ is always normalized upon assignment would allow simplifying this method to just checking the current_path() / "data" / data_directory_ candidate.
| UI::CreaturePropertiesDialog creature_properties_dialog_; | ||
|
|
||
| // Editor-state modal dialog callbacks | ||
| std::function<void(const UI::NewMapPanel::State&)> new_map_callback_; |
There was a problem hiding this comment.
🔴 These callbacks are added to support the new modal dialogs, but the corresponding methods showNewMapDialog() and showOpenSecDialog() are declared in the header without definitions in MainWindow.cpp. This will lead to linker errors.
Additionally, MainWindow lacks a NewMapDialog member and the necessary render() call in renderEditor(), which are required for these dialogs to function.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores