Repository navigation
fix(vulkan): preserve guest mip levels in partial clears - #1084
Conversation
hckrman101
left a comment
There was a problem hiding this comment.
Looks right to me. resolve() already turns the guest level into the physical one (s.level = image_level(...)), and texture_view() runs image_level() on base_level again, so on any texture with skip > 0 the partial-clear path was clearing the wrong level: level - skip clamped to 0 or one too low. Meanwhile renderArea was still sized from s.level, which on the coarser mip runs past the attachment's extent. Adding skip back gives the round trip image_level(s.level + skip) == s.level. resolve() has already rejected levels outside [skip, skip + levels), and since clear_tex doesn't pass scaled_ok, s.down is always 0, so that holds. vkCmdClearAttachments is the only caller with this pattern.
What else I checked: the whole-level path (vkCmdClearColorImage/DepthStencilImage) already used s.level directly and is unchanged. Everything stays in GENERAL with LOAD/STORE, so other mips and layers aren't touched or discarded. The fix doesn't depend on aspect, so depth/stencil partial clears get it too. Cost is the same.
The test checks out as well. The old code gives mip 0 instead of 1 for run(1,2,1,false), so the test actually catches the bug, and the ARGS entry in run_all.py is there with {cxx}/{vulkan_headers}, so it gets skipped cleanly when the headers aren't available.
Non-blocking:
- #1083 adds its ARGS line at the same spot in run_all.py (same
@@ -30,6 +30,7hunk), so whichever one lands second needs a trivial rebase. - Pre-existing and out of scope here: for 3D textures, clear_tex always goes down the region path with
s.layer = 0, s.layers = 1and never usess.z/s.d, so a partial or whole clear of a 3D texture only hits slice 0. Could be a follow-up.
| void texture_written(Texture* t) { t->gpu_written=true; } | ||
| ''' + "\n".join(block(texture, m) for m in ("unsigned view_level(", "unsigned image_level(", "unsigned view_layer(")) + r''' | ||
| VkImageView texture_view(Texture* t,const nvn_tex*,const nvn_view* view,VkImageViewType,ViewUse) { | ||
| selected_mip=image_level(t,view_level(view)); first_layer=view_layer(view); |
There was a problem hiding this comment.
nit: this stub copies the level mapping from the top of the real texture_view() instead of exercising it. That's fine for now, but if texture_view ever changes how it maps levels (e.g. stops subtracting skip), this test will still pass. A short comment saying it mirrors texture_view's level0 computation would help whoever touches it next.
Update
Rebased this change onto
origin/mainat8b2e1d3b895a5f890dcb886d904b62d6b38df2c7.A partial clear resolves a guest mip to its physical image mip. The attachment view takes a guest mip and applies the skipped-level offset itself, so passing the already-resolved physical mip selected the wrong level. The partial-clear path now restores the guest mip before creating the view. Whole-level clears are unchanged. This change is independent of the buffer/image copy-span fix.
Validation
clang++is unavailable. The new fixture passed with GCC AddressSanitizer and UndefinedBehaviorSanitizer.hookshot_vk.dllbuild passed: 276 seconds, zero compilation failures.origin/mainrun reported 115 passed, 7 failed, 12 skipped. The same seven checks failed on both:nx2/test_nup_verify.py,nx2/test_nup_codec.py,nx2/test_mesh_first_types.py,nx2/test_nup_paths.py,tests/test_fps_overlay_order.py,tests/test_precompile_compute_guard.py, andtests/test_program_answer_domains.py. The added regression test passed; no additional failure was observed.The fixture uses synthetic texture objects, instrumented view mapping, and Vulkan command spies. It does not execute a Vulkan driver. No real-GPU or in-game test was performed after the rebase.
This resubmission replaces PR #754, following the maintainer's request to rebase and reopen it. GitHub refused to reopen the closed original PR, so this contribution is submitted as a new independent PR.