Repository navigation
Conversation
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, this is a small, clean change. The description matches the diff: of the −477 lines, 469 are the regenerated SPIR-V header, and the rest are one-line replacements (push size 20→24, the pc struct, the test stub). No features are dropped and nothing else is rewritten.
What I checked:
- CAS math:
cas()matches ffx_cas.h's no-scaling path: soft min/max over the cross plus the 3×3,amp = sqrt(saturate(min(mn, 2 − mx) / mx)),peak = −1/lerp(8, 5, sharpness), the cross-shaped lobe normalised by1 + 4w, then saturate. Themax(mx, 1/1024)guard stands in for the barercpand avoids a divide by zero on black. The neighbour spacing and edge handling are the same as the existing sharpen (one picture pixel, clamp-to-edge sampler). - Colour space / HDR: it runs on the 8-bit picture (rgba8
dst), and the HDR10 pass encodes after it fromframegen_scaled_view()(vk_replay.cpp present). So the 0..1 assumptions in2 − mxand the final clamp hold. Like the Gentle path, it sharpens the encoded values, not linear light, which is also how most CAS integrations use it. - Push constants: the SPIR-V puts
methodat offset 20 and the block is 24 bytes. That matchesinit_scale, the C++ struct (both dispatches, side 3 and the main one, pushsizeof pc) and the test stub. - Generated header: the hash on the first line is this .comp's. I rebuilt it with glslang 16.6.0 (main's header was made with 16.6.0, this one with 11.12.0). Same word count,
spirv-valpasses, and the disassembly differs only in the order of threeOpDecoratelines. So the header is this shader.test_fg_scale_sharpen.pywill pass without comparing bytes because of the version mismatch. Nit: regenerating with 16.6.0 would keep main's header on one glslang. - Menu: no new
HOS_OPT_*id, so no collision with main or the open PRs. The value is saved by name assharpening_method, it ispreset_exemptas described, and it takes no restart. I checked the tests that cut the row table or functions out of vk_menu.cpp (test_upscaler_rows,test_menu_restart_slots,test_framegen_preset, theRow rows[]cutters): none depends on the row after Sharpness, so I don't expect breaks.
Still open:
- Licence notice (inline): MIT asks for the copyright and permission notice to go with substantial portions.
cas()is a direct port of AMD's filter, so please add AMD's copyright line and point to the MIT text.third_party/fidelityfx-sdk/LICENSE.txtalready has it in the tree. - No CI and no in-game run yet: both workflows on this head are
action_required(they wait for a maintainer to approve the run). The description says the C++ fixtures were left to CI, so for now nothing has compiledupscale_independent_test.cppor the shader tests on this branch. This port onto main also hasn't been played. A run in the game with AMD CAS at a couple of Sharpness values, plus a before/after screenshot (CONTRIBUTING asks for screenshots when a change shows on screen), would settle it. - Nit:
test_fg_scale_sharpen.pycheckssharpen()'s properties as compiled C++ (unchanged at 0, within 0..1, no halos past the neighbours), but nothing coverscas(). The same harness could check that a flat neighbourhood is unchanged and the output stays in 0..1.upscale_independent_testcould also check thatpc.methodfollowsopts.sharpen_method. - Heads-up: #1488 (screen filters) also appends to this push-constant block at offset 20 (
int fx; float strength; float time). Whichever lands second has to move its members and regenerate the header. #1942 adds a PQ variant of this shader, sofg_scale_pq_comp_spv.hwould need regenerating too.
| // (and no further than the lightest and darkest pixel around it: an edge gets steeper, no halo beside it) | ||
| return clamp(sharp, lo, hi); | ||
| } | ||
| // AMD FidelityFX CAS, Contrast Adaptive Sharpening (MIT, AMD's ffx_cas.h): its filter without scaling, over the 3x3 neighbourhood |
There was a problem hiding this comment.
Please keep AMD's notice with the port, e.g. // Copyright (c) 2019-2020 Advanced Micro Devices, Inc. MIT licence: third_party/fidelityfx-sdk/LICENSE.txt (or the full permission text). Crediting ffx_cas.h is good, but MIT asks for the copyright line itself to go with substantial portions.
| const vec3 mx2 = max(mx, max(max(a, c), max(g, i))); | ||
| mx += mx2; | ||
| const vec3 amp = sqrt(clamp(min(mn, 2.0 - mx) / max(mx, vec3(1.0 / 1024.0)), 0.0, 1.0)); | ||
| const float peak = -1.0 / mix(8.0, 5.0, clamp(amount, 0.0, 1.0)); |
There was a problem hiding this comment.
Nit, UX: the pass only runs at pc.sharp > 0, and CAS's sharpness 0 is already a full −1/8 lobe (kernel centre 2, sides −0.25 where amp is 1). So the slider's first step (0 → 10%) goes from none straight to near CAS's floor, while Gentle starts from nothing. AMD's mapping is fine. A hint line for the row could say that the first step is already noticeable, or you could fade the lobe in over the first step.
e9a72dd to
7856f59
Compare
…p, tests for cas() and the method push constant Review on NotRexed#1979: keep AMD's copyright line with the port (MIT; the notice is in third_party/fidelityfx-sdk/LICENSE.txt). CAS's sharpness 0 is already a full -1/8 lobe, so the slider's first step (0 to 10%) now fades the lobe in from nothing, as Gentle starts; from 10% on it is AMD's mapping. test_fg_scale_sharpen.py checks cas() as compiled C++ (the picture at 0, finite and inside 0..1, flat areas kept, edges steeper with the setting); upscale_independent_test checks that the pushed method follows opts.sharpen_method. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the careful review. The branch is rebased on main (d7a7306), and these are addressed in 7856f59:
|
|
Heads-up: this conflicts with main now. Could you rebase onto main and resolve the conflicts? Thanks! |
Joey-Magnus
left a comment
There was a problem hiding this comment.
The review points are all addressed in 7856f59 (AMD's notice, the fade-in, the cas() and method tests). What's left is mechanical, but it needs doing before this can go in.
It conflicts with main in gpu/vk/shaders/fg_scale_comp_spv.h, gpu/vk/vk_menu.cpp and tests/upscale_independent_test.cpp. Since #1942/#2342, fg_scale.comp is also compiled with FG_SCALE_PQ into fg_scale_pq_comp_spv.h, which writes RGB10A2 for native HDR. Adding int method to the push block means both headers have to be regenerated with scripts/host_shaders.py, not just the one in this branch. Otherwise the PQ pipeline is left with a 20-byte block while the C++ pushes 24. On the PQ path CAS works on PQ codes, which stay within 0..1, so the clamp still holds, but please say in the description what you saw with HDR on.
The PR title also fails the Conventional Commit check: something like feat(vk): AMD CAS as a Sharpening method would pass. The in-game run and the before/after screenshots are still to come.
The Graphics tab is one list of 24 settings, with the window, HDR, upscaling, frame generation and GPU options mixed together, and about ten more are on their way in open PRs. This puts its rows under section titles, so the upscaler, frame generation or HDR can be found without scrolling the whole list. It is the first step of a menu organization laid out below for discussion. **What changed** - The Graphics rows in six sections, under heading rows like the Debug tab's Developer (a gold title, never selected, nothing to click): - Quality preset (first: it sets rows in every section) - **Resolution and upscaling:** Output resolution, Profile, Upscaler, DLSS model, Render resolution, Sharpness - **Frame generation and latency:** Frame generation, Frame pacing, Game frame rate cap, NVIDIA Reflex, AMD Anti-Lag 2 - **Screen:** Fullscreen, V-Sync, Window scaling filter, Aspect ratio (#2119), Ultrawide menus - **HDR:** HDR, HDR paper white, HDR peak brightness, HDR contrast (#2342), HDR tone mapper (#2481) - **Detail:** Anisotropic filtering - **GPU and memory:** GPU device, VRAM budget (#1859), Mesh shaders (Robust buffers is on Debug ▸ Developer since #2042) - Moves only: every row's definition is unchanged, word for word; no row is renamed and no value or saved key changes, so options files carry over, and #1864's tables (keyed by the English text) still find every row. They need the six headings. - No new drawing or input code: the menu already draws heading rows on any tab, and the keys, the pad and the mouse step over them. - `readme.md`: a sentence on the sections, and the option table in the new order. (Ultrawide menus and Mesh shaders have no line yet; #2105 adds them.) The **Frame generation by** row from #2118 gets its line in the table here too: main's readme has none yet. So do #1859's **VRAM budget** (under GPU and memory) and #2342's **HDR contrast** (under HDR). - `tests/test_upscaler_rows.py`: its order list follows the picture's path (output, upscaler, render size, sharpening, then the frame rows, then the window). New `tests/test_menu_sections.py`: the Graphics sections and which rows they hold, and on every tab no section that `row_shown` could leave empty (a title over nothing on a GPU without Reflex, say). **The plan this is step 1 of (for discussion)** Each tab answers one question; a section groups the rows you set together; a row that depends on another follows it; vendor rows sit beside their counterpart and hide where they can't run (no vendor sections, so nothing is left empty on AMD); rows to touch only when something is wrong go last; moves only, no renames. | Tab | Holds | Sections | |---|---|---| | **Game** (was Gameplay; first, where the menu opens) | how the game behaves and reads, and Restart and Quit, as a pause menu | flat; Restart and Quit last | | Graphics | how the picture is made, and what it costs | Quality preset · Resolution and upscaling · Frame generation and latency · Detail · GPU and memory | | **Display** (new) | how it gets to your screen, and what is drawn over it | Screen · HDR · Overlays (FPS counter, Menu size, from Game) | | Audio | sound out and in | + Microphone (from Game: it picks the input device) | | Controls | how you play | Camera · Controller · Buttons (optional) | | Debug | tools and developer switches | Robust buffers in Developer (#2042, merged) | Next PRs, one topic each: (2) a Display tab, taking the Screen and HDR sections over whole and adding Overlays; (3) the Gameplay tab titled Game, first in the strip, where the menu opens; (4) Microphone to Audio; (5) optional, sections on Controls. If a sixth tab is a no, this PR stands on its own. Open questions: 1. A sixth tab (Display)? 2. V-Sync on Display (the PC convention) or with frame generation (it changes with it)? 3. Sharpness with the upscaling rows (where players look for it) or with the window filter (where it runs)? 4. DLSS 5 (#1590) and #1834's Neural rows as a page opened from a Graphics row, like Motion, rather than a seventh tab? <details><summary>Where open PRs' rows would go</summary> - Resolution and upscaling: Sharpening method (#1979), DLSS 5 page (#1590), Native 3D resolution (#2017) - Frame generation and latency: Frame generation by (#2118, merged: in this section now), Frame generation method (#1834) - Detail: Shadow resolution (#1892), Screen filter and Filter strength (#1488) - GPU and memory: Texture streaming cache (#2017), Refresh memory (#888); VRAM budget (#1859, merged) is in this section now - Game (was Gameplay): Game language and Menu language (#1864), Skip title screen (#1560), Amiibo (#1521) - Controls: Camera speed (#1945) - #2042 (merged) moved Robust buffers to Debug ▸ Developer. - #2105: its readme check would read the heading rows as rows; it needs to skip `{TAB_..., "...", nullptr, 0, 0, 0, ..., true}` headings, or this PR adds that if it lands first. Whichever PR lands second moves one row on rebase; happy to do it for anyone. </details> **Testing** - `tests/run_all.py` on Windows 11 (llvm-mingw clang, 8 at once) after merging main at 2dfe901 (with #2119's Aspect ratio, #2481's HDR tone mapper, #2118's Frame generation by, #2342's HDR contrast and #1859's VRAM budget): 422 passed, 33 skipped, 2 failed, both outside this PR: `test_vram_budget_row.py` (fails the same on main's own sources here) and `test_portable_builder_startup.py` (its `dotnet publish` needs packages this PC does not have). A build of the branch compiles. All 81 of main's rows keep main's definitions word for word; the only additions are the six headings. `test_menu_sections.py` fails on main as it stands (no sections) and passes here; a layout with a section of Reflex and Anti-Lag 2 alone fails it. - A build of this branch (Windows 11, RTX 5090), headless with scripted menu keys at Small, Medium and Large: 12 Downs from the Quality preset land on V-Sync and 13 Ups come back past it to the last row, so every heading is stepped over both ways and Up from the first row wraps to the last; the help box always shows the selected row (at the first head, when Robust buffers was the last row). Again at this head (25feb16, after #2042, #1859 and #2119): Up from the Quality preset wraps to Mesh shaders, the last row now, under VRAM budget (the bottom screenshot). **Not tested**: in a window with a pad, the mouse wheel and a click just under a heading (the headless runs drive the keys); values kept across a restart (no row's saved key changes); AMD and Intel (their rows hide through `row_shown` as before; the new check makes sure no section can end up empty), Linux. **Screenshots** The Graphics tab at 1920x1080, Medium menu size: the top, then scrolled with the keys to V-Sync and to the last row (the selection steps over every heading; Up from the first row wraps to the last row). The bottom one is retaken at this head (25feb16, a 16:9 picture: `HOS_ASPECT=1.7778`, as on a 16:9 monitor); the others are from the first head (634ef65).    Small and Large:   _Taken from headless runs of this branch (`HOS_HEADLESS=1`, `RX_VK_SHOT_MENU=1`, the keys from `RX_VK_MENU_SCRIPT`), RTX 5090. Headless, the list never scrolls on main either: `menu_record_image` leaves `ease_step` at 0, so the build behind the scrolled shots had one more line there (`ease_step = 1.0f`, hence "-dirty" in the first head's build label), not part of this PR. The windowed menu eases as before._ 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
DLSS, DLAA and FSR leave the picture soft, and the window pass's sharpen never goes past the light around a pixel, so it does little to bring that detail back. A Sharpening method row (Gentle / AMD CAS, saved as sharpening_method, outside the Quality preset) picks the pass's sharpen: AMD FidelityFX Contrast Adaptive Sharpening (MIT, ffx_cas.h, its filter without scaling; AMD's copyright line with it) over the 3x3 neighbourhood one picture pixel apart, the Sharpness slider as CAS's sharpness, its lobe faded in over the slider's first step. fg_scale.comp gets cas() and an int method at the end of its push constants (24 bytes), for both of its pipelines: the plain one and FG_SCALE_PQ's for native HDR (vk_framegen.cpp makes both with 24). Both headers made again with tests/test_fg_scale_sharpen.py --write, which also checks cas() as compiled C++; upscale_independent_test checks the pushed method. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7856f59 to
81e31ce
Compare
|
Rebuilt on main at 51da046 (81e31ce):
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Joey-Magnus
left a comment
There was a problem hiding this comment.
Everything from my last review is fixed. Both FG_SCALE pipelines are now made with a 24-byte push block (vk_framegen.cpp:263 and :576), both headers are regenerated, upscale_independent_test checks 24 on both, and the title passes. You were right that the pair comes from tests/test_fg_scale_sharpen.py --write. I had pointed you at the wrong script.
I merged it into current main. test_fg_scale_sharpen, test_upscale_independent, test_framegen_resources, test_upscaler_rows, test_menu_restart_slots, test_menu_i18n and test_framegen_preset all pass.
Still open:
- No game run yet. CONTRIBUTING asks for before/after screenshots for a change you can see on screen, and the HDR run you mention is also still to come.
- #2105 (likely to merge soon) adds
tests/test_readme_menu_rows.py, which needs a readme option-table line for every menu row. With both merged, it fails on "Sharpening method". Please add a line after Sharpness, for example| | Sharpening method | Gentle, AMD CAS | Gentle |. - #1488 also appends to the same push block at offset 20, so whichever lands second has to move its members and regenerate both headers.
This adds a Sharpening method row under Sharpness in the Graphics tab. It offers Gentle (today's sharpen) or AMD CAS, is saved as
sharpening_method, and is left out of the Quality preset. It's a standalone PR onmain(rebuilt on 51da046).Why
Temporal upscalers soften the picture. DLSS, DLAA and FSR rebuild each frame from jittered samples, and the result is noticeably softer than a native render, especially textures, foliage and distant detail. The window pass's current sharpen is RCAS-like and never goes past the light and dark around a pixel. That keeps it free of halos, but it also means it barely recovers detail an upscaler has smoothed over.
AMD's Contrast Adaptive Sharpening suits this job. It is designed to sharpen most where the picture is soft and least where edges already have contrast. That brings back the texture detail DLSS and FSR blur without overshooting edges that are already crisp. It's the sharpener many games pair with their upscalers for exactly this reason.
How
shaders/fg_scale.comp: addscas(), AMD FidelityFX CAS (MIT,ffx_cas.h), using its filter without scaling.main()uses it whenpc.method == 1; otherwise the existingsharpen()runs unchanged.int methodat the end and grow from 20 to 24 bytes, for both offg_scale.comp's pipelines: the plain one and theFG_SCALE_PQone for native HDR (fix(hdr): Native HDR scaled in 10 bits and shown as the game made it at 100% contrast #1942/feat(fsr4): FSR 4 for the game's native HDR, given linear light #2342).vk_framegen.cppmakes both with 24, andtests/upscale_independent_test.cppchecks both.fg_scale_comp_spv.handfg_scale_pq_comp_spv.h, are regenerated withtests/test_fg_scale_sharpen.py --write(glslang 11.12.0). That's the script that writes the pair;scripts/host_shaders.pycovers onlygpu/vk/host_shaders/.opts.sharpen_method, the row, andtx_sharpen_method. Gentle is the default, so nothing changes until someone picks AMD CAS.third_party/fidelityfx-sdk/LICENSE.txt.Testing
tests/run_all.py, the whole suite, locally (clang targeting MSVC, the CPM Vulkan headers, glslang 11.12): 433 passed. The 24 failures are exactly the ones main at 51da046 has on the same machine, all down to the local toolchain.test_fg_scale_sharpen.py(cas()included) andtest_upscale_independent.py(the method, on both pipelines) pass.🤖 Generated with Claude Code