fix(vk): stabilize shadow masks once after drawing #2171

github-sync · 2026-11-11T06:00:47Z

mikaba-eu commented

2026-11-11 06:00:47 +00:00

(Migrated from github.com)

Shadow-mask stabilization can execute before its source draw because assigning an armed temporary RAII guard immediately destroys that temporary; it then executes again when the real guard leaves scope. Arm the existing guard directly so stabilization runs once after the draw, and end dynamic rendering before the history-image barriers. Implemented with GPT-6 Astra medium & xhigh.

The regression test extracts the production guard and checks event order for enabled, disabled, unrelated-program, missing-target, and zero-weight cases. It also verifies that dynamic rendering ends before stabilization barriers and dispatch. The test fails against unchanged upstream and passes with this correction.

This is independent of the opt-in renderer architecture experiments. No visual-quality or FPS improvement is inferred from the source test. The same correction has been exercised in the local combined gameplay build; this isolated branch has not yet had a separate gameplay comparison.

Updated to upstream ddafc1f9. All 360 eligible Linux source checks passed, with 14 documented platform/tool skips, across the initial full suite and resumed/follow-up checks. A one-line display-fixture declaration fixes an upstream Clang 23 unused-state warning without changing runtime behavior. Windows runtime validation was not performed.

The final source includes upstream 2e62a977; its release-only changes passed both affected release/update checks.

Shadow-mask stabilization can execute before its source draw because assigning an armed temporary RAII guard immediately destroys that temporary; it then executes again when the real guard leaves scope. Arm the existing guard directly so stabilization runs once after the draw, and end dynamic rendering before the history-image barriers. Implemented with GPT-6 Astra medium & xhigh. The regression test extracts the production guard and checks event order for enabled, disabled, unrelated-program, missing-target, and zero-weight cases. It also verifies that dynamic rendering ends before stabilization barriers and dispatch. The test fails against unchanged upstream and passes with this correction. This is independent of the opt-in renderer architecture experiments. No visual-quality or FPS improvement is inferred from the source test. The same correction has been exercised in the local combined gameplay build; this isolated branch has not yet had a separate gameplay comparison. Updated to upstream `ddafc1f9`. All **360 eligible Linux source checks passed**, with 14 documented platform/tool skips, across the initial full suite and resumed/follow-up checks. A one-line display-fixture declaration fixes an upstream Clang 23 unused-state warning without changing runtime behavior. Windows runtime validation was not performed. The final source includes upstream `2e62a977`; its release-only changes passed both affected release/update checks.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 16:51:08 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks, this is a real bug and the fix is right. mask_guard = {cmd, ...} builds a temporary MaskGuard, copy-assigns it, and the temporary's destructor runs at the end of that statement with cmd set. So with the blend on, mask_stabilize ran twice per tree-shadow draw: once before the draw (ending whatever pass was open, i.e. the one holding the previous draw), and again after it. Setting the fields directly fixes that, and moving end_pass above the history-image setup takes the first-frame UNDEFINED -> GENERAL barriers out of dynamic rendering, where they were invalid. Both early returns still sit before end_pass, so a skipped blend still leaves the pass open as before.

About #2374: this looks directly relevant to the hang it describes. #2374 says the GPU hangs on the mesh draw b0b3b40244c71ec7, the draw right before 3427a1dbefd5e02c, and that "it may be the mask image's history or barrier rather than the blend itself". That is exactly where the stray temporary ran mask_stabilize: it ended the pass after that mesh draw, recorded the history barriers (inside the pass on the first frame), and ran two compute dispatches on the mask before the shadow pass had drawn it. I can't prove this is the cause from the source alone, but it is the strongest lead so far. @NotRexed, a run of this branch on the castle save with RX_VK_MASK_BLEND=0.85 would tell you if the blend can go back to on by default. The two PRs don't conflict: #2374 only changes mask_weight()'s default and test_tree_shadow_mask.py, and this PR still matters with the default off because the opt-in path had the same bug. test_tree_shadow_mask.py's guard checks (mask_weight(); mw > 0, ~MaskGuard() { if (cmd) mask_stabilize(, end_pass(cmd); in the body) still match the new code.

One side effect: the effective blend changes. Before, the history was advanced twice per frame (once from the previous frame's mask before the draw), so 0.85 was really a stronger smoothing than the shader alone implies. After this fix the streak suppression may look a bit weaker. An in-game before/after screenshot of the sunlit grass would be good.

Small things, none blocking:

The new test has some dead fixture code (see inline). The "disabled" case never calls the guard, and the predicate path is never hit.
The test_wayland_libdecor_hint.py stub is unrelated to this change. It's harmless (hos_display_owns_sdl lives outside hos_display_init, so nothing calls it), but it would fit better in its own PR.
CI: only the commit-title check ran on this head, so the build and run_all jobs still need to run (they probably need a maintainer to approve the workflow).

Approving the code. Whether it's enough to turn the blend back on is a call for @NotRexed after a castle run.

Thanks, this is a real bug and the fix is right. `mask_guard = {cmd, ...}` builds a temporary `MaskGuard`, copy-assigns it, and the temporary's destructor runs at the end of that statement with `cmd` set. So with the blend on, `mask_stabilize` ran twice per tree-shadow draw: once *before* the draw (ending whatever pass was open, i.e. the one holding the previous draw), and again after it. Setting the fields directly fixes that, and moving `end_pass` above the history-image setup takes the first-frame `UNDEFINED -> GENERAL` barriers out of dynamic rendering, where they were invalid. Both early returns still sit before `end_pass`, so a skipped blend still leaves the pass open as before. **About #2374:** this looks directly relevant to the hang it describes. #2374 says the GPU hangs on the mesh draw `b0b3b40244c71ec7`, the draw *right before* `3427a1dbefd5e02c`, and that "it may be the mask image's history or barrier rather than the blend itself". That is exactly where the stray temporary ran `mask_stabilize`: it ended the pass after that mesh draw, recorded the history barriers (inside the pass on the first frame), and ran two compute dispatches on the mask before the shadow pass had drawn it. I can't prove this is *the* cause from the source alone, but it is the strongest lead so far. @NotRexed, a run of this branch on the castle save with `RX_VK_MASK_BLEND=0.85` would tell you if the blend can go back to on by default. The two PRs don't conflict: #2374 only changes `mask_weight()`'s default and `test_tree_shadow_mask.py`, and this PR still matters with the default off because the opt-in path had the same bug. `test_tree_shadow_mask.py`'s guard checks (`mask_weight(); mw > 0`, `~MaskGuard() { if (cmd) mask_stabilize(`, `end_pass(cmd);` in the body) still match the new code. One side effect: the effective blend changes. Before, the history was advanced twice per frame (once from the previous frame's mask before the draw), so 0.85 was really a stronger smoothing than the shader alone implies. After this fix the streak suppression may look a bit weaker. An in-game before/after screenshot of the sunlit grass would be good. Small things, none blocking: - The new test has some dead fixture code (see inline). The "disabled" case never calls the guard, and the predicate path is never hit. - The `test_wayland_libdecor_hint.py` stub is unrelated to this change. It's harmless (`hos_display_owns_sdl` lives outside `hos_display_init`, so nothing calls it), but it would fit better in its own PR. - CI: only the commit-title check ran on this head, so the build and `run_all` jobs still need to run (they probably need a maintainer to approve the workflow). Approving the code. Whether it's enough to turn the blend back on is a call for @NotRexed after a castle run.

gpu/vk/vk_draw.cpp

Right: the old mask_guard = {...} created a temporary whose ~MaskGuard() dispatched immediately, before the draw. Nit: deleting the copy assignment on the struct (MaskGuard& operator=(const MaskGuard&) = delete;) would make the old form fail to compile, so nobody can bring it back by accident. It stays an aggregate, so the fixture in the new test is unaffected.

Right: the old `mask_guard = {...}` created a temporary whose `~MaskGuard()` dispatched immediately, before the draw. Nit: deleting the copy assignment on the struct (`MaskGuard& operator=(const MaskGuard&) = delete;`) would make the old form fail to compile, so nobody can bring it back by accident. It stays an aggregate, so the fixture in the new test is unaffected.

tests/test_shadow_mask_guard.py

					
				@ -0,0 +50,4 @@

				 check(mask,1,0,true,false,{1});

				 check(mask,0,.85f,true,false,{1});

				 check(0,1,.85f,true,false,{1});

				 check(mask,1,.85f,false,false,{});

Nit: check() skips production_draw entirely when enabled and predicate are both false, so this case asserts {} == {} without running the guard. s.render_enabled and draw_predicate_binding aren't used by the extracted code either, so the 3 event can't happen. Could drop these (and the unused <atomic>/logf) to keep the fixture to what the cut-out code actually touches.

Nit: `check()` skips `production_draw` entirely when `enabled` and `predicate` are both false, so this case asserts `{}` == `{}` without running the guard. `s.render_enabled` and `draw_predicate_binding` aren't used by the extracted code either, so the `3` event can't happen. Could drop these (and the unused `<atomic>`/`logf`) to keep the fixture to what the cut-out code actually touches.