Repository navigation
nvn: preserve image writes from unchanged attachment stores - #730
thehobbit85 wants to merge 2 commits into
Conversation
2e672b7 to
2488267
Compare
…raw's colour writes read before the stage loop stops Our rule in 9e02b67 filled the fragment shader's colour mask inside the stage loop, which stops at the first stage with a writable binding: with a storage-writing vertex shader the mask stayed 0, the draw did not count as rasterizing, and its colour store was dropped (check_t_rast_discard failed). PR seedds#730's version reads the mask first, records physical attachments before a merged pipeline's logical-output map is reset, and holds with the store optimization switches off. Its test t_image_attachment_store comes with it. The primitive ID guard of 9e02b67 stays. Tested on DARK SOULS: REMASTERED by the user; the native graphics suite passes. scripts/regress.py has not been run on other games.
14aa626 to
3167343
Compare
3167343 to
0e16986
Compare
0e16986 to
fe348ce
Compare
Separate possible image writes from raster writes in attachment tracking. Suppress the tile store for loaded color attachments written only through images, while preserving raster/clear and depth/stencil store decisions. Capture physical output facts before merged-pipeline mapping is reset. Include GPU readback controls for direct and deferred encoders, sticky raster writes, clears, specialization and the optional store-saving switch. Based on Jacob-Tate's diagnosis and initial separation in #187. Co-authored-by: Eliran Zach <4012041+thehobbit85@users.noreply.github.com>
|
Thanks, Eliran. Landed the reviewed image-write fix on Also landed the Xcode 27.2 / Metal 32023.922.4 pin and idle-MSBuild measurement fix, plus a Python 3.15 AST-fixture typing repair. Both author and committer use the project identity. The integrated shader chain is Design review is clean. Confidence is high for the loaded-color/image-only case: this corrects the attachment-store decision at its root, applies generally, and adds no game-specific rendering workaround. Fresh checks on the landing tree passed:
The compiler change has uncertain reach, so earlier shader comparisons covered five profiles: Breath of the Wild, Scarlet and Tears of the Kingdom for NVN submit/present, Wonder for bindless/AGL paths, and NieR for another engine and attachment-store coverage. Across 95,896 shader rows, there were no new build failures, MSL/reflection changes or interface mismatches. TotK's single raw Gameplay/performance validation remains incomplete: the earlier pin-only BotW run flagged p99 17.62 → 33.52 ms, and the screenshot-free control remained variable. Concurrent work is still active on this Mac, so no clean performance claim is made. Dark Souls gameplay, physical iOS rendering, and mixed raster/image writes remain outside the verified scope. Closing as landed. |
NVN clears all layers of the bound render-target view. A load-action clear in a non-layered Metal pass covered only its first layer, leaving the remaining shadow-cube faces and array layers unchanged. Extend the existing standalone clear pass to cover the view's full layer range before loading it in the draw pass. Extend texraw diagnostics to save every array slice and cube face, and test the production replay. Integrate with #730 by marking a raster clear only when the draw pass's actual load action clears. A clear already completed in a standalone pass must not keep a stale tile store over later shader image writes. The literal GPU regression fails four checks without this tracking fix. Before rebasing, the full Metal graphics suite, LLVM 23 lint, all 11 documentation tests and Breath of the Wild's full outdoors route passed. Those comparisons used the documented Xcode 27.2 compiler override on both revisions against the old compiler lock. After rebasing onto the Xcode 27.2 pin and #730, focused lint and documentation checks pass, as do 120 image-store checks and 30 layered-clear checks under Metal validation. The maintainer stopped the remaining Tears of the Kingdom and Pokemon Scarlet regression runs; document that partial coverage. Co-authored-by: Riski Aditiawarman <62930684+keanu-stitch8@users.noreply.github.com>
What and why
Dark Souls' image-only ambient-occlusion draw writes layers of the texture also bound as its color attachment. Marking every attachment written for a possible image alias retained the tile store, which overwrote the image writes with the loaded pixels. Track possible shader writes separately from color-tile writes and suppress that store only for loaded color attachments that had no raster output or clear.
This correctness rule stays active with optional unwritten-store savings disabled. Physical color-write facts are captured before a merged pipeline's logical-output mapping is reset, and specialized fragments use conservative unknown outputs. Depth/stencil, non-loaded attachments and raster/clear writes retain their store decisions. A prior raster write remains recorded across later image-only draws.
Credit: Jacob-Tate diagnosed this in #187 and published the initial separation in 34c7453. This change implements only the remaining attachment-store branch and preserves main's existing PrimitiveID handling.
Addresses the image-store corruption reported in #187. This PR does not claim to resolve every Dark Souls rendering issue.
Current maintenance and validation (2026-10-31, 22:00 UTC pass)
Rebased onto main
5474c6f34789a41770868564fbf4e9e32fc57584, producingfe348cea25f6fba45b0b06bebce2decfa1864c03. Both feature/ownership commits are identical by range-diff. Main's guarded jump-table recognition, audio callback diagnostics and benchmark archive changes do not implement separate image-write and raster-write attachment tracking, so this PR remains relevant. The shared polling fixture is inherited from main; no redundant prerequisite remains.Fresh verification on this exact head: 60 jump-table, benchmark and documentation tests passed; the freshly built native audio-statistics fixture passed 22 checks.
git diff --checkpasses. All discussion comments, reviews and inline threads were read; no feedback is outstanding.Earlier native Metal/image-store/host and movie validation remains historical. Graphics, shader-chain and feature inputs are unchanged, so those expensive suites were not repeated. Fresh harness discovery found a Legends: Z-A build with no smoke route or baseline; no game was launched, cache rebuilt or fresh gameplay/device result claimed.
CI is pending for exact published head
fe348cea25f6fba45b0b06bebce2decfa1864c03. Earlier-head results are historical.CI history: the old
aa6c5f7host-test attempt exposed the shared wall-clock fixture assumption. Controlled delays reproduced it; the test-only repair retained production thresholds and real wake paths. Upstream rerun permission was unavailable (HTTP 403), so the repair was included as a prerequisite and passed CI on14aa6263. The maintainer has now incorporated that exact repair in main and closed #779; this head inherits it from main. Both the prior failure and old successful CI are historical.Historical verification through 3775e55
All following checks and availability statements describe the named earlier revisions.
Earlier maintenance and validation
Rebased onto main
635e8008d67053214ddd302a0196f5ec971b63ea, producing3775e5507478a0db990ecf1df157d4f039f58316. Main still lacks separate image-write and raster-write attachment tracking, so this change remains needed. Its new attachment-capacity/depth-stencil fixes and Metal 4 function-constant specialization are retained.The two conflicts were resolved by combining the PR's
img/rastfields with main's ten-recordSD_ATT_MAXtracking, and retaining both the attachment-capacity and image-store test targets. Range-diff differs only in that integration context; the reviewed, line-specific ownership annotations are unchanged. Independent review found no actionable issue.Fresh verification on the published head (Apple M2 Max, macOS 26.6):
make -C dm/gfx/tests GAME=01004AB00A260000 -j2 run, exit 0. This includes all eight attachment-capacity compiler/TBDR/encoding configurations (47 checks each), 106 image-store checks, 66 unwritten-store checks, and the current pipeline/depth-stencil tests. Optional game-derived reflection samples remain unavailable.git diff --checkpasses.d1043b780a74and its compiled outputs were reused. Z-A has no smoke route, so this is shader/interface coverage only, not gameplay.t_pollwait.cnullable dereference, now fixed on main by373cc820; no lint baseline was weakened for this PR.Dark Souls gameplay and iOS rendering remain unverified. Mixed raster/clear and image writes to the same subresource remain outside the proof and retain their store. RePOP's newly recovered build has no profile on this PR or main; its separate validation belongs to #684.
regress.py chain (static): PASS
Baseline
635e8008d67053214ddd302a0196f5ec971b63ea635e800, candidate3775e5507478a0db990ecf1df157d4f039f583163775e55; Apple M2 Max, macOS 26.6, Xcode 27.0; 107.6 s0100F43008C44000LegendsZA
0100F43008C44000: PASSEarlier validation
On
98b99591againstf11018a1, the full graphics suite, direct/deferred image-store fixtures, 29 Python tests and 28,603-shader comparison passed. The original negative control lost the attached layer's writes while other layers and raster controls passed (two failures in 50 checks). Earlier Dark Souls analysis and issue attribution remain described above; these maintenance runs do not establish new gameplay results.Docs
docs/reference/rendering.md; symptom index and switch documentation updated.