refactor(rendering): SRP & DOD refinements (Part 1) - #976
Open
pubgkreiss-oss wants to merge 56 commits into
Open
refactor(rendering): SRP & DOD refinements (Part 1)#976pubgkreiss-oss wants to merge 56 commits into
pubgkreiss-oss wants to merge 56 commits into
Conversation
1.1 - Make RenderView immutable per frame:
- Extract per-floor coordinate mutations into FloorViewParams struct
- DrawMap() loop no longer mutates view.start_x/y, end_x/y
- FloorViewParams computed fresh per floor iteration
1.2 - Introduce DrawContext struct:
- Bundle SpriteBatch&, PrimitiveRenderer&, ViewState&, DrawingOptions&, LightBuffer&
- Adopted by MapLayerDrawer, ShadeDrawer, GridDrawer, LiveCursorDrawer,
BrushOverlayDrawer, DragShadowDrawer, TileRenderer
1.3 - Split RenderView into data + GL:
- Rename RenderView -> ViewState (pure data, no GL calls)
- Extract GL side effects into GLViewport::Apply() / GLViewport::Clear()
- Add ViewState::ComputeProjection() (pure math)
- Remove MapCanvas* dependency from view struct
- Inline old Setup() logic into MapDrawer::SetupVars()
No behavior changes. All rendering output identical.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The project uses vcpkg (vcpkg.json) for dependencies like glad, glm, spdlog, and tomlplusplus which aren't available as system apt packages. Switch CI to use lukka/run-vcpkg with the vcpkg toolchain file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Missed call site during Task 1.2 refactor. Now uses DrawContext instead of individual SpriteBatch/ViewState/DrawingOptions/LightBuffer* params. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract PostProcessPipeline from MapDrawer (Task 2.3): - New PostProcessPipeline class owns FBO, texture, VAO/VBO/EBO and shader quad - MapDrawer delegates via Begin()/End() pattern, removing 8 members and 3 methods Split GraphicManager into 4 sub-objects with facade pattern (Task 2.1): - SpriteDatabase: owns sprite/image/editor vectors and resident sets - AtlasLifecycle: owns atlas manager with lazy initialization - SpriteLoaderState: owns loader flags, format info, sprite archive - TextureGC: owns garbage collector, animation timer, cached time - GraphicManager becomes thin facade, all 98+ g_gui.gfx call sites unchanged - Friend classes (GraphicsAssembler, Image, NormalImage, TemplateImage, SpritePreloader) updated to use db()/atlas()/loader()/gc() accessors Split GameSprite into Data + Rendering (Task 2.2): - SpriteMetadata: header-only struct documenting sprite data fields - SpriteDecompression: free functions for RLE decompression and colorization - SpriteIconRenderer: wxDC rendering (DrawTo/getDC/unloadDC) extracted - GameSprite delegates DrawTo to icon_renderer_, static methods to namespace - Existing call sites preserved via thin wrappers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The SpriteDatabase destructor needs Image to be a complete type since it destroys std::vector<std::unique_ptr<Image>>. Move special members out-of-line to sprite_database.cpp where Image is fully defined. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Cache build/vcpkg_installed to skip rebuilding deps on every run - Switch from Make to Ninja for faster parallel compilation - Add ninja-build to system dependencies Should cut build time from ~45min to ~15-20min on cache hit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove all friend declarations from MapDrawer (3) and MapCanvas (7), replacing friend access with explicit parameters. Eliminate three singletons (PostProcessManager, SharedGeometry, SpritePreloader) by transferring ownership to natural lifecycle owners. Decouple DrawingOptions from wx globals by replacing wxColor with DrawColor and Update() with a FromSettings() factory. Clean up TileRenderer with a named TileRenderDeps struct and extract FillItemTooltipData to a reusable TooltipDataExtractor namespace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove dependency on post_process_manager.h from drawing_options.cpp by using the "None" string literal directly instead of the ShaderNames::NONE constant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
map_drawer.cpp calls methods on SelectionController and DrawingController through canvas pointer, requiring full type definitions rather than just forward declarations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split monolithic rendering classes into focused components: Task 4.3 - Split LightDrawer into three classes: - LightFBO: owns framebuffer + texture with grow-only resize - LightShader: owns shader program, VAO/VBO, and SSBO - LightDrawer: thin orchestrator coordinating both Task 4.2 - Split TooltipDrawer into three classes: - TooltipCollector: header-only pool for per-frame tooltip data (no NanoVG dependency) - TooltipRenderer: NanoVG rendering of tooltip cards - NVGImageCache: shared sprite image cache with context change detection - TooltipData: extracted pure data types (no rendering dependency) Task 4.1 - Type-safe resident image tracking: - Replace vector<void*> with vector<Image*> in SpriteDatabase - Remove static_cast<Image*> from TextureGarbageCollector - Remove friend class TooltipDrawer from GameSprite Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix GraphicManager member initialization order UB (client_version reference was bound to loader_.client_version before loader_ was constructed due to C++ declaration-order initialization rules) - Fix SpriteDatabase::resize dangling residency pointers on shrink - Fix SpriteIconRenderer getDC assert crash with 64x64 sprites - Fix RenderKeyHash missing size/lookMountLegs/lookMountFeet fields - Fix tooltip destination sentinel using has_destination bool instead of destination.x > 0 (x=0 is a valid map position) - Fix tooltip_renderer missing nvgRect before nvgFill/nvgStroke in container grid slot background (invisible slots) - Fix tooltip_renderer prepareFields string_view invalidation from storage_ reallocation via two-phase approach - Add LightFBO framebuffer completeness check after EnsureSize resize - Add SpriteLoaderState synchronization contract documentation - Remove unused wx/colour.h include from brush_overlay_drawer.h - Remove dead SelectionDrawer allocation (created but never rendered) - Simplify light_drawer redundant identity translate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Complete implementation of 14 rendering architecture improvements: Wave A (QW-3/4/7/8): Remove duplicate include, dead PreloadItem code, shared_ptr→unique_ptr for LightDrawer, add [[nodiscard]] annotations. Wave B (QW-2/5/6): Extract magic client IDs to SpecialClientId namespace, return TileScreenPos from IsTileVisible, convert ComputeProjection to free function. Wave C (QW-1, MR-1, MR-8): Split DrawingOptions into RenderSettings (persistent) + FrameOptions (transient), introduce FrameAccumulators for per-frame overlay data, fix PostProcessPipeline two-phase init. Wave D (MR-2/3/4/5/6/7): Move hook/door accumulation out of ItemDrawer, separate tooltip accumulation from TileRenderer, split monolithic BrushOverlayDrawer::draw() into 7 focused methods, extract g_settings from BrushOverlayDrawer into RenderSettings, introduce ISpriteResolver interface to decouple from GraphicManager singleton, introduce ViewSnapshot to eliminate MapDrawer→MapCanvas back-reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename local 'frame' to 'anim_frame' in ItemDrawer::BlitItem() to avoid conflict with the FrameOptions& reference also named 'frame' in the same scope. Initialize rtype to 0 to silence -Wmaybe-uninitialized. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace all g_gui.gfx.ensureAtlasManager()/getAtlasManager() calls in drawer files with a const AtlasManager& passed through DrawContext. Extract g_gui brush state (IsDrawingMode, GetCurrentBrush, GetBrushShape, GetBrushSize) into explicit parameters for brush_overlay_drawer and preview_drawer. SpriteDrawer receives atlas via SetAtlas() setter matching existing SetSpriteResolver() pattern. All drawer files under rendering/drawers/ are now free of g_gui.gfx references. Only map_drawer.cpp (single frame entry point) and minimap_renderer.cpp (out of scope) retain g_gui.gfx usage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move 20+ public data fields (floor, zoom, cursor_x/y, dragging, boundbox_selection, screendragging, last_click_map_x/y/z, last_mmb_click_x/y, keyCode, etc.) from public to private in MapCanvas. Add inline getter/setter accessors for fields accessed by the 6 controller files (~67 access sites updated). Remove dead code: BLOCK_SIZE enum, getFillIndex(), static processed[] array (BrushUtility has its own copy). Remove dead fields: view_scroll_x, view_scroll_y, current_house_id (unused MapCanvas members superseded by ViewSnapshot and FrameOptions). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace direct editor->live_manager.GetClient()->queryNode() call in MapLayerDrawer with a NodeRequestFn callback injected via constructor. The callback is provided by MapDrawer, keeping the network I/O at the orchestration layer rather than inside the render loop. Remove live/live_client.h include from map_layer_drawer.cpp as the direct dependency is eliminated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…umulators Convert LightBuffer from AoS (struct Light with 4 fields) to SoA (4 parallel vectors) for cache-friendly iteration in the GPU upload path. Add reserve() methods to both LightBuffer and FrameAccumulators, called from MapDrawer constructor with typical frame-size capacities. Update LightDrawer to iterate parallel arrays. Update LightCalculator signature to match new layout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Group MapDrawer's 14 unique_ptr drawer members into three logical structs: - EntityDrawers (sprite, item, creature, marker) - OverlayDrawers (grid, hook_indicator, door_indicator, preview, shade, brush_overlay, creature_name) - CursorDrawers (live, brush, drag_shadow) Orchestrators (FloorDrawer, MapLayerDrawer, TileRenderer) remain top-level as they coordinate multiple drawer groups. Remove duplicate forward declarations of HookIndicatorDrawer and DoorIndicatorDrawer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split TileRenderer::DrawTile() into two phases: - PlanTile(): data gathering — reads tile data, computes colors, accumulates tooltips/hooks/doors/lights/creature-names into frame accumulators, and builds a vector of draw commands in a TileDrawPlan struct. - ExecutePlan(): GPU submission — iterates the plan's commands and issues BlitItem, BlitCreature, marker draw, and primitive draw calls. DrawTile() remains as a convenience wrapper calling both phases. New header-only file: tile_draw_plan.h containing the TileDrawPlan struct with ItemCmd, CreatureCmd, MarkerCmd, ColorSquare, ZoneBrush, HouseBorder command types. This separation enables future per-tile parallelism by isolating side-effect-free GPU submission from data-dependent gathering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cations Add a reusable TileDrawPlan member to TileRenderer, pre-reserved in the constructor for 16 items (typical tile item count). DrawTile() now clears and reuses this plan instead of allocating a new one per tile, eliminating per-tile heap allocations for the items vector. SpriteInstance is already verified 64-byte aligned via static_assert. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace inline rme::collectTileSprites() calls in PlanTile() with buffered enqueue into a SpritePreloadQueue. The queue is flushed after Draw() completes, batching all preload requests and avoiding per-sprite mutex contention on the SpritePreloader during the hot render loop. New header-only file: sprite_preload_queue.h with enqueue/processAll/clear methods. TileRenderer owns the queue and pre-reserves for 256 requests. MapDrawer calls FlushPreloadQueue() after frame submission. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace single FrameAccumulators with a double-buffered pair. During Draw(), tile-level code writes to writeAccumulators() via DrawContext. Overlay drawers (tooltips, hooks, doors, creature names) read from readAccumulators() which contains the previous frame's data. ClearFrameOverlays() swaps the indices and clears the new write buffer. This separation enables future concurrent accumulation and overlay rendering without data races, as writers and readers operate on different buffers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a mutable std::mutex to guard snapshot_ reads and writes. SetupVars() locks the mutex when writing the snapshot, and getSnapshot() locks when reading (now returns by value to avoid dangling reference under concurrent access). This is preparation for future multi-threaded rendering where the UI thread may read the snapshot while the render thread writes it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the NodeRequestFn callback in MapLayerDrawer with a thread-safe PendingNodeRequests buffer. During Draw(), MapLayerDrawer enqueues node requests into the mutex-protected buffer instead of calling network I/O directly. After frame submission, MapDrawer drains the buffer and dispatches queryNode() calls outside the render loop. This removes the last synchronous network call from the render path and makes node request batching explicit. The PendingNodeRequests class uses swap-based drain for efficient bulk retrieval. New header-only file: pending_node_requests.h Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix 'rme' has not been declared error in sprite_preload_queue.h by including sprite_preloader.h (which declares rme::collectTileSprites) instead of game/sprites.h. Apply clang-format to all files modified across Phases 1-5 to satisfy CI format checks (tabs -> 4-space indentation, brace style normalization). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The removal of graphics.h in HL-1 left sprite_drawer.cpp with only a forward declaration of GameSprite. Add the direct include so the file compiles without relying on transitive or PCH includes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The TS-4 deferred node request drain calls LiveClient::queryNode() but the file only had a forward declaration via live_socket.h. Add the direct include for the full class definition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ualSettings
QW-1: Remove SpriteDrawer::glSetColor() dead stub and unused wxColor include
QW-2: Extract cursor color fields from RenderSettings into BrushVisualSettings
- New BrushVisualSettings struct owns cursor_red/green/blue/alpha,
cursor_alt_* and use_automagic
- BrushOverlayDrawer::draw() now takes const BrushVisualSettings&
- RenderSettings no longer carries brush-UI concerns
QW-4: Inline DrawBackground() — single-line GLViewport::Clear() wrapper removed
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…member Remove the atlas_ member and SetAtlas() from SpriteDrawer. Instead, pass const AtlasManager& through function parameters to glBlitSquare() and glDrawBox(). This eliminates per-frame mutable state from SpriteDrawer and makes the atlas dependency explicit at each call site. - SpriteDrawer: remove atlas_ field, SetAtlas(); add atlas param to glBlitSquare() and glDrawBox() - ItemDrawer::BlitItem: add atlas param, thread through to 6 glBlitSquare calls - Update all BlitItem callers (tile_renderer, drag_shadow_drawer, preview_drawer, floor_drawer) to pass ctx.atlas - Update tile_renderer::ExecutePlan glBlitSquare/glDrawBox calls - Remove entities_.sprite->SetAtlas() call from MapDrawer::Draw() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tions Move the highlight_pulse sine-wave computation from MapDrawer::SetupVars() into a static FrameOptions::ComputeHighlightPulse() method. This makes the timing logic self-contained, testable, and removes inline math from the frame setup path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename ClearFrameOverlays() to BeginFrame() to make the double-buffer swap semantics explicit. The method swaps accumulator buffers and clears the write buffer, which is conceptually the start of a new frame. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace g_gui.gfx access in SpritePreloader with an injected GraphicManager* set after construction. This removes the implicit global dependency, making SpritePreloader testable and decoupled from the GUI layer. - Add setGraphicManager(GraphicManager*) setter to SpritePreloader - Replace all g_gui.gfx.* calls with gfx_->* in preload() and update() - Remove #include "ui/gui.h" from sprite_preloader.cpp - Wire up setter in GraphicManager constructor Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bundle the 13 parameters of BrushOverlayDrawer::draw() into a single BrushOverlayContext struct. This improves readability at the call site (MapDrawer::Draw) and makes it easy to add new brush overlay fields without touching every intermediate signature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract PlanGroundItem() and PlanStackedItems() from the 250-line PlanTile() monolith. Each sub-function handles one logical section: - PlanGroundItem: ground tile sprite, preload, door indicators - PlanStackedItems: item loop with house color, tooltips, preloads, door/hook indicators, creature planning PlanTile() is now a concise orchestrator (~60 lines) that delegates to focused helpers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove dead `using RenderKey`, `using RenderKeyHash`, and `using CachedDC` re-exports from GameSprite. CreatureSprite already uses `SpriteIconRenderer::RenderKey` directly. No external code references these aliases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 169 files, which is 19 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (169)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMassive refactor of the rendering subsystem and CI: introduces DrawContext/ViewState/RenderSettings/FrameOptions, plan-driven tile rendering, sprite subsystem decomposition (SpriteDatabase, SpriteLoaderState, AtlasLifecycle, TextureGC), GPU light and postprocess pipeline, tooltip/NVG caching and renderer, extensive drawer API migrations, and CI switched to vcpkg + CMake/Ninja with caching. Changes
Sequence Diagram(s)sequenceDiagram
participant MapCanvas
participant MapDrawer
participant TileRenderer
participant SpriteDatabase
participant TextureGC
participant PostProcessPipeline
participant GPU
MapCanvas->>MapDrawer: BeginFrame() / SetupVars(ViewSnapshot, BrushSnapshot)
MapDrawer->>TileRenderer: DrawTile(DrawContext, TileLocation...)
TileRenderer->>SpriteDatabase: getSprite/getImage
TileRenderer->>TextureGC: preloader().preload / gc().addSpriteToCleanup
TileRenderer->>MapDrawer: append FrameAccumulators (tooltips/hooks/doors/names)
MapDrawer->>PostProcessPipeline: Begin(view, settings)
PostProcessPipeline->>GPU: UpdateFBO / allocate textures
TileRenderer->>GPU: submit geometry/textures via SpriteBatch
PostProcessPipeline->>GPU: DrawPostProcess(quad)
GPU-->>MapDrawer: final composited frame
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural overhaul of the rendering system, breaking down monolithic classes into smaller, more specialized components. The primary goal is to enhance maintainability, testability, and clarity by applying SRP and DOD principles. This extensive refactoring ensures that the rendering pipeline is more robust and easier to extend in the future, with no behavioral changes to the existing functionality. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and extensive refactoring that significantly improves the rendering architecture by applying SRP and DOD principles. The breakdown of large classes like GraphicManager and DrawingOptions into smaller, more focused components with clear responsibilities is a huge step forward for maintainability and testability. The introduction of context objects like DrawContext and BrushOverlayContext cleans up function signatures, and the use of ViewSnapshot effectively decouples the MapDrawer from the MapCanvas. The change in LightBuffer from AoS to SoA is a great example of data-oriented design. I've found a couple of minor areas for improvement that align with the project's modern C++ and clarity goals.
Note: Security Review did not run due to the size of the PR.
Comment on lines
+3
to
29
| void LightBuffer::AddLight(int in_map_x, int in_map_y, int map_z, const SpriteLight& light) | ||
| { | ||
| if (map_z <= GROUND_LAYER) { | ||
| in_map_x -= (GROUND_LAYER - map_z); | ||
| in_map_y -= (GROUND_LAYER - map_z); | ||
| } | ||
|
|
||
| if (map_x <= 0 || map_x >= MAP_MAX_WIDTH || map_y <= 0 || map_y >= MAP_MAX_HEIGHT) { | ||
| return; | ||
| } | ||
| if (in_map_x <= 0 || in_map_x >= MAP_MAX_WIDTH || in_map_y <= 0 || in_map_y >= MAP_MAX_HEIGHT) { | ||
| return; | ||
| } | ||
|
|
||
| uint8_t intensity = std::min(light.intensity, static_cast<uint8_t>(255)); // Assumed max | ||
| uint8_t in_intensity = std::min(light.intensity, static_cast<uint8_t>(255)); | ||
|
|
||
| if (!lights.empty()) { | ||
| Light& previous = lights.back(); | ||
| if (previous.map_x == map_x && previous.map_y == map_y && previous.color == light.color) { | ||
| previous.intensity = std::max(previous.intensity, intensity); | ||
| return; | ||
| } | ||
| } | ||
| // Merge with previous light at same position and color | ||
| if (!map_x.empty()) { | ||
| size_t last = map_x.size() - 1; | ||
| if (map_x[last] == in_map_x && map_y[last] == in_map_y && color[last] == light.color) { | ||
| intensity[last] = std::max(intensity[last], in_intensity); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| lights.push_back(Light { static_cast<uint16_t>(map_x), static_cast<uint16_t>(map_y), light.color, intensity }); | ||
| map_x.push_back(static_cast<uint16_t>(in_map_x)); | ||
| map_y.push_back(static_cast<uint16_t>(in_map_y)); | ||
| color.push_back(light.color); | ||
| intensity.push_back(in_intensity); | ||
| } |
There was a problem hiding this comment.
The parameter names in_map_x and in_map_y are a bit unconventional. While they avoid shadowing the member vectors, more descriptive names like light_map_x and light_map_y would improve clarity and better communicate the purpose of the parameters.
void LightBuffer::AddLight(int light_map_x, int light_map_y, int map_z, const SpriteLight& light)
{
if (map_z <= GROUND_LAYER) {
light_map_x -= (GROUND_LAYER - map_z);
light_map_y -= (GROUND_LAYER - map_z);
}
if (light_map_x <= 0 || light_map_x >= MAP_MAX_WIDTH || light_map_y <= 0 || light_map_y >= MAP_MAX_HEIGHT) {
return;
}
uint8_t in_intensity = std::min(light.intensity, static_cast<uint8_t>(255));
// Merge with previous light at same position and color
if (!map_x.empty()) {
size_t last = map_x.size() - 1;
if (map_x[last] == light_map_x && map_y[last] == light_map_y && color[last] == light.color) {
intensity[last] = std::max(intensity[last], in_intensity);
return;
}
}
map_x.push_back(static_cast<uint16_t>(light_map_x));
map_y.push_back(static_cast<uint16_t>(light_map_y));
color.push_back(light.color);
intensity.push_back(in_intensity);
}
Comment on lines
+49
to
+58
| enum BrushColor { | ||
| COLOR_BRUSH, | ||
| COLOR_HOUSE_BRUSH, | ||
| COLOR_FLAG_BRUSH, | ||
| COLOR_SPAWN_BRUSH, | ||
| COLOR_ERASER, | ||
| COLOR_VALID, | ||
| COLOR_INVALID, | ||
| COLOR_BLANK, | ||
| }; |
There was a problem hiding this comment.
The BrushColor enum is a C-style enum. According to the style guide (line 101), enum class should be used for better type safety and to avoid polluting the enclosing scope.
Suggested change
| enum BrushColor { | |
| COLOR_BRUSH, | |
| COLOR_HOUSE_BRUSH, | |
| COLOR_FLAG_BRUSH, | |
| COLOR_SPAWN_BRUSH, | |
| COLOR_ERASER, | |
| COLOR_VALID, | |
| COLOR_INVALID, | |
| COLOR_BLANK, | |
| }; | |
| enum class BrushColor { | |
| BRUSH, | |
| HOUSE_BRUSH, | |
| FLAG_BRUSH, | |
| SPAWN_BRUSH, | |
| ERASER, | |
| VALID, | |
| INVALID, | |
| BLANK, | |
| }; |
References
- The style guide requires using
enum classinstead of rawenumfor all new or modified enumerations to improve type safety and scoping. (link)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/app/preferences/graphics_page.cpp (1)
31-36:⚠️ Potential issue | 🟠 MajorPopulate this choice from validated effects, not raw registrations.
EffectRegistry::GetRegisteredNames()can include shaders thatPostProcessManager::Initialize()later prunes on compile failure, so this UI can advertise options the runtime cannot actually apply. The: 0fallback also assumes the registry is non-empty and makes the default depend on registration order instead ofShaderNames::NONE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/preferences/graphics_page.cpp` around lines 31 - 36, The choice should be populated from the post-process manager's validated/available shader list (not raw EffectRegistry names) and default to ShaderNames::NONE; replace the loop over EffectRegistry::GetRegisteredNames() with the validated list returned by PostProcessManager (e.g. its Initialize/available shader API) so only shaders that survived compilation are shown, append those names to screen_shader_choice, then compute shader_index by searching for g_settings.getString(Config::SCREEN_SHADER) but if not found fall back to the index of ShaderNames::NONE (or no selection if NONE is missing) instead of blindly using 0; ensure you call/obtain the validated list after PostProcessManager::Initialize() so the UI reflects runtime-usable effects.source/rendering/drawers/tiles/tile_color_calculator.cpp (1)
48-48:⚠️ Potential issue | 🟡 MinorUnnecessary and potentially unsafe cast to
int.
house_idisuint32_tandcurrent_house_idis alsouint32_t. Castinghouse_idtointfor comparison is unnecessary and could cause incorrect comparisons for house IDs >=INT_MAX(2^31).🐛 Suggested fix
- if (static_cast<int>(house_id) == current_house_id) { + if (house_id == current_house_id) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/tiles/tile_color_calculator.cpp` at line 48, The comparison uses an unnecessary and unsafe cast: replace the expression "static_cast<int>(house_id) == current_house_id" with a direct unsigned comparison using the original types so both operands are uint32_t (e.g., "house_id == current_house_id"); update any surrounding logic in tile_color_calculator.cpp that relies on that cast to operate on uint32_t and remove the static_cast to avoid overflow/incorrect results for large house_id values.
🟡 Minor comments (13)
source/rendering/core/sprite_preloader.cpp-176-177 (1)
176-177:⚠️ Potential issue | 🟡 MinorMissing null guard for
gfx_inupdate()— inconsistent withpreload().The
preload()function guards against nullgfx_at line 52, butupdate()accessesgfx_->at lines 176-177 and 192-193 without any null check. Ifupdate()is called beforegfx_is set via the setter, this will cause a null pointer dereference.For consistency and defensive programming, add a guard at the start of
update():🛡️ Proposed fix
void SpritePreloader::update() { // CRITICAL: This method MUST only be called from the main GUI/OpenGL thread. assert(wxIsMainThread()); + + if (!gfx_) { + return; + } // Move results to a local queue under lock to minimize holding time. std::queue<Result> results;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_preloader.cpp` around lines 176 - 177, update() dereferences gfx_ without a null check (unlike preload()), which can lead to a null pointer dereference if update() runs before the gfx_ setter is called; add a defensive null guard at the start of update() that returns early if gfx_ is null, then proceed to call gfx_->getSpriteArchive() and gfx_->isUnloaded() only after confirming gfx_ is non-null (mirror the null-check pattern used in preload())..github/workflows/build.yml-57-63 (1)
57-63:⚠️ Potential issue | 🟡 MinorInclude vcpkg commit ID in the cache key.
The cache key hashes
vcpkg.jsonandvcpkg-configuration.jsonbut doesn't account for changes tovcpkgGitCommitId(line 55). If the vcpkg commit is updated, stale cached packages may be restored, potentially causing build inconsistencies.Proposed fix
- name: Cache vcpkg installed packages uses: actions/cache@v4 with: path: build/vcpkg_installed - key: vcpkg-installed-${{ runner.os }}-${{ hashFiles('vcpkg.json', 'vcpkg-configuration.json') }} + key: vcpkg-installed-${{ runner.os }}-1940ee77-${{ hashFiles('vcpkg.json', 'vcpkg-configuration.json') }} restore-keys: | - vcpkg-installed-${{ runner.os }}- + vcpkg-installed-${{ runner.os }}-1940ee77-Alternatively, consider extracting the vcpkg commit to an env variable to keep it DRY:
env: VCPKG_COMMIT: '1940ee77e81573713c0d364c42f5990172198be1'Then reference
${{ env.VCPKG_COMMIT }}in both the setup step and cache key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 57 - 63, The cache key for the "Cache vcpkg installed packages" step (uses: actions/cache@v4, name: Cache vcpkg installed packages) currently hashes vcpkg.json and vcpkg-configuration.json but omits the vcpkg commit variable vcpkgGitCommitId; update the key to include vcpkgGitCommitId (or extract that commit into an env var like VCPKG_COMMIT and reference it) so the key becomes something like vcpkg-installed-${{ runner.os }}-${{ env.VCPKG_COMMIT }}-${{ hashFiles('vcpkg.json','vcpkg-configuration.json') }} ensuring the cache invalidates when the vcpkg commit changes.source/rendering/postprocess/effect_registry.h-4-5 (1)
4-5:⚠️ Potential issue | 🟡 MinorInclude
<utility>in this header.Line 24 uses
std::movethree times, but the header only explicitly includes<string>and<vector>. While many standard library implementations transitively include<utility>through these headers, the C++ standard does not guarantee this, making it a fragile portability risk in a public header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/postprocess/effect_registry.h` around lines 4 - 5, This header uses std::move (observed in the header where values are moved) but only includes <string> and <vector>; add an explicit `#include` <utility> to the header so std::move is guaranteed to be declared and the header is portable and self-contained (update the includes at the top of effect_registry.h to add <utility>).source/rendering/core/brush_visual_settings.cpp-6-13 (1)
6-13:⚠️ Potential issue | 🟡 MinorClamp config values before narrowing to
uint8_t.These casts wrap on out-of-range settings values, so
-1becomes255and300becomes44. That turns a bad config into surprising colors instead of a bounded fallback.Suggested fix
BrushVisualSettings BrushVisualSettings::FromSettings(const Settings& settings) { BrushVisualSettings s; - s.cursor_red = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_RED)); - s.cursor_green = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_GREEN)); - s.cursor_blue = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_BLUE)); - s.cursor_alpha = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_ALPHA)); - s.cursor_alt_red = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_ALT_RED)); - s.cursor_alt_green = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_ALT_GREEN)); - s.cursor_alt_blue = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_ALT_BLUE)); - s.cursor_alt_alpha = static_cast<uint8_t>(settings.getInteger(Config::CURSOR_ALT_ALPHA)); + auto to_u8 = [](int value) -> uint8_t { + if (value < 0) return 0; + if (value > 255) return 255; + return static_cast<uint8_t>(value); + }; + s.cursor_red = to_u8(settings.getInteger(Config::CURSOR_RED)); + s.cursor_green = to_u8(settings.getInteger(Config::CURSOR_GREEN)); + s.cursor_blue = to_u8(settings.getInteger(Config::CURSOR_BLUE)); + s.cursor_alpha = to_u8(settings.getInteger(Config::CURSOR_ALPHA)); + s.cursor_alt_red = to_u8(settings.getInteger(Config::CURSOR_ALT_RED)); + s.cursor_alt_green = to_u8(settings.getInteger(Config::CURSOR_ALT_GREEN)); + s.cursor_alt_blue = to_u8(settings.getInteger(Config::CURSOR_ALT_BLUE)); + s.cursor_alt_alpha = to_u8(settings.getInteger(Config::CURSOR_ALT_ALPHA)); s.use_automagic = settings.getBoolean(Config::USE_AUTOMAGIC); return s; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/brush_visual_settings.cpp` around lines 6 - 13, The config integer values for cursor colors (used to set s.cursor_red, s.cursor_green, s.cursor_blue, s.cursor_alpha and their _alt counterparts) are currently narrowed with static_cast<uint8_t> which wraps on out-of-range values; clamp each settings.getInteger(Config::...) result to the 0–255 range before casting (e.g., use std::clamp(value, 0, 255) or an inline clamp helper) so negative or >255 config entries map to the bounded fallback values instead of wrapping.source/rendering/postprocess/post_process_pipeline.cpp-118-120 (1)
118-120:⚠️ Potential issue | 🟡 MinorApply scissor test to constrain
glClear()to the viewport.
glClear()does not respectglViewport()in OpenGL core profile—it clears the entire drawable region unless the scissor test is enabled. While the current code hardcodesviewport_xandviewport_yto 0 (covering the full window), applying the scissor test is defensive and aligns with OpenGL best practices for viewport-scoped operations.🛠️ Safer viewport-scoped clear
glBindFramebuffer(GL_FRAMEBUFFER, 0); glViewport(view.viewport_x, view.viewport_y, view.screensize_x, view.screensize_y); - glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT); + glEnable(GL_SCISSOR_TEST); + glScissor(view.viewport_x, view.viewport_y, view.screensize_x, view.screensize_y); + glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT); + glDisable(GL_SCISSOR_TEST);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/postprocess/post_process_pipeline.cpp` around lines 118 - 120, The glClear call after glViewport in post_process_pipeline.cpp doesn't respect the viewport in core profile; update the sequence around glBindFramebuffer/glViewport/glClear to enable the scissor test, call glScissor(view.viewport_x, view.viewport_y, view.screensize_x, view.screensize_y), then enable GL_SCISSOR_TEST before glClear and disable it afterward (or leave it enabled only if other code expects it). Modify the block using the same symbols (glBindFramebuffer, glViewport, glClear) and add glScissor and glEnable(GL_SCISSOR_TEST)/glDisable(GL_SCISSOR_TEST) to constrain the clear to the viewport.source/rendering/drawers/overlays/preview_drawer.cpp-93-93 (1)
93-93:⚠️ Potential issue | 🟡 MinorBug: Likely typo assigning
rinstead ofbfor PVPZONE blue channel.The pattern in other conditionals assigns the same variable being modified (e.g.,
r = r / 3 * 2), but herebis assigned fromr:b = r / 3 * 2; // Should this be: b = b / 3 * 2; ?This breaks the consistent coloring pattern and may produce incorrect visual output for PVPZONE tiles.
🐛 Proposed fix
if (settings.show_special_tiles && tile->getMapFlags() & TILESTATE_PVPZONE) { r = r / 3 * 2; - b = r / 3 * 2; + b = b / 3 * 2; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/preview_drawer.cpp` at line 93, In preview_drawer.cpp inside the code path handling PVPZONE coloring, there is a typo assigning b = r / 3 * 2 which uses the red channel instead of the blue channel; change that statement to b = b / 3 * 2 so the blue component is scaled consistently like the other channels (adjust the branch that modifies r, g, b for PVPZONE).source/rendering/core/sprite_icon_renderer.cpp-82-94 (1)
82-94:⚠️ Potential issue | 🟡 MinorConsider validating
spriteis not null.The method asserts on
sizevalidity but passesspritedirectly toSpriteIconGenerator::Generatewithout null checking. If callers pass a null sprite, this could cause a crash inside the generator.🛡️ Proposed defensive check
wxMemoryDC* SpriteIconRenderer::getDC(SpriteSize size, GameSprite* sprite) { ASSERT(size == SPRITE_SIZE_16x16 || size == SPRITE_SIZE_32x32 || size == SPRITE_SIZE_64x64); + if (!sprite) { + return nullptr; + } if (!dc_[size]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_icon_renderer.cpp` around lines 82 - 94, The getDC method (SpriteIconRenderer::getDC) passes sprite into SpriteIconGenerator::Generate without checking for null; add an explicit null check at the start of SpriteIconRenderer::getDC (e.g. assert(sprite) or if (!sprite) return nullptr) and avoid calling SpriteIconGenerator::Generate or g_gui.gfx.addSpriteToCleanup when sprite is null; ensure dc_[size] and bm_[size] are not touched in the null case and the function returns a safe value (nullptr) so callers don’t dereference an invalid pointer.source/rendering/ui/drawing_controller.cpp-409-418 (1)
409-418:⚠️ Potential issue | 🟡 MinorFix the wheel accumulator condition.
diff <= 1.0 || diff >= 1.0is always true for finite values, so the accumulator never actually accumulates and every Alt-wheel event resizes immediately.🛠️ Suggested fix
- if (diff <= 1.0 || diff >= 1.0) { + if (diff <= -1.0 || diff >= 1.0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/drawing_controller.cpp` around lines 409 - 418, The Alt-wheel branch uses a static accumulator variable diff (in the alt_down handling block) but checks `diff <= 1.0 || diff >= 1.0`, which is always true and prevents accumulation; change the condition to test the magnitude against the threshold (e.g. use std::abs(diff) >= 1.0 or fabs(diff) >= 1.0) so diff += rotation actually accumulates until the threshold is reached, then call g_gui.IncreaseBrushSize() or g_gui.DecreaseBrushSize() based on diff's sign and reset diff = 0.0.source/rendering/core/frame_options.h-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorMissing license header.
This file lacks the GPL license header present in all other files in this PR. For consistency with the project's licensing requirements, add the standard license header.
📝 Suggested fix
+////////////////////////////////////////////////////////////////////// +// This file is part of Remere's Map Editor +////////////////////////////////////////////////////////////////////// +// Remere's Map Editor is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Remere's Map Editor is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see <http://www.gnu.org/licenses/>. +////////////////////////////////////////////////////////////////////// + `#ifndef` RME_RENDERING_CORE_FRAME_OPTIONS_H_ `#define` RME_RENDERING_CORE_FRAME_OPTIONS_H_🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/frame_options.h` around lines 1 - 2, Add the project's standard GPL license header to the top of this header file so it matches other files in the PR; insert the same multi-line license block used elsewhere immediately above the include guard (before `#ifndef` RME_RENDERING_CORE_FRAME_OPTIONS_H_) in source/rendering/core/frame_options.h and keep the existing header guard and copyright lines intact.source/rendering/core/sprite_loader_state.h-43-43 (1)
43-43:⚠️ Potential issue | 🟡 MinorUninitialized
dat_formatmember.This enum member lacks a default initializer, unlike the
client_versionmember on line 42. If the synchronization contract is violated, readingdat_formatbefore initialization would result in undefined behavior. Provide a default value:Suggested fix
- DatFormat dat_format; + DatFormat dat_format {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_loader_state.h` at line 43, The member DatFormat dat_format is left uninitialized (unlike client_version) and can cause UB if read before being set; initialize it with a safe default by changing the declaration of dat_format in sprite_loader_state.h to include a default value (e.g., DatFormat dat_format = DatFormat::Unknown;) or, if DatFormat lacks an Unknown/default enumerator, add a clear default enumerator to the DatFormat enum and use that; ensure the chosen default is used consistently wherever SpriteLoaderState is constructed.source/rendering/ui/tooltip_data_extractor.cpp-56-63 (1)
56-63:⚠️ Potential issue | 🟡 MinorUse
asTeleport()instead ofstatic_castfor consistency and safety.Lines 49 and 67 safely downcast using
asDoor()andasContainer()respectively, with the results checked in a conditional. Line 58 usesstatic_cast<Teleport*>(item)directly. While the cast is guarded by theis_teleportcheck at line 57, usingasTeleport()maintains consistency with the pattern established in this same function and provides an additional defensive null check ifisTeleport()ever returns a false positive.Suggested fix
// Check if it's a teleport if (is_teleport) { - Teleport* tp = static_cast<Teleport*>(item); - if (tp->hasDestination()) { - destination = tp->getDestination(); - hasDestination = true; + if (Teleport* tp = item->asTeleport()) { + if (tp->hasDestination()) { + destination = tp->getDestination(); + hasDestination = true; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/tooltip_data_extractor.cpp` around lines 56 - 63, Replace the direct static_cast with the safer/as-consistent helper: instead of Teleport* tp = static_cast<Teleport*>(item) inside the is_teleport branch, call asTeleport() (e.g., Teleport* tp = item->asTeleport()) and null-check tp before using it; then call tp->hasDestination() and tp->getDestination() to set destination and hasDestination. This mirrors the pattern used with asDoor()/asContainer() and protects against false-positive is_teleport results while keeping behavior unchanged.source/rendering/core/sprite_decompression.cpp-143-153 (1)
143-153:⚠️ Potential issue | 🟡 MinorStatic log counters are not thread-safe.
The static variables
empty_log_countandblack_log_countare modified without synchronization. IfDecompress()is called from multiple threads, this creates a data race.Since this is only for diagnostic logging with a soft limit, the impact is low (worst case: slightly more or fewer log messages). If thread safety is important for this path, consider using
std::atomic<int>.🛡️ Optional fix using atomics
- if (!non_zero_alpha_found && id > 100) { - static int empty_log_count = 0; - if (empty_log_count++ < 10) { + if (!non_zero_alpha_found && id > 100) { + static std::atomic<int> empty_log_count{0}; + if (empty_log_count.fetch_add(1, std::memory_order_relaxed) < 10) { spdlog::info("Sprite {}: Decoded fully transparent sprite. bpp used: {}, dump size: {}", id, bpp, dump.size()); } } else if (!non_black_pixel_found && non_zero_alpha_found && id > 100) { - static int black_log_count = 0; - if (black_log_count++ < 10) { + static std::atomic<int> black_log_count{0}; + if (black_log_count.fetch_add(1, std::memory_order_relaxed) < 10) { spdlog::warn("Sprite {}: Decoded PURE BLACK sprite (Alpha > 0, RGB = 0). bpp used: {}, dump size: {}. Check hasTransparency() config!", id, bpp, dump.size()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_decompression.cpp` around lines 143 - 153, The static counters empty_log_count and black_log_count in sprite_decompression.cpp are not thread-safe; replace them with std::atomic<int> (e.g., declare static std::atomic<int> empty_log_count{0} and black_log_count{0}), include <atomic>, and use atomic fetch_add or ++ to increment and compare against the limit (e.g., if (empty_log_count.fetch_add(1) < 10) ...) inside Decompress() (or the function containing this logging) so increments and checks are race-free while preserving the soft-limit logging behavior.source/rendering/ui/selection_controller.cpp-344-410 (1)
344-410:⚠️ Potential issue | 🟡 MinorThe new sizing logic disables multithreaded bound-box selection in most modes.
numtilesis only populated inSELECT_ALL_FLOORS, so the current-floor and visible-floor cases always hitthreadcount = 1. Then Line 409 further collapses anywidth < threadcountcase to a single worker withstd::min(1, width). If this path is meant to preserve the old parallel behavior, the tile count and clamp need to be fixed together.🧵 Proposed fix
switch (g_settings.getInteger(Config::SELECTION_TYPE)) { case SELECT_CURRENT_FLOOR: { s_z = e_z = floor; s_x = start_x; s_y = start_y; e_x = end_x; e_y = end_y; + numtiles = (e_x - s_x + 1) * (e_y - s_y + 1); break; } case SELECT_ALL_FLOORS: { s_x = start_x; s_y = start_y; @@ - numtiles = (s_z - e_z) * (e_x - s_x) * (e_y - s_y); + numtiles = (s_z - e_z + 1) * (e_x - s_x + 1) * (e_y - s_y + 1); break; } case SELECT_VISIBLE_FLOORS: { s_x = start_x; s_y = start_y; @@ if (g_settings.getInteger(Config::COMPENSATED_SELECT)) { s_x -= (floor < GROUND_LAYER ? GROUND_LAYER - floor : 0); s_y -= (floor < GROUND_LAYER ? GROUND_LAYER - floor : 0); e_x -= (floor < GROUND_LAYER ? GROUND_LAYER - floor : 0); e_y -= (floor < GROUND_LAYER ? GROUND_LAYER - floor : 0); } + numtiles = (s_z - e_z + 1) * (e_x - s_x + 1) * (e_y - s_y + 1); break; } } @@ int width = e_x - s_x; if (width < threadcount) { - threadcount = std::min(1, width); + threadcount = std::max(1, std::min(threadcount, width + 1)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/selection_controller.cpp` around lines 344 - 410, The selection code only sets numtiles for SELECT_ALL_FLOORS causing threadcount to be forced to 1 for other modes and the width clamp uses std::min(1, width) which collapses threading; fix by computing numtiles for every selection mode (use inclusive counts: xCount = e_x - s_x, yCount = e_y - s_y, zCount = s_z - e_z (or +1 if layers are inclusive) and set numtiles = xCount * yCount * zCount) inside SELECT_CURRENT_FLOOR and SELECT_VISIBLE_FLOORS as well, and change the final clamp from threadcount = std::min(1, width) to threadcount = std::max(1, std::min(threadcount, width)) (or equivalent) so threadcount is reduced to available columns but never dropped below 1; update references in selection_controller.cpp around variables numtiles, threadcount, and the switch cases SELECT_CURRENT_FLOOR / SELECT_ALL_FLOORS / SELECT_VISIBLE_FLOORS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 041f9052-b896-46f3-ac3a-72d7f61b13a1
📒 Files selected for processing (127)
.github/workflows/build.ymlsource/CMakeLists.txtsource/app/preferences/graphics_page.cppsource/game/item_attributes.cppsource/ingame_preview/ingame_preview_renderer.cppsource/ingame_preview/ingame_preview_renderer.hsource/rendering/core/atlas_lifecycle.cppsource/rendering/core/atlas_lifecycle.hsource/rendering/core/brush_visual_settings.cppsource/rendering/core/brush_visual_settings.hsource/rendering/core/draw_context.hsource/rendering/core/drawing_options.cppsource/rendering/core/frame_accumulators.hsource/rendering/core/frame_options.hsource/rendering/core/game_sprite.cppsource/rendering/core/game_sprite.hsource/rendering/core/graphics.cppsource/rendering/core/graphics.hsource/rendering/core/graphics_assembler.cppsource/rendering/core/graphics_sprite_resolver.hsource/rendering/core/image.cppsource/rendering/core/light_buffer.cppsource/rendering/core/light_buffer.hsource/rendering/core/normal_image.cppsource/rendering/core/pending_node_requests.hsource/rendering/core/render_settings.cppsource/rendering/core/render_settings.hsource/rendering/core/render_view.cppsource/rendering/core/render_view.hsource/rendering/core/shared_geometry.hsource/rendering/core/special_client_ids.hsource/rendering/core/sprite_batch.cppsource/rendering/core/sprite_batch.hsource/rendering/core/sprite_database.cppsource/rendering/core/sprite_database.hsource/rendering/core/sprite_decompression.cppsource/rendering/core/sprite_decompression.hsource/rendering/core/sprite_icon_renderer.cppsource/rendering/core/sprite_icon_renderer.hsource/rendering/core/sprite_loader_state.cppsource/rendering/core/sprite_loader_state.hsource/rendering/core/sprite_metadata.hsource/rendering/core/sprite_preload_queue.cppsource/rendering/core/sprite_preload_queue.hsource/rendering/core/sprite_preloader.cppsource/rendering/core/sprite_preloader.hsource/rendering/core/sprite_resolver.hsource/rendering/core/template_image.cppsource/rendering/core/texture_garbage_collector.cppsource/rendering/core/texture_garbage_collector.hsource/rendering/core/texture_gc.cppsource/rendering/core/texture_gc.hsource/rendering/core/view_snapshot.hsource/rendering/drawers/cursors/brush_cursor_drawer.cppsource/rendering/drawers/cursors/brush_cursor_drawer.hsource/rendering/drawers/cursors/drag_shadow_drawer.cppsource/rendering/drawers/cursors/drag_shadow_drawer.hsource/rendering/drawers/cursors/live_cursor_drawer.cppsource/rendering/drawers/cursors/live_cursor_drawer.hsource/rendering/drawers/entities/creature_drawer.cppsource/rendering/drawers/entities/creature_drawer.hsource/rendering/drawers/entities/creature_name_drawer.cppsource/rendering/drawers/entities/creature_name_drawer.hsource/rendering/drawers/entities/item_drawer.cppsource/rendering/drawers/entities/item_drawer.hsource/rendering/drawers/entities/sprite_drawer.cppsource/rendering/drawers/entities/sprite_drawer.hsource/rendering/drawers/map_layer_drawer.cppsource/rendering/drawers/map_layer_drawer.hsource/rendering/drawers/minimap_renderer.cppsource/rendering/drawers/overlays/brush_overlay_drawer.cppsource/rendering/drawers/overlays/brush_overlay_drawer.hsource/rendering/drawers/overlays/door_indicator_drawer.cppsource/rendering/drawers/overlays/door_indicator_drawer.hsource/rendering/drawers/overlays/grid_drawer.cppsource/rendering/drawers/overlays/grid_drawer.hsource/rendering/drawers/overlays/hook_indicator_drawer.cppsource/rendering/drawers/overlays/hook_indicator_drawer.hsource/rendering/drawers/overlays/marker_drawer.cppsource/rendering/drawers/overlays/marker_drawer.hsource/rendering/drawers/overlays/preview_drawer.cppsource/rendering/drawers/overlays/preview_drawer.hsource/rendering/drawers/overlays/selection_drawer.cppsource/rendering/drawers/overlays/selection_drawer.hsource/rendering/drawers/tiles/floor_drawer.cppsource/rendering/drawers/tiles/floor_drawer.hsource/rendering/drawers/tiles/shade_drawer.cppsource/rendering/drawers/tiles/shade_drawer.hsource/rendering/drawers/tiles/tile_color_calculator.cppsource/rendering/drawers/tiles/tile_color_calculator.hsource/rendering/drawers/tiles/tile_draw_plan.hsource/rendering/drawers/tiles/tile_renderer.cppsource/rendering/drawers/tiles/tile_renderer.hsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/postprocess/effect_registry.hsource/rendering/postprocess/effects/scanline.cppsource/rendering/postprocess/effects/screen.cppsource/rendering/postprocess/effects/xbrz.cppsource/rendering/postprocess/post_process_manager.cppsource/rendering/postprocess/post_process_manager.hsource/rendering/postprocess/post_process_pipeline.cppsource/rendering/postprocess/post_process_pipeline.hsource/rendering/ui/drawing_controller.cppsource/rendering/ui/keyboard_handler.cppsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/map_menu_handler.cppsource/rendering/ui/navigation_controller.cppsource/rendering/ui/nvg_image_cache.cppsource/rendering/ui/nvg_image_cache.hsource/rendering/ui/selection_controller.cppsource/rendering/ui/tooltip_collector.hsource/rendering/ui/tooltip_data.hsource/rendering/ui/tooltip_data_extractor.cppsource/rendering/ui/tooltip_data_extractor.hsource/rendering/ui/tooltip_renderer.cppsource/rendering/ui/tooltip_renderer.hsource/rendering/ui/zoom_controller.cppsource/rendering/utilities/light_calculator.cppsource/rendering/utilities/light_calculator.hsource/rendering/utilities/light_drawer.cppsource/rendering/utilities/light_drawer.hsource/rendering/utilities/light_fbo.cppsource/rendering/utilities/light_fbo.hsource/rendering/utilities/light_shader.cppsource/rendering/utilities/light_shader.h
💤 Files with no reviewable changes (1)
- source/rendering/core/drawing_options.cpp
Comment on lines
+250
to
+297
| // Draw Names | ||
| if (creature_name_drawer && vg) { | ||
| TextRenderer::BeginFrame(vg, viewport_width, viewport_height, 1.0f); // Ingame preview doesn't use scale factor yet | ||
|
|
||
| // 1. Draw creatures on map | ||
| creature_name_drawer->draw(vg, view, accumulators_.creature_names); | ||
|
|
||
| // 2. Draw our own label at precise center | ||
| if (vg) { | ||
| nvgSave(vg); | ||
| float fontSize = 11.0f; | ||
| nvgFontSize(vg, fontSize); | ||
| nvgFontFace(vg, "sans"); | ||
| nvgTextAlign(vg, NVG_ALIGN_CENTER | NVG_ALIGN_BOTTOM); | ||
|
|
||
| // Total elevation offset was calculated above. | ||
| // For the label to stay synced, we should probably fetch it again or store it. | ||
| // Since we are at the center of the screen, we can just use screen-relative coords. | ||
| float screenCenterX = static_cast<float>(viewport_width) / 2.0f; | ||
| float screenCenterY = static_cast<float>(viewport_height) / 2.0f; | ||
|
|
||
| // Fetch elevation again to be precise | ||
| int elevation_offset = GetTileElevationOffset(map.getTile(camera_pos)); | ||
|
|
||
| float labelY = screenCenterY - (16.0f + static_cast<float>(elevation_offset)) / zoom - 2.0f; | ||
|
|
||
| std::string name = preview_name; | ||
| float textBounds[4]; | ||
| nvgTextBounds(vg, 0, 0, name.c_str(), nullptr, textBounds); | ||
| float textWidth = textBounds[2] - textBounds[0]; | ||
| float textHeight = textBounds[3] - textBounds[1]; | ||
|
|
||
| float paddingX = 4.0f; | ||
| float paddingY = 2.0f; | ||
|
|
||
| nvgBeginPath(vg); | ||
| nvgRoundedRect( | ||
| vg, screenCenterX - textWidth / 2.0f - paddingX, labelY - textHeight - paddingY * 2.0f, textWidth + paddingX * 2.0f, | ||
| textHeight + paddingY * 2.0f, 3.0f | ||
| ); | ||
| nvgFillColor(vg, nvgRGBA(0, 0, 0, 160)); | ||
| nvgFill(vg); | ||
|
|
||
| nvgFillColor(vg, nvgRGBA(255, 255, 255, 255)); | ||
| nvgText(vg, screenCenterX, labelY - paddingY, name.c_str(), nullptr); | ||
| nvgRestore(vg); | ||
| } | ||
| TextRenderer::EndFrame(vg); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me find the file and examine the broader context around lines 250-297
fd -t f "ingame_preview_renderer.cpp" | head -5Repository: karolak6612/remeres-map-editor-redux
Length of output: 129
🏁 Script executed:
# Once we have the file, let's look at the full function context
wc -l source/ingame_preview/ingame_preview_renderer.cppRepository: karolak6612/remeres-map-editor-redux
Length of output: 133
🏁 Script executed:
# Examine the code around the mentioned lines and look for viewport setup
sed -n '200,350p' source/ingame_preview/ingame_preview_renderer.cpp | cat -nRepository: karolak6612/remeres-map-editor-redux
Length of output: 6375
🏁 Script executed:
# Search for viewport_x and viewport_y usage in this file
rg "viewport_x|viewport_y" source/ingame_preview/ingame_preview_renderer.cpp -B 3 -A 3Repository: karolak6612/remeres-map-editor-redux
Length of output: 1116
🏁 Script executed:
# Look for glViewport calls to see how GL pass sets the viewport
rg "glViewport" source/ingame_preview/ingame_preview_renderer.cpp -B 2 -A 2Repository: karolak6612/remeres-map-editor-redux
Length of output: 318
🏁 Script executed:
# Check TextRenderer::BeginFrame implementation to see if it handles viewport origin
fd -t f "TextRenderer" --extension "h" --extension "cpp" | xargs rg "BeginFrame" -B 2 -A 10 2>/dev/null | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 2864
🏁 Script executed:
# Search for TextRenderer class definition and BeginFrame implementation
rg "class TextRenderer|struct TextRenderer" -A 20 | head -80Repository: karolak6612/remeres-map-editor-redux
Length of output: 1207
🏁 Script executed:
# Search for TextRenderer::BeginFrame implementation
rg "TextRenderer::BeginFrame|BeginFrame\(" -B 2 -A 15 | grep -A 15 "BeginFrame"Repository: karolak6612/remeres-map-editor-redux
Length of output: 13471
🏁 Script executed:
# Look for where TextRenderer.h is located
fd -t f "TextRenderer.h"Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
# Check the actual NanoVG implementation details
cat source/rendering/core/text_renderer.cppRepository: karolak6612/remeres-map-editor-redux
Length of output: 3468
🏁 Script executed:
# Look for any coordinate transform handling in NanoVG context setup
rg "nvgBeginFrame|nvgTranslate|glViewport" source/ingame_preview/ingame_preview_renderer.cpp -B 5 -A 5Repository: karolak6612/remeres-map-editor-redux
Length of output: 627
🏁 Script executed:
# Check the NanoVG header documentation more carefully
sed -n '1,100p' ext/nanovg/src/nanovg.h | grep -A 10 "nvgBeginFrame"Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
Offset the NanoVG overlay into the preview viewport.
The GL pass honors viewport_x / viewport_y via glViewport(), but the NanoVG label rendering computes screen coordinates using only viewport_width and viewport_height. When the preview is embedded with a non-zero viewport origin, labels will render at the wrong screen position.
Pass viewport_x and viewport_y to TextRenderer::BeginFrame() (or apply an nvgTranslate() before drawing), and update label coordinate calculations to account for the viewport offset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ingame_preview/ingame_preview_renderer.cpp` around lines 250 - 297,
The NanoVG overlay is not offset for non-zero viewport origin; update the
BeginFrame/coordinate math so labels account for viewport_x/viewport_y: either
pass viewport_x and viewport_y into TextRenderer::BeginFrame(vg, viewport_width,
viewport_height, scale) if it supports origin, or immediately call nvgSave(vg);
nvgTranslate(vg, viewport_x, viewport_y); before all drawing and nvgRestore
after; also compute screenCenterX and screenCenterY using viewport_x and
viewport_y (e.g., viewport_x + viewport_width/2, viewport_y + viewport_height/2)
so the label drawing in this block (TextRenderer::BeginFrame, the nvg* calls,
and TextRenderer::EndFrame) is correctly positioned inside the preview viewport.
Comment on lines
13
to
+32
| struct LightBuffer { | ||
| struct Light { | ||
| uint16_t map_x = 0; | ||
| uint16_t map_y = 0; | ||
| uint8_t color = 0; | ||
| uint8_t intensity = 0; | ||
| }; | ||
| std::vector<uint16_t> map_x; | ||
| std::vector<uint16_t> map_y; | ||
| std::vector<uint8_t> color; | ||
| std::vector<uint8_t> intensity; | ||
|
|
||
| std::vector<Light> lights; | ||
| void AddLight(int map_x, int map_y, int map_z, const SpriteLight& light); | ||
| void Clear(); | ||
| [[nodiscard]] size_t size() const | ||
| { | ||
| return map_x.size(); | ||
| } | ||
|
|
||
| void AddLight(int map_x, int map_y, int map_z, const SpriteLight& light); | ||
| void Clear(); | ||
| void reserve(size_t capacity) | ||
| { | ||
| map_x.reserve(capacity); | ||
| map_y.reserve(capacity); | ||
| color.reserve(capacity); | ||
| intensity.reserve(capacity); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the SoA storage encapsulated.
With map_x, map_y, color, and intensity public, nothing enforces that they stay the same length, while size() assumes map_x.size() is authoritative. One direct mutation on another vector turns downstream indexed iteration into UB. Please hide the arrays behind AddLight(), Clear(), and reserve(), and expose read-only views if callers need iteration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/light_buffer.h` around lines 13 - 32, Make
LightBuffer's SoA vectors (map_x, map_y, color, intensity) private and preserve
AddLight, Clear, reserve, and size as the canonical mutation/inspection API;
update callers to use new read-only accessors (e.g., methods that return const
references or std::span<const uint16_t>/std::span<const uint8_t>) for iteration
instead of touching the vectors directly. Ensure size() still returns
map_x.size(), AddLight appends to all four arrays in lockstep, Clear clears all
arrays, and reserve forwards capacity to all arrays; add const accessor methods
named something like mapXView(), mapYView(), colorView(), intensityView() that
return immutable views for callers that need to iterate.
Comment on lines
+42
to
+48
| // Returns all pending requests and clears the internal buffer. | ||
| std::vector<NodeRequest> drain() | ||
| { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| std::vector<NodeRequest> result; | ||
| result.swap(pending_); | ||
| return result; |
There was a problem hiding this comment.
drain() currently defeats reserve().
Swapping pending_ with a fresh vector clears the buffer, but it also drops the capacity preallocated on Lines 51-55. In this per-frame path that means the next enqueue() burst reallocates again. Please keep a reusable backing store here instead of swapping with an empty vector, for example via a caller-owned output buffer or a double-buffered scratch vector.
Also applies to: 51-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/pending_node_requests.h` around lines 42 - 48, drain()
currently swaps pending_ with a fresh vector which clears capacity and forces
reallocs on the next enqueue; fix by preserving pending_’s capacity when
draining: capture pending_.capacity() before swapping (or moving) into the
result, perform the swap (or std::move) to return the elements, then call
pending_.reserve(saved_capacity()) to restore the backing store capacity; apply
the same approach to the other drain-like block around the enqueue-related code
(the code that currently swaps/clears pending_) or alternatively change drain()
to accept a caller-owned output buffer (or implement a double-buffer scratch
vector) and reuse pending_.capacity() rather than dropping it.
Comment on lines
+9
to
+43
| void RenderSettings::SetDefault() { | ||
| transparent_floors = false; | ||
| transparent_items = false; | ||
| show_ingame_box = false; | ||
| show_lights = false; | ||
| show_light_str = true; | ||
| show_tech_items = true; | ||
| show_waypoints = true; | ||
| ingame = false; | ||
|
|
||
| show_grid = 0; | ||
| show_all_floors = true; | ||
| show_creatures = true; | ||
| show_spawns = true; | ||
| show_houses = true; | ||
| show_shade = true; | ||
| show_special_tiles = true; | ||
| show_items = true; | ||
|
|
||
| highlight_items = false; | ||
| highlight_locked_doors = true; | ||
| show_blocking = false; | ||
| show_tooltips = false; | ||
| show_as_minimap = false; | ||
| show_only_colors = false; | ||
| show_only_modified = false; | ||
| show_preview = false; | ||
| show_hooks = false; | ||
| hide_items_when_zoomed = true; | ||
| light_intensity = 1.0f; | ||
| ambient_light_level = 0.5f; | ||
| anti_aliasing = false; | ||
| screen_shader_name = "None"; | ||
|
|
||
| } |
There was a problem hiding this comment.
Initialize the members SetDefault() currently skips.
show_towns, always_show_zones, extended_house_shader, and experimental_fog are declared on RenderSettings but never assigned here. Because the constructor only calls SetDefault(), a plain RenderSettings starts with indeterminate booleans and can flip features nondeterministically.
🛠️ Minimal fix
void RenderSettings::SetDefault() {
transparent_floors = false;
transparent_items = false;
show_ingame_box = false;
show_lights = false;
show_light_str = true;
show_tech_items = true;
show_waypoints = true;
ingame = false;
show_grid = 0;
show_all_floors = true;
show_creatures = true;
show_spawns = true;
show_houses = true;
show_shade = true;
show_special_tiles = true;
show_items = true;
highlight_items = false;
highlight_locked_doors = true;
show_blocking = false;
show_tooltips = false;
show_as_minimap = false;
show_only_colors = false;
show_only_modified = false;
show_preview = false;
show_hooks = false;
hide_items_when_zoomed = true;
+ show_towns = false;
+ always_show_zones = false;
+ extended_house_shader = false;
+ experimental_fog = false;
light_intensity = 1.0f;
ambient_light_level = 0.5f;
anti_aliasing = false;
screen_shader_name = "None";
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/render_settings.cpp` around lines 9 - 43,
RenderSettings::SetDefault currently omits initializing the members show_towns,
always_show_zones, extended_house_shader, and experimental_fog, leaving them
with indeterminate values; update SetDefault to explicitly assign sensible
defaults for those four members (e.g., false for boolean flags and an
appropriate default for extended_house_shader if it's a bool or enum) so that a
newly constructed RenderSettings has deterministic state; locate the SetDefault
method and add assignments for show_towns, always_show_zones,
extended_house_shader, and experimental_fog consistent with the other field
initializations.
Comment on lines
+41
to
+49
| void SpriteDatabase::insertSprite(int id, std::unique_ptr<Sprite> sprite) { | ||
| if (id < 0) { | ||
| editor_sprite_space_[id] = std::move(sprite); | ||
| } else { | ||
| if (static_cast<size_t>(id) >= sprite_space_.size()) { | ||
| sprite_space_.resize(id + 1); | ||
| } | ||
| sprite_space_[id] = std::move(sprite); | ||
| } |
There was a problem hiding this comment.
Invalidate the resident cache before replacing a sprite slot.
Lines 45-48 can destroy an existing GameSprite while resident_game_sprites_ still holds its raw pointer. resize() already clears the resident caches on shrink for the same lifetime hazard; slot replacement needs the same protection.
🛠️ One safe fallback
void SpriteDatabase::insertSprite(int id, std::unique_ptr<Sprite> sprite) {
if (id < 0) {
editor_sprite_space_[id] = std::move(sprite);
} else {
if (static_cast<size_t>(id) >= sprite_space_.size()) {
sprite_space_.resize(id + 1);
}
+ if (sprite_space_[id]) {
+ resident_game_sprites_.clear();
+ }
sprite_space_[id] = std::move(sprite);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void SpriteDatabase::insertSprite(int id, std::unique_ptr<Sprite> sprite) { | |
| if (id < 0) { | |
| editor_sprite_space_[id] = std::move(sprite); | |
| } else { | |
| if (static_cast<size_t>(id) >= sprite_space_.size()) { | |
| sprite_space_.resize(id + 1); | |
| } | |
| sprite_space_[id] = std::move(sprite); | |
| } | |
| void SpriteDatabase::insertSprite(int id, std::unique_ptr<Sprite> sprite) { | |
| if (id < 0) { | |
| editor_sprite_space_[id] = std::move(sprite); | |
| } else { | |
| if (static_cast<size_t>(id) >= sprite_space_.size()) { | |
| sprite_space_.resize(id + 1); | |
| } | |
| if (sprite_space_[id]) { | |
| resident_game_sprites_.clear(); | |
| } | |
| sprite_space_[id] = std::move(sprite); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/sprite_database.cpp` around lines 41 - 49, In
insertSprite, invalidate the resident_game_sprites_ cache before any operation
that may destroy an existing sprite to avoid leaving dangling raw pointers;
specifically, before writing to editor_sprite_space_[id] or sprite_space_[id]
(and before calling sprite_space_.resize when it may remove elements), clear or
otherwise invalidate resident_game_sprites_ so the cache no longer references
the old GameSprite pointers being replaced or destroyed.
Comment on lines
+202
to
+214
| void MapCanvas::EnsureNanoVG() | ||
| { | ||
| if (!m_nvg) { | ||
| if (!gladLoadGL()) { | ||
| spdlog::error("MapCanvas: Failed to initialize GLAD"); | ||
| } | ||
| m_nvg.reset(nvgCreateGL3(NVG_ANTIALIAS | NVG_STENCIL_STROKES)); | ||
| if (m_nvg) { | ||
| TextRenderer::LoadFont(m_nvg.get()); | ||
| } else { | ||
| spdlog::error("MapCanvas: Failed to initialize NanoVG"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the file
find . -name "map_display.cpp" -type fRepository: karolak6612/remeres-map-editor-redux
Length of output: 117
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f "source/rendering/ui/map_display.cpp" ]; then
wc -l "source/rendering/ui/map_display.cpp"
fiRepository: karolak6612/remeres-map-editor-redux
Length of output: 119
🏁 Script executed:
# Read the function around lines 202-214
sed -n '195,225p' source/rendering/ui/map_display.cppRepository: karolak6612/remeres-map-editor-redux
Length of output: 822
🏁 Script executed:
# Search for other uses of gladLoadGL in the codebase to understand error handling patterns
rg "gladLoadGL" --type cpp -A 5 -B 5Repository: karolak6612/remeres-map-editor-redux
Length of output: 3754
🏁 Script executed:
# Search for nvgCreateGL3 usage patterns
rg "nvgCreateGL3" --type cpp -A 5 -B 5Repository: karolak6612/remeres-map-editor-redux
Length of output: 4629
Add early return after GLAD initialization failure.
When gladLoadGL() fails, GL function pointers remain unresolved. Proceeding to call nvgCreateGL3() will attempt to use those unresolved pointers, resulting in undefined behavior. This pattern is correctly implemented in source/util/nanovg_canvas.cpp but missing here.
Proposed fix
void MapCanvas::EnsureNanoVG()
{
if (!m_nvg) {
if (!gladLoadGL()) {
spdlog::error("MapCanvas: Failed to initialize GLAD");
+ return;
}
m_nvg.reset(nvgCreateGL3(NVG_ANTIALIAS | NVG_STENCIL_STROKES));
if (m_nvg) {
TextRenderer::LoadFont(m_nvg.get());
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/ui/map_display.cpp` around lines 202 - 214, In
MapCanvas::EnsureNanoVG, when gladLoadGL() returns false you must return
immediately instead of proceeding; modify EnsureNanoVG to check the result of
gladLoadGL() and if false log the error (spdlog::error("MapCanvas: Failed to
initialize GLAD")) and return before calling nvgCreateGL3 or touching m_nvg, so
nvgCreateGL3(NVG_ANTIALIAS | NVG_STENCIL_STROKES) and
TextRenderer::LoadFont(m_nvg.get()) only run when GLAD initialized successfully.
Comment on lines
+118
to
+127
| if (event.ControlDown()) { | ||
| static double diff = 0.0; | ||
| diff += event.GetWheelRotation(); | ||
| if (diff <= 1.0 || diff >= 1.0) { | ||
| if (diff < 0.0) { | ||
| ChangeFloor(canvas, canvas->GetFloor() - 1); | ||
| } else { | ||
| ChangeFloor(canvas, canvas->GetFloor() + 1); | ||
| } | ||
| diff = 0.0; |
There was a problem hiding this comment.
Restore a real wheel delta threshold.
Line 121 is currently a tautology, so every control-wheel event trips a floor change and throws away any leftover rotation. That breaks the intended accumulation behavior on high-resolution wheels/trackpads.
Suggested fix
if (event.ControlDown()) {
- static double diff = 0.0;
- diff += event.GetWheelRotation();
- if (diff <= 1.0 || diff >= 1.0) {
- if (diff < 0.0) {
- ChangeFloor(canvas, canvas->GetFloor() - 1);
- } else {
- ChangeFloor(canvas, canvas->GetFloor() + 1);
- }
- diff = 0.0;
- }
+ static int accumulated_rotation = 0;
+ accumulated_rotation += event.GetWheelRotation();
+ const int wheel_delta = event.GetWheelDelta();
+ while (std::abs(accumulated_rotation) >= wheel_delta) {
+ ChangeFloor(canvas, canvas->GetFloor() + (accumulated_rotation > 0 ? 1 : -1));
+ accumulated_rotation += accumulated_rotation > 0 ? -wheel_delta : wheel_delta;
+ }
canvas->UpdatePositionStatus();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (event.ControlDown()) { | |
| static double diff = 0.0; | |
| diff += event.GetWheelRotation(); | |
| if (diff <= 1.0 || diff >= 1.0) { | |
| if (diff < 0.0) { | |
| ChangeFloor(canvas, canvas->GetFloor() - 1); | |
| } else { | |
| ChangeFloor(canvas, canvas->GetFloor() + 1); | |
| } | |
| diff = 0.0; | |
| if (event.ControlDown()) { | |
| static int accumulated_rotation = 0; | |
| accumulated_rotation += event.GetWheelRotation(); | |
| const int wheel_delta = event.GetWheelDelta(); | |
| while (std::abs(accumulated_rotation) >= wheel_delta) { | |
| ChangeFloor(canvas, canvas->GetFloor() + (accumulated_rotation > 0 ? 1 : -1)); | |
| accumulated_rotation += accumulated_rotation > 0 ? -wheel_delta : wheel_delta; | |
| } | |
| canvas->UpdatePositionStatus(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/ui/navigation_controller.cpp` around lines 118 - 127, The
control-wheel handling currently uses a tautological check and resets
accumulated rotation; change the condition to check magnitude (e.g.,
std::abs(diff) >= 1.0) so high-resolution/trackpad deltas accumulate properly,
and after calling ChangeFloor(canvas, canvas->GetFloor() +/- 1) remove only one
threshold's worth from diff (e.g., diff -= std::copysign(1.0, diff)) instead of
zeroing it so leftover rotation is preserved for subsequent events; update the
block around event.ControlDown(), diff, event.GetWheelRotation(), and
ChangeFloor accordingly.
Comment on lines
+34
to
+52
| class TooltipRenderer { | ||
| public: | ||
| TooltipRenderer() = default; | ||
|
|
||
| // Draw all tooltips from the collected data | ||
| void draw(NVGcontext* vg, const ViewState& view, std::span<const TooltipData> tooltips, NVGImageCache& imageCache); | ||
|
|
||
| private: | ||
| // Layout scratch buffers (reused across frames) | ||
| struct FieldLine { | ||
| std::string_view label; | ||
| std::string_view value; | ||
| uint8_t r, g, b; | ||
| std::vector<std::string_view> wrappedLines; | ||
| }; | ||
| std::vector<FieldLine> scratch_fields_; | ||
| size_t scratch_fields_count_ = 0; | ||
| std::string storage_; // Scratch buffer for text generation | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*tooltip*" -type f | head -20Repository: karolak6612/remeres-map-editor-redux
Length of output: 338
🏁 Script executed:
cat -n ./source/rendering/ui/tooltip_renderer.cppRepository: karolak6612/remeres-map-editor-redux
Length of output: 18091
🏁 Script executed:
cat -n ./source/rendering/ui/tooltip_renderer.h | head -60Repository: karolak6612/remeres-map-editor-redux
Length of output: 2549
🏁 Script executed:
rg "TooltipRenderer" --type cpp --type h -A 3 -B 1 | grep -E "(TooltipRenderer|=|copy|move|std::move)" | head -40Repository: karolak6612/remeres-map-editor-redux
Length of output: 1838
🏁 Script executed:
grep -n "tooltip_renderer" ./source/rendering/map_drawer.h -A 5 -B 5Repository: karolak6612/remeres-map-editor-redux
Length of output: 743
🏁 Script executed:
grep -n "MapDrawer" ./source/rendering/map_drawer.h | head -5Repository: karolak6612/remeres-map-editor-redux
Length of output: 215
🏁 Script executed:
grep -n "class MapDrawer" ./source/rendering/map_drawer.h -A 60 | head -70Repository: karolak6612/remeres-map-editor-redux
Length of output: 2276
🏁 Script executed:
rg "MapDrawer" --type cpp --type h | grep -E "(std::move|copy|=\s*MapDrawer|MapDrawer\s*=)" | head -20Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
rg "MapDrawer\(" --type cpp --type h -B 3 -A 3 | head -40Repository: karolak6612/remeres-map-editor-redux
Length of output: 1245
🏁 Script executed:
rg "std::move.*tooltip_renderer|tooltip_renderer.*std::move" --type cpp --type hRepository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
rg "tooltip_renderer\." --type cpp -A 2 -B 1 | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 1073
Delete the default special members on TooltipRenderer.
scratch_fields_ and wrappedLines hold std::string_views that point into storage_. The default copy/move operations shallow-copy these views, leaving dangling pointers in the new instance while the original remains valid.
Suggested fix
class TooltipRenderer {
public:
TooltipRenderer() = default;
+ TooltipRenderer(const TooltipRenderer&) = delete;
+ TooltipRenderer& operator=(const TooltipRenderer&) = delete;
+ TooltipRenderer(TooltipRenderer&&) = delete;
+ TooltipRenderer& operator=(TooltipRenderer&&) = delete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/ui/tooltip_renderer.h` around lines 34 - 52,
TooltipRenderer's implicitly-generated copy/move operations can yield dangling
std::string_view references (scratch_fields_, FieldLine::wrappedLines) into
storage_, so explicitly delete the special members to prevent shallow copies:
declare TooltipRenderer(const TooltipRenderer&) = delete; TooltipRenderer&
operator=(const TooltipRenderer&) = delete; TooltipRenderer(TooltipRenderer&&) =
delete; TooltipRenderer& operator=(TooltipRenderer&&) = delete; keep the default
constructor and destructor as-is and reference the types FieldLine,
scratch_fields_, wrappedLines, and storage_ when making the change.
tile_renderer.cpp included tile_renderer.h before the PCH, which chains through tile_draw_plan.h → item_drawer.h → creatures.h. On GCC, the PCH must be the first include or dependent types (wxString, FileName, pugi) are undefined. Also add missing PCH include to brush_visual_settings.cpp and sprite_preload_queue.cpp for consistency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
source/rendering/core/brush_visual_settings.cpp (1)
5-17: LGTM — Clean factory implementation.The factory method correctly populates all struct fields from the Settings source, aligning with the SRP refactor goals. The field names and Config keys all match the declarations in the header and
settings.h.Optional consideration: The
static_cast<uint8_t>will silently truncate integer values outside[0, 255]. If Settings could ever contain out-of-range values (e.g., from corrupt config files), consider defensive clamping:s.cursor_red = static_cast<uint8_t>(std::clamp(settings.getInteger(Config::CURSOR_RED), 0, 255));This would require
#include <algorithm>. However, if Settings validation is guaranteed upstream, the current implementation is fine.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/brush_visual_settings.cpp` around lines 5 - 17, The current BrushVisualSettings::FromSettings uses static_cast<uint8_t> on settings.getInteger(...) which will silently truncate out-of-range integers; to harden it, clamp each color/int alpha value from settings.getInteger(Config::...) into [0,255] before casting (e.g., use std::clamp) for fields cursor_red, cursor_green, cursor_blue, cursor_alpha, cursor_alt_red, cursor_alt_green, cursor_alt_blue, cursor_alt_alpha, and add the necessary `#include` <algorithm>; leave use_automagic assigned from settings.getBoolean(Config::USE_AUTOMAGIC) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/rendering/core/sprite_preload_queue.cpp`:
- Around line 7-9: Replace the silent early return in SpritePreloadQueue (the
preloader_ check in source/rendering/core/sprite_preload_queue.cpp) with a
fail-fast assertion so queued preloads are not discarded; instead of returning
when preloader_ is null, assert that preloader_ is set (or log and abort in
debug) before calling processAll()/FlushPreloadQueue to make wiring bugs
visible, referencing the existing preloader_ member and the
FlushPreloadQueue/processAll flow so the check is performed where the current if
(!preloader_) return; is located.
In `@source/rendering/drawers/tiles/tile_renderer.cpp`:
- Around line 282-299: The stacked-item path must guard against a missing
ItemDefinitionView like the ground path does: after obtaining ItemDefinitionView
it = item->getDefinition() in the tile rendering loop, check whether the
definition is valid/null before using it (i.e. before calling
TooltipDataExtractor::Fill, PatternCalculator::Calculate, or inspecting
door/hook logic); if invalid, skip further processing for that item (continue)
or handle it the same way as the ground-item guard so we don't dereference an
empty ItemDefinitionView. Ensure the early-return/continue is applied before any
use of it, including tooltip commit, sprite pattern calculation,
preload_queue_.enqueue, and later door/hook checks.
- Around line 30-35: Add defensive validation in the TileRenderer constructor:
check that TileRenderDeps fields item_drawer, sprite_drawer, creature_drawer,
marker_drawer, and editor are non-null and fail fast (e.g., with assert(...) or
by throwing std::invalid_argument) before assigning them to the members; update
the TileRenderer::TileRenderer(const TileRenderDeps& deps) initializer body to
perform these checks (referencing TileRenderDeps, TileRenderer, and the member
names item_drawer, sprite_drawer, creature_drawer, marker_drawer, editor) so
future partial initializations cannot cause null-pointer dereferences later.
---
Nitpick comments:
In `@source/rendering/core/brush_visual_settings.cpp`:
- Around line 5-17: The current BrushVisualSettings::FromSettings uses
static_cast<uint8_t> on settings.getInteger(...) which will silently truncate
out-of-range integers; to harden it, clamp each color/int alpha value from
settings.getInteger(Config::...) into [0,255] before casting (e.g., use
std::clamp) for fields cursor_red, cursor_green, cursor_blue, cursor_alpha,
cursor_alt_red, cursor_alt_green, cursor_alt_blue, cursor_alt_alpha, and add the
necessary `#include` <algorithm>; leave use_automagic assigned from
settings.getBoolean(Config::USE_AUTOMAGIC) as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdb9aebe-b778-4971-b3ab-16d50ade37e7
📒 Files selected for processing (3)
source/rendering/core/brush_visual_settings.cppsource/rendering/core/sprite_preload_queue.cppsource/rendering/drawers/tiles/tile_renderer.cpp
Comment on lines
+7
to
+9
| if (!preloader_) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fail fast here instead of silently dropping queued preloads.
FlushPreloadQueue() clears the queue unconditionally right after processAll() in source/rendering/drawers/tiles/tile_renderer.h:80-86, so this early return discards every queued request when preloader_ has not been set. Because that path is reachable during frame rendering in source/rendering/map_drawer.cpp:316-331, this turns a wiring bug into silent missed preloads and harder-to-diagnose rendering hitches. An assertion here would be much safer than returning.
Suggested change
void SpritePreloadQueue::processAll()
{
- if (!preloader_) {
- return;
- }
+ ASSERT(preloader_ && "SpritePreloadQueue::processAll() called without a preloader");
for (const auto& req : requests) {
preloader_->preload(req.sprite, req.pattern_x, req.pattern_y, req.pattern_z, req.frame);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/sprite_preload_queue.cpp` around lines 7 - 9, Replace
the silent early return in SpritePreloadQueue (the preloader_ check in
source/rendering/core/sprite_preload_queue.cpp) with a fail-fast assertion so
queued preloads are not discarded; instead of returning when preloader_ is null,
assert that preloader_ is set (or log and abort in debug) before calling
processAll()/FlushPreloadQueue to make wiring bugs visible, referencing the
existing preloader_ member and the FlushPreloadQueue/processAll flow so the
check is performed where the current if (!preloader_) return; is located.
Comment on lines
+30
to
+35
| TileRenderer::TileRenderer(const TileRenderDeps& deps) : | ||
| item_drawer(deps.item_drawer), | ||
| sprite_drawer(deps.sprite_drawer), | ||
| creature_drawer(deps.creature_drawer), | ||
| marker_drawer(deps.marker_drawer), | ||
| editor(deps.editor) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect TileRenderer construction sites and TileRenderDeps initializers.
# Expected: every TileRenderDeps used for TileRenderer populates
# item_drawer, sprite_drawer, creature_drawer, marker_drawer, and editor.
rg -n -C3 '\bTileRenderDeps\b|\.item_drawer\b|\.sprite_drawer\b|\.creature_drawer\b|\.marker_drawer\b|\.editor\b|TileRenderer\s*\(' --type=cpp --type=hRepository: karolak6612/remeres-map-editor-redux
Length of output: 8559
Add defensive validation to the TileRenderer constructor to prevent null-pointer dereferences.
While the current construction site in map_drawer.cpp properly initializes all required dependencies, the constructor accepts TileRenderDeps without validation. The struct defaults all fields to nullptr, but the class dereferences editor at line 137 and the drawer pointers at lines 367–397 without guards. Add assert() statements or an explicit precondition check in the constructor to fail fast if any required dependency is null, preventing latent crashes if future code partially initializes the struct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/drawers/tiles/tile_renderer.cpp` around lines 30 - 35, Add
defensive validation in the TileRenderer constructor: check that TileRenderDeps
fields item_drawer, sprite_drawer, creature_drawer, marker_drawer, and editor
are non-null and fail fast (e.g., with assert(...) or by throwing
std::invalid_argument) before assigning them to the members; update the
TileRenderer::TileRenderer(const TileRenderDeps& deps) initializer body to
perform these checks (referencing TileRenderDeps, TileRenderer, and the member
names item_drawer, sprite_drawer, creature_drawer, marker_drawer, editor) so
future partial initializations cannot cause null-pointer dereferences later.
Create ViewStateManager class that owns floor_ and zoom_ state, with scroll access delegated to the parent MapWindow. Remove raw floor/zoom fields from MapCanvas; all access now goes through view_state_ member. MapCanvas::GetFloor(), GetZoom(), SetFloorDirect(), SetZoomDirect(), and ScreenToMap() all delegate to ViewStateManager. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move garbage-collection timing (shouldCollect / markCollected) from MapCanvas into TextureGC, removing m_last_gc_time from map_display. MapCanvas::PerformGarbageCollection() now delegates to TextureGC. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract BuildViewSnapshot(), ConfigureRenderSettings(), and ConfigureFrameOptions() from OnPaint(). OnPaint is now a slim orchestrator: configure → build snapshot → draw → overlays → cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… pass Introduce BrushSnapshot struct capturing current_brush, brush_shape, brush_size, and is_drawing_mode. MapCanvas snapshots these once in OnPaint() and passes them to MapDrawer::SetupVars(). MapDrawer no longer calls g_gui for brush state — uses stored brush_snapshot_. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… cycle Remove NormalImage::parent and GameSprite::invalidateCache(). The generation_id validation in GameSprite::getAtlasRegion() already detects stale cached_default_region after texture eviction, making the parent back-pointer redundant. This eliminates the bidirectional pointer cycle between NormalImage and GameSprite. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…text Introduce RenderContext struct wrapping GraphicManager&. MapDrawer now receives it via constructor instead of accessing g_gui.gfx globally. All 5 g_gui.gfx calls in map_drawer.cpp replaced with render_ctx_.gfx. The gui.h include is removed from map_drawer.cpp — it no longer depends on the GUI singleton. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bundle RenderSettings, FrameOptions, ViewState, ViewSnapshot, BrushSnapshot, LightBuffer, and atlas pointer into a single DrawFrame struct. MapDrawer now has one frame_ member replacing 7 scattered per-frame fields. This isolates frame-scoped mutable state from long-lived infrastructure, preparing for future multi-threaded frame construction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add early-out for empty TileLocations in the inner traversal loop, skipping function call overhead for null tiles (the common case in typical viewports). Use [[likely]] hint since most tiles are empty. The sequential pointer walk through Floor::locs already provides good spatial locality for the 4x4 tile block. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… contention Replace single queue_mutex with two independent mutexes: - task_mutex_: protects task_queue, pending_ids, active_epoch, stopping - result_mutex_: protects result_buffer_ (was result_queue) Workers pushing decompressed results no longer contend with workers popping tasks. The result container is now a vector (swapped in update()) for better cache locality during drain. The update() loop uses range-for instead of queue pop for cleaner iteration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/rendering/core/sprite_preloader.cpp (1)
28-40:⚠️ Potential issue | 🔴 CriticalMove worker threads and join them before container destruction.
The
std::vector<std::jthread> workersis declared beforetask_queue,result_buffer_, andpending_ids. In C++, members are destroyed in reverse declaration order, so these containers begin destruction before workers are joined. A worker can still be running—decompressing or pushing results—while those containers are being torn down, causing use-after-free.The current
shutdown()only callsrequest_stop()and notifies; the threads are joined later during member destruction. Move workers out and clear them synchronously to ensure all threads have finished before container teardown begins.Suggested fix
void SpritePreloader::shutdown() { + std::vector<std::jthread> workers_to_join; { std::lock_guard<std::mutex> lock(task_mutex_); if (stopping) { return; } stopping = true; + workers_to_join = std::move(workers); } - for (auto& worker : workers) { + for (auto& worker : workers_to_join) { worker.request_stop(); // Correctly signaled transition for jthread's stop_token } cv.notify_all(); + workers_to_join.clear(); // join before other members begin destruction }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_preloader.cpp` around lines 28 - 40, The shutdown() currently only signals worker threads via worker.request_stop() and cv.notify_all(), but leaves std::vector<std::jthread> workers to be destroyed later while task_queue, result_buffer_, and pending_ids may still be alive; move the workers vector out and synchronously join/clear it inside SpritePreloader::shutdown to ensure all threads exit before container destruction: after setting stopping under task_mutex_ and notifying (cv.notify_all()), swap or std::move workers into a local std::vector<std::jthread> local_workers, then let local_workers go out of scope (or explicitly join/clear it) so all jthreads are joined before the member containers (task_queue, result_buffer_, pending_ids) are destroyed. Ensure you reference the same worker.request_stop() calls and preserve notifying via cv.notify_all() prior to moving workers.
♻️ Duplicate comments (5)
source/rendering/ui/map_display.cpp (4)
203-216:⚠️ Potential issue | 🔴 CriticalAdd early return after GLAD initialization failure.
When
gladLoadGL()fails, GL function pointers remain unresolved. Proceeding to callnvgCreateGL3()will attempt to use those unresolved pointers, causing undefined behavior or crashes.Proposed fix
void MapCanvas::EnsureNanoVG() { if (!m_nvg) { if (!gladLoadGL()) { spdlog::error("MapCanvas: Failed to initialize GLAD"); + return; } m_nvg.reset(nvgCreateGL3(NVG_ANTIALIAS | NVG_STENCIL_STROKES));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.cpp` around lines 203 - 216, In MapCanvas::EnsureNanoVG, when gladLoadGL() fails you must return early instead of continuing; modify EnsureNanoVG so that after the failed gladLoadGL() call you log the error and immediately exit the function (preventing the subsequent call to nvgCreateGL3), ensuring m_nvg is not created and TextRenderer::LoadFont is only called when m_nvg is valid.
98-119:⚠️ Potential issue | 🟠 MajorMissing
last_mmb_click_xinitialization in constructor.Line 119 initializes
last_mmb_click_yto-1, butlast_mmb_click_xis left uninitialized. This will cause undefined behavior if middle-click coordinates are read before the first middle-click event.Proposed fix
last_click_x(-1), last_click_y(-1), + last_mmb_click_x(-1), last_mmb_click_y(-1) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.cpp` around lines 98 - 119, The MapCanvas constructor leaves the member last_mmb_click_x uninitialized; update the initializer list in MapCanvas::MapCanvas to initialize last_mmb_click_x to -1 alongside last_mmb_click_y so middle-click coordinates are defined before first use (refer to the MapCanvas constructor and the last_mmb_click_x/last_mmb_click_y members to locate the change).
317-326:⚠️ Potential issue | 🔴 CriticalDo not render when GL context activation fails.
If
m_glContextexists butEnsureContextCurrent()fails,OnPaint()still proceeds intoEnsureNanoVG()and the full render path. Both GLAD and NanoVG require an active GL context, and proceeding without one causes undefined behavior.Proposed fix
void MapCanvas::OnPaint(wxPaintEvent& event) { wxPaintDC dc(this); // validates the paint event - if (m_glContext) { - g_gl_context.EnsureContextCurrent(*m_glContext, this); - g_gl_context.SetFallbackCanvas(this); + wxGLContext* context = m_glContext ? m_glContext.get() : g_gui.GetGLContext(this); + if (!context || !g_gl_context.EnsureContextCurrent(*context, this)) { + spdlog::error("MapCanvas::OnPaint: No valid GL context"); + return; } + g_gl_context.SetFallbackCanvas(this); EnsureNanoVG();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.cpp` around lines 317 - 326, In MapCanvas::OnPaint, guard the NanoVG/GL render path behind a successful GL context activation: call g_gl_context.EnsureContextCurrent(*m_glContext, this) and if it indicates failure, do not call g_gl_context.SetFallbackCanvas or EnsureNanoVG and instead return early (optionally log the failure). Move SetFallbackCanvas and the subsequent EnsureNanoVG/render calls inside the success branch so rendering only proceeds when EnsureContextCurrent succeeds.
605-617:⚠️ Potential issue | 🟠 MajorKeep property-click bookkeeping consistent with left-clicks.
last_click_xis not updated before being used to computelast_click_abs_x, andlast_click_map_zis not set. This leaves stale data that may affect subsequent operations relying on these coordinates.Proposed fix
selection_controller->HandlePropertiesClick( Position(mouse_map_x, mouse_map_y, view_state_->getFloor()), event.ShiftDown(), event.ControlDown(), event.AltDown() ); + last_click_x = int(event.GetX() * view_state_->getZoom()); last_click_y = int(event.GetY() * view_state_->getZoom()); int start_x, start_y; static_cast<MapWindow*>(GetParent())->GetViewStart(&start_x, &start_y); last_click_abs_x = last_click_x + start_x; last_click_abs_y = last_click_y + start_y; last_click_map_x = mouse_map_x; last_click_map_y = mouse_map_y; + last_click_map_z = view_state_->getFloor(); g_gui.RefreshView();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.cpp` around lines 605 - 617, The properties-click handler uses last_click_x to compute last_click_abs_x before last_click_x is updated and fails to set last_click_map_z, leaving stale state; fix by assigning last_click_x (using int(event.GetX() * view_state_->getZoom())) before computing last_click_abs_x/last_click_abs_y and also set last_click_map_z to view_state_->getFloor() (alongside last_click_map_x/last_click_map_y) in the same block after calling selection_controller->HandlePropertiesClick so the bookkeeping matches the left-click flow (references: selection_controller->HandlePropertiesClick, last_click_x/last_click_y, last_click_abs_x/last_click_abs_y, last_click_map_x/last_click_map_y/last_click_map_z, view_state_->getZoom(), view_state_->getFloor(), GetViewStart, g_gui.RefreshView).source/rendering/drawers/map_layer_drawer.cpp (1)
78-88:⚠️ Potential issue | 🟠 MajorMove
setRequestedinside the null-check guard — issue persists from previous review.Line 84 marks the node as requested unconditionally, but
enqueueat line 82 is guarded byif (pending_requests_). Ifpending_requests_is null, the request is never enqueued but the node is still marked as requested, leaving it permanently stuck showing the loading placeholder.Suggested fix
if (live && !nd->isVisible(map_z > GROUND_LAYER)) { if (!nd->isRequested(map_z > GROUND_LAYER)) { // Enqueue node request for deferred dispatch (after frame submission) if (pending_requests_) { pending_requests_->enqueue(nd_map_x, nd_map_y, map_z > GROUND_LAYER); + nd->setRequested(map_z > GROUND_LAYER, true); } - nd->setRequested(map_z > GROUND_LAYER, true); } grid_drawer->DrawNodeLoadingPlaceholder(sprite_batch, ctx.atlas, nd_map_x, nd_map_y, view); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/map_layer_drawer.cpp` around lines 78 - 88, The code marks the node requested unconditionally via nd->setRequested(map_z > GROUND_LAYER, true) even when pending_requests_ is null, leaving nodes stuck; move the nd->setRequested(...) call inside the pending_requests_ null-check so it only executes when pending_requests_->enqueue(nd_map_x, nd_map_y, map_z > GROUND_LAYER) runs (i.e., wrap setRequested together with enqueue inside the if (pending_requests_) block), keeping the nd->isRequested(...) check and DrawNodeLoadingPlaceholder logic unchanged.
🧹 Nitpick comments (2)
source/rendering/ui/view_state_manager.cpp (1)
26-36: Uncheckedstatic_castassumes parent is alwaysMapWindow.The
static_cast<MapWindow*>(canvas_->GetParent())lacks validation. IfGetParent()returns null or a different type, this leads to undefined behavior. Consider adding a debug assertion or null check for defensive safety.🛡️ Proposed defensive check
int ViewStateManager::getScrollX() const { int x = 0, y = 0; - static_cast<MapWindow*>(canvas_->GetParent())->GetViewStart(&x, &y); + auto* parent = canvas_->GetParent(); + assert(parent && "ViewStateManager requires canvas with valid parent"); + static_cast<MapWindow*>(parent)->GetViewStart(&x, &y); return x; } int ViewStateManager::getScrollY() const { int x = 0, y = 0; - static_cast<MapWindow*>(canvas_->GetParent())->GetViewStart(&x, &y); + auto* parent = canvas_->GetParent(); + assert(parent && "ViewStateManager requires canvas with valid parent"); + static_cast<MapWindow*>(parent)->GetViewStart(&x, &y); return y; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/view_state_manager.cpp` around lines 26 - 36, The current implementations of ViewStateManager::getScrollX and getScrollY unconditionally static_cast canvas_->GetParent() to MapWindow* which can UB if GetParent() is null or not a MapWindow; modify both methods to first obtain wxWindow* parent = canvas_->GetParent(), check for null, and use a safe runtime cast (e.g., dynamic_cast<MapWindow*> or wxDynamicCast) to verify the type; if the parent is null or the cast fails, return 0 (or a sensible default) and optionally add a debug assert/log message to aid diagnostics while preventing undefined behavior.source/rendering/core/game_sprite.h (1)
124-133: Keep sprite metadata updates atomic with invariant recomputation.These fields are now publicly writable, but
getIndex()/isSimpleAndLoaded()still depend onis_simple, and the atlas fast-path still has its own cache state. That turns correctness into a “remember to callupdateSimpleStatus()and invalidate caches” convention. A narrow metadata setter/builder would make this refactor safer.Also applies to: 169-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/game_sprite.h` around lines 124 - 133, Make sprite metadata updates atomic by removing direct public writes to the raw fields (id, height, width, layers, pattern_x, pattern_y, pattern_z, frames, numsprites) and add a single metadata setter/builder method (e.g., Sprite::setMetadata(...) or Sprite::updateMetadata(...)) that updates all fields together, calls the existing updateSimpleStatus() to recompute is_simple, and invalidates any atlas/fast-path caches used by getIndex() and isSimpleAndLoaded(); ensure the same change is applied for the other metadata block referenced around the 169-177 region so callers must go through the new method rather than writing fields directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/CMakeLists.txt`:
- Around line 245-250: rme_H is missing the header for the newly added
implementation view_state_manager.cpp; add the header
rendering/ui/view_state_manager.h to the rme_H file list in CMakeLists so the
header is tracked/installed alongside view_state_manager.cpp and other UI
headers (look for the rme_H variable and the nearby entries like tooltip_data.h,
tooltip_renderer.h to match formatting).
In `@source/rendering/core/game_sprite.cpp`:
- Around line 44-48: The cache eviction currently only clears 16x16 and 32x32
entries; add eviction for the 64x64 variant so stale outfits persist no longer:
in the same place where key.size is set to SPRITE_SIZE_16x16 and
SPRITE_SIZE_32x32 and parent->iconRenderer().eraseColoredDC(key) is called, also
set key.size = SPRITE_SIZE_64x64 and call
parent->iconRenderer().eraseColoredDC(key); ensure you reference the SpriteSize
constant SPRITE_SIZE_64x64 and update any related unloadDC() code paths so 64px
creature previews are cleared as well.
- Around line 85-89: In GameSprite::getSpriteId, getIndex is being called with
the sprite pixel dimensions (width, height) instead of the sprite cell counts,
which yields wrong indexes for multi-cell sprites; fix getSpriteId by passing
the sprite's cell count members (the pattern/column and row counts used
elsewhere for indexing — e.g., patternCountX/patternCountY or the members that
represent number of cells horizontally/vertically) into getIndex instead of
width and height so the index calculation matches how spriteList is organized
(leave the rest of the getIndex args and the spriteList/isNormalImage check
unchanged).
In `@source/rendering/core/normal_image.cpp`:
- Around line 43-53: Restore the parent cache invalidation before evicting the
atlas sprite: before calling g_gui.gfx.getAtlasManager()->removeSprite(id) save
the current atlas_region pointer (and any parent/composite pointer, e.g.,
parent) and call parent->invalidateCache(atlas_region) (or the appropriate
parent invalidation method) so external caches that memoized the AtlasRegion*
are cleared; then proceed to removeSprite(id), set isGLLoaded = false,
atlas_region = nullptr, bump generation_id, and call the texture-unloaded
notification; ensure this uses the same symbols found here (removeSprite(id),
atlas_region, generation_id, NormalImage::getAtlasRegion()) so external caches
cannot retain stale pointers.
In `@source/rendering/ui/view_state_manager.cpp`:
- Around line 18-20: The notify_scrollbar parameter on
ViewStateManager::changeFloor is ignored; either implement the missing scrollbar
notification or explicitly acknowledge it. Update changeFloor to pass
notify_scrollbar into the appropriate scrollbar update call (e.g., call
setFloor(new_floor) then invoke the existing scrollbar notification method such
as notifyScrollbar(notify_scrollbar) or updateScrollbarState(notify_scrollbar)),
or if intentional, add a brief comment explaining why notify_scrollbar is unused
or remove the parameter from the method signature and callers to avoid
confusion. Ensure references to ViewStateManager::changeFloor and setFloor are
updated accordingly.
---
Outside diff comments:
In `@source/rendering/core/sprite_preloader.cpp`:
- Around line 28-40: The shutdown() currently only signals worker threads via
worker.request_stop() and cv.notify_all(), but leaves std::vector<std::jthread>
workers to be destroyed later while task_queue, result_buffer_, and pending_ids
may still be alive; move the workers vector out and synchronously join/clear it
inside SpritePreloader::shutdown to ensure all threads exit before container
destruction: after setting stopping under task_mutex_ and notifying
(cv.notify_all()), swap or std::move workers into a local
std::vector<std::jthread> local_workers, then let local_workers go out of scope
(or explicitly join/clear it) so all jthreads are joined before the member
containers (task_queue, result_buffer_, pending_ids) are destroyed. Ensure you
reference the same worker.request_stop() calls and preserve notifying via
cv.notify_all() prior to moving workers.
---
Duplicate comments:
In `@source/rendering/drawers/map_layer_drawer.cpp`:
- Around line 78-88: The code marks the node requested unconditionally via
nd->setRequested(map_z > GROUND_LAYER, true) even when pending_requests_ is
null, leaving nodes stuck; move the nd->setRequested(...) call inside the
pending_requests_ null-check so it only executes when
pending_requests_->enqueue(nd_map_x, nd_map_y, map_z > GROUND_LAYER) runs (i.e.,
wrap setRequested together with enqueue inside the if (pending_requests_)
block), keeping the nd->isRequested(...) check and DrawNodeLoadingPlaceholder
logic unchanged.
In `@source/rendering/ui/map_display.cpp`:
- Around line 203-216: In MapCanvas::EnsureNanoVG, when gladLoadGL() fails you
must return early instead of continuing; modify EnsureNanoVG so that after the
failed gladLoadGL() call you log the error and immediately exit the function
(preventing the subsequent call to nvgCreateGL3), ensuring m_nvg is not created
and TextRenderer::LoadFont is only called when m_nvg is valid.
- Around line 98-119: The MapCanvas constructor leaves the member
last_mmb_click_x uninitialized; update the initializer list in
MapCanvas::MapCanvas to initialize last_mmb_click_x to -1 alongside
last_mmb_click_y so middle-click coordinates are defined before first use (refer
to the MapCanvas constructor and the last_mmb_click_x/last_mmb_click_y members
to locate the change).
- Around line 317-326: In MapCanvas::OnPaint, guard the NanoVG/GL render path
behind a successful GL context activation: call
g_gl_context.EnsureContextCurrent(*m_glContext, this) and if it indicates
failure, do not call g_gl_context.SetFallbackCanvas or EnsureNanoVG and instead
return early (optionally log the failure). Move SetFallbackCanvas and the
subsequent EnsureNanoVG/render calls inside the success branch so rendering only
proceeds when EnsureContextCurrent succeeds.
- Around line 605-617: The properties-click handler uses last_click_x to compute
last_click_abs_x before last_click_x is updated and fails to set
last_click_map_z, leaving stale state; fix by assigning last_click_x (using
int(event.GetX() * view_state_->getZoom())) before computing
last_click_abs_x/last_click_abs_y and also set last_click_map_z to
view_state_->getFloor() (alongside last_click_map_x/last_click_map_y) in the
same block after calling selection_controller->HandlePropertiesClick so the
bookkeeping matches the left-click flow (references:
selection_controller->HandlePropertiesClick, last_click_x/last_click_y,
last_click_abs_x/last_click_abs_y,
last_click_map_x/last_click_map_y/last_click_map_z, view_state_->getZoom(),
view_state_->getFloor(), GetViewStart, g_gui.RefreshView).
---
Nitpick comments:
In `@source/rendering/core/game_sprite.h`:
- Around line 124-133: Make sprite metadata updates atomic by removing direct
public writes to the raw fields (id, height, width, layers, pattern_x,
pattern_y, pattern_z, frames, numsprites) and add a single metadata
setter/builder method (e.g., Sprite::setMetadata(...) or
Sprite::updateMetadata(...)) that updates all fields together, calls the
existing updateSimpleStatus() to recompute is_simple, and invalidates any
atlas/fast-path caches used by getIndex() and isSimpleAndLoaded(); ensure the
same change is applied for the other metadata block referenced around the
169-177 region so callers must go through the new method rather than writing
fields directly.
In `@source/rendering/ui/view_state_manager.cpp`:
- Around line 26-36: The current implementations of ViewStateManager::getScrollX
and getScrollY unconditionally static_cast canvas_->GetParent() to MapWindow*
which can UB if GetParent() is null or not a MapWindow; modify both methods to
first obtain wxWindow* parent = canvas_->GetParent(), check for null, and use a
safe runtime cast (e.g., dynamic_cast<MapWindow*> or wxDynamicCast) to verify
the type; if the parent is null or the cast fails, return 0 (or a sensible
default) and optionally add a debug assert/log message to aid diagnostics while
preventing undefined behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e608815a-cb5c-44ec-9c21-59acd9217937
📒 Files selected for processing (18)
source/CMakeLists.txtsource/rendering/core/brush_snapshot.hsource/rendering/core/draw_frame.hsource/rendering/core/game_sprite.cppsource/rendering/core/game_sprite.hsource/rendering/core/normal_image.cppsource/rendering/core/normal_image.hsource/rendering/core/render_context.hsource/rendering/core/sprite_preloader.cppsource/rendering/core/sprite_preloader.hsource/rendering/core/texture_gc.hsource/rendering/drawers/map_layer_drawer.cppsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/view_state_manager.cppsource/rendering/ui/view_state_manager.h
💤 Files with no reviewable changes (1)
- source/rendering/core/normal_image.h
Comment on lines
+245
to
250
| ${CMAKE_CURRENT_LIST_DIR}/rendering/ui/tooltip_data.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/ui/tooltip_collector.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/ui/tooltip_renderer.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/ui/nvg_image_cache.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/ui/tooltip_data_extractor.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/ui/zoom_controller.h |
There was a problem hiding this comment.
Missing header: view_state_manager.h not added to rme_H.
The implementation file view_state_manager.cpp is added at line 596, but the corresponding header rendering/ui/view_state_manager.h is not included in the rme_H list. This may cause issues with IDE integration, header dependency tracking, or installation targets.
🔧 Proposed fix
${CMAKE_CURRENT_LIST_DIR}/rendering/ui/tooltip_renderer.h
${CMAKE_CURRENT_LIST_DIR}/rendering/ui/nvg_image_cache.h
${CMAKE_CURRENT_LIST_DIR}/rendering/ui/tooltip_data_extractor.h
+ ${CMAKE_CURRENT_LIST_DIR}/rendering/ui/view_state_manager.h
${CMAKE_CURRENT_LIST_DIR}/rendering/ui/zoom_controller.h🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/CMakeLists.txt` around lines 245 - 250, rme_H is missing the header
for the newly added implementation view_state_manager.cpp; add the header
rendering/ui/view_state_manager.h to the rme_H file list in CMakeLists so the
header is tracked/installed alongside view_state_manager.cpp and other UI
headers (look for the rme_H variable and the nearby entries like tooltip_data.h,
tooltip_renderer.h to match formatting).
Comment on lines
+44
to
+48
| key.size = SPRITE_SIZE_16x16; | ||
| parent->colored_dc.erase(key); | ||
| parent->iconRenderer().eraseColoredDC(key); | ||
|
|
||
| key.size = SPRITE_SIZE_32x32; | ||
| parent->colored_dc.erase(key); | ||
| parent->iconRenderer().eraseColoredDC(key); |
There was a problem hiding this comment.
Evict the 64x64 creature icon cache too.
Line 44 and Line 47 only clear the 16px and 32px variants, but SpriteSize still exposes SPRITE_SIZE_64x64. Any cached 64px creature preview will survive unloadDC() and can keep stale outfit colors alive.
Suggested fix
key.size = SPRITE_SIZE_32x32;
parent->iconRenderer().eraseColoredDC(key);
+
+ key.size = SPRITE_SIZE_64x64;
+ parent->iconRenderer().eraseColoredDC(key);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| key.size = SPRITE_SIZE_16x16; | |
| parent->colored_dc.erase(key); | |
| parent->iconRenderer().eraseColoredDC(key); | |
| key.size = SPRITE_SIZE_32x32; | |
| parent->colored_dc.erase(key); | |
| parent->iconRenderer().eraseColoredDC(key); | |
| key.size = SPRITE_SIZE_16x16; | |
| parent->iconRenderer().eraseColoredDC(key); | |
| key.size = SPRITE_SIZE_32x32; | |
| parent->iconRenderer().eraseColoredDC(key); | |
| key.size = SPRITE_SIZE_64x64; | |
| parent->iconRenderer().eraseColoredDC(key); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/game_sprite.cpp` around lines 44 - 48, The cache
eviction currently only clears 16x16 and 32x32 entries; add eviction for the
64x64 variant so stale outfits persist no longer: in the same place where
key.size is set to SPRITE_SIZE_16x16 and SPRITE_SIZE_32x32 and
parent->iconRenderer().eraseColoredDC(key) is called, also set key.size =
SPRITE_SIZE_64x64 and call parent->iconRenderer().eraseColoredDC(key); ensure
you reference the SpriteSize constant SPRITE_SIZE_64x64 and update any related
unloadDC() code paths so 64px creature previews are cleared as well.
Comment on lines
+43
to
+53
| if (g_gui.gfx.hasAtlasManager()) { | ||
| g_gui.gfx.getAtlasManager()->removeSprite(id); | ||
| } | ||
| if (parent) { | ||
| parent->invalidateCache(atlas_region); | ||
| } | ||
|
|
||
| isGLLoaded = false; | ||
| atlas_region = nullptr; | ||
|
|
||
| // Invalidate any pending preloads for this sprite ID | ||
| generation_id++; | ||
|
|
||
| g_gui.gfx.collector.NotifyTextureUnloaded(); | ||
| g_gui.gfx.gc().collector().NotifyTextureUnloaded(); |
There was a problem hiding this comment.
Restore parent-cache invalidation before evicting the atlas sprite.
Dropping the old parent->invalidateCache(atlas_region) call looks unsafe. After removeSprite(id), any parent/composite cache that memoized this AtlasRegion* can keep a stale pointer, and NormalImage::getAtlasRegion() cannot self-heal that external cache because it only checks this->atlas_region. This is a behavior change, not just a refactor.
Suggested fix
if (isGLLoaded && time - static_cast<time_t>(lastaccess.load(std::memory_order_relaxed)) > longevity) {
+ if (parent && atlas_region) {
+ parent->invalidateCache(atlas_region);
+ }
+
if (g_gui.gfx.hasAtlasManager()) {
g_gui.gfx.getAtlasManager()->removeSprite(id);
}
isGLLoaded = false;
atlas_region = nullptr;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/core/normal_image.cpp` around lines 43 - 53, Restore the
parent cache invalidation before evicting the atlas sprite: before calling
g_gui.gfx.getAtlasManager()->removeSprite(id) save the current atlas_region
pointer (and any parent/composite pointer, e.g., parent) and call
parent->invalidateCache(atlas_region) (or the appropriate parent invalidation
method) so external caches that memoized the AtlasRegion* are cleared; then
proceed to removeSprite(id), set isGLLoaded = false, atlas_region = nullptr,
bump generation_id, and call the texture-unloaded notification; ensure this uses
the same symbols found here (removeSprite(id), atlas_region, generation_id,
NormalImage::getAtlasRegion()) so external caches cannot retain stale pointers.
Comment on lines
+18
to
+20
| void ViewStateManager::changeFloor(int new_floor, bool notify_scrollbar) { | ||
| setFloor(new_floor); | ||
| } |
There was a problem hiding this comment.
Unused parameter: notify_scrollbar is ignored.
The changeFloor method accepts a notify_scrollbar parameter but doesn't use it. If this is intentional (e.g., deferred functionality), consider adding a comment. Otherwise, the scrollbar notification logic may be missing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/ui/view_state_manager.cpp` around lines 18 - 20, The
notify_scrollbar parameter on ViewStateManager::changeFloor is ignored; either
implement the missing scrollbar notification or explicitly acknowledge it.
Update changeFloor to pass notify_scrollbar into the appropriate scrollbar
update call (e.g., call setFloor(new_floor) then invoke the existing scrollbar
notification method such as notifyScrollbar(notify_scrollbar) or
updateScrollbarState(notify_scrollbar)), or if intentional, add a brief comment
explaining why notify_scrollbar is unused or remove the parameter from the
method signature and callers to avoid confusion. Ensure references to
ViewStateManager::changeFloor and setFloor are updated accordingly.
…awer contexts Summary: Unified rendering data structures and drawer signatures to improve encapsulation and maintainability. Changes: - `source/rendering/core/game_sprite.h` — encapsulated metadata into `SpriteMetadata` and privatized sprite lists - `source/rendering/ui/map_display.h` — bundled transient input fields into `InputState` struct - `source/rendering/core/graphics.h` — added explicit lifecycle methods for resetting and finalizing catalogs - `source/rendering/postprocess/effect_registry.h` — removed static auto-registration in favor of explicit getters - `source/rendering/core/texture_garbage_collector.h` — updated cleanup list to track sprite IDs instead of pointers - `source/rendering/core/render_view.h` — refactored `getScreenPosition` to return `TileScreenPos` struct - `source/rendering/core/atlas_region_cache.h` — added per-sprite cache to optimize frequent atlas lookups - `source/rendering/drawers/` — updated multiple drawers to use consolidated context objects for drawing - `.gitignore` — added `/protobuf` [minor]
…tecture **Summary:** Introduced a preparation thread and variant-based command queue to decouple frame state computation from GPU submission. Implemented parallel tile planning and double-buffered state to improve rendering performance. **Changes:** - `source/rendering/core/draw_command_queue.h` — [new] Introduced variant-based command queue for deferred drawing - `source/rendering/ui/render_loop.cpp` — [new] Implemented frame preparation thread and main render loop - `source/rendering/core/map_access.h` — [new] Defined IMapAccess interface to decouple drawers from Editor state - `source/rendering/drawers/map_layer_drawer.cpp` — Implemented parallel tile planning and command generation - `source/rendering/core/atlas_manager.cpp` — Added shared_mutex for thread-safe texture atlas access - `source/rendering/core/game_sprite.h` — Extracted animation and icon data into separate component structs - `source/rendering/core/sprite_database.cpp` — Implemented component syncing for thread-safe sprite lookups - `source/rendering/core/spsc_queue.h` — [new] Implemented lock-free Single-Producer Single-Consumer queue - `source/rendering/map_drawer.cpp` — Implemented double-buffering and draw command submission - `source/rendering/ui/map_display.cpp` — Refactored MapCanvas to use RenderLoop and GLContextManager - `source/rendering/drawers/overlays/brush_overlay_drawer.cpp` — Delegated brush logic to specialized sub-drawers - `source/rendering/drawers/tiles/tile_renderer.cpp` — Added command queueing and state merging support - `source/rendering/ui/gl_context_manager.cpp` — [new] Centralized management of GL and NanoVG contexts - `source/rendering/core/frame_builder.cpp` — [new] Extracted DrawFrame construction and ViewState computation - `source/CMakeLists.txt` — Added new rendering and UI source files [minor]
…aded pipeline Replaced global g_gui and g_settings access with dependency injection across the rendering core. Established data structures and loop logic to support an upcoming snapshot-driven threaded rendering pipeline. Changes: - source/rendering/core/graphics_runtime_config.h — [added] Created struct to hold rendering settings independently of global state - source/rendering/core/prepared_frame_buffer.h — [added] Created buffer for cross-thread handoff of prepared rendering data - source/rendering/ui/map_display.cpp — Injected GUI/Settings into MapCanvas and added proxy methods to bypass globals - source/rendering/ui/render_loop.cpp — Integrated PreparedFrameBuffer into the frame loop and added prep thread logic - source/rendering/core/graphics.cpp — Migrated texture GC and management to use local GraphicsRuntimeConfig - source/rendering/core/image.cpp — Decoupled atlas and resident image tracking from global graphics manager - source/rendering/core/game_sprite.cpp — Propagated GraphicManager dependencies to animator and image sub-components - source/rendering/core/sprite_batch.cpp — Parameterized SharedGeometry initialization to remove global state access - source/app/preferences/graphics_page.cpp — Added experimental UI toggle and warning for threaded rendering prep - source/rendering/ui/brush_selector.cpp — Refactored static handles to accept explicit GUI and Settings context
Introduced a decoupled rendering architecture using snapshots to enable safe multi-threaded frame preparation. Replaced direct game object access in the render loop with a validation-backed planning system using a persistent worker pool. Changes: - `source/rendering/core/render_prep_snapshot.h` — [new] added struct for thread-safe frame state capturing - `source/rendering/core/render_validation_layer.cpp` — [new] implemented shadow validation for threaded rendering results - `source/rendering/core/tile_planning_pool.cpp` — [new] implemented persistent worker pool for parallel tile planning - `source/rendering/core/tile_render_snapshot.h` — [new] defined POD structures for tiles, items, and creatures - `source/rendering/core/tile_render_snapshot_builder.cpp` — [new] implemented conversion from game objects to snapshots - `source/rendering/core/draw_command_queue.h` — updated draw commands to use snapshots and added `DrawFilledRectCmd` - `source/rendering/core/prepared_frame_buffer.h` — expanded to carry floor ranges and sprite preload requests - `source/rendering/drawers/map_layer_drawer.cpp` — refactored layer drawing to use snapshot-driven planning - `source/rendering/drawers/tiles/tile_renderer.cpp` — decoupled tile planning from live game state using snapshots - `source/rendering/drawers/entities/item_drawer.cpp` — implemented `BlitItemSnapshot` for rendering from snapshots - `source/rendering/ui/render_loop.cpp` — integrated planning pool and enabled the threaded rendering pipeline - `source/rendering/ui/tooltip_data.h` — converted tooltip fields to owned strings for thread safety - `source/rendering/utilities/pattern_calculator.h` — added pattern calculation support for item snapshots - `source/CMakeLists.txt` — added new rendering core files to build
…Snapshot Optimized the rendering pipeline through plan reuse, memory pre-allocation, and throttled UI updates. Decoupled ItemRenderSnapshot from ItemDefinitionView by replacing it with explicit flags to improve data locality. Changes: - source/rendering/core/tile_render_snapshot.h: replaced definition pointer with primitive flags and added command count estimation - source/rendering/drawers/map_layer_drawer.cpp: implemented plan reuse and command buffer reservation - source/rendering/drawers/tiles/tile_draw_plan.h: unified draw command structures and added pre-allocation - source/rendering/ui/map_display.cpp: throttled hover UI updates and optimized mouse move handling - source/rendering/core/tile_render_snapshot_builder.cpp: added memory reservations for various snapshot vectors - source/rendering/drawers/tiles/tile_renderer.cpp: updated planning to use global command types and move semantics - source/rendering/drawers/entities/item_drawer.cpp: updated blitting logic to use snapshot flags and added client ID sprite resolution - source/rendering/ui/navigation_controller.cpp: updated drag handling to return scroll status - source/rendering/core/render_validation_layer.h: [minor] updated sample interval defaults - source/rendering/utilities/pattern_calculator.h: [minor] updated to use snapshot flags
Optimized the rendering pipeline by implementing in-place command construction and moving tile-specific side effects to floor-level snapshots to reduce allocations and memory overhead. - source/rendering/core/draw_command_queue.h — added constructors and emplace methods for in-place construction - source/rendering/core/tile_render_snapshot.h — moved side-effect vectors from TileRenderSnapshot to VisibleFloorSnapshot - source/rendering/core/tile_render_snapshot_builder.cpp — refactored Build to BuildInto for direct emplacement into floor snapshots - source/rendering/drawers/map_layer_drawer.cpp — implemented capacity estimation and pre-emptive memory reservation for plans and commands - source/rendering/drawers/tiles/tile_renderer.cpp — refactored planning to use emplace and centralized side-effect accumulation - source/rendering/core/frame_accumulators.h — updated reserve method to include tooltip capacity - source/rendering/drawers/tiles/tile_draw_plan.h — updated reserve to include tooltip capacity - source/rendering/ui/tooltip_collector.h — added reserve method [minor]
Summary: Implemented local coordinate offsets for render commands to handle elevation stacking during queuing. Enabled transparency for client version 1098. Changes: - data/clients.toml — enabled transparency for client version 1098 - source/rendering/core/draw_command_queue.h — added local draw offsets to Item, Creature, and Marker commands - source/rendering/drawers/tiles/tile_renderer.cpp — implemented draw height calculation and offset accumulation during queuing - source/rendering/map_drawer.cpp — applied local offsets during command execution and added debug logging - source/rendering/core/prepared_render_chunk_builder.cpp — [minor] updated tile renderer call signature - source/rendering/drawers/map_layer_drawer.cpp — [minor] initialized render variant in chunk snapshots
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
2 participants
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.
Summary
Builds on the Tier 2 god-class decomposition branch with 10 targeted refinements following Single Responsibility Principle (SRP) and Data-Oriented Design (DOD) patterns:
Quick Wins (QW-1 through QW-10):
MapDrawer::current_atlas_backing field, redundantHasTransparentPixels())BrushVisualSettingsfromRenderSettings(cursor color concern separation)DrawContextconstruction into a helperSpritePreloadQueue::processAll()with null-preloader assertionISpriteResolver/GraphicsSpriteResolverLightDraweriteration to useLightBuffer::size()Medium Refactors (MR-1 through MR-8):
g_guidependency fromSpritePreloaderviaGraphicManager*setterSpriteDrawer::atlas_member — pass atlas through method paramsFrameOptions::ComputeHighlightPulse()BrushOverlayContextstruct to reduce 13-parameterdraw()signaturePlanTile()intoPlanGroundItem()andPlanStackedItems()sub-functionsGameSpriteClearFrameOverlays()→BeginFrame()for clarity29 files changed, 415 insertions, 320 deletions — all behavioral changes are zero (pure refactoring).
Test plan
grep -r "g_gui" source/rendering/core/sprite_preloaderreturns zero hitsSpriteDrawerno longer hasatlas_member orSetAtlas()method🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes