Depth prepass: apply the material's alpha cutout - #135
Merged
Conversation
depth_prepass.frag had an empty main by design — the fixed-function depth test does the work — but an alphaMode: MASK material's coverage is not its triangles. The forward shader discards fragments below the material's alphaCutoff and the prepass did not, so it wrote depth across the holes: anything behind a cutout failed the forward pass' LESS_OR_EQUAL test and never shaded, and SSAO — which reconstructs position and normal from this depth alone — occluded as if the cutout were a solid sheet. Long-standing, and visible in the committed acceptance capture, where the cutout quad's holes read uniformly dark instead of showing the floor and sky behind them. The prepass pipeline now opts into the bindless material set (set 2, held at a fixed index by the empty set-1 layout) and carries ForwardPushConstants, and its fragment stage calls the same material.glsl cutout the forward and shadow passes use — one implementation, on the same UVs from the same vertex path, against the same material entry. That identity is the point: a fragment one pass keeps and the other drops leaves either a depth-only occluder or a shaded fragment whose depth nobody wrote. No CPU-side classification was added. The test is gated on the material's packed alphaCutoff, so an opaque draw pays one scalar SSBO read and no texture fetch across a full-screen pass. The shadow path carries a CPU classification because its LOD pin is a CPU decision; the prepass needs none. That gate is only equivalent to running the test while alpha >= 0, and nothing enforced it: Material is a plain value type accepting any float. The divergence was not even limited to cutouts — shader.frag applies `alpha < alphaCutoff` to every material, since a non-MASK one packs cutoff 0, so a negative alpha discarded an OPAQUE surface in the forward pass while the prepass kept it. toMaterialUBO therefore normalises both values: packed alpha into glTF's [0,1], packed cutoff to >= 0. The two clamps differ in kind. The cutoff clamp is behaviour-preserving, since a negative cutoff already discarded nothing. The alpha clamp deliberately CHANGES what an invalid value does — a negative alpha used to discard, and clamped to 0 the fragment is kept — which is glTF-spec normalisation of a value nobody meant, not preservation of it. Sanitisation stays pure and noexcept, with the diagnostic separated from it: toMaterialUBO is reachable from Object::wouldChangeVariant and Mesh::isSelectableVariantState through materialsEquivalent, both noexcept, where a throw out of log formatting or allocation would terminate rather than report. packedAlpha and packedAlphaCutoff therefore do no logging; materialAlphaRangeIssues is the pure decision; and warnOnMaterialAlphaRangeIssues is called from exactly one non-noexcept site — Resources::registerMaterial's deduplicated first-sight branch — so the warning also cannot repeat every frame during variant selection. ForwardPushConstants moves into shaders/forward_push.glsl and joins the guarded blocks, since a second stage now declares it. Verified: 1598 test cases green, including new cases pinning the prepass pipeline's bindless set, push range and shared vertex path; the alpha/cutoff ranges against NaN and both infinities; the reporting decision per field; and the specific MASK/cutoff-0/negative-alpha divergence, which the packed pair can no longer express. Layering and shader-block guards pass, formatting is clean, the render smoke is validation-clean, and Linux clang-tidy reports no findings across 106 files. The clamp fires on no shipped asset (ShadowLodDemo, DamagedHelmet, AlphaBlendModeTest, TransmissionTest all silent), so it is a guard rather than a repair of live data. The SH-03 sweep was re-run because the fix changes what the metric can see: absolute differing-pixel counts are unchanged at every printed digit, while the measured shadowed area moved 12.07% -> 11.68% — localised, by differencing the two runs' area masks, to the cutout quad's own footprint. Budget 1 and ratio 1.0 unchanged. Reference captures regenerated; the out-of-tier finding in codereview.md is marked cleared.
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
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.
depth_prepass.frag had an empty main by design — the fixed-function depth test does the work — but an alphaMode: MASK material's coverage is not its triangles. The forward shader discards fragments below the material's alphaCutoff and the prepass did not, so it wrote depth across the holes: anything behind a cutout failed the forward pass' LESS_OR_EQUAL test and never shaded, and SSAO — which reconstructs position and normal from this depth alone — occluded as if the cutout were a solid sheet. Long-standing, and visible in the committed acceptance capture, where the cutout quad's holes read uniformly dark instead of showing the floor and sky behind them.
The prepass pipeline now opts into the bindless material set (set 2, held at a fixed index by the empty set-1 layout) and carries ForwardPushConstants, and its fragment stage calls the same material.glsl cutout the forward and shadow passes use — one implementation, on the same UVs from the same vertex path, against the same material entry. That identity is the point: a fragment one pass keeps and the other drops leaves either a depth-only occluder or a shaded fragment whose depth nobody wrote.
No CPU-side classification was added. The test is gated on the material's packed alphaCutoff, so an opaque draw pays one scalar SSBO read and no texture fetch across a full-screen pass. The shadow path carries a CPU classification because its LOD pin is a CPU decision; the prepass needs none.
That gate is only equivalent to running the test while alpha >= 0, and nothing enforced it: Material is a plain value type accepting any float. The divergence was not even limited to cutouts — shader.frag applies
alpha < alphaCutoffto every material, since a non-MASK one packs cutoff 0, so a negative alpha discarded an OPAQUE surface in the forward pass while the prepass kept it. toMaterialUBO therefore normalises both values: packed alpha into glTF's [0,1], packed cutoff to >= 0. The two clamps differ in kind. The cutoff clamp is behaviour-preserving, since a negative cutoff already discarded nothing. The alpha clamp deliberately CHANGES what an invalid value does — a negative alpha used to discard, and clamped to 0 the fragment is kept — which is glTF-spec normalisation of a value nobody meant, not preservation of it.Sanitisation stays pure and noexcept, with the diagnostic separated from it: toMaterialUBO is reachable from Object::wouldChangeVariant and Mesh::isSelectableVariantState through materialsEquivalent, both noexcept, where a throw out of log formatting or allocation would terminate rather than report. packedAlpha and packedAlphaCutoff therefore do no logging; materialAlphaRangeIssues is the pure decision; and warnOnMaterialAlphaRangeIssues is called from exactly one non-noexcept site — Resources::registerMaterial's deduplicated first-sight branch — so the warning also cannot repeat every frame during variant selection.
ForwardPushConstants moves into shaders/forward_push.glsl and joins the guarded blocks, since a second stage now declares it.
Verified: 1598 test cases green, including new cases pinning the prepass pipeline's bindless set, push range and shared vertex path; the alpha/cutoff ranges against NaN and both infinities; the reporting decision per field; and the specific MASK/cutoff-0/negative-alpha divergence, which the packed pair can no longer express. Layering and shader-block guards pass, formatting is clean, the render smoke is validation-clean, and Linux clang-tidy reports no findings across 106 files. The clamp fires on no shipped asset (ShadowLodDemo, DamagedHelmet, AlphaBlendModeTest, TransmissionTest all silent), so it is a guard rather than a repair of live data.
The SH-03 sweep was re-run because the fix changes what the metric can see: absolute differing-pixel counts are unchanged at every printed digit, while the measured shadowed area moved 12.07% -> 11.68% — localised, by differencing the two runs' area masks, to the cutout quad's own footprint. Budget 1 and ratio 1.0 unchanged. Reference captures regenerated; the out-of-tier finding in
codereview.md is marked cleared.