feat: OTClient Map Generator by gesior + generate satellite#1549
feat: OTClient Map Generator by gesior + generate satellite#1549kokekanon wants to merge 44 commits intoopentibiabr:mainfrom
Conversation
|
I have no intention of merging it, I'm just sharing it in case anyone needs it. |
|
Did you try to use it with 11+ client ( I can generate items and outfits images using canary files converted by 'spider converter' to 10.98/8.6 format, but my old map/minimap 'mapgen' requires |
This reverts commit 1271bed.
kokekanon
changed the title
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a full map-generation subsystem: new MapGen UI and controller, multi-threaded map/satellite/minimap image generation, satellite chunk management, image utilities (LZMA BMP, shadows), Lua bindings, platform dialogs, and broad client/core API extensions to support offline previews and exports. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I ran the test using protocol bandicam.2026-03-17.04-59-35-992.mp4
It ignores the HTML module; this is intended to make it easier for the user to generate a satellite image. #1662 canary 14.12
|
|
To test the satellite PR
I created a simple interface in an HTML module to make it easy for any user to use. The interface is very intuitive. The required input files are very easy to find using OTC’s For older versions, you need: Warning
if you're going to use Gesior's Mapgen , Be careful with your storage and RAM usage The Canary map is test map in:
bandicam.2026-03-17.04-59-35-992.mp4In this PR you can:
Zoom-in view of the version generated with OTC (map.dat, bmp.lzma) .:facepalm:There are differences compared to the Cipsoft client; I suppose the export LOD needs to be improved. 
Zoom-in satellite view from the native CipSoft client.
Can the generated files map.dat and bmp.lzma from the OTC client be used in the CipSoft client?R: Yes BUT due to the size of the Canary map, I haven’t done many tests, and In the OTC satellite PR, the height and width fields are limited to 512 🤷♂️ (pr1662) if (image->getWidth() != 512 || image->getHeight() != 512) {
g_logger.error("SatelliteMap: chunk '{}' has unexpected size {}x{} (expected 512x512), skipping",
path, image->getWidth(), image->getHeight());
return nullptr;
}In the CipSoft client, there are different sizes fields and different LODs (16, 32, 64) instructionsThe resulting bmp.lzma and map.dat files must be moved to the folder CipSoft Modify the file {
"type": "map",
- "file": "map-f9ff4dddf9a69d6ff185c9564733c7bb1a4c2640e2ac64329bd2a0d486d5c114.dat"
+ "file": "map.dat"
}and test it in Cyclopedia in cipsoft. My focus is not CipSoft; I don’t care enough to fix it :pepelike:
|
I noticed that Mehah and OTCv8 use much more RAM, when they load map than old 'edubart' OTC. If you plan to use it, make sure you do not compile 'debug' version of OTC. It uses 2x more RAM for .otbm than 'release'. I will try to generate canary map today, if I find that 14.12 map. |
|
Short version:
VPS/dedic to host:
My test host with canary map in .webp format: Files to create own host: Some problems with map:EDIT: Near Thais there are some snow tiles (all items on levels above ground are duplicated): Example bugged images: Full description what and how I tested: Then I used tools to generate all zoom levels for website ( https://github.com/gesior/otclient_mapgen/tree/master/website_and_php_files ) - I had to add Then I ran compression script to convert .png to .jpg with 80% quality. It took 6 minutes and after compression images size is 16 GB. Then I compressed it to .7z with 'fast' (level 3) compression. Generated .7z size is 4.7 GB. JPG is very old format. I decided to try .webp. I ran modified compression script to convert .png to .webp with 80% quality. It took 11 minutes and after compression images size is 14.2 GB, but they look much better than .jpg. So to generate full website map of canary you need at least 76 GB on HDD and 20 GB of free RAM. Then you will have to transfer at least 2.4 GB to VPS/dedic and unpack it to 16 GB on VPS/dedic - during decompression you will need 19 GB HDD for .7z + images. |
|
thais: Export PNG 
RME: 
I remember that bug; it only happened in the Canary proto. But I fixed it for floor 7. This code fixes all the floors From bea37748728d7d315cde434b523ccadf0260689b Mon Sep 17 00:00:00 2001
From: kokekanon <[email protected]>
Date: Tue, 17 Mar 2026 16:50:38 -0300
Subject: [PATCH] diff
---
src/client/mapio.cpp | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/client/mapio.cpp b/src/client/mapio.cpp
index 75e26770d..8a56076b6 100644
--- a/src/client/mapio.cpp
+++ b/src/client/mapio.cpp
@@ -52,6 +52,7 @@
#include <thread>
#include <atomic>
#include <set>
+#include <unordered_map>
void Map::loadOtbm(const std::string& fileName)
{
@@ -334,21 +335,28 @@ void Map::loadOtbm(const std::string& fileName)
if (const TilePtr& tile = getTile(pos)) {
if (!strictClassicOtbm) {
- // Some protobuf/canary maps may carry duplicated ground entries.
- // Keep only the last parsed ground to match editor-visible result.
- ThingPtr lastGround = nullptr;
- std::vector<ThingPtr> duplicatedGrounds;
+ // Protobuf/canary maps may carry duplicated entries for fixed-position
+ // items (ground, borders, walls, on-top). These item types can't
+ // legitimately appear more than once per ID on the same tile.
+ // Keep only the last instance to match editor-visible result.
+ // Moveable items (CREATURE/COMMON_ITEMS priority) are left alone.
+ std::unordered_map<uint16_t, ThingPtr> lastById;
+ std::vector<ThingPtr> duplicates;
for (const auto& thing : tile->getThings()) {
- if (!thing->isGround())
+ if (!thing->isItem())
continue;
-
- if (lastGround)
- duplicatedGrounds.emplace_back(lastGround);
- lastGround = thing;
+ const int priority = thing->getStackPriority();
+ if (priority > ON_TOP) // skip CREATURE and COMMON_ITEMS
+ continue;
+ const uint16_t id = thing->static_self_cast<Item>()->getId();
+ auto [it, inserted] = lastById.try_emplace(id, thing);
+ if (!inserted) {
+ duplicates.emplace_back(it->second);
+ it->second = thing;
+ }
}
-
- for (const auto& groundThing : duplicatedGrounds)
- tile->removeThing(groundThing);
+ for (const auto& dup : duplicates)
+ tile->removeThing(dup);
}
if (type == OTBM_HOUSETILE) {
--
2.44.0.windows.1
|
Can you push it as a commit or you want me to test it first with full map generation? |
|
fix --> bea3774 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
src/client/const.h-1210-1212 (1)
1210-1212:⚠️ Potential issue | 🟠 MajorBitmask collision between
TILESTATE_ZONE_BRUSHandTILESTATE_HOUSE.Line 1210 uses
0x0040, which is the same bit as Line 1212 (1 << 6). These two flags become indistinguishable intileflags_t, so house tiles can be misread as zone-brush (and vice versa).Suggested fix
- TILESTATE_ZONE_BRUSH = 0x0040, + TILESTATE_ZONE_BRUSH = 1 << 7,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/const.h` around lines 1210 - 1212, The constants TILESTATE_ZONE_BRUSH and TILESTATE_HOUSE currently share the same bit (0x0040 / 1 << 6) causing a bitmask collision; change one of them to a distinct bit value (e.g., bump TILESTATE_HOUSE to 1 << 7 or TILESTATE_ZONE_BRUSH to 0x0080) so each flag maps to a unique bit, and then search for usages of TILESTATE_HOUSE and TILESTATE_ZONE_BRUSH to ensure any bit-tests/assignments remain correct after the value change.src/protobuf/map.proto-3-3 (1)
3-3:⚠️ Potential issue | 🟠 MajorResolve Buf package-directory mismatch on Line 3.
Line 3 sets
package otclient.protobuf.map, but the file is undersrc/protobuf. WithPACKAGE_DIRECTORY_MATCH, this will fail lint/CI until you either move the file tootclient/protobuf/map/(relative to module root) or adjust package/rule configuration consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protobuf/map.proto` at line 3, The proto package declaration "package otclient.protobuf.map" conflicts with the file location and triggers PACKAGE_DIRECTORY_MATCH; fix by making the package and directory consistent — either move this proto file into the otclient/protobuf/map/ directory hierarchy so it matches "package otclient.protobuf.map", or change the package declaration (e.g., to "package protobuf.map" or "package protobuf") to match the current src/protobuf path, or update the Buf PACKAGE_DIRECTORY_MATCH configuration to permit the existing layout; update the package declaration or relocate the file accordingly and ensure any import references (package otclient.protobuf.map) are updated to match.src/framework/platform/win32platform.cpp-491-516 (1)
491-516:⚠️ Potential issue | 🟠 MajorGuard COM uninitialization with successful initialization result.
CoUninitialize()is called unconditionally, butCoInitialize(nullptr)can fail. Per Microsoft,CoUninitialize()must be paired only with successful initialization (bothS_OKandS_FALSEcount as success). Unpairing causes COM lifetime imbalance.Suggested fix
- CoInitialize(nullptr); + const HRESULT hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED | COINIT_DISABLE_OLE1DDE); + const bool comInitialized = SUCCEEDED(hr); + if (!comInitialized) + return {}; std::string result; @@ - CoUninitialize(); + if (comInitialized) + CoUninitialize(); return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/win32platform.cpp` around lines 491 - 516, CoInitialize(nullptr) is called unconditionally but CoUninitialize() must only be called if initialization succeeded; change the code to capture the HRESULT (e.g. HRESULT hr = CoInitialize(nullptr)) and only execute the COM file dialog flow and call CoUninitialize() when SUCCEEDED(hr) (treat S_OK and S_FALSE as success). Update the block around CoInitialize/CoUninitialize and references to IFileDialog usage (pfd, psi, folderPath) so they are scoped/cleaned only within the successful-initialization branch to avoid unpaired CoUninitialize calls.src/framework/platform/win32platform.cpp-458-487 (1)
458-487:⚠️ Potential issue | 🟠 MajorUse Unicode dialog APIs for file selection.
OPENFILENAMEA+GetOpenFileNameAuse Windows ANSI code page encoding, which fails or corrupts non-ASCII filenames. This is a real correctness and i18n issue on Windows systems with non-Latin characters in file paths. The codebase already providesstdext::utf8_to_utf16()andstdext::utf16_to_utf8()helpers (used elsewhere in this file), so migrating toOPENFILENAMEW+GetOpenFileNameWis straightforward and necessary for proper Unicode support.Suggested fix (Unicode-safe)
-std::string Platform::openFileDialog(std::vector<std::string> extensions) +std::string Platform::openFileDialog(std::vector<std::string> extensions) { - char filename[MAX_PATH] = { 0 }; - std::string filter; + wchar_t filename[MAX_PATH] = { 0 }; + std::wstring filter; for (const auto& ext : extensions) { - filter += ext + " files"; - filter.push_back('\0'); - filter += "*." + ext; - filter.push_back('\0'); + const auto wext = stdext::utf8_to_utf16(ext); + filter += wext + L" files"; + filter.push_back(L'\0'); + filter += L"*." + wext; + filter.push_back(L'\0'); } - filter.push_back('\0'); + filter.push_back(L'\0'); - OPENFILENAMEA ofn; + OPENFILENAMEW ofn; ZeroMemory(&ofn, sizeof(ofn)); ofn.lStructSize = sizeof(ofn); ofn.hwndOwner = nullptr; ofn.lpstrFile = filename; ofn.nMaxFile = sizeof(filename); ofn.lpstrFilter = filter.c_str(); ofn.nFilterIndex = 1; ofn.Flags = OFN_PATHMUSTEXIST | OFN_FILEMUSTEXIST; - if (!GetOpenFileNameA(&ofn)) + if (!GetOpenFileNameW(&ofn)) return ""; - std::string result = filename; + std::string result = stdext::utf16_to_utf8(filename); stdext::replace_all(result, "\\", "/"); return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/win32platform.cpp` around lines 458 - 487, Platform::openFileDialog currently uses OPENFILENAMEA/GetOpenFileNameA which breaks on non-ASCII paths; switch to the Unicode APIs by using OPENFILENAMEW and GetOpenFileNameW, allocate a wchar_t filename buffer (e.g. wchar_t filenameW[MAX_PATH]), build the filter as a UTF-16 (std::wstring) with embedded L'\0' separators and a double-null terminator, set ofn.lpstrFile = filenameW and ofn.nMaxFile = _countof(filenameW), call GetOpenFileNameW, then convert the resulting filenameW back to UTF-8 with stdext::utf16_to_utf8() and normalize slashes; use stdext::utf8_to_utf16() if you need to convert incoming extension strings to build the wide filter.otclientrc.lua-250-259 (1)
250-259:⚠️ Potential issue | 🟠 MajorClear the per-run satellite settings after finishing generation.
satelliteOutputDir_perPartis used as the switch that enables post-PNG satellite export, but nothing resets it infinishFullGeneration(). After one call toprepareSatelliteGeneration(), every latergenerateMap()in the same session will keep reloading the full map and exporting satellite data again.Proposed fix
function finishFullGeneration() isGenerating = false + satelliteOutputDir_perPart = nil + satelliteLod_perPart = 32 MAPGEN_UI_STATUS.active = false MAPGEN_UI_STATUS.phase = 'done' MAPGEN_UI_STATUS.done = mapImagesGeneratedAlso applies to: 298-315, 319-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otclientrc.lua` around lines 250 - 259, prepareSatelliteGeneration sets satelliteOutputDir_perPart (and satelliteLod_perPart) to enable per-run satellite export, but these globals are never cleared so subsequent generateMap() calls keep reloading/exporting; update finishFullGeneration() to clear the per-run settings by setting satelliteOutputDir_perPart = nil, satelliteLod_perPart = 32 (or default), and _satelliteFromFullGenerate = false (and ensure any other cleanup used by prepareSatelliteGeneration is reversed) so satellite export only occurs for the intended run.otclientrc.lua-100-105 (1)
100-105:⚠️ Potential issue | 🟠 MajorValidate
mpcbefore using it as a divisor.
prepareClient()storesmpcverbatim, butprepareClient_action()dividestotalTilesCountby it on Line 184.nil,0, or negative values will break the prepare flow before the user gets any actionable error.Proposed fix
function prepareClient(cv, dp, mp, ttr, mpc) clientVersion = cv definitionsPath = dp mapPath = mp threadsToRun = ttr or 3 - mapPartsCount = mpc + mapPartsCount = math.max(1, math.floor(tonumber(mpc) or 1)) preparedMinPos = nil preparedMaxPos = nil g_logger.info("Loading client data... (it will freeze client for a few seconds)")Also applies to: 183-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otclientrc.lua` around lines 100 - 105, prepareClient currently assigns mapPartsCount = mpc verbatim which later gets used as a divisor in prepareClient_action when computing totalTilesCount / mapPartsCount; validate mpc (and/or mapPartsCount) to ensure it's a positive non-zero integer before storing or before dividing. Fix by checking mpc in prepareClient (function prepareClient) and set mapPartsCount to a safe default (e.g., 1) or reject/raise a clear error if mpc is nil, <= 0, or not an integer; additionally, add a guard in prepareClient_action before dividing totalTilesCount by mapPartsCount to throw/log a descriptive error if mapPartsCount is invalid to avoid runtime divide-by-zero or nil errors.src/framework/platform/unixplatform.cpp-214-222 (1)
214-222:⚠️ Potential issue | 🟠 MajorDo not expose the Unix dialog APIs as silent no-ops.
Both methods always return
"", so Lua cannot distinguish “unsupported on this platform” from “user cancelled the dialog.” On Linux/macOS that turns the new browse flow into a silent failure instead of something the UI can disable or report explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/unixplatform.cpp` around lines 214 - 222, The current implementations of Platform::openFileDialog and Platform::openDirectoryDialog silently return an empty string and thus cannot be distinguished from a user cancel; change both functions to explicitly signal "unsupported on this platform" instead of returning "" — e.g., throw a std::runtime_error or call a dedicated Platform::reportUnsupportedFeature(...) with a clear message like "file/directory dialogs not supported on Unix" so callers (and Lua bindings) can detect and handle unsupported platforms; update both Platform::openFileDialog and Platform::openDirectoryDialog to use this explicit signaling approach.otclientrc.lua-391-399 (1)
391-399:⚠️ Potential issue | 🟠 MajorFail fast when no valid map parts were selected.
If the caller passes
{}or only unknown ids,mapPartsToGeneratestays empty andstartMapPartGenerator()will dereferencemapPartsToGenerate[1]asnil. That turns bad input into a hard Lua error and leavesisGeneratingstucktrue.Proposed fix
for _, i in pairs(mapPartsToGenerateIds) do - table.insert(mapPartsToGenerate, mapParts[i]) + if mapParts[i] then + table.insert(mapPartsToGenerate, mapParts[i]) + else + g_logger.error("generateMap: invalid map part id " .. tostring(i)) + end end + + if `#mapPartsToGenerate` == 0 then + isGenerating = false + MAPGEN_UI_STATUS.active = false + MAPGEN_UI_STATUS.phase = 'idle' + MAPGEN_UI_STATUS.message = 'No valid map parts selected' + return + end startTime = os.time()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otclientrc.lua` around lines 391 - 399, After populating mapPartsToGenerate from mapPartsToGenerateIds, add a guard that checks if mapPartsToGenerate is empty and, if so, set isGenerating = false, log or report an error about no valid map parts (include mapPartsToGenerateIds for context), and return early instead of calling startMapPartGenerator() or scheduling generateManager; this prevents dereferencing mapPartsToGenerate[1] in startMapPartGenerator and leaves state consistent when inputs are invalid.src/framework/graphics/image.cpp-107-165 (1)
107-165:⚠️ Potential issue | 🟠 MajorValidate
m_bppbefore encoding as BGRA.
saveBmpLzma()indexesm_pixelsas 4 bytes per pixel from Line 158 onward, butImageis not restricted to RGBA. Calling this on a 1-byte or 3-byte image will use the wrong stride and can produce corrupted output or an out-of-bounds read.Proposed fix
void Image::saveBmpLzma(const std::string& fileName) { + if (m_bpp != 4) + throw Exception("saveBmpLzma expects 4 BPP RGBA input, got {}", m_bpp); + // Produces a CIP-format .bmp.lzma file compatible with SatelliteMap::loadChunkTexture(): // [32-byte zero header] + [LZMA-alone stream of a 32-bpp BI_BITFIELDS BMP]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/graphics/image.cpp` around lines 107 - 165, The saveBmpLzma function assumes 4 bytes per pixel but Image can have other bpp values; add a guard at the start of Image::saveBmpLzma that checks m_bpp and handles non-4bpp cases (either convert to 32-bit RGBA before building the BMP or fail early with a clear error/return). Specifically, in saveBmpLzma check if m_bpp != 4: if you choose conversion, create a temporary pixel buffer expanded from m_pixels (handle 1-byte grayscale and 3-byte RGB -> produce RGBA with alpha=255) and then use that buffer in the loop (referencing m_pixels usage and the dst conversion loop); otherwise log/throw/return a useful error mentioning saveBmpLzma and m_bpp so callers won't trigger OOB reads. Ensure all references in the pixel-copy loop use the validated/converted buffer.src/client/uiminimap.cpp-172-176 (1)
172-176:⚠️ Potential issue | 🟠 MajorRandom-position source selection is inconsistent with render fallback.
Lines 172–176 choose satellite search based only on mode flags. When chunks are missing, this can return invalid and skip the minimap fallback that
drawSelfalready applies.🛠️ Proposed fix
void UIMinimap::selectRandomPosition() { Position pos; - if (m_satelliteMode || m_useStaticMinimap) { + if (m_satelliteMode && g_satelliteMap.hasChunksForView(m_cameraPosition.z)) { pos = g_satelliteMap.findRandomValidPosition(m_cameraPosition.z); + } else if (m_useStaticMinimap && g_satelliteMap.hasMinimapChunksForFloor(m_cameraPosition.z)) { + pos = g_satelliteMap.findRandomValidPosition(m_cameraPosition.z); } else { pos = g_minimap.findRandomValidPosition(m_cameraPosition.z); } if (pos.isValid()) { setCameraPosition(pos); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/uiminimap.cpp` around lines 172 - 176, The current selection of random-position source uses only m_satelliteMode/m_useStaticMinimap and can return an invalid pos when satellite chunks are missing; update the logic around pos assignment (the block calling g_satelliteMap.findRandomValidPosition and g_minimap.findRandomValidPosition) so that after calling the satellite lookup you validate the returned pos and, if it is invalid, fall back to calling g_minimap.findRandomValidPosition (mirror the same fallback behavior used by drawSelf). Ensure you still prefer the minimap when satellite mode is off and only use the satellite result when it is both requested and yields a valid position.vc18/otclient.vcxproj-279-279 (1)
279-279:⚠️ Potential issue | 🟠 Major
FRAMEWORK_EDITORis enabled only in one build configuration, causing module load failures in other targets.Line 279 enables
FRAMEWORK_EDITORonly forOpenGL|x64. The Lua API bindings for mapgen functions—includingg_minimap.findRandomValidPosition,g_satelliteMap.findRandomValidPosition, andg_map.saveImage—are#ifdef FRAMEWORK_EDITOR-guarded insrc/client/luafunctions.cpp(lines 247, 258, and elsewhere). While thegame_mapgenmodule remains a hard dependency inmodules/game_interface/interface.otmod, non-OpenGL|x64 builds will lack these bindings and fail when the module attempts to call them. EnableFRAMEWORK_EDITORuniformly across all Visual Studio configurations or make the module dependency conditional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vc18/otclient.vcxproj` at line 279, BUILD mismatch: FRAMEWORK_EDITOR is only defined for OpenGL|x64 causing missing Lua bindings (g_minimap.findRandomValidPosition, g_satelliteMap.findRandomValidPosition, g_map.saveImage) in luafunctions.cpp and runtime failures when the game_mapgen module from modules/game_interface/interface.otmod is loaded; fix by either defining FRAMEWORK_EDITOR in all Visual Studio build configurations (remove the platform-specific guard so the PreprocessorDefinitions include FRAMEWORK_EDITOR for every config) or make the game_mapgen dependency conditional (ensure interface.otmod only lists game_mapgen when FRAMEWORK_EDITOR is defined) so the guarded Lua bindings and module dependency remain consistent across builds.src/client/spritemanager.cpp-437-441 (1)
437-441:⚠️ Potential issue | 🟠 MajorStatic local cache becomes stale after reload/unload operations.
The static
SpriteCacheinstance persists for the entire program lifetime. However,SpriteManagerprovidesreload()(called insrc/client/client.cpp:154) andunload()(exposed to Lua) methods that invalidate the internal sprite state without clearing this cache. Subsequent calls togetSpriteImageCached()will return stale image pointers from before the reload/unload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/spritemanager.cpp` around lines 437 - 441, The static SpriteCache in SpriteManager::getSpriteImageCached causes stale images after SpriteManager::reload() or unload() because it lives for program lifetime; fix by ensuring the cache is cleared or recreated when sprite data is reloaded/unloaded—add a clear/reset method on SpriteCache (e.g., SpriteCache::clear() or reset()) and call it from SpriteManager::reload() and the Lua-exposed unload(), or make the cache an instance member of SpriteManager so it gets reconstructed during reload/unload; update getSpriteImageCached to use the non-static cache or rely on the cleared cache.src/client/satellitemap.cpp-88-95 (1)
88-95:⚠️ Potential issue | 🟠 MajorRescan reused chunk directories.
This cache never invalidates for a reused path. In this PR the client can generate
.bmp.lzmafiles and then preview them from the same directory, so oncebuildFileCache()has seen an empty/stale folder,loadFloors()can never discover the new chunks untilclear()or restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 88 - 95, The early return in SatelliteMap::buildFileCache (the line "if (dir == m_fileCacheDir) return;") prevents rescanning a reused directory, so new .bmp.lzma chunks are never discovered; remove that unconditional equality check (or replace it with a proper change-detection check) so the method always clears and repopulates m_fileCache, perform the directory scan regardless, and then update m_fileCacheDir (keep the existing m_fileCache.clear() and m_fileCacheDir = dir assignment around the actual scan) to ensure subsequent calls see newly created files.modules/game_cyclopedia/tab/map/map.lua-38-43 (1)
38-43:⚠️ Potential issue | 🟠 MajorDon't mark floors as loaded after a miss.
loadedFloorSetis flipped totrueeven wheng_satelliteMap.loadFloors()indexed nothing. After that first miss, the same floor is never retried for this client version, so chunks generated later in the session stay invisible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_cyclopedia/tab/map/map.lua` around lines 38 - 43, The code marks floors in loadedFloorSet as true for the full range minNeeded..maxNeeded regardless of whether g_satelliteMap.loadFloors actually loaded any of them; change this so you only set loadedFloorSet[f] = true for floors that were actually loaded by g_satelliteMap.loadFloors (e.g., use the return value from g_satelliteMap.loadFloors if it returns loaded floor IDs, or call a validation function like g_satelliteMap.hasFloor or request per-floor loading and mark only successful loads). Update the loop around loadedFloorSet to consult that success list/result instead of blindly marking the whole range.modules/game_mapgen/mapgen.html-130-131 (1)
130-131:⚠️ Potential issue | 🟠 MajorFix the 740/770 option values.
Lines 130-131 currently set
self.clientVersionto710/760while the UI says740/770, so selecting those entries targets the wrong client build and wrong derived paths.🛠️ Suggested fix
- <option value="710">740</option> - <option value="760">770</option> + <option value="740">740</option> + <option value="770">770</option>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_mapgen/mapgen.html` around lines 130 - 131, Update the mismatched option value attributes so selecting the "740" and "770" UI entries sets the correct client build: change the <option> elements that currently use value="710" and value="760" to value="740" and value="770" respectively so they correctly assign self.clientVersion/derived paths (look for the option tags that populate self.clientVersion).src/client/mapio.cpp-103-124 (1)
103-124:⚠️ Potential issue | 🟠 MajorDetect classic OTBM from the file, not from the current runtime state.
Lines 103-124 treat the map as client-id based whenever
items.otbis not loaded. For a classic server-id OTBM that silently reinterprets server ids as client ids and produces a corrupted map instead of aborting.🛠️ Suggested fix
- const bool useOtbItemIds = g_things.isOtbLoaded() && headerMajorItems > 0; - const bool classicOtbm = useOtbItemIds; + const bool classicOtbm = headerMajorItems > 0; + if (classicOtbm && !g_things.isOtbLoaded()) + throw Exception("This OTBM uses server ids, but items.otb is not loaded."); + const bool useOtbItemIds = classicOtbm; const bool strictClassicOtbm = classicOtbm && headerVersion <= 3;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/mapio.cpp` around lines 103 - 124, The code currently decides whether to treat item ids as OTB/server-based using g_things.isOtbLoaded() (runtime state) which can silently reinterpret server ids as client ids; instead determine OTBM type solely from the map header values you just read: derive useOtbItemIds/classicOtbm/strictClassicOtbm from headerMajorItems and headerVersion (e.g., useOtbItemIds = headerMajorItems > 0; classicOtbm = useOtbItemIds; strictClassicOtbm = classicOtbm && headerVersion <= 3), then use that when constructing createMapItem; additionally, when headerMajorItems/headerMinorItems differ from g_things.getOtbMajor/MinorVersion(), fail or log a clear error and abort (throw) rather than silently proceeding to avoid corrupted maps.modules/game_mapgen/mapgen.lua-1795-1809 (1)
1795-1809:⚠️ Potential issue | 🟠 MajorReject nil and unknown part ids before generation.
Unlike
_resolvePartsIdsSoft, this path assumesself.genCustomPartsis always initialized and accepts ids that do not exist in_mapParts. That can either throw on the first custom run or pass nonexistent parts into the generator.Suggested fix
function MapGenUI:_resolvePartsIds() local partsIds = {} if self.genPartsMode == 'all' then for i = 1, `#_mapParts` do table.insert(partsIds, i) end else - for s in self.genCustomParts:gmatch('[^,]+') do + for s in (self.genCustomParts or ''):gmatch('[^,]+') do local n = tonumber(s:match('^%s*(.-)%s*$')) - if n then table.insert(partsIds, n) end + if n and _mapParts[n] then + table.insert(partsIds, n) + end end end if `#partsIds` == 0 then self:addLog('ERROR: no parts selected.', '#ff6666')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_mapgen/mapgen.lua` around lines 1795 - 1809, MapGenUI:_resolvePartsIds currently assumes self.genCustomParts is set and doesn’t validate ids; update it to reject nil/empty self.genCustomParts and validate each parsed id against the available _mapParts range (1..#_mapParts), adding only existing ids and logging an error and returning nil if none are valid or if any id is out of range; mirror the behavior of _resolvePartsIdsSoft by trimming whitespace, using tonumber safely, and calling self:addLog('ERROR: ...', '#ff6666') before returning nil when inputs are missing or invalid.modules/game_mapgen/mapgen.lua-1833-1844 (1)
1833-1844:⚠️ Potential issue | 🟠 MajorDon't silently widen sparse floor lists into a full range.
The UI says custom floors are comma-separated, but
0,7,15becomes0-15here. On the large maps this tool targets, that can generate far more chunks than the user asked for.Suggested fix
if self.satGenFloorsMode == 'custom' and g_map.setGenerateFloorRange then local floors = {} for s in (self.satGenCustomFloors or '7'):gmatch('[^,]+') do local n = tonumber(s:match('^%s*(.-)%s*$')) if n then table.insert(floors, n) end end if `#floors` > 0 then + table.sort(floors) + for i = 2, `#floors` do + if floors[i] ~= floors[i - 1] + 1 then + self:addLog('ERROR: custom satellite floors must be a contiguous range.', '#ff6666') + return + end + end local minF = math.max(0, math.min(unpack(floors))) local maxF = math.min(15, math.max(unpack(floors))) g_map.setGenerateFloorRange(minF, maxF) self:addLog(string.format('Floor filter: %d-%d', minF, maxF), '#aaaacc') else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_mapgen/mapgen.lua` around lines 1833 - 1844, The code currently expands a comma list like satGenCustomFloors ('0,7,15') into a single wide range via setGenerateFloorRange(minF,maxF); instead, detect whether the user explicitly requested a range (the original string contains '-') or whether the parsed numbers form consecutive sequences, and only then call g_map.setGenerateFloorRange with a min/max; otherwise split the parsed satGenCustomFloors (from satGenCustomFloors string and the floors table) into sorted unique floor numbers, merge any consecutive numbers into contiguous ranges, and call g_map.setGenerateFloorRange once per contiguous range (using the function g_map.setGenerateFloorRange(min,max)), and update the self:addLog message to reflect the actual discrete floors/ranges rather than a single widened min-max; keep behavior tied to satGenFloorsMode and use the existing symbols (satGenCustomFloors, satGenFloorsMode, g_map.setGenerateFloorRange, self:addLog) to locate and change the code.
🟡 Minor comments (7)
modules/client_terminal/terminal.lua-204-204 (1)
204-204:⚠️ Potential issue | 🟡 MinorAvoid forcing terminal open during initialization.
Line 204 makes the terminal auto-open on every init, overriding prior hidden/default behavior and potentially stealing focus on startup.
Suggested fix
- toggle() + -- Keep default startup state; open manually via Ctrl+T.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/client_terminal/terminal.lua` at line 204, The unconditional call to toggle() in the module forces the terminal to open on every initialization; remove that unconditional call and only toggle when explicitly requested (e.g., when a config flag like forceOpenOnInit/open_on_start is true) or when the saved/previous state indicates the terminal should be visible; replace the bare toggle() with a conditional such as: if config.forceOpenOnInit then toggle() end (or check a terminal:is_visible()/is_open() helper before toggling) so initialization preserves prior hidden/default behavior and does not steal focus.src/framework/luafunctions.cpp-139-141 (1)
139-141:⚠️ Potential issue | 🟡 MinorRemove the duplicate
g_platform.getMemoryUsageregistration.Line 141 binds the same Lua symbol that was already registered on Line 128. Best case this is dead duplicate code; worst case the binder treats it as a duplicate-definition error during startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/luafunctions.cpp` around lines 139 - 141, The snippet registers g_platform.getMemoryUsage twice via g_lua.bindSingletonFunction; remove the duplicate registration by deleting the second bindSingletonFunction call that references g_platform and Platform::getMemoryUsage so only the original binding remains (keep the first registration and remove the later redundant g_lua.bindSingletonFunction("g_platform", "getMemoryUsage", &Platform::getMemoryUsage, &g_platform) line).modules/game_mapgen/game_mapgen.otmod-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorFix typo in module description.
Line 3 contains a double space in user-facing metadata (
for map).✏️ Proposed fix
- description: Quality of Life for map generation + description: Quality of Life for map generation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_mapgen/game_mapgen.otmod` at line 3, The module metadata string in the description field contains an accidental double space ("for map"); update the description value to use a single space so it reads "Quality of Life for map generation" (edit the description metadata entry in the module file).src/client/tile.cpp-1156-1167 (1)
1156-1167:⚠️ Potential issue | 🟡 MinorIncorrect break condition may skip valid normal items.
The reverse iteration breaks when encountering
isOnTop(),isOnBottom(),isGroundBorder(),isGround(), orisCreature(). However, since the list is traversed in reverse and these items are typically at the beginning ofm_things, this break will exit prematurely and may skip valid common items that appear before these in reverse order.The condition should likely use
continueto skip these items rather thanbreak, or the loop should iterate forward and filter appropriately.🐛 Proposed fix
// normal items for (auto it = m_things.rbegin(); it != m_things.rend(); ++it) { const ThingPtr& thing = *it; if (thing->isOnTop() || thing->isOnBottom() || thing->isGroundBorder() || thing->isGround() || thing->isCreature()) - break; + continue; thing->drawToImage(Point(x - m_drawElevation, y - m_drawElevation), image); m_drawElevation += thing->getElevation(); if (m_drawElevation > 24) m_drawElevation = 24; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/tile.cpp` around lines 1156 - 1167, The reverse iteration over m_things uses a break when encountering items with isOnTop(), isOnBottom(), isGroundBorder(), isGround(), or isCreature(), which prematurely stops the loop and skips earlier normal items; change the logic in that loop to skip those unwanted types (use continue) instead of breaking so drawToImage and the m_drawElevation adjustments still run for valid normal items while ignoring the listed types during the reverse traversal.modules/game_mapgen/mapgen.html-773-773 (1)
773-773:⚠️ Potential issue | 🟡 MinorKeep the cache path portable.
Line 773 forces backslashes before calling
openDir(). That breaks this button on Linux/macOS because a valid write dir like/home/user/.otclientbecomes\home\user\.otclient.🛠️ Suggested fix
- <button class="mg-btn" style="float: right" onclick='g_platform.openDir(g_resources.getWriteDir():gsub("[/\\]+", "\\"))'>Open Cache</button> + <button class="mg-btn" style="float: right" onclick='g_platform.openDir(g_resources.getWriteDir())'>Open Cache</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_mapgen/mapgen.html` at line 773, The button's onclick forces Windows backslashes by calling g_resources.getWriteDir():gsub("[/\\]+", "\\") which breaks paths on Linux/macOS; update the onclick for the Open Cache button to pass the write directory directly to g_platform.openDir (i.e., use g_platform.openDir(g_resources.getWriteDir())) so the path remains portable across platforms and let openDir handle any needed normalization.modules/game_mapgen/mapgen.lua-800-823 (1)
800-823:⚠️ Potential issue | 🟡 MinorKeep
openPathDirplatform-neutral.This helper is hard-coded for Windows: every non-drive-letter path is remapped into
workDir/writeDir, and Line 817 rewrites separators to\. On Linux/macOS, a real path chosen from the file dialog like/home/user/outputwill be mangled instead of opened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_mapgen/mapgen.lua` around lines 800 - 823, The code in mapgen.lua currently forces Windows-specific path handling: the remapping logic for dir -> fullPath and the final normalization finalPath:gsub("[/\\]+", "\\") assume Windows and will mangle POSIX paths; update the logic in the block that computes fullPath and finalPath (referencing dir, subPath, workDir, writeDir, fullPath, finalPath and g_platform.openDir) so that you only apply the drive-letter check/remapping and backslash normalization on Windows hosts—detect Windows via an existing platform indicator (e.g. g_platform or a runtime check) or package.config—and for non-Windows leave POSIX absolute paths intact and normalize to forward slashes (or the platform separator) instead of forcing "\\" before calling g_platform.openDir.modules/game_mapgen/mapgen.css-194-200 (1)
194-200:⚠️ Potential issue | 🟡 MinorSuccess buttons need a normal-state background.
Hover/pressed colors are defined, but the base rule never sets
background-color. The button will render like a fallback/default control until hovered, which makes the success action look inactive.Suggested fix
.mg-btn-success { - --image-color: `#ff0000`; + background-color: `#22773cff`; border: 1 `#ff9100`; color: white; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_mapgen/mapgen.css` around lines 194 - 200, The .mg-btn-success rule lacks a base background, so add a normal-state background-color to .mg-btn-success (e.g., use the existing --image-color or a dedicated --success-bg) so the button looks active before :hover/:pressed; update the .mg-btn-success selector (not the :hover/:pressed ones) to set background-color and ensure the color/contrast stays readable with the existing color: white declaration.
🧹 Nitpick comments (7)
src/framework/graphics/image.h (1)
43-43: Consider renamingwasBlited/m_blitedtowasBlitted/m_blitted.This is minor, but it improves API clarity before wider adoption.
Suggested rename
- bool wasBlited() const { return m_blited; } // Returns true if blit() was called + bool wasBlitted() const { return m_blitted; } // Returns true if blit() was called @@ - bool m_blited{ false }; // Tracks if any pixel was drawn via blit() + bool m_blitted{ false }; // Tracks if any pixel was drawn via blit()Also applies to: 86-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/graphics/image.h` at line 43, Rename the misspelled accessor and member to use the correct past-tense "blitted": change the method wasBlited() to wasBlitted() and the member m_blited to m_blitted everywhere they appear (including related declarations/usages such as the occurrence noted around line 86); update any callers, tests, and documentation to use wasBlitted() and m_blitted to keep the API consistent and avoid breakage, and add a deprecation forwarder if you need backward compatibility temporarily (e.g., keep wasBlited() forwarding to wasBlitted() with a TODO comment).src/framework/platform/win32platform.cpp (1)
29-37: Remove duplicatepsapiinclude/pragma declarations.
<psapi.h>and#pragma comment(lib, "psapi.lib")are both duplicated, which adds noise and can confuse maintenance.Suggested cleanup
`#include` <psapi.h> `#include` <windows.h> `#include` <shellapi.h> `#include` <commdlg.h> `#include` <shlobj.h> -#include <psapi.h> `#pragma` comment(lib, "psapi.lib") - -#pragma comment(lib, "psapi.lib")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/win32platform.cpp` around lines 29 - 37, Remove the duplicated psapi include and pragma lines: keep a single `#include` <psapi.h> and a single `#pragma` comment(lib, "psapi.lib") at the top of win32platform.cpp; remove the extra occurrences so only one declaration of <psapi.h> and one `#pragma` comment(lib, "psapi.lib") remain (check around the existing includes for windows.h, shellapi.h, commdlg.h, shlobj.h to place the single remaining declarations).src/client/uiminimap.h (1)
59-94: Consider adding explicit#include <algorithm>for code clarity.While
std::clamp(used at line 74) is available through transitive includes viapch.h, explicitly including<algorithm>in this header makes the dependency self-contained and improves code clarity. Current dependency chain:uiminimap.h→declarations.h→global.h→pch.h→<algorithm>.Suggested fix
`#include` "declarations.h" +#include <algorithm> `#include` <framework/ui/uiwidget.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/uiminimap.h` around lines 59 - 94, Add an explicit `#include` <algorithm> to this header so std::clamp used in setFloorSeparatorOpacity is defined without relying on transitive includes; open src/client/uiminimap.h and add the include near the other standard headers (so setFloorSeparatorOpacity and any other uses of std::clamp/algorithm utilities resolve cleanly and the header becomes self-contained).otclientrc.lua (1)
254-259: Either honorlodor stop exposing it.
prepareSatelliteGeneration()stores the caller-supplied LOD, butgenerateSatelliteData()immediately replaces it with a fixed{16, 32}satellite queue and minimap LOD32. On large exports that means the public API can silently do much more work than requested.Also applies to: 682-696
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otclientrc.lua` around lines 254 - 259, prepareSatelliteGeneration currently stores the caller LOD into satelliteLod_perPart but generateSatelliteData then overrides it with hardcoded values (e.g., fixed queue {16,32} and minimap LOD 32), so either honor the provided lod or remove lod from the public API; update generateSatelliteData to use satelliteLod_perPart (and respect satelliteOutputDir_perPart) when building the satellite queue/minimap LOD, or alternatively remove the lod parameter and all assignments to satelliteLod_perPart in prepareSatelliteGeneration and callers, ensuring the unique symbols prepareSatelliteGeneration, generateSatelliteData, satelliteLod_perPart, satelliteOutputDir_perPart, and any satellite queue construction are updated accordingly.vc18/otclient.vcxproj (1)
546-548: Consider groupingClCompilewith other compile entries.Line 546 is valid, but placing
ClCompileitems with the main compile item group keeps the project file easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vc18/otclient.vcxproj` around lines 546 - 548, The ClCompile entry for "..\src\client\satellitemap.cpp" is currently separated from the main set of compile items; move the <ClCompile Include="..\src\client\satellitemap.cpp" /> element into the primary ItemGroup that contains other <ClCompile> entries so all compile units are grouped together, leaving the <ClInclude> entries for "..\src\client\satellitemap.h" and "..\src\client\spritemanager.h" in the header/include ItemGroup; ensure you remove any duplicate entries after moving the ClCompile element and keep the project XML well-formed.src/client/minimap.cpp (1)
314-319: Minor: Hardcoded magic number 255 for transparency check.The color value 255 is used to represent transparent/invalid tiles. Consider using a named constant for clarity.
♻️ Suggested improvement
+// In header or at file scope +static constexpr uint8_t MINIMAP_TRANSPARENT_COLOR = 255; + void Minimap::saveImage(const std::string& fileName, int minX, int minY, int maxX, int maxY, short z) { // ... - if (c != 255) { + if (c != MINIMAP_TRANSPARENT_COLOR) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/minimap.cpp` around lines 314 - 319, Replace the hardcoded 255 used to indicate a transparent/invalid tile with a named constant and use that constant in the transparency check; locate the block that reads getTile(Position(x, y, z)).color and the surrounding logic that assigns Color col = Color::alpha and calls Color::from8bit and col.setAlpha, introduce a descriptive constant (e.g., Tile::TransparentColor or MINIMAP_TRANSPARENT_COLOR) and replace the literal 255 in the comparison (if (c != 255)) with that constant so the meaning is clear and maintainable.src/client/tile.cpp (1)
1138-1154: Consider extracting magic numbers 2322 and 2323 into named constants.These hardcoded item IDs for the "table height fix" hack reduce readability and maintainability. Consider defining them as named constants with a comment explaining their purpose.
♻️ Suggested improvement
+// Item IDs that require special elevation handling (tables) +static constexpr uint16_t TABLE_ITEM_ID_1 = 2322; +static constexpr uint16_t TABLE_ITEM_ID_2 = 2323; + bool Tile::drawToImage(const Point& dest, ImagePtr image) { // ... - if (thing->getId() == 2322 || thing->getId() == 2323) { + if (thing->getId() == TABLE_ITEM_ID_1 || thing->getId() == TABLE_ITEM_ID_2) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/tile.cpp` around lines 1138 - 1154, Extract the magic IDs 2322 and 2323 used for the "table height fix" into named constants (e.g., constexpr int TABLE_ID_SMALL = 2322; constexpr int TABLE_ID_LARGE = 2323; or a small array/bitset) and replace direct uses of thing->getId() == 2322 / 2323 and != checks with comparisons against those constants; add a brief comment above the constants explaining they are table item IDs used to adjust m_drawElevation in the draw loop so future readers understand the hack, and update any related comparisons in the block using m_drawElevation, drawToImage, and thing->getId() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ad014d4-a107-4f53-bb3d-7d942e2c40dc
📒 Files selected for processing (44)
data/styles/10-comboboxes.otuimodules/client_terminal/terminal.luamodules/game_cyclopedia/tab/map/map.luamodules/game_cyclopedia/tab/map/map.otuimodules/game_interface/interface.otmodmodules/game_mapgen/game_mapgen.otmodmodules/game_mapgen/mapgen.cssmodules/game_mapgen/mapgen.htmlmodules/game_mapgen/mapgen.luamodules/game_minimap/minimap.luamodules/gamelib/ui/uiminimap.luaotclientrc.luasrc/CMakeLists.txtsrc/client/client.cppsrc/client/const.hsrc/client/item.cppsrc/client/item.hsrc/client/luafunctions.cppsrc/client/map.cppsrc/client/map.hsrc/client/mapio.cppsrc/client/minimap.cppsrc/client/minimap.hsrc/client/satellitemap.cppsrc/client/satellitemap.hsrc/client/spritemanager.cppsrc/client/spritemanager.hsrc/client/thing.hsrc/client/thingtype.cppsrc/client/thingtype.hsrc/client/tile.cppsrc/client/tile.hsrc/client/uimap.cppsrc/client/uiminimap.cppsrc/client/uiminimap.hsrc/framework/graphics/image.cppsrc/framework/graphics/image.hsrc/framework/luafunctions.cppsrc/framework/platform/platform.hsrc/framework/platform/unixplatform.cppsrc/framework/platform/win32platform.cppsrc/protobuf/map.protovc18/otclient.vcxprojvc18/settings.props
| function MapGenUI:doStopGenerate() | ||
| self:setHighLoad(false) | ||
| self.isGenerating = false | ||
| self.statusText = 'Stopped by user.' | ||
| self:addLog('Generation stopped by user.', '#ddaa44') | ||
| end |
There was a problem hiding this comment.
Stop only resets the UI state.
This never signals the running generator, so CPU/RAM/disk work keeps going after the UI says "Stopped". Because self.isGenerating is cleared immediately, the user can also start a second generation on top of the first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_mapgen/mapgen.lua` around lines 1643 - 1648,
MapGenUI:doStopGenerate currently only resets UI state and does not signal the
running generator; modify it to set a cancellation flag and/or call the
generator's stop method so the background generation actually halts before
clearing isGenerating. Specifically, add something like self.stopRequested =
true (or call self.generator:requestStop()/self.generator:stop() if a stop API
exists) and then defer clearing self.isGenerating and updating statusText/log
until the generator loop or a watcher confirms termination (e.g., generator sets
self.isGenerating = false or calls a callback). Update any start logic (e.g.,
MapGenUI:startGenerate or the generator coroutine) to check self.stopRequested
regularly and to exit promptly, and ensure startGenerate prevents a new run
while the previous generator is still active.
| if count == satelliteLastCount and count > 0 then | ||
| satelliteStableTicks = satelliteStableTicks + 1 | ||
| else | ||
| satelliteStableTicks = 0 | ||
| satelliteLastCount = count | ||
| end | ||
|
|
||
| if (total > 0 and count >= total) or satelliteStableTicks >= 4 then |
There was a problem hiding this comment.
Do not use a 4-tick stall as the completion signal.
A slow chunk render or compression pass can easily leave count unchanged for 4 seconds before the generator is actually finished. This branch then advances to the next LOD or writes map.dat against an in-flight job, which risks incomplete or corrupted output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@otclientrc.lua` around lines 763 - 770, The current logic uses
satelliteStableTicks >= 4 as a completion signal which is unreliable; change the
branch so you only advance LOD or write map.dat when the generator actually
reports completion (e.g. a job-complete flag or status check) or when count >=
total; remove/replace the hard-coded 4-tick stall check on satelliteStableTicks
and instead query an explicit completion indicator (implement or call a function
like isSatelliteJobDone()/satelliteJob.status or satelliteFinished boolean) and
use that in the condition alongside (total > 0 and count >= total) to ensure no
in-flight job is still writing.
| void setTerminating(bool terminating) { m_isTerminating = terminating; } | ||
| uint64_t getAreasCount() { return m_mapAreas.size(); } | ||
| int getGeneratedAreasCount() { return m_generatedAreasCount; } | ||
| void setGeneratedAreasCount(int countOfAreas) { m_generatedAreasCount = countOfAreas; } | ||
| void increaseGeneratedAreasCount(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bm_generatedAreasCount\b|\bm_isTerminating\b|\bsetTerminating\s*\(|\bgetGeneratedAreasCount\s*\(|\bsetGeneratedAreasCount\s*\(|\bincreaseGeneratedAreasCount\s*\('Repository: opentibiabr/otclient
Length of output: 5759
Make generator progress and stop state thread-safe.
Generation is multi-threaded, but the public accessors for m_generatedAreasCount and m_isTerminating bypass synchronization entirely. The getGeneratedAreasCount() and setGeneratedAreasCount() methods read/write without acquiring m_generatedAreasCountMutex, while setTerminating() and unprotected reads in map.cpp (lines 89, 556) access m_isTerminating without any lock. This creates data races where progress polling and stop requests can be torn or missed.
Consider whether to switch both fields to std::atomic<> or extend mutex coverage to all accessor methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/map.h` around lines 243 - 247, Make access to generator progress
and the terminating flag atomic to eliminate data races: change
m_generatedAreasCount and m_isTerminating to std::atomic (e.g., std::atomic<int>
and std::atomic<bool>), then update the public accessors setTerminating(bool),
getGeneratedAreasCount(), setGeneratedAreasCount(int) and
increaseGeneratedAreasCount() to operate on the atomics (use
store/load/fetch_add as appropriate), and replace any raw reads/writes of
m_isTerminating in map.cpp with atomic loads/stores so all reads/writes are
thread-safe.
| void Minimap::saveImage(const std::string& fileName, int minX, int minY, int maxX, int maxY, short z) | ||
| { | ||
| //TODO | ||
| ImagePtr image(new Image(Size(maxX - minX, maxY - minY))); | ||
|
|
||
| for (int x = minX; x < maxX; x++) { | ||
| for (int y = minY; y < maxY; y++) { | ||
| uint8_t c = getTile(Position(x, y, z)).color; | ||
| Color col = Color::alpha; | ||
| if (c != 255) { | ||
| col = Color::from8bit(c); | ||
| } | ||
| col.setAlpha(255); | ||
| image->setPixel(x - minX, y - minY, col); | ||
|
|
||
| } | ||
| } | ||
|
|
||
| image->savePNG(fileName); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how saveImage is called to verify bound conventions
rg -n "saveImage\s*\(" --type cpp --type lua -A 2 | head -50Repository: opentibiabr/otclient
Length of output: 2984
🏁 Script executed:
# Check the Map::saveImage implementation
sed -n '909,950p' src/client/mapio.cppRepository: opentibiabr/otclient
Length of output: 2137
🏁 Script executed:
# Look for how bounds are calculated in Lua to understand the convention
rg -n "partMinX|partMaxX|minPos|maxPos" modules/game_mapgen/mapgen.lua -B 3 -A 1 | head -80Repository: opentibiabr/otclient
Length of output: 1741
Off-by-one error in image dimensions: Minimap::saveImage treats bounds as exclusive, but callers expect inclusive.
The image size calculation uses (maxX - minX, maxY - minY), treating maxX and maxY as exclusive bounds. However, Map::saveImage (line 917-918 in mapio.cpp) uses maxX - minX + 1 for width/height, and Lua callers calculate width as partMaxX - partMinX + 1, indicating inclusive upper bounds are expected. This causes images to be undersized by 1 pixel in both dimensions. The dimension should be (maxX - minX + 1, maxY - minY + 1) to match the calling convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/minimap.cpp` around lines 308 - 326, Minimap::saveImage currently
allocates Image(Size(maxX - minX, maxY - minY)) and iterates with x < maxX / y <
maxY, but callers treat maxX/maxY as inclusive; change the allocation to
Image(Size(maxX - minX + 1, maxY - minY + 1)) and make the loops inclusive (for
x = minX; x <= maxX; x++ and for y = minY; y <= maxY; y++), so the produced
image dimensions and pixel indexing in Minimap::saveImage match the calling
convention used by Map::saveImage and Lua callers.
Still same problem: EDIT: and it printed In C++ I added in if (thing->isItem()) {
const ItemPtr item = thing->static_self_cast<Item>();
if (m_position.x == 32784 && m_position.y == 31161 && m_position.z == 5) {
std::cout << "ORG add item: " << item->getId() << std::endl;
}
if (m_position.x == 32784 && m_position.y == 32185 && m_position.z == 5) {
std::cout << "FAKE add item: " << item->getId() << std::endl;
}
}and it only prints: so during |
|
@kokekanon uint16_t getBlockIndex(const Position& pos) { return ((pos.y / BLOCK_SIZE) * (65536 / BLOCK_SIZE)) + (pos.x / BLOCK_SIZE); }with: uint32_t getBlockIndex(const Position& pos) { return ((pos.y / BLOCK_SIZE) * (65536 / BLOCK_SIZE)) + (pos.x / BLOCK_SIZE); }some results may return values over With proper tiles loading OTCR uses 25 GB of RAM while generating images. EDIT: EDIT 2: |
2 participants
The author of this is @gesior , https://github.com/gesior/otclient_mapgen
I attempted to use his repository for a specific purpose, but I found it difficult to compile vc14.
It was easier for me to transfer the changes to OTCR in order to simplify the compilation process.
The code supports the Oskar zone system...
Warning
and yes compatibility was done with ia . I have little time and I'm noob in cpp
To compile, you need to activate FRAMEWORK_EDITOR.
Add your
map.otbmanditem.otbto this folder./things/%d/and execute this command in terminal (press CTRL+T)
If you want to generate a map for the web, run this command.
Otherwise, just run this.
Before this point, if you have a custom .dat file, you must add the necessary
g_game.enableFeaturenow.1 ) g_map.saveImage - saves view like in OTC as PNG image.
2 ) g_minimap.saveImage - saves minimap as PNG image.
3 ) MAP ZOOM LEVELS FOR WEBSITE
If you need it to be faster, disable unnecessary things in the client.
I have no intention of merging it, I'm just sharing it in case anyone needs it.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor