Hold the repairs a photographer makes, and say which may share a pass

A spot is a disc, a source offset and four numbers, and it lives beside `ops` for the reason `masks` and `film` do: the operation trait is ParamId -> f32, and a list of repairs is neither scalar nor fixed. Two decisions here are not obvious. The id is derived from the position rather than counted, because two devices editing offline would each mint `spot3` for different marks and the sidecar merge would then treat two repairs as one — from the position, two devices that removed the same piece of dust agree, and two that removed different ones do not. And every length is in the frame's isotropic units, not a mixture of those and shorter-edge fractions: one unit for the radius, the feather and the offset agrees on a landscape frame and on a portrait one, where a mixture only agrees on the first. `rounds` is the arithmetic that keeps a source from reading a destination. Every spot in one pass reads the photograph as it stood before that pass, so a spot sourcing from an earlier spot's destination would copy the mark that spot was removing. Grouping is not a pass per spot — that is sixty-four dispatches for a case that almost never arises — it is a new round only when the sources actually collide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-26 19:50:00 +02:00
parent 8d8d6491ad
commit 97479a0512
6 changed files with 794 additions and 18 deletions
@@ -0,0 +1,241 @@
+//! TRACES: FR-DEV-8
+//! The spot model: identity, bounds, and which repairs may share a pass.
+//!
+//! Everything here is arithmetic and bookkeeping, which is exactly why it is
+//! tested without a device: the three ways this model can be wrong — an id that
+//! is not stable, a bound that drops work silently, a grouping that lets a
+//! source read a destination — all produce a *picture* that is subtly wrong and
+//! no error anywhere.
+
+use dr_pipeline::spot::{
+    Spot, SpotMode, SpotSet, DEFAULT_RADIUS, MAX_SOURCE_DISTANCE, MAX_SPOTS, MIN_RADIUS,
+};
+use dr_pipeline::EditGraph;
+
+/// A 3:2 frame, which is the shape that catches a unit confusion. On a square
+/// one every wrong answer happens to be right.
+const ASPECT: f32 = 1.5;
+
+fn spot_at(centre: (f32, f32), offset: (f32, f32)) -> Spot {
+    Spot::new(centre, offset, DEFAULT_RADIUS)
+}
+
+/// Two devices that remove the same piece of dust must agree on its id, or the
+/// sidecar merge treats one repair as two and both survive — a spot drawn
+/// twice, which is visible.
+#[test]
+fn the_same_placement_mints_the_same_id() {
+    let a = spot_at((0.25, 0.75), (0.05, 0.0));
+    let b = spot_at((0.25, 0.75), (-0.02, 0.03));
+    assert_eq!(a.id, b.id, "the id is the position, not the whole spot");
+
+    let elsewhere = spot_at((0.26, 0.75), (0.05, 0.0));
+    assert_ne!(a.id, elsewhere.id);
+}
+
+/// And the id must not move when the repair does: dragging a spot is an edit to
+/// a spot, not the deletion of one and the creation of another. If the id
+/// followed the centre, a drag on one device and a radius change on the other
+/// would merge as two unrelated spots.
+#[test]
+fn dragging_a_spot_keeps_its_id() {
+    let mut spot = spot_at((0.25, 0.75), (0.05, 0.0));
+    let id = spot.id.clone();
+    spot.set_centre((0.9, 0.1));
+    spot.set_offset((0.1, 0.1));
+    spot.set_radius(0.2);
+    assert_eq!(spot.id, id);
+}
+
+/// Two spots placed on the same point are still two repairs, and a set that
+/// held them both under one id would lose one of them at the next save.
+#[test]
+fn a_second_spot_on_the_same_point_gets_its_own_id() {
+    let mut set = SpotSet::new();
+    let first = set.place(spot_at((0.5, 0.5), (0.05, 0.0))).unwrap();
+    let second = set.place(spot_at((0.5, 0.5), (0.0, 0.05))).unwrap();
+
+    assert_ne!(first, second);
+    assert_eq!(set.len(), 2);
+    assert!(set.get(&first).is_some() && set.get(&second).is_some());
+}
+
+/// The bound refuses rather than dropping. A set that quietly discarded the
+/// oldest repair would remove work already on screen, with nothing said.
+#[test]
+fn the_limit_refuses_and_keeps_what_is_there() {
+    let mut set = SpotSet::new();
+    for i in 0..MAX_SPOTS {
+        let y = i as f32 / MAX_SPOTS as f32;
+        assert!(set.place(spot_at((0.5, y), (0.05, 0.0))).is_some());
+    }
+    let first = set.spots()[0].clone();
+
+    assert!(set.place(spot_at((0.1, 0.1), (0.05, 0.0))).is_none());
+    assert_eq!(set.len(), MAX_SPOTS);
+    assert_eq!(
+        set.spots()[0],
+        first,
+        "the oldest repair survives the refusal"
+    );
+}
+
+/// The offset bound is what keeps the detail pass's halo finite, so it has to
+/// hold along the diagonal and not merely per axis — and it must keep the
+/// direction the photographer dragged in.
+#[test]
+fn a_source_dragged_too_far_stops_in_the_direction_it_was_going() {
+    let spot = spot_at((0.5, 0.5), (3.0, 4.0));
+    let distance = spot.distance();
+    assert!(
+        (distance - MAX_SOURCE_DISTANCE).abs() < 1e-3,
+        "clamped to the bound, got {distance}"
+    );
+    // 3:4 in, 3:4 out.
+    assert!((spot.offset.0 / spot.offset.1 - 0.75).abs() < 1e-3);
+}
+
+#[test]
+fn a_radius_cannot_be_dragged_to_nothing() {
+    let mut spot = spot_at((0.5, 0.5), (0.05, 0.0));
+    spot.set_radius(0.0);
+    assert!(spot.radius >= MIN_RADIUS);
+}
+
+/// The offset is in frame units and the source is in normalised ones, and the
+/// aspect goes on exactly one of the two axes. Getting this backwards puts the
+/// source somewhere the photographer did not drag it, by a third of the frame
+/// on a 3:2 — visible, and easy to write.
+#[test]
+fn the_source_converts_frame_units_to_normalised_ones() {
+    let spot = spot_at((0.5, 0.5), (0.15, 0.15));
+    let (sx, sy) = spot.source(ASPECT);
+
+    assert!((sx - (0.5 + 0.15 / ASPECT)).abs() < 1e-4);
+    assert!((sy - 0.65).abs() < 1e-4);
+
+    // The displacement is equal on both axes in frame units, so it must be
+    // *unequal* in normalised ones on a frame that is not square.
+    assert!((sx - 0.5) < (sy - 0.5));
+}
+
+/// A spot with no source reads the pixel it writes: the identity, at the cost
+/// of a dispatch. A freshly placed spot is in that state until a source is
+/// found for it, which is why the question is asked per spot.
+#[test]
+fn a_spot_with_no_offset_draws_nothing() {
+    let mut set = SpotSet::new();
+    let id = set.place(spot_at((0.5, 0.5), (0.0, 0.0))).unwrap();
+    assert!(set.is_neutral());
+    assert_eq!(set.rounds(ASPECT).len(), 0);
+
+    set.get_mut(&id).unwrap().set_offset((0.08, 0.0));
+    assert!(!set.is_neutral());
+    assert_eq!(set.rounds(ASPECT), vec![vec![0]]);
+}
+
+#[test]
+fn a_disabled_spot_draws_nothing_but_is_kept() {
+    let mut set = SpotSet::new();
+    let id = set.place(spot_at((0.5, 0.5), (0.08, 0.0))).unwrap();
+    set.get_mut(&id).unwrap().enabled = false;
+
+    assert!(set.is_neutral());
+    assert_eq!(set.len(), 1, "disabling is not deleting");
+}
+
+/// Spots scattered over a sky with their sources beside them are the
+/// overwhelming majority, and they must cost one dispatch.
+#[test]
+fn repairs_that_do_not_interfere_share_one_pass() {
+    let mut set = SpotSet::new();
+    for i in 0..8 {
+        let y = 0.1 + 0.1 * i as f32;
+        set.place(spot_at((0.5, y), (0.04, 0.0)));
+    }
+    assert_eq!(set.rounds(ASPECT), vec![(0..8).collect::<Vec<_>>()]);
+}
+
+/// The case the grouping exists for: the second spot reads from where the first
+/// one is repairing. In one pass it would copy the mark the first spot is
+/// removing, and the mark would reappear somewhere else in the frame.
+#[test]
+fn a_source_over_an_earlier_repair_opens_a_new_pass() {
+    let mut set = SpotSet::new();
+    // Repairs (0.30, 0.50) from (0.40, 0.50) — both in frame units on x.
+    set.place(spot_at((0.2, 0.5), (0.1, 0.0)));
+    // Repairs (0.60, 0.50) by reading (0.30, 0.50): exactly the first
+    // destination.
+    set.place(spot_at((0.4, 0.5), (-0.3, 0.0)));
+
+    assert_eq!(set.rounds(ASPECT), vec![vec![0], vec![1]]);
+}
+
+/// Two repairs landing on top of each other is not a hazard — they write the
+/// same output and the later one lands on top, which is the order they were
+/// made in. Splitting a pass for it would cost a dispatch for nothing.
+#[test]
+fn overlapping_destinations_stay_in_one_pass() {
+    let mut set = SpotSet::new();
+    set.place(spot_at((0.5, 0.5), (0.2, 0.0)));
+    set.place(spot_at((0.505, 0.5), (0.2, 0.05)));
+
+    assert_eq!(set.rounds(ASPECT).len(), 1);
+}
+
+/// A spot is an edit like any other, so a graph holding one is not clean — and
+/// a graph whose only spot draws nothing is.
+#[test]
+fn a_placed_repair_makes_the_graph_dirty() {
+    let mut graph = EditGraph::default_chain();
+    assert!(graph.is_neutral());
+
+    graph.spots_mut().place(spot_at((0.5, 0.5), (0.0, 0.0)));
+    assert!(graph.is_neutral(), "a spot with no source is not an edit");
+
+    graph.spots_mut().place(spot_at((0.2, 0.2), (0.08, 0.0)));
+    assert!(!graph.is_neutral());
+}
+
+/// Moving a spot must re-run the neighbourhood passes and nothing before them.
+/// If it moved the colour key, dragging a spot would re-run the fused dispatch
+/// and every mask on the frame with it (FR-DEV-3d).
+#[test]
+fn a_repair_moves_the_detail_key_alone() {
+    use dr_pipeline::Affects;
+
+    let mut graph = EditGraph::default_chain();
+    let before = graph.invalidation();
+
+    let id = graph
+        .spots_mut()
+        .place(spot_at((0.5, 0.5), (0.08, 0.0)))
+        .unwrap();
+    let after = graph.invalidation();
+
+    assert_eq!(
+        before.through(Affects::Colour),
+        after.through(Affects::Colour),
+        "a repair is not a colour change"
+    );
+    assert_ne!(
+        before.through(Affects::Detail),
+        after.through(Affects::Detail)
+    );
+
+    // And a change *to* a spot moves it again.
+    let placed = graph.invalidation();
+    graph.spots_mut().get_mut(&id).unwrap().set_radius(0.05);
+    assert_ne!(
+        placed.through(Affects::Detail),
+        graph.invalidation().through(Affects::Detail)
+    );
+}
+
+#[test]
+fn modes_survive_their_own_names() {
+    for mode in [SpotMode::Heal, SpotMode::Clone] {
+        assert_eq!(SpotMode::from_name(mode.name()), Some(mode));
+    }
+    assert_eq!(SpotMode::from_name("smudge"), None);
+}