Skip to content

fix(engine,shader): handle matrix3d transforms and hide non-first scenes#374

Open
vanceingalls wants to merge 1 commit intovance/refactor-helpersfrom
vance/transform-clipping
Open

fix(engine,shader): handle matrix3d transforms and hide non-first scenes#374
vanceingalls wants to merge 1 commit intovance/refactor-helpersfrom
vance/transform-clipping

Conversation

@vanceingalls
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls commented Apr 21, 2026

Summary

Two correctness fixes in the HDR transform & clipping pipeline: parseTransformMatrix now handles matrix3d(...) (GSAP's default force3D: true), and shader-transitions sets every non-first scene to opacity: 0 at t=0 so the engine doesn't over-composite at the start.

Why

Chunk 4 of plans/hdr-followups.md. Transform extraction and border-radius computation existed but were dead — an HDR video with rotation: 45 rendered un-rotated, and 3-scene compositions ghosted at t=0 because every scene defaulted to CSS opacity: 1 and contributed to the first frame.

What changed

Matrix3d support in parseTransformMatrix. DOMMatrix.toString() emits matrix3d whenever any ancestor in the chain has used a 3D transform — most importantly GSAP's default force3D: true, which converts translate(...) into translate3d(..., 0). Without this, every GSAP-driven transform was silently dropped during HDR compositing because videoFrameInjector.getViewportMatrix() would return matrix3d(...) and the blit path would parse it as null and fall back to identity. The 16-value column-major form is converted to its 2D affine projection (indices 0, 1, 4, 5, 12, 13 → m11, m12, m21, m22, m41, m42); Z, perspective, and out-of-plane rotation components are dropped.

Initial-state opacity in initEngineMode. The browser preview branch uses a GL canvas overlay during transitions, so scene opacity at t=0 doesn't matter visually. The engine branch reads scene opacity directly via queryElementStacking() to decide which layers to composite. Without an explicit initial-state tween, every scene defaulted to CSS opacity: 1 and contributed to the very first frame, causing ghosting/overlap until the first transition fired. tl.set() at position 0 anchors the initial state in the timeline graph so reverse seeks from inside a later transition restore it correctly.

These two fixes together make el.transform and el.borderRadius (already wired in Chunk 7A's compositeHdrFrame) actually flow through the GSAP-animated case, and keep the engine's per-frame compositing aligned with what the user sees in browser preview.

Test plan

  • 6 new alphaBlit.test.ts cases (identity matrix3d, translate3d, scale + translate3d, rotateZ, malformed arg count, non-finite values).
  • Existing hdr-regression Window H already CSS-sets #scene-b { opacity: 0 } as a fallback; the new tl.set is redundant for that case but harmless and removes the need for compositions to remember the CSS workaround.
  • Manual: rotated HDR video (rotation: 45) appears rotated; border-radius: 50% clips to circle; 3-scene composition has no overlap at t=0.

Stack

Chunk 4 of plans/hdr-followups.md. Window F of the regression suite documents the bug; the next PR in the stack tightens the maxFrameFailures budget to 0.

Copy link
Copy Markdown
Collaborator Author

vanceingalls commented Apr 21, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch 2 times, most recently from f328731 to 79b029f Compare April 21, 2026 20:54
@vanceingalls vanceingalls force-pushed the vance/refactor-helpers branch from 4f4682b to 78a6af9 Compare April 21, 2026 20:54
@vanceingalls vanceingalls marked this pull request as ready for review April 21, 2026 20:57
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from 79b029f to e78f981 Compare April 21, 2026 22:37
@vanceingalls vanceingalls force-pushed the vance/refactor-helpers branch from 3838031 to 95df8c6 Compare April 22, 2026 01:16
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch 2 times, most recently from 45009c4 to cb840e7 Compare April 22, 2026 02:03
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two real fixes, both structural:

matrix3d in parseTransformMatrix — previously returned null for any 3D matrix, which meant every GSAP composition with force3D: true (GSAP's default for transform tweens) fell off the deterministic engine path and rendered with identity transforms on the HDR compositor. The spec-correct approach is: parse the 16-element column-major matrix, extract the 2D affine components (a=m[0], b=m[1], c=m[4], d=m[5], tx=m[12], ty=m[13]), drop Z-translation, and validate all six extracted components are finite. Tests pin the three common GSAP emission shapes (identity, translate3d, scale + translate3d, rotateZ) plus malformed rejection (wrong arg count, NaN values). Exactly the right coverage.

Worth documenting in code (or the plans doc) that this is a 2D projection of the 3D transform — any composition that actually uses Z-depth (perspective, 3D rotation around X/Y axes) will silently lose those components in the engine path. For force3D: true with a flat Z this is invisible; for a genuine 3D scene the engine just doesn't support it, which was already the case before this PR but is now hidden behind a "parses successfully" result instead of a null. If a Z-significant transform ever shows up in a regression test, the engine will render it flat without flagging that it threw away the Z info. Non-blocking, but worth a log warning when m[8..11] are non-identity.

Non-first scenes start at opacity: 0 in hyper-shader.ts — the fix for the Window F corruption in #365. Without this, non-first scenes were visible during scene-A capture and bled into the HDR compositor's first frame. Cleaner than the alternative (hiding scenes via DOM manipulation) because it stays inside the shader-transitions package's opacity contract.

CI failures here are the stack-wide Format and Render on windows-latest — not specific to this change. The Windows failure across the whole upper stack is worth running down separately (could be LFS-check issue after #376, or could be unrelated).

Approved.

Rames Jusso

@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from cb840e7 to c37d8a5 Compare April 22, 2026 03:03
@vanceingalls vanceingalls force-pushed the vance/refactor-helpers branch from 34e108c to 51aaf25 Compare April 22, 2026 03:03
@vanceingalls vanceingalls changed the base branch from vance/refactor-helpers to graphite-base/374 April 22, 2026 03:28
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from c37d8a5 to a6f91b0 Compare April 22, 2026 03:34
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from a6f91b0 to 43ed55b Compare April 22, 2026 04:45
@vanceingalls
Copy link
Copy Markdown
Collaborator Author

Review feedback follow-up

@jrusso1020's non-blocking suggestion is addressed: parseTransformMatrix now warns when a matrix3d arrives with Z-significant components.

packages/engine/src/utils/alphaBlit.ts:

function warnIfZSignificant(parts: number[]): void {
  if (warnedZSignificant) return;
  // For a flat 2D transform — the only thing this engine path can render
  // faithfully — we expect:
  //   a3 = b3 = c1 = c2 = 0   (no XZ/YZ rotation coupling)
  //   c3 = 1                  (no Z scaling)
  //   d1 = d2 = d3 = 0        (no perspective)
  //   d4 = 1                  (no homogeneous scaling)
  // Z translation (c4 = parts[14]) is explicitly dropped by the 2D affine
  // extraction below — that's the whole point of supporting GSAP's
  // `force3D: true` translate3d(x, y, 0) emission — so it is NOT flagged.
  // ...component checks against Z_EPSILON...
  if (...non-trivial 3D components detected...) {
    warnedZSignificant = true;
    console.warn(
      `[alphaBlit] parseTransformMatrix received a matrix3d with non-trivial 3D components ` +
        `(a3=${a3}, b3=${b3}, c1=${c1}, c2=${c2}, c3=${c3}, d1=${d1}, d2=${d2}, d3=${d3}, d4=${d4}). ` +
        `The engine projects 3D transforms to 2D (m11, m12, m21, m22, m41, m42) and silently ` +
        `discards perspective and out-of-plane rotation. ...`,
    );
  }
}

Key design points (addressing the staff-eng nod):

  • force3D: true happy path is silenttranslateZ (parts[14]) is intentionally not in the check, so GSAP's flat 2D output via matrix3d(...) doesn't trigger the warning.
  • Once-per-process rate limit (warnedZSignificant module flag) so a 60fps render with a real 3D rotation doesn't flood logs.
  • All 9 detector components are echoed back in the warn line (a3, b3, c1, c2, c3, d1, d2, d3, d4) so the author can identify which 3D feature they're using.

Test coverage in alphaBlit.test.ts:

  • warns once when matrix3d has Z-significant components (rotateY 45deg) — pinned the rotateY case as the canonical 3D-significant emission.
  • Existing force3D: true tests confirm translate3d, scale + translate3d, and rotateZ all stay quiet.

Stack-wide CI: ✅ green now (the Format and Render on windows-latest failures you flagged were resolved on subsequent commits in the stack).

@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from 43ed55b to 2060489 Compare April 22, 2026 17:30
@vanceingalls vanceingalls force-pushed the graphite-base/374 branch 2 times, most recently from 16713e6 to a8546d5 Compare April 22, 2026 18:35
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from 2060489 to 30f0fc9 Compare April 22, 2026 18:35
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch 2 times, most recently from 44be838 to e5271b7 Compare April 22, 2026 18:59
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch 2 times, most recently from 558326d to d5c4402 Compare April 22, 2026 20:45
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from d5c4402 to 6d3cc3f Compare April 22, 2026 20:48
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from 6d3cc3f to 0d0a913 Compare April 22, 2026 22:13
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from 0d0a913 to c0cfdcf Compare April 22, 2026 22:48
@graphite-app graphite-app Bot changed the base branch from graphite-base/374 to vance/refactor-helpers April 22, 2026 22:48
Address jrusso1020's nit on PR #365 (non-blocking review): both READMEs now
explain where the tolerance values come from.

- hdr-regression/README.md: add a budget-breakdown table that derives the 30
  frames from the deltas in PRs #369 (window C fix → 5) and #375 (window F
  fix → 0). The table doubles as a contract: if a future change forces the
  budget back up, exactly one bucket has regressed and the table tells you
  which one to investigate first.
- hdr-hlg-regression/README.md: add a 'Tolerance' section explaining why 0
  is the right floor (HLG is a pure pass-through path, HEVC over rgb48le is
  byte-deterministic on the same fixture, so any drift is a real regression).

The regeneration command for generate-hdr-photo-pq.py was already documented
at README lines 67-71, so no changes needed there.
@vanceingalls vanceingalls force-pushed the vance/transform-clipping branch from c0cfdcf to 7f0411c Compare April 22, 2026 23:26
@vanceingalls vanceingalls force-pushed the vance/refactor-helpers branch from fe8806d to d22e419 Compare April 22, 2026 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants