Barrier log redesign

.claude/skills/add-render-pass/SKILL.md

@@ -0,0 +1,180 @@
+---
+name: add-render-pass
+description: Add a new render or compute pass to the FrameGraph end to end — the .sig PassNode and PSO declaration, code generation, the setup/render implementation, the HLSL shader, and pipeline registration. Use this skill whenever adding a new rendering effect, post-process, compute dispatch, shadow, or GBuffer stage, whenever a new PassNode or ComputePSO/GraphicsPSO is needed, or when an existing pass needs new resource reads/writes wired through the FrameGraph. Also use it when a newly added pass never executes, since that is usually a pipeline registration or setup-return problem rather than a bug in the render body.
+---
+
+# Adding a FrameGraph pass
+
+A pass is declared in a `.sig` file and implemented in C++, with the generator
+producing the glue between them. Getting the declaration right matters more
+than the render body — the declaration is what the FrameGraph uses to schedule
+the pass and to compute its barriers, so a wrong read/write flag produces
+validation errors or corrupt results that look like shader bugs.
+
+Read `sources/RenderSystem/Effects/Sky.cpp` alongside `sources/SIGParser/sigs/sky.sig`
+before starting. Between them they show both wiring styles, a graphics PSO and
+several compute PSOs, resource creation, and per-mip view handling — it is the
+best single reference in the tree.
+
+## 1. Declare in a `.sig` file
+
+Put the declaration in an existing `.sig` that matches the subsystem, or a new
+one in `sources/SIGParser/sigs/`.
+
+**Binding struct** — the shader-visible parameters. `[Bind = DefaultLayout::InstanceN]`
+selects the root-signature slot; distinct structs bound in the same pass need
+distinct instance slots:
+
+```
+[Bind = DefaultLayout::Instance0]
+struct MyEffectData
+{
+    float4 params;
+    Texture2D<float> depthBuffer;
+    RWTexture2D<float4> result;
+}
+```
+
+**PSO** — `compute = <name>` refers to `workdir/shaders/<name>.hlsl`, and
+`[EntryPoint = X]` selects the function within it. One shader file can back
+several PSOs through different entry points:
+
+```
+ComputePSO MyEffectCompute
+{
+    root = DefaultLayout;
+
+    [EntryPoint = CS]
+    compute = my_effect;
+}
+```
+
+**PassNode** — the FrameGraph declaration. Each member is a resource the pass
+touches; `[Write]` marks the ones it produces:
+
+```
+[Compute]
+PassNode MyEffect
+{
+    Texture GBuffer_Depth;
+    [Write] Texture ResultTexture;
+}
+```
+
+Resource names are matched **across passes by name** — that is how the graph
+links a producer to its consumers. Use the existing name for an existing
+resource; introducing a new spelling silently creates an unrelated resource.
+
+### PassNode flags, and the one that decides your implementation style
+
+- `[Compute]` — may run on the async compute queue. Whether it actually does is
+  decided by `[Async]` at the pipeline registration site, not here.
+- `[Required]` — never culled, even if nothing consumes its output.
+- `[Static]` — **this flag decides how you implement the pass.** It generates a
+  `PassDefault<Passes::X>` specialization declaring `setup` and `render`, which
+  you then define out-of-line. Without `[Static]` no such specialization exists
+  and the pass must be wired at runtime by assigning `setup_func`/`render_func`.
+  See `sources/SIGParser/templates/cpp/pass_defaults.jinja` for the exact rule.
+
+Choose `[Static]` when the pass is self-contained. Choose runtime wiring when
+the pass needs state owned by a C++ object — loaded textures, cached history
+buffers, persistent settings.
+
+## 2. Regenerate
+
+Use the `sig-regen` skill. In short: run the generator from `sources/SIGParser`,
+then run `generate_project.bat` because a new `PassNode` and PSO create new
+files that the projects don't yet list.
+
+## 3. Implement setup and render
+
+**setup** declares resource intent and returns whether the pass should run this
+frame. Returning `false` culls it — a pass that never executes is usually a
+`setup` returning `false`, not a broken render body.
+
+```cpp
+bool PassDefault<Passes::MyEffect>::setup(
+    Passes::MyEffect::Context& data, TaskBuilder& builder)
+{
+    builder.need(data.GBuffer_Depth, ResourceFlags::ComputeRead);
+    builder.need(data.ResultTexture, ResourceFlags::UnorderedAccess);
+    return true;
+}
+```
+
+- `builder.need(...)` declares use of a resource that already exists.
+- `builder.create(handle, desc, flags)` declares a resource this pass produces.
+- `ResourceFlags` are in `sources/RenderSystem/FrameGraph/FrameGraph.Base.ixx`:
+  `PixelRead`, `ComputeRead`, `DSRead`, `UnorderedAccess`, `RenderTarget`,
+  `DepthStencil`, `CopyDest`, `CopySource`, plus `Static` (persists across
+  frames rather than being transient) and `Required`.
+
+The flags must match what the pass actually does. They are the input to barrier
+computation, so declaring `ComputeRead` on something the shader writes produces
+a validation error whose message points at the barrier, not at the declaration.
+
+**render** records the work:
+
+```cpp
+void PassDefault<Passes::MyEffect>::render(
+    Passes::MyEffect::Context& data, FrameContext& context)
+{
+    auto& compute = context.get_list()->get_compute();
+
+    context.graph->set_slot(SlotID::FrameInfo, compute);
+
+    {
+        Slots::MyEffectData params;
+        params.GetDepthBuffer() = data.GBuffer_Depth->texture2D;
+        params.GetResult()      = data.ResultTexture->rwTexture2D;
+        compute.set(params);
+    }
+
+    compute.set_pipeline<PSOS::MyEffectCompute>();
+    compute.dispatch(context.graph->get_context<ViewportInfo>().frame_size, ivec2{ 16, 16 });
+}
+```
+
+Add `PROFILE(L"my_effect_dispatch")` scopes around logically distinct phases —
+setup versus dispatch versus per-mip loops — per the profiling convention in
+`CLAUDE.md`.
+
+### A binding trap worth knowing
+
+`builder.need()` wires the owning pass's own `data.X` FrameGraph handle;
+`pass_texture()` does not. Using a `pass_texture()`-declared handle through
+`data.X` inside that same pass's `render()` dereferences null and crashes with
+no D3D12 output at all. In that pass, use the owning `HAL::Texture` directly.
+
+## 4. Write the shader
+
+Create `workdir/shaders/<name>.hlsl` matching the PSO's `compute =`/`vertex =`/
+`pixel =` value, with a function named by `[EntryPoint = ...]`. Include the
+generated binding header so the struct layout stays in sync with the `.sig`.
+
+## 5. Register in a pipeline
+
+A declared pass does nothing until it appears in a `Pipeline` block. Order in
+the block is execution order; `[Async]` before an entry lets it overlap on the
+compute queue:
+
+```
+Pipeline AssetPipeline
+{
+    ...
+    [Async]
+    MyEffect;
+    ...
+}
+```
+
+Add it to every pipeline that should run it — `AssetPipeline` and the pipeline
+in `test.sig` are separate graphs and adding to one does not affect the other.
+Then regenerate again, since the pipeline block changed.
+
+## 6. Verify
+
+Use the `build-and-validate` skill. Confirm both that the pass runs and that it
+introduced no new D3D12 errors — a new pass with mismatched resource flags
+typically shows up as a per-frame repeating barrier error rather than as
+anything visible on screen.

.claude/skills/barrier-triage/SKILL.md

@@ -0,0 +1,94 @@
+---
+name: barrier-triage
+description: Diagnose D3D12 barrier, resource-layout, and resource-state validation errors in the operation-based barrier system (CmdListOperation / TrackedResourceState / CommandListGroup). Use this skill whenever the debug layer reports a barrier, layout, sync, access, or discard error — especially message IDs #1334, #1417, and #1422 — or when a resource is in an unexpected layout, a transition is rejected, a GPU hang or TDR is suspected to come from a missing barrier, or work in HAL.CommandList, HAL.CommandListRecorder, HAL.ResourceStates, or HAL.Queue needs its barrier consequences reasoned through.
+---
+
+# Barrier triage
+
+Barriers here are not emitted where the code asks for them. Work is recorded
+into `CmdListOperation` runs, and the barriers are *computed afterwards*, across
+a whole `CommandListGroup`, by `compile_transitions`. So the fix for a barrier
+error is almost never "insert a barrier at the call site" — it is a wrong
+declared state, a wrong resting layout, or a group-boundary handoff that the
+compiler was never told about.
+
+## Read the source before theorising
+
+The implementation is heavily commented and the comments carry the invariants,
+including the specific validation IDs each one prevents. Read the relevant part
+rather than reconstructing the design from the error text:
+
+| File | What it settles |
+|---|---|
+| `sources/HAL/HAL.ResourceStates.ixx` | `TrackedResourceState`, `CmdListOperation`, resting layout, `SubresRange` |
+| `sources/HAL/HAL.CommandList.ixx` | `CommandListGroup`, `PlannedResource`, how barriers are computed across lists |
+| `sources/HAL/HAL.CommandListRecorder.ixx` | How reserved barrier points map back to operations |
+| `sources/HAL/HAL.Queue.cpp` | Submission ordering, `execute(group)` |
+| `sources/HAL/API/D3D12/HAL.D3D12.Queue.cpp` | Where the compiled barriers reach the API |
+
+## The core model
+
+- A resource at rest sits in its **resting layout**, derived from what it was
+  created for (`resting_layout`). Every group leaves a resource there when
+  finished, so the next group can assume it without cross-group bookkeeping.
+- Within a group, lists hand a resource to each other **directly**. A resource
+  read by several consecutive lists does not bounce through the resting layout
+  at each list boundary.
+- A resource whose previous state cannot be known group-locally must have an
+  **entry state** declared (`has_entry_state` / `entry_state`). The FrameGraph
+  knows the whole pass graph, so it declares this for resources crossing a group
+  boundary. Unset means "assume resting", which is correct only when the group
+  model owns the resource end to end.
+- `compile_transitions` must run **exactly once per list**. A list processed
+  twice gets two independently computed barrier sets spliced into one command
+  stream, which produces contradictory `LayoutBefore` values.
+
+## Message IDs and what they actually mean
+
+**#1417 — `SyncBefore` is `NONE` on a resource that has already been accessed.**
+`NONE` is only legal for a resource entering from the creation/undefined layout.
+`CommandListGroup::plan_resources` decides this via `PlannedResource::from_undefined`.
+If you see this, the group believes it is the first-ever toucher of a resource
+that something already used — look at whether an earlier group touched it
+without being accounted for, or whether the resource was reset/recreated without
+its tracked state being reset.
+
+**#1422 — missing initializing discard on a resource's first write.**
+The group holding the first-ever write owns the discard
+(`PlannedResource::owns_discard`). This fires when the first write happens
+somewhere the planner didn't identify as the first write — commonly a resource
+written by a pass that isn't declared as a writer, or an aliased/transient
+resource whose first use after aliasing wasn't treated as initializing.
+
+**#1334 — `LayoutBefore` doesn't match the resource's actual layout.**
+The most common structural cause is a barrier group being emitted twice: the
+second run declares a `LayoutBefore` the first already moved past. `CmdListOperation::closed`
+exists specifically to prevent this — it guards against reserving
+`barriers_after` a second time when something appends to a list after
+`CommandList::end()` closed it. Also check for a resource used mid-frame in
+`COMMON` because it was created or deserialized lazily during a render pass
+rather than at init.
+
+## How to work a specific error
+
+1. Get the full message text, not just the ID — it names the resource and the
+   two layouts, which localises the problem far faster than the ID alone.
+2. Identify which pass or list touches that resource around the failure.
+3. Determine what the compiler *believed*: is this the group's first touch, is
+   there an entry state, is the resource declared as read or written by that
+   pass?
+4. Compare against what actually happens. The mismatch is the bug.
+5. Fix the declaration — the pass's read/write flags, the entry state, the
+   resting layout, or the planning decision — not the emitted barrier.
+
+## Verifying a fix
+
+Barrier reasoning is not self-verifying, so a fix is unproven until the debug
+layer agrees. Use the `build-and-validate` skill to build `Debug-D3D12`, run,
+and count errors by ID before and after. Report the counts.
+
+Be careful about one failure mode in particular: a change that suppresses an
+error by making a resource stop being tracked (rather than by making its
+transitions correct) shows up as a clean log while leaving a genuine missing
+barrier. If an error count drops sharply, confirm the resource is still being
+transitioned rather than merely no longer being visited.

.claude/skills/build-and-validate/SKILL.md

@@ -0,0 +1,106 @@
+---
+name: build-and-validate
+description: Build Spectrum and run it to capture real D3D12 debug-layer output, turning "this should work" into a verified result. Use this skill whenever a change needs to be proven — after editing HAL, FrameGraph, barrier, resource-state, or render pass code, when asked to build, compile, run, or test the engine, when checking whether a fix actually removed validation errors, or when reporting how many D3D12 errors a change fixed or introduced. Prefer this over reasoning about whether code compiles or whether barriers are correct.
+---
+
+# Build and validate
+
+For barrier, resource-state, and FrameGraph work the D3D12 debug layer *is*
+the test. Reasoning about whether a transition is legal is unreliable; running
+the engine and counting what the validation layer says is not. This skill is
+the loop that produces a trustworthy number.
+
+## Choosing a configuration
+
+Use **`Debug-D3D12`** for validation work. The plain `Debug` configuration and
+the `-Vulkan` variants will not produce D3D12 debug-layer output, so a clean
+result from them says nothing about barrier correctness.
+
+Available configurations are `Debug`, `Profile`, `Retail`, each also as
+`-D3D12` and `-Vulkan`, all `x64`.
+
+Note that the output path is derived from the *mode* only
+(`main.sharpmake.cs:202` uses `target.Mode`), so `Debug-D3D12` and
+`Debug-Vulkan` both produce `bin/debug/spectrum.exe`. The binary on disk gives
+no indication of which API it was built against, and a leftover Vulkan build
+runs perfectly while emitting no D3D12 messages at all. The `D3D12 SETUP` line
+described below is the only reliable way to tell them apart.
+
+## Building
+
+Locate MSBuild via vswhere rather than hardcoding a version, since the VS
+install path moves between machines and updates:
+
+```bash
+MSBUILD=$("/c/Program Files (x86)/Microsoft Visual Studio/Installer/vswhere.exe" -latest -requires Microsoft.Component.MSBuild -find 'MSBuild\**\Bin\MSBuild.exe' | head -1)
+"$MSBUILD" projects/spectrum.sln /p:Configuration=Debug-D3D12 /p:Platform=x64 /m /nologo /v:minimal
+```
+
+Building a single project (`projects/hal/hal.vcxproj`) is much faster and is
+the right call when iterating inside one layer. Build the full solution before
+reporting a change as done, because C++ module consumers frequently break in
+ways a single-project build never surfaces.
+
+Build times are long — allow several minutes and set generous tool timeouts
+rather than assuming a hang.
+
+## Running to capture validation output
+
+The engine registers an `ID3D12InfoQueue1` message callback that mirrors
+debug-layer messages into the log (`sources/HAL/API/D3D12/HAL.D3D12.Device.cpp`),
+so validation output is available from a plain console run without a debugger
+attached. The working directory must be `workdir/`, which is where assets,
+shaders, and the log live.
+
+Run it in the background and stop it after roughly 15 seconds. That is enough
+to get through device creation, asset load, and a number of steady-state
+frames, which is where essentially all validation errors appear. Waiting
+minutes adds repeated copies of the same messages and nothing new.
+
+```bash
+cd workdir && ../bin/Debug/spectrum.exe
+```
+
+Stop the process once it has run long enough, then read `workdir/log.txt`.
+
+## Reading the results
+
+Messages are written in a fixed shape, so counting is exact:
+
+```bash
+grep -cE "^D3D12 (ERROR|CORRUPTION)" workdir/log.txt
+grep -E "^D3D12 (ERROR|CORRUPTION|WARNING)" workdir/log.txt | sed -E 's/^(D3D12 [A-Z]+ #[0-9]+).*/\1/' | sort | uniq -c | sort -rn
+```
+
+The second command groups by severity and message ID, which is what makes a
+large error count actionable — 85,000 errors is routinely three distinct IDs
+repeating per frame, and the ID is what identifies the bug.
+
+## Guarding against a fake clean result
+
+A "zero errors" result is only meaningful if validation was actually running.
+Before believing it, confirm all of the following:
+
+- **The callback registered.** `grep "D3D12 SETUP" workdir/log.txt` must show
+  `message callback registered`. If it reports `ID3D12InfoQueue1 unavailable`,
+  nothing was captured and the run proves nothing.
+- **The log is from this run.** The engine appends across runs. Truncate or
+  delete `workdir/log.txt` before launching, or the counts mix old and new.
+- **The build actually succeeded.** A failed build leaves the previous
+  executable in place, so the run silently tests stale code. Check the MSBuild
+  exit code, don't just skim the output.
+- **The run got past device creation.** If the app crashed or exited in the
+  first second, few frames were submitted and most passes never ran.
+
+These are the ways this loop lies. Checking them costs seconds; skipping them
+produces a confident and wrong "fixed it."
+
+## Reporting
+
+Give the before and after counts and the distinct message IDs, not a
+qualitative summary. "Was 85518, now 0" and "the remaining 12 are all #1334 on
+PSSM_Depths" are the useful forms. If errors remain, say so plainly with the
+counts rather than describing the change as complete.
+
+To interpret a specific barrier or layout error once you have the ID, use the
+`barrier-triage` skill.