fix(vk): preserve the picture display aspect when resizing #1339
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
No milestone
No assignees
1 participant
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1339
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/ultrawide-presentation-aspect"
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?
When the host supplies an ultrawide camera aspect, presentation fills the entire
window even after its shape changes, distorting the picture. Fit the intended
display aspect into the current destination instead: a 43:18 picture fills
1720x720, but occupies a centered 1920x804 rectangle in a 1920x1009 window.
The explicit aspect describes the picture after crop, independently of texture
allocation. Absent or invalid overrides retain the physical-crop fallback.
Normal and interpolated presents share the rectangle; the scaling filter receives
its dimensions and the existing clear covers the bars. No camera, host/backend
ABI layout or overlay-coordinate change is introduced by this patch.
Current base and CI
Rebased cleanly on main
7eac5bf225d20124c416907584752953363b8f6basa897d485b62ecf7658b8591ca7441d89226ac0fe, including the upstream securityhardening. The presentation implementation is unchanged from the reviewed patch;
the public diff remains 9 files, +468/-7. Upstream host/NVN, loader and workflow
changes are preserved.
All three applicable workflows passed in the fork on this new head, using
unchanged current upstream workflows through
workflow_dispatch:Source regressions:
Ubuntu 24.04: 204 passed, 0 failed, 6 skipped; Windows 2022: 193 passed, 0 failed, 17 skipped.
Native MSVC checks, SPIR-V fixtures, native F2I and actionlint passed.
Windows builder:
package/setup and Vulkan backend passed.
Linux AppImage: passed.
Upstream PR workflows remain
action_required, with no jobs started, pendingmaintainer approval. Fork results do not claim those upstream runs passed.
Release-only jobs were skipped normally. The optional DLSS SDK workflow is
outside this PR's changed paths and was not manually dispatched.
Validation
Production regression before/after, 512 size/aspect combinations, invalid and
extreme values, consumer-command fixtures and three detected mutations.
Repeated on the new head: four focused checks, seven real-Vulkan offscreen cases
(13,138,044 readback pixels), and synthetic Win32/swapchain resize/restore
(32 frames, 42,685,440 readback pixels), all passed. Every bar is checked; only
the linear-filter footprint inside the padded crop is excluded.
Local strict comparison: main 188 passed/4 failed/16 skipped; candidate 189
passed/the same 4 failed/17 skipped. These local tool/fixture failures did not
reproduce in the Windows CI above. The new backend also built locally.
Historical in-game A/B on
9c74f8f: identical host, image, initial configurationand cloned save; only the Vulkan DLL differs. Base stretches after resize;
candidate preserves the picture and restores. A 16:9 control, Insert interaction,
real 2x interpolation and sharpness 50% passed short resize/restore checks.
Historical integration on
0828863: renderer22eabcband a rebuilt compatibleABI 10 host loaded the Link-house checkpoint with actual FG2x/RCAS50% and
1720x720 → 3440x1369 → 1720x720. Side bars and clean restore were observed.
The completed run used a fresh profile with
RX_VK_NO_PREDICT=1,RX_VK_NO_WARM=1,RX_VK_PRECOMPILE=0; a previous cache-seeded startup wasinconclusive at Starting the game, with cause unestablished.
Those game runs are historical evidence. They do not runtime-validate the newer
security-hardening changes to the host/NVN tables on
7eac5bf, default prewarming,or a new causal A/B. Current-head evidence is the source/build/CI and synthetic
GPU/WSI checks above.
Scope
Short checkpoint coverage; no full-campaign, FOV or HUD-placement claim. Linux
source/build and native MSVC checks passed in CI; Linux game/GPU and DLSS runtime
were not tested. No performance, interpolation-quality or native image-quality
claim. No game files, generated game code, binaries, caches, saves or private logs
are included.
AI assistance: OpenAI Codex.
Current-head validation
Current head
ca495e9, based on main9d6936a. Earlier local/runtime results describe their recorded source hashes.Main advanced with renderer source changes. The standalone contribution patch is unchanged; exact-head build/source checks are refreshed below. Earlier runtime evidence remains tied to its recorded hash.
Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.
Looks good. Checked present_image against main (the PR is 14 commits behind but merges cleanly, and the vk_replay.cpp hunk still lines up).
The old
aspect_ratio() > 1.0f-> full-window stretch is replaced by a fit of the host's display aspect, with the physical-crop fit as fallback for 0/NaN/inf/negative. The overflow-safe ratio compare and the half-pixel rounding are right, and normal, filtered (framegen_scale at rect size) and interpolated presents all share the same rect, so the bars/clear logic stays consistent.Overlap: #1224 is already merged, and on main
hos_aspect_ratiois now also set for 5:4..16:9 screens. The old> 1.0fcheck stretched those too, so this PR is the right behaviour for the new narrow shapes as well (and display.c's default window shape already uses> 0). #1290 (32:9) touches vk_menu/nn_applet only, no file overlap; the float 32:9 ratio test covers the exact-fit case. The rx/WebGPU backend has its own present_rect and isn't touched here, which is fine.Tests are registered in run_all.py (the Vulkan one is correctly in the GPU skip list). Nits only: the two consumer tests splice vk_replay.cpp by text markers (
const PresentAspect aspect{...if (src && framelog_on())), so a refactor of that block will break them loudly rather than silently, which is OK but worth knowing; and the upstream CI hasn't run on this PR yet (fork runs only), so it still needs a maintainer-approved run.Re-checked after the rebase onto current main. The PR is now a single commit on top of main, and the nine files it touches have the same content I approved at 3b7b3c5. The only differences in those files are main's own changes (API version, run_all.py neighbours) coming through the rebase. The new tests are still registered in tests/run_all.py. It merges cleanly with main (GitHub reports it mergeable; main's only newer change is readme.md, which this PR doesn't touch). Approval stands.