Open
Conversation
vllsystems
changed the title
|
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:
📝 WalkthroughWalkthroughReplaces GLUT/freeglut with glad and adds a new GLRenderer (header + implementation). Moves immediate-mode GL and bitmap text to the renderer, separates sprite data IDs from GL texture handles, updates texture lifecycle, and adjusts build/package/project files to include the renderer and stb truetype. Changes
Sequence Diagram(s)sequenceDiagram
participant MapDrawer
participant LightDrawer
participant GLRenderer
participant GPU as "OpenGL Driver / GPU"
MapDrawer->>GLRenderer: init()
MapDrawer->>GLRenderer: setOrtho(left,right,bottom,top)
MapDrawer->>LightDrawer: draw(..., renderer)
LightDrawer->>GLRenderer: drawTexturedQuad(x,y,w,h, textureId, color)
GLRenderer->>GLRenderer: accumulate batch vertices (VAO/VBO)
GLRenderer->>GPU: glBufferData / glDrawArrays / glTexImage2D
GPU-->>GLRenderer: draw complete
GLRenderer->>MapDrawer: flush() completes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vllsystems
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/CMakeLists.txt (1)
267-286:⚠️ Potential issue | 🟠 MajorLink
${CMAKE_DL_LIBS}for dlopen/dlsym usage.
source/gl_renderer.cppusesdlopen("libGL.so")anddlsym()on non-Windows builds, but the target doesn't link the platform's dl library. This will cause linker failures on Linux.Suggested fix
target_link_libraries(${PROJECT_NAME} PRIVATE ${OPENGL_LIBRARIES} ${ZLIB_LIBRARIES} glad::glad fmt::fmt asio::asio nlohmann_json::nlohmann_json pugixml::pugixml wx::base wx::core wx::net wx::gl wx::html wx::aui wx::adv liblzma::liblzma OpenGL::GL protobuf ${LUA_LIB_TARGET} sol2::sol2 cpr::cpr + ${CMAKE_DL_LIBS} $<$<PLATFORM_ID:Linux>:xcb> $<$<PLATFORM_ID:Windows>:psapi> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/CMakeLists.txt` around lines 267 - 286, The CMake target `${PROJECT_NAME}` needs to link the platform dynamic loader library because source/gl_renderer.cpp calls dlopen/dlsym on non-Windows builds; add `${CMAKE_DL_LIBS}` to the target_link_libraries call for `${PROJECT_NAME}` (in the same list where glad::glad, OpenGL::GL, etc. are listed) guarded for non-Windows platforms so the linker pulls in libdl on Linux and other UNIXes.source/light_drawer.cpp (1)
81-93:⚠️ Potential issue | 🔴 CriticalFlush the map batch before switching to the light-pass blend mode.
MapDrawer::Draw()reaches this method right after batched tile rendering on Line 234 ofsource/map_drawer.cpp.GLRenderer::drawTexturedQuad()only flushes when the texture changes (source/gl_renderer.cppLines 229-244), so the pending map batch can be emitted after Line 88 has already switched toGL_DST_COLOR. That makes regular map sprites render with the light-pass blend function.💡 Suggested fix
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, w, h, 0, GL_RGBA, GL_UNSIGNED_BYTE, buffer.data()); + renderer->flush(); glBlendFunc(GL_DST_COLOR, GL_ONE_MINUS_SRC_ALPHA); renderer->enableTexture();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/light_drawer.cpp` around lines 81 - 93, The draw path switches OpenGL blend mode to the light-pass (GL_DST_COLOR) before any pending map sprite batch is flushed, so map sprites get drawn with the wrong blend; to fix, flush the current renderer batch before changing the blend function by calling the renderer's flush() (the same flush used after drawTexturedQuad) prior to glBlendFunc(GL_DST_COLOR, GL_ONE_MINUS_SRC_ALPHA) in light_drawer.cpp so that the batched map draw from MapDrawer::Draw (which uses GLRenderer::drawTexturedQuad) is emitted with the normal blend state before the light pass begins.
🧹 Nitpick comments (1)
source/map_display.cpp (1)
107-118: Don’t silently discard the caller’s GL attribute list.
source/map_window.cppstill constructsMapCanvaswith a non-nullGL_settingsarray, but this constructor now ignores it and always substitutesattribList. Either remove the parameter from the API and update callers, or merge the mandatory core-profile flags into the caller-provided list so future canvas attributes are not lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map_display.cpp` around lines 107 - 118, The MapCanvas constructor currently ignores the caller's GL attribute list (the int* parameter) and always uses the static attribList; fix this by honoring the caller-provided GL_settings from MapWindow instead of discarding it: update MapCanvas (constructor) to merge the mandatory core-profile flags (WX_GL_CORE_PROFILE, WX_GL_MAJOR_VERSION/3, WX_GL_MINOR_VERSION/3, WX_GL_RGBA, WX_GL_DOUBLEBUFFER, WX_GL_DEPTH_SIZE, terminator) into the supplied list (GL_settings) when it is non-null (ensuring proper terminator and avoiding buffer overflow by using a temporary vector or allocating a new int[]), or alternatively remove the int* parameter from the API and update MapWindow call sites to pass only the intended attributes—pick one approach and apply it consistently so MapCanvas no longer silently drops caller attributes.
🤖 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/gl_renderer.cpp`:
- Around line 322-338: GLRenderer::drawStippledLines currently ignores the
factor and pattern parameters so every stipple is identical; update this
function to forward the incoming factor and pattern to the GL shader by setting
the appropriate uniforms (e.g., glUniform1i/glUniform1ui) for the stipple repeat
and bitpattern (use the existing shader uniform names or add
loc_stippleFactor/loc_stipplePattern if missing), ensure these uniform locations
are queried (glGetUniformLocation) during program setup, and confirm the shader
reads those uniforms to implement variable dash repeat and bitmask rather than
using the hard-coded discard pattern.
- Around line 120-136: Check and abort initialization on GL loader or
shader/program failures: after calling
gladLoadGLLoader((GLADloadproc)rmeGetGLProc) verify it returned non-zero and
abort/init-failure if it failed; after glCompileShader(vs) and
glCompileShader(fs) call glGetShaderiv(..., GL_COMPILE_STATUS) and on failure
retrieve the log with glGetShaderInfoLog for each shader, log the error, delete
any created shaders and abort; after glLinkProgram(program) call
glGetProgramiv(program, GL_LINK_STATUS) and on failure retrieve the link log
with glGetProgramInfoLog, log the error, delete program and shaders and abort;
ensure you also check glCreateProgram() and glCreateShader() return non-zero
before using them and set program to 0 on failure so subsequent
glGetUniformLocation calls are avoided.
- Around line 13-16: Replace the simplistic dlopen("libGL.so") loader in
rmeGetGLProc with a platform-aware resolver: on Linux attempt to resolve
glXGetProcAddressARB (via dlsym) and if found call it to get function pointers
(fall back to dlopen("libGL.so.1" or "libGL.so") + dlsym only if
glXGetProcAddressARB is unavailable), and on macOS use the OpenGL framework
(dlopen("/System/Library/Frameworks/OpenGL.framework/OpenGL", RTLD_LAZY) and
dlsym) or the native CGL/NSOpenGL resolution mechanism; ensure rmeGetGLProc
returns the correct void* for gladLoadGLLoader to succeed and update any error
handling/logging in the rmeGetGLProc/gladLoadGLLoader call sites accordingly.
- Around line 267-285: glLineWidth(width) is unsupported for widths >1.0 in
OpenGL 3.3 core, so replace the existing line-primitive approach in
GLRenderer::drawRect (and the sibling functions around those locations) with
geometry-based strokes: compute the quad(s) that represent the rectangle border
for the requested lineWidth (or a helper like GLRenderer::tessellateStroke or
GLRenderer::addThickLine that builds two triangles per segment), populate the
Vertex array with those quad vertices (two triangles per edge or a continuous
triangle-strip), upload to the VBO, set glUniform1i(loc_useTexture, 0) as
before, and call glDrawArrays(GL_TRIANGLES, ...) instead of
glDrawArrays(GL_LINE_STRIP); remove reliance on glLineWidth and ensure
joins/caps are handled (miter/bevel) as needed for selection boxes/grids/brush
previews.
In `@source/graphics.cpp`:
- Around line 1305-1319: The getSpriteID implementation duplicates index math
and omits _pattern_z and frame normalization; replace the manual computation in
GameSprite::getSpriteID with the existing index helper (e.g., sprite_index or
getSpriteIndex) to compute the linear sprite index using _layer, _pattern_x,
_pattern_y, _pattern_z and a normalized _frame (divide _frame by
sprite_phase_size when applicable), then apply the existing numsprites wrapping
logic and return spriteList[v]->id; this ensures multi-Z and animated sprites
resolve correctly.
In `@source/map_drawer.cpp`:
- Around line 1321-1325: The code is using spriteList[0]->id which ignores the
baseIdx (direction*layers) and causes wrong sprite IDs/texture metadata; change
spriteId to use the base slot corresponding to baseIdx (i.e. obtain the id from
spr->spriteList[baseIdx] or otherwise the entry indexed by baseIdx) before
calling spr->getOutfitImage(...) and glBlitTexture(...), so getOutfitImage and
glBlitTexture receive the correct spriteId for the given baseIdx/direction.
- Around line 857-860: The optional-border branch inside the drag-path loop (the
if checking brush->isOptionalBorder() that calls getCheckColor(brush,
Position(x, y, floor), ...)) computes the validation color but never emits
geometry, causing optional-border brushes to disappear while dragging; modify
that branch to emit the same preview geometry used by the non-optional branch
(or reuse the single-tile preview routine used in the code around lines
1023-1030) after computing the color so the optional-border preview is drawn —
i.e., after getCheckColor(...) call, add the same vertex/quad submission or call
the existing preview-draw helper used elsewhere for border previews so both
branches produce identical rendering behavior.
In `@vcpkg.json`:
- Around line 8-9: Remove the unused freeglut entry from the vcpkg.json manifest
so only glad remains; locate the dependency list containing "freeglut" and
delete that entry (leaving "glad"), then run a quick search for any remaining
references to freeglut in build files or source (e.g., CMakeLists.txt or
includes) to confirm there are none before committing.
---
Outside diff comments:
In `@source/CMakeLists.txt`:
- Around line 267-286: The CMake target `${PROJECT_NAME}` needs to link the
platform dynamic loader library because source/gl_renderer.cpp calls
dlopen/dlsym on non-Windows builds; add `${CMAKE_DL_LIBS}` to the
target_link_libraries call for `${PROJECT_NAME}` (in the same list where
glad::glad, OpenGL::GL, etc. are listed) guarded for non-Windows platforms so
the linker pulls in libdl on Linux and other UNIXes.
In `@source/light_drawer.cpp`:
- Around line 81-93: The draw path switches OpenGL blend mode to the light-pass
(GL_DST_COLOR) before any pending map sprite batch is flushed, so map sprites
get drawn with the wrong blend; to fix, flush the current renderer batch before
changing the blend function by calling the renderer's flush() (the same flush
used after drawTexturedQuad) prior to glBlendFunc(GL_DST_COLOR,
GL_ONE_MINUS_SRC_ALPHA) in light_drawer.cpp so that the batched map draw from
MapDrawer::Draw (which uses GLRenderer::drawTexturedQuad) is emitted with the
normal blend state before the light pass begins.
---
Nitpick comments:
In `@source/map_display.cpp`:
- Around line 107-118: The MapCanvas constructor currently ignores the caller's
GL attribute list (the int* parameter) and always uses the static attribList;
fix this by honoring the caller-provided GL_settings from MapWindow instead of
discarding it: update MapCanvas (constructor) to merge the mandatory
core-profile flags (WX_GL_CORE_PROFILE, WX_GL_MAJOR_VERSION/3,
WX_GL_MINOR_VERSION/3, WX_GL_RGBA, WX_GL_DOUBLEBUFFER, WX_GL_DEPTH_SIZE,
terminator) into the supplied list (GL_settings) when it is non-null (ensuring
proper terminator and avoiding buffer overflow by using a temporary vector or
allocating a new int[]), or alternatively remove the int* parameter from the API
and update MapWindow call sites to pass only the intended attributes—pick one
approach and apply it consistently so MapCanvas no longer silently drops caller
attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b217d21-0272-4d45-90f0-33d60e8d6ee3
📒 Files selected for processing (13)
source/CMakeLists.txtsource/application.cppsource/gl_renderer.cppsource/gl_renderer.hsource/graphics.cppsource/graphics.hsource/light_drawer.cppsource/light_drawer.hsource/map_display.cppsource/map_drawer.cppsource/map_drawer.hvcpkg.jsonvcproj/Project/RME.vcxproj
💤 Files with no reviewable changes (1)
- source/application.cpp
… and redundant glFinish
There was a problem hiding this comment.
🧹 Nitpick comments (3)
source/gl_renderer.cpp (2)
62-65: Shader stipple code appears unused.The
uStippleuniform and fragment shader discard logic (lines 62-65) appear to be dead code.drawStippledLinesnow implements stippling on the CPU side by selectively drawing line segments based on the pattern bitmask, andloc_stippleis never set to non-zero in any draw path.Consider removing this unused shader code to avoid confusion, or document it as reserved for future use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 62 - 65, The fragment-shader stipple branch is dead and should be removed or documented: remove the conditional block referencing the uniform uStipple (and the mod/discard logic) from the shader to avoid confusion, and delete or at least stop setting/declaring loc_stipple if it’s never used; alternatively add a clear comment in the shader and in the code around loc_stipple stating it’s reserved for future GPU stippling. Update references in drawStippledLines and any shader setup code so there are no unused uniforms or unused loc_stipple assignments remaining.
73-127: Font atlas generation may vary across platforms.The bitmap font atlas uses
wxFontwith a fixed size of 10 points, but font rendering can differ across operating systems (Windows/macOS/Linux) in terms of glyph metrics and anti-aliasing. This could cause visual inconsistencies in text rendering.Additionally, there's no error checking on
wxBitmaporwxMemoryDCcreation failures, which could occur under low-memory conditions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 73 - 127, The GLRenderer::initFontAtlas implementation should be made more robust and deterministic: check that wxBitmap and wxMemoryDC were created successfully (verify bmp.IsOk() and dc.IsOk()) and bail or fallback if they fail; create the font with a pixel-size-based API (use wxFont::SetPixelSize or wxFontInfo().PixelSize()) instead of a point size to avoid OS-dependent scaling and explicitly disable anti-aliasing (you already use AntiAliased(false)) and then measure each glyph via dc.GetTextExtent to compute exact glyph metrics before placing characters in the atlas; ensure you record those metrics (fontGlyphW/fontGlyphH/fontAtlasCols/fontAtlasW/fontAtlasH) correctly and provide a fallback path (e.g., hardcoded mono bitmap font or return error) if any creation/measurement step fails so rendering won’t proceed with invalid texture or sizes.source/map_display.cpp (1)
107-115: UnusedattribListarray.The static
attribListarray is defined but never referenced. TheMapCanvasconstructor receives its attribute list via theattriblistparameter (fromMapWindow::MapWindowinmap_window.cpp), which is correctly forwarded towxGLCanvas.Consider removing this dead code to avoid confusion:
-static const int attribList[] = { - WX_GL_CORE_PROFILE, - WX_GL_MAJOR_VERSION, 3, - WX_GL_MINOR_VERSION, 3, - WX_GL_RGBA, - WX_GL_DOUBLEBUFFER, - WX_GL_DEPTH_SIZE, 16, - 0 // terminador -}; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map_display.cpp` around lines 107 - 115, Remove the dead static attribList array declared in source/map_display.cpp since it is never used; update the file to delete the attribList definition and any related comments, leaving the MapCanvas constructor to continue receiving its attribute list via the attriblist parameter (as forwarded from MapWindow::MapWindow) for wxGLCanvas initialization.
🤖 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/gl_renderer.cpp`:
- Around line 62-65: The fragment-shader stipple branch is dead and should be
removed or documented: remove the conditional block referencing the uniform
uStipple (and the mod/discard logic) from the shader to avoid confusion, and
delete or at least stop setting/declaring loc_stipple if it’s never used;
alternatively add a clear comment in the shader and in the code around
loc_stipple stating it’s reserved for future GPU stippling. Update references in
drawStippledLines and any shader setup code so there are no unused uniforms or
unused loc_stipple assignments remaining.
- Around line 73-127: The GLRenderer::initFontAtlas implementation should be
made more robust and deterministic: check that wxBitmap and wxMemoryDC were
created successfully (verify bmp.IsOk() and dc.IsOk()) and bail or fallback if
they fail; create the font with a pixel-size-based API (use wxFont::SetPixelSize
or wxFontInfo().PixelSize()) instead of a point size to avoid OS-dependent
scaling and explicitly disable anti-aliasing (you already use
AntiAliased(false)) and then measure each glyph via dc.GetTextExtent to compute
exact glyph metrics before placing characters in the atlas; ensure you record
those metrics (fontGlyphW/fontGlyphH/fontAtlasCols/fontAtlasW/fontAtlasH)
correctly and provide a fallback path (e.g., hardcoded mono bitmap font or
return error) if any creation/measurement step fails so rendering won’t proceed
with invalid texture or sizes.
In `@source/map_display.cpp`:
- Around line 107-115: Remove the dead static attribList array declared in
source/map_display.cpp since it is never used; update the file to delete the
attribList definition and any related comments, leaving the MapCanvas
constructor to continue receiving its attribute list via the attriblist
parameter (as forwarded from MapWindow::MapWindow) for wxGLCanvas
initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01c66024-06ea-4a94-a8cb-42f5cb948ec7
📒 Files selected for processing (8)
source/gl_renderer.cppsource/gl_renderer.hsource/light_drawer.cppsource/map_display.cppsource/map_drawer.cppsource/map_drawer.hsource/map_window.cppvcpkg.json
✅ Files skipped from review due to trivial changes (1)
- vcpkg.json
🚧 Files skipped from review as they are similar to previous changes (2)
- source/light_drawer.cpp
- source/map_drawer.h
There was a problem hiding this comment.
🧹 Nitpick comments (6)
source/map_drawer.cpp (1)
161-168: Consider usingstd::unique_ptrfor ownership ofrenderer.Raw pointer with manual
deleteworks but is error-prone. Using a smart pointer would provide automatic cleanup and exception safety.♻️ Suggested refactor
In the header (
map_drawer.h), change:-GLRenderer* renderer; +std::unique_ptr<GLRenderer> renderer;Then in the constructor:
-renderer = new GLRenderer(); +renderer = std::make_unique<GLRenderer>();And simplify the destructor:
MapDrawer::~MapDrawer() { Release(); - renderer->shutdown(); - delete renderer; + if (renderer) { + renderer->shutdown(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map_drawer.cpp` around lines 161 - 168, The MapDrawer currently owns renderer via a raw pointer (renderer) and manually deletes it in MapDrawer::~MapDrawer(), which is error-prone; change renderer to std::unique_ptr<GLRenderer> in the MapDrawer class declaration, initialize it with std::make_unique<GLRenderer>() inside the MapDrawer constructor (replacing new GLRenderer()), and remove the explicit delete and shutdown pattern in the destructor—either call renderer->shutdown() if necessary and let the unique_ptr automatically destroy the GLRenderer when MapDrawer is destroyed, and keep the existing Release() call as needed.source/gl_renderer.cpp (5)
414-454:drawPolygonanddrawTriangleFanare identical implementations.Both functions perform the same fan triangulation and draw logic. Consider consolidating into a single implementation or having one call the other.
♻️ Suggested refactor
void GLRenderer::drawPolygon(const float* vertices, int vertexCount, uint8_t r, uint8_t g, uint8_t b, uint8_t a) { - if (vertexCount < 3) { - return; - } - flushBatch(); - std::vector<Vertex> tris; - for (int i = 1; i < vertexCount - 1; ++i) { - tris.push_back({ vertices[0], vertices[1], 0, 0, r, g, b, a }); - tris.push_back({ vertices[i * 2], vertices[i * 2 + 1], 0, 0, r, g, b, a }); - tris.push_back({ vertices[(i + 1) * 2], vertices[(i + 1) * 2 + 1], 0, 0, r, g, b, a }); - } - glUseProgram(program); - glBindVertexArray(vao); - glUniform1i(loc_useTexture, 0); - glBindBuffer(GL_ARRAY_BUFFER, vbo); - glBufferData(GL_ARRAY_BUFFER, tris.size() * sizeof(Vertex), tris.data(), GL_DYNAMIC_DRAW); - glDrawArrays(GL_TRIANGLES, 0, (GLsizei)tris.size()); - glBindVertexArray(0); - glUseProgram(0); + drawTriangleFan(vertices, vertexCount, r, g, b, a); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 414 - 454, drawPolygon and drawTriangleFan contain identical triangulation and draw logic; consolidate by implementing the triangulation + GL draw sequence in a single helper (e.g., a private GLRenderer::drawFanImpl or merge into drawPolygon) and have the other method call it to avoid duplication. Move/keep calls to flushBatch(), creation of std::vector<Vertex> tris, the for-loop that builds tris, and the GL sequence (glUseProgram, glBindVertexArray, glUniform1i(loc_useTexture,0), glBindBuffer, glBufferData, glDrawArrays, glBindVertexArray(0), glUseProgram(0)) inside the shared helper so both GLRenderer::drawPolygon and GLRenderer::drawTriangleFan simply validate vertexCount and forward parameters to the helper to preserve behavior.
456-457: Thefontparameter is unused in text rendering methods.
drawText(),getCharWidth(), anddrawBitmapChar()all accept avoid* fontparameter that is never used. The renderer uses its own fixed font atlas. Consider removing the parameter if font selection is not planned, or document it as reserved for future use.Also applies to: 490-491, 499-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 456 - 457, The font parameter is never used by the renderer (it uses its internal fontAtlas) so either remove the unused parameter from GLRenderer::drawText, GLRenderer::getCharWidth, and GLRenderer::drawBitmapChar (and update all declarations and call sites) OR explicitly mark it as intentionally unused (e.g., add (void)font; or a [[maybe_unused]] annotation) and add a short comment stating it’s reserved for future font selection; ensure prototypes, implementations, and all callers are updated consistently so builds remain clean of unused-parameter warnings.
62-68:uStippleshader uniform is unused (dead code).The
loc_stippleuniform location is queried at line 198, and the shader contains stipple logic (lines 65-68), butglUniform1i(loc_stipple, ...)is never called. The stippling is now implemented CPU-side indrawStippledLines(). Consider removing the unused shader stipple path.♻️ Suggested removal
In shader source:
uniform sampler2D uTexture; uniform int uUseTexture; -uniform int uStipple; out vec4 FragColor; void main() { - if (uStipple != 0) { - float p = gl_FragCoord.x + gl_FragCoord.y; - if (mod(p, 4.0) < 2.0) discard; - } if (uUseTexture != 0)In
init():loc_useTexture = glGetUniformLocation(program, "uUseTexture"); loc_texture = glGetUniformLocation(program, "uTexture"); - loc_stipple = glGetUniformLocation(program, "uStipple");And remove
loc_stipplemember from the header.Also applies to: 198-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 62 - 68, The fragment shader contains an unused uniform uStipple and the C++ member loc_stipple is queried but never set; remove the dead shader stipple path and its C++ references: delete the uStipple logic from the shader source, remove the loc_stipple lookup in init() and the loc_stipple field from the class header, and ensure callers continue using the CPU-side drawStippledLines() implementation instead of setting any stipple uniform.
76-130: Font atlas generation may produce platform-dependent results.The atlas is generated using wxWidgets' font rendering, which can vary across platforms (different font backends, anti-aliasing, etc.). For a map editor this is likely acceptable, but if consistent cross-platform text appearance is required, consider using a pre-rendered font atlas texture or a portable font rasterizer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 76 - 130, The current GLRenderer::initFontAtlas uses wxWidgets text rendering (wxBitmap/wxMemoryDC) which yields platform-dependent glyphs; replace this by generating or loading a consistent atlas: either load a pre-rendered RGBA font atlas image into fontAtlas (use the same texture setup code and populate rgba from the image) or rasterize a chosen TTF with a portable rasterizer (e.g., stb_truetype) to produce the same TEX_W/TEX_H RGBA buffer for ASCII 32..127; update the code paths that currently draw into wxMemoryDC (the wxBitmap/wxMemoryDC/dc.DrawText loop) to instead fill the rgba vector from the loaded image or from the rasterizer glyph bitmaps, then keep the existing OpenGL texture upload (glGenTextures/glTexImage2D) and assignments to fontGlyphW/fontGlyphH/fontAtlasCols/fontAtlasW/fontAtlasH.
246-258: Minor: Unbinding program after setting uniform is unnecessary.
glUseProgram(0)at line 257 unbinds the program immediately after setting the projection uniform. This is harmless but adds a small overhead since the program will be bound again influshBatch(). Consider keeping the program bound or deferring the uniform update to draw time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 246 - 258, The GLRenderer::setOrtho routine unbinds the shader program after uploading the projection matrix which is unnecessary; remove the glUseProgram(0) call (or guard it behind a conditional) so the program remains bound after glUniformMatrix4fv(loc_projection, ...) and avoid the redundant bind in flushBatch(); ensure you still call glUseProgram(program) before the uniform upload and leave program state management to the higher-level drawing flow (e.g., flushBatch()).
🤖 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/gl_renderer.cpp`:
- Around line 414-454: drawPolygon and drawTriangleFan contain identical
triangulation and draw logic; consolidate by implementing the triangulation + GL
draw sequence in a single helper (e.g., a private GLRenderer::drawFanImpl or
merge into drawPolygon) and have the other method call it to avoid duplication.
Move/keep calls to flushBatch(), creation of std::vector<Vertex> tris, the
for-loop that builds tris, and the GL sequence (glUseProgram, glBindVertexArray,
glUniform1i(loc_useTexture,0), glBindBuffer, glBufferData, glDrawArrays,
glBindVertexArray(0), glUseProgram(0)) inside the shared helper so both
GLRenderer::drawPolygon and GLRenderer::drawTriangleFan simply validate
vertexCount and forward parameters to the helper to preserve behavior.
- Around line 456-457: The font parameter is never used by the renderer (it uses
its internal fontAtlas) so either remove the unused parameter from
GLRenderer::drawText, GLRenderer::getCharWidth, and GLRenderer::drawBitmapChar
(and update all declarations and call sites) OR explicitly mark it as
intentionally unused (e.g., add (void)font; or a [[maybe_unused]] annotation)
and add a short comment stating it’s reserved for future font selection; ensure
prototypes, implementations, and all callers are updated consistently so builds
remain clean of unused-parameter warnings.
- Around line 62-68: The fragment shader contains an unused uniform uStipple and
the C++ member loc_stipple is queried but never set; remove the dead shader
stipple path and its C++ references: delete the uStipple logic from the shader
source, remove the loc_stipple lookup in init() and the loc_stipple field from
the class header, and ensure callers continue using the CPU-side
drawStippledLines() implementation instead of setting any stipple uniform.
- Around line 76-130: The current GLRenderer::initFontAtlas uses wxWidgets text
rendering (wxBitmap/wxMemoryDC) which yields platform-dependent glyphs; replace
this by generating or loading a consistent atlas: either load a pre-rendered
RGBA font atlas image into fontAtlas (use the same texture setup code and
populate rgba from the image) or rasterize a chosen TTF with a portable
rasterizer (e.g., stb_truetype) to produce the same TEX_W/TEX_H RGBA buffer for
ASCII 32..127; update the code paths that currently draw into wxMemoryDC (the
wxBitmap/wxMemoryDC/dc.DrawText loop) to instead fill the rgba vector from the
loaded image or from the rasterizer glyph bitmaps, then keep the existing OpenGL
texture upload (glGenTextures/glTexImage2D) and assignments to
fontGlyphW/fontGlyphH/fontAtlasCols/fontAtlasW/fontAtlasH.
- Around line 246-258: The GLRenderer::setOrtho routine unbinds the shader
program after uploading the projection matrix which is unnecessary; remove the
glUseProgram(0) call (or guard it behind a conditional) so the program remains
bound after glUniformMatrix4fv(loc_projection, ...) and avoid the redundant bind
in flushBatch(); ensure you still call glUseProgram(program) before the uniform
upload and leave program state management to the higher-level drawing flow
(e.g., flushBatch()).
In `@source/map_drawer.cpp`:
- Around line 161-168: The MapDrawer currently owns renderer via a raw pointer
(renderer) and manually deletes it in MapDrawer::~MapDrawer(), which is
error-prone; change renderer to std::unique_ptr<GLRenderer> in the MapDrawer
class declaration, initialize it with std::make_unique<GLRenderer>() inside the
MapDrawer constructor (replacing new GLRenderer()), and remove the explicit
delete and shutdown pattern in the destructor—either call renderer->shutdown()
if necessary and let the unique_ptr automatically destroy the GLRenderer when
MapDrawer is destroyed, and keep the existing Release() call as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23880d8a-ecd2-426f-b9bf-e1f1201c19b2
📒 Files selected for processing (4)
source/gl_renderer.cppsource/gl_renderer.hsource/light_drawer.cppsource/map_drawer.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- source/light_drawer.cpp
- source/gl_renderer.h
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
source/graphics.cpp (1)
1302-1316:⚠️ Potential issue | 🟠 Major
getSpriteID()still uses the stale flattening logic.This branch still ignores
_pattern_zand never normalizes_frame, so multi-depth or animated sprites can resolve the wrong sprite ID. OncegetIndex()is fixed for static sprites, reuse it here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/graphics.cpp` around lines 1302 - 1316, getSpriteID currently duplicates stale flattening logic, ignores _pattern_z and fails to normalize _frame; replace its manual index computation with a call to the corrected getIndex(...) so multi-depth and animated sprites resolve correctly. In GameSprite::getSpriteID use the same parameters and normalization logic as getIndex (including pattern_x, pattern_y, pattern_z, layers, and proper frame modulo handling) to compute the index, then bounds-check/normalize against numsprites as getIndex does and return spriteList[index]->id. Ensure you remove the old manual formula and reference getIndex(...) so future fixes to indexing affect both static and animated lookups.
🤖 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/graphics.cpp`:
- Around line 1100-1102: The current ternary routes _count < 0 to getIndex(...)
which can divide by zero when sprite_phase_size == 0 (static sprites); change
the fallback so that when sprite_phase_size is zero (or when the sprite is
non-animated) you use index 0 instead of calling getIndex. Locate the expression
computing v (the ternary using _count and getIndex) and replace it with logic
that checks sprite_phase_size (or the same condition used when computing
numsprites/frame % sprite_phase_size) and only calls getIndex when
sprite_phase_size > 0; otherwise set the index to 0.
- Around line 209-212: GraphicManager::getFreeTextureID currently returns raw GL
names which can be recycled by glDeleteTextures and cause
GLRenderer::current_texture-based batching to treat a new texture as the old
one; fix by either (A) ensuring every place that deletes textures calls into a
new invalidation API to reset GLRenderer::current_texture (e.g., add a
GraphicManager::destroyTexture/glDeleteTextures wrapper that calls
GLRenderer::invalidateCurrentTexture()) or (B) introduce a stable renderer-side
texture handle layer (e.g., create TextureHandle objects returned by
getFreeTextureID and map them to GLuints internally, updating GLRenderer to
compare handles instead of raw GLuints) and update code paths that call
glDeleteTextures to clear the handle map; pick one approach and apply it to the
code paths around getFreeTextureID, GLRenderer::current_texture, and all
glDeleteTextures call sites.
---
Duplicate comments:
In `@source/graphics.cpp`:
- Around line 1302-1316: getSpriteID currently duplicates stale flattening
logic, ignores _pattern_z and fails to normalize _frame; replace its manual
index computation with a call to the corrected getIndex(...) so multi-depth and
animated sprites resolve correctly. In GameSprite::getSpriteID use the same
parameters and normalization logic as getIndex (including pattern_x, pattern_y,
pattern_z, layers, and proper frame modulo handling) to compute the index, then
bounds-check/normalize against numsprites as getIndex does and return
spriteList[index]->id. Ensure you remove the old manual formula and reference
getIndex(...) so future fixes to indexing affect both static and animated
lookups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8e434d7-bed7-4e0f-b6bc-c52337b4ca3a
📒 Files selected for processing (1)
source/graphics.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/graphics.cpp (1)
1305-1320:⚠️ Potential issue | 🟠 Major
getSpriteIDstill duplicates index math and ignores_pattern_z.A past review flagged that this implementation drops
_pattern_zand doesn't normalize_framebysprite_phase_size. The_count < 0path at line 1310 can resolve the wrong sprite ID for multi-Z or animated sprites.The recommended fix was to use
getIndex()which now properly handlessprite_phase_sizeclamping (line 1097-1098).🐛 Suggested fix using getIndex()
uint32_t GameSprite::getSpriteID(int _layer, int _count, int _pattern_x, int _pattern_y, int _pattern_z, int _frame) { - uint32_t v; - if (_count >= 0) { - v = _count; - } else { - v = (((_frame)*pattern_y + _pattern_y) * pattern_x + _pattern_x) * layers + _layer; - } + uint32_t v = _count >= 0 + ? static_cast<uint32_t>(_count) + : static_cast<uint32_t>(getIndex(0, 0, _layer, _pattern_x, _pattern_y, _pattern_z, _frame)); if (v >= numsprites) { if (numsprites == 1) { v = 0; } else { v %= numsprites; } } return spriteList[v]->id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/graphics.cpp` around lines 1305 - 1320, The current getSpriteID duplicates index computation and ignores _pattern_z and sprite_phase_size; replace the manual math in GameSprite::getSpriteID with a call to the existing getIndex(...) helper that already handles sprite_phase_size clamping and full pattern (x,y,z) and layer logic: compute the normalized index via getIndex(_layer, _count, _pattern_x, _pattern_y, _pattern_z, _frame) (or the exact getIndex signature used in this class), then bounds-check/normalize against numsprites as before and return spriteList[index]->id; remove the manual multiplication/formula so multi-Z and animated sprites use the correct index logic.
🧹 Nitpick comments (2)
source/gl_renderer.cpp (2)
537-543:setColor()flushes batch even when the color hasn't changed.Every call to
setColor()triggersflushBatch(), which can cause unnecessary draw calls when the color is already set to the requested value or when no geometry is pending.♻️ Suggested optimization
void GLRenderer::setColor(uint8_t r, uint8_t g, uint8_t b, uint8_t a) { + if (textR == r && textG == g && textB == b && textA == a) { + return; + } flushBatch(); textR = r; textG = g; textB = b; textA = a; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 537 - 543, The setColor() method unconditionally calls flushBatch(), causing needless draws; change it to first compare the incoming r,g,b,a against current textR, textG, textB, textA and return early if they are identical, otherwise update the text* fields and only call flushBatch() if there is pending geometry to flush (check whatever batch state you have, e.g. vertex/element count or a “hasPendingGeometry” flag) before applying the new color; keep the flushBatch(), setColor(), and textR/textG/textB/textA symbols intact.
424-464:drawPolygonanddrawTriangleFanare identical — extract common helper.Both methods perform the same triangle-fan tessellation and immediate draw. This duplication increases maintenance burden.
♻️ Suggested consolidation
+void GLRenderer::drawTriangleFanInternal(const float* vertices, int vertexCount, uint8_t r, uint8_t g, uint8_t b, uint8_t a) { + if (vertexCount < 3) { + return; + } + flushBatch(); + std::vector<Vertex> tris; + for (int i = 1; i < vertexCount - 1; ++i) { + tris.push_back({ vertices[0], vertices[1], 0, 0, r, g, b, a }); + tris.push_back({ vertices[i * 2], vertices[i * 2 + 1], 0, 0, r, g, b, a }); + tris.push_back({ vertices[(i + 1) * 2], vertices[(i + 1) * 2 + 1], 0, 0, r, g, b, a }); + } + glUseProgram(program); + glBindVertexArray(vao); + glUniform1i(loc_useTexture, 0); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, tris.size() * sizeof(Vertex), tris.data(), GL_DYNAMIC_DRAW); + glDrawArrays(GL_TRIANGLES, 0, (GLsizei)tris.size()); + glBindVertexArray(0); + glUseProgram(0); +} + void GLRenderer::drawPolygon(const float* vertices, int vertexCount, uint8_t r, uint8_t g, uint8_t b, uint8_t a) { - // ... full implementation + drawTriangleFanInternal(vertices, vertexCount, r, g, b, a); } void GLRenderer::drawTriangleFan(const float* vertices, int vertexCount, uint8_t r, uint8_t g, uint8_t b, uint8_t a) { - // ... full implementation + drawTriangleFanInternal(vertices, vertexCount, r, g, b, a); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/gl_renderer.cpp` around lines 424 - 464, Both GLRenderer::drawPolygon and GLRenderer::drawTriangleFan duplicate the same tessellation and immediate-draw logic; extract that shared logic into a private helper (e.g., GLRenderer::drawTriangleFanImmediate or GLRenderer::tessellateAndDrawFan) that takes (const float* vertices, int vertexCount, uint8_t r, uint8_t g, uint8_t b, uint8_t a), performs the for-loop building std::vector<Vertex>, calls flushBatch(), binds program/vao/vbo, uploads with glBufferData, sets loc_useTexture to 0, and issues glDrawArrays, and then replace both drawPolygon and drawTriangleFan to validate vertexCount and simply call the new helper; keep existing symbols (Vertex, flushBatch, program, vao, loc_useTexture, vbo) 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/gl_renderer.cpp`:
- Around line 139-143: GLRenderer::init currently overwrites the static
s_instance each time a MapTab/MapWindow creates a GLRenderer, causing
invalidateTexture (in graphics.cpp) to only update the last-initialized
renderer's current_texture and leaving stale textures in other tabs; fix by
removing this unconditional s_instance assignment and instead ensure all
renderers are tracked or passed explicitly: either convert GLRenderer to a true
singleton (centralize creation so only one instance exists), or remove
s_instance and modify invalidateTexture to accept a GLRenderer* (or reference)
so callers pass the correct renderer, or implement a registry inside GLRenderer
(e.g., register/unregister instances in ctor/dtor) and update invalidateTexture
to iterate all registered instances and clear their current_texture; update
GLRenderer::init, the constructor/destructor, and callers of invalidateTexture
(graphics.cpp) accordingly to use the chosen approach.
---
Duplicate comments:
In `@source/graphics.cpp`:
- Around line 1305-1320: The current getSpriteID duplicates index computation
and ignores _pattern_z and sprite_phase_size; replace the manual math in
GameSprite::getSpriteID with a call to the existing getIndex(...) helper that
already handles sprite_phase_size clamping and full pattern (x,y,z) and layer
logic: compute the normalized index via getIndex(_layer, _count, _pattern_x,
_pattern_y, _pattern_z, _frame) (or the exact getIndex signature used in this
class), then bounds-check/normalize against numsprites as before and return
spriteList[index]->id; remove the manual multiplication/formula so multi-Z and
animated sprites use the correct index logic.
---
Nitpick comments:
In `@source/gl_renderer.cpp`:
- Around line 537-543: The setColor() method unconditionally calls flushBatch(),
causing needless draws; change it to first compare the incoming r,g,b,a against
current textR, textG, textB, textA and return early if they are identical,
otherwise update the text* fields and only call flushBatch() if there is pending
geometry to flush (check whatever batch state you have, e.g. vertex/element
count or a “hasPendingGeometry” flag) before applying the new color; keep the
flushBatch(), setColor(), and textR/textG/textB/textA symbols intact.
- Around line 424-464: Both GLRenderer::drawPolygon and
GLRenderer::drawTriangleFan duplicate the same tessellation and immediate-draw
logic; extract that shared logic into a private helper (e.g.,
GLRenderer::drawTriangleFanImmediate or GLRenderer::tessellateAndDrawFan) that
takes (const float* vertices, int vertexCount, uint8_t r, uint8_t g, uint8_t b,
uint8_t a), performs the for-loop building std::vector<Vertex>, calls
flushBatch(), binds program/vao/vbo, uploads with glBufferData, sets
loc_useTexture to 0, and issues glDrawArrays, and then replace both drawPolygon
and drawTriangleFan to validate vertexCount and simply call the new helper; keep
existing symbols (Vertex, flushBatch, program, vao, loc_useTexture, vbo)
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea4f7d77-6143-4ea3-bb30-ccf36a3fb834
📒 Files selected for processing (5)
source/gl_renderer.cppsource/gl_renderer.hsource/graphics.cppsource/light_drawer.cppsource/map_drawer.cpp
- Remove commented-out code in map_window.cpp - Use in-class initializers for light_drawer and renderer (map_drawer.h/cpp) - Replace C-style arrays with std::array (map_drawer.cpp, map_display.cpp) - Mark unused parameter brush as [[maybe_unused]] (DrawBrushIndicator) - Reduce cognitive complexity in DrawTooltips (extract MeasureTooltipText/RenderTooltipText) - Fix variable shadowing and add const to loop variable in DrawTooltips - Join duplicate switch cases in getBrushColor - Fix keyboard shortcuts on Linux via DispatchMenuShortcut (map_display.cpp) - Remove redundant Linux accelerator table (main_menubar.cpp) - Remap Q to SHOW_SHADE, B to SelectPreviousBrush, Shift+N to SHOW_NPCS
Author
Gravacao.de.tela.de.2026-04-14.21-21-24.webm |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
source/map_display.cpp (2)
1887-1899: Consider extracting menu toggle logic into a helper.The 'Q' key handler manually toggles the menu item and dispatches an event. This pattern is duplicated in
DispatchMenuShortcut. Consider extracting a helper method to reduce code duplication:void MapCanvas::ToggleMenuCheckItem(int menuId) { int fullId = static_cast<int>(MAIN_FRAME_MENU) + menuId; wxMenuBar* mb = g_gui.root->GetMenuBar(); if (mb) { wxMenuItem* item = mb->FindItem(fullId); if (item && item->IsCheckable()) { item->Check(!item->IsChecked()); } } wxCommandEvent evt(wxEVT_MENU, fullId); g_gui.root->GetEventHandler()->ProcessEvent(evt); }Also note:
wxEVT_COMMAND_MENU_SELECTEDis the legacy name;wxEVT_MENUis preferred in modern wxWidgets (3.0+).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map_display.cpp` around lines 1887 - 1899, Extract the duplicated menu-toggle-and-dispatch logic into a helper on MapCanvas (e.g., MapCanvas::ToggleMenuCheckItem) that takes the relative menuId, computes fullId via static_cast<int>(MAIN_FRAME_MENU) + menuId, finds the wxMenuBar via g_gui.root->GetMenuBar(), toggles the check state if the found wxMenuItem is checkable, then dispatches the event with wxEVT_MENU to g_gui.root->GetEventHandler()->ProcessEvent; replace the inline block in the 'q'/'Q' case and the similar code in DispatchMenuShortcut to call this helper.
1989-2168: Hard-coded shortcut mapping may become a maintenance burden.The
DispatchMenuShortcutfunction serves as a necessary workaround for wxGTK's accelerator propagation issue on Linux. However, the hard-coded mapping between keys and menu IDs duplicates information that should ideally come from the menu/accelerator definitions.Two observations:
Missing shortcuts: Common shortcuts like
Ctrl+S(save),Ctrl+O(open),Ctrl+N(new) are not handled here, despite existing in the menu definitions. Verify if these are intentionally omitted or should be added for consistency on Linux.Potential for inconsistency: If menu accelerators change in the menu definitions, this function must be updated manually to stay in sync.
For now this is acceptable, but consider documenting that this hard-coded mapping must be kept in sync with the menu definitions when they change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map_display.cpp` around lines 1989 - 2168, DispatchMenuShortcut currently hard-codes key->menu ID mappings (function DispatchMenuShortcut, enum MenuBar, MAIN_FRAME_MENU) which misses common accelerators (e.g., Ctrl+S/O/N) and can diverge from real menu definitions; replace the hard-coded switch logic by querying the application's wxMenuBar (g_gui.root->GetMenuBar()) and iterating its wxMenuItems to find the item whose accelerator string or GetAccel() matches the event's key+modifier state, then synthesize and process a wxEVT_COMMAND_MENU_SELECTED for that item's id (or, if you prefer a minimal fix, add the missing mappings for 'S','O','N' to the existing ctrl-case and add a short comment warning to keep this in sync with MenuBar definitions).source/map_drawer.cpp (1)
41-41: Unused include.
<unordered_set>appears to be unused in this file. Consider removing it if not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map_drawer.cpp` at line 41, Remove the unused header include in map_drawer.cpp: delete the line including <unordered_set> since it is not referenced by any symbols in this file (e.g., nothing in MapDrawer, drawMap, or related code uses std::unordered_set); after removal, run a quick build to confirm no missing dependencies and re-add only if a compilation error shows a real need.
🤖 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/map_display.cpp`:
- Around line 110-121: Remove the dead static constexpr std::array named
attribList which is never used; update source by deleting the declaration of
attribList and ensure MapCanvas::MapCanvas continues to accept the incoming
parameter attriblist (keep the constructor signature and usage of the parameter
as-is) so callers still provide their own attribute list; verify no other
references to the symbol attribList remain in the file.
In `@source/map_drawer.cpp`:
- Around line 1839-1846: The inline key construction for tooltips differs from
the tooltipKey() helper and can produce incorrect keys for negative coordinates;
replace the manual key computation at the block using tp, tooltipFadeAlpha, and
newFadeAlpha with a call to tooltipKey(tp) (or replicate the exact casting logic
from tooltipKey) so fade lookup/update uses the same key format, then proceed to
read prev from tooltipFadeAlpha and write newFadeAlpha[key] as before.
---
Nitpick comments:
In `@source/map_display.cpp`:
- Around line 1887-1899: Extract the duplicated menu-toggle-and-dispatch logic
into a helper on MapCanvas (e.g., MapCanvas::ToggleMenuCheckItem) that takes the
relative menuId, computes fullId via static_cast<int>(MAIN_FRAME_MENU) + menuId,
finds the wxMenuBar via g_gui.root->GetMenuBar(), toggles the check state if the
found wxMenuItem is checkable, then dispatches the event with wxEVT_MENU to
g_gui.root->GetEventHandler()->ProcessEvent; replace the inline block in the
'q'/'Q' case and the similar code in DispatchMenuShortcut to call this helper.
- Around line 1989-2168: DispatchMenuShortcut currently hard-codes key->menu ID
mappings (function DispatchMenuShortcut, enum MenuBar, MAIN_FRAME_MENU) which
misses common accelerators (e.g., Ctrl+S/O/N) and can diverge from real menu
definitions; replace the hard-coded switch logic by querying the application's
wxMenuBar (g_gui.root->GetMenuBar()) and iterating its wxMenuItems to find the
item whose accelerator string or GetAccel() matches the event's key+modifier
state, then synthesize and process a wxEVT_COMMAND_MENU_SELECTED for that item's
id (or, if you prefer a minimal fix, add the missing mappings for 'S','O','N' to
the existing ctrl-case and add a short comment warning to keep this in sync with
MenuBar definitions).
In `@source/map_drawer.cpp`:
- Line 41: Remove the unused header include in map_drawer.cpp: delete the line
including <unordered_set> since it is not referenced by any symbols in this file
(e.g., nothing in MapDrawer, drawMap, or related code uses std::unordered_set);
after removal, run a quick build to confirm no missing dependencies and re-add
only if a compilation error shows a real need.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faa3a49a-8250-488e-b5fd-85a0467535ad
⛔ Files ignored due to path filters (1)
data/fonts/DejaVuSans.ttfis excluded by!**/*.ttf
📒 Files selected for processing (17)
.gitignoredata/menubar.xmlsource/CMakeLists.txtsource/gl_renderer.cppsource/gl_renderer.hsource/graphics.cppsource/graphics.hsource/light_drawer.cppsource/main_menubar.cppsource/map_display.cppsource/map_display.hsource/map_drawer.cppsource/map_drawer.hsource/map_window.cppsource/stb_truetype_impl.cppvcpkg.jsonvcproj/Project/RME.vcxproj
💤 Files with no reviewable changes (1)
- source/main_menubar.cpp
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- source/stb_truetype_impl.cpp
- data/menubar.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- source/map_window.cpp
- vcpkg.json
- source/CMakeLists.txt
- source/graphics.cpp
|
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
1 participant
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
Migrate the entire rendering pipeline from legacy OpenGL (immediate mode / Compatibility Profile) to a shader-based architecture using
GLRendererwith vertex batching. Fix keyboard shortcuts on Linux (wxGTK3), add performance stats overlay, and address SonarCloud code quality warnings.Changes
Stage 1 — GLRenderer abstraction
GLRendererclass (gl_renderer.h,gl_renderer.cpp) with a unified shader program, VAO/VBO batching, and orthographic projection via uniformGL/gl.hincludesStage 2 — Migrate draw calls to GLRenderer
glBegin/glEndimmediate-mode draw calls inmap_drawer.cppwith batched renderer methods:drawTexturedQuad,drawColoredQuad,drawLine,drawRect,drawFilledCircle,drawCircle,drawPolygonglBlitTextureto use the shader-based renderer while preserving sprite sheet offset logicenableTexture/disableTextureto no-ops (shader handles texture state viauUseTextureuniform)Stage 3 — Modernize texture management
loaded_textures) withglGenTexturesingetFreeTextureID()id) from GL texture ID (glTextureId) inGameSprite::NormalImageandEditorImagegetDrawOffset,getWidth,getHeight,getDC,DrawTo,BlitItem,BlitSpriteType,BlitCreature,OutfitImage::getRGBAData) to use->idinstead ofgetHardwareID()OutfitImage::unloadGLTextureto deletem_textureIdinstead ofm_spriteId#define GL_CLAMP_TO_EDGE 0x812Fblocks (already provided by GLAD)Stage 4 — Font atlas text rendering and performance stats
wxFont+wxMemoryDC(monospace, ASCII 32–127)drawText,drawBitmapChar,getCharWidth, andsetRasterPosusing textured quads from the atlas, fully replacingglutBitmapCharacter/glutBitmapWidthDrawTooltipsandDrawPerformanceStatsto use the new text rendererDrawPerformanceStatswith FPS, CPU, and RAM monitoring overlayMeasureTooltipTextandRenderTooltipTexthelper methods fromDrawTooltipsto reduce cognitive complexityStage 5 — Remove GLUT dependency
#include <GL/glut.h>fromgl_renderer.h,application.cpp, andmap_drawer.cppglutInitcall fromApplication::OnInitfind_package(GLUT REQUIRED),GLUT::GLUT, and${GLUT_INCLUDE_DIRS}fromCMakeLists.txt#if defined(__LINUX__) || defined(__WINDOWS__)guards from text rendering methods andDrawTooltipsStage 6 — Final cleanup and Core Profile context
glMatrixMode,glPushMatrix,glPopMatrix,glLoadIdentity,glOrtho,glTranslatef) frommap_drawer.cppglColor4ubfromGLRenderer::setColor— colors are passed via vertex attributesdrawStippledLinesfromGL_LINE_STIPPLE(deprecated) to a fragment shader discard pattern viauStippleuniformglLoadIdentityfromDrawBackgroundwxGLCanvasattributes inmap_window.cppStage 7 — Fix keyboard shortcuts on Linux (wxGTK3)
DispatchMenuShortcutinmap_display.cppto manually dispatch menu hotkeys fromwxGLCanvas, since wxGTK3 does not propagate key events to the frame's accelerator tablemain_menubar.cpp(#ifdef __LINUX__block with 47wxAcceleratorEntryentries) — it intercepted key events beforeOnKeyDown, preventing check menu items from toggling correctlyQfrom SelectPreviousBrush to SHOW_SHADE (matchingmenubar.xml)Bas the new key for SelectPreviousBrushXtoShift+Nto avoid conflict with rotate CWStage 8 — SonarCloud code quality fixes
std::array(map_drawer.cpp,gl_renderer.cpp)typedefwithusingfor type aliasing (GLuint,GLint)#defineconstants withconstexpr(GL_CLAMP_TO_EDGE)void* font,_pattern_z)tooltip→tpin tooltip helper methods)constcorrectness to pointer loop variablesswitchcases ingetBrushColorGLColor,Vertex)FontAtlas,GLColor textColor) to reduce field countstd::erase(C++20)autowithstatic_castinstead of C-style castsFiles changed
source/gl_renderer.hFontAtlasandGLColorsub-structssource/gl_renderer.cppsource/map_drawer.cppsource/map_drawer.hGLRenderer* renderer,LightDrawer* light_drawerwith in-class initializers, performance stats fields,MeasureTooltipText,RenderTooltipTextsource/map_display.cppDispatchMenuShortcutfor Linux, remap Q/B/X shortcutssource/map_window.cppsource/main_menubar.cppsource/graphics.hglTextureIdtoImage,getSpriteIDtoGameSpritesource/graphics.cppsource/application.cppsource/light_drawer.cpp#define GL_CLAMP_TO_EDGEblocksource/light_drawer.hdata/menubar.xmlXtoShift+Nsource/CMakeLists.txtTesting
How to test
Open a map — Load any
.otbmmap file. Verify the editor window opens without a black screen or crash. Ground tiles, borders, walls, and mountains should all render with correct sprites and positioning.Zoom — Use
Ctrl++to zoom in andCtrl+-to zoom out. Verify sprites scale correctly at all zoom levels. UseCtrl+0to reset to 100%. Items should hide when zoomed out far enough (if "Hide items when zoomed" is enabled).Navigate floors — Switch between floors using the Floor menu or floor shortcuts. Verify tiles render on each floor. Enable View > Show shade (
Q) and confirm the shade overlay appears on lower floors.Tooltips — Enable View > Show Tooltips (
Y). Hover over tiles with items — a tooltip with item names should appear with a dark background, correct text, and proper line wrapping. Long tooltips should show ellipsis (...). Waypoints should show a green tooltip withwp: name.Grid — Toggle View > Show Grid (
Shift+G). A stippled (dashed) grid overlay should appear over all tiles. Verify the lines are evenly spaced and dashed (not solid).Selection box — Click and drag to select an area. The selection box should render with stippled (dashed) white lines. Selected items should appear darker (halved RGB).
Creatures and NPCs — Open the Creature Palette (
C) or NPC Palette (N) and place creatures/NPCs on the map. Verify they render with correct outfit colors and addons. Toggle View > Show monsters (F) and View > Show npcs (Shift+N) to confirm visibility toggles work.Lights — Enable View > Lights > Show lights (
Shift+L). Place items with light sources (e.g., torches). Verify the light overlay renders with correct colors and blending. Toggle Show light strength (Shift+K) to see light strength indicators.Performance stats — Enable performance stats via Edit > Preferences > Interface > Show Performance Stats. An FPS/CPU/RAM overlay should appear in the top-left corner with yellow text.
Brush indicators — Select a ground brush from the Terrain Palette (
T) and hover over the map. A circular indicator with a polygon tooltip should appear at the cursor position.Special tiles — Toggle View > Show special (
E), Show houses (Ctrl+H), and Show pathing (O). PZ tiles, house tiles, and blocking tiles should render with colored overlays.Minimap mode — Toggle View > Show as minimap (
Shift+E). The map should render using only minimap colors (flat colored squares instead of sprites).Ingame box — Toggle View > Show client box (
Shift+I). A shadow overlay should appear around the edges, showing the visible area from the center of the screen.Wall hooks — Enable View > Show Indicators > Show Wall Hooks (
K). Place hangable items near walls — blue diamond indicators should appear showing hook directions.Screenshot — Press
F10to take a screenshot. Verify the saved image matches what is displayed on screen.Paste preview — Copy a selection (
Ctrl+C) and paste (Ctrl+V). The paste preview should render with ghosted (semi-transparent) sprites following the cursor.Keyboard shortcuts (Linux) — Verify all single-key shortcuts work:
Y(tooltips),L(preview),Q(shade),B(previous brush),F(monsters),S(spawns),O(pathing). Verify shift shortcuts:Shift+L(lights),Shift+N(NPCs),Shift+G(grid),Shift+E(minimap),Shift+I(ingame box),Shift+K(light strength).Cross-platform — If possible, test on both Windows and Linux to verify the font atlas generates correctly on each platform (uses system monospace font via
wxFont).Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores