WIP: feat(vk): a Colour filter row for colour-blind players #139
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!139
Loading…
Reference in a new issue
No description provided.
Delete branch "simonwjackson/Longshot:feat/colour-filter"
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?
Why
Colour-coded hearts, maps and effects can be hard to distinguish for colour-blind players. This adds optional correction, not a replacement for non-colour cues.
What changed
colour_filter=0/1/2/3, independent of quality presets.f87557ae73df279128686b274055e6345dedc102in5c2175e8. Keep main's minification requirement and regenerate both SPIR-V headers from the combined shader. No history rewrite or force-push.Flags and settings
colour_filter=0/1/2/3Measurements
main:
f87557ae73(2026-11-14)this PR: feat/colour-filter @
5c2175e8dfThe pinned comparison completed on aka: main → PR → main → PR, copied Castle Town save at the wooden door, no movement/camera input. Each process warmed at the scene, then sampled 30 seconds; the last five native-presents FPS readings per setting determine each run median. The table reports the median of the two run medians. PR runs sampled Off then Protanopia after another 10-second warm-up. No FPS claim is made for other scenes or 1% lows.
Hardware: RX 7900 XT / RADV Mesa 26.2.3, Ryzen 5 7600X, 32 GB, NixOS 26.11 / Linux 7.2.7. Private headless Sway/Xwayland, 1920x1080, resolution 100%, game upscaler, window filter/sharpness/FG/HDR Off, Uncapped, VSync Auto; mesh fallback, udmabuf, audio off. The shared GPU lock was held and the owner's game was never stopped; an earlier shader-preparation attempt yielded when the owner started a game and is excluded from measurements.
f87557a5c2175e8, Off5c2175e8, Protanopiaf87557ae5c2175e85c2175e8f87557ae5c2175e85c2175e8Exact presents-log readings
The attached SDR captures compare HDR Off and Native selected, with the filter Off and Protanopia. The exact arithmetic proof below is game-free; these animated game captures are not claimed pixel-identical.
Game-free review-fix proof:
4cdeeff5c2175e8The last comparison compiles the actual old/new GLSL correction functions as C++, following the existing shader-math fixture approach. It proves unchanged SDR correction arithmetic, not a GPU screenshot or an FPS result. The production scaler fixture also checks SDR/PQ dispatch and pushed matrices.
Earlier game measurements remain in the PR's history at their named commits. They are not represented as measurements of this head.
Testing
Tested: On zao, with
/tmp/longshot-carry/env.shand the shared suite lock: 533 passed, 2 failed, 16 skipped in 736 s, 8 jobs, at5c2175e8. Both failures reproduce on untouched mainf87557a:test_linux_tools.pyfixes PATH to/usr/bin:/binand misses Nix tools;test_pad_layout.pylacks the extracted binding table'sHoldToggleinclude after #137 merged. The latter is repaired by separate #338. No new failures.Focused colour/scaler/HDR/README/ID checks: 6 passed, 0 failed. Regression checks failed before the fixes. Both shader headers were regenerated with glslang and pass hash, compilation and byte-for-byte checks. Native fixtures compile with warnings as errors.
git diff --checkpasses; no separate formatter or typechecker is configured. Python formatting follows the neighboring tests.Pinned main and PR Linux release builds from the NUP succeeded in isolated folders on aka. Tracked source hashes were checked before and after the current builds. Shader preparation and the four isolated gameplay runs completed. All attached screenshots were inspected. No owner game or live save was changed.
Not tested: Windows builds/gameplay and non-ASCII Windows paths, physical controllers, other GPU families, real HDR/PQ output, frame-generation combinations, colour-blind-player assessment and a full playthrough. No Windows machine is available. Requests are not passing results.
Needs other testers:
Both requests are open but waiting for a maintainer's
testing wantedlabel before testers can claim them.Screenshots
PR attachments, verified by anonymous byte-identical downloads.
Risk and rollback
Correction can alter readability or clip colours. Actual Native HDR/PQ bypasses it. Select Off to restore the uncorrected picture, or revert the feature. Interaction with #109 and real HDR output remains unverified.
Reviewer notes
No
MkwVkHostorMkwVkRendererstructure changed, so no API-version bump is needed. ID 56 is no longer consumed. Main was merged, not rebased; the template's literal rebase box remains unticked rather than claiming otherwise. The six new measurement records are indocs/tested/under main's current format;docs/tested-configurations.mdnow holds a frozen historical table.Checklist
For everyone
type(scope): description).main. Current mainf87557awas merged as5c2175e8, not rebased; no force-push.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.docs/tested/for each measured run.tests/that reads changed source updated in this PR, not weakened.Code review only. I read the diff and the code around it, and ran no tests or builds. The change is tidy. The correction matrix is right: I + E(I - S) with Machado's S and Fidaner's E, and each row is forced to sum to 1 so grey stays grey. The push constants come to 56 bytes in both the C struct and the shader's std430 block. Off keeps the old bypass. Both SPIR-V headers name the SHA-256 of the new
fg_scale.comp(4c93fe5b…).third_party/is untouched. The new row is saved under thecolour_filterini key and is exempt from presets. The branch also merges cleanly onto current main, and main hasn't touched these files since316f9319.I found no blockers in the code. It isn't ready to merge yet because of what the PR already says is missing: a Longshot measurement against main, the Windows test request, and the rebased base. The inline comments are suggestions:
HOS_OPT_COLOUR_FILTERhas no reader on the host side, so it spends option ID 56 for nothing.None of these needs a
MKWVK_API_VERSIONbump: the PR adds an enum value only, and neitherMkwVkHostnorMkwVkRendererchanges. Thepr statushint about the bump doesn't apply.@ -87,0 +87,4 @@// Correction follows scaling/sharpening, before Expanded HDR encoding and host overlays.vec3 accessible(vec3 c) {// Keep the old shader path exact with neutral settings, even while scaling is active.if (pc.colour[0] == 1.0 && pc.colour[4] == 1.0 && pc.colour[8] == 1.0) return c;The bypass only checks the diagonal. That works for today's three matrices: protan and deutan both have m[0] == 1, but m[4] and m[8] are not 1. Still, a future matrix with a unit diagonal and nonzero off-diagonal terms would be skipped without anyone noticing. Comparing all nine entries is safer, or the C side could pass an explicit on/off (for example in an unused bit of
side). Nit: changing it means regenerating both headers.@ -55,2 +55,4 @@90, 120, 144, 165, 240, 6 Uncapped; a change applies in play) */enum { HOS_OPT_FPS_CAP = 53 };/* Graphics: live colour correction, 0 Off, 1 Protanopia, 2 Deuteranopia, 3 Tritanopia. */enum { HOS_OPT_COLOUR_FILTER = 56 };Nothing on the host side asks for
HOS_OPT_COLOUR_FILTER.host/never callsrx_optionfor it, and the control bridge finds rows by ini key, not by ID. The renderer readsopts.colour_filterdirectly, so this enum and thefill_replay_tableline (vk_replay.cpp:5053) are dead, and they take option ID 56 from the IDs being shared out between the open PRs. Unless something is planned to read it, consider dropping both (and the matching asserts intests/test_colour_filter.py). If it stays, please add it to the ID list in theoptioncomment further down this file. Either way no API version bump is needed.@ -34,6 +34,7 @@#include "shaders/fg_luma_comp_spv.h"#include "shaders/fg_median_comp_spv.h"#include "shaders/fg_scale_comp_spv.h"#include "colour_filter.h"Nit: this include splits the
shaders/*_spv.hblock in two, betweenfg_scale_comp_spv.handfg_scale_pq_comp_spv.hand its comment. Move it above or below the block.@ -595,2 +596,3 @@const WideSides wide = wide_now.load(std::memory_order_relaxed); // (wide_sides_tick, each present)if (!opts.upscale && !opts.sharpness && !wide.side) return VK_NULL_HANDLE;// Native HDR supplies PQ codes, not sRGB. Leave that path unchanged, including its SDR fallback.const int colour = opts.hdr == 2 ? 0 : opts.colour_filter;opts.hdr == 2also turns correction off when Native is selected but the display isn't HDR (ordev.hdris off). In that case the game presents SDR andpqis false, so correction would be correct, but a player who left HDR on Native silently gets none. Gating on the actual PQ picture fits better:const int colour = pq ? 0 : opts.colour_filter;, with vk_replay.cpp:3480 using!(dev.hdr && opts.hdr == 2)to match. If keeping the menu wording simple matters more, a hint on the row for when Native HDR is active would at least show the player why nothing changes.@ -1241,6 +1245,8 @@ Row rows[] = {"tone mapper, the same mid grey with highlights that roll off into the peak and keep their colour.",nullptr, false, true},{TAB_GRAPHICS, "Detail", nullptr, 0, 0, 0, nullptr, nullptr, nullptr, false, true, -1, true},{TAB_GRAPHICS, "Colour filter", &opts.colour_filter, 0, 3, 1, tx_colour_filter,Nit: this lands under the "Detail" heading, next to anisotropic filtering and texture detail. It reads more naturally under "Screen", or just after the HDR rows that its help text refers to. The README row would move with it.
@ -0,0 +8,4 @@def main():p=argparse.ArgumentParser();p.add_argument('--cxx',default='clang++');a=p.parse_args()header = (ROOT / 'gpu/vk/vk_menu.h').read_text()menu = (ROOT / 'gpu/vk/vk_menu.cpp').read_text()Nit:
read_text()withoutencoding="utf-8".vk_menu.cpphas non-ASCII bytes, so on a Windows locale code page this can fail or misread.tests/test_option_ids.pypasses the encoding. The file's style (p=argparse...;a=p.parse_args(), no spaces) also differs from the other tests, so a plain reformat would help the next reader.Added a fresh interleaved main/PR Linux measurement session at the named commits, the last-five-sample FPS medians, feature proof and inspected screenshots. Windows testing requests are open; the description names the remaining gaps. The branch also rebases cleanly onto main
1be180bbwith an identical stable patch ID and passing focused tests; the force-with-lease push is still gated, so the measured remote head stays pinned. No new baseline FPS is claimed for the local rebase. The #109 interaction has its own open request.WIP: feat(vk): a Colour filter row for colour-blind playersto feat(vk): a Colour filter row for colour-blind playersMerged main snapshot
dd02c6cca9with a merge commit and normal push; resolved README by keeping main’s HDR text and Colour filter in menu order. Four focused checks pass; full suite: 504 passed, 3 failures reproduced on untouched main, 16 skipped; Linux build and isolated Castle Town smoke pass, and newer mainfdc2840bstill merges cleanly (details in Testing).514591ainto colour filter 4cdeeffe465b81a8aafter upkeep validation bbcb6c53a7c044207into colour filter 0862eb91d8f87557ainto colour filterfeat(vk): a Colour filter row for colour-blind playersto WIP: feat(vk): a Colour filter row for colour-blind playersMain merged at
f87557aas5c2175e8; resolved README/API and later scaler-bypass conflicts, regenerated both shaders; six focused checks pass, full suite 533 pass/2 inherited failures/16 skip; current-head GPU comparison queued.Review 1090 addressed in
473cc95and6e79347, with main merged normally through5c2175e8(mainf87557a). No force-push.HOS_OPT_COLOUR_FILTERand its replay-table entry; the renderer still readsopts.colour_filter. ID 56 is free. No ABI structure changed.const int colour = pq ? 0 : opts.colour_filter;; the sampled-view gate uses!(dev.hdr && opts.hdr == 2). Native selected on SDR now retains correction. Regression fails before and passes after.colour_filter.habove the shader-header block.colour_filter, all values, default Off, PQ bypass and Native-on-SDR behavior.encoding="utf-8", normal formatting, and compiled production gate/identity checks. Updated the scaler fixture's expected matrix to follow the actual SDR/PQ pipeline.test_linux_tools, andHoldTogglemissing fromtest_pad_layoutafter #137 merged (fixed separately by #338). Six focused checks pass. Pinned Linux main/PR builds succeeded on aka; source hashes match.git diff --checkpasses.longshot-winperf-before. The owner's active game is untouched. The Measured and Screenshots boxes remain unticked; the description no longer claims old runs measure this head. Captures will be PR attachments, not private-fork links.testing wantedlabel. Please add that label so testers can claim it. No Windows result is claimed, and its checklist box remains unticked. T-F3WV covers the #109 combination.The template's exact rebase line is present but unticked: main was merged, not rebased. The branch stays WIP while the pending checks and measurements remain. There is no re-request-review command documented in
docs/forge.md.The queued main/PR comparison is done (main
f87557aagainst5c2175e8; the new head4f0af1eadds only the sixdocs/tested/records). Off: 56.40 against 56.70 fps, Protanopia 56.20, two interleaved runs each, so within noise. The description now has the raw log lines, the inspected screenshots as attachments (Off, the three filters, and SDR output with HDR Off against Native selected, which shows correction still applies on an SDR display), and the per-run records. Windows is still untested (T-RB23, waiting for thetesting wantedlabel).View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.