dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Give an edit one complete state, and make omitting part of it a compile error

Browse Source 
An edit used to be a bag of scalars. That stopped being true when the mask
stack and the film stock arrived, both deliberately held apart from `ops`
because a layer is not a scalar and a stock is not a scalar — and nothing
announced the change. What happened instead is that three routines each
captured "the edit" and each captured a different subset of it.

`EditState` is all of it: the parameter map, the masks, the stock. What keeps
it complete is not a comment. `EditGraph::state` destructures the graph
exhaustively, `EditGraph::set_state` destructures the state exhaustively, and
the fields are public so every construction site is a struct literal naming
all of them. Adding a fifth kind of graph state — FR-DEV-8's spot removal is
the one already asked for — fails to compile until somebody has decided
whether an undo has to put it back. Verified both ways round by adding a field
to each type and watching five call sites refuse to build.

A compiler error rather than a runtime check, because the failure being
prevented is silence: the missing halves produced no panic, no warning and no
failing test.

The stack is now shared rather than owned, and that is about the drag path
rather than memory: `state` runs on every parameter change, which during a
drag is once a frame, and deep-copying a painted brush sixty times a second to
record an exposure move would be a cost paid for nothing. `masks_mut` is the
one door a stack is modified through, so it clones on write.

`FilmRebake` is the one thing a caller is still owed. Restoring a stock always
needed the profile database this crate does not link (ARCH §6.5a); it was a
comment before, and it is a `#[must_use]` return value now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-26 22:50:11 +02:00
co-authored by Claude Opus 5
parent 4a82753d22
commit 5622a58ce3
5 changed files with 462 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-pipeline/src/graph.rs
+98 -4
View File
@@ -7,6 +7,8 @@
//! Order is data, not code: operations run in the sequence this holds them,
//! so reordering the pipeline needs no code change.
use std::sync::Arc;
use crate::descriptor::{
Attribute, Facet, LocalizedKey, OpDescriptor, OpId, ParamId, ParamKind, Presentation,
};
@@ -14,7 +16,9 @@ use crate::framing::{CropRect, Framing};
use crate::mask::MaskStack;
use crate::operation::{compose_full, ComposedShader, Operation};
use crate::ops;
use crate::preset::{Preset, Scope};
use crate::spot::SpotSet;
use crate::state::{EditState, FilmRebake, FilmRef};
/// TRACES: FR-DEV-3a
/// What one operation offers, as plain data.
@@ -93,7 +97,7 @@ pub struct EditGraph {
/// layer *contains* a chain of its own. Folding the stack into the global
/// list would make the list recursive and every consumer that walks it
/// have to know that some entries are really sub-graphs.
masks: MaskStack,
masks: Arc<MaskStack>,
/// TRACES: FR-DEV-3f
/// The film stock this edit renders through, if any.
///
@@ -156,7 +160,7 @@ impl EditGraph {
Self {
ops: ops::chain(),
framing: Framing::new(),
masks: MaskStack::new(),
masks: Arc::new(MaskStack::new()),
film: None,
spots: SpotSet::new(),
}
@@ -199,8 +203,15 @@ impl EditGraph {
&self.masks
}
/// The stack, to modify.
///
/// Clones on write. The stack is shared with every [`EditState`] snapshot
/// taken since it last changed — the undo stack holds a run of them — so
/// this is where a shared stack becomes this graph's own again. Callers
/// see no difference; what it buys is that recording a slider drag does
/// not deep-copy a painted mask once a frame.
pub fn masks_mut(&mut self) -> &mut MaskStack {
&mut self.masks
Arc::make_mut(&mut self.masks)
}
/// The framing — crop, straighten, rotation and flips.
@@ -331,6 +342,89 @@ impl EditGraph {
self.film.as_ref()
}
/// TRACES: FR-DEV-5 | FR-CAT-8
/// The whole edit, as data — what an undo step and a sidecar are both
/// made of.
///
/// **The pattern below is exhaustive on purpose.** It is the only thing
/// standing between a new kind of graph state and an undo that quietly
/// ignores it, which is exactly how the mask stack came to be missing
/// from the history for as long as it was. Never add `..` to it: a field
/// added to this struct should fail to compile here until somebody has
/// decided whether stepping backwards has to put it back. See
/// [`crate::state`].
pub fn state(&self) -> EditState {
let Self {
// Both reached through `capabilities`, which is the one walk the
// develop panel, the clipboard and the sidecar already make — so
// an operation is undoable by virtue of being in the chain, with
// nothing to register (FR-DEV-3c).
ops: _,
framing: _,
masks,
film,
spots,
} = self;
EditState {
params: Preset::capture(self),
// A refcount bump. See `EditState::masks` for why that matters on
// a path called once a frame.
masks: Arc::clone(masks),
// The names travel; the tables do not. They are derived, and this
// crate cannot rebuild them — hence `FilmRebake`.
film: film.as_ref().map(|f| FilmRef {
stock: f.stock.clone(),
print: f.print.clone(),
}),
spots: spots.clone(),
}
}
/// TRACES: FR-DEV-5 | FR-CAT-8
/// Put `state` back, replacing whatever this graph held.
///
/// A *replacement*, not an overlay: a parameter absent from the state
/// means default, and a state with no mask blocks means an edit with no
/// local adjustments rather than an edit that keeps whatever was on
/// screen. That is the same rule [`Preset::apply`] and
/// [`crate::Version::apply`] keep, and for the same reason — "the
/// photograph is now as it was" is the whole claim the call makes.
///
/// The **viewport survives**, because [`Preset::apply`] preserves it. Zoom
/// says where the user is looking rather than what the picture is, and an
/// undo that refitted the frame would read as having navigated somewhere.
///
/// The pattern below is exhaustive for the reason [`Self::state`]'s is.
pub fn set_state(&mut self, state: &EditState) -> FilmRebake {
let EditState {
params,
masks,
film,
spots,
} = state;
// At full scope. `Scope` is a question about what a paste carries
// *between* photographs; this is one photograph's own edit being put
// back, so there is nothing to leave behind.
params.apply(self, Scope::Everything);
self.masks = Arc::clone(masks);
self.spots = spots.clone();
// Cleared either way, and when a stock is named the caller bakes it.
// Left standing, the tables now in the graph would be the ones baked
// from the film node's *previous* exposure sliders — and those sliders
// were just replaced, so the restored state would render through the
// film of the state it replaced. Clearing is the conservative half of
// that; `Wanted` is the half that gets it back.
self.set_film(None);
match film {
None => FilmRebake::NotNeeded,
Some(want) => FilmRebake::Wanted(want.clone()),
}
}
pub fn set_param(&mut self, op: OpId, param: ParamId, value: f32) {
if op == crate::framing::ID {
let Some(desc) = self.framing.descriptor().param(param) else {
@@ -383,7 +477,7 @@ impl EditGraph {
// Masks go too, and this is why `apply` can be a replacement rather
// than an overlay: a sidecar with no mask blocks means an edit with no
// local adjustments, not an edit that keeps whatever was on screen.
self.masks = MaskStack::new();
self.masks = Arc::new(MaskStack::new());
// The film goes too, for the reason the masks do. Restoring it is the
// *caller's* job rather than `Version::apply`'s: a sidecar names a
// stock, and turning a name into tables needs the profile database,
core/dr-pipeline/src/lib.rs
+2
View File
@@ -43,6 +43,7 @@ pub mod ops;
pub mod preset;
pub mod sidecar;
pub mod spot;
pub mod state;
pub use descriptor::{
Attribute, Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, ParamKind,
@@ -62,6 +63,7 @@ pub use operation::{
pub use preset::{Preset, Scope};
pub use sidecar::{Sidecar, Version};
pub use spot::{Spot, SpotMode, SpotSet};
pub use state::{EditState, FilmRebake, FilmRef};
#[cfg(test)]
mod tests {
core/dr-pipeline/src/sidecar.rs
+4 -8
View File
@@ -109,14 +109,10 @@ pub struct Sidecar {
/// TRACES: FR-DEV-3f
/// The stock and paper a version names, without the tables they bake to.
#[derive(Debug, Clone, PartialEq, Eq, Default)]
pub struct FilmRef {
pub stock: String,
/// The paper, if the negative is printed. Absent means the film is viewed
/// as it comes — which for a colour negative is the scan, orange and
/// inverted, and is a legitimate thing to ask for.
pub print: Option<String>,
}
///
/// Re-exported rather than defined here: a sidecar is one of the things an
/// edit is written to, not where an edit is defined. See [`crate::state`].
pub use crate::state::FilmRef;
/// TRACES: FR-CAT-12 | FR-NC-8
/// One named edit variant.
core/dr-pipeline/src/state.rs
+313
View File
@@ -0,0 +1,313 @@
//! TRACES: FR-DEV-5 | FR-CAT-8
//! The whole of an edit, gathered so that nothing can be left out of it.
//!
//! # Why this exists
//!
//! An edit used to be a bag of scalars, and [`Preset`] is that bag. It was a
//! true description while the graph held only operations and a framing, and it
//! stopped being true the moment a mask stack and a film stock arrived —
//! because a layer is not a scalar and a stock is not a scalar, which is
//! precisely why [`EditGraph`] holds both of them apart from `ops`.
//!
//! Nothing announced the change. What happened instead is that three places
//! each captured "the edit" and each captured a different subset of it:
//! [`Preset::capture`] took the parameters, `Version::from_graph` took the
//! parameters and the masks and the film, and `Version::update` took the
//! parameters and the masks and dropped the film on the floor. The visible
//! symptom was quieter still — the history snapshots a `Preset`, so drawing a
//! mask opened no undo step at all, and `record` returned `false` while the
//! interface went on calling it in good faith.
//!
//! # What keeps it complete
//!
//! Not vigilance, and not a checklist in a comment. [`EditGraph::state`]
//! destructures the graph **exhaustively** and [`EditGraph::set_state`]
//! destructures this type exhaustively — no `..` in either pattern, and this
//! type's fields are public so that every construction site is a struct
//! literal naming all of them. A fifth kind of state added to the graph does
//! not compile until somebody has decided whether an undo has to put it back.
//!
//! FR-DEV-8's spot removal was named here as the one already asked for, and it
//! arrived. It did not compile, which is the whole of what this was for: the
//! repairs are in [`Self::spots`] because the build refused to proceed without
//! an answer about them, rather than because anyone remembered to look.
//!
//! That is the whole mechanism. It is deliberately a compiler error rather
//! than a runtime check, because the failure being prevented is *silence*: the
//! mask bug produced no panic, no warning and no failing test, and only a
//! diagnostic that fires before the code runs at all could have caught it.
//!
//! # What is deliberately not in here
//!
//! - **The viewport.** Zoom and pan say where the photographer is looking, not
//! what the photograph becomes. An undo that also moved the view would read
//! as navigation — the rule [`Preset::apply`] already keeps for a paste.
//! - **The orientation baseline.** How the camera stored its rows is part of
//! reading the file, not a decision anyone made (see [`crate::framing`]).
//! - **The film tables.** Derived from the stock, the paper and the film
//! node's own exposure sliders, and re-baked in milliseconds. Turning a name
//! back into tables needs the profile database this crate does not link
//! (ARCH §6.5a), which is why [`FilmRebake`] exists.
//! - **The rating and the flag.** Judgements about the photograph rather than
//! edits to it; they change no pixel, and `sidecar::Version` keeps them for
//! that reason.
use crate::graph::EditGraph;
use crate::mask::MaskStack;
use crate::preset::Preset;
use crate::spot::SpotSet;
/// TRACES: FR-DEV-3f
/// The stock and paper an edit names, without the tables they bake to.
///
/// The **id, not an index**. Stocks are files that users add
/// (`core/dr-film/profiles`), so an index would mean installing a profile
/// silently changed which film every existing photograph was developed on.
#[derive(Debug, Clone, PartialEq, Eq, Default)]
pub struct FilmRef {
pub stock: String,
/// The paper, if the negative is printed. Absent means the film is viewed
/// as it comes — which for a colour negative is the scan, orange and
/// inverted, and is a legitimate thing to ask for.
pub print: Option<String>,
}
/// TRACES: FR-DEV-5 | FR-CAT-8
/// Everything about a photograph that an edit decides.
///
/// Fields are public on purpose: a struct literal is exhaustive, so every
/// place that builds one has to account for every part of an edit. See the
/// module note — that is the entire safety mechanism, and hiding these behind
/// a constructor with positional arguments would trade it for two `String`s
/// that can be passed in the wrong order.
#[derive(Debug, Clone, PartialEq, Default)]
pub struct EditState {
/// Every non-default parameter — the operations' and the framing's alike,
/// since the framing is an entry in [`EditGraph::capabilities`] like any
/// other.
pub params: Preset,
/// The local adjustments (FR-DEV-3).
///
/// Shared rather than owned, and that is a decision about the *drag path*
/// rather than about memory. `state` is called on every parameter change,
/// which during a slider drag is once a frame; a mask stack holding a
/// painted brush is thousands of stroke points, and deep-copying it sixty
/// times a second to record an exposure move that did not touch it would
/// be a cost paid for nothing. [`EditGraph::masks_mut`] is the single door
/// through which a stack is modified, and it clones on write, so snapshots
/// share until one of them is actually edited.
pub masks: std::sync::Arc<MaskStack>,
/// TRACES: FR-DEV-3f
/// The stock this edit develops on, named.
pub film: Option<FilmRef>,
/// TRACES: FR-DEV-8
/// The repairs (see [`crate::spot`]).
///
/// Owned rather than shared, where [`Self::masks`] is shared, and the
/// difference is a fact about the two rather than an inconsistency: a
/// repair is a handful of numbers and a photographer places tens of them,
/// while a painted mask is an unbounded path of stroke points. Copying the
/// first once a frame is nothing; copying the second is the reason
/// `masks_mut` clones on write.
pub spots: SpotSet,
}
/// TRACES: FR-DEV-3f
/// What a caller still owes the graph after its state was replaced.
///
/// `dr-pipeline` can hold a film but cannot bake one: the tables come from the
/// profile database, and this crate does not link it (ARCH §6.5a). So
/// restoring an edit that names a stock is necessarily two steps, and this is
/// the second one made impossible to forget rather than left in a comment.
#[derive(Debug, Clone, PartialEq)]
#[must_use = "an ignored rebake leaves the photograph rendering without its film"]
pub enum FilmRebake {
/// The graph's film is already right — either the state named none, in
/// which case it has been cleared, or there was nothing to restore.
NotNeeded,
/// Bake this stock and hand the result back through
/// [`EditGraph::set_film`].
Wanted(FilmRef),
}
impl FilmRebake {
/// The stock to bake, if one is owed.
pub fn wanted(&self) -> Option<&FilmRef> {
match self {
Self::NotNeeded => None,
Self::Wanted(film) => Some(film),
}
}
/// Assert that nothing is owed, for a caller that knows this edit names
/// no film — a test fixture, in practice.
///
/// It *checks* rather than discards, and that is the point of it existing
/// at all. `let _ = …` would make the same claim silently and go on making
/// it the day the fixture grows a stock, at which point the test would
/// pass while rendering the wrong picture — which is precisely the failure
/// this type was introduced to stop.
#[track_caller]
pub fn expect_no_film(self) {
if let Self::Wanted(film) = self {
panic!(
"this edit develops on {:?}, which has to be baked back before \
the photograph can be rendered",
film.stock
);
}
}
}
impl EditState {
/// The state `graph` is in. The same call as [`EditGraph::state`], for a
/// caller that reads better this way round.
pub fn capture(graph: &EditGraph) -> Self {
graph.state()
}
/// Whether this edit does anything to the photograph at all.
///
/// A neutral state is a *decision* rather than a missing one — applying it
/// returns the photograph to its defaults — so this answers "is there
/// anything to show", not "is there anything to store".
pub fn is_neutral(&self) -> bool {
self.params.is_empty()
&& self.masks.is_empty()
&& self.film.is_none()
&& self.spots.is_empty()
}
}
#[cfg(test)]
mod tests {
use super::*;
use crate::framing;
use crate::graph::Film;
use crate::mask::{MaskLayer, MaskSource};
use crate::ops::{curve, exposure, saturation};
use crate::spot::Spot;
use crate::CropRect;
/// A graph with one of *every* kind of state moved off its default: an
/// operation's parameter, a framing, a mask layer, a film, a repair. One
/// of each is the point — a round trip that only exercises the scalars is
/// the test that was already passing while the masks went missing.
fn thoroughly_edited() -> EditGraph {
let mut g = EditGraph::default_chain();
g.set_param(exposure::ID, exposure::EXPOSURE, 0.75);
g.set_param(saturation::ID, saturation::SATURATION, -30.0);
g.set_param(curve::ID, curve::P1_X, 0.3);
g.set_param(framing::ID, framing::ANGLE, 2.5);
g.set_crop(CropRect {
x: 0.1,
y: 0.1,
width: 0.6,
height: 0.6,
});
let mut layer = MaskLayer::new(
"l1",
MaskSource::Radial {
centre: (0.4, 0.6),
radii: (0.25, 0.3),
angle: 0.2,
feather: 0.15,
},
);
layer.opacity = 0.6;
layer.invert = true;
g.masks_mut().push(layer);
g.spots_mut()
.place(Spot::new((0.3, 0.7), (0.05, 0.0), 0.02))
.expect("the repair was placed");
g.set_film(Some(Film {
stock: "kodak_portra_400".into(),
print: Some("kodak_endura".into()),
tables: crate::ops::FilmTables {
exposure_matrix: [[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]],
curves: vec![[0.5, 0.5, 0.5]; crate::ops::film_sim::CURVE_SAMPLES],
curve_log_min: -3.0,
curve_log_max: 1.0,
lut: vec![[0.5, 0.5, 0.5]; 8],
density_max: 2.0,
lut_size: 2,
grain_particles: [0.0; 3],
grain_density_max: [2.0; 3],
grain_uniformity: 1.0,
},
}));
g
}
#[test]
fn an_edit_survives_being_taken_off_a_graph_and_put_back() {
// The property every undo rests on. Checked over the whole state
// rather than the values that were set, because the interesting
// failure is not a value coming back wrong — it is a *kind* of value
// that was never carried at all, and a narrower assertion is exactly
// what missed the masks.
let edited = thoroughly_edited();
let state = edited.state();
let mut fresh = EditGraph::default_chain();
let rebake = fresh.set_state(&state);
assert_eq!(
rebake,
FilmRebake::Wanted(FilmRef {
stock: "kodak_portra_400".into(),
print: Some("kodak_endura".into()),
}),
"the stock has to be asked for, since this crate cannot bake it"
);
// The film is the one part `set_state` deliberately does not restore,
// so it is put back the way a caller would before comparing.
fresh.set_film(edited.film().cloned());
assert_eq!(
fresh.state(),
state,
"something about the edit did not survive the round trip"
);
}
#[test]
fn putting_a_state_back_replaces_rather_than_overlays() {
// A part absent from the state means *default*, not "leave whatever
// was there". Otherwise undoing to the floor would leave the last
// mask standing, which is the shape of the bug this type exists for.
let mut g = EditGraph::default_chain();
let floor = g.state();
let edited = thoroughly_edited();
assert!(g.set_state(&edited.state()).wanted().is_some());
// A caller bakes at this point. Standing in for it here is what makes
// the assertion below mean anything: without a film on the graph,
// "the film was cleared" would be true of a graph that never had one.
g.set_film(edited.film().cloned());
assert!(g.film().is_some() && !g.masks().is_empty() && !g.spots().is_empty());
g.set_state(&floor).expect_no_film();
assert!(
g.masks().is_empty(),
"a layer outlived the state holding it"
);
assert!(g.film().is_none(), "the film outlived the state holding it");
assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(0.0));
assert!(g.crop().is_full(), "the crop outlived it: {:?}", g.crop());
assert_eq!(g.state(), floor);
}
#[test]
fn a_neutral_state_is_a_decision_rather_than_a_missing_one() {
assert!(EditGraph::default_chain().state().is_neutral());
assert!(!thoroughly_edited().state().is_neutral());
}
}