Skip to content

Commit 8f5a2fe

Browse files
Spruill-1Copilot
andcommitted
Phase 0: cleanup (_hidden filter, EffectNode2.h, output.jxr)
- Delete the `_hidden` suffix filter sites from MainWindow.xaml.cpp (Properties panel) and Controls/NodeGraphController.cpp (data-pin discovery). Sink-only properties on the Working Space node remain hidden via the existing customEffect declared-parameter filter, so no behavior change for current effects. Old graphs that still carry stale `_hidden` keys load with the keys present in memory but inert. - Delete empty 0-byte orphan Graph/EffectNode2.h (never referenced, never in any vcxproj). - Remove stray output.jxr test artifact from repo root (already in .gitignore). - Update README decision log #35 (retired) and #51, README Working Space section, .context/resume.md, .github/copilot-instructions.md, and CHANGELOG.md [Unreleased]. Cross-version graph compatibility is not currently promised. All 58 ShaderLabTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 00e144a commit 8f5a2fe

7 files changed

Lines changed: 10 additions & 11 deletions

File tree

.context/resume.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ Every effect carries a stable `effectId` + numeric `effectVersion`; saved graphs
5757
- `PropertyValue` variant: `float`, `int32`, `uint32`, `bool`, `wstring`, `float2`, `float3`, `float4`, `D2D1_MATRIX_5X4_F`, `vector<float>`.
5858
- Per-component property bindings (Grasshopper-style data flow), with array (whole-vector) bindings for LUT-shaped fields.
5959
- Enum labels for named dropdown parameters; `bool` rendered as `ToggleSwitch`.
60-
- `_hidden` suffix convention (legacy / compatibility only after decision #51): properties ending in `_hidden` are excluded from the UI and data pins. Older saved graphs may carry stale `WsRedX_hidden` / `MonMaxNits_hidden` / `SdrWhiteNits_hidden` keys; those are inert because the shader cbuffer no longer references them and the host writers were removed. New effects must not declare `_hidden` parameters; use the `Working Space` node + bindings instead.
60+
- No `_hidden` suffix convention (removed in Phase-0 cleanup, v1.4.x). Earlier saved graphs may carry stale `WsRedX_hidden` / `MonMaxNits_hidden` / `SdrWhiteNits_hidden` keys; those load into memory but are inert (no shader cbuffer references them, no UI surfaces them). Cross-version graph compatibility is not currently promised. Sink-only properties (e.g., the Working Space node's `ActiveColorMode`, `SdrWhiteNits`, primaries) live in `ShaderLabEffectDescriptor::hiddenDefaults` without the `_hidden` suffix and are kept off the UI by the customEffect declared-parameter filter.
6161
- `visibleWhen` conditional visibility on parameters (`"Mode == 1"`, `"Strength > 0"`, etc.).
6262
- Visual data pins (orange diamonds) on the node graph for binding connections.
6363

.github/copilot-instructions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ The built-in effects library lives in `Effects/ShaderLabEffects.h/.cpp`:
147147
- **Categories**:
148148
- **Analysis** (4): Luminance Heatmap (PS), Out-of-Gamut Highlight (CS), CIE Chromaticity Plot (CS), Vectorscope (CS)
149149
- **Source** (5): Gamut Source (PS), Color Checker (PS), Zone Plate (PS), Gradient Generator (PS), HDR Test Pattern (PS)
150-
- **Hidden defaults**: Host-managed properties (e.g., monitor primaries for OOG Highlight) are marked `isHidden = true` in `ParameterDefinition`. They don't appear in the Properties panel but are written by the host before each evaluate.
150+
- No `_hidden` suffix convention. (Removed in Phase-0 cleanup. Sink-only properties e.g., the Working Space node's `ActiveColorMode`, `SdrWhiteNits`, primaries — live in `ShaderLabEffectDescriptor::hiddenDefaults` without any `_hidden` suffix, and are kept off the UI by the customEffect declared-parameter filter. The Working Space node + property bindings is the only path for tracking the active display profile.)
151151
- **Analysis outputs**: Compute shader analysis effects declare `AnalysisFieldDef` entries that describe their output fields (name, type, count). These are read back to CPU via `GraphEvaluator` and can be bound to downstream properties.
152152

153153
## Effect Designer

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@
33
All notable changes to ShaderLab will be documented in this file.
44
Format follows [Keep a Changelog](https://keepachangelog.com/).
55

6+
## [Unreleased]
7+
8+
### Removed
9+
- **`_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).
10+
611
## [1.4.1] - 2026-05-05
712

813
### Added

Controls/NodeGraphController.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -648,9 +648,6 @@ namespace ShaderLab::Controls
648648
// analysis fields as outputs. Build name (for binding lookup) and label (for display).
649649
for (const auto& [key, val] : node.properties)
650650
{
651-
// Skip hidden properties (internal cbuffer plumbing).
652-
if (key.size() > 7 && key.ends_with(L"_hidden"))
653-
continue;
654651
// For nodes with a customEffect, only show input pins for properties
655652
// that correspond to a declared parameter. Properties that exist
656653
// solely as host-driven bootstrap values (e.g. Working Space, fed

Graph/EffectNode2.h

Whitespace-only changes.

MainWindow.xaml.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3878,9 +3878,6 @@ namespace winrt::ShaderLab::implementation
38783878
// Math Expression: Expression is rendered above as a dedicated control.
38793879
if (isMathExpression && key == L"Expression")
38803880
continue;
3881-
// Skip hidden properties (convention: name ends with _hidden).
3882-
if (key.size() > 7 && key.ends_with(L"_hidden"))
3883-
continue;
38843881
// For nodes with a customEffect, only render properties that
38853882
// correspond to a declared parameter. Properties that exist
38863883
// solely as host-driven bootstrap values (e.g. Working Space,

README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ The evaluator runs at the **monitor's refresh rate** (clamped to 60–240 Hz) vi
356356
| 32 | Remove built-in tone mapper | Pipeline passes raw scRGB through. Users build tone mappers as graph effects. Full accuracy by default. | Day 6 |
357357
| 33 | Multi-output windows via OutputWindow | Each Output node gets its own SwapChainPanel window. Shares D2D device context. Bidirectional sync: close window ↔ delete node. | Day 6 |
358358
| 34 | Effect versioning with effectId/effectVersion | Stable IDs survive renames. Per-node and batch upgrade buttons. Preserves user property values on upgrade. | Day 6 |
359-
| 35 | _hidden suffix convention | Properties ending in `_hidden` excluded from UI. Replaces prefix-based filtering. **Originally** used for per-effect working-space and OS-driven cbuffer injection; **superseded by decision #51** (Working Space node + bindings) but the filter is retained because legacy saved graphs may still carry stale `_hidden` keys that need to remain inert in the Properties panel and pin list. | Day 6 |
359+
| 35 | _hidden suffix convention (retired) | Originally used to hide host-managed working-space cbuffer properties from the Properties panel and pin list. **Retired in v1.4.x**superseded by decision #51 (Working Space node + bindings) and then fully removed in the Phase-0 cleanup. The filter no longer exists in the codebase. Saved graphs that still carry stale `_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, so stale keys remain invisible). Old graph compatibility is not promised. | Day 6 / retired Day 12 |
360360
| 36 | D3D11 compute via evaluator dispatch | Bypass D2D tiling for reduction operations. GpuReduction uses 32×32 thread group with groupshared memory. Only 32 bytes read back to CPU. | Day 6 |
361361
| 37 | ID2D1StatisticsEffect COM interface | D2D-compatible effect wrapper with custom interface for analysis results. Pass-through pixel shader + D3D11 compute dispatch. | Day 6 |
362362
| 38 | Parameter nodes as data-only graph elements | No shader, evaluator handles directly. Teal color, inline slider, don't switch preview. Four types: Float, Integer, Toggle, Gamut. | Day 6 |
@@ -372,7 +372,7 @@ The evaluator runs at the **monitor's refresh rate** (clamped to 60–240 Hz) vi
372372
| 48 | ExprTk feature-disable macros for Release safety | The MSVC Release optimizer crashed in ExprTk's regex / IO paths on first evaluation. Defining `exprtk_disable_string_capabilities`, `exprtk_disable_rtl_io`, `exprtk_disable_rtl_io_file`, `exprtk_disable_rtl_vecops`, `exprtk_disable_enhanced_features`, and `exprtk_disable_caseinsensitivity` before `#include`-ing `exprtk.hpp` (with PCH disabled on `MathExpression.cpp`) keeps the math-only core and eliminates the crash. | Day 9 |
373373
| 49 | Switch ICC reader to mscms.dll | Removed the in-house ICC binary parser in favor of `OpenColorProfileW` + `GetColorProfileElement` (mscms.dll). We still interpret the small XYZType / textDescriptionType / multiLocalizedUnicodeType tag bodies, but mscms owns container layout, tag addressing, and v2/v4 version handling. Public `IccProfileParser::LoadFromFile` API and `IccProfileData` struct are unchanged. Engine link list gains `mscms.lib`. | Day 10 |
374374
| 50 | Refresh-rate-driven render loop (60\u2013240 Hz) | Render `DispatcherQueueTimer` interval is now derived from the active monitor's `dmDisplayFrequency` (via `MonitorFromWindow` \u2192 `GetMonitorInfoW` \u2192 `EnumDisplaySettingsW`), clamped to [60, 240] Hz, refreshed on every display change. 120 / 144 / 165 / 240 Hz panels and high-FPS video sources run at native cadence. Interval is set in microseconds so non-integer-ms periods stay accurate. | Day 10 |
375-
| 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 and the stale `_hidden` properties on loaded nodes are inert (the shader cbuffer no longer references them and the `_hidden` filter hides them from the UI). | Day 11 |
375+
| 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 |
378378

@@ -1149,7 +1149,7 @@ The active display profile (live OS-reported caps or any simulated preset / ICC
11491149
- **Single source of truth**: The Working Space node's 14 typed analysis output fields mirror the active profile — `ActiveColorMode` (0=SDR, 1=WCG/ACM, 2=HDR), `HdrSupported`, `HdrUserEnabled`, `WcgSupported`, `WcgUserEnabled`, `IsSimulated`, `SdrWhiteNits`, `PeakNits`, `MinNits`, `MaxFullFrameNits`, plus the four CIE-xy primaries `RedPrimary` / `GreenPrimary` / `BluePrimary` / `WhitePoint` (each Float2).
11501150
- **Updated by `MainWindow::UpdateWorkingSpaceNodes()`**, which runs on `ApplyDisplayProfile`, `RevertToLiveDisplay`, the display-change callback, and once per render tick. Only marks the node dirty when at least one field actually changed, so binding consumers re-evaluate on profile changes only.
11511151
- **Bind, don't hide**: Effects that need to know the working-space primaries or peak nits expose them as bindable `Float2` / `Float` parameters (typically a `Custom` enum mode that gates them via `visibleWhen`, plus a few static convenience modes like `sRGB` / `BT.2020`). Wire those parameters from the Working Space node to follow the live profile, or set them by hand for strict static analysis. There is no "follow the working space" toggle anymore — wiring the binding **is** the toggle.
1152-
- **Legacy `_hidden` properties** (e.g. `WsRedX_hidden`, `MonMaxNits_hidden`, `SdrWhiteNits_hidden`) are no longer written by the host and are not declared on any current effect. The `_hidden` suffix filter is retained so older saved graphs that still carry these keys load cleanly with the stale entries inert and invisible.
1152+
- **No legacy `_hidden` filter**: properties ending in `_hidden` used to be host-managed cbuffer slots filtered from the UI. Decision #51 stopped writing them; a Phase-0 cleanup deleted the filter sites. Old graphs still load — the stale keys sit in memory inert. Cross-version graph compatibility is not promised right now.
11531153

11541154
## Image Statistics Effect
11551155

0 commit comments

Comments
 (0)