From 802ddcd6110ca322cabfd09fb6674b09f3d3e8e7 Mon Sep 17 00:00:00 2001 From: ingolfteetz Date: Mon, 26 Oct 2026 21:08:16 +0100 Subject: [PATCH] LR-CROP-AUTO: Auto straighten reads horizon and lines from one fresh analysis image From CodeRabbit's review of #2809: Auto straighten took the horizon from a fresh analysis image and the segments from the Upright analysis cache, whose key samples the source (and missed the optics section's state), so the two could come from different frames. `level_degrees` now detects the segments on the image it has just built (once per click). The Upright cache key also includes whether the optics section is on. Co-Authored-By: Claude Opus 5.5 --- crates/pipeline/src/tests_geometry.rs | 11 ++++++---- crates/pipeline/src/upright.rs | 30 +++++++++++---------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/crates/pipeline/src/tests_geometry.rs b/crates/pipeline/src/tests_geometry.rs index 12f9ca88ad..d88b6cf5dd 100644 --- a/crates/pipeline/src/tests_geometry.rs +++ b/crates/pipeline/src/tests_geometry.rs @@ -598,11 +598,10 @@ fn auto_straighten_takes_the_roll_from_converging_verticals() { assert!((deg + 3.0).abs() < 0.3, "{deg}"); } -/// The cached analysis tells lenses apart: the analysis image is corrected for `info.lens`, and -/// Auto straighten reads the horizon from a fresh image and the lines from the cache, so a hit -/// for another lens would mix two frames (review of #2809). +/// The Upright analysis cache tells apart what changes the analysis image: another lens (`info.lens`) +/// or the optics section switched off (review of #2809). #[test] -fn the_analysis_cache_tells_lenses_apart() { +fn the_analysis_cache_tells_lenses_and_optics_apart() { let src = tilted_scene(2.0, 30.0); let s = DevelopSettings::default(); let plain = SourceInfo::default(); @@ -612,4 +611,8 @@ fn the_analysis_cache_tells_lenses_apart() { let key = crate::upright::analysis_key; assert_eq!(key(&src, &plain, &s), key(&src, &plain.clone(), &s), "the same source, settings and lens"); assert_ne!(key(&src, &plain, &s), key(&src, &lens, &s), "another lens"); + let mut off = s.clone(); + off.disabled_sections.push("optics".into()); + assert!(!off.section_enabled("optics")); + assert_ne!(key(&src, &plain, &s), key(&src, &plain, &off), "the optics section off"); } diff --git a/crates/pipeline/src/upright.rs b/crates/pipeline/src/upright.rs index e9ce00695b..a77369eb4b 100644 --- a/crates/pipeline/src/upright.rs +++ b/crates/pipeline/src/upright.rs @@ -546,12 +546,6 @@ const ANALYSIS_LONG: usize = 1024; /// Analyse a source: the lens-corrected (optics applied, no perspective/crop) image at [`ANALYSIS_LONG`] px, /// segment detection and vanishing points. Cached per source + optics settings. pub fn analyze_source(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings) -> Analysis { - analyze_cached(src, info, s, None) -} - -/// [`analyze_source`], reading `plane` (the [`analysis_plane`] of the same source and settings) on a -/// cache miss instead of building it again. -fn analyze_cached(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings, plane: Option<&Plane>) -> Analysis { static CACHE: Mutex> = Mutex::new(Vec::new()); let key = analysis_key(src, info, s); if let Ok(c) = CACHE.lock() @@ -559,10 +553,7 @@ fn analyze_cached(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings, plane: O { return a.clone(); } - let a = match plane { - Some(p) => analyze(&detect_segments(p)), - None => analyze(&detect_segments(&analysis_plane(src, info, s))), - }; + let a = analyze(&detect_segments(&analysis_plane(src, info, s))); if let Ok(mut c) = CACHE.lock() { c.push((key, a.clone())); if c.len() > 8 { @@ -572,15 +563,17 @@ fn analyze_cached(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings, plane: O a } -/// What an analysis depends on: the source's pixels, the optics and orientation settings, and the -/// lens the frame is corrected for (`info.lens`, which [`analysis_plane`] reads through the frame). +/// What an analysis depends on: the source (by its sampled fingerprint), the optics and +/// orientation settings, whether the optics section is on, and the lens the frame is corrected for +/// (`info.lens`, which [`analysis_plane`] reads through the frame). pub(crate) fn analysis_key(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings) -> u64 { use std::hash::{Hash, Hasher}; - let mut lens = std::collections::hash_map::DefaultHasher::new(); - format!("{:?}", info.lens).hash(&mut lens); + let mut more = std::collections::hash_map::DefaultHasher::new(); + format!("{:?}", info.lens).hash(&mut more); + s.section_enabled("optics").hash(&mut more); crate::optics::fingerprint(src) ^ lightcraft_develop::DevelopSettings { optics: s.optics, orientation: s.orientation, ..Default::default() }.hash64().rotate_left(17) - ^ lens.finish().rotate_left(29) + ^ more.finish().rotate_left(29) } /// The image an analysis reads: the lens-corrected source (optics applied, no perspective or crop) @@ -600,10 +593,11 @@ pub(crate) fn analysis_plane(src: &Rgb32f, info: &SourceInfo, s: &DevelopSetting } /// The crop angle (degrees, clockwise +) Auto straighten levels `src` by, if it finds a horizon or -/// plumb lines it can trust ([`crate::straighten`]). Used by the crop panel's Auto straighten. +/// plumb lines it can trust ([`crate::straighten`]). Used by the crop panel's Auto straighten. It +/// reads the horizon and the segments from the same analysis image, built here (not through +/// [`analyze_source`]'s cache: it runs once per click). pub fn level_degrees(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings) -> Option { - let plane = analysis_plane(src, info, s); - crate::straighten::decide(&plane, &analyze_cached(src, info, s, Some(&plane))) + crate::straighten::level_plane(&analysis_plane(src, info, s)) } /// The automatic Upright correction for `mode` on `src` (identity for Off and Guided).