Skip to content

fix(agc): accept additional color comparison image forms - #2092

Merged
adriadam10 merged 5 commits into
boykopovar:mainfrom
authurtabo-byte:fix/agc-color-compare-forms
Oct 10, 2026
Merged

adriadam10 merged 5 commits into
boykopovar:mainfrom
authurtabo-byte:fix/agc-color-compare-forms

Conversation

@authurtabo-byte

@authurtabo-byte authurtabo-byte commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What

Accept the existing cube-to-2D-array runtime mode for color comparison, and classify 8-bit sRGB formats with the existing UNORM reference clamp. MiniGolf (PPSA03647) uses an sRGB cube comparison descriptor that previously had no matching prepared mode.

Anisotropic point/bilinear encodings 2/3 remain rejected. The previous revision incorrectly treated them as ordinary point/bilinear filtering; this revision restores main's guard rather than weakening the rejection test reported by rmz-24.

No new comparison instruction lowering is introduced. Cube coordinates use the existing CubeAxis/CubeLayer conversion in SpirvImageEmitter.cpp. sRGB texels use Vulkan's image-format decode before comparison; the GPU regression uses the standard sRGB transfer function as its reference. Console reference-clamping behavior and cube-edge filtering remain unmeasured and are explicitly recorded in TechnicalDebt.

Tested

Rebased onto main 0df342c6 (PR head 9cc55a13). Resolved the TechnicalDebt.md conflict by retaining both main's ancillary-word entry and this change's color-comparison entry. git range-diff confirms the executable code changes are unchanged by this rebase. The focused tests below were rebuilt from this PR checkout, separately from the MiniGolf integration branch. Windows, MinGW-w64 GCC 15.2.0, Release, NVIDIA RTX 3050 6GB Laptop GPU, SPIRV-Tools disabled.

  • agc_shader_memory: passes, with an assertion that the captured cube descriptor selects a cube emulated-comparison mode.
  • agc_driver_emulated_color_compare: passes, including new sRGB point/bilinear result checks and rejection of both anisotropic filter encodings. Existing address, border, offset, cache and reduction cases remain enabled.
  • agc_driver_guarded_null_pointer: passes on the rebased main.
  • python -I tools/check_conventions.py --base 0df342c6 --head 9cc55a13: passes.

The earlier MiniGolf integration accepted anisotropic filtering incorrectly; that run is not evidence that anisotropic sampling works. Linux, validation layers and PS5 hardware equivalence were not tested locally.

Checklist

  • Based on current main; no other open PR implements the same fix
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw; anisotropy remains refused
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source: none added
  • Generic behaviour, not specific to one title
  • Depends on: none; fix(agc): color compare emulation: review follow-up to #668 #907 independently reviews other color-comparison semantics
  • AI-assisted: yes, Codex

@github-actions github-actions Bot added shader core/shader agc libSceAgc* labels Oct 9, 2026
@rmz-24

rmz-24 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

214f75a cherry-picked onto current main d70b899 breaks an existing test: agc_driver_emulated_color_compare fails 3 of 3 runs with this change and passes on main (Windows 11, RTX 5060 Ti, MinGW-w64 GCC 15.2.0 r7, Release, -DANYPS5_ENABLE_SPIRV_TOOLS=ON; full ctest otherwise 509/510):

AGC graphics: expected rejection: point or bilinear

The case is tests/execution/EmulatedColorCompare.cpp:257, Reject(*device, Format8888UNorm, {ClampEdge, FilterAnisoBilinear}, "point or bilinear"). With this change an anisotropic-bilinear color comparison is accepted where the test expects the ResourceMaterializer filter rejection. It doesn't look Windows-specific, since it's a recompiler decision. If accepting that form is intended, the test needs updating along with the semantics source for it; otherwise the new path should keep rejecting anisotropic filters.

AI-assisted: yes

@authurtabo-byte
authurtabo-byte force-pushed the fix/agc-color-compare-forms branch 2 times, most recently from 78b3c0f to 4dd2166 Compare October 9, 2026 17:28
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@authurtabo-byte
authurtabo-byte force-pushed the fix/agc-color-compare-forms branch from 4dd2166 to 2e2ec5d Compare October 9, 2026 20:12
@authurtabo-byte

Copy link
Copy Markdown
Contributor Author

Thanks for catching that. The branch head has been updated to \2e2ec5d4:

  • Retains rejection of anisotropic filter combinations (\FilterAnisoPoint, \FilterAnisoBilinear) so \ResourceMaterializer\ behaves as expected.
  • Retains both upstream guarded-null-pointer tests and adds cube-comparison regression coverage.
  • All execution tests (\�gc_driver_emulated_color_compare, \�gc_driver_guarded_null_pointer, \�gc_shader_memory) now pass cleanly.

@github-actions github-actions Bot added docs needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 9, 2026
@authurtabo-byte
authurtabo-byte force-pushed the fix/agc-color-compare-forms branch from 2e2ec5d to 898f457 Compare October 9, 2026 22:44
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@authurtabo-byte
authurtabo-byte force-pushed the fix/agc-color-compare-forms branch from 898f457 to 9cc55a1 Compare October 9, 2026 23:11
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 9, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for restoring the anisotropic rejection. I measured both new forms on RDNA2 hardware (gfx1035, image_sample_c_lz, IEEE=0), and they don't match what the hardware returns:

  • sRGB (ResourceMaterializer.cpp:507, test at EmulatedColorCompare.cpp:150). On 8_8_8_8_SRGB (and 8_SRGB, 8_8_8_8_UNORM, 8_UNORM, 16_FLOAT), image_sample_c_lz returns 0 for every compare function, ALWAYS included, with point and bilinear filtering. Plain image_sample_lz on the same T# reads the expected decoded texels. Only depth-capable formats (32_FLOAT, 16_UNORM) give a real comparison, and emulatedCompareState already sends exactly those to the native path. So "decode, then compare against a UNORM-clamped reference" is not what the GPU does for these formats. The expected values in the test come from the sRGB transfer function, not from hardware, which is why they pass. Please keep these formats throwing. If you want to model them, the measured result is a constant 0, but that is a separate change (see the note below).
  • Cube (ResourceMaterializer.cpp:531, :696). On 32_FLOAT, face = layer and s/t in [1,2] match the cube-to-array conversion: all 192 point rows and all 54 in-face bilinear rows agree. With the default sampler (word0 bit 28 DISABLE_CUBE_WRAP = 0), bilinear footprints that reach a face edge filter across into the neighbouring face. 46 of 90 edge rows differ from the per-face clamp that the emulation uses, and with bit 28 set all 90 match. Point sampling at the edges matches too. Please throw for bilinear cube comparison unless that bit is set (or model seamless filtering), and add an execution test on a cube view. The current cube test (ShaderMemory.cpp:2138) only checks which mode gets selected.
  • The TechnicalDebt line (docs/dev/TechnicalDebt.md:45) should then describe what is actually accepted.

Note: the UNORM color-comparison emulation already on main has the same mismatch with these measurements (the hardware returns 0 and the emulation compares). That will be raised separately and is not for this PR to fix. Measurements were taken on gfx1035, not on PS5's gfx1013; both are RDNA2.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
authurtabo-byte and others added 5 commits October 10, 2026 01:08
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@authurtabo-byte
authurtabo-byte force-pushed the fix/agc-color-compare-forms branch from 9cc55a1 to af635e2 Compare October 10, 2026 08:08
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked on the PR merged onto current main: agc_shader_memory and agc_driver_emulated_color_compare pass, the AgcDriver/Agc ctest scope is green (333/333), and both tests fail without the ResourceMaterializer.cpp change. sRGB and anisotropic forms are rejected again, bilinear cube comparison requires DISABLE_CUBE_WRAP (matches my gfx1035 measurements: 90/90 edge rows), and the cube execution test covers it. Merging.

@adriadam10
adriadam10 merged commit ddcb251 into boykopovar:main Oct 10, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants