Skip to content

Commit c432e63

Browse files
Spruill-1Copilot
andcommitted
Phase 1: delete dead Rendering/ToneMapper class
Decision #32 (Day 6) removed the built-in tone-mapper from the render path; the class itself lingered as default-None dead code through v1.4.1 -- never instantiated at runtime, only included by Controls/OutputWindow.h for an unused declaration. The .github/copilot-instructions.md still cited "fixing issues with the existing D2D tonemapper effect" as the project #1 development focus, which was actively misleading new contributors and AI agents about where work actually happens. - Delete Rendering/ToneMapper.h and Rendering/ToneMapper.cpp (~280 LoC). - Remove the unused #include from Controls/OutputWindow.h. - Drop entries from ShaderLab.vcxproj, ShaderLabEngine.vcxproj, ShaderLab.vcxproj.filters. - Rewrite the project-identity, architecture, and tone-mapping focus sections of .github/copilot-instructions.md around graph-built tone mappers (the ICtCp suite) and the empirical fidelity loop (Working Space + Delta E Comparator Grayscale dE + Luminance Statistics). Also fix the architecture diagram, namespace-convention bullet, and the two enum/class examples that referenced ToneMapMode / ToneMapper. - Update README architecture mermaid (drop ToneMapper from EV node). - Add README decision log entry #54 explaining the retirement and where new tone-mapping work goes. - Update .context/resume.md rendering bullet and project-structure tree. - Add CHANGELOG [Unreleased] entry. Build clean. All 58 ShaderLabTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 8f5a2fe commit c432e63

10 files changed

Lines changed: 18 additions & 415 deletions

File tree

.context/resume.md

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ Every effect carries a stable `effectId` + numeric `effectVersion`; saved graphs
6565
- **Always scRGB FP16 pipeline** (`DXGI_FORMAT_R16G16B16A16_FLOAT`, `DXGI_COLOR_SPACE_RGB_FULL_G10_NONE_P709`). DWM/ACM handles final display conversion.
6666
- **Refresh-rate-driven render loop** (60–240 Hz) — interval re-derived from `EnumDisplaySettings(dmDisplayFrequency)` on every display change.
6767
- Dirty-gated render loop with **dirty propagation pre-pass** (any dirty node marks its direct downstream consumers dirty before evaluation; runs again after `TickAndUploadVideos` so video updates flow through analysis-only compute nodes too).
68-
- Built-in `ToneMapper` retained (5 modes: None, Reinhard, ACES Filmic, Hable, SDR Clamp) but defaults to **None** — users build tone mappers as graph effects (the ICtCp suite is the preferred path).
68+
- No built-in tone-mapping pass in the render path — users build tone mappers as graph effects (the ICtCp suite is the preferred path). Decision-log entry #54 retired the legacy `Rendering/ToneMapper` class in the Phase-1 cleanup.
6969
- Display profile mocking (presets + ICC file loading via `mscms.dll`).
7070
- Monitor gamut detection from `DXGI_OUTPUT_DESC1` primaries.
7171
- **OS-reported SDR white level** queried via `DisplayConfigGetDeviceInfo(DISPLAYCONFIG_DEVICE_INFO_GET_SDR_WHITE_LEVEL)`, tracks the *Settings → Display → HDR → "SDR content brightness"* slider; exposed to graphs as `working_space.SdrWhiteNits`. Effects pull the value via property bindings — no per-frame host injection.
@@ -242,10 +242,9 @@ ShaderLab\
242242
│ ├── EffectNode.h / EffectEdge.h
243243
│ └── EffectGraph.h / .cpp # DAG, topo sort, JSON, bindings, versioning
244244
245-
├── Rendering\ # (engine) eval + display + tone + math
245+
├── Rendering\ # (engine) eval + display + math
246246
│ ├── RenderEngine.h / .cpp # (app-only) D3D11 + D2D1 + swap chain
247247
│ ├── GraphEvaluator.h / .cpp # Topological eval, dirty propagation, deferred D3D11 compute
248-
│ ├── ToneMapper.h / .cpp # 5 tone map modes (defaults to None)
249248
│ ├── FalseColorOverlay.h / .cpp # False color rendering overlay
250249
│ ├── DisplayMonitor.h / .cpp # HDR/SDR detection, primaries, OS SDR white, jthread
251250
│ ├── DisplayProfile.h # Profile structs, presets

.github/copilot-instructions.md

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
## Project Identity
44

5-
ShaderLab is a WinUI 3 desktop application (C++/WinRT) for developing, testing, and debugging Direct2D shader effects with full HDR/WCG support. The primary goals are fixing issues with the existing D2D tonemapper effect and adding capabilities to the color correction effect.
5+
ShaderLab is a WinUI 3 desktop application (C++/WinRT) for developing, testing, and debugging Direct2D shader effects with full HDR/WCG support. The primary focus is building tone-mapping and color-correction effects as graph nodes, with empirical fidelity tooling — Delta E Comparator + Luminance Statistics + Working Space node form a closed-loop CIEDE2000 readout that lets us tune effect parameters against measured color accuracy, not visual impression.
66

77
## Hard Rules
88

@@ -27,10 +27,9 @@ MainWindow (WinUI 3 XAML)
2727
├── RenderEngine — D3D11 + D2D1 device stack, DXGI swap chain, BeginDraw/EndDraw/Present
2828
├── DisplayMonitor — HDR/SDR detection, WM_DISPLAYCHANGE + adapter-change jthread
2929
├── GraphEvaluator — Topological walk of EffectGraph, per-node D2D effect cache, dirty gating
30-
├── ToneMapper — D2D WhiteLevelAdjustment + HdrToneMap + ColorMatrix exposure chain
3130
├── EffectGraph (DAG) — Nodes, edges, Kahn's topo sort, JSON serialization (Windows.Data.Json)
3231
├── SourceNodeFactory — WIC image loading (HDR/SDR format split) + Flood fill sources
33-
├── ShaderLabEffects — 9 built-in effects (analysis + source) with embedded HLSL + color math
32+
├── ShaderLabEffects — 30+ built-in effects (analysis + source + tone mapping) with embedded HLSL + color math
3433
├── ShaderEditorCtrl — Live HLSL compile (D3DCompile), D3DReflect auto-property generation
3534
├── NodeGraphController — D2D-rendered canvas: bezier edges, color-coded nodes, pan/zoom, hit-test
3635
├── PixelInspectorCtrl — GPU readback (1×1 D2D1Bitmap1), scRGB→sRGB/PQ/luminance conversions
@@ -39,13 +38,13 @@ MainWindow (WinUI 3 XAML)
3938
└── EffectDesignerWindow — Modal window for creating/editing custom pixel/compute shader effects
4039
```
4140

42-
Render loop: `DispatcherQueueTimer` at 16ms `GraphEvaluator.Evaluate()``ToneMapper.Apply()``BeginDraw``DrawImage``EndDraw``Present`.
41+
Render loop: `DispatcherQueueTimer` (refresh-rate-driven, 60–240 Hz) `GraphEvaluator.Evaluate()``BeginDraw``DrawImage``EndDraw``Present`. There is no built-in tone-mapping pass in the render path — users build tone mappers as graph effects (the ICtCp suite is the preferred path).
4342

4443
## Namespace Convention
4544

4645
All code lives under `ShaderLab::` with sub-namespaces matching directories:
4746
- `ShaderLab::Graph` — data model (EffectGraph, EffectNode, EffectEdge, PropertyValue)
48-
- `ShaderLab::Rendering` — device stack, swap chain, evaluator, tone mapping
47+
- `ShaderLab::Rendering` — device stack, swap chain, evaluator, display monitoring, GPU reduction
4948
- `ShaderLab::Effects` — effect registry, custom effects, shader compilation, image loading
5049
- `ShaderLab::Controls` — UI controllers (decoupled from XAML views)
5150
- `winrt::ShaderLab::implementation` — WinRT XAML types (App, MainWindow)
@@ -67,9 +66,9 @@ All code lives under `ShaderLab::` with sub-namespaces matching directories:
6766
### Naming
6867
- **Member variables**: `m_` prefix (`m_graph`, `m_renderEngine`, `m_refCount`)
6968
- **Methods**: PascalCase (`AddNode`, `PrepareForRender`, `TopologicalSort`)
70-
- **Enums**: PascalCase values (`NodeType::BuiltInEffect`, `ToneMapMode::ACESFilmic`)
69+
- **Enums**: PascalCase values (`NodeType::BuiltInEffect`, `AnalysisFieldType::Float2`)
7170
- **Structs**: PascalCase, used for plain data (`EffectNode`, `DisplayCapabilities`, `ShaderVariable`)
72-
- **Classes**: PascalCase, used for stateful objects with methods (`EffectGraph`, `RenderEngine`, `ToneMapper`)
71+
- **Classes**: PascalCase, used for stateful objects with methods (`EffectGraph`, `RenderEngine`, `GraphEvaluator`)
7372

7473
### File Organization
7574
- **Header-only**: Small data structs and inline constants (`NodeType.h`, `PropertyValue.h`, `DisplayInfo.h`, `PipelineFormat.h`, `EffectEdge.h`, `EffectNode.h`)
@@ -94,12 +93,13 @@ When creating new D2D effects (the core purpose of this tool):
9493

9594
## Tone Mapping & Color Correction (Primary Development Focus)
9695

97-
The tone mapping system (`Rendering/ToneMapper.h/.cpp`) and color correction are where active development happens:
98-
- Current tone map modes: None, Reinhard, ACES Filmic (Narkowicz 2015), Hable (Uncharted 2), SDR Clamp
99-
- Uses D2D built-in effects: `CLSID_D2D1WhiteLevelAdjustment`, `CLSID_D2D1HdrToneMap`, `CLSID_D2D1ColorMatrix`
100-
- Reference math implementations exist inline for each operator (for porting to custom shaders)
101-
- Pipeline works in scRGB FP16 linear light (1.0 = 80 nits SDR white)
102-
- **Goals**: Fix issues with the existing D2D tonemapper effect; extend the color correction effect with additional capabilities
96+
Active development centers on **tone-mapping and color-correction effects authored as graph nodes** — not a built-in tone-mapping pass. The render pipeline is intentionally pass-through (scRGB FP16 in, scRGB FP16 out); users compose tone mappers and color correction from graph effects, validate them with empirical fidelity tooling, and iterate.
97+
98+
- **Pipeline is scRGB FP16 linear light** (1.0 = 80 nits SDR white). No fixed built-in tone-mapping pass — DWM/ACM handles final display conversion.
99+
- **The ICtCp suite** in `Effects/ShaderLabEffects.cpp` (Tone Map / Inverse Tone Map / Saturation / Highlight Desaturation / Round-Trip Validator / Gamut Map / Boundary) is the preferred path for tone-mapping work. Math lives in `GetColorMathHLSL()` (PQ/HLG transfer functions, BT.709/2020/P3 matrices, anchored Reinhard / Möbius, scRGB↔ICtCp).
100+
- **Empirical fidelity loop**: `Delta E Comparator` (Heatmap or Grayscale dE mode) + `Luminance Statistics` (D3D11 compute reduction) + `Working Space` node (mirrors active display profile) lets you measure CIEDE2000 mean/p95/max color difference *while sweeping a parameter*, on the GPU, every frame. This is the canonical way to evaluate any new tone-mapping or color-correction work.
101+
- **Working Space node** is the single path for tracking the active display profile. Effects expose first-class `RedPrimary` / `GreenPrimary` / `BluePrimary` / `WhitePoint` (Float2) and peak/SDR-white nit parameters; bind them from `working_space.*` outputs to follow Display Settings or simulated profiles automatically.
102+
- **D2D's built-in `CLSID_D2D1HdrToneMap`** applies a fixed BT.2408-style mid-tone lift independent of `InputMaxLuminance` (decision log #52). Useful as a reference, not a building block — its lift is opinionated and fixed.
103103

104104
## Key Technical Context
105105

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/).
66
## [Unreleased]
77

88
### Removed
9+
- **`Rendering/ToneMapper` class.** Decision #32 (Day 6) removed the built-in tone-mapper from the render path; the class itself lingered as default-`None` dead code through v1.4.1 — never instantiated at runtime, only included by `Controls/OutputWindow.h` for an unused declaration. `.github/copilot-instructions.md` still cited "fixing issues with the existing D2D tonemapper effect" as the project's #1 development focus, which was actively misleading new contributors and AI agents about where work actually happens. Phase-1 cleanup deletes `Rendering/ToneMapper.h` + `.cpp` (~280 LoC), removes the include from `OutputWindow.h`, drops the entries from both vcxproj files + `.filters`, and rewrites the project-identity / architecture / focus sections of `copilot-instructions.md` around the actual workflow: graph-built tone mappers (the ICtCp suite) plus the empirical fidelity loop (`Working Space` node + `Delta E Comparator` Grayscale dE + `Luminance Statistics`). README decision log gains entry #54; `.context/resume.md` rendering bullet and project-structure tree updated. README architecture mermaid diagram drops the `ToneMapper` node.
910
- **`_hidden` suffix filter and the `Graph/EffectNode2.h` orphan file.** Phase-0 health pass: the `_hidden` property name filter (in `MainWindow.xaml.cpp` Properties-panel rebuild and `Controls/NodeGraphController.cpp` data-pin discovery) had been demoted to legacy-compatibility-only by decision #51. Removed both filter sites; sink-only properties on the Working Space node are still kept off the UI by the existing customEffect declared-parameter filter, so no behavior change for current effects. Old graphs that still carry `WsRedX_hidden` / `MonMaxNits_hidden` / `SdrWhiteNits_hidden` keys load with the keys present in memory but inert (no shader cbuffer references them; non-customEffect nodes never had them; customEffect nodes filter their Properties panel by declared parameters). Cross-version graph compatibility is not currently promised. Decision log entries #35 and #51 updated; copilot-instructions and resume.md updated. Also deleted `Graph/EffectNode2.h` (empty 0-byte orphan, never referenced) and the stray `output.jxr` test artifact at the repo root (already gitignored).
1011

1112
## [1.4.1] - 2026-05-05

Controls/OutputWindow.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
#include "pch.h"
44
#include "../Rendering/PipelineFormat.h"
5-
#include "../Rendering/ToneMapper.h"
65

76
namespace ShaderLab::Controls
87
{

README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ graph TB
7777
7878
subgraph Engine["ShaderLabEngine.dll"]
7979
EG[Graph/*]
80-
EV[GraphEvaluator / ToneMapper / FalseColorOverlay]
80+
EV[GraphEvaluator / FalseColorOverlay]
8181
FX[EffectRegistry / ShaderLabEffects / SourceNodeFactory]
8282
IO[ImageLoader / VideoSourceProvider / ShaderCompiler]
8383
MON[DisplayMonitor / ICC / GPU reduction]
@@ -375,6 +375,7 @@ The evaluator runs at the **monitor's refresh rate** (clamped to 60–240 Hz) vi
375375
| 51 | Working Space node is the single path for display tracking | Removed every per-effect "follow the live monitor" / "follow the working space" enum mode (`Current Monitor`, `Working Space` entries on `TargetGamut` / `SourceGamut` / `TargetRange`, plus `SdrWhiteSource` / `ClipSource` switches) and dropped all 5 host-side per-frame writer blocks (`PrimRed/Green/BlueX/Y_hidden`, `MonRed/Green/BlueX/Y_hidden`, `WsRed/Green/BlueX/Y_hidden`, `MonMin/MaxNits_hidden`, `(Ws)SdrWhiteNits_hidden`) from `MainWindow.xaml.cpp`. 12 ShaderLab effects now expose a `Custom` enum entry plus first-class `RedPrimary` / `GreenPrimary` / `BluePrimary` `Float2` parameters (gated by `visibleWhen`), and the SDR-white-consuming ICtCp effects use their existing numeric peak-nit parameters as the sole source. CIE Chromaticity Plot exposes its primaries unconditionally. To follow the active profile, the user wires the `Working Space` parameter node's analysis outputs into the relevant effect properties via the existing binding system. **Breaks saved graphs that used the old enum entries** — accepted because graph-format-version 2 doesn't compatibility-gate this. The `_hidden` filter that previously hid stale legacy keys was removed in the Phase-0 cleanup (decision #35); cross-version graph compatibility is no longer promised. | Day 11 |
376376
| 52 | ICtCp Tone Map gains a configurable `ToneLift` knob; the D2D `HdrToneMap` mid-tone lift is fixed and not adaptive | A/B testing on the *Colors of Journey* HDR clip (3442 → 1015 nits) showed `D2D HdrToneMap` *brightening* mid-tones by 38–350 % of source while ICtCp Tone Map slightly *darkened* them (−9 % to −24 %). Setting `D2D InputMaxLuminance` to 10000 vs the actual 3442 nits left the boost **unchanged** at 140 / 270 / 364 % — confirming D2D applies a fixed BT.2408-style "make HDR readable on SDR" dark-end lift unrelated to the source peak. ICtCp's anchored Möbius/Reinhard `f(I) = I / (1 + k·I)` (with `k = 1/peakOut − 1/peakIn`) is mathematically correct: `f(0) = 0`, `f(peakIn) = peakOut`, `f'(0) = 1`. The two effects encode different *philosophies* — neutral peak compression vs. opinionated readability lift — not different correctness. To let users opt in to the D2D look without losing the option of pure compression, ICtCp Tone Map gained a `ToneLift` parameter (default `0.0` = identity, range `[0..1]`) implementing an anchored polynomial mid-bump `f(x) = x + a·x·(1−x)` evaluated in normalized `[0, peakOut]` I-space. Polynomial form was chosen over `pow(I/peak, exp)` after rubber-duck critique: gamma has *infinite* slope at the toe (`f'(0) = ∞`), which would lift sensor noise and shadow detail aggressively; the polynomial has finite controlled slope `1 + a` at the toe. **Follow-up CIEDE2000 fidelity-to-source measurement (post-1.4.1) refined the recommended setting**: using the new `Delta E Comparator` `OutputMode = Grayscale dE` + `Luminance Statistics` live readout pipeline against the source video on three frames (T=8 / 30 / 60 s, source p95 spanning 0.4 → 124 nits), `ToneLift = 0.30` is the global mean-dE-to-source minimum, beating D2D HDR Tone Map by 9 / 32 / 9 % respectively. The dE-vs-ToneLift curve is U-shaped with a clear minimum at TL ≈ 0.3 in every frame. The earlier note that `ToneLift = 0.6` "matches D2D within ~10 %" was matching luminance histograms, not color fidelity — that setting actually *increases* mean dE relative to TL = 0.3 on this content. Default of `0.0` (neutral, no opinion) retained; `0.3` is the recommended starting point for users targeting best color accuracy. Effect version bumped 9 → 10. Also added a defensive `pqLms = saturate(pqLms);` in `ICtCpToScRGB`: out-of-domain LMS components (e.g. when ICtCp I is modified upstream and pushes an LMS row above 1) would otherwise hit `PQ_EOTF`'s denominator-zero region around `V ≈ 1.16` and emit NaN. Benefits all 8 ICtCp-using effects without changing in-domain behavior. | Day 11 |
377377
| 53 | `Delta E Comparator` gains an `OutputMode` parameter (`Heatmap` / `Grayscale dE`) so live mean-dE telemetry can be read straight off `Luminance Statistics` | The Turbo-colormap heatmap is a great visual but it's not directly readable as a scalar — `mean(R)` of a Turbo heatmap is meaningless because Turbo is non-monotonic in luminance (peaks at yellow ≈ 50 % dE, drops at red ≈ 100 %). Adding a second mode that writes `saturate(dE / MaxDeltaE)` to all RGB channels gives a true gray-scale dE image where `r.mean × MaxDeltaE` (read by a downstream `Luminance Statistics` node) IS the mean CIEDE2000 dE in Lab units. Decision: extend the existing effect rather than create a `Delta E Mean` reduction effect, because (a) heatmap + grayscale share 100 % of the dE math and a parameter switch is cheaper than two effects, (b) users normally want to flip between visual and quantitative views interactively while tuning a tone-mapper, (c) adding a parameter doesn't break old graphs (default `0 = Heatmap` matches v2 behavior). All cbuffer reads happen unconditionally before the mode branch (gotcha #2 in CLAUDE.md) so D3DCompile's `WARNINGS_AS_ERRORS` can't strip `OutputMode` if the agent only ever uses heatmap mode. Pairing pattern: `Source → DeltaE(OutputMode=1) ← Test`, then `DeltaE → Luminance Statistics`. Used to discover the `ToneLift = 0.30` optimum in decision #52. Effect version bumped 2 → 3. | Day 11 |
378+
| 54 | `Rendering/ToneMapper` class deleted | Decision #32 (Day 6) removed the built-in tone-mapper from the render path; the class itself lingered as default-`None` dead code through v1.4.1 — never instantiated at runtime, only included by `Controls/OutputWindow.h` for an unused declaration, but still cited by `.github/copilot-instructions.md` as the project's #1 development focus. That stale instruction was actively misleading new contributors / AI agents about where work happens. Phase-1 cleanup deletes `Rendering/ToneMapper.h` + `.cpp` (~280 LoC), removes the include from `OutputWindow.h`, and rewrites the project-identity / architecture / focus sections of `copilot-instructions.md` around the actual workflow: graph-built tone mappers (the ICtCp suite) plus the empirical fidelity loop (`Working Space` node + `Delta E Comparator` Grayscale dE + `Luminance Statistics`). The 5 historical operators (Reinhard, ACES Filmic, Hable, SDR Clamp, None) were never genuinely used on a live D2D context — the LUT-based ColorMatrix + TableTransfer path was a holdover from before the graph editor was usable. If a "filmic curve" operator is needed in future, it goes into `Effects/ShaderLabEffects.cpp` as a graph node, where it benefits from the Working Space binding system and the dE fidelity loop like every other effect. | Day 12 |
378379

379380
---
380381

0 commit comments

Comments
 (0)