introduce UboManager for lazy UBO updates and binding#472
Merged
nschimme merged 20 commits intoMUME:masterfrom Mar 17, 2026
Merged
introduce UboManager for lazy UBO updates and binding#472nschimme merged 20 commits intoMUME:masterfrom
nschimme merged 20 commits intoMUME:masterfrom
Conversation
Reviewer's GuideIntroduces a central UboManager for shared uniform buffer management, refactors legacy GL functions and OpenGL initialization to route UBO setup/binding through it, adds attribute-less fullscreen mesh utilities, and simplifies quad rendering and buffer upload helpers while wiring everything into the build and shader pipeline. Sequence diagram for NamedColorsBlock UBO rebuild and binding via UboManagersequenceDiagram
actor User
participant MapCanvas
participant OpenGL
participant UboManager as Legacy_UboManager
participant Funcs as Legacy_Functions
%% Registration during initialization
User->>MapCanvas: initializeGL()
MapCanvas->>OpenGL: initializeRenderer(dpr)
OpenGL->>Funcs: getFunctions()
OpenGL->>Funcs: glGetIntegerv(GL_MAX_UNIFORM_BUFFER_BINDINGS)
OpenGL-->>MapCanvas: renderer initialized
MapCanvas->>OpenGL: getUboManager()
MapCanvas->>UboManager: registerRebuildFunction(NamedColorsBlock, lambda)
UboManager->>UboManager: invalidateAll()
%% Later, during rendering
User->>MapCanvas: actuallyPaintGL()
MapCanvas->>OpenGL: getSharedFunctions(Badge)
MapCanvas->>OpenGL: getUboManager()
MapCanvas->>UboManager: bind(Funcs, NamedColorsBlock)
UboManager->>UboManager: updateIfInvalid(Funcs, NamedColorsBlock)
alt block is invalid
UboManager->>UboManager: lookup rebuild function
UboManager->>Funcs: call lambda(Funcs)
note right of Funcs: lambda body
Funcs->>UboManager: update(Funcs, NamedColorsBlock, XNamedColor::getAllColorsAsVec4())
UboManager->>Funcs: getOrCreateVbo(Funcs, NamedColorsBlock)
UboManager->>Funcs: setVbo(GL_UNIFORM_BUFFER, vbo, data)
UboManager->>Funcs: glBindBufferBase(GL_UNIFORM_BUFFER, getUboBindingIndex(block), vbo)
UboManager->>UboManager: record bound buffer in m_boundBuffers
else block already valid
UboManager->>UboManager: reuse cached buffer id
end
UboManager-->>MapCanvas: UBO bound for NamedColorsBlock
MapCanvas->>OpenGL: bindFbo(), clear(), draw...
Sequence diagram for rendering with FullScreenMesh using attribute-less VAOsequenceDiagram
participant Renderer as Caller
participant Mesh as BlitMesh~FullScreenMesh~
participant Funcs as Legacy_Functions
participant Program as BlitShader
participant VAOs as SharedVaos
Renderer->>Mesh: virt_render(renderState)
Mesh->>Program: bind()
Program-->>Mesh: program bound
Mesh->>Funcs: getProjectionMatrix()
alt renderState.mvp has value
Mesh->>Mesh: mvp = renderState.mvp
else no MVP provided
Mesh->>Mesh: mvp = projectionMatrix
end
Mesh->>Program: setUniforms(mvp, renderState.uniforms)
Mesh->>Funcs: getTexLookup()
Mesh->>Funcs: construct RenderStateBinder(Funcs, texLookup, renderState)
Mesh->>Funcs: getSharedVaos()
Funcs->>VAOs: get(EmptyVao)
VAOs-->>Mesh: SharedVao (possibly empty)
alt VAO not yet created
Mesh->>Mesh: create VAO with SharedFunctions
end
Mesh->>Funcs: glBindVertexArray(vao)
Mesh->>Funcs: glDrawArrays(m_mode, 0, m_numVerts)
Mesh->>Funcs: glBindVertexArray(0)
Mesh-->>Renderer: render complete
Class diagram for UboManager integration with OpenGL and Legacy::FunctionsclassDiagram
direction LR
class OpenGL {
- shared_ptr~Legacy::Functions~ m_opengl
- Legacy::UboManager m_uboManager
- bool m_rendererInitialized
+ OpenGL()
+ ~OpenGL()
+ void initializeRenderer(float devicePixelRatio)
+ GLRenderState getDefaultRenderState()
+ void resetNamedColorsBuffer()
+ void setTextureLookup(MMTextureId id, SharedMMTexture tex)
+ Legacy::UboManager &getUboManager()
}
class Legacy_Functions {
- unique_ptr~ShaderPrograms~ m_shaderPrograms
- unique_ptr~StaticVbos~ m_staticVbos
- unique_ptr~SharedVbos~ m_sharedVbos
- unique_ptr~SharedVaos~ m_sharedVaos
- unique_ptr~TexLookup~ m_texLookup
- unique_ptr~FBO~ m_fbo
- Legacy::UboManager &m_uboManager
+ Functions(Badge~Functions~, Legacy::UboManager &uboManager)
+ ~Functions()
+ static shared_ptr~Functions~ alloc(T, Legacy::UboManager &uboManager)
+ SharedVbos &getSharedVbos()
+ SharedVaos &getSharedVaos()
+ FBO &getFBO()
+ Legacy::UboManager &getUboManager()
+ void glBindBufferBase(GLenum target, SharedVboEnum block, GLuint buffer)
+ template~T,A~ GLsizei setVbo(GLenum target, GLuint buffer, vector~T,A~ &batch, BufferUsageEnum usage)
+ template~T~ void setVbo(GLenum target, GLuint buffer, T &data, BufferUsageEnum usage)
+ pair~DrawModeEnum,GLsizei~ setVbo(DrawModeEnum mode, GLuint vbo, vector~T,A~ &batch, BufferUsageEnum usage)
+ static const char *getUniformBlockName(SharedVboEnum block)
+ static void applyDefaultUniformBlockBindings(GLuint program)
}
class Legacy_FunctionsGL33 {
+ FunctionsGL33(Badge~Functions~, UboManager &uboManager)
+ ~FunctionsGL33()
}
class Legacy_FunctionsES30 {
+ FunctionsES30(Badge~Functions~, UboManager &uboManager)
+ ~FunctionsES30()
}
class Legacy_UboManager {
+ typedef RebuildFunction
- EnumIndexedArray~RebuildFunction,SharedVboEnum~ m_rebuildFunctions
- EnumIndexedArray~optional~GLuint~~ m_boundBuffers
+ UboManager()
+ void invalidate(SharedVboEnum block)
+ void invalidateAll()
+ void registerRebuildFunction(SharedVboEnum block, RebuildFunction func)
+ bool isInvalid(SharedVboEnum block) const
+ GLuint updateIfInvalid(Legacy::Functions &gl, SharedVboEnum block)
+ template~T,A~ GLuint update(Legacy::Functions &gl, SharedVboEnum block, vector~T,A~ &data)
+ template~T~ GLuint update(Legacy::Functions &gl, SharedVboEnum block, T &data)
+ void bind(Legacy::Functions &gl, SharedVboEnum block)
- VBO &getOrCreateVbo(Legacy::Functions &gl, SharedVboEnum block)
- GLuint bind_internal(Legacy::Functions &gl, SharedVboEnum block, GLuint buffer)
}
class Legacy_SharedVboEnum {
<<enumeration>>
NamedColorsBlock
NUM_BLOCKS
}
class GLRenderState {
+ Uniforms uniforms
+ optional~glm::mat4~ mvp
+ GLRenderState withBlend(BlendModeEnum new_blend) const
+ GLRenderState withAlpha(float alpha) const
+ GLRenderState withTexture(MMTextureId new_texture) const
+ GLRenderState withMvp(optional~glm::mat4~ new_mvp) const
}
class MapCanvas {
+ void initializeGL()
+ void actuallyPaintGL()
}
OpenGL --> Legacy_UboManager : owns
OpenGL --> Legacy_Functions : uses via m_opengl
Legacy_FunctionsGL33 --|> Legacy_Functions
Legacy_FunctionsES30 --|> Legacy_Functions
Legacy_Functions --> Legacy_UboManager : holds reference
Legacy_UboManager --> Legacy_Functions : uses in update/bind
Legacy_UboManager --> Legacy_SharedVboEnum : indexes by
Legacy_Functions --> Legacy_SharedVboEnum : glBindBufferBase, getUniformBlockName
MapCanvas --> OpenGL : uses
MapCanvas --> Legacy_UboManager : calls registerRebuildFunction, bind
GLRenderState ..> Legacy_Functions : used in rendering
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 1 issue, and left some high level feedback:
- In
UboManager::bind, if a block is invalid and no rebuild function is registered, a VBO is lazily created and bound without any data ever being uploaded; consider either asserting that a rebuild function exists for any block you try to bind or skipping the bind in that case to avoid using an uninitialized UBO. Functions::getUboManager()dereferences a raw pointer set viasetUboManager; it might be safer to assert or guard againstm_uboManager == nullptrto catch accidental use beforeOpenGL::initializeRendererwires it up.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UboManager::bind`, if a block is invalid and no rebuild function is registered, a VBO is lazily created and bound without any data ever being uploaded; consider either asserting that a rebuild function exists for any block you try to bind or skipping the bind in that case to avoid using an uninitialized UBO.
- `Functions::getUboManager()` dereferences a raw pointer set via `setUboManager`; it might be safer to assert or guard against `m_uboManager == nullptr` to catch accidental use before `OpenGL::initializeRenderer` wires it up.
## Individual Comments
### Comment 1
<location path="src/opengl/legacy/Legacy.h" line_range="137-140" />
<code_context>
std::unique_ptr<SharedVaos> m_sharedVaos;
std::unique_ptr<TexLookup> m_texLookup;
std::unique_ptr<FBO> m_fbo;
+ UboManager *m_uboManager = nullptr;
protected:
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against use of Functions::getUboManager() before the pointer is initialized.
`m_uboManager` is a raw pointer defaulted to `nullptr`, and `getUboManager()` always dereferences it. It’s only set via `setUboManager()` from `OpenGL::initializeRenderer()`, but nothing guarantees callers won’t use `Functions::getUboManager()` earlier. Consider either asserting `m_uboManager != nullptr` in `getUboManager()`, or requiring it via construction/factory wiring so the object can’t exist without a valid UBO manager. Otherwise, early access leads to UB instead of a clear failure.
</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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #472 +/- ##
===========================================
- Coverage 66.48% 25.52% -40.96%
===========================================
Files 85 506 +421
Lines 4186 41525 +37339
Branches 255 4529 +4274
===========================================
+ Hits 2783 10600 +7817
- Misses 1403 30925 +29522 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Update UboManager::bind to skip binding if invalid and no rebuild function exists. - Add assertions in UboManager::updateIfInvalid and Functions::getUboManager to catch uninitialized usage. - Change Functions::setUboManager to accept a reference instead of a raw pointer. - Update OpenGL::initializeRenderer to pass the UBO manager by reference.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
UboManager::bind, a missing rebuild function only triggers anassertin debug builds and otherwise results in a silent no-op (invalid block stays invalid and nothing is bound); consider either enforcing registration at compile-time/initialization, or logging/failing loudly in release to avoid hard-to-track rendering issues. - The
UboManager::updatetemplate currently special-cases onlystd::vectorviautils::is_vector_v; if you anticipate passing other contiguous containers (e.g.std::array,std::span), it may be cleaner to add overloads or a more general trait rather than relying on this single-type detection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UboManager::bind`, a missing rebuild function only triggers an `assert` in debug builds and otherwise results in a silent no-op (invalid block stays invalid and nothing is bound); consider either enforcing registration at compile-time/initialization, or logging/failing loudly in release to avoid hard-to-track rendering issues.
- The `UboManager::update` template currently special-cases only `std::vector` via `utils::is_vector_v`; if you anticipate passing other contiguous containers (e.g. `std::array`, `std::span`), it may be cleaner to add overloads or a more general trait rather than relying on this single-type detection.
## Individual Comments
### Comment 1
<location path="src/opengl/UboManager.h" line_range="104-66" />
<code_context>
+ * @brief Binds the UBO to its assigned point.
+ * If invalid and a rebuild function is registered, it will be updated first.
+ */
+ void bind(Legacy::Functions &gl, Legacy::SharedVboEnum block)
+ {
+ updateIfInvalid(gl, block);
+
+ if (isInvalid(block)) {
+ return;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** bind() silently becomes a no-op when a block is invalid and no rebuild function is registered, which can hide configuration mistakes.
Right now, if the block is still invalid after `updateIfInvalid()`, `bind()` just returns and nothing is bound, so missing configuration (e.g. not calling `registerRebuildFunction`) fails silently. Consider surfacing this more explicitly—e.g., assert in debug builds when `isInvalid(block)` remains true after `updateIfInvalid()`, or log an error—to make misconfigured UBOs easier to catch.
Suggested implementation:
```c
/**
* @brief Binds the UBO to its assigned point.
* If invalid and a rebuild function is registered, it will be updated first.
* In debug builds, an assertion will fire if the block remains invalid after the update.
*/
void bind(Legacy::Functions &gl, Legacy::SharedVboEnum block)
{
updateIfInvalid(gl, block);
if (isInvalid(block)) {
#ifndef NDEBUG
// If we get here, either the block is misconfigured (e.g. no rebuild function
// registered) or the rebuild function failed to produce a valid UBO.
assert(false && "UboManager::bind: attempted to bind invalid UBO block after updateIfInvalid (missing or failing rebuild function?)");
#endif
return;
}
bind_internal(gl, block);
}
```
1. Ensure `<cassert>` is included in `src/opengl/UboManager.h` (or a common header) so that `assert` is available. For example:
- Add `#include <cassert>` alongside the other standard-library includes at the top of this header.
2. If the actual `bind_internal` signature requires additional parameters (e.g., a VBO handle), adapt the `bind_internal(gl, block);` call to match the existing declaration/usage in the rest of the file.
3. If the project uses its own logging or assertion macros (e.g., `LOG_ERROR`, `DBG_ASSERT`), you may want to replace the raw `assert` with the project-standard mechanism to stay consistent with the rest of the codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- UboManager::bind: Log error and assert in debug if block remains invalid after update. - Generalized container support in UboManager::update using a new is_contiguous_container trait. - Added Functions::setUboGeneric for uploading data from any contiguous container. - Restored consistent (ubo, data) parameter order across all setUbo methods. - Added missing <cassert> include in Legacy.h. - Re-verified all changes with a successful build and test run.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
UboManager::update/Functions::setUboGeneric, consider adding astatic_assert(utils::is_contiguous_container_v<T>)(or similar constraint) on the generic path so misuse with non-contiguous containers is caught at compile time rather than relying solely on theif constexprinupdate. - The
FullScreenMesh::virt_renderunconditionally uses an identity MVP instead of therenderState's matrix; if some full-screen shaders expect the caller-provided MVP, it may be safer to either userenderState.mvpby default or provide a hook/override to customize the matrix without reimplementing the whole render path. - The new
utils::is_vectortrait is currently unused; if it’s not part of an imminent follow-up change, consider removing it to avoid dead code and keep the utility header focused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UboManager::update` / `Functions::setUboGeneric`, consider adding a `static_assert(utils::is_contiguous_container_v<T>)` (or similar constraint) on the generic path so misuse with non-contiguous containers is caught at compile time rather than relying solely on the `if constexpr` in `update`.
- The `FullScreenMesh::virt_render` unconditionally uses an identity MVP instead of the `renderState`'s matrix; if some full-screen shaders expect the caller-provided MVP, it may be safer to either use `renderState.mvp` by default or provide a hook/override to customize the matrix without reimplementing the whole render path.
- The new `utils::is_vector` trait is currently unused; if it’s not part of an imminent follow-up change, consider removing it to avoid dead code and keep the utility header focused.
## Individual Comments
### Comment 1
<location path="src/opengl/legacy/Legacy.h" line_range="414-423" />
<code_context>
template<typename... Args>
static inline constexpr bool are_distinct_v = are_distinct<Args...>::value;
+template<typename T>
+struct is_vector : std::false_type
+{};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Constrain `setUboGeneric` to contiguous containers to avoid unsafe usage.
`setUboGeneric` currently relies on `T` having `data()`/`size()` and being stored contiguously, which isn’t enforced. To catch misuse at compile time, add a `static_assert(utils::is_contiguous_container_v<T>, ...)` (and optionally `std::is_trivially_copyable_v<ValueType>`) in the template so non‑uploadable containers with `data`/`size` are rejected during compilation.
</issue_to_address>
### Comment 2
<location path="src/global/utils.h" line_range="467-476" />
<code_context>
template<typename... Args>
static inline constexpr bool are_distinct_v = are_distinct<Args...>::value;
+template<typename T>
+struct is_vector : std::false_type
+{};
+
+template<typename T, typename A>
+struct is_vector<std::vector<T, A>> : std::true_type
+{};
+
+template<typename T>
+static inline constexpr bool is_vector_v = is_vector<T>::value;
+
+template<typename T, typename = void>
</code_context>
<issue_to_address>
**nitpick:** Consider removing or using the new `is_vector` trait to avoid dead code.
This trait is defined but unused in these changes, unlike `is_contiguous_container`. If you don’t have an imminent use for it, consider removing it for now to keep the header minimal, and reintroduce it when you actually need it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- src/global/utils.h: Remove unused is_vector trait. - src/opengl/legacy/Legacy.h: Add static_assert constraints to setUboGeneric to ensure contiguous and trivially copyable data. - src/opengl/OpenGLTypes.h: Add mvp matrix to GLRenderState with withMvp helper. - src/opengl/legacy/AttributeLessMeshes.h: Use renderState.mvp in FullScreenMesh::virt_render. - src/opengl/legacy/SimpleMesh.h: Respect renderState.mvp in SimpleMesh::virt_render, falling back to global projection matrix if mvp is identity.
Contributor
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Mar 16, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Using
glm::mat4(1.f)as a sentinel inGLRenderState::mvp(and equality checks inSimpleMesh) is fragile; consider an explicit flag/optional to indicate whether an override MVP is set instead of relying on a specific matrix value. - The generic
utils::is_contiguous_containertrait will treat any type exposingdata()/size()as a contiguous container; if you only intend to support a narrow set (e.g.std::vector,std::array,std::span), consider constraining it or adding documentation to avoid accidental use with unexpected types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `glm::mat4(1.f)` as a sentinel in `GLRenderState::mvp` (and equality checks in `SimpleMesh`) is fragile; consider an explicit flag/optional to indicate whether an override MVP is set instead of relying on a specific matrix value.
- The generic `utils::is_contiguous_container` trait will treat any type exposing `data()`/`size()` as a contiguous container; if you only intend to support a narrow set (e.g. `std::vector`, `std::array`, `std::span`), consider constraining it or adding documentation to avoid accidental use with unexpected types.
## Individual Comments
### Comment 1
<location path="src/opengl/legacy/SimpleMesh.h" line_range="177-179" />
<code_context>
m_functions.checkError();
- const glm::mat4 mvp = m_functions.getProjectionMatrix();
+ const glm::mat4 mvp = renderState.mvp == glm::mat4(1.f)
+ ? m_functions.getProjectionMatrix()
+ : renderState.mvp;
auto programUnbinder = m_program.bind();
m_program.setUniforms(mvp, renderState.uniforms);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using an identity matrix as a sentinel for `renderState.mvp` can break explicit identity use-cases.
Because `glm::mat4(1.f)` is treated as "no MVP provided", callers cannot intentionally pass an identity MVP—`withMvp(glm::mat4(1.f))` will still use the projection matrix. This also couples default initialization to behavior. Prefer an explicit indicator (e.g., a flag or `std::optional<glm::mat4>` in `GLRenderState`) to distinguish "no custom MVP" from a legitimate identity matrix.
</issue_to_address>
### Comment 2
<location path="src/opengl/legacy/Legacy.h" line_range="414-423" />
<code_context>
+struct is_contiguous_container : std::false_type
+{};
+
+template<typename T>
+struct is_contiguous_container<
+ T,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `setUboSingle` should enforce that `T` is trivially copyable, mirroring `setUboGeneric`.
`setUboGeneric` uses a `static_assert` to ensure its element type is trivially copyable before calling `glBufferData`, but `setUboSingle` does not. Since `setUboSingle` also uploads from `&data`, it should add the same `static_assert(std::is_trivially_copyable_v<T>, ...)` to prevent undefined behavior with non-trivially-copyable types and keep both helpers’ guarantees consistent.
</issue_to_address>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.- OpenGLTypes.h: Change GLRenderState::mvp to std::optional<glm::mat4> to distinguish between no override and explicit identity matrix. - AttributeLessMeshes.h / SimpleMesh.h: Update virt_render to use value_or() for MVP selection. - Legacy.h: Add static_assert(std::is_trivially_copyable_v<T>) to setUboSingle for consistent type safety. - utils.h: Remove unused is_vector trait.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
Functions→UboManagercoupling currently relies on a raw pointer and an externalsetUboManagercall; consider passing a reference intoFunctionsat construction or wrapping the pointer in something that guarantees initialization sogetUboManager()can’t be called before it’s valid. FullScreenMesh::virt_renderdefaults the MVP to identity while other paths (e.g.SimpleMesh) default togetProjectionMatrix(), which may lead to subtle inconsistencies in how full-screen passes are transformed; it could be worth unifying the default behavior or documenting the intentional difference where these meshes are constructed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Functions` → `UboManager` coupling currently relies on a raw pointer and an external `setUboManager` call; consider passing a reference into `Functions` at construction or wrapping the pointer in something that guarantees initialization so `getUboManager()` can’t be called before it’s valid.
- `FullScreenMesh::virt_render` defaults the MVP to identity while other paths (e.g. `SimpleMesh`) default to `getProjectionMatrix()`, which may lead to subtle inconsistencies in how full-screen passes are transformed; it could be worth unifying the default behavior or documenting the intentional difference where these meshes are constructed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…avior - src/opengl/legacy/Legacy.h/.cpp: Refactor Functions to take UboManager & at construction. Remove setUboManager. - src/opengl/legacy/FunctionsGL33.h/.cpp, FunctionsES30.h/.cpp: Pass UboManager & to base class. - src/opengl/OpenGL.cpp: Pass m_uboManager during Functions allocation. - src/opengl/legacy/AttributeLessMeshes.h: Unified FullScreenMesh::virt_render to default to getProjectionMatrix() when no custom MVP is provided, matching SimpleMesh. - src/opengl/legacy/SimpleMesh.h: Replaced sentinel-based MVP selection with std::optional's value_or(). - src/opengl/OpenGLTypes.h: Replaced mvp sentinel with std::optional<glm::mat4>. - src/opengl/legacy/Legacy.h: Added static_assert for trivially copyable types in setUboSingle. - src/global/utils.h: Removed unused is_vector trait.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new utils::is_contiguous_container trait will treat types like std::string as valid contiguous containers; if that’s not intended for UBO uploads, consider tightening the constraint (e.g., excluding char-like element types or requiring a user-facing opt‑in) to avoid accidental misuse.
- In UboManager::update() and bind(), the logic to ensure the shared VBO exists and is emplaced is duplicated; factoring this into a small helper (e.g., getOrCreateSharedUboVbo(Functions&, SharedVboEnum)) would reduce repetition and keep future UBO-related changes localized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new utils::is_contiguous_container trait will treat types like std::string as valid contiguous containers; if that’s not intended for UBO uploads, consider tightening the constraint (e.g., excluding char-like element types or requiring a user-facing opt‑in) to avoid accidental misuse.
- In UboManager::update() and bind(), the logic to ensure the shared VBO exists and is emplaced is duplicated; factoring this into a small helper (e.g., getOrCreateSharedUboVbo(Functions&, SharedVboEnum)) would reduce repetition and keep future UBO-related changes localized.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ement - Refactor Legacy::Functions to take UboManager & at construction, eliminating raw pointer risk. - Unified FullScreenMesh and SimpleMesh MVP defaults to use the projection matrix. - Replaced fragile identity sentinel for MVP overrides with std::optional<glm::mat4>. - Hardened all UBO and IBO upload paths with trivially copyable static assertions. - Added getOrCreateVbo helper to UboManager to deduplicate logic. - Improved error visibility in UboManager::bind with logging and debug assertions. - Added is_vector and is_vector_v traits to utils.h.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
UboManager::update, thesetUboSinglecall uses the arguments in the wrong order (gl.setUboSingle(data, vbo.get(), ...)), which does not match thesetUboSingle(const GLuint ubo, const T &data, ...)signature and will either fail to compile or silently misbehave; swap the arguments so the buffer ID comes first. - The
UboManager::updatetemplate falls back tosetUboSinglefor any type that is not exactlystd::vector<T, A>; consider constraining this (e.g., viastatic_assertor additional traits) so that unsupported container types are rejected at compile time rather than being treated as a single POD block.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UboManager::update`, the `setUboSingle` call uses the arguments in the wrong order (`gl.setUboSingle(data, vbo.get(), ...)`), which does not match the `setUboSingle(const GLuint ubo, const T &data, ...)` signature and will either fail to compile or silently misbehave; swap the arguments so the buffer ID comes first.
- The `UboManager::update` template falls back to `setUboSingle` for any type that is not exactly `std::vector<T, A>`; consider constraining this (e.g., via `static_assert` or additional traits) so that unsupported container types are rejected at compile time rather than being treated as a single POD block.
## Individual Comments
### Comment 1
<location path="src/opengl/UboManager.h" line_range="87" />
<code_context>
+ {
+ Legacy::VBO &vbo = getOrCreateVbo(gl, block);
+
+ if constexpr (utils::is_vector_v<T>) {
+ static_cast<void>(gl.setUbo(vbo.get(), data, BufferUsageEnum::DYNAMIC_DRAW));
+ } else {
</code_context>
<issue_to_address>
**issue (bug_risk):** The `utils::is_vector_v` trait reference likely won’t compile due to namespace mismatch.
`is_vector`/`is_vector_v` is declared in the global namespace in `global/utils.h`, so `utils::is_vector_v<T>` will not resolve unless there is an actual `namespace utils` wrapper elsewhere. If that namespace doesn’t exist, use `::is_vector_v<T>` (or move the trait into a real `utils` namespace) so this compiles.
</issue_to_address>
### Comment 2
<location path="src/opengl/UboManager.h" line_range="89-90" />
<code_context>
+
+ if constexpr (utils::is_vector_v<T>) {
+ static_cast<void>(gl.setUbo(vbo.get(), data, BufferUsageEnum::DYNAMIC_DRAW));
+ } else {
+ gl.setUboSingle(data, vbo.get(), BufferUsageEnum::DYNAMIC_DRAW);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The `setUboSingle` call uses parameters in the opposite order of the declared function signature.
In `Legacy::Functions`, the template is `setUboSingle(const GLuint ubo, const T &data, ...)`, so this call will either not compile or bind the wrong buffer. Update it to `gl.setUboSingle(vbo.get(), data, BufferUsageEnum::DYNAMIC_DRAW);` to match the signature and the semantics of `setUbo`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Refactor Legacy::Functions to require UboManager& at construction. - Implement safety checks and lazy rebuild logic in UboManager. - Add type safety assertions for UBO/IBO uploads. - Unify MVP fallback behavior using std::optional in GLRenderState.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
UboManager::update, consider adding astatic_assertto enforce that non-std::vectordata types passed to the templatedupdateare trivially copyable (mirroringsetUbo/setUboSingle) to prevent accidental use with non-POD types. - When registering rebuild functions in
UboManager::registerRebuildFunction, you might want to assert or log if a rebuild function is already present for a givenSharedVboEnumto catch accidental double registration or overwriting of handlers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UboManager::update`, consider adding a `static_assert` to enforce that non-`std::vector` data types passed to the templated `update` are trivially copyable (mirroring `setUbo`/`setUboSingle`) to prevent accidental use with non-POD types.
- When registering rebuild functions in `UboManager::registerRebuildFunction`, you might want to assert or log if a rebuild function is already present for a given `SharedVboEnum` to catch accidental double registration or overwriting of handlers.
## Individual Comments
### Comment 1
<location path="src/opengl/UboManager.h" line_range="130-134" />
<code_context>
+ }
+
+private:
+ void bind_internal(Legacy::Functions &gl, Legacy::SharedVboEnum block, GLuint buffer)
+ {
+ auto &bound = m_boundBuffers[block];
+ if (!bound.has_value() || bound.value() != buffer) {
+ gl.glBindBufferBase(GL_UNIFORM_BUFFER, static_cast<GLuint>(block), buffer);
+ bound = buffer;
+ }
</code_context>
<issue_to_address>
**suggestion:** Relying on enum values as UBO binding indices could benefit from an explicit check or assertion.
`static_cast<GLuint>(block)` as the binding index assumes `SharedVboEnum` is 0-based, contiguous, and within `GL_MAX_UNIFORM_BUFFER_BINDINGS`. That can be broken by future enum changes. Please add a `static_assert` or runtime assert that ties `SharedVboEnum`/`NUM_SHARED_VBOS` to the available binding range (at least checking the casted value is `< NUM_SHARED_VBOS`).
Suggested implementation:
```c
void bind_internal(Legacy::Functions &gl, Legacy::SharedVboEnum block, GLuint buffer)
{
const auto bindingIndex = static_cast<std::size_t>(block);
assert(bindingIndex < m_boundBuffers.size());
auto &bound = m_boundBuffers[block];
if (!bound.has_value() || bound.value() != buffer) {
gl.glBindBufferBase(GL_UNIFORM_BUFFER, static_cast<GLuint>(bindingIndex), buffer);
bound = buffer;
}
```
- Ensure this header (or a common precompiled header it uses) includes `<cassert>` and `<cstddef>` if they are not already included.
- If `m_boundBuffers` is not sized to the number of valid `SharedVboEnum` values, adjust its construction so that `m_boundBuffers.size()` equals the number of valid shared VBO bindings, preserving the invariant enforced by the `assert`.
- If you want a stronger tie to the GL limits, in the constructor or initialization logic of `UboManager`, query `GL_MAX_UNIFORM_BUFFER_BINDINGS` via `glGetIntegerv` and add a runtime check that `m_boundBuffers.size() <= maxBindings`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add static_assert for POD types in UboManager::update. - Prevent duplicate rebuild function registration. - Validate UBO binding indices against OpenGL limits. - Refactor bind_internal for explicit index handling.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
GLRenderState, consider changingwithMvp(const std::optional<glm::mat4>&)to take the optional by value (withMvp(std::optional<glm::mat4> mvp)) to avoid surprising reference semantics and make passing temporaries more ergonomic. - In
UboManager::bind_internal, thestatic bool limitsCheckedpattern is not thread-safe; if there is any chance of UBO binding from multiple threads/contexts, consider guarding this withstd::once_flagor doing the limit check once at initialization. - The
UboManager::updatetemplate currently special-cases onlystd::vectorviautils::is_vector_v; if you expect to upload data from other contiguous containers (e.g.std::arrayor custom spans), consider adding overloads or a more general trait (like accepting any range of trivially copyable elements) to avoid forcing callers intostd::vector.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GLRenderState`, consider changing `withMvp(const std::optional<glm::mat4>&)` to take the optional by value (`withMvp(std::optional<glm::mat4> mvp)`) to avoid surprising reference semantics and make passing temporaries more ergonomic.
- In `UboManager::bind_internal`, the `static bool limitsChecked` pattern is not thread-safe; if there is any chance of UBO binding from multiple threads/contexts, consider guarding this with `std::once_flag` or doing the limit check once at initialization.
- The `UboManager::update` template currently special-cases only `std::vector` via `utils::is_vector_v`; if you expect to upload data from other contiguous containers (e.g. `std::array` or custom spans), consider adding overloads or a more general trait (like accepting any range of trivially copyable elements) to avoid forcing callers into `std::vector`.
## Individual Comments
### Comment 1
<location path="src/opengl/UboManager.h" line_range="89" />
<code_context>
+ {
+ Legacy::VBO &vbo = getOrCreateVbo(gl, block);
+
+ if constexpr (utils::is_vector_v<T>) {
+ static_cast<void>(gl.setUbo(vbo.get(), data, BufferUsageEnum::DYNAMIC_DRAW));
+ } else {
</code_context>
<issue_to_address>
**issue (bug_risk):** Namespace mismatch for is_vector_v causes a compile error
`is_vector` / `is_vector_v` are declared in the global namespace in `utils.h`, but this code uses `utils::is_vector_v<T>`, which will fail to compile. Please either move the trait into the `utils` namespace, or update this usage to `is_vector_v<T>` (possibly via a `using` alias).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Refactor Legacy::Functions to require UboManager& at construction. - Implement lazy rebuild and safety checks in UboManager. - Improve thread-safety of UboManager limits check using std::call_once. - enforce type safety (POD) for GPU buffer uploads. - Unify MVP transform logic in GLRenderState using std::optional.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In UboManager::bind_internal you implicitly rely on the underlying SharedVboEnum values as UBO binding indices; consider making that contract explicit (e.g., a comment and/or static_assert on the enum range/order) so future enum changes don’t silently break the binding layout.
- The utils::is_vector_v trait is only used to distinguish std::vector uploads in UboManager::update; you could simplify this by adding a dedicated overload of update for std::vector instead of a custom type trait, which would keep the generic path cleaner and easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In UboManager::bind_internal you implicitly rely on the underlying SharedVboEnum values as UBO binding indices; consider making that contract explicit (e.g., a comment and/or static_assert on the enum range/order) so future enum changes don’t silently break the binding layout.
- The utils::is_vector_v trait is only used to distinguish std::vector uploads in UboManager::update; you could simplify this by adding a dedicated overload of update for std::vector<T> instead of a custom type trait, which would keep the generic path cleaner and easier to reason about.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Refactor Legacy::Functions to require UboManager& at construction. - Implement lazy rebuild and safety checks in UboManager. - Improve thread-safety of UboManager limits check using std::call_once. - Enforce type safety (POD) for GPU buffer uploads. - Unify MVP transform logic in GLRenderState using std::optional. - Replace generic update trait with specialized overloads for bulk/single data.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The hardcoded
static_assert(static_cast<int>(Legacy::SharedVboEnum::NamedColorsBlock) == 0)and the assumption that enum values map directly to binding indices inUboManager::bind_internalis quite brittle; consider centralizing the binding index mapping (e.g., via a small helper or metadata table) so new enum entries or reordering cannot silently break UBO bindings. - In
MapCanvas::initializeGL, the rebuild lambda forNamedColorsBlockcapturesglby reference and is stored inUboManager; it may be safer to capture by value (or store only lightweight data needed to reacquireFunctions) to avoid subtle lifetime issues if theOpenGLinstance is ever replaced or outlives the canvas.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded `static_assert(static_cast<int>(Legacy::SharedVboEnum::NamedColorsBlock) == 0)` and the assumption that enum values map directly to binding indices in `UboManager::bind_internal` is quite brittle; consider centralizing the binding index mapping (e.g., via a small helper or metadata table) so new enum entries or reordering cannot silently break UBO bindings.
- In `MapCanvas::initializeGL`, the rebuild lambda for `NamedColorsBlock` captures `gl` by reference and is stored in `UboManager`; it may be safer to capture by value (or store only lightweight data needed to reacquire `Functions`) to avoid subtle lifetime issues if the `OpenGL` instance is ever replaced or outlives the canvas.
## Individual Comments
### Comment 1
<location path="src/opengl/legacy/Legacy.h" line_range="140" />
<code_context>
protected:
- explicit Functions(Badge<Functions>);
+ explicit Functions(Badge<Functions>, UboManager &uboManager);
public:
</code_context>
<issue_to_address>
**issue (bug_risk):** Functions now holds a UboManager reference; ensure lifetime guarantees to avoid dangling references.
Because `Functions` now stores `UboManager&`, `UboManager` must strictly outlive every `Functions` instance, including any `SharedFunctions` kept by meshes or other subsystems. Today `OpenGL` owns both, but callers can retain `std::shared_ptr<Functions>` after `OpenGL` is destroyed.
Either ensure `UboManager`’s ownership/lifetime is above all `Functions` (e.g., shared/central owner), or enforce that `Functions` instances cannot outlive `OpenGL` (e.g., by constraining where `SharedFunctions` can be stored). Otherwise, `getUboManager()` on a `Functions` that outlives `OpenGL` will be undefined behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| protected: | ||
| explicit Functions(Badge<Functions>); | ||
| explicit Functions(Badge<Functions>, UboManager &uboManager); |
There was a problem hiding this comment.
issue (bug_risk): Functions now holds a UboManager reference; ensure lifetime guarantees to avoid dangling references.
Because Functions now stores UboManager&, UboManager must strictly outlive every Functions instance, including any SharedFunctions kept by meshes or other subsystems. Today OpenGL owns both, but callers can retain std::shared_ptr<Functions> after OpenGL is destroyed.
Either ensure UboManager’s ownership/lifetime is above all Functions (e.g., shared/central owner), or enforce that Functions instances cannot outlive OpenGL (e.g., by constraining where SharedFunctions can be stored). Otherwise, getUboManager() on a Functions that outlives OpenGL will be undefined behavior.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo methods in Legacy::Functions. - Update UboManager to use the new setVbo methods. - Enforce type safety (POD) for all buffer uploads. - Remove redundant is_vector traits from utils.h. - Fix thread-safety and validation in UboManager::bind_internal. - Transition Legacy::Functions to use UboManager by reference.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo in Legacy::Functions. - Use dedicated overloads in UboManager::update instead of traits. - Harden bind_internal with explicit contract documentation and static_assert. - Improve thread-safety of limit checks using std::call_once. - Enforce POD requirement for all buffer uploads. - Require UboManager& in Functions constructor.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo in Legacy::Functions. - Refactor UboManager::update to use overloads instead of type traits. - Harden UboManager::bind_internal with explicit contract documentation and static_assert. - Implement thread-safe limit validation in UboManager using std::call_once. - Transition Legacy::Functions to require UboManager& at construction. - Enforce POD type safety for all GPU buffer uploads. - Unify rendering state transformations using std::optional in GLRenderState.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- UboManager::bind_internal hard-codes the assumption that Legacy::SharedVboEnum is 0-based and contiguous (with a static_assert on NamedColorsBlock == 0); consider centralizing the enum→binding-index mapping or enforcing this invariant at the enum definition to avoid subtle breakage when adding/reordering SharedVboEnum values.
- There is some duplication around trivially-copyable checks for buffer uploads (Functions::setVbo and UboManager::update(T)), which could be consolidated so that the invariant is enforced in a single place and error messages remain consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- UboManager::bind_internal hard-codes the assumption that Legacy::SharedVboEnum is 0-based and contiguous (with a static_assert on NamedColorsBlock == 0); consider centralizing the enum→binding-index mapping or enforcing this invariant at the enum definition to avoid subtle breakage when adding/reordering SharedVboEnum values.
- There is some duplication around trivially-copyable checks for buffer uploads (Functions::setVbo and UboManager::update(T)), which could be consolidated so that the invariant is enforced in a single place and error messages remain consistent.
## Individual Comments
### Comment 1
<location path="src/opengl/legacy/AttributeLessMeshes.h" line_range="59" />
<code_context>
+
+ RenderStateBinder rsBinder(m_functions, m_functions.getTexLookup(), renderState);
+
+ SharedVao shared = m_functions.getSharedVaos().get(SharedVaoEnum::EmptyVao);
+ VAO &vao = deref(shared);
+ if (!vao) {
</code_context>
<issue_to_address>
**issue (bug_risk):** SharedVaoEnum::EmptyVao is used but not defined by the current XFOREACH_SHARED_VAO macro.
With `XFOREACH_SHARED_VAO` currently empty, `SharedVaoEnum` has no enumerators, so `SharedVaoEnum::EmptyVao` cannot compile unless the macro is updated elsewhere to define it. If `EmptyVao` is meant to be a shared VAO slot, add it through `XFOREACH_SHARED_VAO` (and include it in `NUM_SHARED_VAOS`) to keep the enum and shared VAO storage in sync.
</issue_to_address>
### Comment 2
<location path="src/display/mapcanvas_gl.cpp" line_range="240-245" />
<code_context>
gl.initializeRenderer(static_cast<float>(QPaintDevice::devicePixelRatioF()));
+
+ gl.getUboManager().registerRebuildFunction(
+ Legacy::SharedVboEnum::NamedColorsBlock, [&gl](Legacy::Functions &funcs) {
+ gl.getUboManager().update(funcs,
+ Legacy::SharedVboEnum::NamedColorsBlock,
+ XNamedColor::getAllColorsAsVec4());
+ });
+
</code_context>
<issue_to_address>
**suggestion:** The UBO rebuild lambda ignores its Functions parameter and instead captures OpenGL by reference.
The lambda takes `Legacy::Functions &funcs` but ignores it and instead captures `gl` to call `gl.getUboManager()`. This adds unnecessary coupling to a specific `OpenGL` instance. Prefer using the `funcs` parameter directly, e.g. `[](Legacy::Functions &funcs) { auto &manager = funcs.getUboManager(); manager.update(funcs, Legacy::SharedVboEnum::NamedColorsBlock, XNamedColor::getAllColorsAsVec4()); }`, so the callback remains self-contained and its dependencies are explicit.
```suggestion
gl.getUboManager().registerRebuildFunction(
Legacy::SharedVboEnum::NamedColorsBlock, [](Legacy::Functions &funcs) {
auto &uboManager = funcs.getUboManager();
uboManager.update(funcs,
Legacy::SharedVboEnum::NamedColorsBlock,
XNamedColor::getAllColorsAsVec4());
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo in Legacy::Functions. - Refactor UboManager::update to use overloads instead of type traits. - Harden UboManager::bind_internal with explicit contract documentation and static_assert. - Implement thread-safe limit validation in UboManager using std::call_once. - Transition Legacy::Functions to require UboManager& at construction. - Enforce POD type safety for all GPU buffer uploads. - Unify rendering state transformations using std::optional in GLRenderState. - Fix EmptyVao definition and lambda capturing in MapCanvas.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The tight coupling between Legacy::SharedVboEnum and UBO binding indices is enforced in multiple places (static_asserts in Legacy::Legacy.h and UboManager::bind_internal); consider centralizing this constraint (e.g., a single helper or trait) so that future enum changes only need to be validated in one place.
- UboManager::registerRebuildFunction currently hard-asserts if called twice for the same block; if there are scenarios where the renderer may need to reconfigure or reinitialize UBO sources (e.g., after device/context loss), consider either allowing re-registration or providing an explicit unregister/reset API instead of crashing in debug builds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tight coupling between Legacy::SharedVboEnum and UBO binding indices is enforced in multiple places (static_asserts in Legacy::Legacy.h and UboManager::bind_internal); consider centralizing this constraint (e.g., a single helper or trait) so that future enum changes only need to be validated in one place.
- UboManager::registerRebuildFunction currently hard-asserts if called twice for the same block; if there are scenarios where the renderer may need to reconfigure or reinitialize UBO sources (e.g., after device/context loss), consider either allowing re-registration or providing an explicit unregister/reset API instead of crashing in debug builds.
## Individual Comments
### Comment 1
<location path="src/opengl/legacy/AttributeLessMeshes.h" line_range="45-47" />
<code_context>
+protected:
+ void virt_clear() final {}
+ void virt_reset() final {}
+ NODISCARD bool virt_isEmpty() const final { return false; }
+
+ void virt_render(const GLRenderState &renderState) override
</code_context>
<issue_to_address>
**suggestion (bug_risk):** virt_isEmpty() always returning false might hide configuration errors where m_numVerts is zero.
Since rendering uses m_numVerts as the draw count, a FullScreenMesh constructed with numVerts == 0 would still go through the render path but only issue a no-op draw, masking the misconfiguration. Consider either making virt_isEmpty() return m_numVerts == 0 or adding a constructor assert that m_numVerts > 0 to enforce the invariant explicitly.
```suggestion
void virt_clear() final {}
void virt_reset() final {}
NODISCARD bool virt_isEmpty() const final { return m_numVerts == 0; }
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo in Legacy::Functions. - Centralize UBO binding index mapping and SharedVboEnum contract in Legacy.h. - Allow UboManager rebuild function re-registration with a warning. - Implement thread-safe limit validation in UboManager using std::call_once. - Transition Legacy::Functions to require UboManager& at construction. - Enforce POD type safety for all GPU buffer uploads via a unified private helper. - Unify rendering state transformations using std::optional in GLRenderState. - Fix EmptyVao definition and lambda capturing in MapCanvas. - Improve FullScreenMesh::virt_isEmpty() logic.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
UboManager::bind_internalyou compute abindingIndexviagetUboBindingIndexbut then callgl.glBindBufferBase(GL_UNIFORM_BUFFER, block, buffer)rather than usingbindingIndex; consider passing the computed index directly for consistency with the earlier assertion and to avoid relying on theSharedVboEnumoverload at this low level. UboManager.hincludes<mutex>but only usesstd::once_flag/std::call_oncein a#ifndef NDEBUGblock; if you don’t intend to make the manager generally thread-safe, you can either remove the unused synchronization member/commented intent or document that only the debug binding-limit check is guarded and the rest of the class is not thread-safe.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UboManager::bind_internal` you compute a `bindingIndex` via `getUboBindingIndex` but then call `gl.glBindBufferBase(GL_UNIFORM_BUFFER, block, buffer)` rather than using `bindingIndex`; consider passing the computed index directly for consistency with the earlier assertion and to avoid relying on the `SharedVboEnum` overload at this low level.
- `UboManager.h` includes `<mutex>` but only uses `std::once_flag`/`std::call_once` in a `#ifndef NDEBUG` block; if you don’t intend to make the manager generally thread-safe, you can either remove the unused synchronization member/commented intent or document that only the debug binding-limit check is guarded and the rest of the class is not thread-safe.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo in Legacy::Functions. - Centralize UBO binding index mapping and SharedVboEnum contract in Legacy.h. - Improve thread-safety of limit checks using std::call_once. - Enforce POD type safety for GPU buffer uploads via a unified private helper. - Unify rendering state transformations using std::optional in GLRenderState. - Improve FullScreenMesh safety and virt_isEmpty implementation. - Clean up utils.h includes and improve UboManager documentation.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
UboManager::bind_internalandLegacy::getUboBindingIndex, the assumption thatSharedVboEnumis 0-based and contiguous is duplicated; consider centralizing this invariant and performing theGL_MAX_UNIFORM_BUFFER_BINDINGSsanity check once during GL initialization rather than lazily in a debug call to make the constraints clearer and avoid scattered assumptions. - The
UboManager::bindpath always callsgetOrCreateVboafterupdateIfInvalid, even thoughupdate()already does this and binds; you could avoid the redundant lookup by havingupdateIfInvalidreturn the buffer (or cache it) sobind()can skip re-fetching and re-binding when nothing changed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UboManager::bind_internal` and `Legacy::getUboBindingIndex`, the assumption that `SharedVboEnum` is 0-based and contiguous is duplicated; consider centralizing this invariant and performing the `GL_MAX_UNIFORM_BUFFER_BINDINGS` sanity check once during GL initialization rather than lazily in a debug call to make the constraints clearer and avoid scattered assumptions.
- The `UboManager::bind` path always calls `getOrCreateVbo` after `updateIfInvalid`, even though `update()` already does this and binds; you could avoid the redundant lookup by having `updateIfInvalid` return the buffer (or cache it) so `bind()` can skip re-fetching and re-binding when nothing changed.
## Individual Comments
### Comment 1
<location path="src/opengl/UboManager.h" line_range="76-53" />
<code_context>
+ /**
+ * @brief Rebuilds the UBO if it's invalid using the registered rebuild function.
+ */
+ void updateIfInvalid(Legacy::Functions &gl, Legacy::SharedVboEnum block)
+ {
+ if (isInvalid(block)) {
+ const auto &func = m_rebuildFunctions[block];
+ assert(func && "UBO block is invalid and no rebuild function is registered");
+ if (func) {
+ func(gl);
+ // The rebuild function is expected to call update() which marks it valid.
+ assert(!isInvalid(block));
+ }
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** updateIfInvalid relies on the rebuild function calling update(), which is easy to forget and only enforced by an assert.
Because `updateIfInvalid` assumes the rebuild callback will call an `update()` overload, a missed call leaves the block invalid: `bind()` will then only log and assert (which is compiled out in release builds), resulting in a silently unbound UBO.
To make this safer, you could either:
- Have the rebuild callback return the data/descriptor and let `UboManager` call `update()` itself, or
- Track whether `update()` was invoked during `updateIfInvalid` (e.g., guard/flag) and emit a clear error if not.
This would reduce the chance that new rebuild functions accidentally skip `update()` and cause subtle rendering issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo in Legacy::Functions. - Centralize UBO binding index mapping and SharedVboEnum contract in Legacy.h. - Move UBO limit validation to OpenGL::initializeRenderer. - Optimize UboManager by returning buffer IDs from update/updateIfInvalid. - Implement safety checks to ensure rebuild functions call update(). - Enforce POD type safety for GPU buffer uploads via a unified private helper. - Unify rendering state transformations using std::optional in GLRenderState. - Improve FullScreenMesh safety and virt_isEmpty implementation. - Clean up utils.h includes and document UboManager thread-safety.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since SharedVboEnum values are used as UBO binding indices, consider adding a stronger compile‑time check (e.g., asserting that the last enum value + 1 == NUM_SHARED_VBOS) instead of only asserting that NamedColorsBlock == 0, so future additions can’t accidentally break the 0‑based contiguous invariant.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since SharedVboEnum values are used as UBO binding indices, consider adding a stronger compile‑time check (e.g., asserting that the last enum value + 1 == NUM_SHARED_VBOS) instead of only asserting that NamedColorsBlock == 0, so future additions can’t accidentally break the 0‑based contiguous invariant.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Consolidate setUbo, setIbo, and setUboSingle into overloaded setVbo in Legacy::Functions. - Centralize UBO binding index mapping and SharedVboEnum/SharedVaoEnum contract in Legacy.h. - Move UBO limit validation to OpenGL::initializeRenderer. - Optimize UboManager by returning buffer IDs from update/updateIfInvalid. - Enforce POD type safety for GPU buffer uploads via a unified private helper. - Unify rendering state transformations using std::optional in GLRenderState. - Improve FullScreenMesh safety and virt_isEmpty implementation. - Clean up utils.h includes and document UboManager thread-safety.
Contributor
Author
|
@SourceryAI review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/opengl/UboManager.h" line_range="131-140" />
<code_context>
+ * @brief Binds the UBO to its assigned point.
+ * If invalid and a rebuild function is registered, it will be updated first.
+ */
+ void bind(Legacy::Functions &gl, Legacy::SharedVboEnum block)
+ {
+ const GLuint buffer = updateIfInvalid(gl, block);
+
+ if (buffer == 0) {
+ MMLOG_ERROR() << "UboManager::bind: attempted to bind invalid UBO block "
+ << static_cast<int>(block);
+ assert(false
+ && "UboManager::bind: attempted to bind invalid UBO block after "
+ "updateIfInvalid (missing or failing rebuild function?)");
+ return;
+ }
+
+ // updateIfInvalid already ensured it is bound.
+ assert(m_boundBuffers[block] == buffer);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `bind` assumes no one else mutates UBO bindings, which can desynchronize internal tracking from actual GL state.
This relies on `m_boundBuffers` as the sole source of truth and assumes no external `glBindBufferBase` calls for these binding points. If legacy code bypasses `UboManager`, the cached state can become stale and `bind` will early-return without actually re-binding. If this invariant is intentional, consider enforcing it in debug builds (e.g., compare `glGetIntegeri_v(GL_UNIFORM_BUFFER_BINDING, bindingIndex)` with `m_boundBuffers[block]`) or documenting that all UBO binds for these blocks must go through `UboManager`.
Suggested implementation:
```c
/**
* @brief Binds the UBO to its assigned point.
*
* If invalid and a rebuild function is registered, it will be updated first.
*
* All UBO binds for managed blocks are expected to go through UboManager.
* In debug builds, this is enforced by validating the actual GL binding
* against UboManager's cached state and asserting on desynchronization.
*/
void bind(Legacy::Functions &gl, Legacy::SharedVboEnum block)
{
const GLuint buffer = updateIfInvalid(gl, block);
if (buffer == 0) {
MMLOG_ERROR() << "UboManager::bind: attempted to bind invalid UBO block "
<< static_cast<int>(block);
assert(false
&& "UboManager::bind: attempted to bind invalid UBO block after "
"updateIfInvalid (missing or failing rebuild function?)");
return;
}
#if !defined(NDEBUG)
// Verify that external code did not mutate the GL UBO binding state
// behind UboManager's back for this block.
const GLuint bindingIndex = m_bindingPoints[block];
GLint currentlyBound = 0;
gl.getIntegeri_v(GL_UNIFORM_BUFFER_BINDING, bindingIndex, ¤tlyBound);
if (static_cast<GLuint>(currentlyBound) != buffer) {
MMLOG_ERROR() << "UboManager::bind: GL state desynchronized for block "
<< static_cast<int>(block)
<< ", expected buffer " << buffer
<< " but GL has " << currentlyBound
<< ". All UBO binds for this block must go through UboManager.";
assert(false && "UboManager::bind: GL UBO binding state desynchronized from UboManager cache");
}
#endif
// updateIfInvalid already ensured it is (and remains) bound.
assert(m_boundBuffers[block] == buffer);
}
#include "../global/EnumIndexedArray.h"
```
These edits assume:
1. `m_bindingPoints` is an indexable container mapping `Legacy::SharedVboEnum` to the corresponding UBO binding index.
2. `Legacy::Functions` exposes `getIntegeri_v(GLenum, GLuint, GLint*)` (or a compatible signature). If the actual name/signature differs, adapt the `gl.getIntegeri_v(...)` call accordingly.
3. `GL_UNIFORM_BUFFER_BINDING` is available in this translation unit (it likely already is, since UBOs are used elsewhere).
If `m_bindingPoints` or the `getIntegeri_v` wrapper do not exist or are named differently, you will need to:
- Replace `m_bindingPoints[block]` with the correct way to look up the binding index for `block`.
- Replace `gl.getIntegeri_v(...)` with the correct Legacy wrapper for `glGetIntegeri_v`.
</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
+131
to
+140
| void bind(Legacy::Functions &gl, Legacy::SharedVboEnum block) | ||
| { | ||
| const GLuint buffer = updateIfInvalid(gl, block); | ||
|
|
||
| if (buffer == 0) { | ||
| MMLOG_ERROR() << "UboManager::bind: attempted to bind invalid UBO block " | ||
| << static_cast<int>(block); | ||
| assert(false | ||
| && "UboManager::bind: attempted to bind invalid UBO block after " | ||
| "updateIfInvalid (missing or failing rebuild function?)"); |
There was a problem hiding this comment.
suggestion (bug_risk): bind assumes no one else mutates UBO bindings, which can desynchronize internal tracking from actual GL state.
This relies on m_boundBuffers as the sole source of truth and assumes no external glBindBufferBase calls for these binding points. If legacy code bypasses UboManager, the cached state can become stale and bind will early-return without actually re-binding. If this invariant is intentional, consider enforcing it in debug builds (e.g., compare glGetIntegeri_v(GL_UNIFORM_BUFFER_BINDING, bindingIndex) with m_boundBuffers[block]) or documenting that all UBO binds for these blocks must go through UboManager.
Suggested implementation:
/**
* @brief Binds the UBO to its assigned point.
*
* If invalid and a rebuild function is registered, it will be updated first.
*
* All UBO binds for managed blocks are expected to go through UboManager.
* In debug builds, this is enforced by validating the actual GL binding
* against UboManager's cached state and asserting on desynchronization.
*/
void bind(Legacy::Functions &gl, Legacy::SharedVboEnum block)
{
const GLuint buffer = updateIfInvalid(gl, block);
if (buffer == 0) {
MMLOG_ERROR() << "UboManager::bind: attempted to bind invalid UBO block "
<< static_cast<int>(block);
assert(false
&& "UboManager::bind: attempted to bind invalid UBO block after "
"updateIfInvalid (missing or failing rebuild function?)");
return;
}
#if !defined(NDEBUG)
// Verify that external code did not mutate the GL UBO binding state
// behind UboManager's back for this block.
const GLuint bindingIndex = m_bindingPoints[block];
GLint currentlyBound = 0;
gl.getIntegeri_v(GL_UNIFORM_BUFFER_BINDING, bindingIndex, ¤tlyBound);
if (static_cast<GLuint>(currentlyBound) != buffer) {
MMLOG_ERROR() << "UboManager::bind: GL state desynchronized for block "
<< static_cast<int>(block)
<< ", expected buffer " << buffer
<< " but GL has " << currentlyBound
<< ". All UBO binds for this block must go through UboManager.";
assert(false && "UboManager::bind: GL UBO binding state desynchronized from UboManager cache");
}
#endif
// updateIfInvalid already ensured it is (and remains) bound.
assert(m_boundBuffers[block] == buffer);
}
#include "../global/EnumIndexedArray.h"These edits assume:
m_bindingPointsis an indexable container mappingLegacy::SharedVboEnumto the corresponding UBO binding index.Legacy::FunctionsexposesgetIntegeri_v(GLenum, GLuint, GLint*)(or a compatible signature). If the actual name/signature differs, adapt thegl.getIntegeri_v(...)call accordingly.GL_UNIFORM_BUFFER_BINDINGis available in this translation unit (it likely already is, since UBOs are used elsewhere).
If m_bindingPoints or the getIntegeri_v wrapper do not exist or are named differently, you will need to:
- Replace
m_bindingPoints[block]with the correct way to look up the binding index forblock. - Replace
gl.getIntegeri_v(...)with the correct Legacy wrapper forglGetIntegeri_v.
nschimme
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a centralized UBO management system and attribute-less mesh utilities, refactoring OpenGL legacy code to use them for shared uniform buffers and full-screen rendering.
New Features:
Enhancements:
Build: