fix(renderer): decal per-channel colour masks, and the four defects hiding behind them (#853) - #886
Open
drsnuggles8 wants to merge 7 commits into
Open
fix(renderer): decal per-channel colour masks, and the four defects hiding behind them (#853)#886drsnuggles8 wants to merge 7 commits into
drsnuggles8 wants to merge 7 commits into
Conversation
…ead/colour write Opening any Deferred scene containing an opaque decal killed the editor. SceneDepth, SceneNormals and the three GBuffer* views are all attachment views of the SAME framebuffer (RenderPipeline.cpp's resolvedGBuffer). Read() propagates to the parent framebuffer and the hazard validator expands that parent read back down onto every sibling attachment view, so the decal shader's depth sample reads -- by name -- every colour view the pass writes. Four same-pass feedback hazards, none of them real: one depth attachment read, four colour attachments written, subresources disjoint throughout. This is the legitimate-RMW case RGBuilder::Write's own comment names, and AllowSamePassReadWrite is how a pass states it. Not cosmetic: OLO_CORE_ASSERT on the compiled-hazard list is a __debugbreak, so a Debug build with no debugger terminates. It went unnoticed because the graph validation only runs when the graph's shape changes and the pass only declares these accesses when it has decal work -- so it took the first scene to put a decal on the deferred path to reach it. Verified live: scene opens, RenderGraph hazard count 0, assertion count 0, editor ticking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…paths Renderer3D::DrawDecal paired cullFace = Front -- which keeps the projection box's BACK faces -- with depthFunction = LessOrEqual, which rejects them: the box's far side is behind the surface the decal projects onto. So a decal straddling its receiving surface produced no fragments at all. Both paths read this one PODRenderState, so deferred and forward were equally dead. It predates the RHI refactor (GL_LEQUAL + GL_FRONT) and nothing pinned it. The pass tenant substitutes a proxy QUAD drawn in front of the surface for the production cube, so the one property the cube has and the quad does not -- which of its faces survive the depth test -- was untested; and no sandbox scene put a decal on either path. Back-face decal rendering wants "the box's far side is at or behind the geometry" (GreaterOrEqual); the shader's own in-box test then rejects surfaces that are merely further away. Measured before/after at the same camera pose: with LessOrEqual, GBufferAlbedo is a uniform floor colour with no decal anywhere; with GreaterOrEqual an Albedo decal's authored (1, 0.25, 0.15) lands as (255, 64, 38). Also extracts the decal render state into CreateDecalPODRenderState so the pass tenant applies the state production builds rather than a hand-written copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DecalRenderPass installs channel-level colour masks per decal mode, then the next packet wipes them before the draw: ApplyPODRenderState issues the GLOBAL SetColorMask, which is the indexed call for EVERY draw buffer on both backends (docs/agent-rules/gl-global-setter-resets-indexed-state.md), and the narrowing loop that follows could only re-DISABLE whole attachments -- the decal packet carries the default 0xFF, so it re-narrowed nothing. PODRenderState::colorAttachmentWriteMask is one BIT per attachment and cannot express "write only RT1.xy", so this adds colorAttachmentChannelMask: one nibble per attachment, R,G,B,A low bit first, default all-writable. Shape (a) of the two the issue named -- the mask travels on the COMMAND, not as a pass-scoped "these masks are mine for the next N draws" override. Three reasons: a pass-scoped override's lifetime is manual and a missed clear is the process-permanent indexed-state leak #823 was about; the loop it replaces is already the same idea one bit wide, so this generalises existing code instead of adding a parallel mechanism; and DrawDecalCommand's own OIT-override comment states the design rule ("keeps the queue stateless and replay-safe"). Composition is an AND -- global mask, then the nibble, then the attachment bit; never widening past the global call. That makes the previous behaviour a strict special case, so Renderer3DUtilityDraws' skeleton/joint (0x01) and infinite grid (0xFF & ~(1 << 2)) draws are byte-identical, and an all-writable nibble costs no indexed call at all. DecalGBufferChannelMask is the single source for the routing: DecalRenderPass drives its SetColorMaskForAttachment calls from it and Renderer3D::DrawDecal stamps the same value onto the packet, so the masks the pass installs and the masks the draw re-asserts cannot drift. Deferred path only -- a forward decal draws into scene colour or the WB-OIT accum/revealage MRT, where masking RT1/RT2 would break compositing. Closes #853 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vable scene texture paths PlaceholderTexture::CreatePlaceholderTexture built a full 64x64 magenta/black checkerboard into a local vector, created the texture, and never uploaded it -- "Note: Data upload may require additional API not available here", which is stale (Texture2D::SetData has been on the interface throughout). So every placeholder in the engine was an UNINITIALISED texture, sampling as zeros in practice. The whole purpose of this asset is to be loud. Instead, a failed asset load rendered as transparent black, which does not read as "this asset is missing" -- it reads as "there is nothing here". Found via a decal whose texture path did not resolve: the sample zeroed the decal's alpha and every fragment was discarded, so the decal vanished with nothing on screen connecting it to the "Failed to load asset" lines thousands of entries earlier in the log. Also warns, naming the path, when a scene texture path does not resolve to a Texture2D. Scene texture paths go through EditorAssetManager::ImportAsset and resolve against the PROJECT asset root -- not the working directory, which is the spelling shaders and environment maps use. That warning caught a second bad path during verification of this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h, add the missing scene The tenant substituted FixtureDecalDispatch -- a test-owned dispatch that set depth/blend/culling by hand and never called SetColorMask -- so its channel assertions passed for a reason that did not hold in production. It now applies the production render state through the production function, CommandDispatch::ApplyPODRenderState, on a state built by the shared CreateDecalPODRenderState rather than a hand-written copy. Only the resource-binding half stays substituted, because Renderer3D's statics are live OpenGL objects mid-suite and cannot be bound on the Vulkan device; that half is orthogonal to the colour-mask contract. Confirmed failing before the fix and passing after: RT0.a (metallic) is masked out and must survive -> 255 (clear was 64) RT0.rgb (albedo) is masked out under RMA -> 0,0,0 -- the floor goes black RT1.xy (the oct normal) is masked out under RMA -> moved by 0.25 RT1.z (roughness) ... must survive -> off by 0.25 RT1.w (AO) ... must survive -> off by 0.75 The Emissive arm does NOT fail, and that is correct: RT2 blends One/One, so dst.a + 0 leaves the unlit flag alone whether or not the mask survives. The issue lists an Emissive RT2.a leak as evidence to capture; it is not observable and is not cited as such. Adds PODRenderStateChannelMaskTest -- seven backend-independent contract tests driving the real ApplyPODRenderState against MockRendererAPI, which is the only coverage of the OpenGL arm of the same flattening. Includes a control (a default state must issue NO indexed call, so a loop that re-asserted everything unconditionally would fail) and a guard that the new field is in PODRenderState::operator==, without which FrameDataBufferManager would hand back a cached index for a different mask and silently reinstate the bug. The mock now records colour-mask CHANNELS, not just the call name -- a name-only record cannot tell a mask that masks from one that does nothing. Adds DecalModeMatrixTest.olo: one decal per mode on one uniform floor. No sandbox scene had put a decal on either rendering path, which is a large part of why all of this stayed invisible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 57 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (23)
📒 Files selected for processing (20)
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 |
This was referenced Aug 22, 2026
…nts at their issues Two comment-only corrections, both found while fixing #853. DecalRenderPass's guard comment claimed "the current GLStateGuard only detects leaks, it does not roll back". GLStateGuard::Policy::Restore exists and does roll back via GLStateSnapshot::ApplyCore() -- so the comment is wrong, and is plausibly why this call site never asked for it. Corrected, and pointed at #895 along with what the pass actually leaks (8-9 fields at ERROR level on every frame that drains a decal, on both rendering paths). VulkanRendererAPI's AttachmentBlend comment said the asymmetry "is filed rather than changed here" without saying where. It is #896 now, with the note that the naive symmetric fix was already written and reverted during #823. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…scades, observer camera) 23 evidence PNGs that their originating features never committed. Same shape as 8d771bf, which did this after the fact for #439. VirtualShadowMap* 14 files -- the whole family, ZERO tracked before this ObserverCamera_* 5 files -- that PR committed some of its outputs but not these (different names, same test) ddgi_cascade_* 4 files -- ddgi_angle_*/ddgi_leak_* were tracked, the cascade set was not No .gitignore rule covers them; 305 siblings in the same directory are tracked. The likely reason nobody noticed: these tests are GPU-gated and SKIP cleanly on headless CI, so CI never produces the files and an author without a GL 4.6 context never sees them appear. These are EVIDENCE, not goldens. assets/tests/visual/ is write-only -- 59 test files write into it and nothing reads from it (verified by grep). The L8 SSIM baselines that ARE loaded and compared live in assets/tests/golden/, all 8 are tracked, and none were missing. So committing these defends nothing and cannot bake in a false gate; it only completes the human-facing record. Provenance: rendered on an RTX 4090 during the #853 verification run, not by the features' own authors. TWO PAIRS ARE BYTE-IDENTICAL AND PROBABLY SHOULD NOT BE, flagged rather than quietly committed: ObserverCamera_frozen_pose_view.png == ObserverCamera_wireframe_pose_view.png Both are Capture(MakeFrozenPoseCamera(), ...) at ObserverCameraVisual EvidenceTest.cpp:545 and :596; only the wireframe lever differs, and the frames are identical, so that lever may not be reaching the capture. The wireframe shot's result is passed as `ignored`, so nothing asserts it. VirtualShadowMap_Probe_Plain.png == VirtualShadowMap_VSM_Angled.png Two different configurations in VirtualShadowMapVisualEvidenceTest producing the same bytes. Byte-identical frames across a changed input is the #823 signature -- prove the lever acts before concluding anything from its silence. Neither is investigated here. The other two duplicate pairs ARE expected: _restored == _unfrozen (that is what restoring means) and _frozen == _frozen_without_frustum (the base frozen shot draws no frustum overlay). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes the per-channel colour-mask leak in #853 — and four other live defects found while trying to observe it, none of which the suite could see.
Closes #853The short version
#853 is real and is fixed. But the capture it asked for as "the first thing the fix needs" could never have shown the leak, because no decal had produced a single pixel in any real scene, on either rendering path, for as long as decals have existed. The mask flattening was genuine and is now pinned by tests — it was simply corrupting a draw that never happened.
What was wrong
DeferredOpaqueDecalPassreadsSceneDepth, an attachment view of the same framebuffer as the four colour views it writes; the read fans out onto every sibling and trips four same-pass feedback hazards. The assert on that list is a__debugbreak, so a Debug build with no debugger terminates.afa4eac5dcullFace = Frontkeeps the projection box's back faces;depthFunction = LessOrEqualrejects them, because they are behind the surface the decal projects onto. Both paths read the onePODRenderState, so both were dead. Predates the RHI refactor asGL_LEQUAL+GL_FRONT.bb283315cApplyPODRenderState's globalSetColorMaskis the indexed call for every draw buffer and flattened the mode's channel masks; the narrowing loop after it could only re-disable whole attachments, and a decal packet carries the default0xFF.55a2c687aPlaceholderTexturebuilds its 64×64 magenta/black checkerboard into a local vector, creates the texture, and never uploads it — the comment saying the upload API is unavailable is stale. Every placeholder was an uninitialised texture, sampling as zeros.a582d74af7e43617aeThe fix for #853
PODRenderState::colorAttachmentWriteMaskis one bit per attachment and cannot express "write only RT1.xy". AddedcolorAttachmentChannelMask: one nibble per attachment, R/G/B/A low bit first.This is shape (a) of the two the issue named — the mask travels on the command, not as a pass-scoped "these masks are mine for the next N draws" override. Deliberately, for three reasons:
DrawDecalCommand's own OIT-override comment states the design rule: "keeps the queue stateless and replay-safe."Composition is an AND — global mask, then the nibble, then the attachment bit, never widening past the global call. That makes the old behaviour a strict special case, so
Renderer3DUtilityDraws' skeleton/joint (0x01) and infinite-grid (0xFF & ~(1 << 2)) draws are byte-identical, and an all-writable nibble costs no indexed call at all.DecalGBufferChannelMaskis the single source for the routing —DecalRenderPassdrives itsSetColorMaskForAttachmentcalls from it andRenderer3D::DrawDecalstamps the same value onto the packet, so the two cannot drift. Deferred path only: a forward decal draws into scene colour or the WB-OIT accum/revealage MRT, where masking RT1/RT2 would break compositing outright. The issue did not call this out and it is the one way shape (a) could have silently broken OIT.Evidence
Tenant, failing before the fix (real Vulkan pixels, production dispatch)
RT0.rgb -> 0isDecal_GBuffer_RMA.glsl'sgAlbedo = vec4(0, 0, 0, metallic)reaching an unmasked target — predicted inTEST_SCENES.mdbefore the test was ever run.Contract tests in the same state read
15(0xF, every channel open) where3and7were required.Live G-Buffer probes after the fix (OpenGL,
DecalModeMatrixTest.olo)rgb=albedo, a=metallicxy=normal, z=rough, w=AOrgb=emissive, a=unlit0.549, 0.549, 0.549, 0.00.0, 1.0, 0.8496, 1.00, 0, 0, 01.0, 0.251, 0.149, a=0.0preserved0.3047, 0.2255, zw preserved0.0, 0.0written0.2, 0.8999, 1.0, a=0.0preserved1.0, 0.251, 0.149and0.2, 0.8999, 1.0are the decals' authored colours exactly. Zero render-graph hazards, zero assertions, zero shader errors,ticking: true.Suite: 110/110 across
ChannelMaskTest, the wholeVulkanPassSuite,CommandBucket,FramePipeline,PODCommandand the component-coverage tests.Two things the evidence does not show
One/One, sodst.a + 0leaves the unlit flag alone whether or not the mask survives. The issue lists an Emissive RT2.a leak among the things to capture; it cannot be captured. The tenant corroborates this — its Emissive arm does not fail before the fix.RT0.areads the same inside and outside the footprint, because with no RMA texture bound the metallic sample is 0, which happens to equal the floor's. The table demonstrates RMA's mask (RT0.rgb preserved — the Decal per-channel colour masks are wiped before the draw on both backends (the mode matrix is defeated in production) #853 arm), not that RT0.a is written.Why none of this was caught
VulkanPassSuite.DecalGBufferModeMatrixMasksItsTargetRenderTargetsis a careful 300-line tenant that made three substitutions, and each was hiding a different one of the defects above:FixtureDecalDispatchforCommandDispatch::DrawDecalSetColorMaskPlus: no sandbox scene put a decal on either path, so nothing executed the feature end to end.
Written up as
docs/agent-rules/substituted-seams-compound.md, linked from both indexes. It also records the two green-looking non-results this work produced —PASSED 0 testsfrom a file commented out oftests/CMakeLists.txt, and aRunning 7 testsabort with no[ FAILED ]line at all.Out of scope
AttachmentBlendasymmetry documented inVulkanRendererAPI.his deliberately unchanged, as the issue asks — filed as Vulkan: a per-attachment blend DISABLE cannot override a global blend enable (OITResolve asks for one) #896. This fix's shape does not make it trivially addressable: it is a blend-enable question, not a write-mask one. Confirmed while working here thatOITResolveRenderPassis not affected by the Decal per-channel colour masks are wiped before the draw on both backends (the mode matrix is defeated in production) #853 mechanism either — itsDrawFullscreenTriangleissues an immediatecontext.DrawIndexed, not a command packet, so noApplyPODRenderStateruns and its per-attachment masks are never flattened.GetPath(), so a re-save writesAlbedoTexturePath: ""and drops the reference; through the.scenebinsidecar it is dropped outright. ASpriteRendererComponentin the same session round-tripsAssets/Textures/Otter.pngcorrectly, so it is decal-specific. Root cause not pinned, so not fixed blind.olo_render_why_not_visiblereports "no renderable component" for aDecalComponententity. Logged rather than fixed —OloEditor/src/MCP/McpToolsRender.cppbelongs to the in-flight RHI/Vulkan: diagnostic parity — the inspector has no Vulkan arm, and two MCP tools are still GL-gated #810 branch.GLStateGuard[DecalRenderPass]reports 8–9 escaped state fields at ERROR level on every frame that drains a decal, on both rendering paths, before and after this change — 2723 lines in a couple of minutes. Filed as DecalRenderPass leaks 8-9 GL state fields every frame, reported at error level (2723 lines in minutes) #895. This PR only corrects the stale comment at that call site, which claimedGLStateGuardcannot roll back whenPolicy::Restoredoes exactly that.🤖 Generated with Claude Code