WIP: fix(vk): OoT's clustered lights sample their own shadow maps (handle tables as descriptor arrays, from Lichii) #107
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!107
Loading…
Reference in a new issue
No description provided.
Delete branch "pmwork-personal/Longshot:fix/light-table-shadow-handles"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Why
On Zora's Domain's ice and water, dark stair-step blocks sit around Link, and the snow shows green blocks (#52). OoT's clustered lights read each light's shadow-map handle per pixel from the light table; the shader translation could not compute such a handle, so those shadow tests bound no texture.
What changed
A port of Lichii
bd0171f7("bind OoT light-table shadow maps through descriptor arrays … fixes the dark water grid", its #1485) and282a585a(its #1523), by Zortos, to Longshot's copy of the recompiler, which had the same code as Lichii before them.gpu/vk/eden/shader_recompiler/ir_opt/texture_pass.cpp:Track()no longer searches through texture instructions or float math. Where the shader reuses the handle's register for a sample result, it used to takec6[0x430], a float, as the handle.BuildHandleNode()reads a word of a 12- or 16-byte load (CompositeExtractU32x3/x4: the shadow map's handle is the first word of a 16-byte load from the light's 96-byte entry). Its newaddress/index_wordlambdas take one word read at a per-invocation address, scaled to a byte offset, as the element index of a descriptor array (HandleOp::Index).MakeInst()retries with that index word where the host allows. Where the word dominates the sample (Dominance), the texture becomes an array of the table's first 64 entries (HANDLE_TABLE_SIZE), indexed nonuniformly with the word.TrimTrailingTees()lets the samples of one table share one handle program and one array. A word the earlier search still takes that reaches the handle only through a load's address is a pointer word (ReachesOutsideLoads()).TexturePass(): the stage cap leaves handle-table arrays alone (it is 1 here, becausehost_info()sets no descriptor limits); the index word is the element itself.shader_info.h:HandleOp::Index,EvaluateHandleProgram(..., element),TextureDescriptor::handle_is_pointer_word.host_translate_info.h:support_handle_table_arrays.gpu/vk/vk_shader.cpp:stage_handle(..., element); newstage_handles()computes every element, reading the constant words and storage addresses once; newhandle_tables_on()needsshaderSampledImageArrayNonUniformIndexingand the descriptor limits and logs its answer. It is read byhost_info(), and bytranslator_stamp()when off.gpu/vk/vk_draw.cppbuild_descriptors(): binds a table's elements fromstage_handles(), looks a repeated entry up once, and binds nothing for an entry that names a colour texture for a depth comparison (logged, at most 8 lines).handle_is_storage_pointer()honours the pointer word.tests/test_handle_table_arrays.py. Thestage_handlestubs intest_texture_handle_banks.pyandtest_program_answer_domains.pytake the element.RX_VK_HANDLE_TABLESindocs/development.md.Flags and settings
RX_VK_HANDLE_TABLES=0main(no texture). Part of the program cache's stamp only when offshaderSampledImageArrayNonUniformIndexingmainMeasurements
main:
b9cdc432(2026-11-13)this PR: fix/light-table-shadow-handles @
a801fd0cHardware: offline (RTX 5090, NVIDIA 621.24, Ryzen 7 9800X3D, Windows 11); no game run yet
Scene: the 4,672 fragment stages dumped in Kokiri Forest (
HOS_SHADER_DUMP), through the translator built from each commit (frontend and IR passes; for this PR also the SPIR-V back end)Method: a scratch copy of
texture_pass.cpplogs how each bindless texture instruction got its handle;spirv-valv2026.4 (--target-env vulkan1.3 --uniform-buffer-standard-layout --scalar-block-layout, as the device has them) on each module with a table;tests/test_handle_table_arrays.pyHow to reproduce:
HOS_SHADER_DUMP=<folder>inlongshot_env.txtand one visit to Kokiri Forest; thengpu/shadertool.cpp's path over the dump with that logging (the scripts are not part of this PR; on request). In game: see Testing.b9cdc432a801fd0cc0[0x520](no texture at draw time)c6[0x430](a float)spirv-valSampledImageArrayNonUniformIndexing,NonUniformon the indextests/test_handle_table_arrays.pyIn game (by eye), on this PC: RTX 5090, NVIDIA 621.24, Windows 11; options file
output_res=3840x2160,dlss=2,frame_generation=3(the baseline's 2),upscaling=0. Scene: Zora's River, child Link, reached with the Debug tab's Teleport from a Market save. Neither build is a Longshot build (none on this PC yet); the second carries this PR's change unchanged (af4d3774=a801fd0c's diff on upstream Hookshot'smaine0e60920), and the first has no change to the texture pass or the draw's texture binding.Baseline: Hookshot dev build @ 06ad86fe (upstream
ba3d3f73+ motion-vector research)e0e60920+ this fix @ af4d3774vk: handle tables as descriptor arraysonTesting
Tested:
python -I -B tests/run_all.py(Windows, 16 jobs): 489 passed, 6 failed, 36 skipped. The 6 fail onmain@b9cdc432too on this PC:test_fpx_sticky,test_spawn_emit,test_portable_builder_startup,test_setup_runtime,test_check_third_party,test_release.Indexignoring the element,stage_handles()passing no element, trailing Tees kept, the 16-byte extract dropped.main(the changed code is the same there) prepared every shipped program with the handle tables on:vk: handle tables as descriptor arrays: on;prepared 38809 shader programs (46666 new, 1 failed ...), the same counts as that main's own build; 24,453 pipelines built by the driver, 0 not made.Not tested:
Needs other testers:
RX_VK_HANDLE_TABLES=0; fps with it on and off (T-5ECM).Screenshots
main: the two screenshots in #52 (the reporter's v2.2.0-rc.1). This PR: none yet (the fix was checked by eye at Zora's River).Risk and rollback
main.RX_VK_HANDLE_TABLES=0(these samples as onmain), or revert the commit.Reviewer notes
Start at
MakeInst()andBuildHandleNode()'saddress/index_wordintexture_pass.cpp, then the table path inbuild_descriptors(). Not ported from Lichii, on purpose:host_info()sets no limits here, andhandle_tables_on()asks for at least 256 per stage);HandleTableNoteslog;Issues
Fixes #52
Checklist
For everyone
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.mainand passing here.tests/that reads changed source updated in this PR, not weakened.Tested in game: the fix works.
At Zora's River with child Link (reached with the Debug tab's Teleport), on an RTX 5090 with NVIDIA 621.24 and Windows 11:
06ad86fe, with no change to the texture pass or the texture binding): the dark stair-step blocks are there.af4d3774: this PR's diff on upstream Hookshot'smain): they are gone. The log saysvk: handle tables as descriptor arrays: onand shows no errors.The description's Measurements and Testing now say so. Still open before ready: a Longshot build of
a801fd0c, fps withRX_VK_HANDLE_TABLES=0against on, screenshots, frozen Zora's Domain, and AMD/Linux (T-5ECM, T-7DJC).No blocking bugs found: the port looks right. A few things inline, none of them blockers.
What I checked
b9cdc432).tests/test_handle_table_arrays.pyandtests/test_texture_handle_banks.pypass. The fullrun_allwas 451 passed, 11 failed, 69 skipped. The cache tests among the failures fail the same way on main in the same environment, and a lot of GPU tests were skipped because there are no Vulkan headers here.LONGSHOT_SHADER_STAMPhasheseden/andvk_shader.cpp, andHandleOp::Indexis added at the end of the enum.shaderSampledImageArrayNonUniformIndexingreally is enabled:vk_main.cpppasses the queried feature chain tovkCreateDevice.record_answers) skip count>1, so the tables stay out of them.g_handle_index.HandleOperandsmatchesEvaluateHandleProgram.Still needed before
pr ready(you list these yourself):a801fd0crun in the game. The in-game check so far used the same diff on upstream Hookshot.@ -425,0 +447,4 @@// made from them, while a register the program reuses for the handle and for a sample// result (a Phi of both: OoT's clustered lights) would lead the search through the// sample's coordinates to an unrelated word (c6[0x430], a float).if (IsTextureInstruction(*inst) || IsFloatValue(*inst)) {This skip applies to every bindless sample, whatever
support_handle_table_arrayssays. So doTrimTrailingTeesand thepointer_wordmarking. That meansRX_VK_HANDLE_TABLES=0doesn't translate "as on main", which is what the Flags table and docs/development.md say.It's probably harmless: the offline numbers show the other 5,423 samples unchanged. Still, either have the docs and PR text say what the switch actually turns back, or gate these changes on the switch too.
@ -964,0 +1233,4 @@// The shader indexes the array with the index word, which must be defined wherever// the texture instruction runs; where it isn't, every invocation takes the first// element.u32 count{1};When the index word doesn't dominate the sample, the descriptor stays count 1, but its handle program still contains
HandleOp::Index, which then gets evaluated with element 0. Every pixel would get table entry 0's shadow map. That's a new kind of wrong: main showed nothing there.Does any program in the dump hit this case? If one does, I'd fall back to the legacy search there, or mark the word as a pointer word, rather than bind entry 0.
@ -1423,0 +1713,4 @@const IR::U32 element{size_shift != 0? IR::U32{ir.ShiftRightLogical(cbuf.dynamic_offset, ir.Imm32(size_shift))}: cbuf.dynamic_offset};inst->SetArg(0, ir.UMin(element, ir.Imm32(count - 1)));With
HANDLE_TABLE_SIZE = 64, any light index of 64 or more gets clamped to entry 63's shadow map, and nothing is logged. That's fine at Zora's River (about 43 lights going by your log), but a busier scene would get the wrong shadows without any sign of it.Suggestion: log a warning in
build_descriptorswhen table entry 63 is non-zero, since that means the table probably runs past 64.@ -3591,0 +3595,4 @@// shadow map). The handles of all elements at once: the constant words and storage addresses the program reads// are read oncestatic thread_local std::vector<uint32_t> table;const bool is_table = d.handle_program != ~0U && d.count > 1;For each draw, the 687 table programs run 64 handle evaluations and up to 64
texture_view_oflookups, and the reuse check only catches an entry that equals the one right before it. You already list this as unmeasured. It's the number to get before ready. If it shows up, a per-draw handle-to-view cache (or checking against the entries already seen) would help.@ -560,0 +573,4 @@const bool want = !e || atoi(e) != 0;logf("vk: handle tables as descriptor arrays: %s", !want ? "off (RX_VK_HANDLE_TABLES=0)": can ? "on": "off (no nonuniform sampled image array indexing)");canis also false when the descriptor limits are too low (under 256 per stage, or under 512 per set), but the log always says "no nonuniform sampled image array indexing". Could it say which check failed (the feature, or which limit)? That would save a round trip on a tester's log.Test request T-5ECM: mixed (no regression found; the bug didn't show on main here, so the fix couldn't be confirmed)
Hardware: RTX 3080 10 GB, NVIDIA 621.14, i9-12900K, Windows 11. Built main @
b9cdc432and this PR @a801fd0cthe same way (standalone, llvm-mingw). 1280x720 window, default options (no DLSS: these builds have no NGX SDK), frame cap 60.What I ran, on each build: an adult save, the debug teleport to Zora's Domain (frozen,
ZoraDomain_Adult), then 8 steps forward (2.5 s of stick each) through the domain onto the frozen lake and past the pillar to the snow by the cave, with a screenshot after each step; on main also adult Zora's River with a short walk. Each build had a warm-up run first, then the compared run.vk: handle tables as descriptor arrays: on. No device-lost or die() lines in any run.frames presentedlines after arrival): main 45.6 and 47.6, this PR 48.7 and 46.5 (one warm-up and one compared run each). Level within run-to-run noise. Further in (the lake, the pillar, the cave) both held the 60 cap.RX_VK_HANDLE_TABLES=0(with no visible difference between main and the PR here, it would show nothing either), child Zora's River, Jabu-Jabu's water, the Chamber of Sages and the Sacred Realm (no save there), DLSS or DLAA, and higher resolutions. The reporter of issue 52 used an RTX 3080 Ti at 1920x1080 fullscreen with DLAA; your own repro was 4K with DLSS. The blocks may depend on the upscaler or the resolution, which this PC's builds didn't match.So on an RTX 3080 / Windows the PR changes nothing visible and costs nothing measurable at these spots; whether it removes the blocks needs a run where main shows them.
View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.