fix(shaders): noperspective varyings interpolated right on AMD (Royal Family Tomb water); sirit names an undefined operand #1830

Merged

FIocker merged 2 commits from super-build/shaders into main 2026-11-12 03:51:15 +00:00

Conversation 7 Commits 2 Files changed 8 +129 -9

FIocker commented 2026-11-10 17:46:27 +00:00 (Migrated from github.com)

• Copy link
• Copy Markdown

On AMD, the water in the Royal Family Tomb shows a red dotted line. Separately, when sirit meets an undefined operand it aborts the whole game without logging why. This PR fixes both. It's part of the "super build" series, but it builds and passes the tests on its own, on current main.

Part	Branch	Builds
Shader fixes (this PR)	super-build/shaders (#1830)	on its own
Upscaling and frame generation	super-build/upscaling-framegen (#1831)	only with #1833 and #1834
DLSS 5 neural rendering	super-build/neural (#1833)	only with #1831 and #1834
Menu, renderer and build plumbing	super-build/core (#1834)	only with #1831 and #1833

What changed since the first review

The branch is rebuilt on main at d7a73062. Main now has its own versions of most of what this PR used to carry, so those parts are dropped:

• The R2UR span (#1806).
• The bit-32 immediate and compute local memory (#1800).
• LDG/STG bit 78 with its MEMBAR.GL fences (#1778).
• The closing self-branch (#1810).
• SULD.1D, LEA.HI with a constant base, and ISBERD.O.

lower_to_maxwell.h and its tests are now exactly main's. Also dropped:

• on_big_stack and the RX_VK_TRACE_KEYS tracing, with its process-wide set_terminate.
• The vk_texture.cpp changes (memory_guest_contained, the unswizzle threshold, texture_touch_misses). They moved to #1834, which describes them.
• The test_shader_stamp_cache.py change, which also moved to #1834.

What it does

• Noperspective varyings on AMD (vk_shader.cpp; Eden's SPIR-V back end: runtime_info.h, spirv_emit_context.*, emit_spirv_special.cpp, emit_spirv_context_get_set.cpp).
  • The bug: the Royal Family Tomb's water writes clip.xy/clip.w as a noperspective varying. When a vertex with w close to zero is clipped, AMD interpolates the wrong depth, and its exp2 overflows into a red dotted line across the water.
  • The fix: the vertex stage writes those outputs multiplied by Position.w and lets them interpolate perspective-correct; the fragment stage multiplies by FragCoord.w to get the screen-linear value back. A geometry stage restores the unscaled values after EmitVertex.
  • Where it applies: every VS+FS pipeline whose fragment shader has a noperspective input, on every vendor. The result is the same value, interpolated a different way.
  • Where it doesn't: it's skipped when the fragment shader reads inputs by index (the multiply can't be undone there), when transform feedback is on (it would capture the scaled value), and for mesh pipelines.
• sirit: an operand with no SPIR-V definition (a null Id) now throws instead of std::abort(). EmitSPIRV's catch names the IR op and its arguments, and the program is reported as not translated instead of ending the game with nothing in the log.
• RX_VK_LOWER_SKIP=<key prefix,...>: leaves those programs unlowered, for bisecting. It's a diagnostic and does nothing unless set.

Testing

• Builds: the changed files compile on this branch alone with clang and nx2/build.py's renderer flags.
• Tests: tests/run_all.py on this branch, locally (clang targeting MSVC, the CPM Vulkan headers, glslang 11.12), gives 324 passed. The 18 failures are the same 18 main at d7a73062 has on this machine, all down to this PC's toolchain, so there are no new failures.
• In game: Windows 11, NVIDIA RTX, OoT v131072, as part of the combined build: no pipeline changes visible on NVIDIA. The AMD fix is from the fork's AMD testing; I have no AMD GPU to confirm it here.

🤖 Generated with Claude Code

On AMD, the water in the Royal Family Tomb shows a red dotted line. Separately, when sirit meets an undefined operand it aborts the whole game without logging why. This PR fixes both. It's part of the "super build" series, but **it builds and passes the tests on its own**, on current main. | Part | Branch | Builds | |---|---|---| | Shader fixes (this PR) | `super-build/shaders` (#1830) | **on its own** | | Upscaling and frame generation | `super-build/upscaling-framegen` (#1831) | only with #1833 and #1834 | | DLSS 5 neural rendering | `super-build/neural` (#1833) | only with #1831 and #1834 | | Menu, renderer and build plumbing | `super-build/core` (#1834) | only with #1831 and #1833 | ## What changed since the first review The branch is rebuilt on main at d7a73062. Main now has its own versions of most of what this PR used to carry, so those parts are dropped: - The R2UR span (#1806). - The bit-32 immediate and compute local memory (#1800). - LDG/STG bit 78 with its `MEMBAR.GL` fences (#1778). - The closing self-branch (#1810). - SULD.1D, LEA.HI with a constant base, and ISBERD.O. `lower_to_maxwell.h` and its tests are now exactly main's. Also dropped: - `on_big_stack` and the `RX_VK_TRACE_KEYS` tracing, with its process-wide `set_terminate`. - The `vk_texture.cpp` changes (`memory_guest_contained`, the unswizzle threshold, `texture_touch_misses`). They moved to #1834, which describes them. - The `test_shader_stamp_cache.py` change, which also moved to #1834. ## What it does - **Noperspective varyings on AMD** (`vk_shader.cpp`; Eden's SPIR-V back end: `runtime_info.h`, `spirv_emit_context.*`, `emit_spirv_special.cpp`, `emit_spirv_context_get_set.cpp`). - **The bug**: the Royal Family Tomb's water writes clip.xy/clip.w as a `noperspective` varying. When a vertex with w close to zero is clipped, AMD interpolates the wrong depth, and its `exp2` overflows into a red dotted line across the water. - **The fix**: the vertex stage writes those outputs multiplied by Position.w and lets them interpolate perspective-correct; the fragment stage multiplies by FragCoord.w to get the screen-linear value back. A geometry stage restores the unscaled values after `EmitVertex`. - **Where it applies**: every VS+FS pipeline whose fragment shader has a `noperspective` input, on every vendor. The result is the same value, interpolated a different way. - **Where it doesn't**: it's skipped when the fragment shader reads inputs by index (the multiply can't be undone there), when transform feedback is on (it would capture the scaled value), and for mesh pipelines. - **sirit**: an operand with no SPIR-V definition (a null `Id`) now throws instead of `std::abort()`. `EmitSPIRV`'s catch names the IR op and its arguments, and the program is reported as not translated instead of ending the game with nothing in the log. - **`RX_VK_LOWER_SKIP=<key prefix,...>`**: leaves those programs unlowered, for bisecting. It's a diagnostic and does nothing unless set. ## Testing - **Builds**: the changed files compile on this branch alone with clang and `nx2/build.py`'s renderer flags. - **Tests**: `tests/run_all.py` on this branch, locally (clang targeting MSVC, the CPM Vulkan headers, glslang 11.12), gives 324 passed. The 18 failures are the same 18 main at d7a73062 has on this machine, all down to this PC's toolchain, so there are no new failures. - **In game**: Windows 11, NVIDIA RTX, OoT v131072, as part of the combined build: no pipeline changes visible on NVIDIA. The AMD fix is from the fork's AMD testing; I have no AMD GPU to confirm it here. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hckrman101 (Migrated from github.com) reviewed 2026-11-10 18:06:07 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for splitting the super build up. Several of these fixes are real, but this branch doesn't stand on its own yet, and about half of the lowering changes overlap with open or merged PRs in the same file.

Blocking

• Build break: upload_unswizzle_gpu calls memory_guest_contained(), but nothing on this branch declares or defines it. It's declared in vk_internal.h on super-build/all, which isn't part of this PR (see inline).
• Three registered tests fail as written. run_all.py picks up every tests/test_*.py. These are probably the "tests that need a compiler" that didn't run on your machine:
  • test_lower_suld_1d.py expects a 1D coordinate in R254 to lower, and 1D_BUFFER / 1D_ARRAY to be refused. Both checks now go the other way.
  • test_lower_lea_hi_constant.py expects a high source (Rc not RZ) and a zero shift to be refused. The new LEA.HI block accepts both.
  • test_shader_stamp_cache.py now asserts that nx2/build.py has streamline_sdk_path and opennr_jobs. Neither is on this branch; they come from #1833/#1834.
• No CI has run on this branch. It's 26 commits behind main.

Overlap with other PRs (please rebase and drop the duplicates):

• R2UR span: #1806 is merged (9edb3a5) with the same FieldSpan line, so drop this copy. Main has it after the extended_renaming check, and yours comes before it, so git may keep both.
• Immediate at bit 32 + compute local memory: #1800 (approved) fixes both. Its bit-32 skip is narrowed to form 4 of the ALU families, and it sanity-checks +0x68 against +0x50 (≤64 KB). The !is_register[f] skip here is the broad version. It drops the bit-32 registers of TLD4 0xb63/0xf63, TMML 0xb69/0xf69, TEX 0xf60, SHFL.IDX 0x589 and STL 0x987, none of which is in the register_field list. Their Rb then stops getting its REGMAP prefix whenever another field is high. That's a silent miscompile of programs that lower fine today. Please keep #1800's version and drop these hunks.
• LDG/STG bit 78: #1778 widens the same masks and brackets the access with MEMBAR.GL when bit 78 is set. Bits 77-79 all set is .STRONG.GPU, as on the RED/ATOMG cases in this file. Here the qualifier is just dropped. The fences need to stay, so please take #1778's (approved) version. The LDG mask here also lets bit 78 through without bit 77.
• Closing self-branch: #1810 changes the same line. Your version (no pending forward branch or call target) is more general than #1810's "previous instruction is an unconditional BRA/EXIT", and I think it's sound: an unconditional self-branch never falls through, and finish re-checks the relocations. Only one of them should land, though. Two small differences from the EXIT path: it doesn't check barrier_tokens, and it allows a call target ≤ index before main_exited, which the EXIT path refuses. It also has no test.

Lowering details

• SULD: main already lowers SULD.P.1D, which covers those 17 programs. This hunk adds 1D_BUFFER and 1D_ARRAY, which main's comment says OoT never uses, and there's no test for them. It also gets the coordinate limits wrong. 1D and 1D_BUFFER need one register, so R254 should stay allowed (this is the regression the test catches). 1D_ARRAY needs two (x, layer), so its limit is 253, not 252.
• LEA.HI constant base: the Rc/.X decoding mirrors the 0x211 register form, so the new paths look right. But main's mask also required bits 84-86 and 91-95 to be zero, and this check no longer looks at them, so unknown qualifiers there now pass silently. Please keep the mask, widened only by bit 74 and Rc. Also update the test for what is now accepted. Note that main's comment already says the 40 no-carry programs lower.
• ISBERD.O: the lowering and frontend look fine. Nit: IsbeReadOutput ignores .32 (bit 44), so a U8 read would return the whole word. PayloadWord already clamps, so the extra Clamped in the task branch is redundant.

Not in the description (worth their own PR, or at least a mention):

• The noperspective-varying rewrite (RuntimeInfo::noperspective_outputs, the vertex outputs scaled by Position.w, fragment inputs multiplied by FragCoord.w). The math checks out, but it changes every VS+FS pipeline with a noperspective input on every vendor, not only AMD.
• upload_unswizzle_gpu no longer imports a pool by default, and the multithreaded CPU unswizzle threshold drops from 16 MB to 4 MB. Both are behaviour and performance changes.
• texture_touch_misses() has no caller on this branch.

On "70 programs": per the description, that number comes from the in-game programs (0 failed) log on the combined super-build/all branch, from one Kokiri Forest run on NVIDIA. It wasn't measured on this branch against current main. Main already lowers the 40 LEA.HI and 17 SULD.1D programs, and the R2UR one is merged too. It would help to say which programs still fail on main and which hunk fixes each one.

Nits: on_big_stack is described as a precaution that turned out not to be needed, and it's Windows-only. Its trace path calls std::set_terminate from a worker thread, which replaces the handler for the whole process. The LOWER_DEBUG block and the RX_VK_LOWER_SKIP / RX_VK_TRACE_KEYS hooks are fine as diagnostics.

Thanks for splitting the super build up. Several of these fixes are real, but this branch doesn't stand on its own yet, and about half of the lowering changes overlap with open or merged PRs in the same file. **Blocking** - **Build break:** `upload_unswizzle_gpu` calls `memory_guest_contained()`, but nothing on this branch declares or defines it. It's declared in `vk_internal.h` on `super-build/all`, which isn't part of this PR (see inline). - **Three registered tests fail as written.** `run_all.py` picks up every `tests/test_*.py`. These are probably the "tests that need a compiler" that didn't run on your machine: - `test_lower_suld_1d.py` expects a 1D coordinate in R254 to lower, and 1D_BUFFER / 1D_ARRAY to be refused. Both checks now go the other way. - `test_lower_lea_hi_constant.py` expects a high source (Rc not RZ) and a zero shift to be refused. The new LEA.HI block accepts both. - `test_shader_stamp_cache.py` now asserts that `nx2/build.py` has `streamline_sdk_path` and `opennr_jobs`. Neither is on this branch; they come from #1833/#1834. - No CI has run on this branch. It's 26 commits behind main. **Overlap with other PRs** (please rebase and drop the duplicates): - **R2UR span:** #1806 is merged (9edb3a5) with the same `FieldSpan` line, so drop this copy. Main has it after the `extended_renaming` check, and yours comes before it, so git may keep both. - **Immediate at bit 32 + compute local memory:** #1800 (approved) fixes both. Its bit-32 skip is narrowed to form 4 of the ALU families, and it sanity-checks +0x68 against +0x50 (≤64 KB). The `!is_register[f]` skip here is the broad version. It drops the bit-32 registers of TLD4 0xb63/0xf63, TMML 0xb69/0xf69, TEX 0xf60, SHFL.IDX 0x589 and STL 0x987, none of which is in the `register_field` list. Their Rb then stops getting its REGMAP prefix whenever another field is high. That's a silent miscompile of programs that lower fine today. Please keep #1800's version and drop these hunks. - **LDG/STG bit 78:** #1778 widens the same masks and brackets the access with `MEMBAR.GL` when bit 78 is set. Bits 77-79 all set is `.STRONG.GPU`, as on the RED/ATOMG cases in this file. Here the qualifier is just dropped. The fences need to stay, so please take #1778's (approved) version. The LDG mask here also lets bit 78 through without bit 77. - **Closing self-branch:** #1810 changes the same line. Your version (no pending forward branch or call target) is more general than #1810's "previous instruction is an unconditional BRA/EXIT", and I think it's sound: an unconditional self-branch never falls through, and `finish` re-checks the relocations. Only one of them should land, though. Two small differences from the EXIT path: it doesn't check `barrier_tokens`, and it allows a call target ≤ index before `main_exited`, which the EXIT path refuses. It also has no test. **Lowering details** - **SULD:** main already lowers SULD.P.1D, which covers those 17 programs. This hunk adds 1D_BUFFER and 1D_ARRAY, which main's comment says OoT never uses, and there's no test for them. It also gets the coordinate limits wrong. 1D and 1D_BUFFER need one register, so R254 should stay allowed (this is the regression the test catches). 1D_ARRAY needs two (x, layer), so its limit is 253, not 252. - **LEA.HI constant base:** the Rc/.X decoding mirrors the 0x211 register form, so the new paths look right. But main's mask also required bits 84-86 and 91-95 to be zero, and this check no longer looks at them, so unknown qualifiers there now pass silently. Please keep the mask, widened only by bit 74 and Rc. Also update the test for what is now accepted. Note that main's comment already says the 40 no-carry programs lower. - **ISBERD.O:** the lowering and frontend look fine. Nit: `IsbeReadOutput` ignores `.32` (bit 44), so a U8 read would return the whole word. `PayloadWord` already clamps, so the extra `Clamped` in the task branch is redundant. **Not in the description** (worth their own PR, or at least a mention): - The noperspective-varying rewrite (`RuntimeInfo::noperspective_outputs`, the vertex outputs scaled by Position.w, fragment inputs multiplied by FragCoord.w). The math checks out, but it changes every VS+FS pipeline with a noperspective input on every vendor, not only AMD. - `upload_unswizzle_gpu` no longer imports a pool by default, and the multithreaded CPU unswizzle threshold drops from 16 MB to 4 MB. Both are behaviour and performance changes. - `texture_touch_misses()` has no caller on this branch. **On "70 programs":** per the description, that number comes from the in-game `programs (0 failed)` log on the combined `super-build/all` branch, from one Kokiri Forest run on NVIDIA. It wasn't measured on this branch against current main. Main already lowers the 40 LEA.HI and 17 SULD.1D programs, and the R2UR one is merged too. It would help to say which programs still fail on main and which hunk fixes each one. Nits: `on_big_stack` is described as a precaution that turned out not to be needed, and it's Windows-only. Its trace path calls `std::set_terminate` from a worker thread, which replaces the handler for the whole process. The `LOWER_DEBUG` block and the `RX_VK_LOWER_SKIP` / `RX_VK_TRACE_KEYS` hooks are fine as diagnostics.

gpu/vk/eden/shader_recompiler/frontend/ampere/lower_to_maxwell.h

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:07 +00:00

Duplicate of #1806, which is now merged. Main has this line after the extended_renaming check, so please drop this one when you rebase.

Duplicate of #1806, which is now merged. Main has this line after the `extended_renaming` check, so please drop this one when you rebase.

gpu/vk/eden/shader_recompiler/frontend/ampere/lower_to_maxwell.h

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:07 +00:00

This skips every bit-32 field that isn't in the register_field list, and that list misses opcodes whose lowering reads a register at 32: TLD4 0xb63/0xf63, TMML 0xb69/0xf69, TEX 0xf60, SHFL.IDX 0x589 and STL 0x987. Before, those still got marked whenever another field set any_high. Now their Rb loses the REGMAP prefix. #1800 (approved) skips only bit 32 of form 4 of the ALU families, the actual immediate. Please keep that version and drop this one.

This skips every bit-32 field that isn't in the `register_field` list, and that list misses opcodes whose lowering reads a register at 32: TLD4 0xb63/0xf63, TMML 0xb69/0xf69, TEX 0xf60, SHFL.IDX 0x589 and STL 0x987. Before, those still got marked whenever another field set `any_high`. Now their Rb loses the REGMAP prefix. #1800 (approved) skips only bit 32 of form 4 of the ALU families, the actual immediate. Please keep that version and drop this one.

gpu/vk/eden/shader_recompiler/frontend/ampere/lower_to_maxwell.h

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:07 +00:00

1D and 1D_BUFFER read one coordinate register, so R254 should stay allowed. Main allows it and test_lower_suld_1d.py checks for it. 1D_ARRAY reads two (x, layer), so its limit is 253. Accepting 1D_BUFFER and 1D_ARRAY also flips that test's refusal checks, and OoT doesn't seem to use either. Maybe leave them refused until a program needs them.

1D and 1D_BUFFER read one coordinate register, so R254 should stay allowed. Main allows it and `test_lower_suld_1d.py` checks for it. 1D_ARRAY reads two (x, layer), so its limit is 253. Accepting 1D_BUFFER and 1D_ARRAY also flips that test's refusal checks, and OoT doesn't seem to use either. Maybe leave them refused until a program needs them.

gpu/vk/eden/shader_recompiler/frontend/ampere/lower_to_maxwell.h

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:07 +00:00

Bits 77-79 all set is .STRONG.GPU (as on RED/ATOMG here), so dropping it loses the ordering. #1778 covers the same mask and fences the access with MEMBAR.GL when bit 78 is set, so I'd keep that version. This mask also lets 78 through without 77.

Bits 77-79 all set is `.STRONG.GPU` (as on RED/ATOMG here), so dropping it loses the ordering. #1778 covers the same mask and fences the access with `MEMBAR.GL` when bit 78 is set, so I'd keep that version. This mask also lets 78 through without 77.

gpu/vk/eden/shader_recompiler/frontend/ampere/lower_to_maxwell.h

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:07 +00:00

Main's mask also required bits 84-86 and 91-95 to be zero. This check doesn't test them, so an unknown qualifier there now passes silently. Could you keep the old mask, widened only by bit 74 (.X) and Rc? Also, test_lower_lea_hi_constant.py expects Rc≠RZ and a zero shift to be refused, so it fails now and needs updating.

Main's mask also required bits 84-86 and 91-95 to be zero. This check doesn't test them, so an unknown qualifier there now passes silently. Could you keep the old mask, widened only by bit 74 (.X) and Rc? Also, `test_lower_lea_hi_constant.py` expects Rc≠RZ and a zero shift to be refused, so it fails now and needs updating.

gpu/vk/eden/shader_recompiler/frontend/ampere/lower_to_maxwell.h

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:08 +00:00

#1810 changes this same spot with a narrower rule (the previous instruction must be an unconditional BRA/EXIT). I think yours is sound, but only one should land. Small gaps compared with the EXIT path: no barrier_tokens check, and call targets ≤ index are allowed before main_exited. A test would help.

#1810 changes this same spot with a narrower rule (the previous instruction must be an unconditional BRA/EXIT). I think yours is sound, but only one should land. Small gaps compared with the EXIT path: no `barrier_tokens` check, and call targets ≤ index are allowed before `main_exited`. A test would help.

gpu/vk/vk_shader.cpp

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:08 +00:00

#1800 reads this field too, and cross-checks it against +0x50 (== 32 × +0x68) with a 64 KB cap and a log line. A 1 MB cap with no check is much looser, so please take #1800's version.

#1800 reads this field too, and cross-checks it against +0x50 (== 32 × +0x68) with a 64 KB cap and a log line. A 1 MB cap with no check is much looser, so please take #1800's version.

gpu/vk/vk_shader.cpp

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:08 +00:00

This noperspective rewrite isn't mentioned in the description. It affects every VS+FS pipeline with a noperspective input on every vendor. Worth its own PR, or at least a line in the description.

This noperspective rewrite isn't mentioned in the description. It affects every VS+FS pipeline with a noperspective input on every vendor. Worth its own PR, or at least a line in the description.

gpu/vk/vk_texture.cpp

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:07 +00:00

memory_guest_contained isn't declared or defined anywhere on this branch (it's in vk_internal.h / vk_memory.cpp on super-build/all), so vk_texture.cpp won't compile on its own.

`memory_guest_contained` isn't declared or defined anywhere on this branch (it's in `vk_internal.h` / `vk_memory.cpp` on `super-build/all`), so `vk_texture.cpp` won't compile on its own.

tests/test_shader_stamp_cache.py

hckrman101 (Migrated from github.com) commented 2026-11-10 18:06:07 +00:00

nx2/build.py on this branch has neither streamline_sdk_path nor opennr_jobs, so this assert fails. This change belongs with #1833/#1834.

`nx2/build.py` on this branch has neither `streamline_sdk_path` nor `opennr_jobs`, so this assert fails. This change belongs with #1833/#1834.

FIocker commented 2026-11-11 10:45:29 +00:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Thanks for the thorough review; you were right on all counts. I rebuilt the branch on main at d7a73062, and it now builds and tests on its own. The description is rewritten.

Blocking:

• memory_guest_contained: the vk_texture.cpp changes moved to #1834, together with the function and its description. Nothing on this branch references them now.
• The three failing tests:
  • test_lower_suld_1d.py and test_lower_lea_hi_constant.py: lower_to_maxwell.h is now exactly main's, so both pass unchanged.
  • test_shader_stamp_cache.py: the change moved to #1834, where streamline_sdk_path and opennr_jobs live.
• No CI: I can't approve the runs, but I ran the whole tests/run_all.py on this branch locally with clang targeting MSVC, the CPM Vulkan headers and glslang 11.12. It gives 324 passed; the 18 failures are the same 18 main has on this machine, all down to the toolchain, so there are no new ones.

Overlap: since your review, #1806, #1800, #1778 and #1810 all merged, and main also has SULD.1D, LEA.HI with a constant base and ISBERD.O. I dropped all of ours in their favour, including the broad !is_register[f] skip, the 1 MB local-memory cap, the unfenced LDG/STG and our closing self-branch.

Not in the description before:

• The noperspective rewrite is now the PR's main subject, with the reason: the red dotted line over the Royal Family Tomb water on AMD, where a clipped vertex with w near zero gets the wrong depth interpolated. It also lists where it applies (every VS+FS pipeline with a noperspective input, all vendors) and where it's skipped.
• The unswizzle import and the 4 MB threshold moved to #1834. That description explains both hitches they fix (22-25 ms per import, three at the Deku Tree turn).
• texture_touch_misses moved with its caller.

"70 programs": no longer claimed. Main now lowers everything those hunks did.

Nits: on_big_stack and RX_VK_TRACE_KEYS are gone, and with them the worker thread's std::set_terminate. RX_VK_LOWER_SKIP stays as a diagnostic. IsbeReadOutput's .32 and the redundant Clamped went with our ISBERD.O, since main's is used now.

Thanks for the thorough review; you were right on all counts. I rebuilt the branch on main at d7a73062, and it now **builds and tests on its own**. The description is rewritten. **Blocking:** - **`memory_guest_contained`**: the `vk_texture.cpp` changes moved to #1834, together with the function and its description. Nothing on this branch references them now. - **The three failing tests**: - `test_lower_suld_1d.py` and `test_lower_lea_hi_constant.py`: `lower_to_maxwell.h` is now exactly main's, so both pass unchanged. - `test_shader_stamp_cache.py`: the change moved to #1834, where `streamline_sdk_path` and `opennr_jobs` live. - **No CI**: I can't approve the runs, but I ran the whole `tests/run_all.py` on this branch locally with clang targeting MSVC, the CPM Vulkan headers and glslang 11.12. It gives 324 passed; the 18 failures are the same 18 main has on this machine, all down to the toolchain, so there are no new ones. **Overlap**: since your review, #1806, #1800, #1778 and #1810 all merged, and main also has SULD.1D, LEA.HI with a constant base and ISBERD.O. I dropped all of ours in their favour, including the broad `!is_register[f]` skip, the 1 MB local-memory cap, the unfenced LDG/STG and our closing self-branch. **Not in the description before:** - **The noperspective rewrite** is now the PR's main subject, with the reason: the red dotted line over the Royal Family Tomb water on AMD, where a clipped vertex with w near zero gets the wrong depth interpolated. It also lists where it applies (every VS+FS pipeline with a noperspective input, all vendors) and where it's skipped. - **The unswizzle import and the 4 MB threshold** moved to #1834. That description explains both hitches they fix (22-25 ms per import, three at the Deku Tree turn). - **`texture_touch_misses`** moved with its caller. **"70 programs"**: no longer claimed. Main now lowers everything those hunks did. **Nits**: `on_big_stack` and `RX_VK_TRACE_KEYS` are gone, and with them the worker thread's `std::set_terminate`. `RX_VK_LOWER_SKIP` stays as a diagnostic. `IsbeReadOutput`'s `.32` and the redundant `Clamped` went with our ISBERD.O, since main's is used now.

Joey-Magnus (Migrated from github.com) reviewed 2026-11-11 21:46:08 +00:00

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

The rebase onto d7a73062 handled the earlier review, but main has moved again and vk_shader.cpp conflicts in translate_into. When you resolve it, note that #2042 removed oot_visual(), so the line you moved up with the fragment translation won't compile any more. Main's block now just translates and fills frt. The math is right: perspective interpolation of v*w, times FragCoord.w (1/w), gives back the screen-linear value. But it rewrites every VS+FS pipeline with a noperspective input on every vendor, and it hasn't run on AMD, the GPU it's for. Could you get one AMD run of the Royal Family Tomb with before/after screenshots, plus a quick NVIDIA check that nothing else with noperspective varyings moved? RX_VK_LOWER_SKIP isn't in translator_stamp, so programs translated while it was set stay unlowered in the cache after it's unset. That's fine for a diagnostic, but say so in the comment. The "(super build)" tags in comments can go.

The rebase onto d7a73062 handled the earlier review, but main has moved again and vk_shader.cpp conflicts in translate_into. When you resolve it, note that #2042 removed oot_visual(), so the line you moved up with the fragment translation won't compile any more. Main's block now just translates and fills frt. The math is right: perspective interpolation of v*w, times FragCoord.w (1/w), gives back the screen-linear value. But it rewrites every VS+FS pipeline with a noperspective input on every vendor, and it hasn't run on AMD, the GPU it's for. Could you get one AMD run of the Royal Family Tomb with before/after screenshots, plus a quick NVIDIA check that nothing else with noperspective varyings moved? RX_VK_LOWER_SKIP isn't in translator_stamp, so programs translated while it was set stay unlowered in the cache after it's unset. That's fine for a diagnostic, but say so in the comment. The "(super build)" tags in comments can go.

hckrman101 commented 2026-11-11 22:55:37 +00:00 (Migrated from github.com)

• Copy link
• Copy Markdown

This now conflicts with main in gpu/vk/vk_shader.cpp. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

This now conflicts with main in `gpu/vk/vk_shader.cpp`. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

omgitsgela commented 2026-11-12 03:32:15 +00:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Steam Deck (RADV, VanGogh): the red particles and the red water both clear

Independent hardware confirmation that this fix addresses the red artifacts reported in #1634 / #1000, not just the Royal Family Tomb water.

Setup. Hookshot v2.1.0-rc.3-52-gebbba2e (upstream ebba2e), Steam Deck LCD, RADV on VanGogh, FSR 3.1 from the Handheld picture (1536x864) at 50% render resolution.

Patch. This PR's noperspective work only: the six hunks in runtime_info.h, spirv_emit_context.{h,cpp}, emit_spirv_context_get_set.cpp, emit_spirv_special.cpp and vk_shader.cpp. The RX_VK_LOWER_SKIP bisection and the sirit / emit_spirv error-reporting changes were left out, so this is the interpolation change alone. It applies to ebba2e with offsets only and builds with the release toolchain (clang 23.1.5, renderer-only rebuild, 50 s, 0 failed).

Before. Blinking red particle boxes over most of the screen, everywhere in Kokiri Village, and red-tinted sun highlights on the water — the same red as the particles. Reproduced consistently, and more obvious at 50% render resolution because upscaling magnifies the wrong pixels (a 1440p frame downscaled to the 800p panel averaged them away).

After. Same build, same save, same settings, only the renderer swapped (original 7b32684f…, patched 1f37986a…): the red particles are gone and the water's sun highlights render in their normal colour. Reported by the person playing, confirmed twice.

Mechanism, for the record. Main already emits NoPerspective; what it lacks is this PR's clipped-vertex workaround, so a varying whose vertex clips near w = 0 still interpolates wrongly on AMD and the consuming exp2 saturates to red. On an RTX 3090 the same build is clean, which is why this looked Deck-specific.

Not tested here: geometry-shader and mesh-program paths, and any effect that reads indexed attributes (the PR leaves those alone by design). No game files, saves or logs attached.

### Steam Deck (RADV, VanGogh): the red particles and the red water both clear Independent hardware confirmation that this fix addresses the red artifacts reported in #1634 / #1000, not just the Royal Family Tomb water. **Setup.** Hookshot `v2.1.0-rc.3-52-gebbba2e` (upstream `ebba2e`), Steam Deck LCD, RADV on VanGogh, FSR 3.1 from the Handheld picture (1536x864) at 50% render resolution. **Patch.** This PR's noperspective work only: the six hunks in `runtime_info.h`, `spirv_emit_context.{h,cpp}`, `emit_spirv_context_get_set.cpp`, `emit_spirv_special.cpp` and `vk_shader.cpp`. The `RX_VK_LOWER_SKIP` bisection and the sirit / `emit_spirv` error-reporting changes were left out, so this is the interpolation change alone. It applies to `ebba2e` with offsets only and builds with the release toolchain (clang 23.1.5, renderer-only rebuild, 50 s, 0 failed). **Before.** Blinking red particle boxes over most of the screen, everywhere in Kokiri Village, and red-tinted sun highlights on the water — the same red as the particles. Reproduced consistently, and more obvious at 50% render resolution because upscaling magnifies the wrong pixels (a 1440p frame downscaled to the 800p panel averaged them away). **After.** Same build, same save, same settings, only the renderer swapped (original `7b32684f…`, patched `1f37986a…`): the red particles are **gone** and the water's sun highlights render in their normal colour. Reported by the person playing, confirmed twice. **Mechanism, for the record.** Main already emits `NoPerspective`; what it lacks is this PR's clipped-vertex workaround, so a varying whose vertex clips near `w = 0` still interpolates wrongly on AMD and the consuming `exp2` saturates to red. On an RTX 3090 the same build is clean, which is why this looked Deck-specific. Not tested here: geometry-shader and mesh-program paths, and any effect that reads indexed attributes (the PR leaves those alone by design). No game files, saves or logs attached.

hckrman101 (Migrated from github.com) approved these changes 2026-11-12 03:50:25 +00:00

hckrman101 (Migrated from github.com) left a comment

Heads-up first: I merged main into the branch (d85bd1be) to clear the vk_shader.cpp conflict, so please pull before you push again. The conflict came from #2042 removing oot_visual() (Joey-Magnus pointed it out). The merge keeps your fragment translation moved ahead of the vertex stage and drops the oot_visual call. On my side the shader tests pass with it and the Vulkan back end builds.

Everything from my last review is handled. The rebuilt branch no longer touches lower_to_maxwell.h, vk_texture.cpp or the tests, so the build break, the three failing tests and the overlaps with #1806/#1800/#1778/#1810 are all gone, and so are on_big_stack and the set_terminate. The noperspective rewrite is now the PR's subject, and the description says what it changes on every vendor.

I went through the noperspective path again:

• The math holds, clipping included. Perspective-correct interpolation of v·w, times FragCoord.w (the interpolated 1/w), gives the screen-linear value. On a clipped edge, the clip-space lerp of v·w divided by the lerp of w is the same as the screen-space lerp of v.
• The vertex side covers every output element, including split variables (dedup by id). The fragment side only marks float, non-per-primitive inputs. The scaling runs after ConvertDepthMode, which only touches z.
• Indexed reads, XFB and the mesh path (its own frt) are left alone. The GS EmitVertex hooks are a no-op for now, because only the VS+FS branch sets noperspective_outputs.
• No manual cache bump is needed. HOOKSHOT_SHADER_STAMP hashes eden/, sirit/ and vk_shader.cpp, so the new SPIR-V gets a new stamp by itself.
• omgitsgela's Steam Deck run (RADV, VanGogh) is the AMD confirmation Joey asked for. With only the noperspective hunks applied, the red particles and the red water highlights from #1634/#1000 go away, and the same build is clean on an RTX 3090. Together with your NVIDIA run, that's enough for me.

Left, none of it blocking:

• Joey's point about RX_VK_LOWER_SKIP still stands: it isn't in translator_stamp, so a bisection only works on programs that aren't cached yet. The easiest fix is to add "RX_VK_LOWER_SKIP" to the prefix list. bake_ds.cpp needs the same entry, because test_program_stamp_knobs.py checks that the two lists match. Otherwise, say it in the comment.
• nx2/bake_ds/bake_ds.cpp copies this VS+FS path but doesn't set noperspective_outputs / scaled_noperspective_inputs, so an offline-baked bank would still have the old interpolation. It's self-consistent and not broken, just without the fix. It already leaves out other live-only bits, so a follow-up is fine.
• Nits: the (super build) tags are still in emit_spirv.cpp, stream.h and vk_shader.cpp. A before/after screenshot pair in the description would be good, since this shows on screen (CONTRIBUTING.md). omgitsgela's would do if they're OK with that.

CI: only the title check has run on this branch, and the build and test workflows haven't. That's why GitHub shows it as unstable.

Heads-up first: I merged main into the branch (d85bd1be) to clear the `vk_shader.cpp` conflict, so please pull before you push again. The conflict came from #2042 removing `oot_visual()` (Joey-Magnus pointed it out). The merge keeps your fragment translation moved ahead of the vertex stage and drops the `oot_visual` call. On my side the shader tests pass with it and the Vulkan back end builds. Everything from my last review is handled. The rebuilt branch no longer touches `lower_to_maxwell.h`, `vk_texture.cpp` or the tests, so the build break, the three failing tests and the overlaps with #1806/#1800/#1778/#1810 are all gone, and so are `on_big_stack` and the `set_terminate`. The noperspective rewrite is now the PR's subject, and the description says what it changes on every vendor. I went through the noperspective path again: - The math holds, clipping included. Perspective-correct interpolation of v·w, times FragCoord.w (the interpolated 1/w), gives the screen-linear value. On a clipped edge, the clip-space lerp of v·w divided by the lerp of w is the same as the screen-space lerp of v. - The vertex side covers every output element, including split variables (dedup by id). The fragment side only marks float, non-per-primitive inputs. The scaling runs after `ConvertDepthMode`, which only touches z. - Indexed reads, XFB and the mesh path (its own `frt`) are left alone. The GS `EmitVertex` hooks are a no-op for now, because only the VS+FS branch sets `noperspective_outputs`. - No manual cache bump is needed. `HOOKSHOT_SHADER_STAMP` hashes `eden/`, `sirit/` and `vk_shader.cpp`, so the new SPIR-V gets a new stamp by itself. - omgitsgela's Steam Deck run (RADV, VanGogh) is the AMD confirmation Joey asked for. With only the noperspective hunks applied, the red particles and the red water highlights from #1634/#1000 go away, and the same build is clean on an RTX 3090. Together with your NVIDIA run, that's enough for me. Left, none of it blocking: - Joey's point about `RX_VK_LOWER_SKIP` still stands: it isn't in `translator_stamp`, so a bisection only works on programs that aren't cached yet. The easiest fix is to add `"RX_VK_LOWER_SKIP"` to the prefix list. `bake_ds.cpp` needs the same entry, because `test_program_stamp_knobs.py` checks that the two lists match. Otherwise, say it in the comment. - `nx2/bake_ds/bake_ds.cpp` copies this VS+FS path but doesn't set `noperspective_outputs` / `scaled_noperspective_inputs`, so an offline-baked bank would still have the old interpolation. It's self-consistent and not broken, just without the fix. It already leaves out other live-only bits, so a follow-up is fine. - Nits: the `(super build)` tags are still in `emit_spirv.cpp`, `stream.h` and `vk_shader.cpp`. A before/after screenshot pair in the description would be good, since this shows on screen (CONTRIBUTING.md). omgitsgela's would do if they're OK with that. CI: only the title check has run on this branch, and the build and test workflows haven't. That's why GitHub shows it as unstable.

gpu/vk/eden/shader_recompiler/backend/spirv/spirv_emit_context.cpp

hckrman101 (Migrated from github.com) commented 2026-11-12 03:50:25 +00:00

Nit: loads is already info.loads.mask | info.passthrough.mask (top of DefineInputs), so the info.passthrough.AnyComponent(PositionX) term repeats loads.AnyComponent(PositionX). || noperspective_as_smooth is the only part that does something.

Nit: `loads` is already `info.loads.mask | info.passthrough.mask` (top of `DefineInputs`), so the `info.passthrough.AnyComponent(PositionX)` term repeats `loads.AnyComponent(PositionX)`. `|| noperspective_as_smooth` is the only part that does something.

github-sync referenced this pull request 2026-11-12 06:22:54 +00:00

chore(main): release 2.2.0-rc.1 #2071

github-sync referenced this pull request from a commit 2026-11-12 13:38:09 +00:00

fix(shaders): #1830's follow-ups: the noperspective workaround baked offline too, RX_VK_LOWER_SKIP in the program stamp

github-sync referenced this pull request 2026-11-12 13:38:10 +00:00

fix(shaders): the noperspective workaround baked offline too; RX_VK_LOWER_SKIP in the program stamp #2782

github-sync commented 2026-11-12 13:38:26 +00:00

Owner

• Copy link
• Copy Markdown

@FIocker commented on GitHub · 2026-11-12 12:49 UTC · original

Thanks, both of you, for merging main in and for the review.

@omgitsgela: thank you for the Steam Deck run. Good to know it clears the red particles from #1634/#1000 too, not just the tomb water.

The non-blocking notes are in #2782:

• bake_ds.cpp now bakes the noperspective workaround as live translation does.
• RX_VK_LOWER_SKIP is in translator_stamp and bake_ds.cpp's list.
• The redundant passthrough term is gone.
• The (super build) tags are gone.

> [@FIocker](https://github.com/FIocker) commented on GitHub · 2026-11-12 12:49 UTC · [original](https://github.com/NotRexed/Hookshot/pull/1830#issuecomment-6824056800) Thanks, both of you, for merging main in and for the review. @omgitsgela: thank you for the Steam Deck run. Good to know it clears the red particles from #1634/#1000 too, not just the tomb water. The non-blocking notes are in #2782: - `bake_ds.cpp` now bakes the noperspective workaround as live translation does. - `RX_VK_LOWER_SKIP` is in `translator_stamp` and `bake_ds.cpp`'s list. - The redundant passthrough term is gone. - The `(super build)` tags are gone. <!-- github-sync:c:6824056800 -->

github-sync referenced this pull request 2026-11-12 13:38:29 +00:00

feat(vk): DLSS 5 neural rendering through NGX, in the temporal chain before the HUD #1833

github-sync referenced this pull request 2026-11-12 13:39:06 +00:00

feat(vk): shadow resolution option #1892

Commenting is not possible because the repository is archived.

Reviewers

No reviewers

hckrman101

Labels

Clear labels   

accessibility

Barrier affecting people with disabilities

  

autorelease: pending

  

autorelease: tagged

  

bug

Something isn't working

  

dependencies

Pull requests that update a dependency file

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

github_actions

Pull requests that update GitHub Actions code

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

question

Further information is requested

  

testing wanted

  

wontfix

This will not be worked on

No labels

accessibility

autorelease: pending

autorelease: tagged

bug

dependencies

documentation

duplicate

enhancement

github_actions

good first issue

help wanted

invalid

question

testing wanted

wontfix

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

2 participants

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

MagicBeanDev/Hookshot!1830

No description provided.