Repository navigation
Conversation
ghost
left a comment
There was a problem hiding this comment.
Thanks, this is a nice, self-contained way to do it: width/height stay the game's and only the image and the passes over it grow, so uploads, copies and sampling mostly don't need to know. I went through targets(), the viewport/scissor/clear scaling, the bias checks, copy_t2t/clear_tex, and the image/budget sizing. At Game (factor 1) scissor_rect gives the same rectangles as the old dynamic-state scissor and clear_rect, so the refactor shouldn't change anything when the option is off.
Blockers:
- Conflicts with main. The branch is 191 commits behind.
vk_menu.cppandvk_menu.hconflict with the new Anisotropic filtering row andopts.anisotropy. Both are trivial to fix (keep both). The other four files merge cleanly. Because of the conflict, no CI has run on this PR, so nothing has built it with llvm-mingw or on Linux yet (the description only mentions MSVC, which the project doesn't ship with). Please rebase so the checks run. After the merge there are 12 restart rows againstkMax = 16, sotest_menu_restart_slotsstill passes.
Worth fixing (inline):
- The game's slope-scaled depth bias isn't scaled with the map. Details on the
gwline. - Main's VRAM compaction (
RX_VK_COMPACT=1, off by default) rebuilds images att->width/t->height. It would shrink a scaled shadow map back to the game's size whilersstays above 1. Details on theci.extentline.
Smaller things:
- A texture refused in
targets()(depth drawn with colour) keeps its larger image until the game makes it again. If something samples that depth, it reads the wrong region, because only the game-sized top-left corner was drawn. That's fine as a logged limitation for OoT, whose shadow passes are depth-only. Maybe say so in the comment, since the refusal only applies to the next texture made. - Nit:
clear_rectis now just a forwarder toscissor_rect. Its two callers could callscissor_rectdirectly. - Testing: only 4x has been played. A short run at
Gameand at 1.5x would be good, since 1.5x gives non-integer edges (768 -> 1152) and is the setting most likely to show seams from the rounding. A source test for the qualifying predicate and thescissor_rectrounding (two rectangles that meet still meet) would also fit the repo's style.
No other reviews or comments on the PR yet. Once it's rebased and CI is green, I'd be happy to look again.
| if (const char* e = getenv("RX_VK_SHADOW_BIAS")) sscanf(e, "%f,%f", &units, &slope); | ||
| } | ||
| if (units > 0 && at.n == 0 && at.width == at.height && at.width <= 768 && at.depth) { | ||
| const uint32_t gw = at.scale != 1.0f ? (uint32_t)(at.width / at.scale + 0.5f) : at.width; // (the game's size) |
There was a problem hiding this comment.
The constant bias checks use the game's size now, good. But the game's own slope-scaled bias (d.bias[2] = s.poly_offset[0] a few lines up) is in units of the depth slope per pixel. At scale k, each pixel covers 1/k as much ground, so the slope term drops by k. At 4x the cascades get a quarter of the slope bias the game asked for, which is the usual way acne comes back at higher shadow resolutions. Something like if (at.scale != 1.0f) d.bias[2] *= at.scale; (for depth-only passes) would keep the game's bias. It didn't show on your RTX run, but it's cheap to keep equal, and other GPUs' rasterizers differ.
| ci.imageType = x.type; | ||
| ci.format = x.format; | ||
| ci.extent = {x.width, x.height, x.depth}; | ||
| ci.extent = {image_width(&x), image_height(&x), x.depth}; // (a shadow map's larger image, above) |
There was a problem hiding this comment.
Once this is merged with main: compact_step (VRAM compaction, RX_VK_COMPACT=1, off by default) rebuilds a moved image "as create() made it" with ci.extent = {t->width, t->height, t->depth} and copies t->width >> l per level. For a scaled shadow map that makes a game-sized image while rs stays above 1. The next pass then gets a render area and viewport of image_width(tex), past the image's extent, which is a validation error or a GPU fault. It only applies to textures not created as targets and with compaction on, but it's a one-line fix: use image_width(t)/image_height(t) there and in the copy regions.
| } | ||
| return r; | ||
| } | ||
| VkRect2D clear_rect(const NvnState& s, uint32_t w, uint32_t h, float scale) { return scissor_rect(s, w, h, scale); } |
There was a problem hiding this comment.
Nit: clear_rect only forwards now. The two callers could call scissor_rect directly.
…e's size, shadow_res.h and its test Review on NotRexed#1892: - The game's slope-scaled depth bias is per pixel of depth slope; a map made k times larger has 1/k the slope a pixel, so the factor is multiplied by the targets' scale (depth-only passes over a scaled map only). - compact_step (RX_VK_COMPACT=1) rebuilt a moved image at the game's size: it now uses image_width/image_height for the image and its copy regions, so a scaled shadow map stays its size. - The predicate, the side and the edge rounding move to gpu/vk/shadow_res.h (no Vulkan), used by create, scissor_rect and vk_xfer.cpp. tests/test_shadow_resolution.py compiles it: OoT's maps taken, odd and small sizes left out, sides within 8192 and 1 GB, every qualifying size's whole extent mapped onto the whole image at every factor, and rectangles that meet still meeting (1.5x included). - clear_rect, a forwarder, is gone: the clears call scissor_rect. - The refusal's comment says what it does not do: the texture already made keeps its larger image. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
329271f to
fda8a1c
Compare
|
Thanks for going through it this closely. All points are addressed: the branch is rebased on main (d7a7306), the menu conflicts are resolved by keeping both rows, and the fixes are in fda8a1c.
|
|
Follow-up in cdb8791: |
|
Heads-up: this conflicts with main now. Could you rebase onto main and resolve the conflicts? Thanks! |
Joey-Magnus
left a comment
There was a problem hiding this comment.
The follow-ups to hckrman101's review look right to me: the slope-bias scale in vk_draw.cpp, compact_step building at image_width/image_height, and shadow_res.h with its test. A few things are left before this can be judged. The title isn't a conventional commit, so the only check that ran fails; something like "feat(vk): shadow resolution option" would pass. The branch conflicts with main again in vk_menu.cpp and vk_texture.cpp, so llvm-mingw and Linux have still never built it. The description has no screenshots, and this changes what's on screen; a Game vs 4x pair of the same far shadow would do. Game and 1.5x are still unplayed, and 1.5x is where the non-integer edges are. Also worth saying in the description: a buffer copy of a scaled map only gets its top-left corner (the warn in copy_b2t), so the option is shaped around how OoT uses its maps. The default stays Game, so nobody's picture changes until they opt in.
OoT's shadow maps are coarse on a big monitor: the characters' are 768 px or smaller, and the sun's three cascades 1600 x 1600 (D16, docked), each farther one spreading those pixels over more ground. A Graphics row, Shadow resolution (Game, 1.5x, 2x, 3x, 4x; shadow_resolution in the options file; takes a restart; outside the Quality preset) makes their images larger: - gpu/vk/shadow_res.h (no Vulkan): which depth textures are shadow maps (square, 2D or 2D array, one level, single sample, 256 to 4096 a side and a multiple of 32), the side (8192 and 1 GB at most), and the edge rounding - vk_texture.cpp create makes the image larger (Texture::rs; width and height stay the game's, so uploads, copies and read-backs address the game's texels inside the image); the budget counts the image; compaction keeps its size - vk_draw.cpp: a depth-only pass over such a map is the image's size, the game's viewport, scissor and clears scaled (scissor_rect); the game's slope-scaled bias scaled with it; the character and cascade bias checks on the game's size; a size drawn with colour targets is not a shadow map and is made at the game's size from then on - vk_xfer.cpp: region clears and texture-to-texture copies scaled; a buffer copy of a scaled map gets its top-left corner (warned) tests/test_shadow_resolution.py compiles shadow_res.h; test_vulkan_clear_mip's stand-in Texture has rs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cdb8791 to
5e94a6a
Compare
|
Rebuilt on main at 51da046 (5e94a6a), and the title is now a Conventional Commit. Changes from the rebase:
The whole local |
Joey-Magnus
left a comment
There was a problem hiding this comment.
The title, the rebase and the description are done. 5e94a6a merges cleanly with current main, and CI is green, llvm-mingw and the AppImage included. I also read the row's placement under Detail (it's preset_exempt) and the budget change in create(). At Game, shadow_resolution_factor() is 1 and qualifies() is never reached, so nobody's picture changes.
Two things from my last review are still open, as the description says:
- There are no screenshots yet. This changes what's on screen, and CONTRIBUTING asks for them. A Game vs 4x pair of the same far shadow would do.
- Game and 1.5x haven't been played on this head. 1.5x is where shadow_res::edge rounds to non-integer texels (768 to 1152), so it's the setting most likely to show seams, even though test_shadow_resolution checks that the edges meet.
Once those are posted, I'd be glad to see this go in.
… for none Upstream NotRexed#1892's shadow-map detection (square depth targets of one level, 256-4096) scales them up; this lets the scale go below 1 too (a multiple of 32, 256 a side at least). On an AMD integrated GPU with the Shadow resolution row at Game the sun's cascades are drawn at 800 instead of 1600 a side and the characters' at 384 instead of 768: a quarter of the pixels. LAKUEN_SHADOW_SCALE=<0.25-4> sets the scale anywhere (1: the game's). LAKUEN_SHADOWS=0: the depth-only passes over shadow maps draw nothing, the maps keep their clear, no shadows and their draws saved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C7fdTPXbwDRohew9qanvbp
This adds a Shadow resolution row to the Graphics tab: Game, 1.5x, 2x, 3x or 4x. It is saved as
shadow_resolution, applies after a restart, and sits under Detail, outside the Quality preset. The default is Game, so nobody's picture changes until they opt in. It's a standalone PR onmain(rebuilt on 51da046).Why
OoT's shadow maps are fairly coarse on a big monitor. The characters' maps are 768 px or smaller. The sun's three cascades are only 1600 × 1600 (D16, docked), and each farther cascade spreads those pixels over more ground. Shadows lose detail with distance, and edges show stair steps and shimmer.
How
Everything happens in the renderer; the game isn't patched.
gpu/vk/shadow_res.h, pure, no Vulkan): the predicate, the side and the edge rounding, used bycreate,scissor_rectandvk_xfer.cpp.vk_texture.cpp,create): textures that look like shadow maps get a largerVkImage. That means square depth textures, 2D or 2D array, one level, single sample, 256 to 4096 px and a multiple of 32. The multiple of 32 leaves out the odd sizes that follow dynamic resolution. Each side is capped at 8192 px and each image at 1 GB.Texture::rsstores the scale, and the VRAM budget counts the larger image. VRAM compaction (RX_VK_COMPACT=1) rebuilds a moved image at its image size, so a scaled map stays scaled.widthandheightstay at the game's size, so uploads, buffer copies and read-backs still address the game's texels, inside the image.vk_draw.cpp):targets()sizes a depth-only pass over such a texture to its image and setsTargets::scale. The game's viewport, scissor and clear rectangles are multiplied by that scale. The draw's scissor and both clears usescissor_rect, which rounds each edge to the nearest texel (shadow_res::edge), so rectangles that meet still meet. At Game it gives the same rectangles as before.vk_xfer.cpp):copy_b2t). OoT never copies its shadow maps to buffers, but this is why the option is shaped around how OoT uses its maps, not a general texture scale.A depth texture that qualifies but is drawn alongside colour targets isn't a shadow map. That pass keeps the game's size, and the texture's size and format go on a refusal list (
shadow_res_refuse), so the texture is made at the game's size when the game makes it again. The texture already made keeps its larger image, so anything sampling that depth would read the wrong region until then. It's a logged limitation: OoT's shadow passes are depth-only, and it hasn't happened.Cost
The cascades take about 15 MB at the game's size, 61 MB at 2x (3200² × 3) and 246 MB at 4x (6400² × 3). The characters' maps cost little at any setting.
Testing
vk_draw.cpp,vk_texture.cpp,vk_xfer.cppandvk_menu.cppcompile with clang usingnx2/build.py's renderer flags. The full game builds with MSVC as part of the combined build, which has the same change. llvm-mingw and Linux need CI.tests/run_all.py, the whole suite, locally (clang targeting MSVC, the CPM Vulkan headers, glslang 11.12): 433 passed. The failures are exactly the ones main at 51da046 has on the same machine, all down to the local toolchain. Of these:tests/test_shadow_resolution.pychecks the predicate on OoT's sizes and the odd ones, and the 8192 and 1 GB caps. It also checks that every qualifying size at every factor maps the game's whole extent onto the whole image, and that edges stay in order and shared, so rectangles that meet still meet (1.5x included).test_preset_exempt,test_menu_restart_slotsandtest_native_astc_texture_countpass too.🤖 Generated with Claude Code