Fix quarter-turn crop truncation by separating output rotation - #330
Merged
Merged
Conversation
After rotating a full-image crop by 90° or 270° in PhotosCrop, the crop frame shows the whole rotated image, but Done committed and exported a centered H×H square. CropView keeps the frame's center in image coordinates and takes the output orientation, so a quarter-turned full-image frame is (W/2 - H/2, H/2 - W/2, H, W): it extends past the unrotated image bounds while CropFeature.apply, rotating about its center, fills it exactly. PixelCropRect(cropExtent:in:) clamped that output-oriented rect to the unrotated bounds, cutting it down to the square. RenderCrop now snaps and clamps the crop's footprint on the source (the rect turned back about its center) and turns the result back. Under a sideways turn the footprint also keeps an even width-height difference, so the turned rect stays on the pixel grid instead of half pixels. Unrotated and half-turn crops are unchanged. The UI paths that shared the unrotated-bounds assumption use the same footprint rule: seeding CropEditingState from a stored crop (reopening the editor), the drag normalization and aspect-ratio bounding in CropView.record(), and fitting a locked aspect ratio after a rotation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A stored 90° or 270° crop whose center lies outside the image can overlap the image while its footprint on the source, which shares that center, misses it entirely. Reopening such a crop made CropGeometry.fittingRect(rect:in:rotation:respectingAspectRatio:) intersect the footprint with the image, get a null rect, and trip its assertion in Debug builds (Release seeded the editor with a null rect). 5.1.0 clamped the rect itself, so it opened. When the footprint misses the image, the rect is now first clamped to the image as an unrotated rect is. That brings its center inside, so the footprint then overlaps the image and is clamped as usual. Unrotated and half-turn rects take the same path as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opening the editor on a 90° or 270° crop of an image whose width and height differ by an odd number of pixels, and tapping Done without touching it, lost about 2 px per cycle. On Done, CropView re-reads the frame from its views, a fraction of a pixel off. The inward snap then drops the pixel line the frame only nearly covers (the 1 px creep 5.1.0 already had), which leaves the footprint's width-height difference odd, and trimmedForQuarterTurn() dropped one more line to make it even again. A footprint needs an even difference, but dropping a line of fully covered pixels misses the requested footprint by a whole pixel. The snapper now picks the even rect closest to the request: when the request partly covers the pixel line just outside an edge, the rect takes the most-covered such line; only when there is none does the longer side lose a pixel, as before. For the sub-pixel error on Done this takes back the line the inward snap dropped, so the stored crop stays the same. Integral footprints, such as a full image turned a quarter, snap as before, and unrotated and half-turn crops do not take this path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With a quarter turn and a straighten angle, the snapper clamps the crop's 90° footprint, not the area the crop actually samples (the frame turned back by the whole angle). A thin frame can sample only image pixels while its 90° footprint is longer than the image, so it is stored a few pixels shorter than the frame shows. 5.1.0 clamped the frame itself, which kept such a frame but cut the whole-image quarter turn down to a square. The export stays inside the frame and inside the image. Checking the straightened area needs a footprint that may extend past the image in the snapper, the reopen clamp and CropView's aspect-ratio bounding, so it is left for a separate change. The test pins the case with withKnownIssue, so it reports when the behavior changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4756382 lets the snapper take a partly covered pixel line to give a sideways footprint an even width-height difference. That line is inside the image, but with a straighten angle the crop samples the footprint turned by that angle, which reaches further out. CropView fits a straightened frame so that this area touches the image edge, so the extra line could pull it up to about a pixel past the edge, and the export got semi-transparent corners (for example 6 px with alpha 201 for a 301x200 image turned 90° and straightened by 2°). bfb68ca and 5.1.0 did not do this. A line is now taken only while the footprint, turned by the straighten angle about its center, stays inside the image. Otherwise the longer side loses a pixel, as in bfb68ca. Without straighten the check always passes, so unstraightened crops snap as in 4756382. The doc comments of RenderCrop.cropRect and CropFeature(displayCropRect:) now say that a sideways crop may take one partly covered line instead of snapping strictly inward. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
770dc0f clamped the rect before its footprint only when the footprint missed the image entirely, although its doc comment talks about rects whose center lies outside. A malformed turned crop whose footprint only grazes the image skipped that step and reopened as a sub-pixel sliver (a footprint overlapping by 0.01 px reopened as 55 x 0.01 and was committed again as 55 x 1), where 5.1.0 clamped it to a usable frame. The rect is now clamped first whenever its center is outside the image, which matches the comment and covers both cases. A well-formed crop always has its center inside, and unrotated and half-turn rects skip the step as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4756382 described the 2 px loss per open and Done cycle as an odd width-height case, but it hit any 90° or 270° crop: once the sub-pixel error on Done made the inward snap drop one line, the difference was odd, and the parity trim dropped a second one. With bfb68ca alone, a 300x200 image turned 90° through CropView was already stored 2 rows short on the first Done. 4756382 fixed that as well, so the test now runs on 300x200 as well as 301x200 and is named for what it checks. What stays stable is the whole-image turn (and an aspect-locked one). A partial or straightened turned crop can still change by about 1 px per cycle, the same creep 5.1.0 has for unrotated crops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The comment said the frame is turned back by 120°. `.quarterCW` is −90° and the straighten is added to it, so the total is −60°. The reach it quotes (99.9 px) is the same for 60° and 120°, so the case is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
muukii
changed the title
muukii
changed the title
muukii
changed the title
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.
Summary
Quarter-turning a crop could cut a rectangular image down to a square, lose a pixel on odd dimensions, or change its selection when reopened and confirmed. The crop now keeps its selection before output rotation and renders in this order: straighten → crop → quarter turn. A full 301×200 image exports as 200×301 after a sideways turn, preserving every source pixel.
Changes
API and persistence changes
CropFeature.cropRect, its display-coordinate adapter, andCropEditingState.cropExtentnow describe the selection before the output quarter turn. Code that authored rectangles in the previous output orientation must adopt this convention.Validation
Validated with Xcode 27 on iOS Simulator:
git diff --checkpassed.Remaining behavior
Done uses the latest recorded interaction state. Pressing Done or switching tools before a scroll/pinch settles can omit the final unrecorded movement. This change does not add synchronous gesture flushing. Physical-device verification has not been performed.