refactor(rhi): sweep raw GL call sites out of OloEngine/src — Phase 2 step 2 (#691) - #700
Merged
drsnuggles8 merged 3 commits intoJul 31, 2026
Merged
Conversation
…se 2 step 2) Drives both Phase 2 ratchet counters to zero: sweep_gl_calls 313 -> 0 sweep_glad_includes 39 -> 0 No translation unit in the sweep bucket can name a GL symbol any more, which is a compiler-enforced property rather than a grep. tools_gl_calls stays at 236 (Renderer/Debug/, Phase 8 relocation) and debug_escape_hatch stays at 0. The headline finding is that the facade was not merely GL-typed, it was INCOMPLETE. 84 distinct GL entry points appear across the 313 call sites and roughly 60% of them had no RendererAPI equivalent at all, so closing the sweep needed ~60 new virtuals against an existing 74. Whole categories had never been abstracted and every pass reached past the facade to perform them: buffer binding points (glBindBufferBase, 26 sites), raw buffer lifecycle, named- framebuffer draw/read attachment selection, blits, clears, completeness, occlusion + timer queries, fences, VAO lifecycle, texture clear/upload/readback, debug markers, device idle and MSAA caps. Shape decisions are recorded as ADR 0011 amendments (5)-(10): * RHI::NoAttachment - a "this draw slot writes nowhere" sentinel. DecalRenderPass steers a decal into exactly one G-Buffer attachment with GL_NONE entries, which an attachment index cannot express; both backends need it (GL_NONE / VK_ATTACHMENT_UNUSED). * glGetError DISAPPEARS rather than being abstracted. GL's error model is a sticky global flag and Vulkan's is a per-call result, so exposing either forces the other backend to fake it. The readback virtuals return bool. * DrawBoundIndexed / DrawBoundIndexedInstanced / DrawBoundArrays draw from previously bound geometry, which is the native Vulkan shape (vkCmdBindIndexBuffer + vkCmdDrawIndexed), not a GL-ism. They carry topology and index width explicitly instead of hard-coding triangles + u32. * SetProgramUniformFloat is a recorded, deliberate debt: the one virtual a Vulkan backend cannot implement faithfully, since SPIR-V has no name-queryable default uniform block. One call site (DrawInfiniteGrid's u_GridScale); Phase 6 folds it into a UBO and deletes it. * MemoryResidency moved from RHIResources.h to RHITypes.h. The sweep started reinventing it as a GL-transcribed "BufferUsage" and only a name collision with RHIResources.h's bind-flag BufferUsage surfaced that Phase 1 had already designed the concept, better. RendererAPI.h includes only RHITypes.h, so it belongs there. Two boundary leaks the ratchet cannot see were also closed: * Three passes included Platform/OpenGL/OpenGLUtilities.h for GLClearProgramGuard. Deleting their direct glad includes would have zeroed sweep_glad_includes while all three TUs could still see the whole GL API. The guard is an NVIDIA driver hazard, so it moved INTO the backend clear implementations, where ClearDepthOnly() had been carrying it correctly all along. * RGCommandContext guarded its debug markers with `if (GLAD_GL_KHR_debug)`, a glad loader symbol that does not match gl[A-Z] and so never appeared in any count. The capability check moved into OpenGLRendererAPI::PushDebugGroup. Every new enum is pinned by the same last-ordinal static_assert plus literal-GL_* token table that step 1's amendment (4) established, including a test that the NoAttachment sentinel cannot be folded into attachment-0 arithmetic. Verification: full suite 5195 run / 5189 passed / 0 failed / 6 skipped, with the goldens and visual-evidence tests executing (not skipping) and OLOENGINE_GOLDEN_REBASE unset. Nine evidence PNGs were regenerated; every one differs from HEAD by at or below its own run-to-run noise measured on the identical binary, so all were restored and this commit changes no pixels. Live editor: renders correctly, zero shader compile/link errors, zero GL_INVALID_*, and the frame breakdown shows the rewritten dispatch path executing with real GPU timings. Step 3 (TransientPool's RendererID split) is deliberately not in this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change expands the RHI and ChangesRHI abstraction migration
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9 tasks
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
OloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cpp (1)
110-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the repeated "count color attachments + rebuild 0..n-1 draw-buffer list" snippet into a shared helper.
The same ~15-line pattern — count non-depth color attachments from a
FramebufferSpecification, then build a sequentialstd::array<u32,16>of indices and callRenderCommand::SetFramebufferDrawAttachments— is copy-pasted at 8 call sites across 4 files (3 of them withinDeferredLightingPass.cppalone). A future attachment-layout change (adding/removing a G-Buffer or scene-FB RT) requires updating every site consistently; missing one silently leaves the wrong draw buffers bound.
OloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cpp#L110-L141: extractbindGBufferForDraw's count (113-121) + build (128-132) into a shared helper (e.g.Framebuffer::GetColorAttachmentCount()+RenderCommand::SetFramebufferAllColorDrawAttachments(fbID, count)).OloEngine/src/OloEngine/Renderer/Passes/DeferredLightingPass.cpp#L186-L199: reuse the shared count helper instead of the inline loop feeding the line-199 call.OloEngine/src/OloEngine/Renderer/Passes/DeferredLightingPass.cpp#L358-L370: replace the inline array rebuild with the shared full-restore helper.OloEngine/src/OloEngine/Renderer/Passes/DeferredLightingPass.cpp#L404-L423: replace this second inline rebuild (same file) with the same shared helper.OloEngine/src/OloEngine/Renderer/Passes/DeferredLightingPass.cpp#L504-L523: replace the third occurrence (BlitVirtualGeometryDebugOverlay's count + rebuild) with the shared helper.OloEngine/src/OloEngine/Renderer/Passes/ForwardOverlayRenderPass.cpp#L96-L107: reuse the shared count helper.OloEngine/src/OloEngine/Renderer/Passes/ForwardOverlayRenderPass.cpp#L147-L158: replace the inline rebuild with the shared full-restore helper.OloEngine/src/OloEngine/Renderer/Passes/GPUDrivenOcclusionPass.cpp#L117-L140: replacebindSceneForDraw's inline count + rebuild with the shared helpers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cpp` around lines 110 - 141, Extract the repeated color-attachment counting and sequential draw-buffer setup into shared helpers, such as a Framebuffer color-count method and a RenderCommand full-color draw-attachment method. Update OloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cpp:110-141, DeferredLightingPass.cpp:186-199, 358-370, 404-423, and 504-523, ForwardOverlayRenderPass.cpp:96-107 and 147-158, and GPUDrivenOcclusionPass.cpp:117-140 to use those helpers, preserving the existing attachment ordering and draw behavior.OloEngine/src/OloEngine/Renderer/ReflectionProbeBaker.cpp (1)
182-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCubemap face readback failure doesn't prevent baking the stale buffer into the probe.
RenderCommand::ReadTextureImagefailure only logs a warning;cubemap->SetFaceData(face, pixelBuffer.data(), ...)still runs on the next line regardless, uploading whateverpixelBuffercurrently holds (garbage on the first face, or the previous face's data on later faces) into the baked reflection probe's cubemap that later feedsEnvironmentMap::CreateFromCubemapand is stored asprobe.m_BakedEnvironment.🛡️ Proposed fix: skip the face upload and fail the capture on error
if (!RenderCommand::ReadTextureImage(colorAttachmentID, 0, RHI::Format::RGBA32Float, faceBytes, pixelBuffer.data())) { OLO_CORE_WARN("ReflectionProbeBaker: cubemap face readback failed"); + captureOk = false; + break; }(
captureOkwould need to become non-constand checked after the loop, or an earlyreturn nullptrused instead.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/Renderer/ReflectionProbeBaker.cpp` around lines 182 - 194, Update the face-capture flow in the reflection probe baking method around RenderCommand::ReadTextureImage and cubemap->SetFaceData so a readback failure does not upload pixelBuffer. Track the failure with a mutable capture-success flag and skip or abort the bake, returning the failure result after the loop; only call SetFaceData for successfully read-back faces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0011-rhi-neutral-resource-and-binding-model.md`:
- Around line 955-956: The cross-reference in the enum requirement must point to
the “One new guard” paragraph, which establishes the last-ordinal static_assert
and literal-token table; replace the incorrect amendment (4) reference without
changing the surrounding requirement.
- Around line 816-818: Clarify in
docs/adr/0011-rhi-neutral-resource-and-binding-model.md lines 816-818 that the
84 figure counts GL entry points while the category table counts expanded
RendererAPI virtuals, or correct the percentage so the two measurements are not
compared directly. Update docs/agent-rules/rhi-abstraction-boundary.md lines
203-205 and OloEngine/src/OloEngine/Renderer/RendererAPI.h lines 195-197 to use
the same corrected wording or an ADR cross-reference, eliminating the
inconsistent “~60%” and “~60 new virtuals” restatements.
In `@docs/agent-rules/rhi-abstraction-boundary.md`:
- Around line 143-148: Revise the “last four files standing” sentence in the
Step 2 narrative so it does not claim a set while naming only UIRenderer.cpp.
Either identify the other three files explicitly or remove the count and
describe UIRenderer.cpp as the remaining example.
In `@OloEngine/src/OloEngine/Renderer/Commands/FrameResourceManager.cpp`:
- Around line 295-307: Update the switch on RenderCommand::ClientWaitFence in
FrameResourceManager::WaitForFence to add a default branch that logs the
unrecognized fence status as an error and returns false, ensuring future
RHI::FenceStatus values are not treated as successful waits.
In `@OloEngine/src/OloEngine/Renderer/DDGI/DDGIProbeUpdatePass.cpp`:
- Around line 720-726: Update RelocateAndClassifyProbe’s ReadTextureSubImage
failure path to return immediately after logging, preventing aggregation and
probe relocation/classification from using invalid texels; preserve the existing
success path unchanged.
In `@OloEngine/src/OloEngine/Renderer/LightProbeBaker.cpp`:
- Around line 71-77: Update RenderCubemapAtPosition to propagate a failed
ReadTextureImage result instead of converting rgbaBuffer for that face; ensure
BakeProbeAtPosition/BakeVolume can observe the failure and prevent stale
readback data from contributing to persisted coefficients, with outValid
reflecting any partial capture as appropriate.
In `@OloEngine/src/OloEngine/Renderer/Ocean/OceanFFTGpu.cpp`:
- Around line 271-292: The DebugInverseFFT2D readback path still processes the
buffer after ReadTextureSubImage fails. Update the failure branch in
DebugInverseFFT2D to return immediately with the utility’s appropriate
empty/failed result, preventing normalization and result construction from using
stale readback data.
In `@OloEngine/src/OloEngine/Renderer/Passes/SceneRenderPass.cpp`:
- Around line 589-592: Extract the duplicated identity draw-buffer construction
from the scene render flow into a shared file-local helper, such as
BuildIdentityDrawBufferList(u32 count), preserving the 16-entry cap and
sequential 0..n-1 values. Replace the inline builders in the current pass and
BlitForwardVelocityDebug with calls to this helper.
In `@OloEngine/src/OloEngine/UI/UIRenderer.cpp`:
- Around line 29-32: Correct the type comment in UIRenderer.cpp so it contrasts
the engine coordinate types with the removed OpenGL types, naming the latter as
GLint/GLsizei instead of repeating i32/u32.
In `@OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp`:
- Around line 64-66: Update DrawIndexedPatches and DrawIndexedPatchesRaw to use
the cached m_MaxPatchVertices value instead of calling
glGetIntegerv(GL_MAX_PATCH_VERTICES) per draw. Preserve the existing > 0
fail-open condition in the validation logic, and keep tessellation-cap
validation centralized without changing other patch-draw behavior.
In `@OloEngine/tests/Rendering/MockRendererAPI.h`:
- Around line 602-608: Update MockRendererAPI::SetFramebufferDrawAttachments to
record the actual attachmentIndices values, not only their count in ParamU32_1,
so tests can verify each draw slot’s attachment mapping including
RHI::NoAttachment. Use the existing RecordedCall representation or extend it as
needed while preserving the recorded framebuffer handle and call name.
- Around line 792-806: Update MockRendererAPI::CreateFence to return a nonzero,
monotonically increasing fake fence ID using a counter alongside the existing ID
generators, while continuing to record the call. Preserve the existing
always-signaled behavior in IsFenceSignaled and ClientWaitFence so callers
exercise the successful fence path and use the issued handle.
In `@OloEngine/tests/Rendering/rhi_boundary_baseline.json`:
- Line 107: Replace the non-reproducible measured_on_commit value in the
rendering baseline with the actual finalized commit identifier for the recorded
0/0 measurement, removing the working-tree and phase description so the baseline
can be re-measured.
In `@OloEngine/tests/Rendering/RHIEnumLoweringTest.cpp`:
- Around line 72-90: Update the enum tripwires near the existing static_asserts
so each checked RHI enum exposes and asserts an explicit Count/Max value
representing the number of members, rather than asserting only an existing
member value. Apply this to IndexType, FrontFace, QueryType, MemoryResidency,
BlitAspect, and FenceStatus, and preserve the existing lowering/update guidance
in the assertion messages.
---
Outside diff comments:
In `@OloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cpp`:
- Around line 110-141: Extract the repeated color-attachment counting and
sequential draw-buffer setup into shared helpers, such as a Framebuffer
color-count method and a RenderCommand full-color draw-attachment method. Update
OloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cpp:110-141,
DeferredLightingPass.cpp:186-199, 358-370, 404-423, and 504-523,
ForwardOverlayRenderPass.cpp:96-107 and 147-158, and
GPUDrivenOcclusionPass.cpp:117-140 to use those helpers, preserving the existing
attachment ordering and draw behavior.
In `@OloEngine/src/OloEngine/Renderer/ReflectionProbeBaker.cpp`:
- Around line 182-194: Update the face-capture flow in the reflection probe
baking method around RenderCommand::ReadTextureImage and cubemap->SetFaceData so
a readback failure does not upload pixelBuffer. Track the failure with a mutable
capture-success flag and skip or abort the bake, returning the failure result
after the loop; only call SetFaceData for successfully read-back faces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52701d9c-10c5-4050-9fbf-f83f92bc6672
📒 Files selected for processing (52)
CLAUDE.mdOloEngine/src/OloEngine/Animation/MorphTargets/MorphTargetEvaluator.cppOloEngine/src/OloEngine/Precipitation/PrecipitationSystem.cppOloEngine/src/OloEngine/Renderer/Commands/CommandDispatch.cppOloEngine/src/OloEngine/Renderer/Commands/FrameResourceManager.cppOloEngine/src/OloEngine/Renderer/DDGI/DDGIProbeUpdatePass.cppOloEngine/src/OloEngine/Renderer/GBuffer.cppOloEngine/src/OloEngine/Renderer/IBLPrecompute.cppOloEngine/src/OloEngine/Renderer/Instancing/GPUFrustumCuller.cppOloEngine/src/OloEngine/Renderer/LightCulling/TiledForwardPlus.cppOloEngine/src/OloEngine/Renderer/LightProbeBaker.cppOloEngine/src/OloEngine/Renderer/Occlusion/OcclusionQueryPool.cppOloEngine/src/OloEngine/Renderer/Ocean/OceanFFTGpu.cppOloEngine/src/OloEngine/Renderer/Passes/BloomRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/DecalRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cppOloEngine/src/OloEngine/Renderer/Passes/DeferredLightingPass.cppOloEngine/src/OloEngine/Renderer/Passes/DeferredOpaqueDecalPass.cppOloEngine/src/OloEngine/Renderer/Passes/FluidCompositePass.cppOloEngine/src/OloEngine/Renderer/Passes/FluidIntermediatesPass.cppOloEngine/src/OloEngine/Renderer/Passes/ForwardOverlayRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/GPUDrivenOcclusionPass.cppOloEngine/src/OloEngine/Renderer/Passes/GTAORenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/OITPrepareRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/OITResolveRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/PlanarReflectionRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/SSAORenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/SceneRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/WaterRenderPass.cppOloEngine/src/OloEngine/Renderer/RGCommandContext.cppOloEngine/src/OloEngine/Renderer/RHI/RHIResources.hOloEngine/src/OloEngine/Renderer/RHI/RHITypes.hOloEngine/src/OloEngine/Renderer/ReflectionProbeBaker.cppOloEngine/src/OloEngine/Renderer/RenderCommand.hOloEngine/src/OloEngine/Renderer/RenderGraph.cppOloEngine/src/OloEngine/Renderer/Renderer3DLifecycle.cppOloEngine/src/OloEngine/Renderer/RendererAPI.hOloEngine/src/OloEngine/Renderer/ShaderPack.cppOloEngine/src/OloEngine/Renderer/VirtualGeometry/VirtualGeometryPass.cppOloEngine/src/OloEngine/Renderer/VirtualGeometry/VirtualGeometryShadow.cppOloEngine/src/OloEngine/Renderer/VirtualGeometry/VirtualMeshRegistry.cppOloEngine/src/OloEngine/SaveGame/ThumbnailCapture.cppOloEngine/src/OloEngine/Scene/Scene.cppOloEngine/src/OloEngine/UI/UIRenderer.cppOloEngine/src/Platform/OpenGL/OpenGLRHIConversions.hOloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cppOloEngine/src/Platform/OpenGL/OpenGLRendererAPI.hOloEngine/tests/Rendering/MockRendererAPI.hOloEngine/tests/Rendering/RHIEnumLoweringTest.cppOloEngine/tests/Rendering/rhi_boundary_baseline.jsondocs/adr/0011-rhi-neutral-resource-and-binding-model.mddocs/agent-rules/rhi-abstraction-boundary.md
💤 Files with no reviewable changes (3)
- OloEngine/src/OloEngine/Renderer/ShaderPack.cpp
- OloEngine/src/OloEngine/Renderer/Passes/OITResolveRenderPass.cpp
- OloEngine/src/OloEngine/Renderer/Passes/BloomRenderPass.cpp
Comment on lines
+72
to
+90
| // Phase 2 step 2 vocabulary (ADR 0011 amendment (10)). Same tripwire | ||
| // discipline: IndexType in particular has only two members, which makes a | ||
| // swapped mapping look harmless right up until a u16-indexed mesh reads its | ||
| // element buffer at 4-byte stride and renders as scattered triangles. | ||
| static_assert(static_cast<int>(RHI::IndexType::UInt32) == 1, | ||
| "RHI::IndexType changed — update ToGL() and IndexTypeLowering"); | ||
| static_assert(static_cast<int>(RHI::FrontFace::Clockwise) == 1, | ||
| "RHI::FrontFace changed — update ToGL() and FrontFaceLowering"); | ||
| static_assert(static_cast<int>(RHI::QueryType::TimeElapsed) == 1, | ||
| "RHI::QueryType changed — update ToGL() and QueryTypeLowering"); | ||
| static_assert(static_cast<int>(RHI::MemoryResidency::DeviceToHost) == 2, | ||
| "RHI::MemoryResidency changed — update ToGL() and MemoryResidencyLowering"); | ||
| static_assert(static_cast<int>(RHI::BlitAspect::DepthStencil) == 3, | ||
| "RHI::BlitAspect changed — update ToGLBlitMask() and BlitAspectLowering"); | ||
| // FenceStatus has no ToGL() — it is produced BY the backend, not consumed by | ||
| // it — so its tripwire lives with ClientWaitFence's switch instead. Pinned | ||
| // here anyway so a new member is noticed at the same place as its siblings. | ||
| static_assert(static_cast<int>(RHI::FenceStatus::Failed) == 3, | ||
| "RHI::FenceStatus changed — update OpenGLRendererAPI::ClientWaitFence"); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the enum tripwires detect appended members.
These assertions do not detect a new member appended after the current last member—for example, adding another QueryType leaves TimeElapsed == 1, so an unhandled value could still fall through the lowering default. Add an explicit Count/Max enumerator (or an equivalent maintained count) and assert that instead for every new enum.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/tests/Rendering/RHIEnumLoweringTest.cpp` around lines 72 - 90,
Update the enum tripwires near the existing static_asserts so each checked RHI
enum exposes and asserts an explicit Count/Max value representing the number of
members, rather than asserting only an existing member value. Apply this to
IndexType, FrontFace, QueryType, MemoryResidency, BlitAspect, and FenceStatus,
and preserve the existing lowering/update guidance in the assertion messages.
Two of these were load-bearing and one was a bug introduced by the sweep. * UIRenderer.cpp's clip-rect comment was corrupted by the sweep's own bulk GLint->i32 rewrite: it rendered as "i32/u32, not i32/u32" and "issue //691". Rewritten to say what it meant. * Nine call sites open-coded the same identity draw-attachment list (a std::array, a fill loop and a span). The facade was simply missing the method: RestoreAllFramebufferDrawAttachments(framebufferID, count) is the named-framebuffer counterpart of the RestoreAllDrawBuffers(u32) that already existed for the BOUND framebuffer. All nine collapse to one line, across SceneRenderPass, DeferredLightingPass (x3), ForwardOverlayRenderPass, GPUDrivenOcclusionPass, DeferredGPUOcclusionPass, VirtualGeometryPass and GBuffer. Restoring a narrower list than the target actually has silently drops later shader outputs, which is exactly the off-by-one an open-coded loop invites. * Three migrated readbacks logged failure and then used the buffer anyway — a half-fix from the sweep. DDGI now skips that probe for the frame (the update is amortized, so it retries); OceanFFTGpu::DebugInverseFFT2D returns empty; and LightProbeBaker, the one that matters, reports the probe through its existing outValid out-param instead of projecting undefined pixels into SH coefficients that get PERSISTED into LightProbeVolumeAsset. * SetPatchVertexCount cached GL_MAX_PATCH_VERTICES but the two pre-existing patch-draw sites still queried it per call. Both use the cache now. * MockRendererAPI records the draw-attachment INDICES, not just how many (DecalRenderPass's four modes all pass five entries, so a count cannot tell them apart), and CreateFence() returns a non-zero handle so tests exercise the success path rather than only the creation-failed branch. * rhi_boundary_baseline.json pins measured_on_commit to 11d7cea instead of naming a working tree. * The 84 / 54 / 60 figures are stated as three distinct quantities in all three places that carried them. An entry point can expand into more than one virtual (glClearTexImage becomes a float and a uint clear; the readbacks each gained a bool return), so dividing 60 by 84 compared an operation count with an API count. 54 is the size of the gap, 60 the size of the fix. Also fixes a wrong internal cross-reference and a sentence that promised four files and named one. Two findings were fixed differently than suggested, both to avoid trading a compile-time guarantee for a runtime one: * A `default:` on the RHI::FenceStatus switch would suppress the compiler's exhaustiveness warning, which is what actually catches a new enumerator. The switch stays exhaustive with no `default:`; the post-switch fallthrough is the backstop and now FAILS CLOSED, because a fence gates reuse of double-buffered GPU resources and reporting success for an unrecognized status is the one answer that could hand a caller memory the GPU still owns. * The last-ordinal static_assert genuinely cannot detect an APPENDED enumerator. Chasing that turned up something worse: 16 of the 19 lowering switches deliberately carry no `default:` label — which is what makes clang's -Wswitch catch an append — while OpenGLRHIConversions.h's header comment claimed "Every switch carries a `default:`". Reconciling the code to that comment would have removed the only append guard, engine-wide. The comment is corrected and the division of labour is now documented in the header, the test and the ADR: static_assert catches insert/remove/reorder, absent-`default:` + -Wswitch catches append, and MSVC's C4062 being off by default at /W4 makes the clang-cl CI job the one that enforces it. A per-enum Count sentinel was considered and rejected: it makes an invalid value representable in the neutral vocabulary and forces a dead case in every lowering switch. Full suite 5195 run / 5191 passed / 0 failed / 4 skipped; the RHI ratchet still reports 0 / 0 / 236 / 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Worked through all 14. Thanks — two of these were genuinely load-bearing, and one caught a bug I introduced. Fixed as suggested
Two I fixed differently, with reasoning 1. So I kept the switch exhaustive with no 2. Enum tripwires don't detect appended members. This is correct and it's the most useful finding in the review — but chasing it turned up something worse than the gap itself. Appends are caught, just not by the So rather than change the assertions I documented the actual division of labour, in the test, the ADR, and that header comment:
I considered a Rebuilt clean; full suite 5195 run / 0 failed / 6 skipped (same environment-dependent skips), and the RHI ratchet still reports 0 / 0 / 236 / 0. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp (1)
1735-1748: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard debug groups by actual entry-point availability, not
GLAD_GL_KHR_debug.
GLAD_GL_KHR_debugis a loader symbol and is not the same capability/API symbol the renderer is supposed to rely on when it now checksglad_glCreateTextures != nullptrelsewhere. Useglad_glPushDebugGroup != nullptr/ core-version support instead so core 4.6 marker support is not skipped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp` around lines 1735 - 1748, Update OpenGLRendererAPI::PushDebugGroup and OpenGLRendererAPI::PopDebugGroup to guard each debug-group call using actual entry-point availability, specifically glad_glPushDebugGroup (or equivalent core-version support), instead of GLAD_GL_KHR_debug. Preserve the early-return behavior when the entry point is unavailable and allow core 4.6 debug markers to execute.OloEngine/src/OloEngine/Renderer/LightProbeBaker.h (1)
50-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument readback failure in the public validity contract.
BakeProbeAtPositionnow also reports GPU readback failures throughoutValid, but its public comment still saysfalseonly means the probe is inside geometry. Update that contract so callers do not misclassify failed bakes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/Renderer/LightProbeBaker.h` around lines 50 - 56, Update the public documentation for BakeProbeAtPosition to state that outValid is false both when the probe is inside geometry and when GPU readback fails. Ensure callers are directed not to persist or project SH data when the bake is invalid, while preserving the existing RenderCubemapAtPosition contract.OloEngine/src/OloEngine/UI/UIRenderer.cpp (1)
80-96: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win**Validate clip dimensions before converting to
u32.`
size.x/yare converted before clamping, and non-finite/negative inputs can flow intou32or thei32scissor intersections. Clamp or reject invalid dimensions before the conversion so the scissor rect cannot receive malformed ranges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/UI/UIRenderer.cpp` around lines 80 - 96, In the UI rectangle conversion and parent-clip intersection logic, validate that size.x and size.y are finite and non-negative before converting them to u32 or using them in i32 calculations. Reject or safely clamp invalid dimensions first, then preserve the existing clipping behavior for valid sizes so the scissor rectangle never receives malformed ranges.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp`:
- Around line 1328-1334: Remove the fixed 16-element cap in the attachment setup
around SetFramebufferDrawAttachments. Allocate or use storage sized for the
requested count, populate every attachment index from 0 through count - 1, and
preserve the existing framebuffer draw-attachment call so all requested outputs
are restored.
---
Outside diff comments:
In `@OloEngine/src/OloEngine/Renderer/LightProbeBaker.h`:
- Around line 50-56: Update the public documentation for BakeProbeAtPosition to
state that outValid is false both when the probe is inside geometry and when GPU
readback fails. Ensure callers are directed not to persist or project SH data
when the bake is invalid, while preserving the existing RenderCubemapAtPosition
contract.
In `@OloEngine/src/OloEngine/UI/UIRenderer.cpp`:
- Around line 80-96: In the UI rectangle conversion and parent-clip intersection
logic, validate that size.x and size.y are finite and non-negative before
converting them to u32 or using them in i32 calculations. Reject or safely clamp
invalid dimensions first, then preserve the existing clipping behavior for valid
sizes so the scissor rectangle never receives malformed ranges.
In `@OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp`:
- Around line 1735-1748: Update OpenGLRendererAPI::PushDebugGroup and
OpenGLRendererAPI::PopDebugGroup to guard each debug-group call using actual
entry-point availability, specifically glad_glPushDebugGroup (or equivalent
core-version support), instead of GLAD_GL_KHR_debug. Preserve the early-return
behavior when the entry point is unavailable and allow core 4.6 debug markers to
execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cf2cbfe8-640b-43af-b5aa-9a9858d80e5b
📒 Files selected for processing (23)
OloEngine/src/OloEngine/Renderer/Commands/FrameResourceManager.cppOloEngine/src/OloEngine/Renderer/DDGI/DDGIProbeUpdatePass.cppOloEngine/src/OloEngine/Renderer/GBuffer.cppOloEngine/src/OloEngine/Renderer/LightProbeBaker.cppOloEngine/src/OloEngine/Renderer/LightProbeBaker.hOloEngine/src/OloEngine/Renderer/Ocean/OceanFFTGpu.cppOloEngine/src/OloEngine/Renderer/Passes/DeferredGPUOcclusionPass.cppOloEngine/src/OloEngine/Renderer/Passes/DeferredLightingPass.cppOloEngine/src/OloEngine/Renderer/Passes/ForwardOverlayRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/GPUDrivenOcclusionPass.cppOloEngine/src/OloEngine/Renderer/Passes/SceneRenderPass.cppOloEngine/src/OloEngine/Renderer/RenderCommand.hOloEngine/src/OloEngine/Renderer/RendererAPI.hOloEngine/src/OloEngine/Renderer/VirtualGeometry/VirtualGeometryPass.cppOloEngine/src/OloEngine/UI/UIRenderer.cppOloEngine/src/Platform/OpenGL/OpenGLRHIConversions.hOloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cppOloEngine/src/Platform/OpenGL/OpenGLRendererAPI.hOloEngine/tests/Rendering/MockRendererAPI.hOloEngine/tests/Rendering/RHIEnumLoweringTest.cppOloEngine/tests/Rendering/rhi_boundary_baseline.jsondocs/adr/0011-rhi-neutral-resource-and-binding-model.mddocs/agent-rules/rhi-abstraction-boundary.md
The helper added in the previous commit contradicted its own declaration comment: that comment warns that restoring a narrower list than the target actually has silently drops later fragment outputs, and then the implementation clipped the list to a 16-entry stack array whenever GL_MAX_DRAW_BUFFERS was larger. Above 16 it now allocates instead of clipping, so GL_MAX_DRAW_BUFFERS is the only ceiling — and that one already clamps with a warning. The stack path still covers every framebuffer in the engine (the G-Buffer is the widest at 5), so this costs nothing in practice; it just stops the cap being silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Good catch, and a fair one — that helper contradicted its own declaration comment. The comment warns that restoring a narrower list than the target actually has silently drops later fragment outputs, and then the implementation clipped to a 16-entry stack array whenever Fixed in Rebuilt clean; RHI ratchet + enum lowering (21 tests) and the render-graph / deferred / G-Buffer / scene-pass / occlusion / framebuffer / visual-evidence selection (494 tests) all pass. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the headline target of Phase 2 of #691: the raw OpenGL call sites are gone from
OloEngine/src/**, and the translation units there can no longer name a GL symbol at all.Step 1 (#698) converted the facade's vocabulary. This is step 2 of 3 — the call-site sweep. Step 3 is deliberately not here (reasons at the bottom), so Phase 2's checkbox stays unticked.
Counters
sweep_gl_callssweep_glad_includestools_gl_callsdebug_escape_hatchsweep_glad_includesis the one that proves something: a TU that cannot see<glad/gl.h>provably cannot name a GL symbol. That is compiler-enforced, not a grep.rhi_boundary_baseline.jsonis updated in the same commit, as the ratchet requires.The finding: the facade wasn't just GL-typed, it was incomplete
ADR 0011 §1.7 framed Phase 2 as "strip the
GLenums, then sweep". That undersold it by about half.84 distinct GL entry points appear across the 313 call sites, and ~60% had no
RendererAPIequivalent at all — so closing the sweep needed ~60 new virtuals against an existing 74. Whole categories had never been abstracted, so every pass reached past the facade to do them:glBindBufferBase)VirtualMeshRegistryhand-rolls an arena + persistent-mapped upload ringSetDrawBuffersexisted but only for the bound FBO; every site names one via DSABeginConditionalRenderexisted; the pools feeding it did notFrameResourceManagerusedGLsyncdirectlyCommandDispatchkeeps a redundant-bind cache the*Rawfamily would defeatu32, not aRef<Shader>Generalisable, and worth carrying into Phase 5: an abstraction's completeness is measured by how many distinct operations the layer above performs, not by how many call sites it already serves. 74 virtuals looked thorough while 60 operations went around it, because each was rare enough (1–3 sites) to read as a special case. Frequency wasn't the signal either —
glBindBufferBasehad 26 sites and was still missing.ADR 0011 amendments (5)–(10)
RHI::NoAttachment— a "this draw slot writes nowhere" sentinel.DecalRenderPasssteers a decal into exactly one G-Buffer attachment usingGL_NONEentries, which an attachment index cannot express. Both backends need it (GL_NONE/VK_ATTACHMENT_UNUSED); only the neutral layer lacked it.glGetErrordisappears rather than being abstracted. GL's error model is a sticky global flag, Vulkan's is a per-call result — exposing either forces the other backend to fake it. The readback virtuals returnboolinstead.DrawBoundIndexed/…Instanced/DrawBoundArraysdraw from previously bound geometry. That is the native Vulkan shape (vkCmdBindIndexBuffer+vkCmdDrawIndexed), not a GL-ism — the combined bind-and-draw*Rawform is the less portable one. They also carryRHI::PrimitiveTopology/RHI::IndexTypeexplicitly instead of hard-coding triangles +u32.SetProgramUniformFloatis the single virtual a Vulkan backend cannot implement faithfully — SPIR-V has no name-queryable default uniform block. One call site (DrawInfiniteGrid'su_GridScale). The alternatives were a shader change (forfeits golden-image parity, this branch's only safety net) or reaching forShader(the POD dispatcher holds au32by design). Phase 6 folds it into a UBO and deletes it. Flagged now rather than surprising Phase 7 bring-up.MemoryResidencymoved fromRHIResources.htoRHITypes.h— see below.Two boundary leaks the ratchet cannot see, also closed
A
Platform/<Backend>/include leaks exactly as much asglad/gl.h. Three passes (FluidIntermediatesPass,WaterRenderPass,VirtualMeshRegistry) includedPlatform/OpenGL/OpenGLUtilities.hforGLClearProgramGuard. Deleting their direct glad includes would have drivensweep_glad_includesto zero while all three TUs could still name every symbol in OpenGL — the exact counter-gaming the baseline's own_commentwarns about, arrived at honestly. The fix is a layering one: the guard is an NVIDIA driver hazard, so it moved into the backend clear implementations, whereClearDepthOnly()had been carrying it correctly all along. The passes now get the behaviour for free.A loader-symbol probe is invisible by construction.
RGCommandContextguarded its debug markers withif (GLAD_GL_KHR_debug)— which does not matchgl[A-Z]and so never appeared in any count. The capability check moved intoOpenGLRendererAPI::PushDebugGroup. Same class as step 1'sglad_glCreateTextures != nullptrcontext probe.A third near-miss is recorded because the catch was luck. The sweep started inventing
RHI::BufferUsage { DynamicDraw, DynamicCopy, DynamicRead }— a straight transcription of GL's hints. Phase 1 had already designed that concept, better, asRHI::MemoryResidency { DeviceLocal, HostToDevice, DeviceToHost }, sitting besideBufferDescwhere nothing could reach it. What surfaced the duplication was a name collision withRHIResources.h's bind-flagBufferUsage— and it only fired in the test build, since the engine library never includes that header. Rename either enum and a duplicate concept ships silently. Lesson written up: read an earlier phase's declaration-only header for the vocabulary you are about to invent, not just the types you consume.Testing
Full suite: 5195 run / 5189 passed / 0 failed / 6 skipped. Goldens and visual-evidence tests executed, not skipped;
OLOENGINE_GOLDEN_REBASEunset throughout. The 6 skips are the same environment-dependent ones as refactor(rhi): strip GLenum/GLuint out of RendererAPI's interface (#691 Phase 2, step 1) #698 (app-launch smoke ×3, video fixture, real-asset cook, MCP headless attach).New tests.
RHIEnumLoweringTestgains 6 cases for the new enums, each with the last-ordinalstatic_asserttripwire amendment (4) established — including one asserting theNoAttachmentsentinel cannot be folded into attachment-0 arithmetic (GL_NONEis notGL_COLOR_ATTACHMENT0 + anything, and collapsing it would leave the previous draw-buffer list live).This PR changes no pixels. Nine evidence PNGs regenerated; each was pixel-diffed against
HEADafter measuring the run-to-run noise floor on the identical binary. Every delta sits at or below its own noise:EASU_GTAONativeFluid_WaterlineFluid_SideFluid_ThreeQuarterVirtualGeometry_Debug_ClusterIdOcclusionCull_*,WorldOriginRebase,CameraRelative,SphereAreaLightThe
max=158on the cluster-ID debug image was the alarming one; it appears identically in the no-change control, so it is GPU-order-dependent cluster-ID colouring, not a regression. All nine restored.Live editor. Renders correctly; zero shader compile/link errors and zero
GL_INVALID_*(exactly what a wrong enum lowering or a bad draw-buffer list would produce).olo_render_frame_breakdownshows the rewritten dispatch path (DrawMeshInstanced/DrawSkybox) executing with real GPU timings.One verification gap, stated plainly
I could not exercise virtual geometry or deferred lighting in the live editor. Both require the Deferred path; that path is not scene-serialized (the
VirtualGeometrySponza.oloheader documents this and says to switch it by hand after load), and the MCPolo_renderer_settings_setwrite gate is off by default and cannot be unlocked non-interactively. Both paths are covered by the suite's evidence/golden tests, which executed and matched HEAD within noise. Worth a human eyeball onVirtualGeometrySponza.olo+SponzaDeferred.olowith the path switched to Deferred.One behaviour delta worth knowing about
VirtualGeometryPassset depth state with rawglEnable(GL_DEPTH_TEST), which leftOpenGLRendererAPI::m_DepthTestEnabledstale — andClear()derives itsGLbitfieldfrom that member. Routing the pass through the facade makes that cache truthful, so a laterClear()can now clear depth where it previously did not. That is a fix, but it is rendering-visible; the golden/evidence tests show no resulting pixel change.Profiler counters also move slightly: the facade's state setters bump
RendererProfiler::StateChangeswhere the raw calls did not. Draw counters deliberately do not move — the newDrawBound*family does not touchRendererProfiler, because the call sites it replaces never did and several keep their ownCommandDispatch::Statistics.Why step 3 is not in this PR
Step 3 is
TransientPool's bareRendererID/GetRendererID()split three ways per ADR 0011 §1.1/§1.3/§1.4. Three reasons it is separate rather than an unjustified slice:RHI::ResourceHandlehas no producer.RHIResources.hsays so in its own header comment, andGetNativeHandleForDebugis declared but defined nowhere. Step 3 needs a generation-checked handle-minting layer built first.GetRendererID()has ~320 call sites, not a handful (Scene.cpp56,Renderer3DMeshSubmission.cpp33, …).BindTexture"does not get abstracted, it disappears" — under heap-bindless, which is Phase 3'sARB_bindless_texturerehearsal.This matches how #691 is deliberately structured (phase-at-a-time PRs; step 1 shipped alone as #698).
🤖 Generated with Claude Code
Summary by CodeRabbit