Skip to content

feat(studio): add professional grading controls - #2797

Merged
ukimsanov merged 3 commits into
mainfrom
feat/studio-professional-color-grading
Jul 26, 2026
Merged

feat(studio): add professional grading controls#2797
ukimsanov merged 3 commits into
mainfrom
feat/studio-professional-color-grading

Conversation

@ukimsanov

@ukimsanov ukimsanov commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

What

  • Add Studio scopes, three-way wheels, master/RGB and hue curves, HSL secondary controls, and processing-order-aware LUT placement.
  • Add exact look thumbnails, live hover preview, transactional inputs, secondary matte/bypass, and current-frame eyedropper sampling.

Why

The advanced runtime needs a direct, inspectable Studio surface for manual review, before/after comparison, and agent-output verification.

How

  • Reuse the existing color-grading controller, runtime preview, history, file-write, and render paths.
  • Generate preset thumbnails from the same resolved payload used by hover/apply.
  • Capture scopes on open, after a debounced committed change, or on manual refresh; no continuous polling loop.
  • Preserve independent Effects, Palette, and user LUT state when applying/resetting grading.
  • Keep all production components below Studio's 600-line architecture gate.

Test plan

  • Unit tests added/updated
  • Focused Studio grading/gesture tests: 75 passing
  • Studio typecheck/build, oxlint, oxfmt, Fallow, and repository hooks
  • Full Studio regression run: 2,809 passing; one unchanged localStorage test-order leak passes in isolation
  • Manual heygen-showcase selection, scopes, edit, Undo, playback, and render-parity checks

Stack: 3/3. Base: feat/agent-native-color-grading. Merge after #2798.

Copilot AI review requested due to automatic review settings July 26, 2026 05:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a more “pro” color-grading surface to Studio (scopes + wheels/curves + HSL secondary sampling) while reusing the existing grading controller/preview runtime so grading can be inspected, previewed, and applied consistently with render output.

Changes:

  • Introduces new grading UI primitives (transactional numeric input, 3-way wheels, RGB/hue curves, secondary qualifier/correction with eyedropper sampling) and integrates them into the Flat grading section.
  • Adds capture-based scopes (histogram/waveform/RGB parade/vectorscope) with debounced capture/refresh behavior (no polling).
  • Improves preview/thumbnail generation to be pipeline-order-aware (effects/palette included for preset thumbnails) and supports frame capture for sampling/scopes.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/studio/src/icons/SystemIcons.tsx Adds Eyedropper/Trash icons used by new grading UI.
packages/studio/src/components/editor/useInspectorGestureTransaction.ts Adds useInspectorGestureDraft for transactional draft state during gesture edits.
packages/studio/src/components/editor/useColorGradingScopes.ts New hook to capture a frame and compute scopes analysis with debounce/refresh.
packages/studio/src/components/editor/useColorGradingScopes.test.tsx Unit tests for scopes capture behavior (open/refresh/unavailable).
packages/studio/src/components/editor/useColorGradingPreviews.ts Extends preview rendering to include effects/palette for preset thumbnails; adds frame capture API.
packages/studio/src/components/editor/useColorGradingController.ts Wires capture API into controller; adjusts persistence gating to “authored values” semantics.
packages/studio/src/components/editor/useColorGradingController.test.ts Updates/expands tests for preview batching/partial results and secondary persistence behavior.
packages/studio/src/components/editor/propertyPanelGradingNumberField.tsx New transactional numeric text field for grading controls.
packages/studio/src/components/editor/propertyPanelGradingNumberField.test.tsx Tests for the new numeric field (begin/preview/settle/cancel semantics).
packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx Major Flat grading UI expansion: scopes, primary sliders, wheels, curves, secondary, LUT actions refactor.
packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx Updates and adds coverage for new UI ordering and sampling behavior.
packages/studio/src/components/editor/propertyPanelFlatColorGradingAccessory.tsx Extracts the Flat grading accessory (compare/status/reset) into its own component.
packages/studio/src/components/editor/PropertyPanelFlat.tsx Passes captureGradedFrame through to the Flat grading section.
packages/studio/src/components/editor/propertyPanelColorWheels.tsx New 3-way color wheels component with pointer/keyboard interactions and transactional commit.
packages/studio/src/components/editor/propertyPanelColorWheels.test.tsx Tests wheels hue mapping, rendering direction, and preview vs commit behavior.
packages/studio/src/components/editor/propertyPanelColorSecondary.tsx New secondary (HSL) qualifier/correction UI with captured-frame sampling + matte visualization.
packages/studio/src/components/editor/propertyPanelColorSecondary.test.tsx Tests adding/removing secondaries, bypass, range ordering, capture error handling, keyboard sampling.
packages/studio/src/components/editor/propertyPanelColorScopes.tsx New scopes UI (mode tabs + canvas rendering + refresh).
packages/studio/src/components/editor/propertyPanelColorGradingControls.tsx Refactors existing controls to share slider specs and grading actions/helpers.
packages/studio/src/components/editor/propertyPanelColorCurves.tsx New curves UI wrapper (tabs + graph + numeric editing + reset/delete).
packages/studio/src/components/editor/propertyPanelColorCurves.test.tsx Tests curve tabs, seam behavior, and commit semantics.
packages/studio/src/components/editor/propertyPanelColorCurveGraph.tsx New curve graph implementation (pointer + full keyboard model) for RGB and hue curves.
packages/studio/src/components/editor/colorGradingFrameAnalysis.ts New frame analysis utilities (scope analysis, pixel decode, matte build, secondary sampling).
packages/studio/src/components/editor/colorGradingFrameAnalysis.test.ts Unit tests for scope endpoints, alpha weighting, circular hue averaging, and matte behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/studio/src/components/editor/propertyPanelColorScopes.tsx
Comment thread packages/studio/src/components/editor/useColorGradingScopes.ts
@ukimsanov
ukimsanov force-pushed the feat/agent-native-color-grading branch 2 times, most recently from adc63c8 to 6595ef3 Compare July 26, 2026 06:39
@ukimsanov
ukimsanov force-pushed the feat/studio-professional-color-grading branch 2 times, most recently from e82f474 to 96b67cf Compare July 26, 2026 06:55

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at 96b67cfc as the studio leg of the whole trio (#2796 core → #2798 CLI → #2797 studio), not in isolation — the findings below are mostly only visible that way. I verified the runtime-facing claims by running them rather than reading them, which is what turned one of them from a suspicion into a reproduction.

Additive only. @Copilot's two stale-scope comments are fixed in e82f4745 and covered by tests (useColorGradingScopes.ts:29,35 clears prior analysis on close and at capture start; useColorGradingScopes.test.tsx 4/4 pass) — not restating them. Magi is reviewing the same stack in parallel.

Audited (read end-to-end): colorGradingFrameAnalysis.ts, propertyPanelColorSecondary.tsx, propertyPanelColorWheels.tsx, propertyPanelColorCurveGraph.tsx, useColorGradingPreviews.ts, useColorGradingScopes.ts, useInspectorGestureTransaction.ts, propertyPanelFlatColorGradingSection.tsx, plus core/src/runtime/colorGrading.ts (GLSL + texture packing) and core/src/colorGrading.ts capabilities on the other side of the boundary.
Trusting: the scope-drawing canvas code in propertyPanelColorScopes.tsx (read structure, did not verify pixel output), SystemIcons.tsx.

First: this PR's CI never ran its own tests, and that includes the gate the body cites

CI is gated pull_request: branches: [main] (ci.yml:22-23). This PR's base is feat/agent-native-color-grading, so CI never fires — only regression, preview-regression and Player perf ran at this head, and the PR reads CLEAN and mergeable off those. Same gap as #2798.

Worth naming specifically: the body says "Keep all production components below Studio's 600-line architecture gate." That gate is ci.yml:673-704, inside the skipped workflow. The claim is true — I checked every changed non-test file, largest is PropertyPanelFlat.tsx at 575 — but nothing enforced it here.

So I ran the skipped set locally at this head:

Gate CI skipped Result
Studio grading/gesture focused tests 76 pass (body said 75)
Full studio suite 2811 pass, 18 todo, 0 fail (body said 2,809 + one order-leak failure — no failure reproduced here)
tsc --noEmit (studio) clean
oxlint . 0 warnings, 0 errors
oxfmt --check (changed dirs) clean
check:package-subpaths / workspace-contracts / package-cycles ✓ (no new cycle)
gen-skills-manifest --check / check-skill-mirror
file-size gate (600) replicated on this PR's studio files ✓ none over

All green. Reporting that as verification, not as a substitute for CI running it.

blocker — the matte and eyedropper use HALF the saturation/luma softness the render uses

colorGradingFrameAnalysis.ts:95,101:

softRangeMask(hsv.saturation, key.saturation.min, key.saturation.max, key.saturation.softness * 0.5)
softRangeMask(rec709Luma(...),  key.luma.min,       key.luma.max,       key.luma.softness * 0.5)

That * 0.5 was copied from the shader (runtime/colorGrading.ts GLSL :1075-1076) — but the shader's * 0.5 exists to undo the pack-time / 0.5 at runtime/colorGrading.ts:2725,2731:

secondary.key.saturation.softness / 0.5,   // packed into an RGBA8 texel (:2792-2795, unitFloatToByte)

The two cancel, so the runtime's effective softness is softness, not softness * 0.5. The JS port applies the read-side scale without the pack-side one and lands at half.

Measured, not argued. Pixel rgb(255,204,204) (HSV sat = 0.2), key saturation {min 0.5, max 1, softness 0.4}, hue/luma wide open:

saturation axis   studio matte 0.000000   runtime 0.150772    matte byte 0  vs render byte 38
luma axis         studio matte 0.000000   runtime 0.196538
hue axis          studio        0.798376   runtime 0.796850   <- control: agrees

So a pixel the render feathers in at ~15% reads as completely unselected in the matte. The hue axis agreeing is the control — it rules out my simulation being the thing that's wrong, and it localises the bug to the two axes that go through the /0.5 pack.

Fix is two lines — drop both * 0.5. Verified:

after fix   saturation  studio 0.156250  runtime 0.150772   (byte 40 vs 38 = 8-bit texel quantization, expected)
            luma        studio 0.196538  runtime 0.196538   (exact)

Grading this a blocker rather than a nit because the matte is the verification surface this PR exists to add — the body's stated purpose is "manual review, before/after comparison, and agent-output verification." A selection preview that disagrees with the render on the softness axes undercuts the feature's whole reason for being, and the softness sliders let you author right into the broken range (propertyPanelColorSecondary.tsx:356-370 allows softness to 0.5; the matte can never show more than 0.25 of feather).

important — the test named after this convention can't detect it, in either direction

colorGradingFrameAnalysis.test.ts:68 is called "matches the shader's half-softness edge convention" and pins nearly the same key I used above (saturation {0.5, 1, 0.4}). It encodes the misreading: there is no half-softness convention in the shader, there's a /0.5 pack and a *0.5 unpack that cancel.

I applied the correct two-line fix and re-ran:

colorGradingFrameAnalysis.test.ts        6/6 pass   (unchanged)
src/components/editor/ (whole dir)     823 pass    (unchanged)

So the suite is green with the bug and green with the fix — 823 tests that cannot distinguish the correct softness from double it. belowOuterEdge uses a pure grey (saturation exactly 0), which sits below min − softness at either value, so the assertion is satisfied by construction.

This is the shape #2798 — the PR directly beneath this one — just fixed for the vendored analyzer with mediaGradeAnalyzer.vendoredParity.test.ts. Here the duplication is unavoidable (GLSL can't be imported into a React panel), which makes the parity guard more necessary, not less. Concrete remedy, mirroring #2798's own fix: move the scalar mask into packages/core next to the GLSL it mirrors, and pin it with a test that runs sample triples through the JS mask and asserts against the pack → unitFloatToByte → shader-decode round-trip. A unit test that asserts a belief about another system is not a guard on that system.

important — studio is a fourth bounds layer that doesn't derive from the SSOT, and the prior art is in this same stack

#2796 added COLOR_GRADING_ADVANCED_LIMITS (parsers/src/colorGradingContract.ts:5-14) and wired all three existing layers to it — validation, core capabilities, core clamps. Zero studio files reference it, and the new panels hardcode every bound instead.

Two things make this more than a style preference:

  • The CLI leg of this stack already derives from core: media-treatment.ts:34,85,567 all call getHfColorGradingCapabilities().
  • Studio's own sibling Effects panel already derives from core: propertyPanelFlatEffectSpecs.ts:49, forty lines from the new files.

And getHfColorGradingCapabilities() already exposes every bound the new panels hardcode (colorGrading.ts:980-1043) — wheels.controls.{hue,amount,level}, secondaries.hue.{center,range,softness} plus rangePlusSoftnessMax, secondaries.{saturation,luma}.softness, secondaries.correction.*. So this is a swap, not a new abstraction.

I checked each hardcoded value against the table and they all agree today180 (propertyPanelColorSecondary.tsx:279,290,300), max={50} → 0.5 (:361), ±180 / ±100 (:379-383). So it's drift risk, not a live bug, with one exception below.

important — hue-center has three different behaviours for one contract bound

Same parameter, same feature, three answers:

Site Behaviour
propertyPanelColorSecondary.tsx:265 slider max={359}, no wrap
propertyPanelColorWheels.tsx:222 number field max={359.99}, and wrapHue on drag/keys (:20-22,41,63-65)
colorGrading.ts:1016-1017 (contract) min: 0, maxExclusive: 360, wrap: true

An agent writing through the CLI can author hue.center: 359.7 legally; open it in studio's secondary panel and it silently clamps to 359, and there's no wrap where the contract says there is. Deriving from capabilities.secondaries.hue.center fixes this and the item above together.

nit — refreshKey re-serializes the whole grading on every render

propertyPanelFlatColorGradingSection.tsx:209refreshKey={serializeHfColorGrading(grading)}, unmemoized, two lines below resolvedGrading and secondaryInputGrading which are both useMemo'd. It's only a nit because the drag path previews through the runtime rather than through grading, so this doesn't run per pointermove — but it's a free useMemo and it's inconsistent with its immediate neighbours.

What's genuinely strong here

Three things I checked specifically because they're the easy ones to get wrong, and they're right:

  1. The processing-order handling is correct, and I verified it against the GLSL call order rather than the comment. secondaryInputGrading (propertyPanelFlatColorGradingSection.tsx:143-160) reconstructs exactly {intensity: 1, adjust, wheels, curves, hueCurves, colorSpace} and feeds that to the capture at :367. That is precisely the runtime's pre-secondary prefix — applyPrimaryGrade → applyTonalWheels → applyRgbCurves → applyHueCurves (GLSL :1091-1094, :1153-1155) — and it correctly excludes the LUT, which the runtime applies after secondaries (:1156), plus effects/palette. Sampling the fully graded frame here would have made the eyedropper self-referential and the matte drift as you tuned the correction. It doesn't. That's the hard part of this feature and it's exactly right.

  2. Real reuse where reuse is possible. propertyPanelColorCurveGraph.tsx:3-12,93,96 imports compileHfColorCurve / compileHfHueCurve / HF_COLOR_CURVE_MAX_POINTS from core, so the curve you see on screen is drawn with the same monotone interpolation the runtime samples — no second interpolator. Secondary count comes from COLOR_GRADING_MAX_SECONDARIES. The two findings above are about finishing that instinct, not introducing it. (Incidentally: my #2796 note about the unused compileHfColorCurve barrel export removal doesn't break this — they're still exported from colorGrading.ts:20-24, so the ./color-grading subpath studio imports from still carries them. Checked, since it was my own finding.)

  3. useInspectorGestureDraft is cleanly additive. I audited every consumer of the module — propertyPanelCommitField.tsx:37, propertyPanelColor.tsx:205, plus the new wheels/curves — and the existing useInspectorGestureTransaction signature is untouched, so there's no contract change for the two pre-existing callers.

Verdict

Verdict: REQUEST CHANGES
Reasoning: The softness divergence is a reproduced two-line correctness defect in the exact surface this PR exists to provide, and the test named after that convention is green in both directions, so nothing downstream will catch it. Everything else — the pipeline-order handling especially — is solid work, and the SSOT/hue-center items are cheap to fold into the same pass. Not a stamp either way: that's James's call, not mine.

— hyperframes

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed exact head 96b67cfc4715953098ded2b67a98852ae71654b5 additively after reading the existing feedback. The two Copilot scope-lifecycle threads are resolved at this head. Rames's exact-head CHANGES_REQUESTED review already covers the matte-softness mismatch, missing parity guard, capability-bound/hue-center drift, and the skipped full Studio CI caused by this PR targeting a stacked base rather than main; I am not repeating those points.

  • [Blocker] Copying a grade to file/project scope uses pixel-active semantics at packages/studio/src/components/editor/useColorGradingController.ts:523-528, while ordinary persistence correctly uses authored-value semantics at :480-482. Core explicitly preserves a keyed disabled secondary as authored but inactive (packages/core/src/colorGrading.ts:1490-1522), and the existing regression only covers ordinary persistence (useColorGradingController.test.ts:248-268). As written, Apply to Scope serializes that grade as null and drops the authored qualifier. Use hasHfColorGradingAuthoredValues here and add a disabled-secondary scope-copy regression.

  • [Blocker] Reset omits the current LUT while preserving Effects and Palette (useColorGradingController.ts:551-558), and the new test pins lut: null (useColorGradingController.test.ts:569-589). That contradicts the PR body's explicit promise to preserve independent user LUT state when resetting grading. Preserve the current LUT and update the regression accordingly.

  • [Important] Curve deletion can be overwritten by settlement of an overlapping keyboard gesture. Arrow movement begins/previews a transaction (propertyPanelColorCurveGraph.tsx:344-352), Delete commits directly without cancelling it (propertyPanelColorCurves.tsx:56-67), and the matching ArrowUp later settles the stale transaction (propertyPanelColorCurveGraph.tsx:475-478; useInspectorGestureTransaction.ts:35-45). Cancel the active transaction before committing deletion and cover ArrowDown → Delete → ArrowUp.

The exact-head checks that did run are terminal green. As Rames noted, the stacked base means the repository's full Studio CI workflow did not run; his local full-suite/typecheck/lint/format/contract verification supplies the missing evidence for this review.

Verdict: REQUEST CHANGES
Reasoning: Existing feedback already blocks on render/matte parity. Additively, Apply to Scope drops authored data, reset violates the promised LUT-preservation contract, and overlapping curve gestures can restore a deleted point.

— Magi

@ukimsanov
ukimsanov force-pushed the feat/agent-native-color-grading branch from be791e3 to 20f4bde Compare July 26, 2026 08:50
@ukimsanov
ukimsanov force-pushed the feat/studio-professional-color-grading branch from 96b67cf to 794930a Compare July 26, 2026 08:50
@ukimsanov

Copy link
Copy Markdown
Collaborator Author

Addressed the complete new review set in 794930a07: Studio matte analysis now delegates to Core’s canonical secondary-mask contract; bounds and fractional hue wrapping derive from capabilities; Apply to Scope preserves authored disabled secondaries; reset preserves LUT/effects/palette; curve deletion cancels an active gesture before commit; and the scopes refresh key is memoized. Regressions cover each reported case. Focused Studio tests pass 75/75, Studio typecheck/lint/format/Fallow pass, and the unchanged order-dependent playback test passes 11/11 in isolation.

miguel-heygen
miguel-heygen previously approved these changes Jul 26, 2026

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed exact head 794930a07568deddd55ba0d891b68bec739641fa against every prior finding and the full fix delta.

All prior blockers are resolved:

  • The matte/eyedropper no longer carries a second softness implementation. colorGradingFrameAnalysis.ts delegates to the shared core calculateHfColorGradingSecondaryMask, whose saturation/luma softness uses the full authored value. Core now pins the runtime texture-pack/decode convention, and Studio pins both feather edges.
  • Studio wheels and secondaries derive ranges, identities, wrapping, and correction limits from getHfColorGradingCapabilities() rather than maintaining a fourth bounds table. The legal 359.7° hue-center case is regression-covered.
  • Apply to Scope now uses hasHfColorGradingAuthoredValues, with a disabled-secondary scope-copy regression.
  • Reset preserves LUT, Effects, and Palette, with the expected contract pinned.
  • Curve deletion cancels the active transaction before committing, and ArrowDown → Delete → ArrowUp is regression-covered.
  • The scopes refresh serialization is memoized.

Fresh delta audit found no replacement blocker or unnecessary abstraction. Moving the qualifier mask into core reduces duplication and gives Studio a single product-level implementation while leaving the unavoidable GLSL mirror explicitly guarded.

Verification at the exact head:

  • Core color-grading tests: 36/36
  • Focused Studio grading/controller tests: 35/35
  • Core typecheck: pass
  • Studio typecheck: pass
  • Oxlint on the touched Core/Studio files: 0 warnings, 0 errors
  • Exact-head GitHub checks: terminal green

Verdict: APPROVE
Reasoning: Every prior correctness and SSOT finding is fixed with targeted regression coverage, the replacement design is simpler, and fresh exact-head verification found no new blocker.

— Magi

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed at 794930a0. All four of my findings are fixed, and the blocker took the stronger option — the math moved into core rather than being patched in place. My blocking concerns are resolved on the merits.

Not re-verifying @miguel-heygen's three (Apply-to-Scope authored semantics, reset/LUT preservation, curve-delete gesture race) — it's re-reviewing those itself and has approved at this head. Mine below.

The blocker is fixed, and I verified it independently rather than by reading the diff

calculateHfColorGradingSecondaryMask now lives in packages/core/src/colorGrading.ts:1132-1152, next to the GLSL it mirrors and with a comment saying so, and colorGradingFrameAnalysis.ts:59-71 is a thin adapter that does RGB→HSV + Rec.709 luma and delegates. One owner for the math. The * 0.5 is gone repo-wide.

Two things I checked beyond that it "looks right":

The new mask agrees with the runtime. I re-ran my independent simulation of the pack → unitFloatToByte → GLSL-decode path against the real core function at this head:

saturation   core 0.156250   runtime-sim 0.150772   delta 0.005478   (= 8-bit texel quantization)
luma         core 0.156250   runtime-sim 0.150772   delta 0.005478
hue          core 0.500000   runtime-sim 0.500000   delta 0.000000

Also worth noting the hue branch is now a closer port than before — it mirrors the GLSL hueRangeMask structure directly (:1139-1146) instead of routing through the generic softRangeMask, and it drops the redundant min(softness, 180 − range) clamp that the normalizer already guarantees.

Both new guards actually catch a reintroduction — mutation-verified, not assumed. I put the * 0.5 back into calculateHfColorGradingSecondaryMask and re-ran:

core/src/colorGrading.test.ts    FAIL  "saturation: expected 0 to be greater than 0"        (baseline 36/36)
studio colorGradingFrameAnalysis.test.ts  FAIL  "expected +0 to be close to 0.15625"        (baseline 6/6)

I expected the core test's parity assertion to be insensitive, since toBeCloseTo(shaderMask, 2) compares the same function against itself with two softness values and so only measures byte-quantization tolerance. It's the toBeGreaterThan(0) liveness assertion on the same test that does the catching. So the guard works; the thing to know is which assertion is load-bearing — if a future fixture change makes productMask > 0 at half softness, that protection is gone. The studio-side absolute pin (toBeCloseTo(0.15625, 5), colorGradingFrameAnalysis.test.ts:80-81) is the more durable of the two, and it uses the exact pixel from the original repro. Renaming the old test away from "half-softness convention" also removes the misleading premise.

The other three

  • SSOT derivation — done properly. propertyPanelColorSecondary.tsx:19 and propertyPanelColorWheels.tsx:19 now hold getHfColorGradingCapabilities() slices, and every bound reads from them: hue center min, range, softness with rangePlusSoftnessMax (:315,327-328), secondaries.max (:106,150), per-axis capability lookup (:342), correction limits (:417), and the wheels' identity values for reset. Studio is no longer a fourth hardcoded layer.
  • hue-center — consistent now, and both panels wrap: wrapHueCenter (propertyPanelColorSecondary.tsx:24-27) and wrapHue (propertyPanelColorWheels.tsx:28-32), each computing its span from min/maxExclusive rather than a literal. 359.7 round-trips.
  • refreshKey nit — memoized as scopesRefreshKey (propertyPanelFlatColorGradingSection.tsx:160).

One small leftover, nit only: wrapHueCenter and wrapHue are now byte-identical functions in two files, and the epsilon is RANGE_GAP = 0.01 in one and a bare 0.01 in the other. A single exported helper next to the contract would close the last seam. Behaviour is correct either way.

Gates re-run at this head (CI still skips them)

Base is still feat/agent-native-color-grading, so CI still never fires — regression, preview-regression, Player perf all terminal green, and the PR reads CLEAN off those.

Gate Result
studio full suite 2814 pass, 18 todo, 0 fail (was 2811 — 3 new)
core full suite 1411 pass
parsers colorGradingContract.test.ts 6 pass
tsc --noEmit studio / core / parsers clean
oxlint . 0 warnings, 0 errors
oxfmt --check on this PR's 24 changed files clean
subpaths / workspace-contracts / package-cycles
skills-manifest / skill-mirror
600-line file-size gate replicated ✓ none over

Verdict

Verdict: COMMENT — my blockers are cleared
Reasoning: All four findings fixed, the blocker fixed at the right layer, and both new regression guards mutation-verified to catch a reintroduction. I'm not holding a block on the merits. Leaving this as COMMENT rather than an approval because clearing my earlier CHANGES_REQUESTED requires a stamp that isn't mine to give — @jrusso1020 can greenlight or dismiss it. The PR is mechanically CLEAN, so the stale state isn't gating the merge button.

— hyperframes

Base automatically changed from feat/agent-native-color-grading to main July 26, 2026 10:23
@ukimsanov
ukimsanov dismissed miguel-heygen’s stale review July 26, 2026 10:23

The base branch was changed.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-stamp after the base update at exact head 794930a07568deddd55ba0d891b68bec739641fa.

The PR head/tree is byte-identical to my previously approved exact head. The effective diff is now the 24-file Studio surface on top of merged #2798, with no new production or test content introduced by the base update.

Audited: current base/head topology and effective 24-file diff; every prior fix site and its regression coverage in the Core mask, Studio capabilities/wrapping, scope lifecycle, controller scope-copy/reset paths, curve transaction cancellation, and scopes refresh memoization.

Trusting: unchanged feature implementation already reviewed at this identical tree; there is no new line-level delta to reassess.

All prior findings remain resolved:

  • Studio matte/eyedropper analysis still delegates to Core's shared secondary-mask implementation, with runtime pack/decode and feather-edge coverage.
  • Wheels and secondaries still derive bounds, identities, and wrapping from the shared capabilities contract.
  • Apply to Scope preserves authored disabled secondaries; reset preserves LUT, Effects, and Palette; curve deletion cancels an active keyboard transaction.
  • Scope state clears stale analysis, redraws empty state, and memoizes its refresh serialization.

Both review threads are resolved. Fresh exact-head verification shows 52 check runs terminal with 44 successes and eight intentional skips, including all eight regression shards, Windows render/tests, Studio load smoke, global-install smoke, build, typecheck, lint, format, and unit/runtime-contract suites.

Verdict: APPROVE
Reasoning: The base update introduced no content delta at the reviewed head, every previously verified correctness and SSOT fix remains present, and all applicable exact-head CI is terminal green.

— Magi

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at 794930a07568deddd55ba0d891b68bec739641fa to clear my stale CHANGES_REQUESTED, which was pinned to the superseded commit 96b67cfc4. Doing this on @jrusso1020's explicit go.

My blockers were resolved at this head and I detailed the verification in review 4781325396. Recapping only what the approval rests on:

  • The secondary qualifier moved into core as calculateHfColorGradingSecondaryMask, next to the GLSL it mirrors, and colorGradingFrameAnalysis.ts is now a thin RGB→HSV adapter. That function is on main as of #2796 (packages/core/src/colorGrading.ts:1132), with no softness * 0.5 reintroduced anywhere.
  • I mutation-verified both new guards actually catch a reintroduction of the halving, rather than grading them by reading them. Core fails expected 0 to be greater than 0; studio fails expected +0 to be close to 0.15625, using the exact pixel from the original repro.
  • Both panels derive their bounds from getHfColorGradingCapabilities(), hue center wraps off min/maxExclusive in both, and refreshKey is memoized.

Re-confirmed at this head before stamping:

  • Head unchanged, and the diff is still the same 24 files, all under packages/studio. The base change moved #2796's core files onto main rather than pulling anything new in.
  • The reparent onto main un-skipped the CI workflow, which never fired once while this PR was stacked. Tests, Typecheck, Lint, Format, the File size check that the PR body cites, and Windows render verification are now green in CI itself rather than only in my local replication. Zero non-success check-runs at this head.

One nit stands, unchanged and non-blocking: wrapHueCenter and wrapHue are byte-identical in two files, with the epsilon as RANGE_GAP in one and a bare 0.01 in the other. A single exported helper next to the contract would close the last seam.

Not merging. That's @jrusso1020's call.

— hyperframes

@ukimsanov
ukimsanov merged commit 5bfda4e into main Jul 26, 2026
52 checks passed
@ukimsanov
ukimsanov deleted the feat/studio-professional-color-grading branch July 26, 2026 17:45
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.

4 participants