docs/ had 26 developer documents flat beside the manual, and the two audiences are very differently sized: most readers want the manual and the gesture reference, a few want the register, the designs and the measurements. The manual and gestures.md stay at the top; everything for someone changing the code moves to docs/dev/, and the two documents that name their own successors — the v0.1 milestone and the UI-refinement plan — go to docs/dev/archive/ rather than being deleted, since both are still cited. docs/README.md is the index, users first. Every reference follows: code comments, Cargo manifests, the workflows, the pre-commit hook, the bench and traceability tools (which locate the repo root by docs/dev/requirements.md now), packaging, the Docker READMEs, CLAUDE.md, CONTRIBUTING.md and the README. The matrix links one level deeper and is regenerated. Links out of the moved documents into the tree gain a level; a link checker over every Markdown file finds none broken.
576 lines
29 KiB
Markdown
576 lines
29 KiB
Markdown
# Spot removal
|
||
|
||
**Status:** Draft · 2026-08-26
|
||
**Companion to:** [requirements.md](requirements.md) §3.3 FR-DEV-8 · [architecture.md](architecture.md) §5.2
|
||
|
||
The last develop feature the requirements ask for that nothing in the tree
|
||
implements. FR-DEV-8 states the shape — "non-destructive clone and heal spots
|
||
stored as parameters in the edit graph (target, radius, feather, source offset,
|
||
opacity, mode), with automatic source placement and manual override, plus a
|
||
visualise-spots mode" — and this document is how that lands on the pipeline
|
||
that exists now.
|
||
|
||
---
|
||
|
||
## 1. Why it is worth the work
|
||
|
||
Sensor dust is unavoidable with interchangeable lenses, and a dust spot is the
|
||
most common reason a photographer leaves a RAW editor for a pixel editor
|
||
mid-workflow. Every other develop operation in this application can be the best
|
||
one in its class and the workflow still breaks at the first frame with a mark on
|
||
the sky.
|
||
|
||
It is also, unusually, a feature whose cost has already been paid twice over.
|
||
The neighbourhood stage exists ([`crate::detail`](../../core/dr-pipeline/src/detail.rs)),
|
||
the convention for storing geometry in normalised source coordinates exists
|
||
([`mask.rs`](../../core/dr-pipeline/src/mask.rs)), the canvas-drag pattern exists
|
||
([`gradient.rs`](../../ui/dr-ui/src/gradient.rs)), and the merge-by-id rule exists
|
||
([`sidecar.rs`](../../core/dr-pipeline/src/sidecar.rs)). What is genuinely new is
|
||
small and is named in §3.
|
||
|
||
## 2. Non-goals
|
||
|
||
- **Not layer-based pixel editing.** §1.3 of the requirements excludes that and
|
||
this does not reopen it. A spot is a handful of numbers in the edit graph; no
|
||
pixels are stored, and the original file is never touched.
|
||
- **Not content-aware fill.** The source is a patch from the same photograph,
|
||
chosen by an offset. Synthesising texture that is not in the frame is a
|
||
different problem with a different budget.
|
||
- **Not a general clone brush.** A spot is a disc, not a stroke. A dragged
|
||
clone brush is expressible on top of this (a stroke *is* a run of discs) and
|
||
is deliberately left until the disc is finished and used.
|
||
- **Not automatic dust detection.** Finding spots without being asked is a
|
||
reasonable later feature and a bad first one: a false positive silently alters
|
||
a photograph, which is the failure this application must not have.
|
||
|
||
## 3. What is new, precisely
|
||
|
||
Four things, and it is worth being blunt about them because everything else in
|
||
this document is assembly of parts that already work:
|
||
|
||
1. **A detail pass with variable-length data.** Every [`DetailPass`] today
|
||
carries a `Vec<f32>` of uniforms fixed by its own structure. A spot list is
|
||
neither fixed nor small. §7.
|
||
2. **A neighbourhood operation whose reach is not a small kernel.** Every
|
||
existing pass declares a halo of a few pixels. A spot reads from wherever its
|
||
source is, which may be a third of the frame away. §5.3.
|
||
3. **Undo over something that is not a parameter.** [`History`] snapshots a
|
||
[`Preset`], which is a map of scalars — so mask edits are already outside
|
||
undo, and spots must not be. §10.4.
|
||
4. **A canvas mode that *creates* objects.** Crop edits one rect; local selects
|
||
a region; gradient drags an existing shape. Nothing yet makes a new thing
|
||
where the pointer went down. §10.
|
||
|
||
## 4. The model
|
||
|
||
```rust
|
||
/// TRACES: FR-DEV-8
|
||
pub struct Spot {
|
||
/// Stable across devices; see below.
|
||
pub id: String,
|
||
/// What is being covered, in normalised **source** coordinates.
|
||
pub centre: (f32, f32),
|
||
/// What covers it, as an offset from `centre` in **frame units**
|
||
/// (y spans 0..1, x spans 0..aspect — the mask convention).
|
||
pub offset: (f32, f32),
|
||
/// The radius of the disc, in frame units.
|
||
pub radius: f32,
|
||
/// Fraction of `radius` over which the edge falls away. 0 is hard.
|
||
pub feather: f32,
|
||
/// How much of the patch is laid down. 1.0 is opaque.
|
||
pub opacity: f32,
|
||
pub mode: SpotMode, // Heal | Clone
|
||
pub enabled: bool,
|
||
}
|
||
```
|
||
|
||
**Units follow the mask rule, for the mask reason.** A length stored in pixels
|
||
is a length that means something different in the preview and in the export
|
||
([`RenderScale`]'s whole documentation is this argument). `centre` is normalised
|
||
source, so a crop, a zoom, a pan and a rotation move the spot with the
|
||
photograph and no arithmetic is needed to keep it there.
|
||
|
||
Every *length* — radius, feather, offset — is in the frame's **isotropic
|
||
units**, `MaskSource::Radial`'s convention, where y spans `0..1` and x spans
|
||
`0..aspect`. Only in those units is a disc a disc: normalised coordinates would
|
||
make a spot on a 3:2 frame an ellipse half again wider than it is tall. They are
|
||
lengths against the *source* frame rather than the rendered region, so cropping
|
||
does not resize a spot already placed — a dust mark is a fact about the sensor,
|
||
not about the composition.
|
||
|
||
One unit for all three, deliberately. A radius in shorter-edge fractions beside
|
||
an offset in frame units agrees on a landscape frame and silently disagrees on a
|
||
portrait one, which is a bug that stays invisible until somebody rotates a
|
||
photograph.
|
||
|
||
**`offset` is a vector, not a second point.** Dragging the destination moves the
|
||
source with it, which is what a photographer expects when they nudge a spot half
|
||
a pixel and do not want to re-place the source. Moving the source alone is
|
||
editing `offset`.
|
||
|
||
**The id is derived, not counted.** `MaskStack::next_id` numbers layers, which
|
||
is fine for a stack a user names, and wrong here: two devices that each place a
|
||
spot offline would both produce `spot3`, and the merge in §11 would treat two
|
||
different marks as one. So the id is a short base-36 hash of the centre at
|
||
creation, and two devices that place a spot in the same place produce the same
|
||
id — which is the correct outcome, because they removed the same piece of dust.
|
||
|
||
```rust
|
||
pub struct SpotSet {
|
||
spots: Vec<Spot>, // in creation order; the order matters, see §5.2
|
||
}
|
||
```
|
||
|
||
### 4.1 Why it lives beside `ops`, not in it
|
||
|
||
The [`Operation`] trait takes a `ParamId` and returns an `f32`, and the whole
|
||
generic machinery above it — the panel, the sidecar, the presets, the history —
|
||
is built on that being true. A spot list is not scalars, and the trait says so
|
||
explicitly where it refuses a downcast for film tables.
|
||
|
||
[`EditGraph`] already holds three things that are not operations for exactly
|
||
this reason: `framing`, `masks` and `film`. `spots` is the fourth, and the
|
||
argument is the same one `masks` makes — a stack of layers is not a slider, and
|
||
folding it into the list would make every consumer that walks `ops` know that
|
||
some entries are not really operations.
|
||
|
||
Bounds, following `mask.rs`'s example of bounding what a sidecar can grow to:
|
||
|
||
| Constant | Value | Why |
|
||
|---|---|---|
|
||
| `MAX_SPOTS` | 64 | Beyond a few dozen the answer is to clean the sensor. Refuses rather than dropping, as `MaskStack::push` does. |
|
||
| `MAX_SOURCE_DISTANCE` | 0.5 | Frame units. Bounds the halo in §5.3, which is otherwise unbounded. |
|
||
| `DEFAULT_RADIUS` | 0.012 | Frame units — about 25 px on a 24 MP frame's short edge, which is a dust mark. |
|
||
| `MIN_RADIUS` / `MAX_RADIUS` | 0.001 / 0.5 | Not zero, because a spot that repairs nothing reads as a broken tool; not larger, because the halo bound has to mean something. |
|
||
| `DEFAULT_FEATHER` | 0.35 | Fraction of the radius. Soft enough that a heal on a gradient sky has no visible boundary. |
|
||
|
||
## 5. Where it runs
|
||
|
||
### 5.1 First in the detail chain
|
||
|
||
ARCH §5.2 draws spot removal *after* texture and clarity and *before* sharpen
|
||
and NR. That diagram is already out of step with the operation set — the
|
||
`order:` keys in `core/dr-pipeline/ops/` put noise reduction at 110 and capture
|
||
sharpening at 120, ahead of clarity at 130 and texture at 140 — so it needs a
|
||
correction anyway, and the correction should put spot removal **first among the
|
||
neighbourhood passes**, at a notional order of 105.
|
||
|
||
The reason is the halo. Sharpening a dust spot before removing it amplifies its
|
||
edge, and the amplified edge is wider than the spot: the sharpening kernel has
|
||
already smeared a dark ring into pixels that the spot's own disc does not cover,
|
||
so the heal leaves a faint circle of over-sharpened background around a patch
|
||
that is otherwise perfect. Removing the mark first means every later pass sees a
|
||
photograph with no mark in it, which is also the photograph the photographer
|
||
thinks they are sharpening.
|
||
|
||
Since the spot set is not in `ops`, `EditGraph::compose_detail_for` splices its
|
||
passes in front of the ops' passes rather than sorting by a declared order. That
|
||
is a two-line change and it is stated here so nobody looks for a `spots.yaml`.
|
||
|
||
### 5.2 Rounds, because sources can read destinations
|
||
|
||
Every pass reads one texture and writes another. So within a single pass, every
|
||
spot reads the *unhealed* image — and a spot whose source overlaps an earlier
|
||
spot's destination copies the mark the earlier spot was removing.
|
||
|
||
The fix is not to run one pass per spot (64 dispatches for a frame that needs
|
||
one). It is to group: walking the spots in creation order, a spot joins the
|
||
current round unless its source disc intersects the destination disc of a spot
|
||
already in that round, in which case it opens a new one. One pass per round, and
|
||
the common case — spots scattered over a sky, sources near their own
|
||
destinations — is a single round. The grouping is plain CPU code over at most 64
|
||
discs and belongs in `SpotSet`, with a test that says an overlapping pair
|
||
produces two rounds and a disjoint pair produces one.
|
||
|
||
### 5.3 The halo, honestly
|
||
|
||
[`DetailPass::radius`] is "the furthest this pass reads from the pixel it
|
||
writes", and it exists so that ARCH §5.3's tile scheduler knows how far to grow
|
||
a tile. For a spot pass that is `max(|offset| + radius)` over the pass's spots,
|
||
in render pixels — which with `MAX_SOURCE_DISTANCE` at 0.5 can approach half the
|
||
frame.
|
||
|
||
That is a real cost and it should be written down rather than discovered: a
|
||
frame with a long-armed spot is close to untileable for that one pass, so the
|
||
tiled path will compute it whole-frame. Two things keep it affordable. The pass
|
||
is cheap per pixel (§6.4), and it is only the *spot* passes that carry the halo
|
||
— the sharpening pass after it still declares its three pixels and still tiles.
|
||
The alternative, clamping the source distance to something tile-sized, would
|
||
make the tool useless exactly where it is most needed: a mark on a face is
|
||
healed from the other cheek, and that is a long way.
|
||
|
||
## 6. What a spot does to the pixels
|
||
|
||
Both modes work on the same disc. For a pixel at render coordinate `p` inside a
|
||
spot centred at `d` with radius `r`, with the source at `s = d + offset`:
|
||
|
||
```
|
||
w = falloff(|p - d| / r) // 1 at the centre, 0 at the rim
|
||
patch = bilinear(source_texture, p - d + s)
|
||
c = mix(c, patch + membrane, w * opacity)
|
||
```
|
||
|
||
`falloff` is a smoothstep over the outer `feather` fraction of the radius; a
|
||
feather of 0 is a hard disc. `bilinear` is four `tap`s and two lerps, because
|
||
the generated preamble offers `textureLoad` only and the offset is fractional in
|
||
render space — it becomes a [`Helper`], deduplicated across passes like the
|
||
existing luminance helper.
|
||
|
||
`membrane` is what separates the two modes, and it is zero for `Clone`.
|
||
|
||
### 6.1 Heal, without a Poisson solve — **implemented**
|
||
|
||
The classic heal is Poisson blending: copy the *gradients* of the source and
|
||
solve for the image whose gradients they are, subject to matching the
|
||
destination on the boundary. Solved properly that is an iterative linear system
|
||
— tens of Jacobi passes over the disc — and each iteration is a dispatch in this
|
||
architecture. Sixty dispatches to remove a dust spot is not a frame budget.
|
||
|
||
What that solve produces is a smooth membrane interpolating the boundary
|
||
difference, and a membrane can be interpolated directly instead of solved. The
|
||
shipped form samples the difference between destination and source at `K` points
|
||
around the rim and interpolates them into the interior by inverse square
|
||
distance:
|
||
|
||
```
|
||
for k in 0..K:
|
||
b_k = tap(rim_k) - tap(rim_k + offset) // boundary difference
|
||
w_k = 1 / max(|p - rim_k|², 1)
|
||
membrane = Σ w_k·b_k / Σ w_k
|
||
```
|
||
|
||
`K = 24` — `RIM_SAMPLES` in `core/dr-pipeline/src/spot.rs`, a uniform rather
|
||
than a constant in the source, so tuning it uploads a buffer instead of
|
||
recompiling. The cost is `2K` bilinear samples per pixel *inside a disc*, and
|
||
nothing at all outside one.
|
||
|
||
**What was originally specified here was mean-value seamless cloning** (Farbman
|
||
et al., 2009), whose weights are half-angle tangents over the rim rather than
|
||
inverse squares. The difference matters when the boundary difference varies
|
||
sharply around the rim; on the case that actually arises — a repair on a
|
||
smoothly varying background — both reduce to the same answer, and the inverse
|
||
square form costs two transcendentals per sample fewer. The measurement in
|
||
`core/dr-gpu/tests/spot_removal.rs` is what decides whether that trade stays
|
||
good: on a linear ramp steep enough to make a clone wrong by 38 levels out of
|
||
255, the heal is wrong by **0**. If a case turns up where it is not, the weights
|
||
are four lines and the tests are already written.
|
||
|
||
### 6.2 Why not compute the boundary statistics on the CPU
|
||
|
||
Because that means reading the rendered image back, and FR-DEV-4 forbids it in
|
||
the render path for reasons ARCH §6.1 spends a page on. A per-frame readback to
|
||
find out what colour a sky is would reintroduce exactly the stall the whole
|
||
architecture exists to avoid. The mean-value form needs no reduction at all,
|
||
which is most of why it is the right answer here.
|
||
|
||
### 6.3 Clone
|
||
|
||
`membrane = 0`. Kept because heal is wrong on a boundary: a spot straddling a
|
||
horizon healed by mean-value blending smears the horizon's contrast into the
|
||
disc, and the honest tool then is a straight copy from a matching part of the
|
||
frame. This is why FR-DEV-8 asks for both, and it costs one branch in the shader
|
||
and one segmented control in the panel.
|
||
|
||
### 6.4 Cost
|
||
|
||
Per pixel, per pass: a rejection test per spot in the pass (a squared distance
|
||
and a compare), and for the pixels actually inside a disc, `4 + 2K` taps. With
|
||
64 spots the rejection cost is the dominant term and it is about 64 × 4 ALU ops
|
||
on every pixel of the frame — call it a millisecond at 2 MP on integrated
|
||
graphics, which is affordable but not free.
|
||
|
||
If it proves not to be, the fix is the one `mask.rs` already uses for strokes: a
|
||
bounding box per spot and a dispatch sized to it. That needs the detail runner
|
||
to dispatch something other than the whole frame, which is a change to its
|
||
shape, and it is deliberately not being made until a measurement asks for it.
|
||
|
||
## 7. Getting a spot list to the GPU
|
||
|
||
[`DetailPass`] gains one field:
|
||
|
||
```rust
|
||
/// Per-instance data too large or too variable for the uniform block.
|
||
pub storage: Option<Vec<[f32; 4]>>,
|
||
```
|
||
|
||
and the generated preamble gains one binding:
|
||
|
||
```wgsl
|
||
@group(0) @binding(3) var<storage, read> instances: array<vec4<f32>>;
|
||
```
|
||
|
||
In `dr-gpu`, both bind group layouts gain a read-only storage entry at binding
|
||
3, and a pass that declares no storage binds a shared one-element dummy buffer.
|
||
wgpu permits a layout entry the shader does not use, so the two layouts stay two
|
||
rather than four, and no existing pass changes at all.
|
||
|
||
**A spot is two `vec4`s**: `(centre.x, centre.y, radius, feather)` and
|
||
`(offset.x, offset.y, opacity, flags)`, all in render pixels except `flags`,
|
||
converted on the CPU in `compose_detail_for` where the framing is in scope. This
|
||
matters: the shader never sees a normalised coordinate and never has to know
|
||
about crop, rotation or zoom — `Framing::output_at` does that map on the way in,
|
||
exactly as `gradient.rs` does it for handles. The framing is an affine
|
||
similarity, so a disc stays a disc and one radius scales by one factor:
|
||
`radius_px = radius × min(source_w, source_h) × scale.ratio()`.
|
||
|
||
**The source list does not recompile anything.** The WGSL is identical for one
|
||
spot and for sixty-four — the count is a uniform and the loop is over the
|
||
buffer — so `structure_hash` is unchanged as spots are placed, and placing the
|
||
tenth spot re-uploads a 512-byte buffer. This is the same property the fused
|
||
pass has for slider movement and it is worth a test that asserts
|
||
`cached_pipelines()` does not grow while spots are added.
|
||
|
||
**Rejected: packing spots into the uniform block.** It would touch no bind group
|
||
layout, which is genuinely attractive. It also requires the composer to emit
|
||
`vec4` uniform fields (it emits scalars), forces a fixed `MAX_SPOTS`-sized array
|
||
and its fixed upload cost into every spot pass, and gives the next operation
|
||
that wants a table — a LUT, a curve, a lens grid — nothing to build on. The
|
||
storage buffer is a few more lines once and useful again later.
|
||
|
||
**Invalidation.** The spot set folds into the `detail` key in
|
||
`EditGraph::invalidation`, alongside the detail operations. Dragging a spot
|
||
therefore re-runs the detail chain and *not* the fused colour pass or the
|
||
demosaic, which is exactly the reuse FR-DEV-3d asks for and is the difference
|
||
between a spot that follows the finger and one that stutters.
|
||
|
||
## 8. Automatic source placement
|
||
|
||
FR-DEV-8 asks for automatic placement with manual override. Two stages, because
|
||
the useful half is much cheaper than the good half.
|
||
|
||
**Stage one — a placed default.** A new spot's source is offset by `2.5 × radius`
|
||
in the direction that keeps it furthest inside the frame, biased towards the
|
||
frame centre. For dust on a sky, which is the overwhelming majority of spots,
|
||
this is right often enough to be worth having, and it is wrong in a way that is
|
||
immediately visible and one drag from fixed.
|
||
|
||
**Stage two — a scored search.** A compute dispatch per new spot scores candidate
|
||
offsets on two rings around the destination (say 32 candidates), each scored by
|
||
the sum of squared differences over the annulus just outside the destination
|
||
disc — the ring is what has to match, since the disc's interior is being
|
||
replaced anyway. Penalise candidates whose disc overlaps another spot's
|
||
destination, or the frame edge. The winner's offset is read back **once**, when
|
||
the spot is created, through `readback.rs` — a few hundred bytes, which is the
|
||
size the histogram already moves and completes in well under a frame — and
|
||
written into the spot.
|
||
|
||
Two rules about that readback, both of which are the difference between a
|
||
feature and a bug:
|
||
|
||
- It is **not** in the render loop. `readback.rs` blocks with a deadline, and
|
||
that is tolerable exactly once per placement and intolerable per frame. It
|
||
happens on the gesture, and the render that follows uses whatever the spot
|
||
currently holds.
|
||
- The result is **stored**, and the search is never re-run behind the user. A
|
||
spot whose source moved on its own when the file was reopened would be an edit
|
||
changing itself, and non-destructive editing means the sidecar decides what
|
||
the picture is.
|
||
|
||
If the readback fails, the stage-one default stands. There is no state in which
|
||
a spot has no source.
|
||
|
||
## 9. Visualising spots
|
||
|
||
FR-DEV-8's "visualise-spots mode" is two different things, and conflating them
|
||
is how one of them ends up missing:
|
||
|
||
**The overlay** — where the spots *are*. Circles for the destination, a fainter
|
||
circle for the source, a line between them for the selected spot. Drawn in Slint
|
||
over the canvas, alongside the gradient handles and by the same coordinate map,
|
||
so it costs the render path nothing and cannot leak into an export.
|
||
|
||
**The reveal** — where the spots *should be*. Lightroom's "Visualize Spots": a
|
||
high-contrast, desaturated view of the frame's high-frequency content, in which
|
||
sensor dust on a smooth sky is obvious and in a normal view is nearly invisible.
|
||
It is a detail pass appended to the chain:
|
||
|
||
```
|
||
c = abs(c - blur(c)) stretched by a threshold, greyscale, inverted
|
||
```
|
||
|
||
It is a **view**, not an edit. So it is not in the graph and not in the sidecar:
|
||
`compose_detail_for` takes a `DetailView` (`Normal` | `RevealSpots`) and the
|
||
export path passes `Normal`. A flag on the session would work until the day
|
||
somebody exports while the mode is on, and then it would produce a black-and-
|
||
white file that looks like corruption. Making the export call site name it is
|
||
what stops that from ever being possible.
|
||
|
||
## 10. Interaction
|
||
|
||
### 10.1 The mode
|
||
|
||
A `ViewMode::spots`, and a row in `ToolRail`'s table beside Crop and Local.
|
||
That table's own documentation already predicts this shape for the brush; the
|
||
spot tool is the same shape and arrives first.
|
||
|
||
(It landed as a third chip in `ModeStrip`, at the head of the develop column.
|
||
The three canvas tools have since moved out to a fixed rail down the left of
|
||
the develop view — `ui/dr-ui/ui/toolrail.slint` carries why — and what remains
|
||
of that strip is the adjustment-group filters, now `GroupStrip`. Nothing about
|
||
the mode itself changed in the move.)
|
||
|
||
### 10.2 Gestures
|
||
|
||
| Gesture | Effect |
|
||
|---|---|
|
||
| Tap / click on the photograph | Place a spot at the current radius, source auto-placed (§8), and select it |
|
||
| Drag from a spot's centre | Move the destination; the source follows |
|
||
| Drag from the source circle | Change the offset |
|
||
| Drag *out* from a fresh placement | Set the source directly, without the auto-placement |
|
||
| Scroll / pinch on a selected spot | Radius |
|
||
| Tap a spot | Select it; the panel scopes to it |
|
||
| `Delete` / `Backspace` | Remove the selected spot |
|
||
| Alt-click a spot | Remove it without selecting first |
|
||
| `Esc` | Leave the mode |
|
||
|
||
A drag is a displacement from the press, not a snap to the pointer — the rule
|
||
`gradient.rs` states and for the same reason: a finger-sized touch target snapped
|
||
to the pointer jumps by half a target the instant it is grabbed.
|
||
|
||
### 10.3 The panel
|
||
|
||
While a spot is selected, the adjust column shows radius, feather, opacity and
|
||
a Heal/Clone control for *that* spot, exactly as selecting a mask layer
|
||
re-scopes the column today. With nothing selected it shows the defaults new
|
||
spots will be created with, plus the reveal toggle.
|
||
|
||
### 10.4 Undo
|
||
|
||
[`History`] snapshots a [`Preset`], which is a parameter map — so today mask
|
||
edits are not undoable, and spot placement must not inherit that. `History`
|
||
should hold `(Preset, SpotSet)` and restore both.
|
||
|
||
That is a narrow change with a wide benefit: the same door lets the mask stack
|
||
join later, which closes a gap FR-DEV-5 has open right now. The coalescing rule
|
||
needs one addition — a drag of one spot's handle is one step, keyed by the spot
|
||
id in the same way a slider drag is keyed by its control — and placement,
|
||
deletion and mode changes each open a step of their own.
|
||
|
||
### 10.5 Touch
|
||
|
||
Every handle is a `Theme.touch-target`, per FR-UI-3. On a phone the destination
|
||
and source circles of a small spot overlap at that size, so the source handle is
|
||
drawn at a minimum arm length from the centre while the stored offset is
|
||
untouched — the trick `gradient.rs` uses with `MIN_ARM`, for the identical
|
||
reason: a handle that cannot be grabbed again is a one-way edit.
|
||
|
||
## 11. Persistence
|
||
|
||
One line per spot in the version block:
|
||
|
||
```
|
||
[version 8f04c0e2-…]
|
||
exposure.exposure = 0.75
|
||
spot.3f9k = 0.4213 0.2871 0.0120 0.35 0.0310 -0.0180 1 heal
|
||
```
|
||
|
||
Fields in order: `centre.x centre.y radius feather offset.x offset.y opacity
|
||
mode`. A line rather than a block because a spot is eight numbers and sixty-four
|
||
blocks would bury the rest of the file; a line *per spot* rather than one line
|
||
for the set because the line is the unit of merge and of a readable diff — the
|
||
same reasoning `write_strokes` gives for a line per stroke.
|
||
|
||
Coordinates are written at the same precision they are held at, as strokes are,
|
||
so a round trip is exact and two devices do not generate a diff of noise in the
|
||
sixth decimal.
|
||
|
||
**A malformed line costs that spot and not the file.** A truncated line is
|
||
dropped with a warning, exactly as `parse_stroke` drops a bad stroke: a spot
|
||
that silently lands somewhere the user never put it is worse than a spot that is
|
||
missing, because only one of the two is noticeable.
|
||
|
||
**Merge** follows `merge_masks` precisely: by id, disjoint survives, a spot both
|
||
sides edited resolves wholesale to the higher revision. Half of one device's
|
||
offset with the other's radius is a repair neither photographer made. Deletion
|
||
propagates through the base comparison exactly as a layer's does.
|
||
|
||
**Presets do not carry spots** in the first version — a preset is a look, and a
|
||
look does not include where the dust was. But dust is in the *same place on
|
||
every frame from that body*, which makes "copy spot removal to the selection"
|
||
genuinely valuable, and it is a stage of its own (§12, S7) rather than a
|
||
surprise inside the existing paste.
|
||
|
||
## 12. Stages
|
||
|
||
Each stage is shippable and each has something to look at. Test names are the
|
||
files they belong in.
|
||
|
||
**S1 — The model.** *(Done.)* `spot.rs` in `dr-pipeline`: `Spot`, `SpotMode`, `SpotSet`,
|
||
the bounds, id derivation, round grouping (§5.2). No GPU, no UI.
|
||
*Tests:* `core/dr-pipeline/tests/spots.rs` — id stability across two identical
|
||
placements, `MAX_SPOTS` refuses rather than drops, overlapping sources produce
|
||
two rounds, disjoint produce one.
|
||
|
||
**S2 — Persistence.** *(Done.)* Sidecar write, parse, round trip, merge.
|
||
*Acceptance:* a hand-written sidecar with three spots survives a load/save round
|
||
trip byte-identically, and two devices that each add a spot offline end with
|
||
both.
|
||
|
||
**S3 — The storage binding.** *(Done.)* `DetailPass::storage`, the preamble's binding 3,
|
||
the dummy buffer, the two layouts.
|
||
*Acceptance:* every existing detail test still passes untouched, and a synthetic
|
||
pass reading the buffer gets what was uploaded.
|
||
|
||
**S4 — Clone.** *(Done.)* The disc, the feather, the bilinear helper, the pass grouping,
|
||
the halo declaration, spliced first into the chain.
|
||
*Acceptance:* `core/dr-gpu/tests/spot_removal.rs` — a synthetic frame with a
|
||
black disc on a flat grey field is clean to within a tolerance after one clone
|
||
spot; the same edit at a one-quarter proxy and at full size land the disc in the
|
||
same *normalised* place; adding spots does not grow `cached_pipelines()`.
|
||
|
||
**S5 — Heal.** The membrane, the mode switch. **Done** — §6.1, and the measurement came out at 0 levels of error against a clone's 38.
|
||
*Acceptance:* a dark spot on a linear grey **gradient** — the case clone fails —
|
||
is clean to within a tolerance, and the residual at the disc boundary is below
|
||
the residual a clone leaves by an order of magnitude. This is the measurement
|
||
that decides `K`.
|
||
|
||
**S6 — The tool.** `ViewMode::spots`, the chip, placement, handles, selection,
|
||
the panel scope, deletion, history carrying the spot set, the stage-one source
|
||
default. **Done, except the reveal view** — §9's second half is the one piece
|
||
of S6 not built, and it is separable: it is a view mode over the detail chain
|
||
rather than part of the tool.
|
||
*Acceptance:* a dust mark on a real frame is gone in one click, the edit survives
|
||
a restart, and undo takes it back.
|
||
|
||
*What was verified, and how.* Everything below the interface is under test —
|
||
the model, the sidecar, the merge, the passes, both blend modes, and undo. The
|
||
interface itself was compiled, laid out and photographed: the strip renders
|
||
`Crop | Local | Repair` and the column re-scopes. It was **not** driven, because
|
||
synthetic clicks do not reach this application (the compositor refuses them),
|
||
so the gestures in §10 are as-written rather than as-felt. A first pass with a
|
||
real pointer is the outstanding work on this stage.
|
||
|
||
**S7 — The rest of FR-DEV-8.** The scored source search (§8 stage two), and
|
||
copying a spot set across a selection.
|
||
|
||
S1–S6 is the requirement met in the sense a photographer would recognise; S7 is
|
||
the sentence in FR-DEV-8 about automatic placement met in the sense the document
|
||
means it.
|
||
|
||
## 13. Documents to amend
|
||
|
||
- **requirements.md** — FR-DEV-8 has no *Acceptance:* line; every other
|
||
requirement of its weight does. Proposed: *"a dust mark on a smooth sky is
|
||
removed in one click with no visible boundary at 1:1, the spot survives a
|
||
crop, a rotation and an export at another size, and the exported file matches
|
||
the preview."*
|
||
- **architecture.md §5.2** — the stage list is out of step with the `order:`
|
||
keys in `ops/` and does not show spot removal first among the neighbourhood
|
||
passes. §5.1 above is the correction.
|
||
- **traceability.md** — regenerated, as ever, rather than edited. FR-DEV-8's row
|
||
currently points only at two comments that mention it.
|
||
|
||
## 14. Open questions
|
||
|
||
1. ~~**`K = 24`?**~~ Settled by S5's measurement: 24 samples, inverse square weights, zero error on the case the mode exists for.
|
||
2. **Does the reveal view belong to spot mode only,** or is it a view mode of
|
||
its own that a photographer can turn on while doing something else? It is
|
||
cheap to allow both; the risk is a mode nobody remembers turning on. Still
|
||
open, and now the only part of §9 unbuilt.
|
||
3. **Should a spot be clamped inside the crop?** A spot outside the current crop
|
||
costs nothing to render and is invisible, and re-cropping should bring it
|
||
back rather than find it deleted. Leaning strongly towards no clamp.
|
||
4. **One radius, or an ellipse?** Lightroom's spot tool is circular and its
|
||
users cope. An ellipse doubles the handle count for a case a second spot
|
||
already covers.
|