WIP: fix(vk): keep the shadow map and other drawn textures the game samples again through evictions #211
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/igpu
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
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!211
Loading…
Reference in a new issue
No description provided.
Delete branch "TrashPanda/Longshot:fix/keep-resampled-drawn"
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
The game draws its large shadow map once and then samples it for many frames. When textures went over the VRAM budget, the renderer evicted it and made it again empty, so shadows went missing or wrong and the ground turned dark (Hookshot issue 2209) until the game drew it again some 40 frames later. This was Hookshot PR 2368. It wasn't merged before the GitHub repo went down, so I'm bringing it here, bounded so it can't add VRAM pressure on small cards.
What changed
gpu/vk/keep_resampled.h(new) andgpu/vk/vk_texture.cpp's eviction pass. A texture the GPU drew that the game samples again after finalizing it is marked (Texture::resampled,gpu/vk/vk_texture.h) and kept through evictions over the budget. It stays a candidate only while it was used in the last 1,800 presents and its storage is still the GPU's (gpu_owned, the checkkeep_finalizedalready makes). When another render target takes that range, for example after an area change, the pin goes right away.RX_VK_KEEP_RESAMPLED_MB(512), an eighth of the texture budget (128 MB on a 1 GB budget, 200 MB on 1.6 GB) andkeep_finalized's cap. It is 0 once VRAM use reaches 97% of what the driver allows, the same linekeep_rendereduses. Past that line the textures go as they did on main, and the small ones arekeep_finalized's again.keep_finalized's 512 MB. Together the two hold no more thankeep_finalizedheld alone on main. When the cap is reached, the least recently used go first.tests/test_keep_resampled.pyand.cppcheck each rule: the budget/8 cap, nothing kept at 97% (the same line askeep_rendered, checked across a range), the pin released once the storage is no longer the GPU's, and the shared cap never exceeded. The source checks find the pass's calls. Without the change the checks fail.docs/development.md, and the capped-VRAM runs indocs/tested-configurations.md(from Hookshot, before this bound).Flags and settings
RX_VK_KEEP_RESAMPLED_MB=<MB>RX_VK_KEEP_FINALIZED_MB's room. It is 0 at 97% of the driver's limit.0gives the old behaviorkeep_finalizedcould already hold, and steps aside at the driver's limitMeasurements
No Longshot measurement yet. The numbers below are from Hookshot, measured on the version the review saw (a 512 MB cap of its own, no budget or driver-limit bound, no shared cap). They show what the rule costs when it acts, but they don't measure this head. The PR stays WIP until T-DCLN and T-J6DC are done.
main: Hookshot
6c8c1bfb(2026-11-11)this PR: Hookshot
fix/keep-resampled-drawn@3fbad818Hardware: RTX 4070 Ti 12 GB, 595.99.02, Ryzen 7 9800X3D, 32 GB, Ubuntu 26.04.1
Scene: Hyrule Field, 2560x1440 window, 2048x1152 output,
HOS_FPS_MAX=240Method: alternating scripted 3-minute runs with
RX_VK_VRAM_MBat 4000, 2000 and 1000, and at 4000 with 3.8 GB of VRAM held by another process;RX_VK_LOG_REMADE=1How to reproduce: the row dated 2026-11-11 in
docs/tested-configurations.md6c8c1bfbIn those Hyrule Field runs the rule kept nothing. It kept textures only in Hookshot's Dodongo's Cavern run at a 512 MB budget (both shadow maps no longer sampled EMPTY). Under the new bound, that budget's cap is 64 MB, so the 288 MB map would now go there by design. T-DCLN therefore uses a 3000 MB budget.
Testing
Tested:
python -I -B tests/run_all.pyon Linux with the Vulkan and SPIR-V headers, onmainf3fe1763: 513 passed.test_lift_cacheandtest_menu_i18nfail, and both fail the same way onmain.test_keep_resampledpasses, and its checks fail with the rule's bounds taken out one at a time (no budget/8, no 97% line, nogpu_owned, no shared cap).gpu/vk/vk_texture.cppcompiles (clang++ with the renderer's flags, syntax only). The Hookshot runs above are of the earlier version.Not tested: any in-game run of Longshot with this branch. I could not run the game here, and this head with the bound and the shared cap has not been run in the game anywhere. Windows build.
Needs other testers:
RX_VK_VRAM_MB=3000, Dodongo's Cavern and Hyrule Field shadows, against main andRX_VK_KEEP_RESAMPLED_MB=0. The author's RTX 4070 Ti can do this one: T-DCLNScreenshots
Nothing new on screen when it works: the shadows stay as they should.
Risk and rollback
The VRAM kept over the budget is no more than on main. The two rules share
RX_VK_KEEP_FINALIZED_MB's 512 MB, this rule's part is at most an eighth of the budget, and it is nothing at 97% of the driver's limit. The trade: while the shadow maps are kept (320 MB in Dodongo's Cavern),keep_finalizedhas 192 MB for the terrain's small finalized textures (the square streaks on the ground thatkeep_finalizedwas added for) instead of 512 MB. If those streaks come back over the budget, that is why.RX_VK_KEEP_RESAMPLED_MB=0brings back the old behavior.Reviewer notes
keep_resampled.hholds the rule:candidate,cap,finalized_roomandkeep.vk_texture.cpp's eviction pass calls it. The cap uses the budget as a size (size_budget), before the 90% rule and the compaction's hold take an excess off it, so it stays steady from pass to pass. The driver's limit is the 97% line's job.Issues
Ports Hookshot PR 2368, which addresses Hookshot issue 2209. Bounded with Longshot issue 130 in mind. No Longshot issue tracks the shadow map yet.
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.docs/tested-configurations.mdfor each measured run.mainand passing here.c75a3c3ef287610ac035Code review only (no build or run). The fix itself looks right. A finalized texture the GPU drew is marked
resampledwhen a sampling touch reaches it, and the mark is cleared at both placesreleasedis cleared. The pass sums the candidates first and lets the oldest go until the rest fit the cap, the same way askeep_finalized. Nothing new for threads:resampledis written and read wherereleasedandlast_framealready are.What I'd like changed before this leaves WIP is the VRAM it can pin on small cards:
keep_rendereddrops from 3600 to 600 presents once use is at 97% of what the driver allows. This rule has no step like that, and no step that scales with the budget. On a 4 GB card or an iGPU (budget about 1-1.6 GB), 512 MB is a third to a half of the budget. That is the state of #130 (an 8 GB RX 6600 at the driver's limit, then device lost). Please bound it: for example a cap tied to the budget (min(512 MB, budget/8)), and 0 or the old behaviour whenknown && used >= allowed/100*97.keep_finalized's 512 MB. Now they have their own 512 MB, and they free their old room inkeep_finalizedfor other textures. Together the two rules can hold 1 GB over the budget instead of 512 MB.finalized_oncealready checks that the storage still belongs to the GPU (gpu_owned);candidatedoesn't. Adding that check would release a pin as soon as the game gives its memory to something else.Smaller points:
resampled,gpu_writtenandrecenttwice (candidatechecks them again). Intest_keep_resampled.py, one needle is a substring of the one before it.@ -0,0 +14,4 @@// rendered, not uploaded, and sampled EMPTY until the game drew it again some 40 frames later (Dodongo's Cavern at a 512 MB// budget, issue 2209's dark ground in Hyrule Field).//// Only while used in the last kMaxUnused presents (unused for fewer), the compaction reserve's own line (#2198): one from an"#2198" and "issue 2209" are Hookshot numbers, but read here as Longshot's. Full URLs please (AGENTS.md): https://github.com/NotRexed/Hookshot/pull/2198, https://github.com/NotRexed/Hookshot/issues/2209.
@ -0,0 +19,4 @@// unused 400 presents before the eviction took it.constexpr uint64_t kMaxUnused = vram_compact::kReserveUnused;inline bool recent(uint64_t unused) { return unused < kMaxUnused; }inline bool candidate(bool overwritten, bool gpu_written, bool resampled, uint64_t unused) {The only things that release a pin are age (1,800 presents: 90 s at 20 fps) and a GPU draw at the same start (
drawn_over, ordrawn_over_shiftedfor single-level render targets). If the game frees the shadow map's storage on an area change and uploads ordinary textures there, the old map stays pinned through the new area's load.finalized_oncechecks thatgpu_ownedstill covers the texture's range; could the candidate check do the same, so the pin goes once the storage is someone else's?@ -4552,0 +4560,4 @@// over the budget up to RX_VK_KEEP_RESAMPLED_MB (512 by default, the most recently used first; 0: evicted as before). Their// total first, as finalized_kept's: keep_resampled.hstatic const uint64_t keep_resampled_max =(uint64_t)(getenv("RX_VK_KEEP_RESAMPLED_MB") ? std::max(0, atoi(getenv("RX_VK_KEEP_RESAMPLED_MB"))) : 512) << 20;A fixed 512 MB default, separate from
RX_VK_KEEP_FINALIZED_MB's 512. On an iGPU or 4 GB card with a budget of about 1-1.6 GB, this alone is a third to a half of it. The resampled textures also leavekeep_finalized's total (line 4576), so that rule now has room for 512 MB of other textures. The two can hold 1 GB over the budget where main held 512 MB. Suggestmin(512 MB, budget / 8)or similar, or one shared cap.@ -4563,0 +4580,4 @@uint64_t resampled_total = 0;if (keep_resampled_max)for (auto& [last, tex] : order)if (tex->resampled && tex->gpu_written && keep_resampled::recent(frame_no - last) &&Nit:
tex->resampled && tex->gpu_written && keep_resampled::recent(...)repeats whatcandidatechecks.candidate(drawn_over_shifted(tex) || drawn_over(tex), true, true, frame_no - last)after the cheap checks would read more simply. The order matters, though: keep the cheap checks first so the overlap search runs only for the few that are marked.@ -4625,0 +4648,4 @@// (above. Oldest first, as keep_finalized. Past its cap, a small finalized one is keep_finalized's again, below)const bool resampled_candidate =keep_resampled::candidate(overwritten, tex->gpu_written, tex->resampled, frame_no - last);if (resampled_candidate && keep_resampled::keep(tex->bytes, resampled_total, keep_resampled_max)) {This keep runs before every other check in the pass. It holds up to 512 MB whatever drove the pass: the budget, the 90% rule or the driver's limit.
keep_rendered::presents(a few lines up) shortens to 600 presents onceused >= allowed/100*97. This rule has no step like that, and on a 4-8 GB card at the driver's limit it keeps half a GB pinned (the state of #130). Could it give way near the limit (cap 0, or the old behaviour, whenknown && used >= allowed/100*97) and scale its cap withbudget?@ -0,0 +16,4 @@"keep_resampled::candidate(overwritten, tex->gpu_written, tex->resampled, frame_no - last);","if (resampled_candidate && keep_resampled::keep(tex->bytes, resampled_total, keep_resampled_max)) {","keep_resampled::keep(tex->bytes, resampled_total, keep_resampled_max)","if (keep_resampled_max && tex->resampled && keep_resampled::recent(frame_no - tex->last_frame)) return false;",Nit: this needle is a substring of the one before it, so it checks nothing new.
87610ac035ff0dbae4a8Thanks @HumanBean. Rebased on main (
fbd42024), new headff0dbae4. The original commit is unchanged apart from the rebase (b6fe2703), and the answer to the review is one commit on top of it (ff0dbae4). Point by point:(also line comment on
vk_texture.cpp, "Could it give way near the limit ... and scale its cap withbudget?")Done in
ff0dbae4.keep_resampled::capreturns 0 whenknown && used >= allowed / 100 * 97, the same linekeep_rendered::presentssteps at. With the cap at 0 the pass behaves as on main:keep_finalizedcounts the small resampled textures again and the big ones go. Otherwise the cap ismin(RX_VK_KEEP_RESAMPLED_MB, size_budget / 8, keep_finalized's cap). I usedsize_budget, the budget as a size before the 90% rule and the compaction's hold take an excess off it, so the cap doesn't swing from pass to pass. Near the driver's limit, the 97% line makes the decision. On a 1-1.6 GB budget the cap is 128-200 MB. The test checks 1 GB → 128, 1.6 GB → 200, 2.5 GB → 320 and 6 GB → 512. It also checks that, across 90-101% of the allowance, the cap is 0 exactly wherekeep_renderedsteps to 600.(also line comment on
keep_resampled_max, "Suggestmin(512 MB, budget / 8)or similar, or one shared cap.")Done in
ff0dbae4, with one shared cap.keep_finalized_max(RX_VK_KEEP_FINALIZED_MB, 512) is the cap for both rules. Whatkeep_resampledmay hold in a pass,min(its candidates' total, its cap), comes offkeep_finalized's room (keep_resampled::finalized_room), and its own cap is never above the shared one. Together they hold at most 512 MB, the same askeep_finalizedalone on main. The test checks this sum over budgets of 0.5-16 GB and totals of 0-1 GB.Why a shared cap instead of putting the resampled textures back inside
keep_finalized:keep_finalizedonly takes textures of 4 MB or less that were drawn in one frame. The shadow maps (288 MB and 32 MB) were never inside its 512 MB, so counting them "inside it again" would need its own size and drawn-once rules changed. A shared cap leaveskeep_finalizedas it is and bounds the total all the same. Resampled textures take their part first because the game has been seen sampling them again.keep_finalized's textures are kept on the chance that the game remakes them. The cost: while 320 MB of shadow maps are kept,keep_finalizedhas 192 MB for the terrain's small textures instead of 512. The description's Risk section says so. Thekept finalized textures over N MBlog line now prints that room, so this can be seen in a log.(also line comment on
keep_resampled.h'scandidate)Done in
ff0dbae4.candidate(overwritten, gpu_owned, gpu_written, resampled, unused)now requires the storage to still be the GPU's.vk_texture.cpphas onegpu_ownslambda, the checkfinalized_oncemade, and both rules use it. One limit, so it's on record:gpu_ownedchanges when a render target is drawn over that range (at the same start, or covering it), not when the CPU writes there. If the game puts render targets in the old area's memory, the pin goes at once. If it only uploads ordinary textures there, the pin still goes by age (1,800 presents) or when the texture is next touched and found written. The test covers the release (candidate(false, false, true, true, 0)is false).Not done yet: I couldn't run the game here, so the PR stays WIP and Testing says so. I've opened two test requests:
RX_VK_VRAM_MB=3000(cap 375 MB, so both shadow maps fit), Dodongo's Cavern for 5 min, then Hyrule Field, against main and againstRX_VK_KEEP_RESAMPLED_MB=0. TrashPanda's RTX 4070 Ti 12 GB can run this one. Note that the old Dodongo's Cavern run at a 512 MB budget would now keep only 64 MB, so the 288 MB map goes there by design. That's why this run uses a higher budget.vk: VRAM:line and any device lost.The relay's T-LYJB (from the
testing wantedlabel) was claimed before this push. @d4nkn3ss, please test headff0dbae4, not87610ac0.(also line comment on
keep_resampled.h)Done in
ff0dbae4and in the description.keep_resampled.hlinks https://github.com/NotRexed/Hookshot/pull/2198 and https://github.com/NotRexed/Hookshot/issues/2209, and the test's message no longer says "#2198". The Issues section links Hookshot PR 2368, Hookshot issue 2209 and Longshot issue 130.(also line comment on the pre-sum loop)
Done in
ff0dbae4. The loop reads only thetex->resampledfield first, then callscandidate(drawn_over_shifted(tex) || drawn_over(tex), gpu_owns(tex), tex->gpu_written, true, frame_no - last). The cheap check still comes first, so the overlap search runs only for the textures that are marked.gpu_writtenstays an argument:resampleddoesn't imply it, because two places cleargpu_writtenand leaveresampledset (the write-back into memory is one). The pass uses the same form.(also line comment on the test)
Done in
ff0dbae4. The needle is removed. No needle in the list is now a substring of another; I checked this mechanically. The new needles findcap(...),finalized_room(...),gpu_owns(tex)in both candidate calls andfinalized_kept <= finalized_room. The test also fails iffinalized_kept <= keep_finalized_maxcomes back.Tests:
python -I -B tests/run_all.py: 510 passed.test_lift_cachefails, and it fails the same way on main.test_recorder_synctimed out once while the PC was under heavy load and passed when run alone. I also took each new rule out ofkeep_resampled.hin turn (the budget/8 bound, the 97% line, thegpu_ownedcheck, the shared room), andtest_keep_resampledfailed each time. One fix outside the new code:test_target_overlapreadsfinalized_once's last line, so that line keeps its exact text.@TrashPanda please add measurements
ff0dbae4a8d6219eb738Main moved while I worked, so I rebased again onto
f3fe1763. The new head isd6219eb7.git range-diffshows both commits unchanged (738cda2d=b6fe2703,d6219eb7=ff0dbae4), so everything in the answer above still applies, and so do the test requests T-DCLN and T-J6DC (their notes nameff0dbae4: same change). Tests on the rebased head: 513 passed.test_lift_cacheandtest_menu_i18nfail, and both fail the same way onmainf3fe1763.d6219eb738aad9cc15c4Reworded d6219eb7's message only (an unwanted tool trailer removed), no code change: it is now
aad9cc15. The test requests' code (ff0dbae4/d6219eb7) is the same asaad9cc15.Test request T-LYJB: mixed. No regression with the rule on, but I couldn't reproduce the dark ground / missing shadows with it off, so this doesn't confirm the fix.
Hardware: RTX 3080 10 GB, NVIDIA 621.14, i9-12900K, Windows 11. Built
87610ac0(this PR's head; it conflicts with main now, so the runs are on that head as it was). Headless, scripted input only,RX_VK_VRAM_MBcapped so textures are evicted; an in-build A/B:RX_VK_KEEP_RESAMPLED_MB=0(the old behaviour) against unset (512).What I ran:
RX_VK_VRAM_MB=1024: a warm-up, then four runs interleaved off/on/off/on. Each run: two laps of Kakariko → Hyrule Field → Market → Hyrule Field by the debug teleport, and 12 shots 2 s apart from 3 s after each Field arrival (48 a run). For each shot I measured the ground's mean brightness (the lower part of the picture), and counted a "dip" as a shot more than 25% darker than its visit's median.RX_VK_VRAM_MB=512, as in your Hookshot walk.textures retired ...: budget N)So the bug didn't show on this card at these spots and caps, and I can't say the PR fixes it. A run that shows it on the old behaviour first, ideally an 8 GB card in sunlit Hyrule Field away from the tunnel as you asked, or a screenshot pair from your Dodongo's Cavern walk, would settle it. The shots stay on this PC (game imagery from my dump), so I'm not attaching them.
Re-review at
aad9cc15. Code only: I didn't build it, run the tests or run the game.Earlier points:
keep_resampled::capismin(RX_VK_KEEP_RESAMPLED_MB, keep_finalized's cap, size_budget / 8)and 0 whenknown && used >= allowed / 100 * 97. That is the sameused,allowedand line askeep_rendered::presents, and the test checks that both rules step at the same point. With the cap at 0, everything goes back to main's behaviour:resampled_candidateis false,keep_finalizedno longer leaves resampled textures out, andfinalized_roomis the full 512 MB. Usingsize_budget(before the 90% rule shrinks it) is fine: the 97% line covers the driver's limit.keep_finalized's room (shared - min(shared, total, cap)), and its cap is never above the shared one. I worked through the case where a resampled texture goes past the cap and joinsfinalized_kept: the two rules still hold at most 512 MB together. The Risk section names the trade-off (192 MB for the terrain's textures while 320 MB of shadow maps are kept).candidatenow requiresgpu_owns(tex), the same checkfinalized_oncemakes, in the pre-sum and in the pass. Thanks for writing down the limit (a CPU upload into the old range doesn't release the pin; age or the next touch does).keep_resampled.h, in the test and in the Issues section.candidate's checks: addressed. The cheaptex->resampledcheck comes first, andgpu_writtenstays an argument for the reason you gave.gpu_ownsin both calls, and the old<= keep_finalized_maxline coming back.New since the review: I found nothing wrong in the rework.
resampledis cleared at both placesreleasedis, and the source check makes sure that stays true. The comment besidefinalized_after_resampledsays honestly that older textures walked before it were judged without it. That only keeps a little more, and the 512 MB bound still holds.Measurements ("please add measurements"): there are no Longshot measurements yet. The Measurements section still has only the Hookshot runs of the earlier version (512 MB cap, no bound), and it says so. That's why the description check fails on the
main:andthis PR:lines and the Measured box is unticked. T-LYJB doesn't fill this gap either:87610ac0, the head from before the bound. At theirRX_VK_VRAM_MB=1024this head's cap is 128 MB, not 512, so their numbers (about 4% fewer evictions over the budget, no regression) don't carry over.Verdict: the code is good to me. Only the WIP gates are left, and they are the earlier "Evidence" point:
aad9cc15or a later rebase) where the rule keeps textures, such as T-DCLN:RX_VK_VRAM_MB=3000, Dodongo's Cavern then Hyrule Field, against main andRX_VK_KEEP_RESAMPLED_MB=0. It should show the bug with the rule off first: the shadow maps'sampled EMPTYlines withRX_VK_LOG_REMADE=1, and the dark ground. Then it should show them gone, with thekept, not evictedlines, with the rule on. Give frame times from at least 2 interleaved runs each way.vk: VRAM:line and any device lost, against main.main: <hash> (<date>)andthis PR: <branch> @ <hash>lines and Longshot numbers, the Measured box ticked, a tested-configurations row for each run, andpr ready.Main has moved past the base (two menu and input commits). They don't touch these files, so no rebase is needed for review.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.