use instanced rendering for rooms, drastically reducing VRAM usage #446
Conversation
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
Reviewer's GuideIntroduce instanced room quad rendering with a dedicated shader and shared buffers (including a uniform named-colors UBO and a 1x1 white texture), refactor room tint/highlight and diff rendering to use these instanced meshes, extend the legacy OpenGL wrapper for uniform blocks and instancing, and modernize several Qt dialogs/menus to be non-blocking and async-friendly, plus various build and utility tweaks. Sequence diagram for instanced room tint and highlight rendering with named-colors UBOsequenceDiagram
actor User
participant MapCanvas
participant Diff as MapCanvas_Diff
participant OpenGL
participant Legacy as Legacy_Functions
participant ShaderPrograms
participant RoomQuadShader as RoomQuadTexShader
participant SharedVbos
User->>MapCanvas: triggers repaint (paintGL)
MapCanvas->>OpenGL: bindNamedColorsBuffer()
OpenGL->>SharedVbos: get(NamedColorsBlock)
alt first_time_alloc
SharedVbos-->>OpenGL: empty SharedVbo
OpenGL->>SharedVbos: emplace VBO
OpenGL->>Legacy: setUbo(vbo, XNamedColor::getAllColorsAsVec4())
end
OpenGL->>Legacy: glBindBufferBase(GL_UNIFORM_BUFFER, NamedColorsBlock, vbo)
MapCanvas->>Diff: maybeAsyncUpdate(savedMap, currentMap)
activate Diff
Diff-->>Diff: compute DiffQuadVector(RoomQuadTexVert)
Diff-->>MapCanvas: futureHighlight with DiffQuadVector
deactivate Diff
MapCanvas->>OpenGL: Diff::MaybeDataOrMesh::uploadOrRender(gl, room_highlight_tex)
alt holds DiffQuadVector
OpenGL->>OpenGL: createRoomQuadTexBatch(DiffQuadVector, room_highlight_tex)
OpenGL->>Legacy: createRoomQuadTexBatch(...)
Legacy->>ShaderPrograms: getRoomQuadTexShader()
ShaderPrograms-->>Legacy: RoomQuadTexShader
Legacy-->>OpenGL: UniqueMesh(RoomQuadTexMesh)
end
MapCanvas->>OpenGL: render UniqueMesh with gl.getDefaultRenderState().withBlend(TRANSPARENCY)
OpenGL->>RoomQuadShader: bind program and uniforms
OpenGL->>Legacy: drawRoomQuad(m_functions, numVerts)
Legacy->>SharedVbos: get(InstancedQuadIbo)
alt first_time_alloc_ibo
SharedVbos-->>Legacy: empty SharedVbo
Legacy->>SharedVbos: emplace VBO
Legacy->>Legacy: setIbo(indices=[0,1,2,3])
end
Legacy-->>Legacy: glDrawElementsInstanced(TRIANGLE_FAN, 4, UNSIGNED_BYTE, null, numVerts)
Updated class diagram for instanced room quad rendering and shared VBOsclassDiagram
class OpenGL {
+UniqueMesh createColoredTexturedQuadBatch(vector~ColoredTexVert~, MMTextureId)
+UniqueMesh createRoomQuadTexBatch(vector~RoomQuadTexVert~, MMTextureId)
+GLRenderState getDefaultRenderState()
+void bindNamedColorsBuffer()
+void resetNamedColorsBuffer()
}
class Legacy_Functions {
-unique_ptr~ShaderPrograms~ m_shaderPrograms
-unique_ptr~StaticVbos~ m_staticVbos
-unique_ptr~SharedVbos~ m_sharedVbos
+void applyDefaultUniformBlockBindings(GLuint program)
+void glBindBufferBase(GLenum target, SharedVboEnum block, GLuint buffer)
+void glUniformBlockBinding(GLuint program, SharedVboEnum block)
+pair~DrawModeEnum,GLsizei~ setVbo~T~(DrawModeEnum, GLuint, vector~T~, BufferUsageEnum)
+GLsizei setIbo~T~(GLuint, vector~T~, BufferUsageEnum)
+GLsizei setUbo~T~(GLuint, vector~T~, BufferUsageEnum)
+void enableAttribI(GLuint, GLint, GLenum, GLsizei, GLvoid*)
+UniqueMesh createColoredTexturedBatch(DrawModeEnum, vector~ColoredTexVert~, MMTextureId)
+UniqueMesh createRoomQuadTexBatch(vector~RoomQuadTexVert~, MMTextureId)
+StaticVbos& getStaticVbos()
+SharedVbos& getSharedVbos()
+void cleanup()
+optional~GLenum~ virt_toGLenum(DrawModeEnum)
}
class DrawModeEnum {
<<enum>>
INVALID
POINTS
LINES
TRIANGLES
QUADS
INSTANCED_QUADS
}
class SharedVboEnum {
<<enum>>
NamedColorsBlock
InstancedQuadIbo
}
class SharedVbos {
-EnumIndexedArray~SharedVbo,SharedVboEnum,NUM_SHARED_VBOS~ base
+SharedVbo get(SharedVboEnum)
+void reset(SharedVboEnum)
+void resetAll()
}
class StaticVbos {
-vector~SharedVbo~ base
+SharedVbo get(size_t)
+void resetAll()
}
class VBO {
-SharedFunctions m_functions
-GLuint m_id
+VBO(SharedFunctions)
+GLuint get() const
+explicit operator bool() const
}
class ShaderPrograms {
-Functions &m_functions
-shared_ptr~AColorPlainShader~ m_aColorShader
-shared_ptr~UColorPlainShader~ m_uColorShader
-shared_ptr~AColorTexturedShader~ m_aTexturedShader
-shared_ptr~UColorTexturedShader~ m_uTexturedShader
-shared_ptr~RoomQuadTexShader~ m_roomQuadTexShader
-shared_ptr~FontShader~ m_font
-shared_ptr~PointShader~ m_point
+void resetAll()
+void early_init()
+shared_ptr~AColorPlainShader~ getPlainAColorShader()
+shared_ptr~UColorPlainShader~ getPlainUColorShader()
+shared_ptr~AColorTexturedShader~ getTexturedAColorShader()
+shared_ptr~UColorTexturedShader~ getTexturedUColorShader()
+shared_ptr~RoomQuadTexShader~ getRoomQuadTexShader()
+shared_ptr~FontShader~ getFontShader()
+shared_ptr~PointShader~ getPointShader()
}
class AbstractShaderProgram {
-GLuint m_program
+void setColor(const char* name, Color)
+void setMatrix(const char* name, mat4)
+void setTexture(const char* name, int unit)
+GLuint getProgramId() const
+int getAttribLocation(const char* name) const
+virtual void virt_setUniforms(mat4, GLRenderState::Uniforms) = 0
}
class RoomQuadTexShader {
+~RoomQuadTexShader()
+void virt_setUniforms(mat4 mvp, GLRenderState::Uniforms uniforms)
}
class SimpleMesh~VertexType_,ProgramType_~ {
-SharedFunctions m_shared_functions
-Functions &m_functions
-ProgramType_ &m_program
-optional~VBO~ m_vbo
-DrawModeEnum m_drawMode
-GLsizei m_numVerts
+void updateVerts(vector~VertexType_~, DrawModeEnum, BufferUsageEnum)
+void render(GLRenderState) const
-void virt_bind()
-void virt_unbind()
}
class RoomQuadTexMesh~VertexType_~ {
-optional~Attribs~ m_boundAttribs
-void virt_bind()
-void virt_unbind()
}
class Attribs {
+GLuint vertTexColPos
+static Attribs getLocations(RoomQuadTexShader&)
}
class RoomQuadTexVert {
+ivec4 vertTexCol
+RoomQuadTexVert(ivec3 vert, int tex_z)
+RoomQuadTexVert(ivec3 vert, int tex_z, NamedColorEnum color)
}
class NamedColorEnum {
<<enum>>
DEFAULT
TRANSPARENT
ROOM_DARK
ROOM_NO_SUNDEATH
HIGHLIGHT_TEMPORARY
HIGHLIGHT_NEEDS_SERVER_ID
HIGHLIGHT_UNSAVED
+...
}
class XNamedColor {
+static const vector~Color~& getAllColors()
+static const vector~vec4~& getAllColorsAsVec4()
}
class LayerMeshesIntermediate {
<<struct>>
+typedef function~UniqueMesh(OpenGL&)~ Fn
+typedef vector~Fn~ FnVec
+FnVec terrain
+FnVec trails
+RoomTintArray~Fn~ tints
+FnVec overlays
+FnVec doors
+FnVec walls
+FnVec exits
+FnVec upDownExits
+FnVec streamIns
+FnVec streamOuts
+Fn layerBoost
+bool isValid
}
class LayerBatchData {
+MMTexArrayPosition white_pixel
+RoomTexVector roomTerrains
+RoomTexVector roomTrails
+RoomTexVector roomOverlays
+ColoredRoomTexVector doors
+ColoredRoomTexVector solidWallLines
+ColoredRoomTexVector exits
+ColoredRoomTexVector roomUpDownExits
+RoomTexVector streamIns
+RoomTexVector streamOuts
+RoomTintArray~RoomTintVector~ roomTints
+RoomTintVector roomLayerBoostQuads
+LayerBatchData(MMTexArrayPosition white_pixel)
+LayerMeshesIntermediate createIntermediate() const
}
class RoomTint {
+Coordinate coord
+NamedColorEnum color
+RoomTint(Coordinate, NamedColorEnum)
}
class RoomTintVector {
<<typedef>> vector~RoomTint~
}
class MapCanvas_Diff {
+typedef vector~RoomQuadTexVert~ DiffQuadVector
+struct MaybeDataOrMesh
+future~HighlightDiff~ futureHighlight
}
class MapCanvas_Diff_MaybeDataOrMesh {
<<variant>>
+bool empty() const
+bool hasData() const
+bool hasMesh() const
+const DiffQuadVector& getData() const
+const UniqueMesh& getMesh() const
+void uploadOrRender(OpenGL&, MMTextureId)
}
class GLRenderState {
+GLRenderState withDepthFunction(DepthFunctionEnum) const
+GLRenderState withBlend(BlendModeEnum) const
+GLRenderState withColor(Color) const
}
%% Relationships
OpenGL --> Legacy_Functions : uses
Legacy_Functions --> ShaderPrograms : owns
Legacy_Functions --> StaticVbos : owns
Legacy_Functions --> SharedVbos : owns
SharedVbos --> VBO : returns shared VBO
StaticVbos --> VBO : returns shared VBO
ShaderPrograms --> RoomQuadTexShader : creates
RoomQuadTexShader --|> AbstractShaderProgram
RoomQuadTexMesh --|> SimpleMesh~RoomQuadTexVert,RoomQuadTexShader~
openGLTypes_DrawModeEnum <|-- DrawModeEnum
Legacy_Functions --> DrawModeEnum : uses
RoomQuadTexVert --> NamedColorEnum : embeds
MapCanvas_Diff_MaybeDataOrMesh --> RoomQuadTexVert : uses DiffQuadVector
MapCanvas_Diff_MaybeDataOrMesh --> UniqueMesh : holds
LayerBatchData --> LayerMeshesIntermediate : createIntermediate
LayerBatchData --> RoomTintVector
LayerBatchData --> RoomTexVector
LayerMeshesIntermediate --> OpenGL : Fn/OpenGL&
OpenGL --> RoomQuadTexVert : createRoomQuadTexBatch
OpenGL --> RoomQuadTexMesh : createRoomQuadTexBatch
OpenGL --> SharedVbos : bindNamedColorsBuffer
SharedVbos --> VBO : get(NamedColorsBlock)
XNamedColor --> OpenGL : getAllColorsAsVec4
GLRenderState <.. OpenGL : getDefaultRenderState
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
updateNamedColorsUBO()and the lambda inrenderMapBatches()both create and upload the named-colors UBO using a static weak VBO; consider consolidating this into a single helper (or just reusingupdateNamedColorsUBO()) to avoid duplicated logic and potential divergence.createMeshFn/Resolver::resolvecontainconstexpr boolflags and null-function handling paths that are effectively dead code; cleaning these up or replacing them with a clearer optional/empty-fn convention would make the lazy mesh generation easier to reason about.- The
getTintColor(RoomTintEnum)and UBO index packing inRoomQuadTexVertboth assume small, bounded enum ranges; adding explicit static_asserts or comments tying the enum ranges toMAX_NAMED_COLORSwould help prevent subtle issues if new tint or named color values are introduced later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `updateNamedColorsUBO()` and the lambda in `renderMapBatches()` both create and upload the named-colors UBO using a static weak VBO; consider consolidating this into a single helper (or just reusing `updateNamedColorsUBO()`) to avoid duplicated logic and potential divergence.
- `createMeshFn`/`Resolver::resolve` contain `constexpr bool` flags and null-function handling paths that are effectively dead code; cleaning these up or replacing them with a clearer optional/empty-fn convention would make the lazy mesh generation easier to reason about.
- The `getTintColor(RoomTintEnum)` and UBO index packing in `RoomQuadTexVert` both assume small, bounded enum ranges; adding explicit static_asserts or comments tying the enum ranges to `MAX_NAMED_COLORS` would help prevent subtle issues if new tint or named color values are introduced later.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas_gl.cpp:1013-1022` </location>
<code_context>
+void MapCanvas::updateNamedColorsUBO()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid duplicating named-colors UBO setup logic by reusing `updateNamedColorsUBO()`
The static `Legacy::VBO` allocation, upload of `XNamedColor::getAllColorsAsVec4()`, and assignment of `m_named_colors_buffer_id` now live both in `updateNamedColorsUBO()` and inline in `renderMapBatches()`. This duplication risks the two paths diverging (e.g., if `MAX_NAMED_COLORS` or buffer usage changes) and can create two separate static `weak_ptr<VBO>`s instead of a single shared buffer.
To avoid this, have `renderMapBatches()` call `updateNamedColorsUBO()` instead, and make that function idempotent/cheap when no data changes. That keeps all named-color UBO handling in one place and reduces maintenance risk.
Suggested implementation:
```cpp
// Ensure named colors UBO is allocated and up to date
updateNamedColorsUBO();
```
I assumed the duplicated block in `renderMapBatches()` matches the inline named-colors UBO logic you moved into `updateNamedColorsUBO()` (static `g_weak_vbo`, `XNamedColor::getAllColorsAsVec4()`, and `m_named_colors_buffer_id` assignment). If the actual code differs slightly (e.g., variable names, comments, or `bufferStatic` call signature), adjust the `SEARCH` section to match your current `renderMapBatches()` implementation exactly, keeping the `REPLACE` section as a simple `updateNamedColorsUBO();` call.
</issue_to_address>
### Comment 2
<location> `src/display/mapcanvas_gl.cpp:1026-1035` </location>
<code_context>
+ vbo.emplace(fns);
+ }
+
+ m_named_colors_buffer_id = std::invoke([&gl, &shared_vbo]() {
+ auto &fns = gl.getSharedFunctions(Badge<MapCanvas>{});
+ Legacy::VBO &vbo = deref(shared_vbo);
+
+ // the shader is declared vec4, so the data has to be 4 floats per entry.
+ const auto &vec4_colors = XNamedColor::getAllColorsAsVec4();
+
+ // Can we avoid the upload if it's not necessary?
+ MAYBE_UNUSED const auto result = fns->setUbo(vbo.get(),
+ vec4_colors,
+ BufferUsageEnum::DYNAMIC_DRAW);
+ assert(result == static_cast<int>(vec4_colors.size()));
+ return vbo.get();
+ });
+}
</code_context>
<issue_to_address>
**issue (performance):** `renderMapBatches()` re-uploads the named-colors UBO even though `updateNamedColorsUBO()` already does it
`actuallyPaintGL()` already calls `updateNamedColorsUBO()` every frame, but `renderMapBatches()` allocates/uploads the same UBO again and overwrites `m_named_colors_buffer_id`. This causes a redundant upload each frame and makes the earlier call pointless.
Consider making `updateNamedColorsUBO()` the single place that creates/updates `m_named_colors_buffer_id`, and have `renderMapBatches()` just use it. If this path must upload, guard it with a cheap check (e.g., only when colors change or the buffer ID is zero).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
AbstractShaderProgram::setUBOdoes aglGetUniformBlockIndexandglUniformBlockBindingon every call; consider caching the block index (and binding point) per program to avoid repeated GL queries in the hot render path.- The static
std::weak_ptr<VBO>usage in bothdrawRoomQuadandMapCanvas::updateNamedColorsUBOassumes the GL context remains valid for the lifetime of the shared VBO; consider tying these to the existing shader/VBO reset lifecycle (e.g.,ShaderPrograms::resetAllor a similar hook) to handle context loss or reinitialization more robustly. - Named-color UBO binding is now set up in multiple places (
LayerMeshes::renderandMapCanvas::getDefaultRenderState); it may be worth centralizing the construction of a "default render state" with the named-colors buffer to avoid divergence if the uniforms layout evolves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `AbstractShaderProgram::setUBO` does a `glGetUniformBlockIndex` and `glUniformBlockBinding` on every call; consider caching the block index (and binding point) per program to avoid repeated GL queries in the hot render path.
- The static `std::weak_ptr<VBO>` usage in both `drawRoomQuad` and `MapCanvas::updateNamedColorsUBO` assumes the GL context remains valid for the lifetime of the shared VBO; consider tying these to the existing shader/VBO reset lifecycle (e.g., `ShaderPrograms::resetAll` or a similar hook) to handle context loss or reinitialization more robustly.
- Named-color UBO binding is now set up in multiple places (`LayerMeshes::render` and `MapCanvas::getDefaultRenderState`); it may be worth centralizing the construction of a "default render state" with the named-colors buffer to avoid divergence if the uniforms layout evolves.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas_gl.cpp:725-727` </location>
<code_context>
DECL_TIMER(t3, "[async] actuallyPaintGL: compute highlights");
- ColoredTexVertVector highlights;
- auto drawQuad = [&highlights](const RawRoom &room, const Color color) {
+ DiffQuadVector highlights;
+ auto drawQuad = [&highlights](const RawRoom &room, const NamedColorEnum color) {
const auto &pos = room.getPosition().to_vec3();
- for (auto &corner : corners) {
- highlights.emplace_back(color, corner, pos + corner);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using to_vec3() when constructing RoomQuadTexVert is inconsistent with other call sites and may rely on an unintended implicit conversion.
Here `drawQuad` passes `room.getPosition().to_vec3()` while the other new call sites use `to_ivec3()`, and `RoomQuadTexVert` expects a `glm::ivec3`. This relies on an implicit float→int conversion (and may even fail to compile, depending on GLM). Please switch to `to_ivec3()` here for consistency and to avoid the implicit conversion.
</issue_to_address>
### Comment 2
<location> `src/opengl/legacy/AbstractShaderProgram.cpp:165-172` </location>
<code_context>
setUniform1iv(uFontTextureLoc, 1, &textureUnit);
}
+void AbstractShaderProgram::setUBO(const char *const block_name, const GLuint uboId)
+{
+ assert(uboId != 0);
+ auto functions = m_functions.lock();
+ const GLuint program = m_program.get();
+ auto block_index = functions->glGetUniformBlockIndex(program, block_name);
+ functions->glUniformBlockBinding(program, block_index, 0);
+ deref(functions).glBindBufferBase(GL_UNIFORM_BUFFER, 0, uboId);
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** setUBO does not handle the case where the uniform block is not found in the program.
If `glGetUniformBlockIndex` returns `GL_INVALID_INDEX` (e.g., from a typo in `block_name` or shader changes), calling `glUniformBlockBinding`/`glBindBufferBase` with that index is undefined. Please guard against `GL_INVALID_INDEX` and early-return (optionally asserting/logging in debug builds) to make such failures visible.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #446 +/- ##
==========================================
+ Coverage 66.48% 75.67% +9.19%
==========================================
Files 85 94 +9
Lines 4186 4354 +168
Branches 255 295 +40
==========================================
+ Hits 2783 3295 +512
+ Misses 1403 1059 -344 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nschimme
force-pushed
the
jahara-wip-2026-01-31d
branch
from
424e891 to
f4fe847
Compare
Contributor
Author
|
@sourcery-ai review |
nschimme
force-pushed
the
jahara-wip-2026-01-31d
branch
from
f4fe847 to
c218282
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The bit‑packing for
RoomQuadTexVert(shifts/masks in C++ and GLSL) is duplicated between CPU and GPU; consider centralizing the layout (e.g., helper pack/unpack functions or a shared header comment) so future changes to the packing scheme can’t drift out of sync. - The lazy mesh factories for tints and layer boosts (
createMeshFn/createMeshFns) are very similar; factoring out the common logic into a shared helper (e.g., a generic builder forstd::vector<RoomQuadTexVert>fromRoomTintVector-like containers) would reduce duplication and make future changes to the instanced quad representation safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The bit‑packing for `RoomQuadTexVert` (shifts/masks in C++ and GLSL) is duplicated between CPU and GPU; consider centralizing the layout (e.g., helper pack/unpack functions or a shared header comment) so future changes to the packing scheme can’t drift out of sync.
- The lazy mesh factories for tints and layer boosts (`createMeshFn`/`createMeshFns`) are very similar; factoring out the common logic into a shared helper (e.g., a generic builder for `std::vector<RoomQuadTexVert>` from `RoomTintVector`-like containers) would reduce duplication and make future changes to the instanced quad representation safer.
## Individual Comments
### Comment 1
<location> `src/resources/shaders/legacy/room/tex/acolor/vert.glsl:20-21` </location>
<code_context>
+ ivec3 ioffset = ioffsets_ccw[gl_VertexID];
+
+ int texZ = aVertTexCol.w & 0xFF;
+ int colorId = (aVertTexCol.w >> 8) % MAX_NAMED_COLORS;
+
+ vColor = uNamedColors[colorId];
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Modulo on `colorId` may hide out-of-range packing bugs and diverges from the C++-side validation.
On the CPU side, `RoomQuadTexVert` already asserts `color` is in `[0, NUM_NAMED_COLORS)`, and `MAX_NAMED_COLORS >= NUM_NAMED_COLORS` is enforced by `static_assert`. Doing `% MAX_NAMED_COLORS` in the shader instead silently wraps out-of-range values, which can hide packing bugs. Given the existing validation, consider removing the modulo or replacing it with a clamp (e.g. `min(colorId, MAX_NAMED_COLORS - 1)`) to make unexpected values more detectable during debugging.
```suggestion
int texZ = aVertTexCol.w & 0xFF;
int colorId = min((aVertTexCol.w >> 8), MAX_NAMED_COLORS - 1);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+20
to
+21
| int texZ = aVertTexCol.w & 0xFF; | ||
| int colorId = (aVertTexCol.w >> 8) % MAX_NAMED_COLORS; |
There was a problem hiding this comment.
suggestion (bug_risk): Modulo on colorId may hide out-of-range packing bugs and diverges from the C++-side validation.
On the CPU side, RoomQuadTexVert already asserts color is in [0, NUM_NAMED_COLORS), and MAX_NAMED_COLORS >= NUM_NAMED_COLORS is enforced by static_assert. Doing % MAX_NAMED_COLORS in the shader instead silently wraps out-of-range values, which can hide packing bugs. Given the existing validation, consider removing the modulo or replacing it with a clamp (e.g. min(colorId, MAX_NAMED_COLORS - 1)) to make unexpected values more detectable during debugging.
Suggested change
| int texZ = aVertTexCol.w & 0xFF; | |
| int colorId = (aVertTexCol.w >> 8) % MAX_NAMED_COLORS; | |
| int texZ = aVertTexCol.w & 0xFF; | |
| int colorId = min((aVertTexCol.w >> 8), MAX_NAMED_COLORS - 1); |
nschimme
force-pushed
the
jahara-wip-2026-01-31d
branch
from
c218282 to
38bccc5
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Packing the colorId and tex_z into RoomQuadTexVert::vertTexCol relies on tex_z < 256 and colorId < MAX_NAMED_COLORS; since the debug asserts are compiled out in release, consider adding inexpensive runtime parameter validation or static helpers to avoid silent wrapping if a caller passes larger values.
- NamedColors UBO initialization currently fills m_vec4s up to MAX_NAMED_COLORS but only updates indices < NUM_NAMED_COLORS; if additional entries are ever used on the GPU, it may be safer to explicitly zero/initialize the full range when colors change to avoid relying on default-constructed contents.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Packing the colorId and tex_z into RoomQuadTexVert::vertTexCol relies on tex_z < 256 and colorId < MAX_NAMED_COLORS; since the debug asserts are compiled out in release, consider adding inexpensive runtime parameter validation or static helpers to avoid silent wrapping if a caller passes larger values.
- NamedColors UBO initialization currently fills m_vec4s up to MAX_NAMED_COLORS but only updates indices < NUM_NAMED_COLORS; if additional entries are ever used on the GPU, it may be safer to explicitly zero/initialize the full range when colors change to avoid relying on default-constructed contents.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Quad meshes are now 75% (as GL_QUADS) to 83% (as GL_TRIANGLES) smaller. * read named colors from a uniform buffer objects (UBO) for named colors * consolidate plain and textured rendering into a single shader path * use lazy mesh generation for room tints and layer boosts
nschimme
force-pushed
the
jahara-wip-2026-01-31d
branch
from
38bccc5 to
fe886d5
Compare
Contributor
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Feb 3, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The named-colors UBO is populated only once in OpenGL::bindNamedColorsBuffer, so if named colors can change at runtime (e.g., theme or palette updates) the GPU buffer will go stale; consider either re-uploading into the existing UBO when colors change or explicitly documenting/centralizing a hook that calls resetNamedColorsBuffer + bindNamedColorsBuffer when the palette is updated.
- RoomQuadTexVert packs the color ID and texture layer into a single 32-bit int using 8 bits for each, but only guards against NUM_NAMED_COLORS > MAX_NAMED_COLORS; it may be worth adding a clear static_assert and comment tying MAX_NAMED_COLORS and the 8-bit packing together (and/or asserting MAX_NAMED_COLORS <= 256) so future changes to the number of named colors don’t silently break the packing scheme or shader.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The named-colors UBO is populated only once in OpenGL::bindNamedColorsBuffer, so if named colors can change at runtime (e.g., theme or palette updates) the GPU buffer will go stale; consider either re-uploading into the existing UBO when colors change or explicitly documenting/centralizing a hook that calls resetNamedColorsBuffer + bindNamedColorsBuffer when the palette is updated.
- RoomQuadTexVert packs the color ID and texture layer into a single 32-bit int using 8 bits for each, but only guards against NUM_NAMED_COLORS > MAX_NAMED_COLORS; it may be worth adding a clear static_assert and comment tying MAX_NAMED_COLORS and the 8-bit packing together (and/or asserting MAX_NAMED_COLORS <= 256) so future changes to the number of named colors don’t silently break the packing scheme or shader.Hi @nschimme! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Quad meshes are now 75% (as GL_QUADS) to 83% (as GL_TRIANGLES) smaller.
Summary by Sourcery
Introduce instanced room quad rendering with shared uniform-buffer-based named colors to reduce VRAM usage and modernize the OpenGL pipeline.
New Features:
Bug Fixes:
Enhancements:
Build:
Chores: