Repository navigation
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
approved these changes
Oct 23, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head 5fb1167. The fallback is narrowly gated to the supported full-byte replace case, preserves the pre-draw stencil for uncovered/depth-failed pixels, and rejects partial masks and the documented unsupported state combinations. The added runtime fixtures exercise varying output values, scissor preservation, depth failure, and explicit refusal; reported Windows validation is detailed and includes the dependent independent-blend PR. I did not run the GPU tests locally. I found no blocking issue in the reviewed diff.
This was referenced Oct 23, 2026
Contributor
|
Intel run: a device with native stencil export, so this covers the other side from the RTX 4070 Ti SUPER. Setup: Intel Arc Graphics (Meteor Lake,
AI-assisted: yes (Claude Code) |
Ondra97
marked this pull request as ready for review
October 24, 2026 04:40
Ondra97
force-pushed
the
codex/feat-stencil-operation-attachment
branch
from
October 24, 2026 14:39
6c5b6b6 to
d55607d
Compare
MarcZX10
suggested changes
Oct 24, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Blocking Vulkan feature omission in the new attachment fallback: ValidateStencilAttachment accepts the path without checking independentBlend, while VulkanDevice.cpp leaves enabled.independentBlend false. The fallback then sets masked->blends[7].colorWriteMask to VK_COLOR_COMPONENT_R_BIT and submits the full attachment-state array through Pipeline.cpp. This differs from the other attachment states in ordinary draws, so pipeline creation violates Vulkan VUID VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605 when independentBlend is not enabled. The Khronos spec says all pAttachments elements must be identical without that feature: https://registry.khronos.org/VulkanSC/specs/1.0-extensions/man/html/VkPipelineColorBlendStateCreateInfo.html
Please either query and enable independentBlend when supported and reject this fallback on devices where it is unavailable, or restructure the fallback so the color blend attachment states remain identical. Add a validation or regression test for the unsupported-feature case; this path currently has no feature guard. The existing hosted checks are policy/convention checks; graphics build and device execution are skipped for this path, so they do not exercise pipeline creation.
Ondra97
force-pushed
the
codex/feat-stencil-operation-attachment
branch
from
October 24, 2026 17:13
d55607d to
7bc0471
Compare
Ondra97
force-pushed
the
codex/feat-stencil-operation-attachment
branch
from
October 24, 2026 18:13
7bc0471 to
3167887
Compare
github-actions
Bot
added
needs rebase
Ondra97
force-pushed
the
codex/feat-stencil-operation-attachment
branch
from
October 24, 2026 20:03
3167887 to
be104da
Compare
|
Tested this head ( 🤖 Generated with Claude Code |
MarcZX10
approved these changes
Oct 24, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed updated head be104dae9cd53deed979cffe1699c2e111f136e3. The new independentBlend check is wired from the enabled Vulkan device feature into Context and rejects the attachment fallback before pipeline creation when the feature is disabled; the graphics regression covers both rejection and acceptance cases. This addresses the VUID blocking issue from my prior review. The PR reports the focused Windows/Vulkan test set and dependency-composed fallback cases passing with validation enabled and no VUID- lines. I verified conventions against current main, git diff --check, and a strict main-sync preview: the PR is 5 commits behind and 2 ahead, and the merge-tree is clean. The feature-enabling dependency #6782 is still open and current main does not enable independentBlend, so merge this after #6782 and refresh the base as needed. The Demon's Souls run on this head proceeds past stencil export and later reaches a write fault; that is separate evidence and does not change this review.
Contributor
|
In-title datapoint (PPSA20397 / Assassin's Creed Shadows, RTX 3070, MinGW Release). Integrated this head onto our local stencil-replay follow-up chain and ran the ACS dump. Shader Integration note: cherry-picked onto a base that also touches |
Ondra97
force-pushed
the
codex/feat-stencil-operation-attachment
branch
from
October 25, 2026 13:58
be104da to
0a12832
Compare
12 tasks done
12 tasks done
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 484 tests)Skipped (49)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
12 tasks done
Contributor
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 495 tests)Skipped (49)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
12 tasks done
12 tasks done
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 517 tests)Skipped (52)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
|
Windows / NVIDIA re-run of Setup: Windows 11 Pro 26300, RTX 4080 SUPER, driver 621.01, WinLibs GCC 15.2.0 r7, Release,
Linux (WSL Ubuntu 22.04, GCC 11.4, both arms, 3 runs each): GPUs tested: NVIDIA RTX 4080 SUPER, driver 621.01, Windows 11 Pro 26300. AI-assisted: yes (Claude Code) |
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 517 tests)Skipped (63)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (13 IDs in 549 tests)Skipped (53)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (6, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 549 tests)Skipped (65)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
Author
|
@MarcZX10 The PR description now records a refresh to main |
Contributor
Author
|
The The failed #10351 macOS x86_64 job tested head I built current main Build passed, all 15 selected Python-backed tests were registered and passed, with no CTest skips. The two fixture halves took 6.13 s and 6.06 s. I also instrumented the reviewed original and split fixture on that same binary: each covers 168 successful module conversions and 48 individual rejection/no-output checks, with identical input/result/output-artifact multisets. The Linux-only I compared the complete base/head diffs of the existing alternatives:
Recommended order: use main containing merged #15526, then run a fresh supported Build workflow for #10351. No new patch is needed for this fixture blocker. I did not run macOS locally or the current-main + #10351 composition, and the old failed check remains failed. My upstream permission is READ, so a maintainer with write access must start the new Build run; rerunning the old run would retain its old main revision. This evidence closes the fixture investigation, not the outstanding fresh CI verification. |
12 tasks done
Contributor
Author
|
Refreshed onto main d63781e with ordinary merges, retaining the unchanged feature patch. Submitted head: 949480f. The exact main -> #6782 4433a62 -> submitted-head composition is 937f63736d155e80f9b3221838862b390a645c77. Fresh focused Windows/RTX 4070 Ti SUPER validation: main 14 passed/1 skipped, standalone 15/5, composition 18/2; no failures or VUIDs, with the same 20 existing layered-vertex warnings and the same loaded validation/OBS/Overwolf/NVIDIA layers. All four dependency-only attachment cases actually execute and pass in the composition. The standalone missing-independentBlend control passes before colour/stencil writes. Both existing guest_none_relocations halves pass (2/2). Cache format 57 rejects main's 56. The dependency remains #6782; the following #11551 composition must advance to 58 absent an intervening schema change. Exact commands, overlap limitations and untested stacks are recorded in Tested. These are local results; no new hosted-CI success is claimed. AI-assisted: yes. |
Contributor
❌ NVIDIA GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
agc_driver_submit_progress: Failed (also failed when re-run alone), assertion and diagnosticsagc_submit_progress: Failed (also failed when re-run alone), assertion and diagnosticsUnstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (9 IDs in 592 tests)Skipped (55)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Validated but not compared (6, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 556 tests)Skipped (72)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A pixel shader exporting a stencil operation value can be unsafe to replay on a device without native stencil export. Support the bounded full-byte replace case after late depth/stencil tests: copy existing stencil into an R8_UINT attachment at unused colour location 7, execute the draw once and copy the result back while retaining the depth owner. Native export and bounded reference replay remain supported; unsupported masks, tests, stage/layer/sample configurations and attachment collisions throw.
The fallback requires enabled
independentBlendfrom #6782. The four dependency-only attachment cases skip without it; a separate runtime control verifies refusal before colour/stencil writes. The order remains #6782 -> #10351 -> #11551.This refresh merges main
d63781e9ce826dd5445b35b91960bb576f4834aethrough ordinary merges, without changing the feature patch from original headeee4853c161beac518e433a017b405f02c7ad76f: its complete added/removed lines relative to the respective main bases are identical. Main's cache format remains 56; the additionalstencilAttachmentExportfield uses 57 and rejects 56. Ordinary draws retain main's shape lookup, while attachment export uses the complete pipeline lookup. No new shader semantics or additional compatibility changes are introduced.Tested
November 4, 2026, fresh local validation:
d63781e9ce826dd5445b35b91960bb576f4834ae.949480fb1a97891558ee1b57daa1aef95fa6f0cc.4433a62b3bf6248df830994a255701c72581cc95, still open.937f63736d155e80f9b3221838862b390a645c77, built in the order main -> exact fix(agc): enable independentBlend for color targets with different blend states #6782 -> submitted head. Its tree differs from the submitted head only by fix(agc): enable independentBlend for color targets with different blend states #6782's twoindependentBlendlines inVulkanDevice.cpp. fix(agc): enable independentBlend for color targets with different blend states #6782 is not included in the submitted branch.Windows 11 Pro 10.0.26300; Ryzen 9 7900X; 31.7 GiB RAM; WinLibs MinGW-w64 GCC 15.2.0 r7 UCRT; Ninja Release, full runtime configuration with
BUILD_TESTING=ONandANYPS5_ENABLE_SPIRV_TOOLS=ON. Every GPU run selected the NVIDIA GeForce RTX 4070 Ti SUPER, driver 621.01 (32.0.16.2101), with Khronos validation API 1.4.335. The same focused targets and serial verbose pattern ran on all three sources with required Vulkan, shader cache disabled, explicit layer path, loader layer logging andAPS5_VERIFY_SHAPE_MEMO=1. Cache fixtures enable their own isolated cache to test the codec and persistence. The logs also showVK_LAYER_OBS_HOOK,VK_LAYER_OW_OBS_HOOK,VK_LAYER_OW_OVERLAY,VK_LAYER_NV_presentandVK_LAYER_NV_optimusinserted in all three arms despite the requestedVK_LOADER_LAYERS_DISABLE='*implicit*'setting; these actual layer conditions were kept the same.The standalone missing-feature control executes and verifies unchanged colour/stencil memory; all four dependency-only cases skip. With #6782, all four actually execute and pass: varying stencil values, scissor preservation, depth-fail preservation and partial-mask refusal. Native stencil export skips on this GPU in every arm; the missing-feature control skips only in the enabled-feature composition. Both two-device pending-store cases pass.
Each log confirms actual GPU selection and validation-layer insertion. There are no missing requested layers,
VUID-diagnostics, validation errors or sync hazards. Every arm retains the same 20WARNING-VkImageSubresourceRange-layerCount-compatibilitydiagnostics from the two layered-vertex cases.agc_shader_disk_cachereruns the rejection of format 56, artifact/resource-plan round trips, corruption, persistence and process-loading cases; pack ownership, long paths and host stderr also pass. The previous negative control onfa4267e6859e44ecf4724d947c357e97d3090860, retaining colliding format 56, failed with exit 1:a stencil artifact with the upstream resource plan format loads. That historical negative control is reused only for its unchanged codec and regression: codec blobd2020f11620dcb55b4f430e950569d092e08233dandverifyEntryRoundTripare unchanged in the submitted head. The current main, standalone and composition results above are new runs.Both existing
guest_none_relocationshalves are registered with their existing 20 s limits and passed on the submitted head (2/2, no skips). This verifies the refresh contains merged #15526; the earlier macOS timeout investigation is documented in the existing comment. No new fixture audit or fixture patch was added.Reproduction, using the exact source revisions above and the same build configuration for each arm:
Whitespace, the verified main convention checker and strict main sync pass (13 ahead, 0 behind the exact main). A fresh
tools/pr_overlap.py --branch codex/c012-stencil-refresh-20261104snapshot against this main reports 107 shared-file PRs: 14 line-overlap reports and 93 conflicting-base comparisons. Its exit code is 2, not a clean automated result; the stencil/cache intersections were compared manually. No second attachment fallback implementation was found.The material intersections remain distinct: #6858 adds cache-header inspection helpers; #9897 changes vertex buffer extents and the draw-recording guard; #11551 adds the depth-bounds prologue and its serialized fields. #9897 must retain the attachment's
!stencilAttachmentguard when integrated. #11551 must retainSelectStencilExportModeand pass its extra allocation argument afterstencilReplay. Both #9897 and #11551 currently also use 57 for different schemas; those layouts must not be combined or merged after this one without advancing the format and retaining rejection tests. The previously agreed #10351 -> #11551 composition needs 58, assuming no intervening schema change. Other reported schema branches likewise require a fresh format choice when integrated. None is merged or runtime-tested here.Full CTest, new title/console runs, another GPU and the combined #11551/#9897 stacks were not run. Console behavior remains unmeasured as recorded in technical debt. Hosted checks for the new pushed head are separate from these local results; the exact enabled-prerequisite Build composition still needs a maintainer if it has not been started, since this account has READ upstream access.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)