Give back what a straighten took, when the angle comes back
Build and test / Desktop (Linux) (push) Successful in 2h16m6s
Build and test / Layer separation (push) Successful in 52s
🐳 Android image / Build and push (push) Successful in 4s
Build and test / android-image (push) Successful in 4s
Traceability / Requirement traces (push) Successful in 51s
Build and test / Android (aarch64) (push) Successful in 47m10s
Build and test / Desktop (Linux) (push) Successful in 2h16m6s
Build and test / Layer separation (push) Successful in 52s
🐳 Android image / Build and push (push) Successful in 4s
Build and test / android-image (push) Successful in 4s
Traceability / Requirement traces (push) Successful in 51s
Build and test / Android (aarch64) (push) Successful in 47m10s
The auto-crop only ever shrank. Straighten to 20 degrees and the corners are cropped away correctly; come back to 3, or all the way to zero, and the crop stays at the size 20 degrees demanded. Nothing on screen explains why the photograph is still small, and the only way back was undo. The cause was that each correction was computed from the previous correction's output, so it accumulated: every angle the slider rested at took its cut and none was ever returned. The fix is to stop accumulating and recompute. The applied crop is now always the user's own rectangle fitted into the current angle's safe area, so as the angle falls and that area opens up the crop grows back — and stops, exactly, at the rectangle they chose. At zero the safe area is the whole frame and the fit is the identity, which is what carries it the last of the way home. There is deliberately no early exit for the upright case now: that exit is precisely what would strand the crop small. **The intent is remembered as a pair, so it repairs itself.** The session keeps `(applied, intended)` — what the correction wrote, and what it was derived from — and trusts the remembered intent only while the graph still holds `applied`. Every other route to the crop leaves something else there: a handle dragged, a ratio chosen, a sidecar loaded, a paste, an undo. That mismatch is the signal the memory is stale, and the current rectangle becomes the new intent. The alternative was a write into this field from each of those paths, which is the kind of bookkeeping that is correct until someone adds a seventh path. Dragging a handle therefore *is* the user choosing, including at a non-zero angle: the correction will not later grow the crop past what they dragged it to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+70
-15
@@ -799,6 +799,34 @@ pub struct DevelopSession {
|
||||
/// sRGB until the application says otherwise, which is the same answer
|
||||
/// `dr_plat::display`'s fallback gives and means a session constructed in
|
||||
/// a test behaves exactly as it did before this existed.
|
||||
/// TRACES: FR-DEV-3
|
||||
/// What the straightening auto-crop last wrote, and what it was derived
|
||||
/// from: `(applied, intended)`.
|
||||
///
|
||||
/// **The graph holds the corrected rectangle; this remembers the intent
|
||||
/// behind it.** `auto_crop_to_angle` pulls the crop inside the area an
|
||||
/// angle leaves defined, and that operation can only ever shrink. Applied
|
||||
/// to its own output it ratchets — straighten to 20 degrees, come back to
|
||||
/// 3, and the crop stays at the size 20 degrees demanded, which is not
|
||||
/// what turning the slider back means. So the correction is never
|
||||
/// accumulated: it is recomputed from the intent every time, and as the
|
||||
/// angle falls the crop grows back and stops exactly where the user put
|
||||
/// it. At zero degrees the safe area is the whole frame and the two are
|
||||
/// equal again.
|
||||
///
|
||||
/// **A pair rather than a single remembered rectangle, so it repairs
|
||||
/// itself.** Every other route to the crop — a handle dragged, a ratio
|
||||
/// chosen, a sidecar loaded, a paste, an undo — leaves the graph holding
|
||||
/// something other than `applied`, and that mismatch is exactly the signal
|
||||
/// that the remembered intent is stale. [`Self::intended_crop`] checks it
|
||||
/// rather than requiring each of those paths to remember to write here,
|
||||
/// which is the kind of bookkeeping that is correct until someone adds a
|
||||
/// seventh path.
|
||||
///
|
||||
/// Session-scoped. A sidecar records the crop that was *applied*, because
|
||||
/// that is the one that describes the photograph, so reopening starts from
|
||||
/// that rectangle as its own intent.
|
||||
auto_crop: Option<(CropRect, CropRect)>,
|
||||
display_space: dr_types::ColourSpace,
|
||||
}
|
||||
|
||||
@@ -871,6 +899,7 @@ impl DevelopSession {
|
||||
active_tab: None,
|
||||
curve_channel: 0,
|
||||
display_space: dr_types::ColourSpace::Srgb,
|
||||
auto_crop: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3333,9 +3362,23 @@ impl DevelopSession {
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3
|
||||
/// Pull the crop inside the area a straightening angle leaves defined.
|
||||
/// The crop the user chose, as distinct from the one currently applied.
|
||||
///
|
||||
/// Turning a rectangle inside its own bounds exposes the corners: there is
|
||||
/// The remembered intent, but only while it is still credible: if the
|
||||
/// graph no longer holds what the auto-crop wrote, something else has set
|
||||
/// the crop since — a handle, a ratio, a sidecar, a paste, an undo — and
|
||||
/// that new rectangle *is* the intent. See [`Self::auto_crop`].
|
||||
fn intended_crop(&self) -> CropRect {
|
||||
match self.auto_crop {
|
||||
Some((applied, intended)) if applied == self.graph.crop() => intended,
|
||||
_ => self.graph.crop(),
|
||||
}
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3
|
||||
/// Fit the crop to the area the straightening angle leaves defined.
|
||||
///
|
||||
/// Turning a rectangle inside its own bounds exposes its corners: there is
|
||||
/// no source pixel out there and the shader renders it black. Nothing in
|
||||
/// the render prevents it — a free angle deliberately does *not* change the
|
||||
/// output size, so that straightening a horizon leaves the frame where the
|
||||
@@ -3343,27 +3386,39 @@ impl DevelopSession {
|
||||
/// the corners of the finished photograph.
|
||||
///
|
||||
/// This is the correction, and it runs when the gesture **finishes**.
|
||||
/// Applied continuously it would shrink the crop on every frame of the
|
||||
/// slider and never grow it back, so a user who overshot to 20° and came
|
||||
/// back to 3° would be left with a crop ratcheted down by the excursion
|
||||
/// rather than by the angle they settled on. Once per gesture, that is
|
||||
/// bounded: the crop is only ever as small as the angles actually rested
|
||||
/// at, and undo steps back through them.
|
||||
/// Applied continuously it would fight the drag, shrinking the crop on
|
||||
/// every frame of the slider.
|
||||
///
|
||||
/// **It grows as well as shrinks.** The crop is recomputed from
|
||||
/// [`Self::intended_crop`] rather than from itself, so straightening
|
||||
/// further in takes more away and straightening back out gives it back,
|
||||
/// stopping at the rectangle the user actually chose. Deriving it from the
|
||||
/// applied crop instead — the obvious way, and how this first shipped —
|
||||
/// ratchets: every angle the slider rested at takes its cut and none of
|
||||
/// them is ever returned, so coming back to zero leaves a crop that
|
||||
/// nothing on screen explains.
|
||||
///
|
||||
/// The crop keeps its own shape — so a locked ratio survives — and keeps
|
||||
/// the side of the frame it was on; see [`CropRect::fitted_into`] for why
|
||||
/// it is not simply replaced by the inscribed rectangle.
|
||||
pub fn auto_crop_to_angle(&mut self) {
|
||||
let (sw, sh) = self.demosaiced.size();
|
||||
// At zero this is the whole frame, and fitting into it is the identity
|
||||
// — which is what returns an over-corrected crop to its full size.
|
||||
// There is deliberately no early exit for the upright case: that exit
|
||||
// is precisely what would strand the crop small.
|
||||
let bound = self.graph.framing().max_inscribed_crop(sw, sh);
|
||||
// Upright, so every corner is already defined and there is nothing to
|
||||
// pull in from. Returning early rather than fitting into a full rect
|
||||
// matters: it keeps letting go of a slider at zero from touching the
|
||||
// crop at all.
|
||||
if bound.is_full() {
|
||||
return;
|
||||
let intended = self.intended_crop();
|
||||
let want = intended.fitted_into(bound);
|
||||
|
||||
if want != self.graph.crop() {
|
||||
self.graph.set_crop(want);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Op(dr_pipeline::framing::ID));
|
||||
}
|
||||
self.set_crop(self.graph.crop().fitted_into(bound));
|
||||
// Recorded even when nothing moved: the pairing is what tells the next
|
||||
// call that this rectangle is a correction rather than a choice.
|
||||
self.auto_crop = Some((want, intended));
|
||||
}
|
||||
|
||||
/// Set the straightening angle, in degrees.
|
||||
|
||||
Reference in New Issue
Block a user