refactor(rendering): Tier 3 eliminate hidden state and coupling - #975
Open
pubgkreiss-oss wants to merge 29 commits into
Open
refactor(rendering): Tier 3 eliminate hidden state and coupling#975pubgkreiss-oss wants to merge 29 commits into
pubgkreiss-oss wants to merge 29 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the rendering architecture through introduction of context-driven APIs, decomposition of graphics management, and reorganization of rendering state. Key changes include build system upgrades (vcpkg, Ninja), sprite/graphics subsystem modularization (SpriteDatabase, AtlasLifecycle, TextureGC, SpriteLoaderState), replacement of DrawingOptions with RenderSettings/FrameOptions, introduction of DrawContext aggregation, sprite resolver abstraction, post-processing pipeline modernization, and light rendering improvements via LightFBO/LightShader. Changes
Sequence Diagram(s)The scale and diversity of changes (encompassing new architectural abstractions like DrawContext/ViewSnapshot, modularized component decomposition, updated rendering pipelines, and refactored tooltip/light systems) warrant a diagram showing the primary per-frame rendering flow: sequenceDiagram
participant MapCanvas as MapCanvas
participant MapDrawer as MapDrawer
participant TileRenderer as TileRenderer
participant DrawContext as DrawContext (per-tile)
participant FrameAccumulators as FrameAccumulators
participant PostProcessPipeline as PostProcessPipeline
MapCanvas->>MapDrawer: OnPaint()
MapCanvas->>MapCanvas: Construct ViewSnapshot
MapDrawer->>MapDrawer: SetupVars(ViewSnapshot)
MapDrawer->>MapDrawer: Construct DrawContext<br/>(sprite_batch, view, settings, frame, accumulators)
MapDrawer->>MapDrawer: Begin(PostProcessPipeline)
loop For each visible tile
MapDrawer->>TileRenderer: DrawTile(DrawContext, location, ...)
TileRenderer->>FrameAccumulators: Collect tooltips, doors, hooks, creature_names
end
MapDrawer->>FrameAccumulators: getTooltips/getDoors/getHooks
MapDrawer->>PostProcessPipeline: End() (render FBO to screen)
MapDrawer->>MapCanvas: Return
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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 | 🟡 MinorPrefer explicit fallback to ShaderNames::NONE instead of index-based default.
Line 36 defaults to
SetSelection(0)if the saved shader is not found, which assumes the choice control has at least one entry and relies on positional ordering. Instead, explicitly fall back toShaderNames::NONE, which is always registered at static initialization. This makes the intent clearer and prevents accidental breakage if the shader order changes.Suggested fix
const auto current_shader = wxstr(g_settings.getString(Config::SCREEN_SHADER)); const int shader_index = screen_shader_choice->FindString(current_shader); - screen_shader_choice->SetSelection(shader_index != wxNOT_FOUND ? shader_index : 0); + const int default_index = screen_shader_choice->FindString(ShaderNames::NONE); + screen_shader_choice->SetSelection( + shader_index != wxNOT_FOUND ? shader_index : + default_index != wxNOT_FOUND ? default_index : 0 + );🤖 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 code currently falls back to SetSelection(0) when the saved shader name isn't found; instead, compute a clear fallback index by finding the index of ShaderNames::NONE and use that when shader_index == wxNOT_FOUND. Update the selection logic around EffectRegistry::GetRegisteredNames()/screen_shader_choice: keep using wxstr(g_settings.getString(Config::SCREEN_SHADER)) and screen_shader_choice->FindString(current_shader) but if not found call screen_shader_choice->FindString(wxstr(ShaderNames::NONE)) and pass that index to screen_shader_choice->SetSelection (ensuring you use ShaderNames::NONE as the explicit fallback rather than a hardcoded 0).source/rendering/ui/map_display.cpp (1)
265-270:⚠️ Potential issue | 🟡 MinorAlways rebuild
DrawingOptionsbefore applying capture overrides.When capture is active, this path only flips
ingameon the existing object. The rest of the fields stay whatever the previous frame left behind, so screenshot rendering can drift from the current settings snapshot. Build fromFromSettings(...)every frame, then callSetIngame()if needed.Proposed fix
DrawingOptions& options = drawer->getOptions(); - if (screenshot_controller->IsCapturing()) { - options.SetIngame(); - } else { - options = DrawingOptions::FromSettings(g_settings, g_gui.GetLightIntensity(), g_gui.GetAmbientLightLevel()); - } + options = DrawingOptions::FromSettings(g_settings, g_gui.GetLightIntensity(), g_gui.GetAmbientLightLevel()); + if (screenshot_controller->IsCapturing()) { + options.SetIngame(); + }🤖 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 265 - 270, The current code mutates the existing DrawingOptions obtained via drawer->getOptions() and only calls SetIngame() when screenshot_controller->IsCapturing(), which leaves other fields stale; instead always construct a fresh DrawingOptions from DrawingOptions::FromSettings(g_settings, g_gui.GetLightIntensity(), g_gui.GetAmbientLightLevel()) each frame and then, if screenshot_controller->IsCapturing() is true, call SetIngame() on that newly built object before assigning it back to the drawer (use the same drawer->getOptions()/setter flow to replace the options).
🧹 Nitpick comments (24)
source/rendering/core/sprite_preloader.cpp (1)
193-205: Consider caching theimages()reference to avoid repeated method chain calls.The
g_gui.gfx.db().images()chain is called twice—once for the bounds check and once for element access. While this likely returns a reference and is functionally correct, caching it improves clarity and avoids redundant calls.♻️ Suggested refactor
- if (res.archive == current_archive && !graphics_unloaded && id < g_gui.gfx.db().images().size()) { - auto& img_ptr = g_gui.gfx.db().images()[id]; + const auto& images = g_gui.gfx.db().images(); + if (res.archive == current_archive && !graphics_unloaded && id < images.size()) { + const auto& img_ptr = images[id];🤖 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 193 - 205, The code repeatedly calls g_gui.gfx.db().images() for size() and indexing; cache that reference to avoid duplicate method chains by storing a local reference (e.g., auto& images = g_gui.gfx.db().images()) before the if, then use images.size() and images[id] for the bounds check and element access; keep the existing logic around current_archive, graphics_unloaded, id, img_ptr, static_cast to NormalImage*, and the img->fulfillPreload(std::move(res.data)) call unchanged.source/rendering/ui/map_display.h (1)
168-168: Prefer a narrower dependency than a publicMapWindow*accessor.This removes the
friend, but it also lets any caller reach back into the full parent window, which keeps the coupling in the public API. If the new call sites only need a few services, passing those explicitly would stay closer to the Tier 3 goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.h` at line 168, Publicly exposing MapWindow* via GetMapWindow() leaks the full parent window and keeps high coupling; instead create and return a narrow interface or explicit accessors for only the services consumers need (e.g. IMapWindowServices or specific methods like GetTileProvider(), RequestRepaint(), or GetCoordinateTransformer()) and update callers to use those methods; replace or remove GetMapWindow() and the friend usage, add the new minimal interface (or individual getters) to the class declared in map_display.h, and adjust all call sites that used GetMapWindow() to depend on the new narrow API.source/rendering/utilities/light_drawer.cpp (1)
117-121: Inconsistent: wxColor still used for light buffer colors.The
global_colorparameter was refactored toDrawColor, but individual light colors still usewxColorviacolorFromEightBit(). Consider refactoringcolorFromEightBit()to returnDrawColorfor consistency, or document this as a follow-up task.♻️ Suggested approach
Create or update a helper that returns
DrawColor:// Option 1: New helper returning DrawColor DrawColor drawColorFromEightBit(uint8_t color); // Then use it here: DrawColor c = drawColorFromEightBit(light.color); gpu_lights_.push_back({ .position = { screen_x, screen_y }, .intensity = static_cast<float>(light.intensity), .padding = 0.0f, .color = { (c.r / 255.0f) * light_intensity, (c.g / 255.0f) * light_intensity, (c.b / 255.0f) * light_intensity, 1.0f } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/utilities/light_drawer.cpp` around lines 117 - 121, colorFromEightBit is still returning wxColor while the codebase now uses DrawColor; update or add a helper (e.g., drawColorFromEightBit) to return DrawColor and use it in light_drawer.cpp where gpu_lights_ is populated. Replace the wxColor usage in the block that calls colorFromEightBit with the new DrawColor-returning helper, then map its r/g/b fields (divide by 255.0f and multiply by light_intensity) into the .color initializer for the gpu_lights_ entry so the light buffer consistently uses DrawColor across the codebase..github/workflows/build.yml (1)
55-63: Include the pinned vcpkg revision in the cache key.The installed-tree cache is keyed only by the manifest files, so changing
vcpkgGitCommitIdcan restore packages built against a different ports baseline. Including the pinned commit, and ideally the triplet, keeps the restored tree aligned with the toolchain this job is configuring.Suggested fix
- key: vcpkg-installed-${{ runner.os }}-${{ hashFiles('vcpkg.json', 'vcpkg-configuration.json') }} + key: vcpkg-installed-${{ runner.os }}-x64-linux-1940ee77e81573713c0d364c42f5990172198be1-${{ hashFiles('vcpkg.json', 'vcpkg-configuration.json') }} restore-keys: | vcpkg-installed-${{ runner.os }}-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 55 - 63, The cache key for the "Cache vcpkg installed packages" step must include the pinned vcpkg revision (vcpkgGitCommitId) and ideally the triplet so restored installed trees match the configured toolchain; update the actions/cache `key` (and optionally `restore-keys`) to append `${{ env.vcpkgGitCommitId }}` (or the workflow variable name used for vcpkgGitCommitId) and the triplet variable (e.g., `${{ env.VCPKG_DEFAULT_TRIPLET }}`) to the current `vcpkg-installed-${{ runner.os }}-${{ hashFiles(...) }}` value so the cache is partitioned by revision and triplet for the vcpkg install tree.source/rendering/core/atlas_lifecycle.h (1)
27-27: Consider adding a const overload forget().The
get()method doesn't modifyAtlasLifecyclestate, so it could beconst. Alternatively, provide both const and non-const overloads to support const-correct usage patterns.♻️ Suggested const-correctness improvement
public: - AtlasManager* get() { return atlas_manager_.get(); } + AtlasManager* get() const { return atlas_manager_.get(); } bool has() const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/atlas_lifecycle.h` at line 27, The get() accessor in AtlasLifecycle should be made const-correct: add a const overload AtlasManager* get() const (or change the existing signature to const if no callers rely on non-const) that returns atlas_manager_.get(); also keep or provide the non-const AtlasManager* get() for mutation contexts so both const and non-const callers are supported; update the declaration and definition of AtlasLifecycle::get to include the const variant and ensure callers compile against the new overloads.source/rendering/core/sprite_loader_state.h (1)
35-45: Inconsistent member naming:sprite_archive_has trailing underscore while others don't.The member
sprite_archive_uses a trailing underscore suffix, but other members (spritefile,client_version,dat_format, etc.) do not. Consider aligning the naming convention.🤖 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` around lines 35 - 45, The member name sprite_archive_ is inconsistent with the class's other members; rename it to sprite_archive to match the naming convention (no trailing underscore) and update all references/usages accordingly (e.g., constructors, methods, accessors, and any places that refer to SpriteArchive or sprite_archive_); ensure you also update forward declarations or initialization sites that use sprite_archive_ and run a build to catch any remaining references.source/rendering/drawers/tiles/shade_drawer.cpp (1)
13-18: Move includes to the top of the file.Includes are placed after the constructor/destructor definitions, which is unconventional and reduces readability. Standard practice is to group all includes at the top of the file.
📁 Suggested include reorganization
`#include` "app/main.h" // glut include removed `#include` "rendering/drawers/tiles/shade_drawer.h" +#include "rendering/core/draw_context.h" +#include "rendering/core/render_view.h" +#include "rendering/core/drawing_options.h" +#include "rendering/core/sprite_batch.h" +#include "rendering/core/graphics.h" +#include "ui/gui.h" ShadeDrawer::ShadeDrawer() { } ShadeDrawer::~ShadeDrawer() { } -#include "rendering/core/draw_context.h" -#include "rendering/core/render_view.h" -#include "rendering/core/drawing_options.h" -#include "rendering/core/sprite_batch.h" -#include "rendering/core/graphics.h" -#include "ui/gui.h" - void ShadeDrawer::draw(const DrawContext& ctx) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/tiles/shade_drawer.cpp` around lines 13 - 18, The file's `#include` directives (e.g., rendering/core/draw_context.h, render_view.h, drawing_options.h, sprite_batch.h, graphics.h, ui/gui.h) must be moved to the very top of shade_drawer.cpp before any function or method definitions (such as the ShadeDrawer constructor/destructor) to follow conventional include ordering and improve readability; group includes logically (system/third-party then project headers), remove any duplicate or unused includes, and ensure the constructor/destructor implementations remain below the relocated includes so compilation and ordering are preserved.source/rendering/ui/tooltip_data_extractor.h (1)
12-12: Minor naming inconsistency:isHouseTilevs other parameters.The parameter
isHouseTileuses camelCase while the codebase context suggests snake_case might be more consistent. Consider renaming tois_house_tilefor consistency, or keep as-is if camelCase is the project convention for boolean parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/tooltip_data_extractor.h` at line 12, Rename the boolean parameter isHouseTile in the TooltipDataExtractor::Fill declaration to is_house_tile and update the matching definition and all callers to use is_house_tile to maintain naming consistency; specifically change the parameter name in the declaration bool Fill(TooltipData& data, Item* item, const ItemDefinitionView& it, const Position& pos, bool is_house_tile, float zoom), then update the corresponding implementation of Fill and every place that calls Fill to pass/expect is_house_tile instead of isHouseTile.source/rendering/core/sprite_decompression.cpp (1)
143-152: Keep decode logging stateless/thread-safe.These
static intthrottles reintroduce hidden state into a low-level helper, and they are not thread-safe if decompression ever runs concurrently. Prefer an atomic counter or a caller/logger-owned rate limiter.🤖 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 - 152, The static int throttles in the logging branch (referencing non_zero_alpha_found, non_black_pixel_found and id) are stateful and not thread-safe; replace them with thread-safe atomics or delegate rate-limiting to the caller/logger. Concretely, change the file-local/static counters empty_log_count and black_log_count to std::atomic<int> and use fetch_add(1) (or atomic compare) to check the < 10 condition before calling spdlog, ensuring no data races during concurrent decompression in functions surrounding this logging code.source/rendering/drawers/cursors/live_cursor_drawer.cpp (1)
14-17: Finish the dependency cleanup for atlas access.
draw()now receives the per-frame context, but it still reaches back intog_gui.gfxlater in the method. That leaves a hidden rendering dependency behind and makes this drawer harder to reuse and test. Please thread atlas access throughDrawContext(or pass it explicitly) instead.Also applies to: 59-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/cursors/live_cursor_drawer.cpp` around lines 14 - 17, LiveCursorDrawer::draw still reaches into the global g_gui.gfx to fetch the texture atlas, leaving a hidden dependency; update the code to take the atlas from the per-frame DrawContext (or an explicit Atlas& param) and replace all g_gui.gfx uses in LiveCursorDrawer::draw (including the later atlas/texture calls around the section ~59-60) with ctx.atlas (or the new parameter). Ensure DrawContext's definition is extended to carry the atlas/texture handle and update all places that construct/pass DrawContext (or the draw call sites) so LiveCursorDrawer::draw uses only the passed-in context instead of g_gui.gfx.source/rendering/ui/tooltip_data_extractor.cpp (1)
81-122: Reset preview fields before the container branch.
containerCapacityandcontainerItemsare only touched inside the preview branch. IfTooltipDatais reused between calls, a zoomed-out or non-container tooltip can inherit stale preview data. Clearing them once up front keepsFill()self-contained.Suggested refactor
data.text = text; data.description = description; data.destination = destination; + data.containerCapacity = 0; + data.containerItems.clear(); // Populate container items if (it.isContainer() && zoom <= 1.5f) {🤖 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 81 - 122, The container preview fields data.containerCapacity and data.containerItems must be reset before the "Populate container items" branch to avoid leaking stale preview state when TooltipData is reused; update the Fill() path (immediately before the comment/branch that starts "Populate container items") to set data.containerCapacity = 0 (or an appropriate empty sentinel) and call data.containerItems.clear() so that non-container or zoomed-out tooltips do not retain previous container values.source/rendering/ui/tooltip_drawer.h (1)
22-22: Consider removing<iostream>from the header.Including
<iostream>in a header can increase compile times across all translation units that include this header. If it's only needed for debugging or implementation details, move it to the.cppfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/tooltip_drawer.h` at line 22, Remove the `#include` <iostream> from the header file tooltip_drawer.h to avoid increasing compile times; if any debugging or implementation uses std::cout/ostream, move that include into the corresponding tooltip_drawer.cpp and keep only headers required for the public API in tooltip_drawer.h (or use forward declarations where possible); ensure any inline functions in tooltip_drawer.h that currently depend on <iostream> are either moved to the .cpp or updated to avoid iostream types so compilation remains correct.source/rendering/drawers/overlays/preview_drawer.h (1)
22-22: Tie floor bounds to the floor they describe.
floor_paramsandmap_zhave to stay in sync, but this signature lets callers mix them up. A mismatched call would iterate one floor’s bounds while sampling another, and the already-long parameter list makes that harder to spot. Consider foldingmap_zintoFloorViewParamsor passing a single preview context so the API can’t drift out of sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/preview_drawer.h` at line 22, The public draw(...) in preview_drawer.h can accept mismatched floor data because map_z is separate from FloorViewParams; update the API so floor bounds and z cannot drift out of sync by folding map_z into FloorViewParams (or by introducing a single PreviewContext that contains FloorViewParams plus z) and then change the draw signature (function draw(...) in PreviewDrawer/preview_drawer.h) to accept the combined FloorViewParams (or PreviewContext) instead of a separate map_z parameter; update all callers to construct/populate the new FloorViewParams/PreviewContext and remove the standalone map_z argument so iterations and sampling always reference the same floor descriptor.source/rendering/drawers/tiles/tile_renderer.h (1)
23-35: Avoid default-null deps for required collaborators.
TileRenderDepscan now be default-constructed with every drawer/editor missing. If a required field is forgotten at a call site,TileRendererfails later insideDrawTileinstead of at construction. Make mandatory deps references or non-null wrappers, or validate them in the constructor and leave only truly optional deps nullable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/tiles/tile_renderer.h` around lines 23 - 35, TileRenderDeps currently defaults all collaborator pointers to nullptr which lets callers forget required drawers and pushes failures into TileRenderer::DrawTile; update TileRenderDeps and TileRenderer to enforce required dependencies by either (a) changing required pointer members (e.g., item_drawer, sprite_drawer, creature_drawer, creature_name_drawer, marker_drawer, tooltip_drawer, editor) to references or a non-null wrapper type, or (b) keeping nullable for truly optional collaborators and adding explicit validation in TileRenderer::TileRenderer(const TileRenderDeps& deps) that checks each required member and throws or asserts on null, leaving only optional deps nullable; adjust call sites to pass valid deps accordingly.source/rendering/drawers/tiles/floor_drawer.cpp (1)
24-24: CarryFloorViewParamsintoFloorDraweras well.This drawer still renders a non-current floor (
view.floor - 1) but only receivesViewState, so the loop below continues to depend on the implicitstart_x/end_xfields fromview. Passing the matching floor params here too would keep per-floor bounds explicit, likePreviewDrawer, and reduce the chance of the two code paths drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/tiles/floor_drawer.cpp` at line 24, The FloorDrawer::draw currently takes only ViewState and therefore uses implicit per-floor bounds (e.g., view.start_x/view.end_x) for a non-current floor; update FloorDrawer::draw to accept a FloorViewParams parameter (like PreviewDrawer does), replace usages of view.start_x/view.end_x (and any other per-floor bounds) with the supplied FloorViewParams fields, and propagate the new parameter to every caller of FloorDrawer::draw so the correct per-floor bounds are passed in; ensure any tests or callsites constructing FloorViewParams for the target floor (view.floor - 1) are updated accordingly.source/rendering/core/sprite_icon_renderer.cpp (1)
96-108: Consider using designated initializers forRenderKeyconstruction.The current field-by-field assignment is verbose. C++20 designated initializers (or aggregate initialization) would be more concise and less error-prone if fields are added later.
Proposed refactor using aggregate initialization
- RenderKey key; - key.size = size; - key.colorHash = outfit.getColorHash(); - key.mountColorHash = outfit.getMountColorHash(); - key.lookMount = outfit.lookMount; - key.lookAddon = outfit.lookAddon; - key.lookMountHead = outfit.lookMountHead; - key.lookMountBody = outfit.lookMountBody; - key.lookMountLegs = outfit.lookMountLegs; - key.lookMountFeet = outfit.lookMountFeet; + RenderKey key { + .size = size, + .colorHash = outfit.getColorHash(), + .mountColorHash = outfit.getMountColorHash(), + .lookMount = outfit.lookMount, + .lookAddon = outfit.lookAddon, + .lookMountHead = outfit.lookMountHead, + .lookMountBody = outfit.lookMountBody, + .lookMountLegs = outfit.lookMountLegs, + .lookMountFeet = outfit.lookMountFeet + };🤖 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 96 - 108, Replace the verbose field-by-field assignments in SpriteIconRenderer::getDC by constructing RenderKey with an aggregate or C++20 designated initializer (e.g., RenderKey key = { .size = size, .colorHash = outfit.getColorHash(), .mountColorHash = outfit.getMountColorHash(), .lookMount = outfit.lookMount, .lookAddon = outfit.lookAddon, .lookMountHead = outfit.lookMountHead, .lookMountBody = outfit.lookMountBody, .lookMountLegs = outfit.lookMountLegs, .lookMountFeet = outfit.lookMountFeet };), keeping the ASSERT(size ...) unchanged; this collapses the assignments into a single initializer expression and ensures future added fields are less error-prone to initialize.source/rendering/core/texture_gc.h (2)
37-40: Consider adding assertions for defensive programming.The inline methods
getElapsedTime(),pauseAnimation(), andresumeAnimation()dereferenceanimation_timer_without null checks. While the constructor always initializes it, adding assertions would catch misuse during maintenance.🛡️ Proposed defensive assertions
- long getElapsedTime() const { return animation_timer_->getElapsedTime(); } + long getElapsedTime() const { assert(animation_timer_); return animation_timer_->getElapsedTime(); } - void pauseAnimation() { animation_timer_->Pause(); } - void resumeAnimation() { animation_timer_->Resume(); } + void pauseAnimation() { assert(animation_timer_); animation_timer_->Pause(); } + void resumeAnimation() { assert(animation_timer_); animation_timer_->Resume(); }Note: Requires
#include <cassert>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/texture_gc.h` around lines 37 - 40, Add defensive assertions to the inline methods that dereference animation_timer_: in getElapsedTime(), pauseAnimation(), and resumeAnimation() assert(animation_timer_) before using it so misuse is caught early; also add `#include` <cassert> to the header and keep the existing behavior otherwise (references: getElapsedTime, pauseAnimation, resumeAnimation, animation_timer_).
49-50: Same consideration forpreloader()accessor.The
preloader()method dereferencespreloader_without a null check. An assertion would improve robustness.🛡️ Proposed defensive assertion
// Sprite preloader (background decompression threads) - SpritePreloader& preloader() { return *preloader_; } + SpritePreloader& preloader() { assert(preloader_); return *preloader_; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/texture_gc.h` around lines 49 - 50, The preloader() accessor dereferences preloader_ without a null check; add a defensive assertion or check at the start of preloader() (e.g., assert(preloader_) or an equivalent CHECK/VERIFY) to ensure preloader_ is non-null before returning *preloader_, so calls to SpritePreloader& preloader() safely assume a valid pointer.source/ingame_preview/ingame_preview_renderer.cpp (1)
176-177: Fix indentation:DrawTilecall appears misaligned.Line 177 should be indented to the same level as line 176 to maintain consistent code style within the nested block.
🔧 Proposed fix
const DrawContext ctx { *sprite_batch, *primitive_renderer, view, options, *light_buffer }; - tile_renderer->DrawTile(ctx, tile->location, 0, draw_x, draw_y, lighting_enabled); + tile_renderer->DrawTile(ctx, tile->location, 0, draw_x, draw_y, lighting_enabled);🤖 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 176 - 177, The DrawTile call is misaligned relative to the DrawContext construction; align the call to match the indentation of the const DrawContext line so both statements sit at the same nesting level. Locate the block where DrawContext ctx is created (symbol DrawContext and variable ctx) and adjust the indentation of the call to tile_renderer->DrawTile(ctx, tile->location, 0, draw_x, draw_y, lighting_enabled) to match that line’s indentation for consistent formatting.source/rendering/postprocess/effect_registry.h (1)
27-33: Consider reserving capacity for minor efficiency.Since the number of registrations is known, reserving space avoids reallocations.
♻️ Proposed optimization
inline std::vector<std::string> GetRegisteredNames() { std::vector<std::string> names; + names.reserve(Pending().size()); for (const auto& reg : Pending()) { names.push_back(reg.name); } return names; }🤖 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 27 - 33, GetRegisteredNames currently constructs names without reserving capacity, causing potential reallocations; modify GetRegisteredNames to reserve Pending().size() before pushing elements (use the Pending() call to get count), then emplace_back or push_back reg.name as before so the vector allocates once and avoids repeated growth.source/rendering/core/texture_gc.cpp (1)
48-50: Consider clarifyingclear()scope or making it comprehensive.
clear()only resetscollector_, leavingpreloader_unchanged. Callers must separately callpreloader().clear()(as seen ingraphics_assembler.cpp:109). This split could surprise maintainers.Consider either documenting this behavior or making
clear()comprehensive:♻️ Option A: Comprehensive clear
void TextureGC::clear() { collector_.Clear(); + preloader_->clear(); }♻️ Option B: Document current behavior
+// Clears the garbage collector only. Call preloader().clear() separately +// if preloader state should also be reset. void TextureGC::clear() { collector_.Clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/texture_gc.cpp` around lines 48 - 50, TextureGC::clear currently only resets collector_ leaving preloader_ intact, which surprises callers (e.g., callers that must call preloader().clear() separately); update the implementation so clear() is comprehensive by also clearing the preloader_ (call preloader_.clear() or preloader().clear() inside TextureGC::clear) and ensure any invariants are preserved, or alternatively add a clear() comment/docstring making explicit that it only clears collector_ and that callers must clear preloader_ themselves (choose one approach and apply consistently across TextureGC and its usages).source/rendering/core/graphics.h (3)
110-114: Sub-object accessors expose internal implementation details.These accessors are marked "for internal/friend use" in the comment, but they are
publicmethods. If this exposure is intentional for the refactoring transition, consider:
- Moving them to a
protectedorprivatesection with friend declarations for authorized classes, or- Adding a comment indicating these are transitional APIs to be removed/restricted later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/graphics.h` around lines 110 - 114, The public accessors db(), atlas(), loader(), and gc() leak internal implementation; move these methods into a protected or private section and expose them only to trusted classes via friend declarations (add friends for the classes that need SpriteDatabase, AtlasLifecycle, SpriteLoaderState, TextureGC), or if this is intentionally temporary, add a clear transitional API comment above these methods indicating they will be removed/restricted in a follow-up refactor; update any callers to use the friend classes or approved accessors accordingly (refer to the symbols db(), atlas(), loader(), gc(), db_, atlas_, loader_, gc_ to locate and change the declarations).
75-76: Consider documenting ownership semantics for raw-pointer overload.The raw-pointer
insertSprite(int id, Sprite* sprite)implicitly transfers ownership to aunique_ptr. While this likely exists for backward compatibility, callers may not realize they're giving up ownership.📝 Suggested documentation improvement
void insertSprite(int id, std::unique_ptr<Sprite> sprite) { db_.insertSprite(id, std::move(sprite)); } + // Legacy overload: takes ownership of the raw pointer void insertSprite(int id, Sprite* sprite) { db_.insertSprite(id, std::unique_ptr<Sprite>(sprite)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/graphics.h` around lines 75 - 76, Document that the raw-pointer overload insertSprite(int id, Sprite* sprite) transfers ownership to the function (it constructs a std::unique_ptr<Sprite>), so callers must not delete or use the pointer after calling; update the declaration/comments for both overloads (insertSprite(int, std::unique_ptr<Sprite>) and insertSprite(int, Sprite*)) to state the ownership transfer and recommend using the unique_ptr overload (and mark the raw-pointer overload as legacy/for-compatibility if appropriate) so callers are clear about lifetime semantics.
99-100: Reference member works correctly for backward compatibility, but consider a getter method for future flexibility.The current implementation is sound:
client_versioncorrectly referencesloader_.client_versionin the initializer list, andloader_is a stable member variable ofGraphicManager. The reference pattern prevents copying but is appropriate given the heavy object composition. If you want to avoid the constraints of a reference member (non-copyable, non-movable), a getter method would be a cleaner long-term approach:Optional: Use accessor methods instead of reference member
- // client_version exposed via loader for backward compatibility - ClientVersion*& client_version; + // client_version exposed via loader for backward compatibility + ClientVersion*& client_version() { return loader_.client_version; } + ClientVersion* const& client_version() const { return loader_.client_version; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/graphics.h` around lines 99 - 100, Current use of the reference member client_version (which aliases loader_.client_version in GraphicManager) is correct for backward compatibility but restricts copying/moving; replace the reference with an accessor to improve flexibility by removing the reference member and adding a getter method (e.g., ClientVersion& client_version() or const ClientVersion& client_version() const) on GraphicManager that returns loader_.client_version, update callers to use the new client_version() accessor, and remove the reference field and its initializer from the class to restore copy/move semantics.
🤖 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/graphics.cpp`:
- Around line 41-43: GraphicManager's client_version reference is being bound to
loader_.client_version before loader_ is constructed due to declaration order;
fix by moving the declaration of loader_ above client_version in the class so
loader_ is constructed first, or remove the reference member and expose
loader_.client_version via a getter (e.g., add a ClientVersion&
getClientVersion() or const accessor) and update the constructor and uses of
client_version accordingly; ensure the declaration order in the class matches
the dependency (loader_ then client_version) if you keep the reference.
In `@source/rendering/core/sprite_database.cpp`:
- Around line 71-74: SpriteDatabase::resize currently shrinks sprite_space_ and
image_space_ without touching the resident tracking collections, risking
dangling residency pointers; update resize(size_t sprite_size, size_t
image_size) to detect when sprite_size < sprite_space_.size() or image_size <
image_space_.size() and either reject the shrink (throw or return an error) or
invalidate/clear the resident caches before performing the resize (e.g. clear
whatever resident tracking containers you use for sprites and images), ensuring
you invalidate entries prior to calling sprite_space_.resize(...) and
image_space_.resize(...).
In `@source/rendering/core/sprite_icon_renderer.cpp`:
- Around line 24-25: DrawTo currently allows SPRITE_SIZE_64x64 but getDC asserts
only for SPRITE_SIZE_16x16/SPRITE_SIZE_32x32, causing debug crashes; either
extend getDC to accept and allocate/handle SPRITE_SIZE_64x64 (update its ASSERT
and buffer/bitmap sizing in getDC) or clamp/convert the draw size inside
SpriteIconRenderer::DrawTo before calling getDC (e.g., treat 64x64 as 32x32 or
split handling so getDC only ever sees 16/32). Locate and modify the methods
SpriteIconRenderer::DrawTo and SpriteIconRenderer::getDC (update the ASSERT and
related allocation/bitmap logic) so the size paths are consistent and 64x64 is
handled safely.
In `@source/rendering/core/sprite_icon_renderer.h`:
- Around line 53-60: The RenderKeyHash::operator() currently only mixes
colorHash, mountColorHash, lookMount, lookAddon, lookMountHead and
lookMountBody, leaving out RenderKey::size, lookMountLegs and lookMountFeet
which causes unnecessary collisions; update operator() in struct RenderKeyHash
to incorporate those three fields into the hash mix (e.g. feed size,
lookMountLegs and lookMountFeet into the same hash-combining sequence you
already use for other pairs or append additional std::hash<uint64_t> mixes with
the same 0x9e3779b9-based combine) so every field of RenderKey participates in
the final size_t result.
In `@source/rendering/core/sprite_loader_state.h`:
- Line 35: The current mixed atomic/non-atomic pattern around unloaded,
sprite_archive_, and spritefile is racy; fix by either (A) using proper
acquire/release semantics: when initializing in graphics_assembler.cpp write
non-atomic members (sprite_archive_, spritefile) first then set
unloaded.store(false, std::memory_order_release), and in all readers
(sprite_preloader.cpp and getSpriteArchive()/isUnloaded()) load unloaded with
std::memory_order_acquire and check isUnloaded() before accessing
sprite_archive_ or spritefile (or return the archive via an atomic-safe
accessor), or (B) replace this ad-hoc sync with a mutex protecting the entire
SpriteLoaderState so all readers/writers (getSpriteArchive(), isUnloaded(), code
in sprite_preloader.cpp and graphics_assembler.cpp) lock the mutex around
reads/writes to sprite_archive_ and spritefile; pick one approach and apply it
consistently.
In `@source/rendering/drawers/overlays/brush_overlay_drawer.cpp`:
- Around line 100-104: The draw method currently takes a nullable raw pointer
BrushCursorDrawer* brush_cursor_drawer but treats it as mandatory; change the
signature of BrushOverlayDrawer::draw to accept a reference (BrushCursorDrawer&
brush_cursor_drawer) so the contract is non-null by construction, and update all
call sites accordingly, or if you prefer a smaller change add an immediate
null-check/assert at the top of BrushOverlayDrawer::draw (e.g.,
assert(brush_cursor_drawer) or throw/LOG and return) to fail fast; update any
other overloads/usages noted in the file (also apply same change where the draw
overload appears around lines 377-399) so the parameter contract is consistent
across callers and implementations.
In `@source/rendering/drawers/overlays/brush_overlay_drawer.h`:
- Line 9: Remove the unused wx dependency by deleting the stray include
directive "<wx/colour.h>" from brush_overlay_drawer.h; the file does not use
wxColor/wxColour (it uses uint8_t triplets and glm::vec4 for colors in the brush
overlay structures), so simply remove the include to align with the rendering
decoupling goal and ensure no other references to wx::Colour remain in functions
or types in this header (check any code referencing wxColor/wxColour and replace
with the existing uint8_t/ glm::vec4 types if found).
In `@source/rendering/map_drawer.cpp`:
- Around line 252-273: The selection overlay is never rendered—restore a call to
selection_drawer->draw(...) in the frame overlay sequence (the member
initialized earlier as selection_drawer) so the selection is actually drawn; add
the call using the existing DrawContext ctx and the same
editor/canvas/selection-related objects used elsewhere (e.g. pass ctx, editor
and the canvas selection controller or its selection state) and place it after
brush_overlay_drawer->draw(...) (or just before the grid/ingame box draws) so
selection overlays render each frame.
In `@source/rendering/postprocess/post_process_manager.cpp`:
- Around line 7-10: PostProcessManager::LoadFromRegistry currently blindly calls
Register(...) for entries from EffectRegistry::Pending(), which when run after
the manager is initialized leaves newly-added effects without compiled shaders
because Initialize() returns early; either guard against post-init calls or
immediately initialize the new effects. Fix by checking the manager's
initialization state inside LoadFromRegistry (use whatever flag/method
Initialize() sets) and if already initialized, after Register(...) call the
shader setup routine for that effect (the same logic Initialize() uses to create
shaders/bindings), or alternatively reject/abort by asserting/logging that
PostProcessManager::LoadFromRegistry must be called before Initialize();
reference PostProcessManager::LoadFromRegistry, EffectRegistry::Pending(),
Register(), and Initialize() when making the change.
In `@source/rendering/ui/tooltip_data_extractor.cpp`:
- Line 25: Replace the sentinel check on Position destination (currently using
destination.x == 0) with an explicit bool has_destination flag: add a member
bool has_destination (default false) alongside Position destination, set
has_destination = true whenever destination is assigned (wherever Position
destination is populated in tooltip_data_extractor.cpp), set has_destination =
false when clearing/resetting the destination, and change the existing
hasDestination() method to return has_destination instead of inspecting
destination.x; update all code paths that relied on the x==0 sentinel to use
hasDestination() so valid positions at x==0 are preserved (apply same change to
the other occurrences referenced around the 55-72 region).
---
Outside diff comments:
In `@source/app/preferences/graphics_page.cpp`:
- Around line 31-36: The code currently falls back to SetSelection(0) when the
saved shader name isn't found; instead, compute a clear fallback index by
finding the index of ShaderNames::NONE and use that when shader_index ==
wxNOT_FOUND. Update the selection logic around
EffectRegistry::GetRegisteredNames()/screen_shader_choice: keep using
wxstr(g_settings.getString(Config::SCREEN_SHADER)) and
screen_shader_choice->FindString(current_shader) but if not found call
screen_shader_choice->FindString(wxstr(ShaderNames::NONE)) and pass that index
to screen_shader_choice->SetSelection (ensuring you use ShaderNames::NONE as the
explicit fallback rather than a hardcoded 0).
In `@source/rendering/ui/map_display.cpp`:
- Around line 265-270: The current code mutates the existing DrawingOptions
obtained via drawer->getOptions() and only calls SetIngame() when
screenshot_controller->IsCapturing(), which leaves other fields stale; instead
always construct a fresh DrawingOptions from
DrawingOptions::FromSettings(g_settings, g_gui.GetLightIntensity(),
g_gui.GetAmbientLightLevel()) each frame and then, if
screenshot_controller->IsCapturing() is true, call SetIngame() on that newly
built object before assigning it back to the drawer (use the same
drawer->getOptions()/setter flow to replace the options).
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 55-63: The cache key for the "Cache vcpkg installed packages" step
must include the pinned vcpkg revision (vcpkgGitCommitId) and ideally the
triplet so restored installed trees match the configured toolchain; update the
actions/cache `key` (and optionally `restore-keys`) to append `${{
env.vcpkgGitCommitId }}` (or the workflow variable name used for
vcpkgGitCommitId) and the triplet variable (e.g., `${{ env.VCPKG_DEFAULT_TRIPLET
}}`) to the current `vcpkg-installed-${{ runner.os }}-${{ hashFiles(...) }}`
value so the cache is partitioned by revision and triplet for the vcpkg install
tree.
In `@source/ingame_preview/ingame_preview_renderer.cpp`:
- Around line 176-177: The DrawTile call is misaligned relative to the
DrawContext construction; align the call to match the indentation of the const
DrawContext line so both statements sit at the same nesting level. Locate the
block where DrawContext ctx is created (symbol DrawContext and variable ctx) and
adjust the indentation of the call to tile_renderer->DrawTile(ctx,
tile->location, 0, draw_x, draw_y, lighting_enabled) to match that line’s
indentation for consistent formatting.
In `@source/rendering/core/atlas_lifecycle.h`:
- Line 27: The get() accessor in AtlasLifecycle should be made const-correct:
add a const overload AtlasManager* get() const (or change the existing signature
to const if no callers rely on non-const) that returns atlas_manager_.get();
also keep or provide the non-const AtlasManager* get() for mutation contexts so
both const and non-const callers are supported; update the declaration and
definition of AtlasLifecycle::get to include the const variant and ensure
callers compile against the new overloads.
In `@source/rendering/core/graphics.h`:
- Around line 110-114: The public accessors db(), atlas(), loader(), and gc()
leak internal implementation; move these methods into a protected or private
section and expose them only to trusted classes via friend declarations (add
friends for the classes that need SpriteDatabase, AtlasLifecycle,
SpriteLoaderState, TextureGC), or if this is intentionally temporary, add a
clear transitional API comment above these methods indicating they will be
removed/restricted in a follow-up refactor; update any callers to use the friend
classes or approved accessors accordingly (refer to the symbols db(), atlas(),
loader(), gc(), db_, atlas_, loader_, gc_ to locate and change the
declarations).
- Around line 75-76: Document that the raw-pointer overload insertSprite(int id,
Sprite* sprite) transfers ownership to the function (it constructs a
std::unique_ptr<Sprite>), so callers must not delete or use the pointer after
calling; update the declaration/comments for both overloads (insertSprite(int,
std::unique_ptr<Sprite>) and insertSprite(int, Sprite*)) to state the ownership
transfer and recommend using the unique_ptr overload (and mark the raw-pointer
overload as legacy/for-compatibility if appropriate) so callers are clear about
lifetime semantics.
- Around line 99-100: Current use of the reference member client_version (which
aliases loader_.client_version in GraphicManager) is correct for backward
compatibility but restricts copying/moving; replace the reference with an
accessor to improve flexibility by removing the reference member and adding a
getter method (e.g., ClientVersion& client_version() or const ClientVersion&
client_version() const) on GraphicManager that returns loader_.client_version,
update callers to use the new client_version() accessor, and remove the
reference field and its initializer from the class to restore copy/move
semantics.
In `@source/rendering/core/sprite_decompression.cpp`:
- Around line 143-152: The static int throttles in the logging branch
(referencing non_zero_alpha_found, non_black_pixel_found and id) are stateful
and not thread-safe; replace them with thread-safe atomics or delegate
rate-limiting to the caller/logger. Concretely, change the file-local/static
counters empty_log_count and black_log_count to std::atomic<int> and use
fetch_add(1) (or atomic compare) to check the < 10 condition before calling
spdlog, ensuring no data races during concurrent decompression in functions
surrounding this logging code.
In `@source/rendering/core/sprite_icon_renderer.cpp`:
- Around line 96-108: Replace the verbose field-by-field assignments in
SpriteIconRenderer::getDC by constructing RenderKey with an aggregate or C++20
designated initializer (e.g., RenderKey key = { .size = size, .colorHash =
outfit.getColorHash(), .mountColorHash = outfit.getMountColorHash(), .lookMount
= outfit.lookMount, .lookAddon = outfit.lookAddon, .lookMountHead =
outfit.lookMountHead, .lookMountBody = outfit.lookMountBody, .lookMountLegs =
outfit.lookMountLegs, .lookMountFeet = outfit.lookMountFeet };), keeping the
ASSERT(size ...) unchanged; this collapses the assignments into a single
initializer expression and ensures future added fields are less error-prone to
initialize.
In `@source/rendering/core/sprite_loader_state.h`:
- Around line 35-45: The member name sprite_archive_ is inconsistent with the
class's other members; rename it to sprite_archive to match the naming
convention (no trailing underscore) and update all references/usages accordingly
(e.g., constructors, methods, accessors, and any places that refer to
SpriteArchive or sprite_archive_); ensure you also update forward declarations
or initialization sites that use sprite_archive_ and run a build to catch any
remaining references.
In `@source/rendering/core/sprite_preloader.cpp`:
- Around line 193-205: The code repeatedly calls g_gui.gfx.db().images() for
size() and indexing; cache that reference to avoid duplicate method chains by
storing a local reference (e.g., auto& images = g_gui.gfx.db().images()) before
the if, then use images.size() and images[id] for the bounds check and element
access; keep the existing logic around current_archive, graphics_unloaded, id,
img_ptr, static_cast to NormalImage*, and the
img->fulfillPreload(std::move(res.data)) call unchanged.
In `@source/rendering/core/texture_gc.cpp`:
- Around line 48-50: TextureGC::clear currently only resets collector_ leaving
preloader_ intact, which surprises callers (e.g., callers that must call
preloader().clear() separately); update the implementation so clear() is
comprehensive by also clearing the preloader_ (call preloader_.clear() or
preloader().clear() inside TextureGC::clear) and ensure any invariants are
preserved, or alternatively add a clear() comment/docstring making explicit that
it only clears collector_ and that callers must clear preloader_ themselves
(choose one approach and apply consistently across TextureGC and its usages).
In `@source/rendering/core/texture_gc.h`:
- Around line 37-40: Add defensive assertions to the inline methods that
dereference animation_timer_: in getElapsedTime(), pauseAnimation(), and
resumeAnimation() assert(animation_timer_) before using it so misuse is caught
early; also add `#include` <cassert> to the header and keep the existing behavior
otherwise (references: getElapsedTime, pauseAnimation, resumeAnimation,
animation_timer_).
- Around line 49-50: The preloader() accessor dereferences preloader_ without a
null check; add a defensive assertion or check at the start of preloader()
(e.g., assert(preloader_) or an equivalent CHECK/VERIFY) to ensure preloader_ is
non-null before returning *preloader_, so calls to SpritePreloader& preloader()
safely assume a valid pointer.
In `@source/rendering/drawers/cursors/live_cursor_drawer.cpp`:
- Around line 14-17: LiveCursorDrawer::draw still reaches into the global
g_gui.gfx to fetch the texture atlas, leaving a hidden dependency; update the
code to take the atlas from the per-frame DrawContext (or an explicit Atlas&
param) and replace all g_gui.gfx uses in LiveCursorDrawer::draw (including the
later atlas/texture calls around the section ~59-60) with ctx.atlas (or the new
parameter). Ensure DrawContext's definition is extended to carry the
atlas/texture handle and update all places that construct/pass DrawContext (or
the draw call sites) so LiveCursorDrawer::draw uses only the passed-in context
instead of g_gui.gfx.
In `@source/rendering/drawers/overlays/preview_drawer.h`:
- Line 22: The public draw(...) in preview_drawer.h can accept mismatched floor
data because map_z is separate from FloorViewParams; update the API so floor
bounds and z cannot drift out of sync by folding map_z into FloorViewParams (or
by introducing a single PreviewContext that contains FloorViewParams plus z) and
then change the draw signature (function draw(...) in
PreviewDrawer/preview_drawer.h) to accept the combined FloorViewParams (or
PreviewContext) instead of a separate map_z parameter; update all callers to
construct/populate the new FloorViewParams/PreviewContext and remove the
standalone map_z argument so iterations and sampling always reference the same
floor descriptor.
In `@source/rendering/drawers/tiles/floor_drawer.cpp`:
- Line 24: The FloorDrawer::draw currently takes only ViewState and therefore
uses implicit per-floor bounds (e.g., view.start_x/view.end_x) for a non-current
floor; update FloorDrawer::draw to accept a FloorViewParams parameter (like
PreviewDrawer does), replace usages of view.start_x/view.end_x (and any other
per-floor bounds) with the supplied FloorViewParams fields, and propagate the
new parameter to every caller of FloorDrawer::draw so the correct per-floor
bounds are passed in; ensure any tests or callsites constructing FloorViewParams
for the target floor (view.floor - 1) are updated accordingly.
In `@source/rendering/drawers/tiles/shade_drawer.cpp`:
- Around line 13-18: The file's `#include` directives (e.g.,
rendering/core/draw_context.h, render_view.h, drawing_options.h, sprite_batch.h,
graphics.h, ui/gui.h) must be moved to the very top of shade_drawer.cpp before
any function or method definitions (such as the ShadeDrawer
constructor/destructor) to follow conventional include ordering and improve
readability; group includes logically (system/third-party then project headers),
remove any duplicate or unused includes, and ensure the constructor/destructor
implementations remain below the relocated includes so compilation and ordering
are preserved.
In `@source/rendering/drawers/tiles/tile_renderer.h`:
- Around line 23-35: TileRenderDeps currently defaults all collaborator pointers
to nullptr which lets callers forget required drawers and pushes failures into
TileRenderer::DrawTile; update TileRenderDeps and TileRenderer to enforce
required dependencies by either (a) changing required pointer members (e.g.,
item_drawer, sprite_drawer, creature_drawer, creature_name_drawer,
marker_drawer, tooltip_drawer, editor) to references or a non-null wrapper type,
or (b) keeping nullable for truly optional collaborators and adding explicit
validation in TileRenderer::TileRenderer(const TileRenderDeps& deps) that checks
each required member and throws or asserts on null, leaving only optional deps
nullable; adjust call sites to pass valid deps accordingly.
In `@source/rendering/postprocess/effect_registry.h`:
- Around line 27-33: GetRegisteredNames currently constructs names without
reserving capacity, causing potential reallocations; modify GetRegisteredNames
to reserve Pending().size() before pushing elements (use the Pending() call to
get count), then emplace_back or push_back reg.name as before so the vector
allocates once and avoids repeated growth.
In `@source/rendering/ui/map_display.h`:
- Line 168: Publicly exposing MapWindow* via GetMapWindow() leaks the full
parent window and keeps high coupling; instead create and return a narrow
interface or explicit accessors for only the services consumers need (e.g.
IMapWindowServices or specific methods like GetTileProvider(), RequestRepaint(),
or GetCoordinateTransformer()) and update callers to use those methods; replace
or remove GetMapWindow() and the friend usage, add the new minimal interface (or
individual getters) to the class declared in map_display.h, and adjust all call
sites that used GetMapWindow() to depend on the new narrow API.
In `@source/rendering/ui/tooltip_data_extractor.cpp`:
- Around line 81-122: The container preview fields data.containerCapacity and
data.containerItems must be reset before the "Populate container items" branch
to avoid leaking stale preview state when TooltipData is reused; update the
Fill() path (immediately before the comment/branch that starts "Populate
container items") to set data.containerCapacity = 0 (or an appropriate empty
sentinel) and call data.containerItems.clear() so that non-container or
zoomed-out tooltips do not retain previous container values.
In `@source/rendering/ui/tooltip_data_extractor.h`:
- Line 12: Rename the boolean parameter isHouseTile in the
TooltipDataExtractor::Fill declaration to is_house_tile and update the matching
definition and all callers to use is_house_tile to maintain naming consistency;
specifically change the parameter name in the declaration bool Fill(TooltipData&
data, Item* item, const ItemDefinitionView& it, const Position& pos, bool
is_house_tile, float zoom), then update the corresponding implementation of Fill
and every place that calls Fill to pass/expect is_house_tile instead of
isHouseTile.
In `@source/rendering/ui/tooltip_drawer.h`:
- Line 22: Remove the `#include` <iostream> from the header file tooltip_drawer.h
to avoid increasing compile times; if any debugging or implementation uses
std::cout/ostream, move that include into the corresponding tooltip_drawer.cpp
and keep only headers required for the public API in tooltip_drawer.h (or use
forward declarations where possible); ensure any inline functions in
tooltip_drawer.h that currently depend on <iostream> are either moved to the
.cpp or updated to avoid iostream types so compilation remains correct.
In `@source/rendering/utilities/light_drawer.cpp`:
- Around line 117-121: colorFromEightBit is still returning wxColor while the
codebase now uses DrawColor; update or add a helper (e.g.,
drawColorFromEightBit) to return DrawColor and use it in light_drawer.cpp where
gpu_lights_ is populated. Replace the wxColor usage in the block that calls
colorFromEightBit with the new DrawColor-returning helper, then map its r/g/b
fields (divide by 255.0f and multiply by light_intensity) into the .color
initializer for the gpu_lights_ entry so the light buffer consistently uses
DrawColor across the codebase.
| void clear(); | ||
|
|
||
| // All public — GraphicsAssembler writes directly to these. | ||
| std::atomic<bool> unloaded { true }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how unloaded atomic is used in relation to other members
# to verify thread-safety assumptions
rg -n -A5 -B2 'unloaded' --glob '*.cpp' --glob '*.h' | head -80Repository: karolak6612/remeres-map-editor-redux
Length of output: 4493
🏁 Script executed:
#!/bin/bash
# Check for concurrent access patterns and threading context
# Look for thread creation and synchronization patterns
echo "=== Checking for threading patterns ==="
rg -n 'thread|mutex|lock|atomic' --glob '*.cpp' --glob '*.h' | grep -E '(std::thread|std::mutex|std::lock|GraphicManager|SpriteLoaderState)' | head -30
# Look at GraphicManager class to understand synchronization
echo -e "\n=== Checking GraphicManager class structure ==="
rg -n -A20 'class GraphicManager' --glob '*.h'
# Check if loader() returns a reference that could be modified from different threads
echo -e "\n=== Checking loader() accessor method ==="
rg -n -B2 -A5 'loader()' --glob '*.h' | head -40Repository: karolak6612/remeres-map-editor-redux
Length of output: 15638
🏁 Script executed:
#!/bin/bash
# Check how sprite_archive_ and spritefile are accessed in relation to unloaded
echo "=== Checking sprite_archive_ access patterns ==="
rg -n 'sprite_archive_' --glob '*.cpp' --glob '*.h' | head -25
echo -e "\n=== Checking if there are mutexes protecting loader state ==="
rg -n 'mutable.*mutex\|std::mutex' source/rendering/ --glob '*.h' | head -20Repository: karolak6612/remeres-map-editor-redux
Length of output: 821
🏁 Script executed:
#!/bin/bash
# Check for concurrent access to GraphicManager/loader state
echo "=== Looking for GraphicManager being accessed from different contexts ==="
rg -n -B3 -A3 'g_gui.gfx.loader()' --glob '*.cpp' | head -40
echo -e "\n=== Checking isUnloaded usage context ==="
rg -n -B2 -A2 'isUnloaded()' --glob '*.cpp'Repository: karolak6612/remeres-map-editor-redux
Length of output: 1239
🏁 Script executed:
#!/bin/bash
# Check all callers of getSpriteArchive() to see if they check isUnloaded() first
echo "=== Checking getSpriteArchive() usage context ==="
rg -n -B3 -A3 'getSpriteArchive()' --glob '*.cpp' --glob '*.h' | head -60
echo -e "\n=== Checking the exact getter implementation ==="
rg -n -A2 'std::shared_ptr<SpriteArchive> getSpriteArchive' --glob '*.h'
echo -e "\n=== Checking if any write to unloaded happens without holding the state ==="
rg -n -B5 -A5 'unloaded = ' --glob '*.cpp'Repository: karolak6612/remeres-map-editor-redux
Length of output: 5964
Confirm thread-safety issue with mixed atomic/non-atomic members.
The concern is valid. unloaded serves as a synchronization guard for sprite_archive_ and spritefile, but the implementation has race conditions:
-
Read ordering violation: In
sprite_preloader.cpp:177-178,getSpriteArchive()is called before checkingisUnloaded(), reading a potentially stale pointer. -
Write ordering violation: In
graphics_assembler.cpp:153-155, non-atomic members are written before settingunloaded = false, exposing partially-initialized state. -
Unguarded read:
getSpriteArchive()(graphics.h:97) directly returns the non-atomic member without synchronization checks.
Document the expected memory ordering (acquire/release) and ensure readers check isUnloaded() before accessing sprite_archive_ or spritefile. Alternatively, protect the entire state with a mutex since the current atomic-only approach is incomplete.
🤖 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 35, The current mixed
atomic/non-atomic pattern around unloaded, sprite_archive_, and spritefile is
racy; fix by either (A) using proper acquire/release semantics: when
initializing in graphics_assembler.cpp write non-atomic members
(sprite_archive_, spritefile) first then set unloaded.store(false,
std::memory_order_release), and in all readers (sprite_preloader.cpp and
getSpriteArchive()/isUnloaded()) load unloaded with std::memory_order_acquire
and check isUnloaded() before accessing sprite_archive_ or spritefile (or return
the archive via an atomic-safe accessor), or (B) replace this ad-hoc sync with a
mutex protecting the entire SpriteLoaderState so all readers/writers
(getSpriteArchive(), isUnloaded(), code in sprite_preloader.cpp and
graphics_assembler.cpp) lock the mutex around reads/writes to sprite_archive_
and spritefile; pick one approach and apply it consistently.
Comment on lines
+100
to
+104
| void BrushOverlayDrawer::draw(const DrawContext& ctx, ItemDrawer* item_drawer, SpriteDrawer* sprite_drawer, CreatureDrawer* creature_drawer, BrushCursorDrawer* brush_cursor_drawer, Editor& editor, bool is_dragging_draw, int last_click_map_x, int last_click_map_y) { | ||
| auto& sprite_batch = ctx.sprite_batch; | ||
| auto& primitive_renderer = ctx.primitive_renderer; | ||
| const auto& view = ctx.view; | ||
| const auto& options = ctx.options; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make BrushCursorDrawer non-null by construction.
This dependency is now explicit, but the implementation still treats it as mandatory. Leaving it as a raw pointer means a missed call-site migration turns into a waypoint-only null dereference instead of a compile-time failure. Prefer a reference here, or at least assert non-null at entry.
Proposed contract-tightening diff
-void BrushOverlayDrawer::draw(const DrawContext& ctx, ItemDrawer* item_drawer, SpriteDrawer* sprite_drawer, CreatureDrawer* creature_drawer, BrushCursorDrawer* brush_cursor_drawer, Editor& editor, bool is_dragging_draw, int last_click_map_x, int last_click_map_y) {
+void BrushOverlayDrawer::draw(const DrawContext& ctx, ItemDrawer* item_drawer, SpriteDrawer* sprite_drawer, CreatureDrawer* creature_drawer, BrushCursorDrawer& brush_cursor_drawer, Editor& editor, bool is_dragging_draw, int last_click_map_x, int last_click_map_y) {- brush_cursor_drawer->draw(sprite_batch, primitive_renderer, cx, cy, brush, r, g, b);
+ brush_cursor_drawer.draw(sprite_batch, primitive_renderer, cx, cy, brush, r, g, b);- brush_cursor_drawer->draw(sprite_batch, primitive_renderer, cx, cy, brush, r, g, b);
+ brush_cursor_drawer.draw(sprite_batch, primitive_renderer, cx, cy, brush, r, g, b);Also applies to: 377-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/drawers/overlays/brush_overlay_drawer.cpp` around lines 100
- 104, The draw method currently takes a nullable raw pointer BrushCursorDrawer*
brush_cursor_drawer but treats it as mandatory; change the signature of
BrushOverlayDrawer::draw to accept a reference (BrushCursorDrawer&
brush_cursor_drawer) so the contract is non-null by construction, and update all
call sites accordingly, or if you prefer a smaller change add an immediate
null-check/assert at the top of BrushOverlayDrawer::draw (e.g.,
assert(brush_cursor_drawer) or throw/LOG and return) to fail fast; update any
other overloads/usages noted in the file (also apply same change where the draw
overload appears around lines 377-399) so the parameter contract is consistent
across callers and implementations.
Comment on lines
+7
to
+10
| void PostProcessManager::LoadFromRegistry() { | ||
| for (auto& reg : EffectRegistry::Pending()) { | ||
| Register(reg.name, reg.fragment_source, reg.vertex_source); | ||
| } |
There was a problem hiding this comment.
Prevent LoadFromRegistry() from registering post-init no-op effects.
This method can now be called after the manager is already initialized, but new entries only get pushed into effects; they never receive a shader because Initialize() returns early on Line 25. That makes late registry loads fail silently at lookup time. Either enforce pre-init usage here or initialize newly added effects immediately.
Minimal safeguard
void PostProcessManager::LoadFromRegistry() {
- for (auto& reg : EffectRegistry::Pending()) {
+ if (initialized) {
+ spdlog::error("PostProcessManager::LoadFromRegistry() must be called before Initialize().");
+ return;
+ }
+ for (const auto& reg : EffectRegistry::Pending()) {
Register(reg.name, reg.fragment_source, reg.vertex_source);
}
}Also applies to: 24-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/postprocess/post_process_manager.cpp` around lines 7 - 10,
PostProcessManager::LoadFromRegistry currently blindly calls Register(...) for
entries from EffectRegistry::Pending(), which when run after the manager is
initialized leaves newly-added effects without compiled shaders because
Initialize() returns early; either guard against post-init calls or immediately
initialize the new effects. Fix by checking the manager's initialization state
inside LoadFromRegistry (use whatever flag/method Initialize() sets) and if
already initialized, after Register(...) call the shader setup routine for that
effect (the same logic Initialize() uses to create shaders/bindings), or
alternatively reject/abort by asserting/logging that
PostProcessManager::LoadFromRegistry must be called before Initialize();
reference PostProcessManager::LoadFromRegistry, EffectRegistry::Pending(),
Register(), and Initialize() when making the change.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
source/rendering/map_drawer.cpp (2)
231-249: Begin/End pattern lacks exception safety.If any call between
BeginandEndthrows or returns early, the FBO remains bound and the viewport stays modified. Per thePostProcessPipelineimplementation, the destructor is defaulted and does not restore GL state.Consider wrapping the FBO scope in a RAII guard, or at minimum using a scope-exit pattern:
♻️ Suggested RAII approach
// Post-processing: bind FBO if shader or AA is active bool use_fbo = post_process_->Begin(view, options); + auto fbo_guard = [&]() { if (use_fbo) post_process_->End(view, options); }; + struct ScopeExit { decltype(fbo_guard)& f; ~ScopeExit() { f(); } } guard{fbo_guard}; DrawBackground(); // Clear screen (or FBO) DrawMap(); // Flush Map for Light Pass sprite_batch->end(*atlas); primitive_renderer->flush(); if (options.isDrawLight()) { DrawLight(); } - // If using FBO, resolve to screen - if (use_fbo) { - post_process_->End(view, options); - } + // FBO resolved by guard destructor + fbo_guard = []{}; // Clear guard since we're proceeding normallyAlternatively, move RAII responsibility into
PostProcessPipelineby returning a scope guard fromBegin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/map_drawer.cpp` around lines 231 - 249, The code calls post_process_->Begin(view, options) and later post_process_->End(view, options) guarded by the local bool use_fbo, but this Begin/End pair is not exception-safe so any early return or thrown exception between DrawBackground/DrawMap/DrawLight will leave the FBO and viewport modified; fix by introducing an RAII scope guard that calls End in its destructor when Begin returns true (e.g., create a PostProcessScope or similar local guard constructed with post_process_ and view/options or change PostProcessPipeline::Begin to return a scoped guard object), and replace the current if(use_fbo){ post_process_->End(...) } logic with the RAII guard so End is always invoked even on exceptions or early returns.
319-327: Consider caching DrawContext to reduce repetition.
DrawContextis constructed identically five times across this file. Since it holds references to the same members, you could construct it once inDraw()orDrawMap()and pass it to helper methods.♻️ Example consolidation
void MapDrawer::DrawIngameBox(const ViewBounds& bounds) { - const DrawContext ctx { *sprite_batch, *primitive_renderer, view, options, light_buffer }; - grid_drawer->DrawIngameBox(ctx, bounds); +void MapDrawer::DrawIngameBox(const DrawContext& ctx, const ViewBounds& bounds) { + grid_drawer->DrawIngameBox(ctx, bounds); } -void MapDrawer::DrawGrid(const ViewBounds& bounds) { - const DrawContext ctx { *sprite_batch, *primitive_renderer, view, options, light_buffer }; +void MapDrawer::DrawGrid(const DrawContext& ctx, const ViewBounds& bounds) { grid_drawer->DrawGrid(ctx, bounds); }Then pass the single
ctxinstance from the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/map_drawer.cpp` around lines 319 - 327, Constructing DrawContext repeatedly in MapDrawer methods is duplicated; instead create a single DrawContext instance (using members sprite_batch, primitive_renderer, view, options, light_buffer) at the start of the top-level drawing entry (e.g., MapDrawer::Draw() or MapDrawer::DrawMap()) and pass that DrawContext by const reference into helper methods such as MapDrawer::DrawIngameBox and MapDrawer::DrawGrid and any other callers that currently construct it locally; remove the local DrawContext constructions in MapDrawer::DrawIngameBox, MapDrawer::DrawGrid, and similar functions and update their signatures to accept a const DrawContext& parameter so they reuse the single cached ctx and avoid repetition (ensure grid_drawer->DrawGrid / DrawIngameBox calls are updated to use the passed ctx).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/rendering/map_drawer.cpp`:
- Around line 231-249: The code calls post_process_->Begin(view, options) and
later post_process_->End(view, options) guarded by the local bool use_fbo, but
this Begin/End pair is not exception-safe so any early return or thrown
exception between DrawBackground/DrawMap/DrawLight will leave the FBO and
viewport modified; fix by introducing an RAII scope guard that calls End in its
destructor when Begin returns true (e.g., create a PostProcessScope or similar
local guard constructed with post_process_ and view/options or change
PostProcessPipeline::Begin to return a scoped guard object), and replace the
current if(use_fbo){ post_process_->End(...) } logic with the RAII guard so End
is always invoked even on exceptions or early returns.
- Around line 319-327: Constructing DrawContext repeatedly in MapDrawer methods
is duplicated; instead create a single DrawContext instance (using members
sprite_batch, primitive_renderer, view, options, light_buffer) at the start of
the top-level drawing entry (e.g., MapDrawer::Draw() or MapDrawer::DrawMap())
and pass that DrawContext by const reference into helper methods such as
MapDrawer::DrawIngameBox and MapDrawer::DrawGrid and any other callers that
currently construct it locally; remove the local DrawContext constructions in
MapDrawer::DrawIngameBox, MapDrawer::DrawGrid, and similar functions and update
their signatures to accept a const DrawContext& parameter so they reuse the
single cached ctx and avoid repetition (ensure grid_drawer->DrawGrid /
DrawIngameBox calls are updated to use the passed ctx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a434788-ebca-4034-a651-e57d929e0249
📒 Files selected for processing (2)
source/rendering/core/drawing_options.cppsource/rendering/map_drawer.cpp
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>
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the summary. Please try again in a few hours by commenting |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/rendering/ui/tooltip_renderer.cpp (1)
100-152:⚠️ Potential issue | 🔴 CriticalCritical:
string_viewreferences intostorage_may dangle on reallocation.The pattern stores
string_viewpointing intostorage_(e.g., line 127). Ifstorage_reallocates during subsequentstd::format_tocalls (when it exceeds current capacity), all previously createdstring_views will dangle.The
reserve(4096)at line 103-104 only helps if total content stays under 4096 bytes. For tooltips with long text or many fields, this can be exceeded.🐛 Proposed fix: pre-calculate total size or use indices
Option 1: Use indices instead of string_view
struct FieldLine { std::string_view label; - std::string_view value; + size_t valueStart; + size_t valueLen; // ... rest };Option 2: Ensure no reallocation by reserving enough space upfront
void TooltipRenderer::prepareFields(const TooltipData& tooltip) { scratch_fields_count_ = 0; storage_.clear(); - if (storage_.capacity() < 4096) { - storage_.reserve(4096); - } + // Reserve generous space to prevent any reallocation + storage_.reserve(8192);Option 3: Store the string_views after all formatting is complete
Separate the formatting pass from the string_view creation pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/tooltip_renderer.cpp` around lines 100 - 152, The prepareFields function creates FieldLine::label/value string_views that point into storage_, which can reallocate and invalidate those views; fix by separating formatting from view creation: first append all formatted text into storage_ (or a temporary std::string) while recording start offsets/lengths (e.g., capture size_t start for actionId/uniqueId/doorId/destination/text but do NOT call addField yet), then after all std::format_to calls complete, iterate the recorded entries and call addField (or set FieldLine::value) using string_views constructed from storage_.data()+start with the recorded length; this ensures no string_view is created before all potential reallocations finish (references: prepareFields, storage_, scratch_fields_, FieldLine, addField).source/rendering/ui/tooltip_data.h (1)
48-59:⚠️ Potential issue | 🟠 MajorDangling
string_viewrisk withstd::string_viewmembers.The
string_viewfields (itemName,text,description,waypointName) do not own their data. If the source strings are temporary or go out of scope before the tooltip is rendered, these views will dangle. This is particularly risky in a pooled/collector pattern whereTooltipDataobjects persist across frames.Ensure all source strings outlive the
TooltipDatalifetime, or consider usingstd::stringfor fields that may reference transient data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/tooltip_data.h` around lines 48 - 59, The TooltipData struct currently uses non-owning std::string_view for itemName, text, description, and waypointName which can dangle if source strings are transient; change these members to owning std::string (replace itemName, text, description, waypointName types) or ensure callers provide stable storage (e.g., interned/static strings) and clearly document lifetime requirements; update any constructors/assignment operators and places that construct TooltipData to move/copy std::string accordingly (or use an explicit factory that takes std::string) so TooltipData no longer holds potentially dangling string_views.
♻️ Duplicate comments (1)
source/rendering/ui/tooltip_data_extractor.cpp (1)
25-25:⚠️ Potential issue | 🟠 MajorStop using
destination.x == 0as the "unset" signal.Line 71 still treats
x == 0as "no destination", which drops valid teleports and ties correctness toPosition's default state. Track destination presence explicitly instead.Suggested fix
- Position destination; + Position destination{}; + bool hasDestination = false; @@ if (is_teleport) { Teleport* tp = static_cast<Teleport*>(item); if (tp->hasDestination()) { destination = tp->getDestination(); + hasDestination = true; } } @@ - if (unique == 0 && action == 0 && doorId == 0 && text.empty() && description.empty() && destination.x == 0 && !hasContent) { + if (unique == 0 && action == 0 && doorId == 0 && text.empty() && description.empty() && !hasDestination && !hasContent) { return false; }Also applies to: 56-60, 70-72
🤖 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` at line 25, The code currently treats Position destination.x == 0 as an "unset" sentinel which drops valid teleports; change this to track destination presence explicitly by adding a boolean flag (e.g., has_destination) alongside the Position destination field and use that flag instead of checking destination.x; update all usages in tooltip_data_extractor.cpp (initialization, places around lines where destination.x is checked and where Position is assigned) to set has_destination = true when a destination is assigned and clear it when there is no destination, and replace all destination.x == 0 checks with !has_destination to preserve valid Position values.
🧹 Nitpick comments (10)
source/rendering/core/game_sprite.h (1)
121-126: Consider adding a const overload foriconRenderer().The non-const accessor is appropriate for callers that need to modify the renderer, but a const overload would enable read-only access without breaking const-correctness in calling code.
const SpriteIconRenderer& iconRenderer() const { return icon_renderer_; }The type aliases for backward compatibility are a good migration strategy.
🤖 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 121 - 126, Add a const overload for the iconRenderer accessor to preserve const-correctness: implement a method const SpriteIconRenderer& iconRenderer() const that returns a const reference to the existing member icon_renderer_, alongside the existing non-const SpriteIconRenderer& iconRenderer(); locate these in the same class where icon_renderer_ and the existing iconRenderer() are declared and ensure the signature uses SpriteIconRenderer and icon_renderer_ exactly as named.source/rendering/utilities/light_drawer.cpp (1)
111-113: Redundant identity translation.
glm::translate(glm::mat4(1.0f), glm::vec3(0.0f, 0.0f, 0.0f))returns the identity matrix unchanged. You can simplify this.♻️ Simplified code
-glm::mat4 model = glm::translate(glm::mat4(1.0f), glm::vec3(0.0f, 0.0f, 0.0f)); -model = glm::scale(model, glm::vec3(draw_dest_w, draw_dest_h, 1.0f)); +glm::mat4 model = glm::scale(glm::mat4(1.0f), glm::vec3(draw_dest_w, draw_dest_h, 1.0f));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/utilities/light_drawer.cpp` around lines 111 - 113, The glm::translate call is redundant because translating by vec3(0,0,0) returns identity; update the model matrix construction in light_drawer.cpp by removing the translate and instead start from an identity matrix before scaling (i.e., initialize model as glm::mat4(1.0f) and then apply glm::scale to it), keeping the subsequent mvp calculation that uses view.projectionMatrix * view.viewMatrix * model.source/rendering/core/sprite_database.h (1)
54-57: Consider addingconstaccessors for consistency.The
sprites()andimages()methods have both mutable and const overloads, buteditorSprites(),residentImages(), andresidentGameSprites()only have mutable versions. Adding const overloads would improve API consistency and allow read-only access patterns.♻️ Suggested const accessors
std::unordered_map<int, std::unique_ptr<Sprite>>& editorSprites() { return editor_sprite_space_; } +const std::unordered_map<int, std::unique_ptr<Sprite>>& editorSprites() const { return editor_sprite_space_; } std::vector<Image*>& residentImages() { return resident_images_; } +const std::vector<Image*>& residentImages() const { return resident_images_; } + std::vector<GameSprite*>& residentGameSprites() { return resident_game_sprites_; } +const std::vector<GameSprite*>& residentGameSprites() const { return resident_game_sprites_; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/sprite_database.h` around lines 54 - 57, Add const accessor overloads for editorSprites(), residentImages(), and residentGameSprites() so callers can read without modifying internal containers; specifically add methods with the same names returning const std::unordered_map<int, std::unique_ptr<Sprite>>& (or const ref to the map type) for editorSprites(), const std::vector<Image*>& for residentImages(), and const std::vector<GameSprite*>& for residentGameSprites(), matching the existing mutable signatures to maintain API consistency with sprites()/images() const overloads.source/rendering/ui/tooltip_data.h (2)
91-93: Long single-line condition reduces readability.The
hasVisibleFields()condition spans many checks in one line, making it hard to scan and maintain.♻️ Suggested refactor for readability
bool hasVisibleFields() const { - return !waypointName.empty() || actionId > 0 || uniqueId > 0 || doorId > 0 || !text.empty() || !description.empty() || destination.x > 0 || !containerItems.empty(); + return !waypointName.empty() + || actionId > 0 + || uniqueId > 0 + || doorId > 0 + || !text.empty() + || !description.empty() + || destination.x > 0 + || !containerItems.empty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/tooltip_data.h` around lines 91 - 93, The hasVisibleFields() one-liner is hard to read; refactor the function to break the condition into multiple, clearly named checks (e.g. checkWaypointName, checkActionOrIds, checkTextOrDescription, checkDestination, checkContainerItems) or evaluate each predicate on its own line and combine them with || so each field (waypointName, actionId, uniqueId, doorId, text, description, destination.x, containerItems) is obvious; keep the function signature but replace the single long expression with these smaller checks or local bools to improve readability and maintainability.
76-88: Consider adding anisValid()helper or usingstd::optionalfor destination.Using
destination.x > 0as a validity check is a magic value pattern. A position at x=0 would be incorrectly treated as invalid. Consider:
- Adding a dedicated
bool hasTeleportDestinationflag, or- Using
std::optional<Position>for destination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/tooltip_data.h` around lines 76 - 88, The validity check for teleport uses a magic value (destination.x > 0) in updateCategory(), causing x==0 to be treated as invalid; replace this with an explicit validity mechanism: add a bool hasTeleportDestination (or change destination to std::optional<Position>) and update updateCategory() to check hasTeleportDestination (or destination.has_value()) when deciding TooltipCategory::TELEPORT, and ensure any code that sets destination also sets/clears hasTeleportDestination (or assigns/clears the optional) so waypointName, doorId, text and category logic remain consistent.source/rendering/drawers/tiles/tile_renderer.h (1)
39-48: Consider storingTileRenderDepsdirectly instead of duplicating members.The private members duplicate all fields from
TileRenderDeps. Storing the struct directly would reduce code duplication and simplify the constructor.♻️ Alternative design
private: void PreloadItem(const Tile* tile, Item* item, const ItemDefinitionView& definition, const SpritePatterns* patterns = nullptr); - ItemDrawer* item_drawer; - SpriteDrawer* sprite_drawer; - CreatureDrawer* creature_drawer; - MarkerDrawer* marker_drawer; - TooltipCollector* tooltip_collector; - CreatureNameDrawer* creature_name_drawer; - Editor* editor; + TileRenderDeps deps_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/tiles/tile_renderer.h` around lines 39 - 48, The class currently duplicates all fields from TileRenderDeps via individual members (item_drawer, sprite_drawer, creature_drawer, marker_drawer, tooltip_collector, creature_name_drawer, editor); replace these separate members with a single TileRenderDeps member (e.g., TileRenderDeps deps_) and update the constructor to accept and store a TileRenderDeps instance/reference, then update all usages (including PreloadItem and any member accesses) to access deps_.<field> (for example deps_.item_drawer) so the class no longer duplicates the dependency fields and construction is simplified.source/rendering/ui/nvg_image_cache.cpp (3)
106-110: NanoVG image creation error handling could be more explicit.
nvgCreateImageRGBAreturns0on failure, but it's worth noting that NanoVG internally starts handles from1. The current checkimage > 0is correct, but consider adding a log or assertion for debugging failed image creations in development builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/nvg_image_cache.cpp` around lines 106 - 110, The code currently calls nvgCreateImageRGBA(...) and only stores the result if image > 0; add explicit debug feedback when creation fails by logging or asserting on image == 0 so failures are visible during development. Locate the call to nvgCreateImageRGBA in this function (the local variable image, the cache_ map and itemId) and, for non-production builds, emit a processLogger/SDL_log/assert with a descriptive message including itemId and any relevant state (e.g., width/height/flags) when image == 0; keep the existing cache_ insertion logic unchanged for image > 0.
26-38: Consider defensive handling when NVG context is externally invalidated.If
last_context_points to a context that was destroyed externally before this cache is cleaned up, callingnvgDeleteImageon it would be undefined behavior. The destructor relies on the context still being valid.Consider documenting this lifetime requirement or adding a method to explicitly clear the cache when the context is about to be destroyed, which callers can invoke before context teardown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/nvg_image_cache.cpp` around lines 26 - 38, The destructor and invalidateAll() assume last_context_ remains valid which is unsafe if the NVG context can be destroyed elsewhere; add a safe detach/clear flow: implement a method like NVGImageCache::detachContext() that sets last_context_ to nullptr and clears cache_ without calling nvgDeleteImage (or alternately accept a context-validity flag and only call nvgDeleteImage when true), update NVGImageCache::~NVGImageCache() to call detachContext(), and document the new API contract so callers can invoke detachContext() before tearing down the NVG context; refer to the NVGImageCache class, invalidateAll(), last_context_, and the destructor when making changes.
70-93: Magic number 32×32 for sprite dimensions.The sprite size is hard-coded throughout this function. If sprite dimensions ever change or vary, this would break.
♻️ Suggested refactor to extract constant
+namespace { + constexpr int SPRITE_SIZE = 32; + constexpr int SPRITE_PIXELS = SPRITE_SIZE * SPRITE_SIZE; +} + // For legacy sprites (no transparency), use RGB + Magenta masking if (!g_gui.gfx.hasTransparency()) { std::unique_ptr<uint8_t[]> rgb = img->getRGBData(); if (rgb) { - rgba = std::make_unique<uint8_t[]>(32 * 32 * 4); - for (int i = 0; i < 32 * 32; ++i) { + rgba = std::make_unique<uint8_t[]>(SPRITE_PIXELS * 4); + for (int i = 0; i < SPRITE_PIXELS; ++i) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/nvg_image_cache.cpp` around lines 70 - 93, The code uses the hard-coded sprite size 32×32 when allocating and iterating (see rgba, rgb and the loop in the branch where g_gui.gfx.hasTransparency() is false and img->getRGBData() is used); change to use the image's actual dimensions (e.g., query width and height via img->getWidth()/getHeight() or the appropriate img accessor) to compute sizes and strides (allocate width*height*4, loop for width*height, and use width for per-row indexing) so the allocation and pixel indexing adapt to varying sprite sizes.source/rendering/map_drawer.cpp (1)
319-327: Consider reducing repeated DrawContext construction.
DrawContextis constructed multiple times throughout the render flow (lines 254, 293, 320, 325, 348). While the struct is small, consider creating it once at the start ofDraw()orDrawMap()and reusing it.♻️ Example refactor
void MapDrawer::Draw() { // ... setup code ... + const DrawContext ctx { *sprite_batch, *primitive_renderer, view, options, light_buffer }; // ... later ... - const DrawContext ctx { *sprite_batch, *primitive_renderer, view, options, light_buffer }; if (drag_shadow_drawer) { drag_shadow_drawer->draw(ctx, editor, ...); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/map_drawer.cpp` around lines 319 - 327, Multiple DrawContext instances are being constructed in MapDrawer (e.g., in MapDrawer::DrawIngameBox and MapDrawer::DrawGrid); instead, create a single DrawContext at the start of the main render entry (e.g., in MapDrawer::Draw or MapDrawer::DrawMap) using the same members (sprite_batch, primitive_renderer, view, options, light_buffer) and pass that single context by const reference into grid_drawer->DrawIngameBox and grid_drawer->DrawGrid (and any other callers that currently construct a local DrawContext) to eliminate repeated construction and centralize context creation.
🤖 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 171-174: The manifest still lists the retired singleton units
sprite_preloader, shared_geometry, and post_process_manager alongside the new
subsystem headers (e.g., sprite_database.h, atlas_lifecycle.h,
sprite_loader_state.h, texture_gc.h); remove all occurrences of
sprite_preloader, shared_geometry, and post_process_manager from the target's
public/compiled sources so only the new subsystem headers remain, and audit the
other manifest blocks referenced (the other duplicated blocks around the same
target) to delete any remaining references to those three symbols to complete
the singleton-elimination cleanup.
In `@source/rendering/map_drawer.h`:
- Line 78: The member selection_drawer (type SelectionDrawer) is dead code —
either remove the declaration and any related initialization in the
constructor/initializer list and include cleanup code, or wire it into the
rendering path by invoking its draw method where selection rendering happens
(e.g., inside MapDrawer::Draw/Render or the existing draw-related method),
ensuring SelectionDrawer is constructed before use and headers are included;
update constructor, any initialization code that creates selection_drawer, and
remove unused includes if you choose deletion.
In `@source/rendering/ui/tooltip_collector.h`:
- Around line 64-74: addWaypointTooltip stores the incoming std::string_view
into TooltipData::waypointName causing potential dangling references if the
source is temporary; change the API or storage so ownership is guaranteed:
either accept std::string in addWaypointTooltip (or an overload) and move/copy
it into TooltipData, or change TooltipData::waypointName to std::string and copy
the view there before commitTooltip; update call sites and any relevant
constructors/assignments for TooltipData to ensure the waypoint name is owned
when the tooltip is rendered.
In `@source/rendering/ui/tooltip_data_extractor.cpp`:
- Around line 92-122: Clear the container preview fields before the
container/zoom conditional so stale data isn't reused: explicitly reset
data.containerItems (clear) and data.containerCapacity (set to 0) prior to the
if (it.isContainer() && zoom <= 1.5f) check; keep the existing fill logic that
uses Container::getVolume(), Container::getVector(), and the loop that populates
containerItems and limits to 32 items.
In `@source/rendering/ui/tooltip_renderer.cpp`:
- Around line 355-364: The slot background path is started with nvgBeginPath(vg)
but no shape is added before nvgFill/nvgStroke; add a call to nvgRect(vg, ... )
(using the existing slot rectangle variables in this scope — e.g. x,y,w,h or
slotRect.x/slotRect.y/slotRect.width/slotRect.height) immediately after
nvgBeginPath(vg) so the rectangle is defined before nvgFill(vg) and
nvgStroke(vg) in the same block that currently contains nvgBeginPath,
nvgFillColor, nvgStrokeColor, nvgStrokeWidth, nvgFill and nvgStroke.
In `@source/rendering/utilities/light_fbo.cpp`:
- Around line 27-31: The code currently logs an incomplete framebuffer in
LightFBO but continues execution; modify the framebuffer check in light_fbo.cpp
(the block that calls glCheckNamedFramebufferStatus(fbo_->GetID(),
GL_FRAMEBUFFER)) to either throw a clear exception (e.g., std::runtime_error)
when status != GL_FRAMEBUFFER_COMPLETE or set an internal validity flag (add a
bool member like valid_ with an isValid() accessor) and initialize it false on
failure; ensure callers of methods that render to the FBO (e.g., any draw or
bind methods on LightFBO) check isValid() and abort or skip rendering if false.
- Around line 46-53: After reattaching the new texture in LightFBO::EnsureSize
(after createTexture(...) and glNamedFramebufferTexture(...)), call the
framebuffer completeness API (e.g. glCheckNamedFramebufferStatus or
glCheckFramebufferStatus on fbo_->GetID()) and verify it equals
GL_FRAMEBUFFER_COMPLETE; if not, log an error including the status and the
involved IDs (fbo_->GetID(), texture_->GetID()) and fail fast (return/throw) to
avoid undefined draws — ensure this check is placed immediately after the
reattachment so any incomplete FBO is detected and handled.
---
Outside diff comments:
In `@source/rendering/ui/tooltip_data.h`:
- Around line 48-59: The TooltipData struct currently uses non-owning
std::string_view for itemName, text, description, and waypointName which can
dangle if source strings are transient; change these members to owning
std::string (replace itemName, text, description, waypointName types) or ensure
callers provide stable storage (e.g., interned/static strings) and clearly
document lifetime requirements; update any constructors/assignment operators and
places that construct TooltipData to move/copy std::string accordingly (or use
an explicit factory that takes std::string) so TooltipData no longer holds
potentially dangling string_views.
In `@source/rendering/ui/tooltip_renderer.cpp`:
- Around line 100-152: The prepareFields function creates FieldLine::label/value
string_views that point into storage_, which can reallocate and invalidate those
views; fix by separating formatting from view creation: first append all
formatted text into storage_ (or a temporary std::string) while recording start
offsets/lengths (e.g., capture size_t start for
actionId/uniqueId/doorId/destination/text but do NOT call addField yet), then
after all std::format_to calls complete, iterate the recorded entries and call
addField (or set FieldLine::value) using string_views constructed from
storage_.data()+start with the recorded length; this ensures no string_view is
created before all potential reallocations finish (references: prepareFields,
storage_, scratch_fields_, FieldLine, addField).
---
Duplicate comments:
In `@source/rendering/ui/tooltip_data_extractor.cpp`:
- Line 25: The code currently treats Position destination.x == 0 as an "unset"
sentinel which drops valid teleports; change this to track destination presence
explicitly by adding a boolean flag (e.g., has_destination) alongside the
Position destination field and use that flag instead of checking destination.x;
update all usages in tooltip_data_extractor.cpp (initialization, places around
lines where destination.x is checked and where Position is assigned) to set
has_destination = true when a destination is assigned and clear it when there is
no destination, and replace all destination.x == 0 checks with !has_destination
to preserve valid Position values.
---
Nitpick comments:
In `@source/rendering/core/game_sprite.h`:
- Around line 121-126: Add a const overload for the iconRenderer accessor to
preserve const-correctness: implement a method const SpriteIconRenderer&
iconRenderer() const that returns a const reference to the existing member
icon_renderer_, alongside the existing non-const SpriteIconRenderer&
iconRenderer(); locate these in the same class where icon_renderer_ and the
existing iconRenderer() are declared and ensure the signature uses
SpriteIconRenderer and icon_renderer_ exactly as named.
In `@source/rendering/core/sprite_database.h`:
- Around line 54-57: Add const accessor overloads for editorSprites(),
residentImages(), and residentGameSprites() so callers can read without
modifying internal containers; specifically add methods with the same names
returning const std::unordered_map<int, std::unique_ptr<Sprite>>& (or const ref
to the map type) for editorSprites(), const std::vector<Image*>& for
residentImages(), and const std::vector<GameSprite*>& for residentGameSprites(),
matching the existing mutable signatures to maintain API consistency with
sprites()/images() const overloads.
In `@source/rendering/drawers/tiles/tile_renderer.h`:
- Around line 39-48: The class currently duplicates all fields from
TileRenderDeps via individual members (item_drawer, sprite_drawer,
creature_drawer, marker_drawer, tooltip_collector, creature_name_drawer,
editor); replace these separate members with a single TileRenderDeps member
(e.g., TileRenderDeps deps_) and update the constructor to accept and store a
TileRenderDeps instance/reference, then update all usages (including PreloadItem
and any member accesses) to access deps_.<field> (for example deps_.item_drawer)
so the class no longer duplicates the dependency fields and construction is
simplified.
In `@source/rendering/map_drawer.cpp`:
- Around line 319-327: Multiple DrawContext instances are being constructed in
MapDrawer (e.g., in MapDrawer::DrawIngameBox and MapDrawer::DrawGrid); instead,
create a single DrawContext at the start of the main render entry (e.g., in
MapDrawer::Draw or MapDrawer::DrawMap) using the same members (sprite_batch,
primitive_renderer, view, options, light_buffer) and pass that single context by
const reference into grid_drawer->DrawIngameBox and grid_drawer->DrawGrid (and
any other callers that currently construct a local DrawContext) to eliminate
repeated construction and centralize context creation.
In `@source/rendering/ui/nvg_image_cache.cpp`:
- Around line 106-110: The code currently calls nvgCreateImageRGBA(...) and only
stores the result if image > 0; add explicit debug feedback when creation fails
by logging or asserting on image == 0 so failures are visible during
development. Locate the call to nvgCreateImageRGBA in this function (the local
variable image, the cache_ map and itemId) and, for non-production builds, emit
a processLogger/SDL_log/assert with a descriptive message including itemId and
any relevant state (e.g., width/height/flags) when image == 0; keep the existing
cache_ insertion logic unchanged for image > 0.
- Around line 26-38: The destructor and invalidateAll() assume last_context_
remains valid which is unsafe if the NVG context can be destroyed elsewhere; add
a safe detach/clear flow: implement a method like NVGImageCache::detachContext()
that sets last_context_ to nullptr and clears cache_ without calling
nvgDeleteImage (or alternately accept a context-validity flag and only call
nvgDeleteImage when true), update NVGImageCache::~NVGImageCache() to call
detachContext(), and document the new API contract so callers can invoke
detachContext() before tearing down the NVG context; refer to the NVGImageCache
class, invalidateAll(), last_context_, and the destructor when making changes.
- Around line 70-93: The code uses the hard-coded sprite size 32×32 when
allocating and iterating (see rgba, rgb and the loop in the branch where
g_gui.gfx.hasTransparency() is false and img->getRGBData() is used); change to
use the image's actual dimensions (e.g., query width and height via
img->getWidth()/getHeight() or the appropriate img accessor) to compute sizes
and strides (allocate width*height*4, loop for width*height, and use width for
per-row indexing) so the allocation and pixel indexing adapt to varying sprite
sizes.
In `@source/rendering/ui/tooltip_data.h`:
- Around line 91-93: The hasVisibleFields() one-liner is hard to read; refactor
the function to break the condition into multiple, clearly named checks (e.g.
checkWaypointName, checkActionOrIds, checkTextOrDescription, checkDestination,
checkContainerItems) or evaluate each predicate on its own line and combine them
with || so each field (waypointName, actionId, uniqueId, doorId, text,
description, destination.x, containerItems) is obvious; keep the function
signature but replace the single long expression with these smaller checks or
local bools to improve readability and maintainability.
- Around line 76-88: The validity check for teleport uses a magic value
(destination.x > 0) in updateCategory(), causing x==0 to be treated as invalid;
replace this with an explicit validity mechanism: add a bool
hasTeleportDestination (or change destination to std::optional<Position>) and
update updateCategory() to check hasTeleportDestination (or
destination.has_value()) when deciding TooltipCategory::TELEPORT, and ensure any
code that sets destination also sets/clears hasTeleportDestination (or
assigns/clears the optional) so waypointName, doorId, text and category logic
remain consistent.
In `@source/rendering/utilities/light_drawer.cpp`:
- Around line 111-113: The glm::translate call is redundant because translating
by vec3(0,0,0) returns identity; update the model matrix construction in
light_drawer.cpp by removing the translate and instead start from an identity
matrix before scaling (i.e., initialize model as glm::mat4(1.0f) and then apply
glm::scale to it), keeping the subsequent mvp calculation that uses
view.projectionMatrix * view.viewMatrix * model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71abdabb-2769-4988-9e5d-c64fd735f5f6
📒 Files selected for processing (22)
source/CMakeLists.txtsource/rendering/core/game_sprite.hsource/rendering/core/sprite_database.hsource/rendering/core/texture_garbage_collector.cppsource/rendering/core/texture_garbage_collector.hsource/rendering/drawers/tiles/tile_renderer.cppsource/rendering/drawers/tiles/tile_renderer.hsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/ui/nvg_image_cache.cppsource/rendering/ui/nvg_image_cache.hsource/rendering/ui/tooltip_collector.hsource/rendering/ui/tooltip_data.hsource/rendering/ui/tooltip_data_extractor.cppsource/rendering/ui/tooltip_renderer.cppsource/rendering/ui/tooltip_renderer.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
Comment on lines
+171
to
+174
| ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_database.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/core/atlas_lifecycle.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_loader_state.h | ||
| ${CMAKE_CURRENT_LIST_DIR}/rendering/core/texture_gc.h |
There was a problem hiding this comment.
Remove the retired singleton units from this target.
Lines 171-174 and 251-253 add the replacement subsystems, but Line 179 / Line 191 / Line 252 and Line 539 / Line 550 / Line 607 still keep sprite_preloader, shared_geometry, and post_process_manager in the manifest. That means the pre-refactor hidden-state path is still compiled and publicly exposed, which undercuts the PR’s stated singleton-elimination goal.
Suggested manifest cleanup
${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_loader_state.h
${CMAKE_CURRENT_LIST_DIR}/rendering/core/texture_gc.h
${CMAKE_CURRENT_LIST_DIR}/rendering/core/image.h
${CMAKE_CURRENT_LIST_DIR}/rendering/core/normal_image.h
${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_archive.h
${CMAKE_CURRENT_LIST_DIR}/rendering/core/template_image.h
- ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_preloader.h
${CMAKE_CURRENT_LIST_DIR}/rendering/core/light_buffer.h
@@
${CMAKE_CURRENT_LIST_DIR}/rendering/core/shader_program.h
- ${CMAKE_CURRENT_LIST_DIR}/rendering/core/shared_geometry.h
${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_batch.h
@@
${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/effect_registry.h
- ${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_manager.h
${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_pipeline.h ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_loader_state.cpp
${CMAKE_CURRENT_LIST_DIR}/rendering/core/texture_gc.cpp
${CMAKE_CURRENT_LIST_DIR}/rendering/core/image.cpp
${CMAKE_CURRENT_LIST_DIR}/rendering/core/normal_image.cpp
${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_archive.cpp
${CMAKE_CURRENT_LIST_DIR}/rendering/core/template_image.cpp
- ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_preloader.cpp
@@
${CMAKE_CURRENT_LIST_DIR}/rendering/core/shader_program.cpp
- ${CMAKE_CURRENT_LIST_DIR}/rendering/core/shared_geometry.cpp
${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_batch.cpp
@@
- ${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_manager.cpp
${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_pipeline.cppAlso applies to: 251-253, 527-534, 608-608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/CMakeLists.txt` around lines 171 - 174, The manifest still lists the
retired singleton units sprite_preloader, shared_geometry, and
post_process_manager alongside the new subsystem headers (e.g.,
sprite_database.h, atlas_lifecycle.h, sprite_loader_state.h, texture_gc.h);
remove all occurrences of sprite_preloader, shared_geometry, and
post_process_manager from the target's public/compiled sources so only the new
subsystem headers remain, and audit the other manifest blocks referenced (the
other duplicated blocks around the same target) to delete any remaining
references to those three symbols to complete the singleton-elimination cleanup.
Comment on lines
+64
to
+74
| // Add a waypoint tooltip | ||
| void addWaypointTooltip(Position pos, std::string_view name) { | ||
| if (name.empty()) { | ||
| return; | ||
| } | ||
| TooltipData& data = requestTooltipData(); | ||
| data.pos = pos; | ||
| data.category = TooltipCategory::WAYPOINT; | ||
| data.waypointName = name; | ||
| commitTooltip(); | ||
| } |
There was a problem hiding this comment.
string_view lifetime concern for waypoint name.
The name parameter is stored directly as data.waypointName without copying. If the source string is temporary, this will dangle when the tooltip is rendered later.
This is a downstream effect of the string_view design in TooltipData. Consider documenting the lifetime requirement prominently or accepting std::string for owned storage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/ui/tooltip_collector.h` around lines 64 - 74,
addWaypointTooltip stores the incoming std::string_view into
TooltipData::waypointName causing potential dangling references if the source is
temporary; change the API or storage so ownership is guaranteed: either accept
std::string in addWaypointTooltip (or an overload) and move/copy it into
TooltipData, or change TooltipData::waypointName to std::string and copy the
view there before commitTooltip; update call sites and any relevant
constructors/assignments for TooltipData to ensure the waypoint name is owned
when the tooltip is rendered.
Comment on lines
+92
to
+122
| // Populate container items | ||
| if (it.isContainer() && zoom <= 1.5f) { | ||
| if (const Container* container = item->asContainer()) { | ||
| // Set capacity for rendering empty slots | ||
| data.containerCapacity = static_cast<uint8_t>(container->getVolume()); | ||
|
|
||
| const auto& items = container->getVector(); | ||
| data.containerItems.clear(); | ||
| // Reserve only what we need (capped at 32) | ||
| data.containerItems.reserve(std::min(items.size(), size_t(32))); | ||
| for (const auto& subItem : items) { | ||
| if (subItem) { | ||
| ContainerItem ci; | ||
| ci.id = subItem->getID(); | ||
| ci.subtype = subItem->getSubtype(); | ||
| ci.count = subItem->getCount(); | ||
| // Sanity check for count | ||
| if (ci.count == 0) { | ||
| ci.count = 1; | ||
| } | ||
|
|
||
| data.containerItems.push_back(ci); | ||
|
|
||
| // Limit preview items to avoid massive tooltips | ||
| if (data.containerItems.size() >= 32) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear container preview fields before the conditional fill.
Line 93 only updates the container preview on the container/zoom path. If data is reused, a non-container item—or the same container above 1.5f zoom—can inherit stale containerItems / containerCapacity from the previous tooltip.
Suggested fix
data.actionId = action;
data.uniqueId = unique;
data.doorId = doorId;
data.text = text;
data.description = description;
data.destination = destination;
+ data.containerCapacity = 0;
+ data.containerItems.clear();
// Populate container items
- if (it.isContainer() && zoom <= 1.5f) {
+ if (is_container && zoom <= 1.5f) {
if (const Container* container = item->asContainer()) {
// Set capacity for rendering empty slots
data.containerCapacity = static_cast<uint8_t>(container->getVolume());
const auto& items = container->getVector();
- data.containerItems.clear();
// Reserve only what we need (capped at 32)
data.containerItems.reserve(std::min(items.size(), size_t(32)));🤖 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 92 - 122, Clear
the container preview fields before the container/zoom conditional so stale data
isn't reused: explicitly reset data.containerItems (clear) and
data.containerCapacity (set to 0) prior to the if (it.isContainer() && zoom <=
1.5f) check; keep the existing fill logic that uses Container::getVolume(),
Container::getVector(), and the loop that populates containerItems and limits to
32 items.
Comment on lines
+27
to
+31
| GLenum status = glCheckNamedFramebufferStatus(fbo_->GetID(), GL_FRAMEBUFFER); | ||
| if (status != GL_FRAMEBUFFER_COMPLETE) { | ||
| spdlog::error("LightFBO Incomplete: {}", status); | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete FBO is logged but not handled.
When the framebuffer is incomplete, an error is logged but execution continues. Drawing to an incomplete FBO leads to undefined behavior and rendering artifacts.
🛡️ Proposed fix to throw or flag error state
GLenum status = glCheckNamedFramebufferStatus(fbo_->GetID(), GL_FRAMEBUFFER);
if (status != GL_FRAMEBUFFER_COMPLETE) {
spdlog::error("LightFBO Incomplete: {}", status);
+ throw std::runtime_error("LightFBO initialization failed: framebuffer incomplete");
}Alternatively, add an isValid() accessor and check it before drawing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/utilities/light_fbo.cpp` around lines 27 - 31, The code
currently logs an incomplete framebuffer in LightFBO but continues execution;
modify the framebuffer check in light_fbo.cpp (the block that calls
glCheckNamedFramebufferStatus(fbo_->GetID(), GL_FRAMEBUFFER)) to either throw a
clear exception (e.g., std::runtime_error) when status !=
GL_FRAMEBUFFER_COMPLETE or set an internal validity flag (add a bool member like
valid_ with an isValid() accessor) and initialize it false on failure; ensure
callers of methods that render to the FBO (e.g., any draw or bind methods on
LightFBO) check isValid() and abort or skip rendering if false.
Comment on lines
+46
to
+53
| void LightFBO::EnsureSize(int width, int height) { | ||
| if (width_ >= width && height_ >= height) { | ||
| return; | ||
| } | ||
|
|
||
| // Grow to at least the requested size (never shrinks) | ||
| createTexture(std::max(width_, width), std::max(height_, height)); | ||
| glNamedFramebufferTexture(fbo_->GetID(), GL_COLOR_ATTACHMENT0, texture_->GetID(), 0); |
There was a problem hiding this comment.
Missing FBO completeness check after texture reattachment.
After creating a new texture and reattaching it to the FBO in EnsureSize(), the FBO completeness is not verified. If the reattachment fails, subsequent draws will produce undefined results.
🛡️ Proposed fix
void LightFBO::EnsureSize(int width, int height) {
if (width_ >= width && height_ >= height) {
return;
}
// Grow to at least the requested size (never shrinks)
createTexture(std::max(width_, width), std::max(height_, height));
glNamedFramebufferTexture(fbo_->GetID(), GL_COLOR_ATTACHMENT0, texture_->GetID(), 0);
+
+ GLenum status = glCheckNamedFramebufferStatus(fbo_->GetID(), GL_FRAMEBUFFER);
+ if (status != GL_FRAMEBUFFER_COMPLETE) {
+ spdlog::error("LightFBO incomplete after resize: {}", status);
+ }
}📝 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 LightFBO::EnsureSize(int width, int height) { | |
| if (width_ >= width && height_ >= height) { | |
| return; | |
| } | |
| // Grow to at least the requested size (never shrinks) | |
| createTexture(std::max(width_, width), std::max(height_, height)); | |
| glNamedFramebufferTexture(fbo_->GetID(), GL_COLOR_ATTACHMENT0, texture_->GetID(), 0); | |
| void LightFBO::EnsureSize(int width, int height) { | |
| if (width_ >= width && height_ >= height) { | |
| return; | |
| } | |
| // Grow to at least the requested size (never shrinks) | |
| createTexture(std::max(width_, width), std::max(height_, height)); | |
| glNamedFramebufferTexture(fbo_->GetID(), GL_COLOR_ATTACHMENT0, texture_->GetID(), 0); | |
| GLenum status = glCheckNamedFramebufferStatus(fbo_->GetID(), GL_FRAMEBUFFER); | |
| if (status != GL_FRAMEBUFFER_COMPLETE) { | |
| spdlog::error("LightFBO incomplete after resize: {}", status); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/utilities/light_fbo.cpp` around lines 46 - 53, After
reattaching the new texture in LightFBO::EnsureSize (after createTexture(...)
and glNamedFramebufferTexture(...)), call the framebuffer completeness API (e.g.
glCheckNamedFramebufferStatus or glCheckFramebufferStatus on fbo_->GetID()) and
verify it equals GL_FRAMEBUFFER_COMPLETE; if not, log an error including the
status and the involved IDs (fbo_->GetID(), texture_->GetID()) and fail fast
(return/throw) to avoid undefined draws — ensure this check is placed
immediately after the reattachment so any incomplete FBO is detected and
handled.
Collaborator
|
/gemini review |
- 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>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
source/rendering/ui/tooltip_data_extractor.cpp (1)
87-93:⚠️ Potential issue | 🟡 MinorClear container fields unconditionally before the conditional fill.
The container preview fields (
containerCapacity,containerItems) are only cleared inside the container block (line 102). IfFill()is called on a reusedTooltipDataobject where the previous fill was a container but the current item is not (or zoom > 1.5f), stale container data persists.While callers may clear the data beforehand, this function should be defensive.
🛡️ Proposed fix
data.has_destination = hasDestination; data.destination = destination; + data.containerCapacity = 0; + data.containerItems.clear(); // Populate container items if (it.isContainer() && zoom <= 1.5f) { if (const Container* container = item->asContainer()) { // Set capacity for rendering empty slots data.containerCapacity = static_cast<uint8_t>(container->getVolume()); const auto& items = container->getVector(); - data.containerItems.clear(); // Reserve only what we need (capped at 32)🤖 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 87 - 93, In TooltipData::Fill (in source/rendering/ui/tooltip_data_extractor.cpp) ensure container preview fields are reset unconditionally before the container-specific conditional block: clear or reset data.containerItems (e.g. clear() or assign empty) and reset data.containerCapacity (e.g. set to 0) as the first step in Fill(), so stale container data from a previous Fill() cannot persist when the current item is not a container or zoom > 1.5f.source/rendering/core/graphics.h (1)
82-106:⚠️ Potential issue | 🔴 CriticalDo not expose
loader_'s guarded state through unsynchronized facade methods.These accessors read
loader_'s non-atomic fields directly even thoughSpriteLoaderStatesays they are only valid after establishing theunloadedguard.getCreatureSprite()is the sharpest edge:source/rendering/core/sprite_database.cpp:52-61usesitem_countas the sprite-array offset, so a reload/clear racing here can return the wrong creature sprite to callers likesource/ui/dialogs/outfit_chooser_dialog.cpp:73-88.getSpriteFile()is also unsafe because it returns a reference into reloadable state. Please route these through a synchronized loader snapshot/accessor instead of exposing the raw fields directly.#!/bin/bash # Verify the contract on SpriteLoaderState, the direct reads in GraphicManager, # and representative callers that currently use these APIs. echo "=== SpriteLoaderState synchronization contract ===" sed -n '34,49p' source/rendering/core/sprite_loader_state.h echo -e "\n=== GraphicManager facade methods reading loader_ directly ===" sed -n '80,106p' source/rendering/core/graphics.h echo -e "\n=== Representative callers of creature sprite APIs ===" rg -n -C3 '\bgetCreatureSprite(MaxID)?\s*\(' source --glob '!source/rendering/core/graphics.h'
🧹 Nitpick comments (5)
source/rendering/drawers/overlays/brush_overlay_drawer.h (1)
25-25: Consider grouping related parameters to reduce signature complexity.The signature has 9 parameters. Given the PR already introduces the
TileRenderDepspattern for grouping dependencies, you might consider a similar approach here. The drag-related parameters (is_dragging_draw,last_click_map_x,last_click_map_y) are only used together and could form a small struct.This would also improve readability by breaking the long line.
💡 Example grouping approach
// Optional: Group drag state parameters struct DragState { bool is_dragging; int click_map_x; int click_map_y; }; // Then the signature becomes: void draw(const DrawContext& ctx, ItemDrawer* item_drawer, SpriteDrawer* sprite_drawer, CreatureDrawer* creature_drawer, BrushCursorDrawer* brush_cursor_drawer, Editor& editor, const DragState& drag_state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/brush_overlay_drawer.h` at line 25, The draw signature in BrushOverlayDrawer (function draw in source/rendering/drawers/overlays/brush_overlay_drawer.h) is crowded with nine parameters; group the related drag parameters into a small struct (e.g., DragState with is_dragging, last_click_map_x, last_click_map_y) and replace the three separate params with a single const DragState& drag_state to mirror the TileRenderDeps grouping pattern used elsewhere; update the declaration in brush_overlay_drawer.h and all corresponding definitions/call sites (and any documentation) to pass the new DragState object instead of the three individual drag-related arguments.source/rendering/core/sprite_database.cpp (1)
52-61: Consider adding a debug assertion for the downcast.Line 60 performs
static_cast<GameSprite*>without runtime verification. If the architectural invariant (creature sprites always stored as GameSprite at offsetid + item_count) is ever violated, this leads to undefined behavior.A debug assertion would catch misuse during development without runtime overhead in release builds.
♻️ Suggested debug assertion
GameSprite* SpriteDatabase::getCreatureSprite(int id, uint16_t item_count) const { if (id < 0) { return nullptr; } size_t target_id = static_cast<size_t>(id) + item_count; if (target_id >= sprite_space_.size()) { return nullptr; } + assert(dynamic_cast<GameSprite*>(sprite_space_[target_id].get()) != nullptr + && "Expected GameSprite at creature offset"); return static_cast<GameSprite*>(sprite_space_[target_id].get()); }Note: This requires RTTI enabled for debug builds. Alternatively, if the type hierarchy supports it, use a type tag or
isGameSprite()method.🤖 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 52 - 61, The downcast in SpriteDatabase::getCreatureSprite (return static_cast<GameSprite*>(sprite_space_[target_id].get())) lacks runtime verification; add a debug-only assertion to verify the object is actually a GameSprite before the cast (e.g., use dynamic_cast<GameSprite*> or an isGameSprite()/type tag check) so violations of the sprite_space_ invariant are caught in debug builds without affecting release performance; locate this in getCreatureSprite, perform the check after computing target_id and before the static_cast, and ensure it is compiled out in non-debug builds (e.g., using assert or a DEBUG-only macro).source/rendering/ui/tooltip_data_extractor.cpp (1)
96-96: Use the localis_containervariable for consistency.Line 36 already caches
it.isContainer()intois_container, but line 96 callsit.isContainer()again. Use the cached variable for consistency.♻️ Proposed fix
- if (it.isContainer() && zoom <= 1.5f) { + if (is_container && zoom <= 1.5f) {🤖 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` at line 96, The condition currently calls it.isContainer() again instead of using the cached boolean; replace the second call to it.isContainer() with the local variable is_container so the if condition uses is_container (keeps consistency with the cached value set earlier and avoids redundant calls in tooltip_data_extractor.cpp).source/rendering/utilities/light_drawer.cpp (1)
70-73: Update function signature and internal color handling for consistency with DrawColor adoption.The
draw()function signature acceptsconst DrawColor& global_color, but the internal light color processing at line 70 still converts towxColorviacolorFromEightBit(). To maintain consistency with the DrawColor parameter type, consider either refactoringcolorFromEightBit()to returnDrawColordirectly or adding an inline conversion that avoids the wxColor intermediate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/utilities/light_drawer.cpp` around lines 70 - 73, The draw() implementation is still converting light.color to wxColor via colorFromEightBit() although the function now accepts a DrawColor global_color; update internal color handling by converting light.color to a DrawColor (or refactor colorFromEightBit to return DrawColor) and use that when building the gpu_lights_ entry instead of creating a wxColor; specifically modify the code around colorFromEightBit(light.color) and the gpu_lights_.push_back call so the .color field is computed from a DrawColor (respecting light_intensity) and remove the wxColor intermediate.source/rendering/core/sprite_icon_renderer.cpp (1)
24-66: Extract the sharedDrawToblit/fallback path.Both overloads duplicate the same size normalization, defaulting, and placeholder drawing. Pulling that into a small helper would make future changes to icon sizing and fallback behavior land in one place.
🤖 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 24 - 66, The two SpriteIconRenderer::DrawTo overloads duplicate size normalization, defaulting width/height and the blit-or-placeholder path; extract that shared logic into a small private helper (e.g., renderBlitOrPlaceholder or drawSpriteBlock) that accepts the destination wxDC*, the source wxDC* (sdc returned by getDC or nullptr), src_width/src_height, start_x/start_y and width/height, then call that helper from both DrawTo overloads after computing sprite_dim and obtaining sdc via getDC(sz, sprite) or getDC(sz, sprite, outfit); keep getDC calls in each overload and move only the common normalization and the StretchBlit / red-rectangle fallback into the helper so future sizing/fallback changes are made in one place.
🤖 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_icon_renderer.cpp`:
- Around line 96-124: The unload path currently erases only 16x16 and 32x32
RenderKey entries causing 64x64 colored renders in
SpriteIconRenderer::colored_dc_ to remain stale; update
CreatureSprite::unloadDC() to also construct and erase the RenderKey for
SPRITE_SIZE_64x64 (using the same fields used in SpriteIconRenderer::getDC:
size, outfit.getColorHash(), outfit.getMountColorHash(), lookMount, lookAddon,
lookMountHead, lookMountBody, lookMountLegs, lookMountFeet) so the 64x64 cache
entry is removed the same way as the 16x16 and 32x32 entries.
In `@source/rendering/core/sprite_loader_state.h`:
- Line 43: The member dat_format is not initialized and can be left
indeterminate; update the declaration of dat_format in sprite_loader_state.h so
it has an in-class initializer set to DAT_FORMAT_UNKNOWN to match clear() and
the pattern used in DatCatalog (i.e., initialize the DatFormat dat_format member
to DAT_FORMAT_UNKNOWN).
---
Duplicate comments:
In `@source/rendering/ui/tooltip_data_extractor.cpp`:
- Around line 87-93: In TooltipData::Fill (in
source/rendering/ui/tooltip_data_extractor.cpp) ensure container preview fields
are reset unconditionally before the container-specific conditional block: clear
or reset data.containerItems (e.g. clear() or assign empty) and reset
data.containerCapacity (e.g. set to 0) as the first step in Fill(), so stale
container data from a previous Fill() cannot persist when the current item is
not a container or zoom > 1.5f.
---
Nitpick comments:
In `@source/rendering/core/sprite_database.cpp`:
- Around line 52-61: The downcast in SpriteDatabase::getCreatureSprite (return
static_cast<GameSprite*>(sprite_space_[target_id].get())) lacks runtime
verification; add a debug-only assertion to verify the object is actually a
GameSprite before the cast (e.g., use dynamic_cast<GameSprite*> or an
isGameSprite()/type tag check) so violations of the sprite_space_ invariant are
caught in debug builds without affecting release performance; locate this in
getCreatureSprite, perform the check after computing target_id and before the
static_cast, and ensure it is compiled out in non-debug builds (e.g., using
assert or a DEBUG-only macro).
In `@source/rendering/core/sprite_icon_renderer.cpp`:
- Around line 24-66: The two SpriteIconRenderer::DrawTo overloads duplicate size
normalization, defaulting width/height and the blit-or-placeholder path; extract
that shared logic into a small private helper (e.g., renderBlitOrPlaceholder or
drawSpriteBlock) that accepts the destination wxDC*, the source wxDC* (sdc
returned by getDC or nullptr), src_width/src_height, start_x/start_y and
width/height, then call that helper from both DrawTo overloads after computing
sprite_dim and obtaining sdc via getDC(sz, sprite) or getDC(sz, sprite, outfit);
keep getDC calls in each overload and move only the common normalization and the
StretchBlit / red-rectangle fallback into the helper so future sizing/fallback
changes are made in one place.
In `@source/rendering/drawers/overlays/brush_overlay_drawer.h`:
- Line 25: The draw signature in BrushOverlayDrawer (function draw in
source/rendering/drawers/overlays/brush_overlay_drawer.h) is crowded with nine
parameters; group the related drag parameters into a small struct (e.g.,
DragState with is_dragging, last_click_map_x, last_click_map_y) and replace the
three separate params with a single const DragState& drag_state to mirror the
TileRenderDeps grouping pattern used elsewhere; update the declaration in
brush_overlay_drawer.h and all corresponding definitions/call sites (and any
documentation) to pass the new DragState object instead of the three individual
drag-related arguments.
In `@source/rendering/ui/tooltip_data_extractor.cpp`:
- Line 96: The condition currently calls it.isContainer() again instead of using
the cached boolean; replace the second call to it.isContainer() with the local
variable is_container so the if condition uses is_container (keeps consistency
with the cached value set earlier and avoids redundant calls in
tooltip_data_extractor.cpp).
In `@source/rendering/utilities/light_drawer.cpp`:
- Around line 70-73: The draw() implementation is still converting light.color
to wxColor via colorFromEightBit() although the function now accepts a DrawColor
global_color; update internal color handling by converting light.color to a
DrawColor (or refactor colorFromEightBit to return DrawColor) and use that when
building the gpu_lights_ entry instead of creating a wxColor; specifically
modify the code around colorFromEightBit(light.color) and the
gpu_lights_.push_back call so the .color field is computed from a DrawColor
(respecting light_intensity) and remove the wxColor intermediate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 104fd582-b4ff-49d7-a7ac-0b1f282b212b
📒 Files selected for processing (13)
source/rendering/core/graphics.hsource/rendering/core/sprite_database.cppsource/rendering/core/sprite_icon_renderer.cppsource/rendering/core/sprite_icon_renderer.hsource/rendering/core/sprite_loader_state.hsource/rendering/drawers/overlays/brush_overlay_drawer.hsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/ui/tooltip_data.hsource/rendering/ui/tooltip_data_extractor.cppsource/rendering/ui/tooltip_renderer.cppsource/rendering/utilities/light_drawer.cppsource/rendering/utilities/light_fbo.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- source/rendering/core/sprite_icon_renderer.h
Comment on lines
+96
to
+124
| wxMemoryDC* SpriteIconRenderer::getDC(SpriteSize size, GameSprite* sprite, const Outfit& outfit) { | ||
| ASSERT(size == SPRITE_SIZE_16x16 || size == SPRITE_SIZE_32x32 || size == SPRITE_SIZE_64x64); | ||
|
|
||
| RenderKey key; | ||
| key.size = size; | ||
| key.colorHash = outfit.getColorHash(); | ||
| key.mountColorHash = outfit.getMountColorHash(); | ||
| key.lookMount = outfit.lookMount; | ||
| key.lookAddon = outfit.lookAddon; | ||
| key.lookMountHead = outfit.lookMountHead; | ||
| key.lookMountBody = outfit.lookMountBody; | ||
| key.lookMountLegs = outfit.lookMountLegs; | ||
| key.lookMountFeet = outfit.lookMountFeet; | ||
|
|
||
| auto it = colored_dc_.find(key); | ||
| if (it == colored_dc_.end()) { | ||
| wxBitmap bmp = SpriteIconGenerator::Generate(sprite, size, outfit); | ||
| if (bmp.IsOk()) { | ||
| auto cache = std::make_unique<CachedDC>(); | ||
| cache->bm = std::make_unique<wxBitmap>(bmp); | ||
| cache->dc = std::make_unique<wxMemoryDC>(*cache->bm); | ||
|
|
||
| auto res = colored_dc_.insert(std::make_pair(key, std::move(cache))); | ||
| g_gui.gfx.addSpriteToCleanup(sprite); | ||
| return res.first->second->dc.get(); | ||
| } | ||
| return nullptr; | ||
| } | ||
| return it->second->dc.get(); |
There was a problem hiding this comment.
Invalidate 64x64 outfit cache entries too.
This method now caches colored renders for SPRITE_SIZE_64x64, but CreatureSprite::unloadDC() in source/rendering/core/game_sprite.cpp:32-50 still erases only 16x16 and 32x32 keys. That leaves 64x64 outfit icons in colored_dc_ after unload and can serve stale renders on the next draw.
🧹 Suggested fix
diff --git a/source/rendering/core/game_sprite.cpp b/source/rendering/core/game_sprite.cpp
@@
- key.size = SPRITE_SIZE_16x16;
- parent->iconRenderer().eraseColoredDC(key);
-
- key.size = SPRITE_SIZE_32x32;
- parent->iconRenderer().eraseColoredDC(key);
+ for (SpriteSize size : {SPRITE_SIZE_16x16, SPRITE_SIZE_32x32, SPRITE_SIZE_64x64}) {
+ key.size = size;
+ 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/sprite_icon_renderer.cpp` around lines 96 - 124, The
unload path currently erases only 16x16 and 32x32 RenderKey entries causing
64x64 colored renders in SpriteIconRenderer::colored_dc_ to remain stale; update
CreatureSprite::unloadDC() to also construct and erase the RenderKey for
SPRITE_SIZE_64x64 (using the same fields used in SpriteIconRenderer::getDC:
size, outfit.getColorHash(), outfit.getMountColorHash(), lookMount, lookAddon,
lookMountHead, lookMountBody, lookMountLegs, lookMountFeet) so the 64x64 cache
entry is removed the same way as the 16x16 and 32x32 entries.
| std::string spritefile; | ||
| std::shared_ptr<SpriteArchive> sprite_archive_; | ||
| ClientVersion* client_version = nullptr; | ||
| DatFormat dat_format; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how DatFormat is defined and whether SpriteLoaderState gets a guaranteed reset before use.
echo "=== DatFormat definition(s) ==="
rg -n -C2 '\bDatFormat\b' app source
echo -e "\n=== SpriteLoaderState reset / construction paths ==="
rg -n -C3 'SpriteLoaderState::clear|GraphicManager::GraphicManager|dat_format' source/renderingRepository: karolak6612/remeres-map-editor-redux
Length of output: 8396
🏁 Script executed:
# Check for SpriteLoaderState constructor definition
rg -n 'SpriteLoaderState::SpriteLoaderState' source
rg -n 'struct SpriteLoaderState' source -A 20Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
# Read the sprite_loader_state.h header file to see the full struct definition
cat source/rendering/core/sprite_loader_state.hRepository: karolak6612/remeres-map-editor-redux
Length of output: 2141
Initialize dat_format to DAT_FORMAT_UNKNOWN.
Every other scalar field in this state object has an explicit in-class initializer, but dat_format does not. Since DatFormat is an enum, default initialization leaves it with an indeterminate value. Initialize it consistently with the clear() method and the pattern used in the similar DatCatalog struct:
DatFormat dat_format = DAT_FORMAT_UNKNOWN;🤖 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
dat_format is not initialized and can be left indeterminate; update the
declaration of dat_format in sprite_loader_state.h so it has an in-class
initializer set to DAT_FORMAT_UNKNOWN to match clear() and the pattern used in
DatCatalog (i.e., initialize the DatFormat dat_format member to
DAT_FORMAT_UNKNOWN).
![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 extensive and impressive refactoring that significantly improves the rendering architecture. The decoupling of components by removing singletons (PostProcessManager, SharedGeometry, SpritePreloader) and introducing dedicated classes (PostProcessPipeline, SpriteDatabase, TextureGC, etc.) is excellent. The introduction of DrawContext and ViewState to eliminate long parameter lists and dependencies on UI elements is a major step forward for maintainability and testability. The tooltip system refactoring is also very well done, breaking it down into logical, single-responsibility components. My review includes a few minor suggestions for further improvement.
| std::string_view external_value; | ||
| Theme::Role colorRole; | ||
| }; | ||
| PendingField pending[8]; |
There was a problem hiding this comment.
Using a fixed-size C-style array here is brittle. If more fields are added to tooltips in the future, this could lead to a buffer overflow if the array size is not updated. It would be more robust and idiomatic to use a std::vector or std::array.
For example:
std::vector<PendingField> pending;
pending.reserve(8);
Suggested change
| PendingField pending[8]; | |
| std::vector<PendingField> pending; |
| #include <wx/dcmemory.h> | ||
|
|
||
| enum SpriteSize { | ||
| enum SpriteSize : int { |
There was a problem hiding this comment.
It's great that you've converted this to an enum class in line with the style guide. To be even more memory-efficient and explicit, you could specify a smaller underlying type like uint8_t, since there are only a few values.
Suggested change
| enum SpriteSize : int { | |
| enum SpriteSize : uint8_t { |
References
- The style guide mandates using
enum classand provides an example with an explicit underlying type (enum class TileState : uint16_t). Using a smaller, explicit type likeuint8_tis a good practice for memory efficiency and clarity. (link)
Comment on lines
+53
to
+62
| struct RenderKeyHash { | ||
| size_t operator()(const RenderKey& k) const noexcept { | ||
| size_t h = std::hash<uint64_t> {}((uint64_t(k.colorHash) << 32) | k.mountColorHash); | ||
| h ^= std::hash<uint64_t> {}((uint64_t(k.lookMount) << 32) | k.lookAddon) + 0x9e3779b9 + (h << 6) + (h >> 2); | ||
| h ^= std::hash<uint64_t> {}((uint64_t(k.lookMountHead) << 32) | k.lookMountBody) + 0x9e3779b9 + (h << 6) + (h >> 2); | ||
| h ^= std::hash<uint64_t> {}((uint64_t(k.lookMountLegs) << 32) | k.lookMountFeet) + 0x9e3779b9 + (h << 6) + (h >> 2); | ||
| h ^= std::hash<int> {}(static_cast<int>(k.size)) + 0x9e3779b9 + (h << 6) + (h >> 2); | ||
| return h; | ||
| } | ||
| }; |
There was a problem hiding this comment.
The hash implementation is correct and more complete than the previous version. For better readability and maintainability, you could consider extracting the hash combining logic into a hash_combine helper function. This would reduce code duplication and make it easier to add or remove fields from the hash in the future.
For example, you could add a helper like this:
template <class T>
inline void hash_combine(std::size_t& seed, const T& v) {
std::hash<T> hasher;
seed ^= hasher(v) + 0x9e3779b9 + (seed<<6) + (seed>>2);
}And then use it to build the hash.
| // along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| ////////////////////////////////////////////////////////////////////// | ||
|
|
||
| #ifndef RME_RENDERING_CORE_SPRITE_METADATA_H_ |
There was a problem hiding this comment.
This file is newly added and included in CMakeLists.txt, but it doesn't appear to be used anywhere in the codebase. It defines a SpriteMetadata struct which seems like a good idea for encapsulating GameSprite data, but GameSprite itself was not refactored to use it. If this is not intended for immediate use, it might be better to remove it to avoid dead code.
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>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/rendering/ui/map_display.h (1)
38-39:⚠️ Potential issue | 🟡 MinorDuplicate forward declaration of
AnimationTimer.
AnimationTimeris forward-declared twice on consecutive lines.🧹 Proposed fix
class MapWindow; class AnimationTimer; -class AnimationTimer; class MapDrawer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.h` around lines 38 - 39, The header contains a duplicate forward declaration of the class AnimationTimer; remove the redundant line so AnimationTimer is only forward-declared once (locate the consecutive declarations of AnimationTimer in map_display.h and delete the duplicate declaration).source/rendering/utilities/light_drawer.cpp (1)
31-118:⚠️ Potential issue | 🟠 Major
experimental_fogno longer affects the light pass.Line 31 still accepts
fog, but the refactored path never reads it. The callers still passrender_settings.experimental_fog, so this change turns that setting into a silent no-op unless the fog behavior is reintroduced elsewhere.
♻️ Duplicate comments (1)
source/CMakeLists.txt (1)
176-179:⚠️ Potential issue | 🟠 MajorRemove the retired singleton units from this target.
The new registry/GC/database pipeline is added here, but the old singleton-backed files are still part of the manifest on Lines 184, 196, 257, 544, 555, and 612. That keeps
sprite_preloader,shared_geometry, andpost_process_managerbuildable, which undercuts the PR’s singleton-elimination goal.Suggested manifest cleanup
${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_archive.h ${CMAKE_CURRENT_LIST_DIR}/rendering/core/template_image.h - ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_preloader.h ${CMAKE_CURRENT_LIST_DIR}/rendering/core/light_buffer.h @@ ${CMAKE_CURRENT_LIST_DIR}/rendering/core/render_view.h ${CMAKE_CURRENT_LIST_DIR}/rendering/core/ring_buffer.h ${CMAKE_CURRENT_LIST_DIR}/rendering/core/shader_program.h - ${CMAKE_CURRENT_LIST_DIR}/rendering/core/shared_geometry.h ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_batch.h @@ ${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/effect_registry.h - ${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_manager.h ${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_pipeline.h${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_archive.cpp ${CMAKE_CURRENT_LIST_DIR}/rendering/core/template_image.cpp - ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_preloader.cpp ${CMAKE_CURRENT_LIST_DIR}/rendering/core/light_buffer.cpp @@ ${CMAKE_CURRENT_LIST_DIR}/rendering/core/render_view.cpp ${CMAKE_CURRENT_LIST_DIR}/rendering/core/ring_buffer.cpp ${CMAKE_CURRENT_LIST_DIR}/rendering/core/shader_program.cpp - ${CMAKE_CURRENT_LIST_DIR}/rendering/core/shared_geometry.cpp ${CMAKE_CURRENT_LIST_DIR}/rendering/core/sprite_batch.cpp @@ - ${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_manager.cpp ${CMAKE_CURRENT_LIST_DIR}/rendering/postprocess/post_process_pipeline.cppAlso applies to: 256-258, 536-539, 613-613
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/CMakeLists.txt` around lines 176 - 179, The CMake target still includes retired singleton-backed source units (e.g., rendering/core/sprite_database.h, rendering/core/atlas_lifecycle.h, rendering/core/sprite_loader_state.h, rendering/core/texture_gc.h) and therefore keeps sprite_preloader, shared_geometry, and post_process_manager buildable; remove those deprecated singleton files from the target manifest so only the new registry/GC/database pipeline remains, deleting the entries that reference the old singleton units and ensuring no other CMake target lists sprite_preloader, shared_geometry, or post_process_manager sources.
🧹 Nitpick comments (18)
source/rendering/core/render_settings.cpp (1)
117-124: Consider range validation for color values.The
static_cast<uint8_t>will silently truncate ifgetInteger()returns values outside [0, 255]. If settings can be user-edited or corrupted, consider adding validation or clamping:opts.cursor_red = static_cast<uint8_t>(std::clamp(settings.getInteger(Config::CURSOR_RED), 0, 255));This is optional if the settings layer already guarantees valid ranges.
🤖 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 117 - 124, The assignments to opts.cursor_* use static_cast<uint8_t>(settings.getInteger(...)) which will silently truncate out-of-range values; update the code around opts.cursor_red/green/blue/alpha and opts.cursor_alt_* to validate or clamp the integer results from settings.getInteger(Config::CURSOR_*) into the 0–255 range before casting (e.g., call a clamp helper or std::clamp on the returned int), ensuring you perform this for both primary and alt cursor fields and keep the static_cast<uint8_t> only after clamping.source/rendering/drawers/tiles/shade_drawer.cpp (1)
26-27:ShadeDrawerstill depends ong_gui.gfx.
DrawContextremoved the view/settings plumbing, but atlas access is still implicit here. Passing the atlas/graphics dependency throughDrawContextas well would finish the decoupling and make this drawer easier to test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/tiles/shade_drawer.cpp` around lines 26 - 27, ShadeDrawer currently reaches out to global g_gui.gfx for atlas access; modify ShadeDrawer to use the atlas/graphics dependency carried on DrawContext instead. Add an atlas/graphics accessor (e.g., atlasManager or gfx pointer) to DrawContext, update callers that construct DrawContext so it populates that field, and replace g_gui.gfx.ensureAtlasManager()/g_gui.gfx.getAtlasManager() in ShadeDrawer with ctx.<atlasAccessor>.ensureAtlasManager() / ctx.<atlasAccessor>.getAtlasManager() and pass that manager into sprite_batch.drawRect; keep the same behavior of checking ensureAtlasManager before drawing and only change the dependency lookup to the new DrawContext member.source/rendering/core/graphics_sprite_resolver.h (1)
31-33: Consider documenting or handling thedynamic_castfailure case.If
gfx_.getSprite()returns aSprite*that is not aGameSprite*,dynamic_castreturnsnullptr. While downstream code likely handlesnullptr, this silent conversion could mask configuration or data issues. Consider adding a debug assertion or log when the cast fails unexpectedly.Also, for API consistency with the PR's use of
[[nodiscard]]elsewhere, consider annotating the return value:- GameSprite* getSprite(int client_id) override { + [[nodiscard]] GameSprite* getSprite(int client_id) override { return dynamic_cast<GameSprite*>(gfx_.getSprite(client_id)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/graphics_sprite_resolver.h` around lines 31 - 33, The override getSprite(int client_id) silently returns nullptr if dynamic_cast<GameSprite*>(gfx_.getSprite(client_id)) fails; update the function (getSprite) to check the result of dynamic_cast, emit a debug assert or log (e.g., when result==nullptr) to surface unexpected type/configuration problems, and preserve existing behavior of returning the pointer; additionally annotate the method with [[nodiscard]] for API consistency. Ensure references are to getSprite, GameSprite, gfx_.getSprite and the dynamic_cast result so reviewers can locate the change.source/rendering/drawers/overlays/hook_indicator_drawer.h (1)
10-22: Good refactor to externalize hook storage viastd::span.The API change correctly shifts ownership of hook data to callers, eliminating internal state. The
HookRequeststruct remains appropriately public for callers to construct.Consider defaulting the constructor/destructor if they're now empty (similar to
DoorIndicatorDrawer).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/hook_indicator_drawer.h` around lines 10 - 22, Constructor and destructor for HookIndicatorDrawer are now no-op and should be defaulted like DoorIndicatorDrawer; change HookIndicatorDrawer() and ~HookIndicatorDrawer() to =default to convey trivial special members and allow inlining/optimization, keeping the public HookRequest struct and draw(NVGcontext*, const ViewState&, std::span<const HookRequest>) signature unchanged.source/rendering/drawers/map_layer_drawer.cpp (1)
46-50: Unused variablelight_bufferextracted from context.The
light_bufferreference is extracted fromctxat line 50 but is not used anywhere in this file. IfDrawTileaccesses it viactxdirectly, this local extraction is unnecessary and should be removed.🧹 Proposed fix to remove unused variable
void MapLayerDrawer::Draw(const DrawContext& ctx, int map_z, bool live_client, const FloorViewParams& floor_params) { auto& sprite_batch = ctx.sprite_batch; const auto& view = ctx.view; const auto& settings = ctx.settings; const auto& frame = ctx.frame; - auto& light_buffer = ctx.light_buffer; int nd_start_x = floor_params.start_x & ~3;🤖 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 46 - 50, The local reference "light_buffer" is extracted from ctx in map_layer_drawer.cpp but never used; remove the unused variable declaration (the line creating auto& light_buffer = ctx.light_buffer) so code relies on ctx.light_buffer directly (e.g., any calls in DrawTile or other functions that access ctx will remain unchanged) to eliminate the dead variable warning.source/rendering/drawers/tiles/tile_color_calculator.h (1)
7-8: Unused forward declaration ofFrameOptions.
FrameOptionsis forward-declared but not referenced in any method signature in this header. If it's not needed, remove it to keep the header clean.🧹 Proposed fix
class Tile; struct RenderSettings; -struct FrameOptions; class TileColorCalculator {🤖 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.h` around lines 7 - 8, Remove the unused forward declaration FrameOptions from the header: the struct FrameOptions is not referenced in any function signatures or members in this file (only RenderSettings is used), so delete the line "struct FrameOptions;" to clean up the header and avoid an unnecessary forward declaration; ensure no uses of FrameOptions exist in tile_color_calculator.h or its inline functions before removing.source/rendering/drawers/overlays/grid_drawer.cpp (1)
72-72: StaticwxColorusage contradicts PR goal to decouple from wx globals.The PR objective states "Decouple DrawingOptions from wx globals by replacing wxColor with DrawColor." However, this file still uses
static wxColor side_color(0, 0, 0, 200). Consider usingglm::vec4directly (as done on line 20) or aDrawColortype for consistency:- static wxColor side_color(0, 0, 0, 200); + static glm::vec4 side_color(0.0f, 0.0f, 0.0f, 200.0f / 255.0f);Then update the
drawFilledRectcalls to use theglm::vec4overload or adjust the helper accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/grid_drawer.cpp` at line 72, Replace the static wxColor side_color(0, 0, 0, 200) with a DrawColor or glm::vec4 (e.g. glm::vec4(0,0,0,200/255.0f)) so this file no longer depends on wx globals; then update the drawFilledRect(...) calls that use side_color to call the glm::vec4/DrawColor overload (or adjust the helper to accept DrawColor) so types are consistent with the rest of the PR and no wxColor symbols remain in grid_drawer.cpp.source/rendering/drawers/overlays/preview_drawer.h (1)
22-22: Long parameter list (11 parameters) is a code smell.This method signature with 11 parameters is difficult to maintain and easy to misuse at call sites. The PR introduces
DrawContextto bundle common rendering state (sprite_batch,view,settings,frame,light_buffer). Consider refactoring to useDrawContextfor consistency with other drawers:void draw(const DrawContext& ctx, const ViewSnapshot& snapshot, const FloorViewParams& floor_params, int map_z, Editor& editor, ItemDrawer* item_drawer, SpriteDrawer* sprite_drawer, CreatureDrawer* creature_drawer, uint32_t current_house_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/preview_drawer.h` at line 22, The draw method in PreviewDrawer (draw) has a long 11-parameter signature; refactor it to accept a DrawContext to bundle common rendering state and reduce parameter count: change the declaration in preview_drawer.h to take const DrawContext& ctx as the first parameter and keep the remaining specific params (const ViewSnapshot& snapshot, const FloorViewParams& floor_params, int map_z, Editor& editor, ItemDrawer* item_drawer, SpriteDrawer* sprite_drawer, CreatureDrawer* creature_drawer, uint32_t current_house_id); then update the corresponding draw implementation and all call sites to construct or forward an existing DrawContext (using sprite_batch, view, settings, frame, light_buffer) when calling PreviewDrawer::draw.source/rendering/drawers/tiles/floor_drawer.h (1)
26-26: Consider consolidating parameters into DrawContext for consistency.The
drawmethod takes 8 parameters. Other drawers in this PR (e.g.,MapLayerDrawer,GridDrawer) have migrated to accept aDrawContextwhich bundlessprite_batch,view,settings,frame, andlight_buffer. Consider aligning this drawer with that pattern for API consistency across the rendering subsystem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/tiles/floor_drawer.h` at line 26, The draw method on FloorDrawer currently accepts many separate params; change its signature to accept a single DrawContext (which bundles sprite_batch, view, settings, frame, and light_buffer) and keep the remaining per-drawer collaborators (ItemDrawer*, SpriteDrawer*, CreatureDrawer*, Editor&) as explicit parameters to match other drawers (MapLayerDrawer, GridDrawer). Update FloorDrawer::draw declaration and all call sites to pass a DrawContext instance instead of the individual bundled arguments, and adjust the implementation to read sprite_batch, view, settings, frame and light_buffer from the DrawContext.source/rendering/core/render_view.h (1)
9-13: Consider adding a factory ornoexceptguarantee forTileScreenPos.The struct is a simple POD used as an
std::optionalreturn type. For clarity and potential performance, consider marking it as an aggregate with a comment or adding= defaultconstructors withnoexcept.♻️ Optional enhancement
// Return type for IsTileVisible — replaces out-parameter pattern. struct TileScreenPos { int x; int y; + + // Aggregate initialization is supported; explicit default for clarity + TileScreenPos() noexcept = default; + TileScreenPos(int x_, int y_) noexcept : x(x_), y(y_) {} };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/render_view.h` around lines 9 - 13, TileScreenPos is a small POD used as an std::optional return; make its construction and use explicit and noexcept by adding defaulted constructors and marking them noexcept (e.g., default constructor and value constructor =default and noexcept) or add a brief comment stating it is an aggregate POD so compilers can optimize when returning from IsTileVisible; update TileScreenPos (and any value-constructor you add) to be noexcept to ensure no-throw semantics for optional/return-paths.source/rendering/core/render_view.cpp (2)
73-76: File missing trailing newline.The file ends without a newline after
glClear(...). POSIX text files should end with a newline.♻️ Add trailing newline
void GLViewport::Clear() { glClearColor(0.0f, 0.0f, 0.0f, 0.0f); glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT); } +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/render_view.cpp` around lines 73 - 76, The file ends without a POSIX trailing newline after the GLViewport::Clear implementation; update the source so that the final line (the glClear(...) call or the closing brace) is followed by a newline character, ensuring the file terminates with a newline as required by POSIX.
59-65: Verify the 0.375f translation offset is intentional.The
glm::vec3(0.375f, 0.375f, 0.0f)translation in the view matrix is a classic OpenGL pixel-perfect alignment trick. This is correct for legacy GL fixed-function contexts but may need adjustment if the rendering backend changes. Consider adding a comment explaining its purpose.📝 Documentation suggestion
void ViewProjection::Compute(ViewState& vs) { int width = vs.screensize_x; int height = vs.screensize_y; vs.projectionMatrix = glm::ortho(0.0f, width * vs.zoom, height * vs.zoom, 0.0f, -1.0f, 1.0f); + // 0.375f offset ensures pixel-perfect alignment in OpenGL rasterization vs.viewMatrix = glm::translate(glm::mat4(1.0f), glm::vec3(0.375f, 0.375f, 0.0f)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/render_view.cpp` around lines 59 - 65, The translation glm::vec3(0.375f, 0.375f, 0.0f) in ViewProjection::Compute is the pixel-alignment (half‑texel/half‑pixel) offset used for legacy OpenGL rasterization; add a short comment immediately above the vs.viewMatrix = glm::translate(...) line explaining that this 0.375f offset is intentional for pixel-perfect alignment in legacy GL, note that backends using different coordinate conventions or modern APIs (e.g., Vulkan/Direct3D with different center/texel rules) may require a different offset or none, and document that the value should be revisited if the rendering backend changes.source/rendering/map_drawer.h (1)
127-135: Accessors return non-const references—ensure callers respect ownership semantics.
getRenderSettings()andgetFrameOptions()return mutable references, which is appropriate for per-frame mutation during the render loop. However, consider documenting that these should only be modified by the owningMapDrawerto avoid unintended side effects from external callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/map_drawer.h` around lines 127 - 135, The accessors getRenderSettings() and getFrameOptions() expose mutable references to render_settings and frame_options; add a short ownership/usage comment above these methods in class MapDrawer stating that the returned references are owned by MapDrawer and callers must not retain or modify them outside the render loop (or without coordinating with MapDrawer), and optionally suggest using const accessors if mutation is not required by callers to enforce safety; reference getRenderSettings, getFrameOptions, render_settings, frame_options, and MapDrawer in the comment so callers understand ownership and intended mutation patterns.source/rendering/drawers/overlays/preview_drawer.cpp (1)
141-141: Minor: Local variableframeshadows theFrameOptionsconcept in scope.On line 141, the variable
frame(fromSpritePatterns) shadows thepreview_frameconstant conceptually. While not a bug (different types), consider renaming toanim_frameorpattern_framein future cleanup to reduce cognitive load when reading code that also deals withFrameOptions.🤖 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 141, The local variable named `frame` in the SpritePatterns usage within preview_drawer.cpp shadows the higher-level `preview_frame` concept; rename that local `frame` variable to `anim_frame` (or `pattern_frame`) everywhere it is declared and used (e.g., in the SpritePatterns access and any subsequent references) to avoid confusion with the `FrameOptions`/`preview_frame` concept and ensure consistent, clear naming throughout the function.source/rendering/drawers/overlays/brush_overlay_drawer.h (1)
26-26: Consider grouping parameters into a struct to reduce signature complexity.The
draw()method has 9 parameters, which can be error-prone at call sites. Consider creating aBrushOverlayDrawParamsstruct to bundle the drawer pointers and state.♻️ Optional: Parameter struct
struct BrushOverlayDrawParams { ItemDrawer* item_drawer; SpriteDrawer* sprite_drawer; CreatureDrawer* creature_drawer; BrushCursorDrawer* brush_cursor_drawer; Editor* editor; bool is_dragging_draw; int last_click_map_x; int last_click_map_y; }; void draw(const DrawContext& ctx, const BrushOverlayDrawParams& params);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/brush_overlay_drawer.h` at line 26, The draw() signature is too long—create a BrushOverlayDrawParams struct that bundles ItemDrawer*, SpriteDrawer*, CreatureDrawer*, BrushCursorDrawer*, Editor* (or Editor& if ownership guarantees), bool is_dragging_draw, int last_click_map_x, and int last_click_map_y, then change the method signature from draw(const DrawContext& ctx, ItemDrawer* ..., int ...) to draw(const DrawContext& ctx, const BrushOverlayDrawParams& params); update the header (drawers/overlays/brush_overlay_drawer.h) and corresponding implementation of BrushOverlayDrawer::draw to unpack params, then find and update all call sites to construct and pass a BrushOverlayDrawParams instance (prefer passing by const reference) so behavior is unchanged but the parameter list is simplified.source/rendering/postprocess/post_process_pipeline.h (1)
33-39: Consider explicitly handling move semantics.The class deletes copy operations but doesn't address move operations. Since it manages GL resources via
unique_ptr, it could be movable by default. However, if the GL resources have context-specific semantics that make moving problematic, consider explicitly deleting move operations as well for clarity.♻️ Explicit move handling
PostProcessPipeline(const PostProcessPipeline&) = delete; PostProcessPipeline& operator=(const PostProcessPipeline&) = delete; + + // If movable is desired (unique_ptr members support it): + PostProcessPipeline(PostProcessPipeline&&) noexcept = default; + PostProcessPipeline& operator=(PostProcessPipeline&&) noexcept = default; + + // Or if GL context semantics prevent safe moves: + // PostProcessPipeline(PostProcessPipeline&&) = delete; + // PostProcessPipeline& operator=(PostProcessPipeline&&) = delete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/postprocess/post_process_pipeline.h` around lines 33 - 39, The class currently deletes copy ctor/assign but omits move semantics; explicitly declare move operations for clarity by adding either PostProcessPipeline(PostProcessPipeline&&) noexcept = default; and PostProcessPipeline& operator=(PostProcessPipeline&&) noexcept = default; if the GL resources managed by the unique_ptrs are safe to transfer, or instead add PostProcessPipeline(PostProcessPipeline&&) = delete; and PostProcessPipeline& operator=(PostProcessPipeline&&) = delete; if moves are unsafe due to GL/context-specific constraints; update the class declaration accordingly next to the existing copy/delete declarations.source/rendering/drawers/overlays/brush_overlay_drawer.cpp (2)
388-446: Consider reducing duplicated logic between square and circle brush shapes.Lines 402-421 and 422-443 contain nearly identical rendering code paths, differing only in the distance check condition. This duplication increases maintenance burden.
Sketch of potential consolidation
// Consolidate by extracting the actual drawing into a helper: auto drawBrushTile = [&](int cx, int cy, const Position& pos) { if (brush->is<RAWBrush>()) { item_drawer->DrawRawBrush(sprite_batch, sprite_drawer, cx, cy, raw_brush->getItemID(), 160, 160, 160, 160); } else if (brush->is<WaypointBrush>()) { uint8_t r, g, b; get_color(brush, editor, pos, r, g, b); brush_cursor_drawer->draw(sprite_batch, primitive_renderer, cx, cy, brush, r, g, b); } else { glm::vec4 c = brushColor; if (brush->is<HouseExitBrush>() || brush->is<OptionalBorderBrush>()) { c = get_check_color(brush, editor, pos, ctx.settings); } if (g_gui.gfx.ensureAtlasManager()) { sprite_batch.drawRect(static_cast<float>(cx), static_cast<float>(cy), static_cast<float>(TILE_SIZE), static_cast<float>(TILE_SIZE), c, *g_gui.gfx.getAtlasManager()); } } }; // Then in the loop, just check shape condition and call drawBrushTile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/brush_overlay_drawer.cpp` around lines 388 - 446, The drawStationaryGeneric function duplicates the tile rendering logic for square and circle brushes; extract the common rendering branch into a helper (lambda or private method) e.g. drawBrushTile(cx, cy, Position) and call it from the two shape checks, preserving behavior for RAWBrush, WaypointBrush, HouseExitBrush/OptionalBorderBrush, and usage of get_color/get_check_color, sprite_batch.drawRect, item_drawer->DrawRawBrush and brush_cursor_drawer->draw; ensure raw_brush is used when brush->is<RAWBrush>() and keep the same atlas-manager guard (g_gui.gfx.ensureAtlasManager()) and parameter order so only the distance/shape condition remains in the loop.
180-251: Consider extracting repeated coordinate calculation pattern.The pattern
x * TILE_SIZE - view.view_scroll_x - view.getFloorAdjustment()appears multiple times throughout this file. While not blocking, extracting this into a small helper (e.g.,mapToScreenX/Y) would reduce repetition and improve readability.Example helper extraction
// Could be added as a helper or as methods on RenderView inline int mapToScreenX(const RenderView& view, int map_x) { return map_x * TILE_SIZE - view.view_scroll_x - view.getFloorAdjustment(); } inline int mapToScreenY(const RenderView& view, int map_y) { return map_y * TILE_SIZE - view.view_scroll_y - view.getFloorAdjustment(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/brush_overlay_drawer.cpp` around lines 180 - 251, Repeated map-to-screen coordinate math (map_x * TILE_SIZE - view.view_scroll_x - view.getFloorAdjustment()) is used in loops and in the rectangle calculations; extract it into small helpers (e.g., mapToScreenX(const RenderView&, int) and mapToScreenY(const RenderView&, int) or methods on RenderView) and replace inline expressions in brush_overlay_drawer.cpp with calls to those helpers. Update usages where cx/cy are computed in the RAWBrush/OptionalBorderBrush loop and where last_click_start_sx/last_click_start_sy/last_click_end_sx/last_click_end_sy are computed so all conversions use mapToScreenX/Y, keeping TILE_SIZE and view.getFloorAdjustment() semantics intact and leaving calls to sprite_batch.drawRect and item_drawer->DrawRawBrush unchanged.
🤖 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/drawers/entities/item_drawer.cpp`:
- Line 141: The local declaration int frame = patterns.frame; shadows the
existing const FrameOptions& frame and breaks subsequent usage; rename this
local integer to anim_frame (or similar) and update all later uses in the sprite
rendering code (references that previously used the local frame variable—see
usages around sprite rendering logic in item_drawer.cpp) to use anim_frame
instead so the original FrameOptions& frame remains unshadowed and used where
intended.
In `@source/rendering/postprocess/post_process_pipeline.cpp`:
- Around line 140-168: The code attaches the color target to scale_fbo_ (via
glNamedFramebufferTexture / glNamedFramebufferDrawBuffers) but does not verify
framebuffer completeness before binding and rendering in UpdateFBO()/Begin(), so
if incomplete rendering silently fails; after setting up the attachments (right
after the glNamedFramebufferDrawBuffers call) call the GL framebuffer status
check (e.g., glCheckNamedFramebufferStatus or glCheckFramebufferStatus on
scale_fbo_->GetID()), verify it equals GL_FRAMEBUFFER_COMPLETE, and if not log
an error including the status and involved IDs (use spdlog::error), mark/clear
scale_fbo_ as unusable (reset scale_fbo_ or set a flag), and ensure subsequent
code (Begin()/UpdateFBO()/the binding code using scale_fbo_->GetID()) falls back
to the default framebuffer (do not call glBindFramebuffer with the incomplete
FBO) so rendering continues instead of disappearing.
- Around line 93-99: Begin currently enables the offscreen FBO whenever
anti_aliasing is true (via UpdateFBO and scale_fbo_), but the final pass bails
out when no screen/effect shader is set, leaving the rendered image stuck in the
FBO; add an explicit AA-only copy path so the FBO contents are presented when
options.screen_shader_name == ShaderNames::NONE and scale_fbo_ is set.
Concretely, in the PostProcessPipeline final/End/RenderFinalPass logic: detect
the case where scale_fbo_ (or anti_aliasing) is active but no screen shader is
returned, and perform a direct copy/resolve from the offscreen FBO to the
default framebuffer (either glBlitFramebuffer resolve or bind the FBO texture
and draw a full-screen quad with a simple copy shader) instead of skipping the
pass; touch UpdateFBO/Begin and the final pass function to ensure the FBO
handle/texture is available for this explicit blit/copy.
In `@source/rendering/ui/map_display.cpp`:
- Around line 320-322: The code in MapCanvas::OnPaint populates
snapshot.secondary_map from
g_gui.GetCurrentMapTab()->GetSession()->secondary_map which pulls state from the
global active tab; change it to read the session for the canvas being painted
(use this or the MapCanvas instance's tab/session accessor) and assign
snapshot.secondary_map from that session (i.e., replace g_gui.GetCurrentMapTab()
with the canvas's own tab/session accessor so snapshot.secondary_map comes from
the current canvas's GetSession()->secondary_map).
---
Outside diff comments:
In `@source/rendering/ui/map_display.h`:
- Around line 38-39: The header contains a duplicate forward declaration of the
class AnimationTimer; remove the redundant line so AnimationTimer is only
forward-declared once (locate the consecutive declarations of AnimationTimer in
map_display.h and delete the duplicate declaration).
---
Duplicate comments:
In `@source/CMakeLists.txt`:
- Around line 176-179: The CMake target still includes retired singleton-backed
source units (e.g., rendering/core/sprite_database.h,
rendering/core/atlas_lifecycle.h, rendering/core/sprite_loader_state.h,
rendering/core/texture_gc.h) and therefore keeps sprite_preloader,
shared_geometry, and post_process_manager buildable; remove those deprecated
singleton files from the target manifest so only the new registry/GC/database
pipeline remains, deleting the entries that reference the old singleton units
and ensuring no other CMake target lists sprite_preloader, shared_geometry, or
post_process_manager sources.
---
Nitpick comments:
In `@source/rendering/core/graphics_sprite_resolver.h`:
- Around line 31-33: The override getSprite(int client_id) silently returns
nullptr if dynamic_cast<GameSprite*>(gfx_.getSprite(client_id)) fails; update
the function (getSprite) to check the result of dynamic_cast, emit a debug
assert or log (e.g., when result==nullptr) to surface unexpected
type/configuration problems, and preserve existing behavior of returning the
pointer; additionally annotate the method with [[nodiscard]] for API
consistency. Ensure references are to getSprite, GameSprite, gfx_.getSprite and
the dynamic_cast result so reviewers can locate the change.
In `@source/rendering/core/render_settings.cpp`:
- Around line 117-124: The assignments to opts.cursor_* use
static_cast<uint8_t>(settings.getInteger(...)) which will silently truncate
out-of-range values; update the code around opts.cursor_red/green/blue/alpha and
opts.cursor_alt_* to validate or clamp the integer results from
settings.getInteger(Config::CURSOR_*) into the 0–255 range before casting (e.g.,
call a clamp helper or std::clamp on the returned int), ensuring you perform
this for both primary and alt cursor fields and keep the static_cast<uint8_t>
only after clamping.
In `@source/rendering/core/render_view.cpp`:
- Around line 73-76: The file ends without a POSIX trailing newline after the
GLViewport::Clear implementation; update the source so that the final line (the
glClear(...) call or the closing brace) is followed by a newline character,
ensuring the file terminates with a newline as required by POSIX.
- Around line 59-65: The translation glm::vec3(0.375f, 0.375f, 0.0f) in
ViewProjection::Compute is the pixel-alignment (half‑texel/half‑pixel) offset
used for legacy OpenGL rasterization; add a short comment immediately above the
vs.viewMatrix = glm::translate(...) line explaining that this 0.375f offset is
intentional for pixel-perfect alignment in legacy GL, note that backends using
different coordinate conventions or modern APIs (e.g., Vulkan/Direct3D with
different center/texel rules) may require a different offset or none, and
document that the value should be revisited if the rendering backend changes.
In `@source/rendering/core/render_view.h`:
- Around line 9-13: TileScreenPos is a small POD used as an std::optional
return; make its construction and use explicit and noexcept by adding defaulted
constructors and marking them noexcept (e.g., default constructor and value
constructor =default and noexcept) or add a brief comment stating it is an
aggregate POD so compilers can optimize when returning from IsTileVisible;
update TileScreenPos (and any value-constructor you add) to be noexcept to
ensure no-throw semantics for optional/return-paths.
In `@source/rendering/drawers/map_layer_drawer.cpp`:
- Around line 46-50: The local reference "light_buffer" is extracted from ctx in
map_layer_drawer.cpp but never used; remove the unused variable declaration (the
line creating auto& light_buffer = ctx.light_buffer) so code relies on
ctx.light_buffer directly (e.g., any calls in DrawTile or other functions that
access ctx will remain unchanged) to eliminate the dead variable warning.
In `@source/rendering/drawers/overlays/brush_overlay_drawer.cpp`:
- Around line 388-446: The drawStationaryGeneric function duplicates the tile
rendering logic for square and circle brushes; extract the common rendering
branch into a helper (lambda or private method) e.g. drawBrushTile(cx, cy,
Position) and call it from the two shape checks, preserving behavior for
RAWBrush, WaypointBrush, HouseExitBrush/OptionalBorderBrush, and usage of
get_color/get_check_color, sprite_batch.drawRect, item_drawer->DrawRawBrush and
brush_cursor_drawer->draw; ensure raw_brush is used when brush->is<RAWBrush>()
and keep the same atlas-manager guard (g_gui.gfx.ensureAtlasManager()) and
parameter order so only the distance/shape condition remains in the loop.
- Around line 180-251: Repeated map-to-screen coordinate math (map_x * TILE_SIZE
- view.view_scroll_x - view.getFloorAdjustment()) is used in loops and in the
rectangle calculations; extract it into small helpers (e.g., mapToScreenX(const
RenderView&, int) and mapToScreenY(const RenderView&, int) or methods on
RenderView) and replace inline expressions in brush_overlay_drawer.cpp with
calls to those helpers. Update usages where cx/cy are computed in the
RAWBrush/OptionalBorderBrush loop and where
last_click_start_sx/last_click_start_sy/last_click_end_sx/last_click_end_sy are
computed so all conversions use mapToScreenX/Y, keeping TILE_SIZE and
view.getFloorAdjustment() semantics intact and leaving calls to
sprite_batch.drawRect and item_drawer->DrawRawBrush unchanged.
In `@source/rendering/drawers/overlays/brush_overlay_drawer.h`:
- Line 26: The draw() signature is too long—create a BrushOverlayDrawParams
struct that bundles ItemDrawer*, SpriteDrawer*, CreatureDrawer*,
BrushCursorDrawer*, Editor* (or Editor& if ownership guarantees), bool
is_dragging_draw, int last_click_map_x, and int last_click_map_y, then change
the method signature from draw(const DrawContext& ctx, ItemDrawer* ..., int ...)
to draw(const DrawContext& ctx, const BrushOverlayDrawParams& params); update
the header (drawers/overlays/brush_overlay_drawer.h) and corresponding
implementation of BrushOverlayDrawer::draw to unpack params, then find and
update all call sites to construct and pass a BrushOverlayDrawParams instance
(prefer passing by const reference) so behavior is unchanged but the parameter
list is simplified.
In `@source/rendering/drawers/overlays/grid_drawer.cpp`:
- Line 72: Replace the static wxColor side_color(0, 0, 0, 200) with a DrawColor
or glm::vec4 (e.g. glm::vec4(0,0,0,200/255.0f)) so this file no longer depends
on wx globals; then update the drawFilledRect(...) calls that use side_color to
call the glm::vec4/DrawColor overload (or adjust the helper to accept DrawColor)
so types are consistent with the rest of the PR and no wxColor symbols remain in
grid_drawer.cpp.
In `@source/rendering/drawers/overlays/hook_indicator_drawer.h`:
- Around line 10-22: Constructor and destructor for HookIndicatorDrawer are now
no-op and should be defaulted like DoorIndicatorDrawer; change
HookIndicatorDrawer() and ~HookIndicatorDrawer() to =default to convey trivial
special members and allow inlining/optimization, keeping the public HookRequest
struct and draw(NVGcontext*, const ViewState&, std::span<const HookRequest>)
signature unchanged.
In `@source/rendering/drawers/overlays/preview_drawer.cpp`:
- Line 141: The local variable named `frame` in the SpritePatterns usage within
preview_drawer.cpp shadows the higher-level `preview_frame` concept; rename that
local `frame` variable to `anim_frame` (or `pattern_frame`) everywhere it is
declared and used (e.g., in the SpritePatterns access and any subsequent
references) to avoid confusion with the `FrameOptions`/`preview_frame` concept
and ensure consistent, clear naming throughout the function.
In `@source/rendering/drawers/overlays/preview_drawer.h`:
- Line 22: The draw method in PreviewDrawer (draw) has a long 11-parameter
signature; refactor it to accept a DrawContext to bundle common rendering state
and reduce parameter count: change the declaration in preview_drawer.h to take
const DrawContext& ctx as the first parameter and keep the remaining specific
params (const ViewSnapshot& snapshot, const FloorViewParams& floor_params, int
map_z, Editor& editor, ItemDrawer* item_drawer, SpriteDrawer* sprite_drawer,
CreatureDrawer* creature_drawer, uint32_t current_house_id); then update the
corresponding draw implementation and all call sites to construct or forward an
existing DrawContext (using sprite_batch, view, settings, frame, light_buffer)
when calling PreviewDrawer::draw.
In `@source/rendering/drawers/tiles/floor_drawer.h`:
- Line 26: The draw method on FloorDrawer currently accepts many separate
params; change its signature to accept a single DrawContext (which bundles
sprite_batch, view, settings, frame, and light_buffer) and keep the remaining
per-drawer collaborators (ItemDrawer*, SpriteDrawer*, CreatureDrawer*, Editor&)
as explicit parameters to match other drawers (MapLayerDrawer, GridDrawer).
Update FloorDrawer::draw declaration and all call sites to pass a DrawContext
instance instead of the individual bundled arguments, and adjust the
implementation to read sprite_batch, view, settings, frame and light_buffer from
the DrawContext.
In `@source/rendering/drawers/tiles/shade_drawer.cpp`:
- Around line 26-27: ShadeDrawer currently reaches out to global g_gui.gfx for
atlas access; modify ShadeDrawer to use the atlas/graphics dependency carried on
DrawContext instead. Add an atlas/graphics accessor (e.g., atlasManager or gfx
pointer) to DrawContext, update callers that construct DrawContext so it
populates that field, and replace
g_gui.gfx.ensureAtlasManager()/g_gui.gfx.getAtlasManager() in ShadeDrawer with
ctx.<atlasAccessor>.ensureAtlasManager() / ctx.<atlasAccessor>.getAtlasManager()
and pass that manager into sprite_batch.drawRect; keep the same behavior of
checking ensureAtlasManager before drawing and only change the dependency lookup
to the new DrawContext member.
In `@source/rendering/drawers/tiles/tile_color_calculator.h`:
- Around line 7-8: Remove the unused forward declaration FrameOptions from the
header: the struct FrameOptions is not referenced in any function signatures or
members in this file (only RenderSettings is used), so delete the line "struct
FrameOptions;" to clean up the header and avoid an unnecessary forward
declaration; ensure no uses of FrameOptions exist in tile_color_calculator.h or
its inline functions before removing.
In `@source/rendering/map_drawer.h`:
- Around line 127-135: The accessors getRenderSettings() and getFrameOptions()
expose mutable references to render_settings and frame_options; add a short
ownership/usage comment above these methods in class MapDrawer stating that the
returned references are owned by MapDrawer and callers must not retain or modify
them outside the render loop (or without coordinating with MapDrawer), and
optionally suggest using const accessors if mutation is not required by callers
to enforce safety; reference getRenderSettings, getFrameOptions,
render_settings, frame_options, and MapDrawer in the comment so callers
understand ownership and intended mutation patterns.
In `@source/rendering/postprocess/post_process_pipeline.h`:
- Around line 33-39: The class currently deletes copy ctor/assign but omits move
semantics; explicitly declare move operations for clarity by adding either
PostProcessPipeline(PostProcessPipeline&&) noexcept = default; and
PostProcessPipeline& operator=(PostProcessPipeline&&) noexcept = default; if the
GL resources managed by the unique_ptrs are safe to transfer, or instead add
PostProcessPipeline(PostProcessPipeline&&) = delete; and PostProcessPipeline&
operator=(PostProcessPipeline&&) = delete; if moves are unsafe due to
GL/context-specific constraints; update the class declaration accordingly next
to the existing copy/delete declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2236eba1-df49-47e2-859d-9754f8972453
📒 Files selected for processing (55)
source/CMakeLists.txtsource/ingame_preview/ingame_preview_renderer.cppsource/ingame_preview/ingame_preview_renderer.hsource/rendering/core/draw_context.hsource/rendering/core/frame_accumulators.hsource/rendering/core/frame_options.hsource/rendering/core/graphics_sprite_resolver.hsource/rendering/core/render_settings.cppsource/rendering/core/render_settings.hsource/rendering/core/render_view.cppsource/rendering/core/render_view.hsource/rendering/core/special_client_ids.hsource/rendering/core/sprite_batch.hsource/rendering/core/sprite_resolver.hsource/rendering/core/view_snapshot.hsource/rendering/drawers/cursors/drag_shadow_drawer.cppsource/rendering/drawers/cursors/live_cursor_drawer.cppsource/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/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/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/tile_color_calculator.cppsource/rendering/drawers/tiles/tile_color_calculator.hsource/rendering/drawers/tiles/tile_renderer.cppsource/rendering/drawers/tiles/tile_renderer.hsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/postprocess/post_process_pipeline.cppsource/rendering/postprocess/post_process_pipeline.hsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/tooltip_collector.hsource/rendering/utilities/light_drawer.cppsource/rendering/utilities/light_drawer.h
🚧 Files skipped from review as they are similar to previous changes (1)
- source/rendering/drawers/cursors/live_cursor_drawer.cpp
Comment on lines
+93
to
+99
| bool PostProcessPipeline::Begin(const ViewState& view, const RenderSettings& options) { | ||
| EnsureInitialized(); | ||
| bool use_fbo = (options.screen_shader_name != ShaderNames::NONE) || options.anti_aliasing; | ||
| if (use_fbo) { | ||
| UpdateFBO(view, options); | ||
| } | ||
| return use_fbo; |
There was a problem hiding this comment.
AA-only post-processing needs an explicit copy path.
Line 95 enables scale_fbo_ whenever anti_aliasing is on, but Line 113 through Line 116 abort the final pass if no effect shader is returned. With screen_shader_name == ShaderNames::NONE, that can leave the fully rendered scene stranded in the offscreen FBO instead of ever presenting it.
🛠️ Suggested fallback
void PostProcessPipeline::DrawPostProcess(const ViewState& view, const RenderSettings& options) {
if (!scale_fbo_ || !pp_vao_) {
return;
}
ShaderProgram* shader = post_process_mgr_->GetEffect(options.screen_shader_name);
if (!shader) {
- return;
+ glBindFramebuffer(GL_READ_FRAMEBUFFER, scale_fbo_->GetID());
+ glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0);
+ glBlitFramebuffer(
+ 0, 0, fbo_width_, fbo_height_,
+ view.viewport_x, view.viewport_y,
+ view.viewport_x + view.screensize_x,
+ view.viewport_y + view.screensize_y,
+ GL_COLOR_BUFFER_BIT,
+ options.anti_aliasing ? GL_LINEAR : GL_NEAREST
+ );
+ return;
}Also applies to: 102-130
🤖 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 93 - 99,
Begin currently enables the offscreen FBO whenever anti_aliasing is true (via
UpdateFBO and scale_fbo_), but the final pass bails out when no screen/effect
shader is set, leaving the rendered image stuck in the FBO; add an explicit
AA-only copy path so the FBO contents are presented when
options.screen_shader_name == ShaderNames::NONE and scale_fbo_ is set.
Concretely, in the PostProcessPipeline final/End/RenderFinalPass logic: detect
the case where scale_fbo_ (or anti_aliasing) is active but no screen shader is
returned, and perform a direct copy/resolve from the offscreen FBO to the
default framebuffer (either glBlitFramebuffer resolve or bind the FBO texture
and draw a full-screen quad with a simple copy shader) instead of skipping the
pass; touch UpdateFBO/Begin and the final pass function to ensure the FBO
handle/texture is available for this explicit blit/copy.
Comment on lines
+140
to
+168
| if (fbo_width_ != target_w || fbo_height_ != target_h || !scale_fbo_) { | ||
| fbo_width_ = target_w; | ||
| fbo_height_ = target_h; | ||
| scale_fbo_ = std::make_unique<GLFramebuffer>(); | ||
| scale_texture_ = std::make_unique<GLTextureResource>(GL_TEXTURE_2D); | ||
|
|
||
| glTextureStorage2D(scale_texture_->GetID(), 1, GL_RGBA8, fbo_width_, fbo_height_); | ||
| glTextureParameteri(scale_texture_->GetID(), GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); | ||
| glTextureParameteri(scale_texture_->GetID(), GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); | ||
|
|
||
| glNamedFramebufferTexture(scale_fbo_->GetID(), GL_COLOR_ATTACHMENT0, scale_texture_->GetID(), 0); | ||
| GLenum drawBuffers[] = { GL_COLOR_ATTACHMENT0 }; | ||
| glNamedFramebufferDrawBuffers(scale_fbo_->GetID(), 1, drawBuffers); | ||
|
|
||
| if (fbo_width_ < 1 || fbo_height_ < 1) { | ||
| spdlog::error("PostProcessPipeline: FBO dimension is zero ({}, {})!", fbo_width_, fbo_height_); | ||
| } | ||
| fbo_resized = true; | ||
| } | ||
|
|
||
| if (scale_texture_ && (fbo_resized || options.anti_aliasing != last_aa_mode_)) { | ||
| GLenum filter = options.anti_aliasing ? GL_LINEAR : GL_NEAREST; | ||
| glTextureParameteri(scale_texture_->GetID(), GL_TEXTURE_MIN_FILTER, filter); | ||
| glTextureParameteri(scale_texture_->GetID(), GL_TEXTURE_MAG_FILTER, filter); | ||
| last_aa_mode_ = options.anti_aliasing; | ||
| } | ||
|
|
||
| glBindFramebuffer(GL_FRAMEBUFFER, scale_fbo_->GetID()); | ||
| glViewport(0, 0, fbo_width_, fbo_height_); |
There was a problem hiding this comment.
Check scale_fbo_ completeness before using it.
After Line 146 through Line 152 attach the color target, UpdateFBO() immediately binds scale_fbo_ for rendering and Begin() still reports success. If the framebuffer is incomplete, the whole frame disappears with no fallback or actionable diagnostic.
🤖 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 140 -
168, The code attaches the color target to scale_fbo_ (via
glNamedFramebufferTexture / glNamedFramebufferDrawBuffers) but does not verify
framebuffer completeness before binding and rendering in UpdateFBO()/Begin(), so
if incomplete rendering silently fails; after setting up the attachments (right
after the glNamedFramebufferDrawBuffers call) call the GL framebuffer status
check (e.g., glCheckNamedFramebufferStatus or glCheckFramebufferStatus on
scale_fbo_->GetID()), verify it equals GL_FRAMEBUFFER_COMPLETE, and if not log
an error including the status and involved IDs (use spdlog::error), mark/clear
scale_fbo_ as unusable (reset scale_fbo_ or set a flag), and ensure subsequent
code (Begin()/UpdateFBO()/the binding code using scale_fbo_->GetID()) falls back
to the default framebuffer (do not call glBindFramebuffer with the incomplete
FBO) so rendering continues instead of disappearing.
Comment on lines
+320
to
+322
| if (auto* mapTab = g_gui.GetCurrentMapTab()) { | ||
| snapshot.secondary_map = mapTab->GetSession()->secondary_map; | ||
| } |
There was a problem hiding this comment.
Build secondary_map from the canvas being painted, not the global active tab.
Line 320 runs inside MapCanvas::OnPaint(), but it pulls secondary_map from g_gui.GetCurrentMapTab(). If another canvas repaints while a different tab is active, snapshot.secondary_map comes from the wrong session and the render path mixes state across tabs.
🤖 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 320 - 322, The code in
MapCanvas::OnPaint populates snapshot.secondary_map from
g_gui.GetCurrentMapTab()->GetSession()->secondary_map which pulls state from the
global active tab; change it to read the session for the canvas being painted
(use this or the MapCanvas instance's tab/session accessor) and assign
snapshot.secondary_map from that session (i.e., replace g_gui.GetCurrentMapTab()
with the canvas's own tab/session accessor so snapshot.secondary_map comes from
the current canvas's GetSession()->secondary_map).
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>
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
wxColorwithDrawColorandUpdate()with aFromSettings()factory that takesconst Settings&explicitlyTileRenderDepsstruct (removing unused FloorDrawer*) and extractFillItemTooltipDatato reusableTooltipDataExtractornamespaceTest plan
friend classin MapDrawer, MapCanvas, GraphicManager::Instance()or::get()singleton calls<wx/includes🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor