Repository navigation
feat(vk): the upscalers' texture LOD bias at Auto, from the size drawn - #1654
TwoToneEddy wants to merge 6 commits into
Conversation
While DLSS or FSR 3.1 scales the scene up, the scene's draws picked their texture mips for the smaller scene, so the upscaler had only that blur to rebuild. The scene's own mipmapped textures are now sampled with a bias of log2(scene width / picture width) mip levels, in quarter steps, as both upscalers' guides ask (gpu/vk/upscale_lod_bias.h). Only with the camera jitter on, only for the scene target the upscaler last took, only in the fragment stage, and never for what the GPU drew or for comparing samplers. A biased sampler is a sampler of its own (the bias is part of sampler_make's key), and a draw with another bias builds its descriptors again. DLAA has none: its scene is the picture's size. RX_VK_UPSCALE_LOD_BIAS=0 turns it off, a number up to 2 scales it. The log says what is applied and what bias the game's own samplers already ask for. Not run in the game: checked by tests/test_upscale_lod_bias.py and a g++ syntax pass over the changed files. Co-Authored-By: Claude <noreply@anthropic.com>
hckrman101
left a comment
There was a problem hiding this comment.
Read through the diff and the surrounding code on main. The design holds together:
- The bias only goes to the scene's own mipmapped fragment-stage textures (
!gpu_written,levels > 1), never to compare samplers or ones without a mip filter, and only for the jittered B10G11R11 scene target at the size the upscaler last took. It is 0 for DLAA. - A biased sampler has its own key in
sampler_make, with the unbiased key unchanged.sampler_forhas a separate biased table that checks the steps it was filled for. last_desc_lod_stepsis part ofsame_desc, so a draw with another bias rebuilds its descriptors, and this also holds withRX_VK_DESC_SLOTGEN. The compute path'sbuild_descriptorsis not affected because of the fragment-stage gate.- The quarter-step rounding and the 2-level clamp keep the number of samplers small.
Nits and housekeeping:
- The PR currently shows as not mergeable (conflicts with main), so it needs a rebase before it can go in.
tests/test_upscale_lod_bias.pyis picked up byrun_all.pyautomatically but runs with its default--cxx g++. Other C++ tests list["--cxx", "{cxx}"]inARGS, which would also cover a Windows runner withoutg++on PATH.- The Python test is mostly string matching on
vk_draw.cpp, so it will break on harmless refactors. The C++ unit test is the useful part. - The bias applies to FSR 3.1 via
fsr3_inputs::jitter_size. #1008's FSR 4 path doesn't set this, so it keeps the unbiased mips. That seems fine for now, but it is worth a follow-up if FSR 4 wants the same.
Not run in-game as the description says, so I haven't seen how it looks. Approving on the code.
On the frame chain (NotRexed#1008): the width the scene is scaled to comes from vk_temporal.cpp's temporal_jitter_size (axis 3) in place of dlss_jitter_size, so NVIDIA DLSS and AMD FSR 4 both take the bias, and from fsr3_inputs::jitter_size for FSR 3.1. DLAA and FSR 4's Native AA have none. tests/test_upscale_lod_bias.py is given run_all.py's compiler. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks! |
tests/run_all.py's ARGS table is retired on main: tests/test_upscale_lod_bias.py now declares its compiler itself (RUN_ALL_ARGS). Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Thanks for the review. Main is merged in, and each of your points is answered below.
Done. It is also ported to the TemporalEngine chain (#1008): the scale-to width now comes from
Fixed. The test now declares
Agreed. I've kept it in line with the other source-reading checks; the C++ unit test carries the logic.
Covered by the port. FSR 4 runs on the same chain as DLSS, so it takes the bias too (none for its Native AA, as for DLAA). Docs and readme are updated. Checked
|
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for the merge and the answers. All four points from my last review are dealt with: the test now declares RUN_ALL_ARGS = ["--cxx", "{cxx}"], the string-matching test is a fair call to keep, and the port to the TemporalEngine chain (temporal_jitter_size axis 3, set beside jitter_w/jitter_h in temporal_upscale) is right. FSR 4 now gets the bias and Native AA gets none, because rw == dw there. The new code is fine as far as it goes.
But main has moved under it again, and this time it's my fault: #1898 (merged at 19:53, after your 18:15 merge) landed an upscaler LOD bias of its own in vk_draw.cpp (upscale_lod_bias(), added in sampler_make and part of the sampler key). That's why the PR is conflicting again (vk_draw.cpp, vk_temporal.*, vk_fsr3_inputs.*, readme.md, docs/development.md). Sorry about that. A plain merge would stack two biases, so the two need reconciling first:
- Same env var, different meaning. Main's
RX_VK_UPSCALE_LOD_BIASis just "0 = off". Here it's a 0..2 strength. If this PR's strength is kept, main's readme line needs updating too. - The game's own bias. OoT already gives its world materials
lod_bias = -0.5for its 3/4 render (that's what the sampler log in #1898 showed). Main only addslog2(share / 0.75)on top of that, and only to the samplers the game biases below 0. This PR adds the fulllog2(render / picture)to every mipmapped scene texture, so those materials end up about 0.5 past the guides' number. DLSS Quality would be -1.0 total instead of about -0.58, and Performance -1.5 instead of -1.0. See inline. - What this PR adds over main. Main returns 0 at Auto (dynamic resolution) and works from the menu's share rather than the size actually drawn. This PR follows the scene's real width each frame via the jitter sizes, and that's the part worth keeping.
My suggestion: rebase onto main's upscale_lod_bias(), and change it (or replace it) to use your measured scene width / picture width relative to the game's 3/4, so that Auto is covered as well. Keep main's rule of biasing only the samplers the game biases, or say why the others (textures the game left at 0) should get it too. Then use one env var with one meaning. I'd also like @NotRexed's view on which shape he wants.
CI hasn't run here (no checks reported while it conflicts). It's still not run in the game, which matters more for a sharpness change like this one.
main has an upscaler LOD bias of its own since NotRexed#1898 (upscale_lod_bias in vk_draw.cpp: log2(share / 0.75) on the samplers the game biases below 0, from the Render resolution row's share). This branch's per-draw bias would have stacked on it, so it is gone: main's function, rule and RX_VK_UPSCALE_LOD_BIAS (0: off) are kept as they are, and what remains here is what main leaves out. At Auto the share is the game's dynamic resolution's, which the menu does not know, and main applies no bias. The upscalers now say what they drew (upscale_lod_drew, from vk_temporal.cpp's chain and vk_fsr3.cpp), and at Auto the bias is the same log2(share / 0.75) of that size, in quarter levels (upscale_lod_bias.h), with the samplers' generation moved when the step changes. A fixed Render resolution is as on main. Co-Authored-By: Claude <noreply@anthropic.com>
|
A heads-up before this gets merged: on today's main ( Cause: in Fix that works for me: close the anonymous namespace just around that one function. It still sees the file-local } // namespace (upscale_lod_drew is called from vk_temporal.cpp and vk_fsr3.cpp, so it is not in the anonymous one)
void upscale_lod_drew(uint32_t scene_width, uint32_t picture_width) {
...
}
namespace {Checked: with that change the renderer links, 🤖 Generated with Claude Code |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for reconciling it with #1898. The points from my last review are dealt with:
- Nothing stacks any more. The per-draw bias is gone, and main's
upscale_lod_bias()with its rule (only the samplers the game biases below 0,log2(share / 0.75)) is used as is. - One env var, one meaning.
RX_VK_UPSCALE_LOD_BIAS=0is off, as on main, and the 0..2 strength is gone. - Auto is covered. This was the part worth keeping. Both upscalers report the size they drew from (
upscale_lod_drewintemporal_upscaleandfsr3_upscale). The quarter-level steps fromupscale_lod_bias.hare relative to the game's 3/4, and moving to a new step bumps the samplers' generation. That looks right to me, and so do the unit tests. Keeping the fixed Render resolutions exactly as on main is fine for this PR.
Still open:
- It doesn't link. Joey-Magnus already reported this above, and it applies to this head too, not just to newer main. In
vk_draw.cppthe anonymous namespace that opens near the top of the file (line 68 here) only closes at line 3283, soupscale_lod_drewat 2193 is defined in it. Themkwvk::upscale_lod_drewthatvk_draw.hdeclares andvk_temporal.cpp/vk_fsr3.cppcall is then never defined. Main'supscale_lod_bias()sits in the same namespace, but only this file uses it. Closing the anonymous namespace around the one function, as Joey suggested, fixes it. A check intest_upscale_lod_bias.pywould also help, since none of the source-reading checks would catch this. - Conflicts again. GitHub shows the branch as conflicting with main (43 commits ahead since your merge;
vk_draw.cpp,vk_temporal.cpp,readme.mdanddocs/development.mdall moved), so it needs another merge of main. CI hasn't run beyond the title check because of that. - Title and description. You said you'd update them, but they still describe the earlier per-draw bias with its 0..2 strength and "FSR 3.1" only. Something like "the upscalers' texture LOD bias at Auto, from the size drawn" would match what's left.
One small thing, see inline: a share sitting on a quarter-step boundary could flip the step from frame to frame.
Still not run in the game, and @NotRexed hasn't said which shape he wants yet. Once it links and is merged up, I'm fine with it on the code.
| if (share <= 0) return upscale_lod::bias(upscale_lod_auto_steps.load()); // (Auto: what was measured) | ||
| return std::min(0.0f, std::log2((float)share / 7500.0f)); | ||
| } | ||
| void upscale_lod_drew(uint32_t scene_width, uint32_t picture_width) { |
There was a problem hiding this comment.
This definition is inside the anonymous namespace that opens at line 68 and closes at 3283, so it's a file-local function, not the mkwvk::upscale_lod_drew that vk_draw.h declares. vk_temporal.cpp and vk_fsr3.cpp then hit an unresolved symbol (Joey-Magnus's _ZN5mkwvk16upscale_lod_drewEjj). } // namespace before it and namespace { after it fixes this.
| } | ||
| void upscale_lod_drew(uint32_t scene_width, uint32_t picture_width) { | ||
| const int steps = upscale_lod::steps(scene_width, picture_width); | ||
| if (upscale_lod_auto_steps.exchange(steps) == steps) return; |
There was a problem hiding this comment.
Nit / worth watching in the game: at Auto, if the dynamic resolution hovers where lround flips between two quarter steps, this bumps the samplers' generation every frame or two. That empties sampler_for's tables and descriptor reuse each time. The told log stops after 16 lines, so you'd see it as a burst at start. If it happens, a little hysteresis (only move once the unrounded value is past the boundary by, say, an eighth of a level) would keep it steady.
upscale_lod_drew was defined inside vk_draw.cpp's anonymous namespace, so the mkwvk::upscale_lod_drew that vk_draw.h declares and vk_temporal.cpp and vk_fsr3.cpp call was never defined: hookshot_vk.so had an unresolved symbol and would not load. The anonymous namespace is closed around that one function, and tests/test_upscale_lod_bias.py checks that it stays outside. At Auto a size hovering where the quarter steps round the other way moved the samplers' generation every frame or two. A step is now left only once the size is an eighth of a level past its edge (upscale_lod_bias.h steps with the steps there are). Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Sorry I didn't mean to close the PR. Hopefully it's all good now |
Joey-Magnus
left a comment
There was a problem hiding this comment.
The anonymous-namespace fix around upscale_lod_drew is in, and tests/test_upscale_lod_bias.py passes on this head. The Windows part 2/2 failure is on a base from before #2397 (the test_native_astc_texture_count fix), so I don't think it's this PR.
It conflicts again because #2441 (merged 14:27) moved the rule into upscaler_rows::upscale_lod_bias_for(up, share, offset). For DLSS/DLAA it now covers Auto by assuming the game's 3/4 and adds RX_VK_DLSS_LOD_OFFSET (-1 by default). That covers part of what this PR adds. What's left is the measured share at Auto, which is worth keeping. I'd turn the measured steps into a share and pass it to upscale_lod_bias_for when resolution_scale() <= 0. Then DLSS gets log2(measured / 0.75) + offset and FSR 3.1/4 get log2(measured / 0.75), without a second Auto path inside upscale_lod_bias(). Note that upscale_lod::steps() clamps at 0 from 3/4 up, which would drop DLSS's offset if used directly. The readme line should then describe the combined rule.
|
This now conflicts with main in |
|
I think I'll leave it now, I've synced my fork and merge main so many times now and I see no indication this will ever be merged |
Main's upscaler texture LOD bias (#1898) is 0 at Auto, because the menu does not know the size the game's dynamic resolution draws. A scene the game shrank below its 3/4 under DLSS, FSR 3.1 or FSR 4 therefore lost its textures' detail there.
What this adds
upscale_lod_drew, called fromtemporal_upscaleandfsr3_upscale). At Auto the bias is main's samelog2(share / 0.75)of that measured share, in quarter mip levels (gpu/vk/upscale_lod_bias.h).samplers_changed()), after the step is stored.What it leaves as on main
upscale_lod_bias()'s rule: only the samplers the game biases below 0.RX_VK_UPSCALE_LOD_BIAS=0turns it off. There is no strength setting.The log's
vk: upscaler LOD bias at Auto:lines say what is measured.Checked
tests/test_upscale_lod_bias.py: the steps and the hold at an edge as C++ unit tests, and thatupscale_lod_drewis outsidevk_draw.cpp's anonymous namespace (it did not link before).vk_draw.cppbuilt to an object with g++:mkwvk::upscale_lod_drewis an exported symbol. The whole renderer was not linked here.🤖 Generated with Claude Code