Repository navigation
Fix/flaky fog visual evidence test - #465
Conversation
…ogVisualEvidenceTest (#446) The Fog (binding 17) and FogVolumes (binding 20) UBOs are glBindBufferBase'd only in their constructor and thereafter only have their contents refreshed via SetData. Deleting any GL buffer that GL currently has bound at a slot reverts that slot to 0, so ordinary cross-test buffer churn in the full suite knocked binding 17 to 0. The late post-process fog pass then read an unbound FogData UBO (all zeros) -> u_FogFlags.x == 0 -> the shader's disabled early-out -> fog silently not applied, producing a frame byte-identical with fog OFF. This only manifested in the full suite (isolation never accumulated the churn), making FogVisualEvidenceTest flaky. Re-bind the Fog and FogVolumes UBOs after every per-frame SetData, mirroring the binding-8 motion-blur UBO which already guards against this exact hazard. Adds a deterministic regression test (ReappliesFogAfterFogUboBindingKnockedOut) that knocks binding 17 to 0 directly so it guards the fix even in isolation, plus a postmortem in docs/bug-investigations. Confirmed via GPU read-back of the live UBO binding ids: full suite had binding 17 unbound (id 0) vs id 46 in isolation. Full clean suite now green. Fixes #446 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…raPosition read During the fog post-process pass, UBO binding 0 can hold a 64-byte ViewProjection-only camera UBO (Renderer2D / ParticleBatchRenderer style) left by an earlier stage, not the full 272-byte CameraUBO. The fog shaders read past it: u_CameraPosition (std140 offset 192, PostProcess_Fog.glsl) and u_Projection (offset 128, PostProcess_FogUpsample.glsl). Origin-centred scenes survive only because NVIDIA's robust-buffer-access returns 0 for OOB reads (0 happens to equal the true camera there); OOB UBO reads are undefined per the GL spec, and any scene whose camera sits far from the world origin fogs wrong. FogRenderPass::Execute now re-binds the full shared camera UBO at binding 0 before drawing (plumbed via SetCameraUBO), mirroring its existing binding-7 PostProcess UBO re-bind. Re-binding at the per-frame upload site does not work here because a later scene stage re-binds the 64-byte buffer afterward. Adds FogVisualEvidenceOffOriginTest, which renders the identical scene ~1000 units from the origin; verified empirically that without the re-bind the near floor floods blue (nearOn.R=113 B=182) and with it the near floor stays warm. Surfaced during the #446 investigation; documented in docs/bug-investigations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo OpenGL UBO binding hazards in fog rendering are fixed. ChangesFog UBO Binding Fixes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/OloEngine/Renderer/Passes/FogRenderPass.h`:
- Around line 76-85: `FogRenderPass` still treats the pass as ready without
verifying `m_CameraUBO`, so a missing `SetCameraUBO()` can let `Execute()` run
against an unintended binding-0 UBO. Update `IsReadyForExecution()` to include
`m_CameraUBO` in its readiness check, or add a fail-fast guard in `Execute()`
using the same `FogRenderPass`/`SetCameraUBO` state so the pass cannot run
without the required camera buffer.
In `@OloEngine/tests/Rendering/PropertyTests/FogVisualEvidenceTest.cpp`:
- Around line 395-411: The FogVisualEvidenceTest setup is mutating global GL
state by unbinding the Fog UBO without restoring it, which can leak into later
GPU tests. In the test around the glBindBufferBase(GL_UNIFORM_BUFFER,
ShaderBindingLayout::UBO_FOG, 0) call, save the current binding first and
restore it afterward using a small RAII/scoped guard so the original Fog UBO
binding is reinstated even on fatal failures. Keep the change localized to the
test flow around Capture and the post-frame glGetIntegeri_v invariant check.
🪄 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
Run ID: d03d4706-21c4-40f1-9f27-4c52f0b181f1
⛔ Files ignored due to path filters (2)
OloEditor/assets/tests/visual/Fog_KnockoutOn.pngis excluded by!**/*.pngOloEditor/assets/tests/visual/Fog_OffOriginOn.pngis excluded by!**/*.png
📒 Files selected for processing (6)
OloEngine/src/OloEngine/Renderer/Passes/FogRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/FogRenderPass.hOloEngine/src/OloEngine/Renderer/RenderPipeline.cppOloEngine/tests/Rendering/PropertyTests/FogVisualEvidenceTest.cppdocs/README.mddocs/bug-investigations/fog-ubo-binding-knockout-investigation.md
…event rendering issues
Summary by CodeRabbit