Fix Crop→Blur viewport desync after a crop zoom change#310
Merged
Conversation
When the crop zoom changed and the user switched to Blur (tool/mask) mode, the crop output rendered small and pinned to the top-left until the tabs were toggled again. `makeToolCropDisplayViewport` derives the canvas placement from presentation-layer conversions, gated only on `isInteractiveZoomGestureActive == false`. The Crop→Blur switch applies the tool viewport as a one-shot, synchronously, in the same runloop turn that `updateToolScrollGeometry` just reconfigured the freshly-un-hidden tool scroll view (non-animated `setZoomScale`, etc.). Core Animation has not committed that geometry to the presentation layers yet, so the one-shot sampled the previous session's stale presentation transform — small + top-left placement — and with no display link to re-sample, the stale frame persisted until the next mode switch. Presentation-layer reads are only meaningful while the viewport display link is actively re-sampling an in-flight bounce. Gate them on `toolSurface.viewportRendering.isRunning` so the link-idle one-shot paths fall back to the model layers (which the synchronous reconfigure already made authoritative). All display-link-driven interaction/bounce paths are unchanged, so the bounce tracking from the previous fix still works. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
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
makeToolCropDisplayViewportderives the Metal canvas placement from presentation-layer conversions (currentLayerRect(... usesPresentationLayers:)), gated only ontoolSurface.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
updateToolScrollGeometryjust reconfigured the freshly-un-hidden tool scroll view (non-animatedsetZoomScale,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 touchesCropView.swift, andEditingCanvasContentBakemaps the bake back to the originalextent, 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:
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
makeCropDisplayViewporthas the same presentation-read pattern, but the crop path is not a reported repro and its animator-driven rotation computes the viewport inside aUIViewPropertyAnimatorblock (where presentation reads are intended), so it is intentionally left untouched.Test plan
xcodebuild -scheme BrightroomUI -destination 'platform=iOS Simulator,name=iPhone 17 Pro'builds.🤖 Generated with Claude Code