Conversation
## Summary - Add a new `BrightroomParametric` SwiftPM product/target for platform-neutral parametric editing features. - Move Feature documents, registry-backed definitions, graph compilation, image rendering, video rendering, and Metal-backed mask kernels into the new module. - Keep the `EditingStack` bridge inside `BrightroomEngine` so legacy `EditingStack.Edit.Filters` can convert into parametric feature pipelines without making the new module depend on Engine/UI. - Add SwiftUI image/video playgrounds plus a lightweight macOS `ParametricMacDemo` target for interactive checks. ## Notes `Package.swift` now declares macOS support so the new `BrightroomParametric` product can build for macOS. This PR does not make the UIKit-backed `BrightroomEngine` or `BrightroomUI` surfaces a full macOS port. ## Validation - `plutil -lint Dev/Brightroom.xcodeproj/project.pbxproj` - `xcodebuild -project Dev/Brightroom.xcodeproj -scheme ParametricMacDemo -destination 'platform=macOS' -derivedDataPath build/CodexDerivedData build` - `xcodebuild -project Dev/Brightroom.xcodeproj -scheme BrightroomEngineTests -destination 'platform=iOS Simulator,id=05718949-3329-4852-9F7A-FA1649441821' -destination-timeout 60 -derivedDataPath build/CodexDerivedData -only-testing:BrightroomEngineTests/ParametricFeatureTreeTests test` - `xcodebuild -project Dev/Brightroom.xcodeproj -scheme SwiftUIDemo -destination 'platform=iOS Simulator,id=05718949-3329-4852-9F7A-FA1649441821' -destination-timeout 60 -derivedDataPath build/CodexDerivedData build`
## Summary
- Remove the load-time path that eagerly decoded the editing source into
an `MTLTexture` via `MTKTextureLoader`; collapse `CGImage._makeCIImage`
down to `CIImage(cgImage:).oriented()`.
- Delete `EditingStack.Options.usesMTLTextureForEditingImage` and its
plumbing (`makeMTLTexture` / `supportsImage` / `MTLImageCreationError`,
the now-unused `mtlDevice`, and the `MetalKit` import).
- Add `EditingImageWarmUp`: render the source once on a background queue
before publishing `loadedState`, warming Core Image's pipeline.
## Why
In the v5 canvas, `EditingCanvasMTKView` re-renders the source `CIImage`
into its own viewport texture every frame (`viewportSourceImage`). The
source's backing — `MTLTexture` vs `CGImage` — is therefore irrelevant
to display, so the load-time texture conversion is unnecessary. It only
carried downsides:
- `MTKTextureLoader` quantizes to **8-bit**, defeating the P3/EDR pixel
format `MetalImageView` configures.
- **16-bit images were never supported** and silently fell back to the
`CGImage` path, so behavior was already split in two.
- `CIImage(mtlTexture:)` required a **y-flip** that has been a recurring
source of orientation bugs (e.g. flipped export masks).
This direction also matches the Core Image guidance in WWDC 2026 Session
305 ("Enhance RAW image processing with Core Image"): use a per-view
Metal-backed `CIContext` rendering into an `MTKView`, and do not
pre-convert the source into a texture yourself.
## Preload (warm-up)
Dropping the texture path moves the GPU upload — previously done
off-thread by `MTKTextureLoader` during load — plus Core Image's
one-time pipeline-state compilation onto the first main-thread
`draw(in:)`, which visibly stalls the first frame. `EditingImageWarmUp`
addresses this:
- Renders the source downscaled to 256px through a dedicated
process-wide `CIContext(mtlDevice:)` (downscaling forces the whole
source to be sampled → full upload + pipeline compilation), with a tiny
read-back.
- Runs on a background queue in `handleImageLoaded` **before**
`loadedState` is assigned. Since `isLoading == (loadedState == nil)`,
the loading spinner stays up until warm-up finishes, so the canvas only
draws once the pipeline is hot.
- Never blocks the main thread. No new public API.
## Test plan
- [x] `xcodebuild -scheme BrightroomUI` build succeeds
- [x] `BrightroomEngineTests` pass with 0 failures (incl.
orientation/render suites: `RendererOrientationTests`,
`LocalAdjustmentMaskOrientationTests`, `RendererDeviceEquivalenceTests`)
- [ ] On-device verification of the first-load hitch (not yet done)
## Notes
- `Options.usesMTLTextureForEditingImage` was public, so its removal is
technically a breaking change. v5 already carries larger breaking
changes (e.g. PixelEditor removal), so this is in scope.
- The warm-up gates presentation: the spinner is slightly longer in
exchange for a hitch-free first frame.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
- EditingStack.Edit stores only [EditingFeature]; undo/redo over document snapshots - PixelEditor removed; PhotosCrop is the single built-in UI - BrightroomParametric: JSON/registry model deleted; Swift-native value tree with protocol-dispatched evaluation; ParametricDocumentCodec with typed versioned migration, encode-side boundary enforcement, and formatVersion - Core Image kernels moved to ParametricKernels.metal (precompiled metallib first, runtime-source fallback for pure SwiftPM) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the legacy Filtering/AnyFilter/Filter* layer with the BrightroomParametric vocabulary across the engine and UI. - EditingStack.Edit features carry parametric payloads: .effects(EffectPipeline) / .localAdjustment(LocalAdjustmentFeature) / .crop(EditingCrop). FeatureID is the feature identity; the UUID glue is gone. - Renderer evaluates .effects/.localAdjustment operations, and preview and export share one composition path (pinned by EditingPreviewExportParityTests). Local-adjustment effect failures now surface on export (throwing) while the preview degrades to identity. Disabled brush leaves select nothing in both paths, matching the parametric compiler. - Canvas/CropView/PhotosCrop speak EffectPipeline / BrushMaskStroke / FeatureID. PhotosCrop owns effect ordering (PhotosCropEffectOrder); the blur seed is the scale-invariant GaussianBlurFeature(value: 40); presets are PresetFeature; ColorCubeLoader returns ColorCubeFeature. - Delete the legacy filter layer: 15 Filter* files, Filtering/AnyFilter, EditingStackParametricBridge, ColorCubeStorage, and the previewFilterPresets / previewScale machinery. - Tests: port the suite to parametric types; add preview/export parity, brush-falloff parity, visual-evidence, and per-path performance benchmarks (111 passing). - Docs: refresh AGENTS.md guidance and add docs/performance-notes.md with profiling results and improvement opportunities. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SwiftUIImagePreviewView (and the internal _ImagePreviewView / _PreviewImageView) was only referenced by demo code. Remove it together with the demos that used it: ImagePreviewDemoView in RenderingDemoView, the DemoFilterView custom-effects screen, and their ContentView entry points. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y disk export
Engine/UI checkpoint for the v5 parametric editing work. Builds (engine, UI, SwiftUI demo) and 136 BrightroomEngineTests pass.
Document & crop model:
- Delete EditingCrop and EditingFeature; EditingStack.Edit now holds an
EditingDocument directly and crop is a parametric CropFeature (y-up rect,
QuarterTurn rotation, straighten). Add CropGeometry helper and rebase the
RenderCrop math on CropFeature. BrightroomUI edits crop through a y-down
CropEditingState/CropRotation working model with a single shared y-flip/snap.
Rendering:
- Single rendering path: every export compiles the document to one CIImage
recipe via ParametricImageRenderer. Remove the CoreGraphics fast path
(renderOnlyCropping / axisAlignedCropOnlyFeature / pixelCropRect) and the
Rendered.Engine distinction.
- BrightRoomImageRenderer.render is now a single async throws -> Rendered.
Output (in-memory bitmap vs file) is chosen via Options.output; Rendered hides
the disk/memory backing (cgImage/uiImage get throws, fileURL, thumbnail).
Large-image memory:
- Fix the editing-canvas local-adjustment blur OOM on huge images (e.g. 12000^2
'Nasa'): evaluate effects + the local effect at the downsampled ~2560 editing
source then upscale, instead of blurring the full-canvas image. The blur
radius is a fraction of image extent, so the upscaled preview matches the
full-resolution export.
- Bounded-memory disk export (Output.file): CIImageStreamingFileWriter renders
the recipe strip-by-strip into an mmap'd temp file and encodes lazily through
a direct CGDataProvider (no full-resolution CGImage, no CGImageDestination
over the whole bitmap). Replaces write{JPEG,HEIF}Representation, which
materialized the full bitmap in RAM. (On-device peak-memory verification still
pending.)
Brush mask:
- Share one brush falloff (BrushStampSharedSource) across the export/preview
CIKernel and the live Metal shader; the live canvas renders committed + active
strokes in one real-time pass.
Tests/docs:
- Add CropGeometry, BrushStampFalloff, BrushMaskRasterizerParity, LargeMaskedExport,
MaskedPreviewExportScaleConsistency, and DiskExport suites; migrate renderer
tests to the async API. Track remaining follow-ups in docs/editing-engine-todo.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Set the package to swift-tools-version 6.3 with swiftLanguageModes [.v6] and resolve the data-race-safety diagnostics across all three library modules (0 errors; demo and tests build green). Key changes: - Lock-guarded global CIContext caches -> nonisolated(unsafe). - Add Sendable to renderer value types (Options/Rendered/Output/Resolution/RenderingDevice) and CropViewFeatureFocus; ImageSource -> @unchecked Sendable. - PresetStorage, UIKit layout/frame-rate helpers, and the display-link holder -> @mainactor; view teardown uses isolated deinit. - BrightRoomImageRenderer.render runs off-actor (@Concurrent) over a Sendable snapshot, with a shared synchronous core kept for benchmarks; makeRenderer returns a sending value. - Remove the now-unused CIImageDisplaying protocol; add SwiftUIMetalImageView wrappers. Remaining warnings: EditingStack/ImageProvider background-queue self-captures (isolation-model follow-up) and pre-existing iOS deprecations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Fixes the editing canvas clamping **Display-P3 to sRGB** (export was already correct — this was a **preview-only WYSIWYG defect**, not data loss), and makes the canvas wide-gamut end to end. **Root cause:** the canvas ran a closed sRGB/8-bit loop. One color constant tagged every Metal↔Core Image hop as sRGB, and all color textures + the drawable were `.bgra8Unorm`, so the very first source bake hard-clipped out-of-sRGB-gamut P3 chroma; every downstream stage re-clipped. ## Changes **Four-boundary color contract** (`EditingCanvasImageProcessing`), replacing the single `colorSpace`: - `workingColorSpace` == `intermediateColorSpace` = `extendedLinearDisplayP3` — the **same** space writes AND reads back every intermediate texture, so each hop round-trips losslessly through a float texture. - `drawableColorSpace` = `displayP3`, kept identical to `CAMetalLayer.colorspace` → a single linear→display conversion, no double color management. - `maskColorSpace` = `sRGB` (unchanged) — the mask is a [0,1] selection field, so the brush feather fed to `CIBlendWithAlphaMask` is behavior-preserving. **Canvas** (`EditingCanvasMTKView`): - CIContext: `workingColorSpace` = extendedLinearDisplayP3, `workingFormat` = `.RGBAh`. - Color intermediates (source/base/adjusted) → `.rgba16Float`; drawable → `.bgr10a2Unorm` (10-bit P3 SDR). - Layer colorspace set in `init` and re-asserted in `didMoveToWindow` (MTKView rebuilds its drawable from `colorPixelFormat`). Both canvas surfaces share the init, so CropView's instance is fixed too. **Export** (`ParametricExportRenderer`): always process through an **extended-linear Display-P3 working space + half-float (`.RGBAh`) working buffer**, so out-of-sRGB / out-of-range chroma survives the multi-filter chain. The previous setup (CI's default sRGB-primary working space, 8-bit) clipped edited wide-gamut sources mid-chain. This is the processing precision only; the **output** stays `options.workingFormat` (8-bit, source-space-tagged, for SDR delivery). The export working space now matches the canvas, so canvas and export agree. Measured — an Adobe RGB green `(0,1,0)` (outside both sRGB and P3) edited (blur) then exported back to Adobe RGB: | working space / format | exported (R,G,B) | |---|---| | default sRGB-primary, 8-bit (old) | `(144, 255, 60)` — badly contaminated | | default sRGB-primary, RGBAh | `(4, 255, 60)` | | **extended-linear Display-P3, RGBAh (this PR)** | **`(0, 255, 0)` — faithful** | On the editing canvas, out-of-P3 source colors (Adobe RGB greens/cyans) are clamped to the P3 gamut boundary at the final drawable write — correct and unavoidable, since a P3 panel cannot display colors outside its gamut; the internal pipeline preserves them up to that point. **Test + demo:** `EditingCanvasColorContractTests` renders P3 red through the new vs old contract — both encoded to Display-P3 — and asserts the new path stays saturated while the old desaturates (runs on the simulator: it tests color **math**, not the panel, which can't show wide gamut). Added an "Insta Logo" PhotosCrop demo entry using the existing Display-P3 `Asset.instaLogo` sample. ## Verification - BrightroomUI + SwiftUIDemo build clean; **137 engine tests pass** (135 prior + 2 new). - **On real P3 hardware:** P3 now renders correctly in the editing canvas and matches export (confirmed via the PhotosCrop "Insta Logo" sample). ## Not included (follow-up) - **EDR/HDR:** float drawable + `wantsExtendedDynamicRangeContent` + per-frame `currentEDRHeadroom` tone-mapping, plus HDR gain-map ingest. The float intermediates + extended-linear working space introduced here make that mostly a layer-config + one view-node addition. ## Note - The `.bgr10a2Unorm` drawable has only 2-bit alpha; verified on-device that transparent-edge quality is fine. Flipping the single `drawablePixelFormat` constant to `.rgba16Float` (full alpha, also the EDR format) is the escape hatch if it ever matters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Migrates the `BrightroomEngineTests` suite from **XCTest** to the **Swift Testing** framework (`import Testing`). 27 test files are converted; one performance file is intentionally left on XCTest (see below). `XCTestCase` classes become `struct` / `final class` suites, test methods gain `@Test` with sentence-case raw identifiers, and assertions move to `#expect` / `#require`. ## Required project change The `BrightroomEngineTests` target was typed as `com.apple.product-type.bundle.**ui-testing**` — a hostless logic-test bundle that had been mistyped. Swift Testing is **not available in UI-testing bundles** (`import Testing` fails with `Unable to resolve module dependency: '_Testing_Unavailable'`), so the product type is changed to `com.apple.product-type.bundle.**unit-test**`. No host app / `TEST_HOST` is involved, so this is a no-op for how the logic tests run. ## Notable conversions - **`expectation` / `wait`** (`LoadingTests`) → `withCheckedContinuation` (kept `@MainActor` for the StateGraph observation). - **`XCTSkip`** for a missing Metal device → `@Test(.enabled(if: MTLCreateSystemDefaultDevice() != nil))` + `#require`. - **`XCTAttachment(image:)`** → `Attachment.record(uiImage.pngData()!, named:)`. - **`file:`/`line:` forwarding helper** → `sourceLocation: SourceLocation = SourceLocation(fileID:filePath:line:column:)` default arg (no underscored API). - **`continueAfterFailure = false`** → `try #require` throughout that suite. - **`tearDown` cleanup** → `deinit` (`DiskExportTests` kept as `final class`). - **`XCTAssertThrowsError`** → `#expect(throws:)`, capturing the value where the error is inspected. ## Kept on XCTest (deliberate) `EnginePerformanceWorkloadTests` uses `measure(metrics:options:)` with `XCTClockMetric` / `XCTCPUMetric` as CPU-instruction regression guards. Swift Testing has no performance-measurement equivalent, so this file stays on XCTest. A single test bundle runs both frameworks. ## Verification - `xcodebuild ... build-for-testing` — **succeeds, 0 errors**. - `xcodebuild ... test-without-building` — **all 134 tests in 31 suites pass** (mixed XCTest + Swift Testing), including the rebased wide-gamut `EditingCanvasColorContractTests`. Rebased on top of the latest `v5` (wide-gamut PR #298); the new `EditingCanvasColorContractTests` added there is migrated here as well. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pth (#300) ## What Fixes two visible defects in blur-mask painting (PhotosCrop), plus a related export-path safety change. ### 1. See-through — the unblurred image bled through the blur The brush default `opacity` was **0.9**, and the mask accumulates with **max** (live = Metal `.max` blend; export = `CIBlendKernel.componentMax`). So opacity acts as a *hard ceiling* — overlapping/repeated strokes can never exceed the single-stamp peak. `CIBlendWithAlphaMask` then composited `blur·0.9 + original·0.1`, leaving ~10% of the sharp original visible even at the stroke core. **Fix:** default `opacity` 0.9 → **1.0** (`hardness` 0.72 still feathers the edge). The color-space / premultiplied-alpha theories were investigated and **ruled out**: the mask, base, and adjusted images are all sRGB (`EditingCanvasImageProcessing.colorSpace`), and `CIBlendWithAlphaMask` blends using only the alpha channel, which is color-space independent. ### 2. Visible stamp seams along a stroke Default `spacing` 0.18 placed stamps `0.36·radius` apart — wider than the hardness‑0.72 falloff ring of `(1−0.72)·radius = 0.28·radius` — so the feathered edge scalloped between stamps. **Fix:** default `spacing` 0.18 → **0.05** (`~0.10·radius`, about ⅓ of the ring). Preview and export share the **same stored stamp list** (commit performs no decimation; export rasterizes stored stamps 1:1), so this densifies both — as requested. ### 3. Export safety — bound the mask Core Image graph depth `FeatureGraphCompiler.render(_:BrushMask)` folded each stamp into the mask with `componentMax` **linearly**, making the CI graph depth O(n stamps). Denser stamps would risk slow compiles / stack overflow on long strokes. **Fix:** reduce stamps in a **balanced tree** (`reduceComponentMax`, O(log n) depth). `componentMax` is associative and commutative and `componentMax(transparent, x) == x`, so the result is identical. ## Why Direct response to the report: blur mask compositing looked wrong (original faintly showing through) and stamp joints were visible. Both trace to brush defaults; the export reducer change keeps the denser default safe. ## Reviewer notes - Behavior change is limited to **new** strokes' defaults; existing committed strokes render from their stored stamps unchanged. - `EditingCanvasStrokeRecord.init(brushMaskStroke:)` spacing is a display-only placeholder (`BrushMaskStroke` doesn't persist spacing); updated to match the authoring default. - Base branch is **v5** (this branch sits on the v5 line, not `main`). ## Verification - `BrightroomUI` builds (iOS Simulator). - Tests pass: `BrushMaskRasterizerParityTests`, `CommittedMaskParametricParityTests`, `LargeMaskedExportTests`, `LocalAdjustmentRenderingTests` (the balanced-tree reduction preserves mask parity). - `opacity 1.0` / `spacing 0.05` are values best confirmed visually; `spacing` can be lowered further if seams persist (export cost is now O(log n)). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…esult screen (#301) ## Summary Adds a reusable **FeatureTree viewer** to `BrightroomParametric` (as a debug component) and uses it on the SwiftUI Demo's **PhotosCrop export result screen** to show the parametric `EditingDocument` that produced the render. The viewer is expressed with `List` + `DisclosureGroup`, as requested. ## BrightroomParametric New `Sources/BrightroomParametric/Debug/FeatureTreeView.swift`, guarded by `#if canImport(SwiftUI)` so the otherwise SwiftUI-free module keeps building. - `FeatureTreeView(document:)` — standalone `List`-based screen. - `FeatureTreeOutline(document:)` — embeddable rows for an existing `List`/`Form`/`Section` (used by the demo). - `FeatureTreeDescriptor` / `FeatureTreeNode` — the tree builder and display model (reusable for tests/other hosts). Design: - Structural containers (`EffectPipelineFeature`, `PresetFeature`, `LocalAdjustmentFeature`, `MaskTree`/`MaskNode`) expand into child nodes explicitly. - Scalar parameters are read via `Mirror` reflection, so **new leaf features show up automatically** without touching the viewer. `CGRect` / `ParametricRGBAColor` / `GaussianBlurRadius` / `QuarterTurn` etc. get dedicated formatting. - Each node shows an icon, title, enabled state, a one-line summary, and (when expanded) `name`(left)/monospaced `value`(right) parameter rows plus child nodes. Top level is expanded by default; nested structure stays collapsed. ## Demo - `RenderedResultView` is now a `List` with a "Result" section (image + metadata) and a "Feature Tree" section (`FeatureTreeOutline`). - `DemoPhotosCropView` snapshots `stack.loadedState?.currentEdit.document` on done and passes it to the result screen. ## Verification Built `SwiftUIDemo` (Xcode 27) and verified on iPhone 17 Pro Simulator: opened PhotosCrop, applied the *Vivid* preset + blur brush strokes, tapped Done, and confirmed the result screen shows the Feature Tree: - **Effects** (`1 effect`) → child **Preset (Vivid)** - **Local Adjustment** (`alpha`) → **Mask → Brush** (Strokes 3 / Stamps 61) - **Crop** (`Crop Rect (…) · Rotation 0°`) with parameter rows DisclosureGroup expand/collapse, icons, layout, and long-ID wrapping all render correctly. No visual bugs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Fixes PhotosCrop rendering as a black canvas on iOS Simulator while keeping the 10-bit Display-P3 drawable path on real devices. ## Root Cause The editing canvas drawable pixel format was changed to `.bgr10a2Unorm` for the wide-gamut Display-P3 canvas path. That works on device, but recent iOS Simulator runtimes can create the MTKView and still present the drawable as black. ## Changes - Use `.bgra8Unorm` for the editing canvas drawable on Simulator builds. - Keep `.bgr10a2Unorm` for device builds. - Add a regression test that locks the intended platform-specific drawable format while preserving the existing wide-gamut color math tests. ## Verification - Reproduced the black PhotosCrop canvas in SwiftUIDemo on iPhone 17 Pro, iOS 26.5 Simulator before the fix. - Verified the same PhotosCrop Horizontal screen renders the image after the fix. - Ran `BrightroomEngineTests/EditingCanvasColorContractTests`: 3 passed. - Built and launched `SwiftUIDemo` successfully on the same Simulator.
## Summary - Move parametric brush Core Image kernels from copied `.metal.txt` source to build-compiled `.metal` sources. - Move the live brush-mask render shader into BrightroomParametric so it is compiled into the same `default.metallib`. - Share brush falloff through `BrushStampFalloff.metalh`, loaded by both the Core Image kernel and live Metal render shader. - Load kernels via `CIColorKernel(functionName:fromMetalLibraryData:)` and live render functions via `MTLDevice.makeLibrary(URL:)`. ## Why The previous resource-copy/runtime-compile setup existed to avoid missing `brushStampAlpha` during Metal compilation. Now the shared falloff is a real Metal header included by both compiled sources, so the package can ship compiled Metal instead of text resources. ## Validation - `BrightroomUI` simulator build via XcodeBuildMCP: succeeded. - `BrightroomEngineTests` targeted tests via XcodeBuildMCP: 4 passed. - `BrushStampFalloffTests` - `BrushMaskRasterizerParityTests` - Confirmed build output contains `Brightroom_BrightroomParametric.bundle/default.metallib` and build log runs `CompileMetalFile` + `MetalLink` for the new `.metal` sources.
## Summary - Add a finite maximum crop zoom based on a minimum authored crop-output side length. - Clamp programmatic crop zoom to the scroll view's configured zoom range before calculating content offset. - Add focused tests for the crop zoom policy on large and small images. ## Root Cause Crop editing allowed an unbounded zoom scale, so users could author a final crop output that was only a few pixels wide or tall. Blur masking uses that final crop output as the tool canvas; once the crop output became extremely small, the tool surface entered huge zoom-scale space and viewport-sized brushes resolved into sub-pixel image-space strokes. ## Validation - `BrightroomEngineTests/CropEditingZoomScaleTests` via XcodeBuildMCP: 2 tests passed - `git diff --check`
## What Each filter in the PhotosCrop **Filters** strip now renders a live thumbnail of the photo with that filter applied (Photos/Instagram-style), replacing the text-only pills — including an "Original" swatch. ## How - **New `PhotosCropFilterThumbnail.swift`** — a `@concurrent` single-swatch renderer on a dedicated wide-gamut `CIContext` whose working/output color spaces match the editing canvas (`extendedLinearDisplayP3` → `displayP3`), so a swatch and the live canvas resolve the same colors. It center-square-crops the session thumbnail, never upscales past the source, and threads a source-relative `radiusReferenceExtent` so host-supplied blur/sharpen presets preview at the correct strength. - **Per-cell lazy rendering** — each `PhotosCropFilterChip` renders its own swatch in a `.task` into its `@State`. A `LazyHStack` defers off-screen first renders and retains created chips, so scrolling never re-renders. No shared cache or stringly-typed dictionary. - **Selected chip auto-centers** via `.scrollPosition(id:anchor:)` on selection change (clamps at the ends, so Original / the last filter rest at the edge). - **Selection ring** uses a dark shadow halo so it stays legible on bright swatches (Original / Mono / Noir on light photos). - Swatches preview the **filter only** — not the user's manual Adjust edits, blur mask, or current crop — matching the Photos/Instagram strip. ## Notes - Uses the iOS 17 `scrollPosition(id:anchor:)` modifier (not deprecated). The iOS 18 `ScrollPosition` struct would need `if #available` since the deployment floor is iOS 17. ## Verification - BrightroomUI builds clean in Swift 6 language mode. - Verified on the iOS Simulator: all swatches render distinctly (Mono/Noir desaturated, Warm/Cool shifted, Fade washed, Vivid/Dramatic as expected), off-screen chips render lazily when scrolled into view, the selection ring is visible on bright swatches, and selecting a chip scrolls it to center. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary `EditingCanvasMode.preview` was a dead compatibility alias of `.renderedEditPreview`. This removes it and collapses its `switch` arms into `.renderedEditPreview`. ## Why An audit of all four `EditingCanvasMode` cases found that `.preview`: - Was documented in-source as a *"Compatibility spelling for `renderedEditPreview`"* and was introduced in the **same** commit as `.renderedEditPreview` (it is not an older, migrated-away-from name). - Is **never constructed anywhere** — production, demo, or tests. `git grep` for any producer (`= .preview`, `return .preview`, `mode: .preview`, `EditingCanvasMode.preview`) returns nothing. - Appears **only** as a passive `switch` co-arm, always paired with `.renderedEditPreview` and handled identically, in every `switch` over the enum: - `EditingCanvasPublicTypes.swift` — `localEffect`, `activeLocalEffect`, `defaultInteractionMode` - `EditingCanvasRenderImageFactory.swift` — `makeRenderImages`, `makeCropOutputRenderImages` So the case carried no distinct behavior and could never be reached. ## Behavioral impact None. No first-party code path ever produced `.preview`, and it was rendered byte-for-byte identically to `.renderedEditPreview`. The other three cases (`.viewportBase`, `.localAdjustment`, `.renderedEditPreview`) are unchanged and remain in use. ## API note This removes a `public` enum case, so it is technically source-breaking for any external caller that explicitly referenced `EditingCanvasMode.preview`. There are no such references inside this repository. ## Verification - `git grep` confirms zero remaining `.preview` references in `Sources/` and `Dev/`. - `xcodebuild -scheme BrightroomUI -destination 'generic/platform=iOS Simulator'` → **BUILD SUCCEEDED** (Swift 6 mode). ## Follow-up (not in this PR) `.renderedEditPreview` itself is only selected by `CropView.CanvasRenderPlan` when 2+ local-adjustment layers are simultaneously active — a state the built-in PhotosCrop UI cannot currently produce. It (and `CanvasRenderPlan`, already marked `// TODO: Consider to remove this`) is a candidate for removal/replacement once the live canvas composites multiple local-adjustment layers; left out of this PR to keep the change a pure, behavior-preserving cleanup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Problem In Crop mode, holding a pinch still (fingers down, no movement) would snap the scroll view back to the recorded crop after ~0.8s, making it look like the crop was committed mid-gesture. ## Cause `cropSurface.onDidZoom` schedules `updateCropLayout()` on a trailing `debounce` (interval 0.8s). `updateCropLayout()` → `updateScrollContainerView()` → `customZoom(to: crop.zoomExtent())` resets the scroll view to `state.proposedCrop`, which is **not** updated mid-pinch (`record()` only runs on settle). While the user moves their fingers, continuous `scrollViewDidZoom` events keep resetting the 0.8s debounce. The moment the user **holds still with fingers down**, events stop firing and the debounce fires mid-gesture, snapping the view. The sibling `onDidScroll` debounce already guards against this for drags with an `isTracking == false` check, but `onDidZoom` had no equivalent guard — and both closures share the same `debounce` instance, so whether the snap happens depends on which event was scheduled last. ## Fix Guard the `onDidZoom` settle layout with `cropSurface.isInteractiveZoomGestureActive == false` (pinch recognizer not in `.began`/`.changed`), deferring the reflow until the pinch actually ends. The crop **recording** path (`record()` via the `scrollViewSettleDebounce` → `didSettleScrollViewAdjustment`) already checks `isContentOffsetResting` (`isZooming == false`, etc.), so commit semantics are unchanged — this only stops the premature layout snap. ## Verification - `BrightroomUI` builds (iPhone 17 Pro, iOS 26.5). - Confirmed in SwiftUIDemo PhotosCrop: pinching and holding still no longer auto-snaps the crop. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The crop surface shows its pixels on a Metal canvas that is repositioned each frame to follow the transparent zooming platter, rather than being zoomed by the scroll view directly, so the bounce-back animation depends on per-frame following of the scroll view's layers. Two issues broke it: - makeCropDisplayViewport read model-layer values whenever the zoom was "interaction active", lumping the active pinch together with the post-release bounce-back. During the bounce-back UIKit animates the presentation layer while the model has already jumped to the clamped value, so model reads snapped the canvas. Read presentation layers during the bounce-back (new isInteractiveZoomDriving) and keep model reads only while the pinch actively drives the zoom. - updateCurrentEditingStackDisplay's document-follow guard checked isZooming but not isZoomBouncing/isDecelerating, so a SwiftUI re-render during the bounce could re-load the document crop and customZoom(animated: false) back, cancelling the bounce. Treat bounce/decelerate as live interaction, matching isContentOffsetResting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This reverts commit e45c4fc.
…ent bake (#308) ## Summary Two performance fixes for slow CropView rotation, found and verified with on-device Instruments Time Profiler traces. Both are behind pixel-equivalence tests (the editing canvas renders black on the Simulator, so correctness is guarded via CIContext read-back, and the felt win was confirmed on device). ### 1. Make the editing source GPU-resident (adaptive bit depth) The editing source was a CPU-backed `CIImage(cgImage:)`, so Core Image re-uploaded its bitmap CPU→GPU on **every** canvas render. During rotation the viewport cache invalidates every frame, so the trace showed `CIMetalTextureSetBytes` / `replaceRegion:withBytes:` consuming ~40% of all main-thread time. `EditingSourcePreparation.makeGPUResidentSource` uploads the oriented source into a persistent private `MTLTexture` once at load (background queue) and returns a texture-backed `CIImage`. The texture depth **adapts to the source**: `rgba8Unorm` for an 8-bit source (the common case — lossless, half the memory), `rgba16Float` for >8-bit / HDR. This re-does what #297 removed, but avoids that PR's downsides (`MTKTextureLoader` 8-bit quantization / no 16-bit / y-flip) by rendering through a `CIContext` in the source's own color space — the `CIContext.render(to:)` / `CIImage(mtlTexture:)` round-trip is flip-free. `EditingImageWarmUp` is deleted (building the texture warms the pipeline). ### 2. Bake the local-effect layer once per generation With the upload gone, the trace's remaining per-frame cost was Core Image re-evaluating the blur-heavy `adjusted` graph every frame (`create_intermediate` / `GetSurfaceFromCache` — IOSurface allocation under a barrier lock). The effect content is invariant during a gesture; only the viewport moves. `EditingCanvasContentBake.bake` renders the `adjusted` layer into a capped (2560) texture once per render-images generation; the composite path resamples it per frame. The cache is invalidated only on `setRenderImages` / `setViewportCachedSourceEnabled`, never on viewport changes. `base` (global pointwise effects on the now-GPU-resident source) stays a live graph — cheap to re-evaluate, and not baking it halves the held texture memory. ## Measured (device, normalized ms per wall-second) | Main-thread cost | before | after | | --- | --- | --- | | Canvas draw (`MTKView.draw`) | 102 | 26 | | CI graph eval (`tile_node_graph`) | 70 | 3 | | IOSurface alloc + barrier lock | 58 | 0 | | Per-frame source upload | (eliminated in step 1) | 0 | `CA::Transaction::commit` dropped 158 → 90. The new dominant rotation cost is UIKit layout (`updateUIViewController` → `updateCropLayout`), tracked as a follow-up. ## Memory Adaptive source depth + baking only `adjusted` keep the footprint bounded. For an 8-bit source: plain crop/rotate holds just the ~26MB source texture (≈ the pre-change CGImage footprint); a blur-mask gesture adds one ~52MB `adjusted` bake. The `adjusted` bake stays `rgba16Float` deliberately — it is the extended-linear-Display-P3 intermediate, where 8-bit linear bands and can't carry exposure overshoot / wide gamut. ## Test plan - [x] `xcodebuild -scheme BrightroomUI` builds. - [x] `EditingSourceTextureTests` — texture-backed source is pixel-equivalent to `CIImage(cgImage:).oriented()` across 8 orientations × {8,16}-bit (16 cases). - [x] `EditingCanvasContentBakeTests` — bake is a faithful drop-in (offset extent, asymmetric content, within float tolerance; cap respected). - [x] Regression-green: ColorContract, CropSurfaceBlurRenderPath, CommittedMaskParametricParity, LocalAdjustmentRendering/MaskOrientation, EditingPreviewExportParity, MaskedPreviewExportScaleConsistency, RendererOrientation. - [ ] Device re-trace to confirm keeping `base` un-baked did not re-introduce per-frame cost (expected fine — pointwise on a GPU texture). ## Follow-ups (docs/editing-engine-todo.md item 7) - UIKit per-frame layout during the angle drag is now the dominant rotation cost. - Remaining bake-memory levers (resolution cap, confirmed-SDR formats) if needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Summary - Make the crop Metal canvas follow UIScrollView zoom bounce-back by sampling presentation-layer geometry after the pinch ends. - Apply the same presentation-layer viewport tracking to the Tool/Mask surface so the mask canvas follows min-zoom shrink/bounce without drifting toward the top-left. - Defer document-crop reload while scroll/zoom settling is still active, avoiding a non-animated `customZoom` snap during bounce-back. ## Root Cause The viewport display link was already running, but `makeCropDisplayViewport()` disabled presentation-layer reads for the full zoom interaction, including the post-release bounce. During bounce-back UIKit animates the presentation layer while model values are already clamped, so the canvas snapped instead of following the visible image. Tool/Mask mode also computed its viewport from model `contentOffset`/`contentInset`/`zoomScale`, which lost the presentation-time centering during min-zoom shrink. ## Validation - `git diff --check` - Xcode 27 `SwiftUIDemo` launched on `iPhone 17 Pro (27.0)` simulator from the cwd worktree; build completed with no errors. Launch session: `77a4d4780`.
## Summary Fixes a regression where, after changing the zoom in **Crop** mode and switching to **Blur** (tool/mask) mode, the crop output rendered **small and pinned to the top-left**. Toggling the Crop/Blur tabs again corrected it; zooming the other way produced the inverse misplacement. ## Root cause `makeToolCropDisplayViewport` derives the Metal canvas placement from **presentation-layer** conversions (`currentLayerRect(... usesPresentationLayers:)`), gated only on `toolSurface.isInteractiveZoomGestureActive == false`. This was introduced in #309 to make the mask canvas follow the post-release zoom bounce. The Crop→Blur switch applies the tool viewport **once, synchronously**, in the same runloop turn that `updateToolScrollGeometry` just reconfigured the freshly-un-hidden tool scroll view (non-animated `setZoomScale`, `centerContentInViewport`, etc.): `setFeatureFocus` → `applySurfaceMode(syncsToolViewportFromCrop: true)` → `updateToolScrollGeometry` (model set synchronously) → `updateToolCropDisplayViewport()` (one-shot, no display link). At that instant no pinch is active, so `usesPresentationLayers == true`, but Core Animation has not yet committed the new geometry to the presentation layers — they still hold the **previous** Blur session's geometry. The one-shot samples that stale transform → small + top-left placement, and because no display link runs afterward, the stale frame sticks until the next mode switch (the "switch back and forth fixes it" symptom). `#308` (GPU-resident source / per-generation content bake), originally suspected, is **not** involved: it never touches `CropView.swift`, and `EditingCanvasContentBake` maps the bake back to the original `extent`, so it changes layer *content*, never viewport *placement*. ## Fix Presentation-layer reads are only meaningful while the viewport display link is actively re-sampling an in-flight bounce. Gate them on whether that link is running: ```swift let usesPresentationLayers = toolSurface.viewportRendering.isRunning && toolSurface.isInteractiveZoomGestureActive == false ``` This flips **only** the link-idle one-shot paths (i.e. the mode switch) back to the model layers, which the synchronous reconfigure already made authoritative. Every display-link-driven path (interactive pinch, post-release bounce, deceleration) is unchanged, so #309's bounce tracking still works. ## Scope / notes - Scoped to the **tool** path only. `makeCropDisplayViewport` has the same presentation-read pattern, but the crop path is not a reported repro and its animator-driven rotation computes the viewport inside a `UIViewPropertyAnimator` block (where presentation reads are intended), so it is intentionally left untouched. - The tool path's animator case early-returns (the tool surface is hidden in Crop mode), so the gate is safe there. ## Test plan - [x] `xcodebuild -scheme BrightroomUI -destination 'platform=iOS Simulator,name=iPhone 17 Pro'` builds. - [x] Verified on device: Crop zoom change → switch to Blur now displays the crop output correctly (no small/top-left misplacement, no need to toggle tabs). - [ ] Editing canvas renders black on the Simulator, so visual confirmation is device-only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Summary - Add Blur to the PhotosCrop Adjust parameter list. - Write Blur edits into the global-effects node as `GaussianBlurFeature(value:)`. - Add focused tests for blur insertion, neutral removal, and PhotosCrop effect ordering. ## Why `GaussianBlurFeature` already existed in the parametric layer and PhotosCrop effect ordering, but PhotosCrop's Adjust UI did not expose a global Blur parameter. This makes Gaussian blur available as a first-class Feature adjustment in that UI. ## Validation - BrightroomUI simulator build succeeded via XcodeBuildMCP. - `BrightroomEngineTests/PhotosCropAdjustmentParameterTests` passed: 3 tests.
## Summary - Introduce `PhotosCropEditingModel` as the PhotosCrop policy layer over the generic FeatureTree document. - Add `CropViewDocument` so CropView edits through a document boundary instead of depending on `EditingStack` directly. - Move undo/redo checkpoint storage behind `EditingHistory` and expose clearer stack APIs: `commitCurrentEditIfNeeded`, `undo`, `redo`, `revertCurrentEdit`, and `removeAllHistory`. - Remove legacy crop-specific stack helpers and update demos, docs, and render tests to use FeatureTree-oriented mutation paths. ## Validation - `BrightroomEngineTests`: 152 passed on iPhone 17 simulator. - `BrightroomUI` simulator build: succeeded. - `SwiftUIDemo` simulator build: succeeded. - `git diff --check`: passed. Note: `SwiftUIDemo` still reports existing iOS 17 `onChange(of:perform:)` deprecation warnings unrelated to this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.