feat: add satellite map functionality in Cyclopedia#1662
Open
zoelner wants to merge 8 commits intoopentibiabr:mainfrom
Open
feat: add satellite map functionality in Cyclopedia#1662zoelner wants to merge 8 commits intoopentibiabr:mainfrom
zoelner wants to merge 8 commits intoopentibiabr:mainfrom
Conversation
* feat: Implement satellite map functionality in Cyclopedia - Added SatelliteMap class to manage satellite view image chunks. - Integrated satellite map loading and rendering in the Cyclopedia map. - Enhanced UI to support Surface and Map view toggling based on player position. - Updated map UI layout to accommodate new view options. - Improved minimap functionality to handle satellite mode. - Added Lua bindings for satellite map functions. - Refactored existing map loading logic to ensure compatibility with new features. * feat: Enhance minimap functionality with static minimap support and satellite chunk handling * feat: Remove obsolete satellite chunk formula documentation * feat: preserve initial zoom for Cyclopedia minimap and handle alternatives safely * feat: optimize LOD selection and repaint on satellite mode changes in minimap * feat: add CLAUDE.md for project context and guidelines reference
📝 WalkthroughWalkthroughAdds a SatelliteMap subsystem (C++) with on-disk chunk indexing and lazy texture loading, exposes it to Lua, and integrates floor-aware satellite/static-minimap rendering plus new view UI and floor controls in the Cyclopedia map tab. Changes
Sequence Diagram(s)sequenceDiagram
participant CycloLua as Cyclopedia (Lua)
participant SatMap as SatelliteMap (C++)
participant Tex as Texture Loader
participant Renderer as Renderer
participant Engine as Game Engine
CycloLua->>SatMap: loadDirectory(dir) / loadFloors(dir,min,max)
SatMap->>SatMap: scan directory, build per-floor indices
Engine->>CycloLua: onUpdateCameraPosition()
CycloLua->>CycloLua: detect floor/view change
CycloLua->>SatMap: ensureViewFloorsLoaded(view/floor)
SatMap->>Tex: lazily load required chunk textures
CycloLua->>SatMap: draw(screenRect,cameraPos,scale,color,sepOpacity)
SatMap->>SatMap: pick LODs, compute visible tiles
SatMap->>Tex: loadChunkTexture(path)
Tex->>Tex: decompressLzma() & parseBmp()
Tex-->>SatMap: texture ready / error
SatMap->>Renderer: composite draw calls
Renderer-->>Engine: render frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@modules/game_cyclopedia/tab/map/map.lua`:
- Around line 87-92: In loadMap(), after priming virtualFloor from the player
position (the block that sets virtualFloor = pos.z), call refreshVirtualFloors()
to initialize layersMark and automapLayers immediately so the UI reflects the
correct floor; also apply the same fix to the other initialization site that
sets virtualFloor (the second occurrence) so both code paths call
refreshVirtualFloors() after assigning virtualFloor.
- Around line 169-174: The filter lookup is ambiguous because two CheckBoxButton
elements in map.otui share the same id "12", so change the UI ids to be unique
(e.g., prefix with "filter_" or "flag_") and update the code that finds the
checkbox (the markList:getChildById(tostring(flag.icon)) call in map.lua) to use
the new composite id (e.g., markList:getChildById("filter_" .. flag.icon)) or
alternatively change the lookup to iterate markList:getChildren() and match a
custom attribute on the CheckBoxButton to flag.icon; update either the map.otui
CheckBoxButton ids or the map.lua lookup consistently so flags always resolve to
the correct checkbox.
In `@src/client/satellitemap.cpp`:
- Around line 48-52: When loadFloors(const std::string& dir, ...) is called with
a different directory you must clear the previously indexed chunk state so old
ChunkKey entries don't reference stale files: detect a directory change (either
by adding a stored lastDir member or by making buildFileCache return a boolean
indicating it rebuilt for a new dir) and when a change is detected, clear
m_chunks and m_mmChunks and reset any related indexes or caches before
proceeding to repopulate from the new directory; update loadFloors to perform
this clear using the identified symbols (loadFloors, buildFileCache, m_chunks,
m_mmChunks, ChunkKey).
- Around line 125-135: The draw routine is currently hardcoding an opaque black
background and unmodulated texture draws, which ignores the caller-supplied
color; update SatelliteMap::draw to pass the provided color into the background
fill and to modulate all texture draw calls with that color (preserving alpha).
Concretely, replace the g_drawPool.addFilledRect(screenRect, Color::black) call
with g_drawPool.addFilledRect(screenRect, color) and change any
g_drawPool.addTexturedRect / addTexturedRectPart (and similar texture-queueing
calls used in the other render paths noted around lines 185, 193-201, 243) to
include the color parameter so the minimap tint/alpha from the widget is
applied. Ensure you use the same const Color& color parameter from
SatelliteMap::draw in each place.
- Around line 144-148: The loop currently iterates over {coarserLod, bestLod}
which yields duplicate LODs when bestLod == coarserLod (e.g., bestLod==64),
causing double drawing; update the LOD iteration in the nested loop that uses
bestLod and coarserLod (the for (int floor = SURFACE_FLOOR; floor >=
targetFloor; --floor) { for (const int lod : { coarserLod, bestLod }) { ... } }
pattern) so it only processes each distinct LOD once—either branch to a
single-pass when coarserLod == bestLod or conditionally iterate both LODs only
when they differ; apply the same fix to the other occurrence that mirrors this
pattern (the second loop mentioned around the 207-209 region).
- Around line 177-182: The code treats ChunkInfo::texture==nullptr as "not
loaded yet", so failed loads are retried every repaint; modify the chunk load
logic to cache failures by adding a failure marker on ChunkInfo (e.g., bool
loadFailed or an enum/state) and only attempt loadChunkTexture(info.path) if
loadFailed==false and texture==nullptr; when loadChunkTexture returns null, set
info.loadFailed=true and record/log the error once (instead of leaving texture
null), and update the same logic for both places that call loadChunkTexture (the
block using m_chunks.at(key) around ChunkInfo::texture and the other occurrence
at lines ~236-240) so subsequent repaints skip retries.
- Around line 354-383: The decompression loop can append arbitrarily large
decompressed data from a malformed .bmp.lzma; before inserting buf[0..got) into
output enforce a hard cap on chunk size and/or total output size: after calling
lzma_code(&strm, action) compute got = buf.size() - strm.avail_out then clamp
got to a safe MAX_CHUNK (and check output.size() + got against a MAX_TOTAL_SIZE)
and only insert the clamped number of bytes into output; use the existing
symbols strm, buf, output, lzma_code, LZMA_STREAM_END and ensure parseBmp() will
later still validate overall image size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc0558db-3b36-43b9-a386-7b2d31a3483c
📒 Files selected for processing (10)
modules/game_cyclopedia/tab/map/map.luamodules/game_cyclopedia/tab/map/map.otuimodules/gamelib/ui/uiminimap.luasrc/CMakeLists.txtsrc/client/luafunctions.cppsrc/client/satellitemap.cppsrc/client/satellitemap.hsrc/client/uiminimap.cppsrc/client/uiminimap.hsrc/framework/graphics/ttfloader.cpp
kokekanon
reviewed
Mar 12, 2026
Contributor
kokekanon
left a comment
kokekanon
left a comment
There was a problem hiding this comment.
https://github.com/opentibiabr/otclient/blob/main/vc18/otclient.vcxproj
you forgot to add
+ <ClCompile Include="..\src\client\satellitemap.cpp" />+ <ClInclude Include="..\src\client\satellitemap.h" />
Author
Thanks bro, I didn't even realize about the Visual Studio project, I'll add it, I'm adjusting some details and I'll upload it together. |
* feat: add floor separator opacity control to satellite map view * feat: add satellite map source files to project configuration * feat: update floor separator color and improve logging verbosity in satellite map * feat: remove unused checkbox and fix zoom method name in map UI
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/game_cyclopedia/tab/map/map.otui (1)
415-426:⚠️ Potential issue | 🔴 CriticalThese zoom buttons now call a missing Lua function.
The handlers were updated to
modules.game_cyclopedia.Cyclopedia.setZoom(...), butmodules/game_cyclopedia/tab/map/map.luastill exportsCyclopedia.setZooom. Clicking either button will fail until the Lua function is renamed or these handlers keep the old name.Suggested follow-up in
modules/game_cyclopedia/tab/map/map.luafunction Cyclopedia.setZoom(zoom) if zoom then UI.MapBase.minimap:zoomIn() else UI.MapBase.minimap:zoomOut() end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_cyclopedia/tab/map/map.otui` around lines 415 - 426, The OTUI handlers call modules.game_cyclopedia.Cyclopedia.setZoom but the Lua export is misspelled as Cyclopedia.setZooom; rename the Lua function in modules/game_cyclopedia/tab/map/map.lua from setZooom to setZoom (exact identifier Cyclopedia.setZoom) and ensure its signature accepts a boolean parameter (zoom) and calls UI.MapBase.minimap:zoomIn() when true and zoomOut() when false so the onClick handlers in the map.otui work without errors.
♻️ Duplicate comments (5)
src/client/satellitemap.cpp (5)
126-135:⚠️ Potential issue | 🟠 MajorHonor the caller-supplied
colorin both render paths.Both APIs accept
const Color& color, but the background and chunk draws are still hardcoded/unmodulated. That drops widget tint and alpha in satellite/static-minimap mode.Also applies to: 200-200, 210-218, 260-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 126 - 135, The draw path in SatelliteMap::draw is ignoring the caller-supplied color and uses hardcoded colors for the background and chunk rendering; update the calls that render the background (g_drawPool.addFilledRect) and the chunk drawing paths (the routines that draw m_chunks / tile layers within SatelliteMap::draw) to modulate or replace the hardcoded Color::black and Color(0xFFA54C27U) with the passed-in const Color& color (respecting its alpha) and apply floorSeparatorOpacity where appropriate so the widget tint and alpha are honored in both sea-floor and non-sea-floor branches as well as the chunk draw loops referenced in this function.
192-197:⚠️ Potential issue | 🟡 MinorCache failed chunk loads instead of retrying every repaint.
texture == nullptrcurrently means both “not loaded yet” and “load failed”. Broken visible chunks will be re-read, re-decompressed, and re-logged every frame.Also applies to: 253-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 192 - 197, Chunk loads currently use info.texture == nullptr for both "not attempted" and "failed", causing repeated expensive retries; add an explicit load state on ChunkInfo (e.g., bool loadFailed or an enum LoadState) and use it in the visible-chunk loading logic (the block that calls loadChunkTexture and the similar block around the lines 253-257) so that after a failed load you mark loadFailed=true (or state=Failed) and skip reattempts on subsequent repaints; modify logic that checks info.texture to also check the new load state and only call loadChunkTexture when the state is "not attempted" (or Unknown) and set state to Loaded/Failed based on the result from loadChunkTexture.
371-387:⚠️ Potential issue | 🟠 MajorCap decompressed output size before growing
output.This loop appends until
LZMA_STREAM_ENDwith no hard ceiling. A malformed.bmp.lzmacan force arbitrarily large allocations beforeparseBmp()rejects it.Suggested fix
std::vector<uint8_t> output; output.reserve(512 * 512 * 4 + 2048); // typical 32-bpp BMP for 512×512 + constexpr size_t MAX_DECOMPRESSED_SIZE = 512u * 512u * 4u + 4096u; @@ ret = lzma_code(&strm, action); const size_t got = buf.size() - strm.avail_out; + if (output.size() + got > MAX_DECOMPRESSED_SIZE) { + lzma_end(&strm); + g_logger.warning("SatelliteMap: decoded chunk exceeds {} bytes", MAX_DECOMPRESSED_SIZE); + return {}; + } output.insert(output.end(), buf.begin(), buf.begin() + got);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 371 - 387, The loop that calls lzma_code and appends into output (using buf, got, strm, ret) can grow unbounded for malformed .bmp.lzma files; add a hard ceiling (e.g., MAX_DECOMPRESSED_SIZE) and before inserting check if output.size() + got would exceed that ceiling, then either truncate the bytes appended to stay within the limit and break/return an error, or fail early; ensure the ceiling is enforced before calling parseBmp so parseBmp is never handed a decompressed buffer larger than the allowed MAX_DECOMPRESSED_SIZE.
145-147:⚠️ Potential issue | 🟠 MajorSkip the extra pass when
bestLod == 64.At the coarsest zoom,
{ coarserLod, bestLod }becomes{ 64, 64 }, so every visible chunk is loaded and drawn twice.Suggested fix
- const int coarserLod = (bestLod == 16) ? 32 : 64; + const int coarserLod = (bestLod == 16) ? 32 : (bestLod == 32 ? 64 : -1); for (const int lod : { coarserLod, bestLod }) { + if (lod < 0) + continue; const int indexKey = floor * 100 + lod;Also applies to: 163-163, 224-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 145 - 147, The code computes coarserLod from bestLod and ends up with coarserLod == bestLod at the coarsest zoom, causing a duplicate load/draw pass; update the logic around pickLod and coarserLod (variables bestLod and coarserLod) to avoid that: either set coarserLod to a sentinel (e.g. 0 or -1) when it would equal bestLod, or add a guard before the second pass that skips loading/drawing when coarserLod == bestLod; apply the same fix in the other occurrences referenced (the spots around the other occurrences of bestLod/coarserLod).
49-52:⚠️ Potential issue | 🟠 MajorClear indexed chunks when
loadFloors()switches directories.
buildFileCache()only resets the scan list. IfloadFloors()is called with a differentdir, the oldm_chunks/m_mmChunksentries survive, and overlappingChunkKeys keep pointing at stale files because the duplicate checks skip the new ones.Suggested fix
int SatelliteMap::loadFloors(const std::string& dir, const int floorMin, const int floorMax) { + if (!m_fileCacheDir.empty() && dir != m_fileCacheDir) + clear(); + // Build the file list exactly once per directory — reused on every subsequent call. buildFileCache(dir);Also applies to: 79-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 49 - 52, loadFloors currently calls buildFileCache(dir) but doesn't clear prior state, so when dir changes old entries in m_chunks and m_mmChunks with overlapping ChunkKey point to stale files; modify SatelliteMap::loadFloors to detect when the requested dir differs from the cached directory (or always when switching directories) and clear/reset m_chunks and m_mmChunks before calling buildFileCache (and before the duplicate checks later in the function, e.g., the block around where duplicate-check logic runs) so new files are not skipped by stale entries.
🧹 Nitpick comments (2)
src/client/uiminimap.h (1)
70-77: Make this header self-contained before usingstd::clamp.
setFloorSeparatorOpacity()is inline and callsstd::clamp, butuiminimap.hdoes not include<algorithm>. Right now this compiles only if a transitive include or the PCH happens to provide it.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 70 - 77, The header uiminimap.h uses std::clamp inside the inline method setFloorSeparatorOpacity(float) but doesn't include <algorithm>, making the header not self-contained; add a direct include for <algorithm> at the top of uiminimap.h so std::clamp is always available, then keep the implementation of setFloorSeparatorOpacity (which clamps, compares to m_floorSeparatorOpacity and calls repaint()) unchanged.modules/game_cyclopedia/tab/map/map.lua (1)
10-12: Don't duplicate the sea-floor threshold in Lua.This bakes in the default
7, while the engine-side map logic already reads the boundary fromg_gameConfig.getMapSeaFloor(). If a client overrides that config, Cyclopedia will enable the wrong view mode and floor-separator behavior. Please exposegetMapSeaFloor()to Lua and deriveSEA_FLOORfrom config instead of hardcoding it here.🤖 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 10 - 12, Replace the hardcoded SEA_FLOOR constant with a runtime value obtained from the engine config: call g_gameConfig.getMapSeaFloor() instead of using the literal 7 (i.e. remove/replace the local SEA_FLOOR = 7 and set SEA_FLOOR = g_gameConfig.getMapSeaFloor()). If that engine getter is not yet bound to Lua, add a Lua binding for getMapSeaFloor on the g_gameConfig interface so Lua can call it; keep all existing uses of SEA_FLOOR (or its new variable) unchanged so view-mode and floor-separator behavior derives from the engine config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/satellitemap.cpp`:
- Around line 435-443: The code currently calls std::abs on the int32_t variable
height and negates it, which can overflow for INT32_MIN; change the logic in the
block that reads height (uses readI32 and variable height, and determines
topDown) to compute the magnitude in a wider type first (e.g. int64_t mag =
(height < 0 ? -(int64_t)height : (int64_t)height)); use mag for the bounds check
(ensure mag > 0 and mag <= 4096) and for validating image size, retain topDown =
(height < 0), and only assign back to height after confirming mag fits in
int32_t (or reject if mag > INT32_MAX) to avoid undefined behavior from std::abs
or negation on INT32_MIN.
---
Outside diff comments:
In `@modules/game_cyclopedia/tab/map/map.otui`:
- Around line 415-426: The OTUI handlers call
modules.game_cyclopedia.Cyclopedia.setZoom but the Lua export is misspelled as
Cyclopedia.setZooom; rename the Lua function in
modules/game_cyclopedia/tab/map/map.lua from setZooom to setZoom (exact
identifier Cyclopedia.setZoom) and ensure its signature accepts a boolean
parameter (zoom) and calls UI.MapBase.minimap:zoomIn() when true and zoomOut()
when false so the onClick handlers in the map.otui work without errors.
---
Duplicate comments:
In `@src/client/satellitemap.cpp`:
- Around line 126-135: The draw path in SatelliteMap::draw is ignoring the
caller-supplied color and uses hardcoded colors for the background and chunk
rendering; update the calls that render the background
(g_drawPool.addFilledRect) and the chunk drawing paths (the routines that draw
m_chunks / tile layers within SatelliteMap::draw) to modulate or replace the
hardcoded Color::black and Color(0xFFA54C27U) with the passed-in const Color&
color (respecting its alpha) and apply floorSeparatorOpacity where appropriate
so the widget tint and alpha are honored in both sea-floor and non-sea-floor
branches as well as the chunk draw loops referenced in this function.
- Around line 192-197: Chunk loads currently use info.texture == nullptr for
both "not attempted" and "failed", causing repeated expensive retries; add an
explicit load state on ChunkInfo (e.g., bool loadFailed or an enum LoadState)
and use it in the visible-chunk loading logic (the block that calls
loadChunkTexture and the similar block around the lines 253-257) so that after a
failed load you mark loadFailed=true (or state=Failed) and skip reattempts on
subsequent repaints; modify logic that checks info.texture to also check the new
load state and only call loadChunkTexture when the state is "not attempted" (or
Unknown) and set state to Loaded/Failed based on the result from
loadChunkTexture.
- Around line 371-387: The loop that calls lzma_code and appends into output
(using buf, got, strm, ret) can grow unbounded for malformed .bmp.lzma files;
add a hard ceiling (e.g., MAX_DECOMPRESSED_SIZE) and before inserting check if
output.size() + got would exceed that ceiling, then either truncate the bytes
appended to stay within the limit and break/return an error, or fail early;
ensure the ceiling is enforced before calling parseBmp so parseBmp is never
handed a decompressed buffer larger than the allowed MAX_DECOMPRESSED_SIZE.
- Around line 145-147: The code computes coarserLod from bestLod and ends up
with coarserLod == bestLod at the coarsest zoom, causing a duplicate load/draw
pass; update the logic around pickLod and coarserLod (variables bestLod and
coarserLod) to avoid that: either set coarserLod to a sentinel (e.g. 0 or -1)
when it would equal bestLod, or add a guard before the second pass that skips
loading/drawing when coarserLod == bestLod; apply the same fix in the other
occurrences referenced (the spots around the other occurrences of
bestLod/coarserLod).
- Around line 49-52: loadFloors currently calls buildFileCache(dir) but doesn't
clear prior state, so when dir changes old entries in m_chunks and m_mmChunks
with overlapping ChunkKey point to stale files; modify SatelliteMap::loadFloors
to detect when the requested dir differs from the cached directory (or always
when switching directories) and clear/reset m_chunks and m_mmChunks before
calling buildFileCache (and before the duplicate checks later in the function,
e.g., the block around where duplicate-check logic runs) so new files are not
skipped by stale entries.
---
Nitpick comments:
In `@modules/game_cyclopedia/tab/map/map.lua`:
- Around line 10-12: Replace the hardcoded SEA_FLOOR constant with a runtime
value obtained from the engine config: call g_gameConfig.getMapSeaFloor()
instead of using the literal 7 (i.e. remove/replace the local SEA_FLOOR = 7 and
set SEA_FLOOR = g_gameConfig.getMapSeaFloor()). If that engine getter is not yet
bound to Lua, add a Lua binding for getMapSeaFloor on the g_gameConfig interface
so Lua can call it; keep all existing uses of SEA_FLOOR (or its new variable)
unchanged so view-mode and floor-separator behavior derives from the engine
config.
In `@src/client/uiminimap.h`:
- Around line 70-77: The header uiminimap.h uses std::clamp inside the inline
method setFloorSeparatorOpacity(float) but doesn't include <algorithm>, making
the header not self-contained; add a direct include for <algorithm> at the top
of uiminimap.h so std::clamp is always available, then keep the implementation
of setFloorSeparatorOpacity (which clamps, compares to m_floorSeparatorOpacity
and calls repaint()) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fb46e31-ba0d-4519-a402-0d0167fd6b4a
📒 Files selected for processing (9)
modules/game_cyclopedia/tab/map/map.luamodules/game_cyclopedia/tab/map/map.otuisrc/client/luafunctions.cppsrc/client/satellitemap.cppsrc/client/satellitemap.hsrc/client/uimap.cppsrc/client/uiminimap.cppsrc/client/uiminimap.hvc18/otclient.vcxproj
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/satellitemap.cpp (1)
142-143:⚠️ Potential issue | 🟠 MajorHonor the widget
colorin both satellite render paths.
UIMinimap::drawSelf()passesm_colorinto these methods, but both paths still draw an opaque background and unmodulated textures here, so tint/alpha disappears when the widget switches fromg_minimap.draw(...)to satellite/static-minimap rendering.Also applies to: 216-216, 234-235, 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 142 - 143, The background and texture draws in the satellite/static-minimap paths ignore the widget tint/alpha (m_color) passed from UIMinimap::drawSelf()/g_minimap.draw(), so update the calls (e.g., g_drawPool.addFilledRect and the texture draw calls) to modulate the existing colors (sea floor Color(0xFFA54C27U) or Color::black) and any opaque texture colors by the passed-in m_color (or the function parameter that receives it) so the final color respects tint and alpha; find usages in the satellite rendering paths referenced around g_drawPool.addFilledRect and ensure the same multiplication/modulation is applied for all equivalent spots (also where textures are drawn) so both satellite and static-minimap render paths honor the widget color.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/satellitemap.cpp`:
- Around line 386-389: The liblzma decoder is being initialized with
lzma_alone_decoder(&strm, UINT64_MAX) which disables liblzma's memory cap;
change this to pass a bounded memlimit (e.g. size_t memlimit = 64ull * 1024 *
1024) instead of UINT64_MAX and store the lzma_alone_decoder return value in a
variable; then update the subsequent error handling (the check that currently
logs and returns) to explicitly detect LZMA_MEMLIMIT_ERROR and treat it as a
memory-limit failure (log a clear message and return) while still handling other
lzma_ret values as before; reference lzma_stream strm, lzma_alone_decoder,
MAX_DECOMPRESSED, and check for LZMA_MEMLIMIT_ERROR in the decompression error
branch.
---
Duplicate comments:
In `@src/client/satellitemap.cpp`:
- Around line 142-143: The background and texture draws in the
satellite/static-minimap paths ignore the widget tint/alpha (m_color) passed
from UIMinimap::drawSelf()/g_minimap.draw(), so update the calls (e.g.,
g_drawPool.addFilledRect and the texture draw calls) to modulate the existing
colors (sea floor Color(0xFFA54C27U) or Color::black) and any opaque texture
colors by the passed-in m_color (or the function parameter that receives it) so
the final color respects tint and alpha; find usages in the satellite rendering
paths referenced around g_drawPool.addFilledRect and ensure the same
multiplication/modulation is applied for all equivalent spots (also where
textures are drawn) so both satellite and static-minimap render paths honor the
widget color.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea9a6b14-6b54-4601-922f-ceed7aaf9bbf
📒 Files selected for processing (2)
src/client/satellitemap.cppsrc/client/satellitemap.h
…A_MEMLIMIT_ERROR Co-authored-by: zoelner <[email protected]>
fix: unbounded memory limit in LZMA decoder initialization
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/client/satellitemap.cpp (2)
386-394:⚠️ Potential issue | 🟠 MajorMove the
LZMA_MEMLIMIT_ERRORhandling to the decode result path.The decoder hasn't seen the stream header yet at Line 388, so the memory-limit failure won't be reported there for oversized-dictionary inputs. As written, those cases still fall through to the generic error at Line 437 instead of the clearer memlimit log/return branch.
In liblzma, can `lzma_alone_decoder()` itself return `LZMA_MEMLIMIT_ERROR`, or is `LZMA_MEMLIMIT_ERROR` reported by subsequent `lzma_code()` calls after the `.lzma` header has been read?Also applies to: 437-439
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 386 - 394, The LZMA_MEMLIMIT_ERROR check belongs in the decode/result handling, not the init path: remove or simplify the LZMA_MEMLIMIT_ERROR branch after lzma_alone_decoder(&strm, ...) (keep a generic init-failure log for non-OK initRet), and add handling where lzma_code(...) (or the decode loop that consumes the .lzma header) returns LZMA_MEMLIMIT_ERROR — log "SatelliteMap: LZMA decoder init failed: memory limit exceeded" (or similar) and return {} there; ensure you reference the same lzma_stream strm and use lzma_code's return value to detect LZMA_MEMLIMIT_ERROR so oversized-dictionary inputs give the clearer memlimit path.
142-143:⚠️ Potential issue | 🟠 MajorHonor the caller-supplied
colorin both render paths.
draw()/drawStaticMinimap()still hardcode the background fill and queue textures unmodulated, so widget tint/alpha disappears in satellite mode. Please threadcolorthrough the background and textured draws here as well.Also applies to: 216-216, 234-235, 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 142 - 143, The background fill is hardcoded to specific Colors and ignores the caller-supplied color/tint; update the satellite rendering code paths (draw() / drawStaticMinimap()) to use the incoming color/tint when queuing background and texture draws: replace the hardcoded Color(0xFFA54C27U) / Color::black in the g_drawPool.addFilledRect call with the caller-supplied color (respecting the sea-floor conditional using cameraPos.z == g_gameConfig.getMapSeaFloor()), and ensure all textured queue calls (the functions that enqueue sprite/texture draws in this file) multiply/modulate their vertex colors by that same color/alpha so widget tint/alpha is preserved in satellite mode (apply this change to the other occurrences referenced around the g_drawPool.addFilledRect usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/satellitemap.cpp`:
- Line 216: The code currently assumes chunk bitmaps are 512x512 when calling
g_drawPool.addTexturedRect(dest, info.texture, Rect(0,0,512,512)) and may create
oversized textures; before creating or assigning info.texture (and before any
subsequent g_drawPool.addTexturedRect calls), validate the decoded image's width
and height are exactly 512 and fail fast (log error/return false/skip) if not,
releasing any decoded bitmap resources; apply the same exact size check and
early-fail logic wherever chunk textures are created/used (the code that
constructs info.texture and the callers that use Rect(0,0,512,512), e.g., the
other chunk-load sites referenced) so no non-512 BMP ever becomes a GPU texture.
---
Duplicate comments:
In `@src/client/satellitemap.cpp`:
- Around line 386-394: The LZMA_MEMLIMIT_ERROR check belongs in the
decode/result handling, not the init path: remove or simplify the
LZMA_MEMLIMIT_ERROR branch after lzma_alone_decoder(&strm, ...) (keep a generic
init-failure log for non-OK initRet), and add handling where lzma_code(...) (or
the decode loop that consumes the .lzma header) returns LZMA_MEMLIMIT_ERROR —
log "SatelliteMap: LZMA decoder init failed: memory limit exceeded" (or similar)
and return {} there; ensure you reference the same lzma_stream strm and use
lzma_code's return value to detect LZMA_MEMLIMIT_ERROR so oversized-dictionary
inputs give the clearer memlimit path.
- Around line 142-143: The background fill is hardcoded to specific Colors and
ignores the caller-supplied color/tint; update the satellite rendering code
paths (draw() / drawStaticMinimap()) to use the incoming color/tint when queuing
background and texture draws: replace the hardcoded Color(0xFFA54C27U) /
Color::black in the g_drawPool.addFilledRect call with the caller-supplied color
(respecting the sea-floor conditional using cameraPos.z ==
g_gameConfig.getMapSeaFloor()), and ensure all textured queue calls (the
functions that enqueue sprite/texture draws in this file) multiply/modulate
their vertex colors by that same color/alpha so widget tint/alpha is preserved
in satellite mode (apply this change to the other occurrences referenced around
the g_drawPool.addFilledRect usage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09f229ab-2303-427e-a984-f8e5d3690568
📒 Files selected for processing (1)
src/client/satellitemap.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/satellitemap.cpp (1)
151-151: Consider extractingSURFACE_FLOORas a class-level or file-level constant.
SURFACE_FLOOR = 7is defined twice as astatic constexprlocal variable (lines 151 and 305). Extracting it to a single definition at file scope or as a class constant would reduce duplication.💡 Suggested refactor
static constexpr std::string_view PREFIX_SATELLITE = "satellite"; static constexpr std::string_view PREFIX_MINIMAP = "minimap"; +static constexpr int SURFACE_FLOOR = 7;Then remove the local definitions inside
draw()andhasChunksForView().Also applies to: 305-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` at line 151, SURFACE_FLOOR is defined twice as a local static constexpr in draw() and hasChunksForView(); extract a single constant (e.g., constexpr int SURFACE_FLOOR = 7) at file scope or as a static member of the surrounding class, replace the local definitions with references to that single symbol, and remove the duplicated local declarations in draw() and hasChunksForView(); ensure all uses still compile by updating any scope qualifiers if you make it a class constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/satellitemap.cpp`:
- Line 151: SURFACE_FLOOR is defined twice as a local static constexpr in draw()
and hasChunksForView(); extract a single constant (e.g., constexpr int
SURFACE_FLOOR = 7) at file scope or as a static member of the surrounding class,
replace the local definitions with references to that single symbol, and remove
the duplicated local declarations in draw() and hasChunksForView(); ensure all
uses still compile by updating any scope qualifiers if you make it a class
constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 399f54d7-8d80-4b43-a2ed-f8422b166cc0
📒 Files selected for processing (1)
src/client/satellitemap.cpp
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/satellitemap.cpp (1)
135-145:⚠️ Potential issue | 🟠 Major
coloris still ignored in both render paths.Line 135/226 takes
const Color& color, but Line 144/234 uses hardcoded fill colors and Line 216/284 draw textures unmodulated. Satellite/static-minimap tint/alpha from callers won’t apply.#!/bin/bash # Verify draw API supports color modulation and confirm current call sites skip it. # Expected: addTexturedRect/addTexturedRectPart overloads with Color exist, # and SatelliteMap draw paths currently omit Color argument. fd drawpoolmanager.h rg -nP -C2 --type=cpp '\baddTexturedRect(Part)?\s*\(' rg -nP -C3 --type=cpp 'SatelliteMap::draw\s*\(|SatelliteMap::drawStaticMinimap\s*\(' src/client/satellitemap.cpp rg -nP -C3 --type=cpp '\bg_satelliteMap\b|\bcolor\b' src/client/uiminimap.cpp src/client/uiminimap.h src/client/luafunctions.cppAlso applies to: 216-216, 226-235, 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` around lines 135 - 145, The draw functions (SatelliteMap::draw and SatelliteMap::drawStaticMinimap) currently ignore the passed Color& color: replace hardcoded fill/texture draws so they use the provided color (and floorSeparatorOpacity where relevant) when calling g_drawPool.addFilledRect and the texture blit functions; specifically, update the g_drawPool.addFilledRect call that uses Color(0xFFA54C27U)/Color::black to modulate with color (e.g., multiply or blend with color and apply floorSeparatorOpacity), and change g_drawPool.addTexturedRect/addTexturedRectPart calls in SatelliteMap::draw (and the analogous calls in drawStaticMinimap) to the overloads that accept a Color so the passed-in color (with alpha applied) is used to tint both background and textures; ensure you preserve existing conditional logic for sea floor vs others while threading color/floorSeparatorOpacity through the draw calls.
🧹 Nitpick comments (1)
src/client/satellitemap.cpp (1)
429-429: Improve decompression-limit log context.Line 429 logs a literal
"chunk", which makes production triage harder. Consider threading the source path intodecompressLzma(...)(or include a chunk key) so limit-hit logs identify the offending asset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/satellitemap.cpp` at line 429, The warning uses a literal "chunk" which hides which asset triggered the decompression-limit; modify decompressLzma(...) to accept an identifying string (e.g. sourcePath or chunkKey) and propagate that identifier from its call sites, then change the g_logger.warning call in decompressLzma to include that identifier instead of the literal ("chunk") so the log becomes e.g. "SatelliteMap: decompressed size exceeds limit, aborting '{}'" with the passed identifier; update all callers of decompressLzma and any related signatures accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/satellitemap.cpp`:
- Around line 135-145: The draw functions (SatelliteMap::draw and
SatelliteMap::drawStaticMinimap) currently ignore the passed Color& color:
replace hardcoded fill/texture draws so they use the provided color (and
floorSeparatorOpacity where relevant) when calling g_drawPool.addFilledRect and
the texture blit functions; specifically, update the g_drawPool.addFilledRect
call that uses Color(0xFFA54C27U)/Color::black to modulate with color (e.g.,
multiply or blend with color and apply floorSeparatorOpacity), and change
g_drawPool.addTexturedRect/addTexturedRectPart calls in SatelliteMap::draw (and
the analogous calls in drawStaticMinimap) to the overloads that accept a Color
so the passed-in color (with alpha applied) is used to tint both background and
textures; ensure you preserve existing conditional logic for sea floor vs others
while threading color/floorSeparatorOpacity through the draw calls.
---
Nitpick comments:
In `@src/client/satellitemap.cpp`:
- Line 429: The warning uses a literal "chunk" which hides which asset triggered
the decompression-limit; modify decompressLzma(...) to accept an identifying
string (e.g. sourcePath or chunkKey) and propagate that identifier from its call
sites, then change the g_logger.warning call in decompressLzma to include that
identifier instead of the literal ("chunk") so the log becomes e.g.
"SatelliteMap: decompressed size exceeds limit, aborting '{}'" with the passed
identifier; update all callers of decompressLzma and any related signatures
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87506a9a-b327-45a9-9cd0-40dab5724a8d
📒 Files selected for processing (1)
src/client/satellitemap.cpp
Contributor
|
@kokekanon |
Author
|
@majestyotbr @kokekanon @dudantas Is there any impediment to merging this branch, or something blocking it that was not mentioned? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preview:
minimap-surface-view.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Refactor