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
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
Reviewer's GuideRefactors room rendering to use an instanced quad shader backed by a named-colors UBO and a 1x1 white texture, consolidating tint/highlight/layer-boost paths while extending the GL helpers for UBOs, instancing, and named-color upload. Sequence diagram for instanced room quad rendering with named-colors UBOsequenceDiagram
actor User
participant MC as MapCanvas
participant GL as OpenGL
participant LF as Legacy_Functions
participant SP as ShaderPrograms
participant LM as LayerMeshes
participant GPU as GPU/Driver
User->>MC: paintGL / renderMapBatches
MC->>MC: updateNamedColorsUBO()
MC->>GL: getSharedFunctions()
GL-->>MC: SharedFunctions
MC->>LF: setUbo(ubo, XNamedColor::getAllColorsAsVec4)
LF->>GPU: glBindBuffer(GL_UNIFORM_BUFFER)
LF->>GPU: glBufferData(GL_UNIFORM_BUFFER, vec4_colors)
LF->>GPU: glBindBufferBase(binding=0, ubo)
MC->>LM: render(thisLayer, focusedLayer, named_colors_buffer_id)
LM->>LM: resolve(Fn/FnVec) -> UniqueMesh
LM->>MC: getDefaultRenderState()
MC-->>LM: GLRenderState(namedColorBufferObject=ubo)
loop For each room batch
LM->>GL: UniqueMesh::render(GLRenderState)
GL->>LF: createRoomQuadTexBatch(verts, texture)
LF->>SP: getRoomQuadTexShader()
SP-->>LF: RoomQuadTexShader
LF->>GPU: glUseProgram(RoomQuadTexShader)
LF->>GPU: glBindTexture(texture_array)
LF->>GPU: glBindBuffer(GL_ARRAY_BUFFER, instance_VBO)
LF->>GPU: glVertexAttribIPointer(aVertTexCol)
LF->>GPU: glVertexAttribDivisor(aVertTexCol, 1)
LF->>GPU: glDrawElementsInstanced(TRIANGLE_FAN, indexCount=4, instanceCount=numRooms)
end
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.
Comment on lines
+725
to
727
| DiffQuadVector highlights; | ||
| auto drawQuad = [&highlights](const RawRoom &room, const NamedColorEnum color) { | ||
| const auto &pos = room.getPosition().to_vec3(); |
There was a problem hiding this comment.
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.
Comment on lines
+165
to
+172
| 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); |
There was a problem hiding this comment.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #446 +/- ##
==========================================
+ Coverage 66.48% 75.72% +9.24%
==========================================
Files 85 94 +9
Lines 4186 4355 +169
Branches 255 295 +40
==========================================
+ Hits 2783 3298 +515
+ Misses 1403 1057 -346 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Quad meshes are now 75% (as GL_QUADS) to 83% (as GL_TRIANGLES) smaller.
Summary by Sourcery
Switch room rendering to an instanced quad path with a new shader and UBO-driven named colors to reduce VRAM usage and simplify draw calls.
New Features:
Enhancements: