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 instanced textured quads driven by a new RoomQuadTex shader and a named-colors uniform buffer, consolidating color handling and lazily generating meshes for room tints and layer boosts to reduce VRAM usage. Sequence diagram for rendering a map layer with instanced room quadssequenceDiagram
participant MC as MapCanvas
participant GL as OpenGL
participant F as Legacy_Functions
participant SP as ShaderPrograms
participant RQTS as RoomQuadTexShader
participant GPU as GPU_Pipeline
MC->>MC: updateNamedColorsUBO()
MC->>GL: getSharedFunctions()
GL->>F: setUbo(ubo, vec4_colors)
F->>GPU: glBindBuffer(GL_UNIFORM_BUFFER)
F->>GPU: glBufferData(GL_UNIFORM_BUFFER, data)
F->>GPU: glBindBuffer(GL_UNIFORM_BUFFER, 0)
MC->>MC: m_named_colors_buffer_id = ubo
MC->>GL: renderMapBatches()
MC->>MC: getDefaultRenderState()
MC->>LM: render(thisLayer, focusedLayer, m_named_colors_buffer_id)
loop For each room batch
LM->>GL: createRoomQuadTexBatch(vector_RoomQuadTexVert_, texture)
GL->>F: createRoomQuadTexBatch(batch, texture)
F->>SP: getRoomQuadTexShader()
SP-->>F: shared_ptr_RoomQuadTexShader_
F->>GPU: glBindBuffer(GL_ARRAY_BUFFER, vbo)
F->>GPU: glBufferData(GL_ARRAY_BUFFER, batch)
F->>GPU: glVertexAttribIPointer(aVertTexCol)
F->>GPU: glVertexAttribDivisor(aVertTexCol, 1)
F->>GPU: glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, quad_ibo)
F->>GPU: glDrawElementsInstanced(TRIANGLE_FAN, 4, UNSIGNED_BYTE, numInstances)
end
note over GPU,RQTS: Vertex shader uses NamedColorsBlock UBO and aVertTexCol
LM-->>MC: finished rendering layer
Class diagram for instanced room quad rendering pipelineclassDiagram
direction LR
class OpenGL {
+UniqueMesh createColoredTexturedQuadBatch(vector_ColoredTexVert_ verts, MMTextureId texture)
+UniqueMesh createRoomQuadTexBatch(vector_RoomQuadTexVert_ verts, MMTextureId texture)
}
class Legacy_Functions {
+pair_DrawModeEnum_GLsizei_ setVbo(DrawModeEnum mode, GLuint vbo, vector_T_ batch, BufferUsageEnum usage)
+GLsizei setIbo(GLuint ibo, vector_T_ batch, BufferUsageEnum usage)
+GLsizei setUbo(GLuint ubo, vector_T_ batch, BufferUsageEnum usage)
+UniqueMesh createColoredTexturedBatch(DrawModeEnum mode, vector_ColoredTexVert_ batch, MMTextureId texture)
+UniqueMesh createRoomQuadTexBatch(vector_RoomQuadTexVert_ batch, MMTextureId texture)
}
class Legacy_AbstractShaderProgram {
-weak_ptr_Legacy_Functions_ m_functions
-GLuintWrapper m_program
+void setColor(char_name, Color color)
+void setMatrix(char_name, mat4 m)
+void setTexture(char_name, int textureUnit)
+void setUBO(char_block_name, GLuint uboId)
}
class Legacy_RoomQuadTexShader {
+~RoomQuadTexShader()
-void virt_setUniforms(mat4 mvp, GLRenderState_Uniforms uniforms)
}
class GLRenderState_Uniforms {
+Color color
+Textures textures
+GLuint namedColorBufferObject
+optional_float_ pointSize
}
class Legacy_SimpleMesh~VertexType_ProgramType_~ {
-optional_VBO_ m_vbo
-ProgramType_ m_program
-DrawModeEnum m_drawMode
-GLsizei m_numVerts
+void uploadVerts(DrawModeEnum mode, vector_VertexType__ verts, BufferUsageEnum usage)
+void render(GLRenderState rs)
#void virt_bind()
#void virt_unbind()
}
class Legacy_RoomQuadTexMesh~VertexType_~ {
-optional_Attribs_ m_boundAttribs
+Legacy_RoomQuadTexMesh(shared_ptr_Legacy_Functions_ f, DrawModeEnum mode, vector_VertexType__ verts, shared_ptr_RoomQuadTexShader_ prog)
#void virt_bind()
#void virt_unbind()
}
class RoomQuadTexVert {
+ivec4 vertTexCol
+RoomQuadTexVert(ivec3 vert, int tex_z)
+RoomQuadTexVert(ivec3 vert, int tex_z, NamedColorEnum color)
}
class Legacy_ShaderPrograms {
-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
+const shared_ptr_RoomQuadTexShader_ getRoomQuadTexShader()
+void early_init()
+void resetAll()
}
class GLRenderState {
+GLRenderState_Uniforms uniforms
+GLRenderState withDepthFunction(DepthFunctionEnum func)
+GLRenderState withBlend(BlendModeEnum mode)
}
class UniqueMesh {
-shared_ptr_IRenderable_ m_mesh
+void render(GLRenderState rs)
+operator bool()
}
%% Relationships
OpenGL --> Legacy_Functions : uses
Legacy_Functions --> Legacy_ShaderPrograms : getShaderPrograms()
Legacy_ShaderPrograms --> Legacy_RoomQuadTexShader : owns
Legacy_RoomQuadTexShader --|> Legacy_AbstractShaderProgram
Legacy_RoomQuadTexMesh --|> Legacy_SimpleMesh
Legacy_SimpleMesh --> Legacy_Functions : m_functions
UniqueMesh --> IRenderable : wraps
Legacy_Functions --> UniqueMesh : createRoomQuadTexBatch
Legacy_SimpleMesh --> RoomQuadTexVert : uploadVerts for instanced quads
Legacy_SimpleMesh --> GLRenderState : render
GLRenderState --> GLRenderState_Uniforms : has
GLRenderState_Uniforms --> GLuint : namedColorBufferObject
Legacy_AbstractShaderProgram --> GLRenderState_Uniforms : uses in virt_setUniforms
Legacy_AbstractShaderProgram --> GLuint : setUBO
Legacy_RoomQuadTexShader --> Legacy_AbstractShaderProgram : base
Legacy_RoomQuadTexShader --> GLRenderState_Uniforms : reads namedColorBufferObject
Class diagram for layer batching, lazy meshes, and diff highlightsclassDiagram
direction LR
class RoomTint {
+Coordinate coord
+NamedColorEnum color
+RoomTint(Coordinate input_coord, NamedColorEnum input_color)
}
class RoomTintVector {
}
class ColoredRoomTex {
+Coordinate coord
+MMTexArrayPosition pos
+NamedColorEnum colorId
+ColoredRoomTex(Coordinate input_coord, MMTexArrayPosition input_pos, NamedColorEnum input_color)
}
class RoomTex {
+Coordinate coord
+MMTexArrayPosition pos
+RoomTex(Coordinate input_coord, MMTexArrayPosition input_pos)
}
class LayerMeshesIntermediate {
+using_Fn_ = function_UniqueMesh_OpenGL_ref__
+using_FnVec_ = vector_Fn_
+FnVec terrain
+FnVec trails
+RoomTintArray_Fn_ tints
+FnVec overlays
+FnVec doors
+FnVec walls
+FnVec normalExits
+FnVec upDownExits
+FnVec streamIns
+FnVec streamOuts
+Fn layerBoost
+bool isValid
+LayerMeshes getLayerMeshes(OpenGL_gl) const
}
class LayerMeshes {
+UniqueMeshVector terrain
+UniqueMeshVector trails
+RoomTintArray_UniqueMesh_ tints
+UniqueMeshVector overlays
+UniqueMeshVector doors
+UniqueMeshVector walls
+UniqueMeshVector normalExits
+UniqueMeshVector upDownExits
+UniqueMeshVector streamIns
+UniqueMeshVector streamOuts
+UniqueMesh layerBoost
+bool isValid
+void render(int thisLayer, int focusedLayer, GLuint named_colors_buffer_id)
+operator bool()
}
class LayerBatchData {
+MMTexArrayPosition white_pixel
+RoomTexVector roomTerrains
+RoomTexVector roomTrails
+RoomTexVector roomOverlays
+ColoredRoomTexVector doors
+ColoredRoomTexVector solidWallLines
+ColoredRoomTexVector normalExits
+ColoredRoomTexVector roomUpDownExits
+RoomTexVector streamIns
+RoomTexVector streamOuts
+RoomTintArray_RoomTintVector_ roomTints
+RoomTintVector roomLayerBoostQuads
+LayerBatchData(MMTexArrayPosition white_pixel_)
+LayerMeshesIntermediate generateMeshes(OpenGL_gl)
}
class LayerBatchBuilder {
-LayerBatchData~ref~ m_data
-Textures m_textures
-RoomBounds m_bounds
+LayerBatchBuilder(LayerBatchData_ref_ data, Textures textures, RoomBounds bounds)
+void virt_visitRoom(RoomHandle room, MMTexArrayPosition terrain)
+void virt_visitTrailTexture(RoomHandle room, MMTexArrayPosition trail)
+void virt_visitNamedColorTint(RoomHandle room, RoomTintEnum tint)
+void virt_visitWall(RoomHandle room, WallOrientationEnum orientation, NamedColorEnum color)
+void virt_visitExit(RoomHandle room, ExitDirEnum dir, NamedColorEnum color, bool upDown)
}
class MapCanvas {
+GLuint m_named_colors_buffer_id
+MapCanvasTextures m_textures
+void renderMapBatches()
+void updateNamedColorsUBO()
+GLRenderState getDefaultRenderState()
}
class MapCanvas_Diff {
+MaybeDataOrMesh highlights
+future_HighlightDiff_ futureHighlight
+void maybeAsyncUpdate(Map saved, Map current)
}
class MapCanvas_Diff_MaybeDataOrMesh {
+using_DiffQuadVector_ = vector_RoomQuadTexVert_
+bool empty() const
+bool hasData() const
+bool hasMesh() const
+const DiffQuadVector_ getData() const
+const UniqueMesh getMesh() const
+void render(MapCanvas_mc, OpenGL_gl, MMTextureId texId)
}
class RoomQuadTexVert {
+ivec4 vertTexCol
+RoomQuadTexVert(ivec3 vert, int tex_z)
+RoomQuadTexVert(ivec3 vert, int tex_z, NamedColorEnum color)
}
class NamedColorEnum {
}
class RoomTintEnum {
}
class RoomTintArray_T_ {
}
%% Relationships
RoomTintVector --|> vector_RoomTint_ : inherits
RoomTintArray_RoomTintVector_ --|> RoomTintArray_T_ : specialization
RoomTintArray_Fn_ --|> RoomTintArray_T_ : specialization
RoomTintArray_UniqueMesh_ --|> RoomTintArray_T_ : specialization
ColoredRoomTex --> RoomTex : conceptually similar
RoomTint --> NamedColorEnum : color
LayerBatchData o--> RoomTexVector : aggregates
LayerBatchData o--> ColoredRoomTexVector : aggregates
LayerBatchData o--> RoomTintArray_RoomTintVector_ : aggregates
LayerBatchData o--> RoomTintVector : roomLayerBoostQuads
LayerBatchBuilder --> LayerBatchData : fills
LayerBatchBuilder ..> RoomTint : creates
LayerBatchData --> LayerMeshesIntermediate : generateMeshes
LayerMeshesIntermediate --> LayerMeshes : getLayerMeshes
LayerMeshes --> UniqueMeshVector : contains
LayerMeshes --> RoomTintArray_UniqueMesh_ : contains
MapCanvas --> MapCanvas_Diff : has
MapCanvas --> LayerMeshes : uses in renderMapBatches
MapCanvas --> GLuint : m_named_colors_buffer_id
MapCanvas_Diff_MaybeDataOrMesh --> UniqueMesh : variant
MapCanvas_Diff_MaybeDataOrMesh --> RoomQuadTexVert : DiffQuadVector_
MapCanvas_Diff ..> MapCanvas_Diff_MaybeDataOrMesh : uses
MapCanvas_Diff ..> MapCanvas : uses in render
RoomQuadTexVert --> NamedColorEnum : packs color
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.
Comment on lines
+1013
to
+1022
| void MapCanvas::updateNamedColorsUBO() | ||
| { | ||
| auto &gl = getOpenGL(); | ||
| static std::weak_ptr<Legacy::VBO> g_weak_vbo; | ||
| std::shared_ptr<Legacy::VBO> shared_vbo = g_weak_vbo.lock(); | ||
| if (shared_vbo == nullptr) { | ||
| auto &fns = gl.getSharedFunctions(Badge<MapCanvas>{}); | ||
|
|
||
| g_weak_vbo = shared_vbo = fns->getStaticVbos().alloc(); | ||
| Legacy::VBO &vbo = deref(shared_vbo); |
There was a problem hiding this comment.
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:
// 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.
Comment on lines
+1026
to
+1035
| 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, |
There was a problem hiding this comment.
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).
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
Use instanced room quad rendering with a new shader and UBO-backed named colors to reduce VRAM usage and unify room tint/highlight drawing.
New Features:
Enhancements: