MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(vk): mesh fallback grass is left out while its shaders build, not drawn as holes #2382

Merged

thomas-montano merged 2 commits from fix/fallback-depth-hold into main

2026-11-11 22:21:28 +00:00

thomas-montano commented

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

(Migrated from github.com)

Part of #1001

Why it happens

Without mesh shaders, grass and foliage go through the mesh fallback. Its colour programs take long to build (on Intel they spill at 32 lanes), and until they are ready their draws are left out. The depth pass's programs for the same objects are small and already built, so the depth pass still draws the grass. What is left is depth with no colour: the sky behind shows through, and bloom spreads it. Both passes draw from the same culled list (the draw's indirect arguments), e.g. 43ec86f3's colour and 8a8304ed's depth in Kokiri Forest, and d4abbcb4's with 3bde059f's and the shadow map's 6b7f79e2.

What changed

gpu/vk/vk_draw.cpp: a depth-only mesh-fallback draw is left out while the same culled list's colour fallback compute pipeline is compiling, and for up to 120 frames after its last wait. The objects are missing while they build instead of being holes, and they appear once their colour program is ready. Draws whose colour draw ran, programs that write memory, and permanent fallback-record failures are not held.
RX_VK_FALLBACK_DEPTH_HOLD=0 draws them as before.
The key is the indirect-argument address, assumed to stay with one culled list for the hold's lifetime. A colour draw skipped earlier for a pending graphics pipeline returns before this tracking, so that case can still draw depth without colour.

Tests

tests/test_fallback_depth_hold.py: checks the draw roles, all four memory-write cases, the compute-build-only hold, its 120-frame release, and draw()'s order around the fallback's dispatch. It fails on current main.
tests/test_draw_side_effects.py now compiles the shared writes_memory() helper too; tests/test_mesh_fallback.py follows the new fallback-record block.
python -I -B tests/run_all.py fallback_depth_hold draw_side_effects mesh_fallback on the rebased branch: 5 passed on Linux.
Full python -I -B tests/run_all.py: 354 passed, 31 skipped. Of the 13 failures, 12 were unrelated local environment/cache-test failures (missing clang/clang++/zstandard and test_make_game_cache.py); the other was test_mesh_fallback.py's old source anchor, fixed and passing in the focused run above.

Tested in the game

Iris Xe (Raptor Lake-P, i9-13900H, 32 GB), Mesa ANV 26.2.4, Bazzite (KDE Wayland). Kokiri Forest, walking out of Link's house:

Before: every lit surface in the forest specked with sky while the grass built.
After: no holes from the first frame in the forest. 19.8 fps against 17, as the held depth draws are no longer drawn.

That build also had the Intel table-loop change (RX_VK_GM_LOOP, a separate PR to come). Without it the forest's culling programs do not finish building on Intel at all, so the colour draws there never arrive.

Review changes were not rerun in the game. Not tested: other GPUs without mesh shaders (GTX 9xx/10xx, RX 400/500/5000), where the fallback's colour programs build faster and the hold is shorter; Windows.

On a GPU without mesh shaders, the grass in an area you enter for the first time is drawn as holes the sky shows through, blown up into bright specks by the bloom, for as long as the grass's shaders take to build. On an Iris Xe that is minutes per shader and up to an hour for a whole area. Part of #1001 ## Why it happens Without mesh shaders, grass and foliage go through the mesh fallback. Its colour programs take long to build (on Intel they spill at 32 lanes), and until they are ready their draws are left out. The depth pass's programs for the same objects are small and already built, so the depth pass still draws the grass. What is left is depth with no colour: the sky behind shows through, and bloom spreads it. Both passes draw from the same culled list (the draw's indirect arguments), e.g. 43ec86f3's colour and 8a8304ed's depth in Kokiri Forest, and d4abbcb4's with 3bde059f's and the shadow map's 6b7f79e2. ## What changed - `gpu/vk/vk_draw.cpp`: a depth-only mesh-fallback draw is left out while the same culled list's colour fallback compute pipeline is compiling, and for up to 120 frames after its last wait. The objects are missing while they build instead of being holes, and they appear once their colour program is ready. Draws whose colour draw ran, programs that write memory, and permanent fallback-record failures are not held. - `RX_VK_FALLBACK_DEPTH_HOLD=0` draws them as before. - The key is the indirect-argument address, assumed to stay with one culled list for the hold's lifetime. A colour draw skipped earlier for a pending *graphics* pipeline returns before this tracking, so that case can still draw depth without colour. ## Tests - `tests/test_fallback_depth_hold.py`: checks the draw roles, all four memory-write cases, the compute-build-only hold, its 120-frame release, and `draw()`'s order around the fallback's dispatch. It fails on current `main`. - `tests/test_draw_side_effects.py` now compiles the shared `writes_memory()` helper too; `tests/test_mesh_fallback.py` follows the new fallback-record block. - `python -I -B tests/run_all.py fallback_depth_hold draw_side_effects mesh_fallback` on the rebased branch: 5 passed on Linux. - Full `python -I -B tests/run_all.py`: 354 passed, 31 skipped. Of the 13 failures, 12 were unrelated local environment/cache-test failures (missing clang/clang++/zstandard and `test_make_game_cache.py`); the other was `test_mesh_fallback.py`'s old source anchor, fixed and passing in the focused run above. ## Tested in the game Iris Xe (Raptor Lake-P, i9-13900H, 32 GB), Mesa ANV 26.2.4, Bazzite (KDE Wayland). Kokiri Forest, walking out of Link's house: - Before: every lit surface in the forest specked with sky while the grass built. - After: no holes from the first frame in the forest. 19.8 fps against 17, as the held depth draws are no longer drawn. That build also had the Intel table-loop change (`RX_VK_GM_LOOP`, a separate PR to come). Without it the forest's culling programs do not finish building on Intel at all, so the colour draws there never arrive. **Review changes were not rerun in the game.** Not tested: other GPUs without mesh shaders (GTX 9xx/10xx, RX 400/500/5000), where the fallback's colour programs build faster and the hold is shorter; Windows. <img width="984" height="308" alt="image" src="https://github.com/user-attachments/assets/2728e282-b08d-449c-b69e-b2aba046adca" />

❤️ 1

hckrman101 (Migrated from github.com) reviewed

2026-11-11 17:35:43 +00:00

hckrman101 (Migrated from github.com) left a comment

The new skip doesn't exclude memory-writing draws. Since #1573, a draw is only left out while its pipeline builds when none of its stages store to global memory or write storage buffers/images (the may_skip loop above). The new SKIP_HERE() for held depth-only draws skips the whole draw, including the fallback's task/mesh stages, without that check. If a held depth draw's program writes memory, those writes are lost while it is held. Please pull the #1573 check into a small helper (e.g. writes_memory(const Program&)) and return FallbackRole::None for programs that write memory, with a case in the test.
The hold also triggers when the colour draw is left out for good, not just while it builds. fallback_record() also returns false for a count-buffer draw (issue 640), descriptors of another program, a records budget over 128 MB, an indirect draw with no converted arguments, and a compute build that failed (compute_state == 2, fallback_compute empty). In those cases the colour draw is retried every frame, so its depth draw is now hidden for good where before it was drawn. That may look better than holes, but it differs from what the description says ("they appear once their colour program is ready"). I'd only mark the list as held when the colour program is still compiling (p.compute_state != 2 after fallback_record fails), or else say in the description that permanent failures hide the depth too.
Gap (not blocking): when the colour draw's graphics pipeline is the one still building, draw() returns at if (may_skip) return void(st_pending_skips++); before it gets to the role code, so the list isn't marked and the depth still draws as holes. On ANV the compute build is probably the slow one, so this may not matter in practice. Worth a sentence in the description if so.
Nit: the key is the indirect-argument address. If the game reuses the same args address for another object's list, an unrelated depth-only fallback draw can be held for up to 120 frames (about 7 s at your 17-20 fps). Probably rare. A comment saying the address is assumed to belong to one list would help.

Overlaps: #1573 (merged) owns the pending-pipeline skip rule this should follow (point 1). #1946 changes how a waiting draw gets its pipeline, not fallback_pipelines_ready, so the two shouldn't conflict textually, but if it lands the graphics-side gap in point 3 shrinks. #2362 (RADV mesh shaders off by default on Linux AMD) and #2215 (more Navi 21 boards on the task + mesh fallback) send more GPUs down this path, so the hold will affect AMD too, and only Iris Xe has been tested. A run on one of those GPUs would help.

CI: only the title check has been queued so far. The build and test workflows haven't run on this head yet.

Thanks, the diagnosis (depth from the same culled list drawn while its colour is still building) is clear, and holding the depth draw is a sensible way to turn holes into missing grass. The test is good: it checks the roles, the 120-frame release and the order in `draw()`. A few things before this goes in: 1. **The new skip doesn't exclude memory-writing draws.** Since #1573, a draw is only left out while its pipeline builds when none of its stages store to global memory or write storage buffers/images (the `may_skip` loop above). The new `SKIP_HERE()` for held depth-only draws skips the whole draw, including the fallback's task/mesh stages, without that check. If a held depth draw's program writes memory, those writes are lost while it is held. Please pull the #1573 check into a small helper (e.g. `writes_memory(const Program&)`) and return `FallbackRole::None` for programs that write memory, with a case in the test. 2. **The hold also triggers when the colour draw is left out for good, not just while it builds.** `fallback_record()` also returns false for a count-buffer draw (issue 640), descriptors of another program, a records budget over 128 MB, an indirect draw with no converted arguments, and a compute build that failed (`compute_state == 2`, `fallback_compute` empty). In those cases the colour draw is retried every frame, so its depth draw is now hidden for good where before it was drawn. That may look better than holes, but it differs from what the description says ("they appear once their colour program is ready"). I'd only mark the list as held when the colour program is still compiling (`p.compute_state != 2` after `fallback_record` fails), or else say in the description that permanent failures hide the depth too. 3. Gap (not blocking): when the colour draw's *graphics* pipeline is the one still building, `draw()` returns at `if (may_skip) return void(st_pending_skips++);` before it gets to the role code, so the list isn't marked and the depth still draws as holes. On ANV the compute build is probably the slow one, so this may not matter in practice. Worth a sentence in the description if so. 4. Nit: the key is the indirect-argument address. If the game reuses the same args address for another object's list, an unrelated depth-only fallback draw can be held for up to 120 frames (about 7 s at your 17-20 fps). Probably rare. A comment saying the address is assumed to belong to one list would help. Overlaps: #1573 (merged) owns the pending-pipeline skip rule this should follow (point 1). #1946 changes how a waiting draw gets its pipeline, not `fallback_pipelines_ready`, so the two shouldn't conflict textually, but if it lands the graphics-side gap in point 3 shrinks. #2362 (RADV mesh shaders off by default on Linux AMD) and #2215 (more Navi 21 boards on the task + mesh fallback) send more GPUs down this path, so the hold will affect AMD too, and only Iris Xe has been tested. A run on one of those GPUs would help. CI: only the title check has been queued so far. The build and test workflows haven't run on this head yet.

gpu/vk/vk_draw.cpp

If fallback_record then fails for a permanent reason (count buffer, failed build, records budget, no converted args), the list stays held for good and its depth draw never comes back. Marking it held only while p.compute_state isn't 2 would limit the hold to the build wait.

If `fallback_record` then fails for a permanent reason (count buffer, failed build, records budget, no converted args), the list stays held for good and its depth draw never comes back. Marking it held only while `p.compute_state` isn't 2 would limit the hold to the build wait.

gpu/vk/vk_draw.cpp

This skips the whole draw, including the fallback's compute stages, without the memory-write check that #1573 requires for every other draw left out while building. Could fallback_role_of return None when any stage stores global memory or writes storage buffers/images?

This skips the whole draw, including the fallback's compute stages, without the memory-write check that #1573 requires for every other draw left out while building. Could `fallback_role_of` return `None` when any stage stores global memory or writes storage buffers/images?

thomas-montano (Migrated from github.com) reviewed

2026-11-11 18:23:39 +00:00

gpu/vk/vk_draw.cpp

Done. writes_memory() now owns the #1573 check and is reused by the pending graphics-pipeline skip; fallback_role_of() returns None for those programs. The fixture covers global stores and written storage buffers, image buffers, and images.

Done. `writes_memory()` now owns the #1573 check and is reused by the pending graphics-pipeline skip; `fallback_role_of()` returns `None` for those programs. The fixture covers global stores and written storage buffers, image buffers, and images.

thomas-montano (Migrated from github.com) reviewed

2026-11-11 18:23:41 +00:00

gpu/vk/vk_draw.cpp

Done. A failed colour record marks the list only when compute_state == 1. That also excludes permanent failures which never start a build (0) and completed or failed builds (2); any earlier hold still expires after 120 frames. Rebased onto current main.

Done. A failed colour record marks the list only when `compute_state == 1`. That also excludes permanent failures which never start a build (`0`) and completed or failed builds (`2`); any earlier hold still expires after 120 frames. Rebased onto current `main`.

Joey-Magnus (Migrated from github.com) reviewed

2026-11-11 21:25:12 +00:00

Joey-Magnus (Migrated from github.com) left a comment

All four earlier points are handled (writes_memory() with tests for all four write kinds, the hold only while compute_state == 1, the graphics-build gap documented, the address-reuse comment). The red Windows part 2 ran on a base before #2397, so updating the branch should clear it.

The one open question for the maintainers: the hold is on by default for every GPU on the mesh fallback, but it's only tested on an Iris Xe. #2362 and #2215 send more AMD cards down this path. RX_VK_FALLBACK_DEPTH_HOLD=0 turns it off.

All four earlier points are handled (`writes_memory()` with tests for all four write kinds, the hold only while `compute_state == 1`, the graphics-build gap documented, the address-reuse comment). The red Windows part 2 ran on a base before #2397, so updating the branch should clear it. The one open question for the maintainers: the hold is on by default for every GPU on the mesh fallback, but it's only tested on an Iris Xe. #2362 and #2215 send more AMD cards down this path. `RX_VK_FALLBACK_DEPTH_HOLD=0` turns it off.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 22:18:04 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for the quick turnaround. All four points from my last review are fixed:

writes_memory() now holds the #1573 check, the pending-pipeline skip uses it, and fallback_role_of() returns None for memory-writing programs. The test covers all four write kinds.
The list is only marked held when compute_state == 1. I checked fallback_record(): the count-buffer and other-descriptors cases return before a build starts, and a finished or failed build is 2. So a permanent failure can only extend a hold that a real build started, and that hold still runs out after 120 frames.
The graphics-pipeline gap is now in the description.
The comment on the address-reuse assumption is there.

CI is green on this head, including Windows part 2, which Joey-Magnus saw red before. The branch merges cleanly.

One nit inline. Joey-Magnus's question is still open: the hold is on by default for every GPU on the mesh fallback, but it has only been tested on an Iris Xe. Whether to ship it on by default is up to the maintainer. The review changes only narrow the hold, so the Iris Xe result should still hold, though it hasn't been rerun in the game.

Thanks for the quick turnaround. All four points from my last review are fixed: 1. `writes_memory()` now holds the #1573 check, the pending-pipeline skip uses it, and `fallback_role_of()` returns `None` for memory-writing programs. The test covers all four write kinds. 2. The list is only marked held when `compute_state == 1`. I checked `fallback_record()`: the count-buffer and other-descriptors cases return before a build starts, and a finished or failed build is 2. So a permanent failure can only extend a hold that a real build started, and that hold still runs out after 120 frames. 3. The graphics-pipeline gap is now in the description. 4. The comment on the address-reuse assumption is there. CI is green on this head, including Windows part 2, which Joey-Magnus saw red before. The branch merges cleanly. One nit inline. Joey-Magnus's question is still open: the hold is on by default for every GPU on the mesh fallback, but it has only been tested on an Iris Xe. Whether to ship it on by default is up to the maintainer. The review changes only narrow the hold, so the Iris Xe result should still hold, though it hasn't been rerun in the game.

gpu/vk/vk_draw.cpp

Nit: every recorded colour fallback draw now takes the global fallback_held_mu to erase a key that is usually absent, and recording threads can contend on it. An if (fallback_held.empty()) check is cheap, but it needs the lock too, or a relaxed atomic count beside the map, so this is only worth doing if it shows up in a profile.

Nit: every recorded colour fallback draw now takes the global `fallback_held_mu` to erase a key that is usually absent, and recording threads can contend on it. An `if (fallback_held.empty())` check is cheap, but it needs the lock too, or a relaxed atomic count beside the map, so this is only worth doing if it shows up in a profile.