diff --git a/docs/adr/0001-viewport-color-correction.md b/docs/adr/0001-viewport-color-correction.md new file mode 100644 index 0000000..1a9e4cb --- /dev/null +++ b/docs/adr/0001-viewport-color-correction.md @@ -0,0 +1,183 @@ +# ADR 0001 — Custom Viewport Color Correction (sRGB / OCIO) + +- **Status:** Accepted +- **Date:** 2026-06-27 +- **Component:** `src/core/UsdSceneRenderer.cpp` (`ViewportColorCorrector`, `ApplyViewportColorCorrection`) +- **Commit:** `fb1ae9d` + +## Context + +The viewport renders a USD stage through `UsdImagingGLEngine` (Hydra / Storm) +into an offscreen `GlfDrawTarget`, whose color texture ImGui samples for +display. Color management (linear → display) was delegated to Hydra's +`HdxColorCorrectionTask` via `UsdImagingGLEngine::SetColorCorrectionSettings()`, +configured by the per-viewport `ColorCorrectionMode` (Disabled / sRGB / +OpenColorIO) plus OCIO display/view/colorspace/look. + +Two problems made the OCIO path unusable: + +1. **OCIO output was black.** sRGB correction rendered correctly through the + same color AOV, proving the correction *input* was fine — the hdx OCIO + shader/LUT path itself emitted black. The exact cause stayed opaque because + USD reports it via `TF_WARN`, which this GUI app discarded (no console). + +2. **The failure was sticky.** OCIO resources are built lazily *inside* + `Render()` (`HdxColorCorrectionTask::Sync`), not in `SetColorCorrectionSettings()`. + A throw there escaped `UsdSceneRenderer::Render()` and skipped + `m_drawTarget->Unbind()`, permanently unbalancing the `GlfDrawTarget` + bind/restore stack — so every later frame, **including after switching back + to sRGB**, stayed black until an app restart. + +We do not control the bundled USD build, so fixing `HdxColorCorrectionTask` +itself was not an option. App and USD both link OpenColorIO 2.1, and a usable +ACES 1.2 config is bundled and pointed to via `$OCIO`. + +## Goals / Non-Goals + +**Goals:** +- Correct, non-black OCIO display transforms in the viewport (ACES 1.2). +- sRGB output visually identical to the previous hdx sRGB path. +- A color-management failure can never permanently corrupt the viewport. +- Overlays (grid, axis, bbox, camera/light gizmos) remain display-referred, + drawn on top of the corrected image — unchanged from the prior behavior. + +**Non-Goals:** +- Replacing Hydra's *rendering* — only its color-correction stage is replaced. +- Exposure/gamma dynamic-property UI, or interactive grading. +- Applying an explicit OCIO look override beyond the view's own looks. +- Color-managing the ImGui UI chrome outside the viewport image. + +## Decision + +### D1 — Replace `HdxColorCorrectionTask` with our own GL post-process + +**Decision:** Hydra always renders **linear** (`colorCorrectionMode = "disabled"`). +A custom fullscreen pass (`ViewportColorCorrector`) applies the correction +afterwards, fully under our control. + +**Rationale:** The hdx OCIO path is a black box we cannot patch in the bundled +USD. Owning the post-process gives deterministic, debuggable behavior and +removes the lazy-throw-inside-Render hazard entirely. sRGB and OCIO now share +one code path with consistent ordering. + +--- + +### D2 — Render linear into an `RGBA16F` color attachment + +**Decision:** The draw-target color attachment was changed from `GL_RGBA` +(8-bit unorm) to `GL_RGBA16F`. + +**Rationale:** We now store the *linear* scene as the intermediate. Display +transforms (especially ACES) need scene values outside `[0,1]`; an 8-bit unorm +buffer would clip highlights and band the shadows before correction even runs. +16F is ample headroom at negligible cost. ImGui and `CaptureFrame` sample/read +it unchanged. + +--- + +### D3 — OCIO via the OCIO GPU API (`GpuShaderDesc`), LUTs uploaded by us + +**Decision:** For OCIO mode, build the transform directly with OCIO 2.1: +`config->getProcessor(srcColorSpace, display, view, FORWARD)` → +`getDefaultGPUProcessor()` → `extractGpuShaderInfo(GpuShaderDesc)` with +`GPU_LANGUAGE_GLSL_1_3` and function name `OCIODisplay`. We compile the +generated GLSL into our fragment shader and upload/bind every 1D/2D/3D LUT it +requests as GL textures. The source colorspace defaults to the `scene_linear` +role when unset. + +**Rationale:** This is exactly what `HdxColorCorrectionTask` does internally, +but visible and ours to debug. `GLSL_1_3` matches the project's existing +`#version 130` shaders and emits modern `texture()` calls. The program and LUTs +are cached and rebuilt only when display/view/colorspace/look change. + +--- + +### D4 — Graceful degradation to sRGB; never black + +**Decision:** Any OCIO failure — processor build, shader compile/link, or LUT +upload — logs the reason and falls back to the sRGB encode shader for that +frame. The failing key is remembered so we do not retry (and re-spam logs) +until the parameters change. + +**Rationale:** A misconfigured display/view must degrade to a usable image, not +a black viewport. sRGB is the correct neutral fallback and matches the default. + +--- + +### D5 — Implemented inline in `UsdSceneRenderer.cpp` (pimpl), not new TU/files + +**Decision:** `ViewportColorCorrector` is a class defined in +`UsdSceneRenderer.cpp`, held via `std::unique_ptr` behind a forward declaration +in the header. + +**Rationale:** `CORE_SOURCES` is a `file(GLOB_RECURSE ...)`; adding a new `.cpp` +forces a CMake reconfigure, which risks re-triggering the heavy Cycles +`ExternalProject` build. Keeping it in an existing TU avoids that while the +pimpl keeps OCIO/GL includes out of the public header. The corrector follows +the file's existing `Init*/Destroy*` GL-resource lifecycle conventions. + +--- + +### D6 — Route USD diagnostics to the app log + +**Decision:** Install a process-wide `TfDiagnosticMgr::Delegate` +(`InstallUsdDiagnosticLogger`) that forwards USD `TF_ERROR` / `TF_WARN` / +`TF_STATUS` to the app logger with a `[USD]` prefix. + +**Rationale:** USD's own diagnostics were invisible in this GUI app. This made +the original OCIO failure undiagnosable and is generally useful for any Hydra +issue. + +## Pipeline + +Per frame, after Hydra renders linear into the draw target: + +1. `m_drawTarget->Resolve()` — resolves MSAA into the color attachment (no-op + when MSAA is off), giving a single-sample linear image. +2. Blit that attachment into `m_ccLinearTex` — a separate copy is required + because sampling and writing the same texture in one pass is illegal. +3. Bind the draw-target render FBO (the MSAA FBO when multisampling) and draw + the corrected fullscreen triangle sampling `m_ccLinearTex`. +4. Overlays draw on top (uncorrected), then the final `Resolve()` in + `GetColorTextureID()` produces the texture ImGui samples. + +This keeps overlays display-referred exactly as before, and the correction is +MSAA-correct (a flat fullscreen quad resolves to the same value per sample). + +## Consequences + +**Positive:** +- OCIO display transforms work; OCIO failures degrade to sRGB, never black. +- A color-correction failure can no longer corrupt the bind stack — the throw + hazard is gone because Hydra no longer does OCIO. +- The full transform is inspectable/loggable in our own code. +- HDR-linear intermediate enables correct tone-mapped output. + +**Negative / trade-offs:** +- We reimplement and now maintain OCIO GPU plumbing (LUT upload, sampler + binding) that USD would otherwise own; OCIO major-version API changes could + require updates. +- One extra fullscreen pass, one blit, and an `RGBA16F` buffer per viewport. +- OCIO dynamic properties (exposure/gamma) and explicit look overrides are not + wired up. +- Logic lives in `UsdSceneRenderer.cpp` rather than its own module, by D5. + +## Alternatives Considered + +- **Keep `HdxColorCorrectionTask`, fix configuration.** Rejected: the failure + was inside the bundled USD's GPU/LUT integration, not our parameters; we + cannot patch it, and sRGB-correct/OCIO-black pointed at the hdx shader path. +- **Custom sRGB only, keep hdx for OCIO.** Rejected: OCIO is the actual + requirement; a custom sRGB-only path wouldn't address it. +- **Bake the OCIO transform to a single 3D LUT on the CPU.** Rejected: loses + HDR shaper handling and precision that `GpuShaderDesc` manages correctly. +- **New `ViewportColorCorrection.{h,cpp}` module.** Rejected for now (D5) to + avoid a CMake reconfigure / Cycles rebuild; revisit if the file grows. + +## Verification + +Built clean (Release) and confirmed in-app by the maintainer: OCIO produces the +expected corrected (non-black) image and sRGB is unchanged. Headless automated +verification is blocked by the stale test-target CMake config (missing USD +include dirs); restoring those targets would let an OCIO-vs-sRGB center-pixel +test assert this automatically.