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

Keep the film stock when an edit is saved

Browse Source 
`Version::update` is the write path an automatic save goes through. It copied
the parameters and the masks and said nothing about the film, so a photograph
developed on a stock was written back without it and opened the next time
without its emulsion. Nothing reported a failure — the line was simply not
there.

It is the third of the three routines that captured "the edit" and the only
one that got it wrong, which is the argument for not having three. All of them
now destructure one `EditState`, so `from_graph`, `update` and `apply` cannot
disagree about what an edit consists of, and the next part of one cannot be
lost by anybody writing a line too few.

Two tests, both of which fail without the fix: the field survives `update`,
and the stock survives the round trip through the file. `apply` returns the
`FilmRebake` it always implicitly owed, so `apply_version` now reads the debt
off the call rather than off `version.film` — and pays it in both directions,
since a version with no film has to clear the adjust pass too or it keeps
textures bound that nothing will sample.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-26 22:53:00 +02:00
co-authored by Claude Opus 5
parent 5622a58ce3
commit 93efdf27a6
7 changed files with 222 additions and 86 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-pipeline/src/sidecar.rs
+173 -61
View File
@@ -68,8 +68,9 @@ use std::fmt::Write as _;
use crate::graph::EditGraph;
use crate::mask::{Falloff, MaskLayer, MaskSource, MaskStack, Morphology, Stroke, DEFAULT_FEATHER};
use crate::preset::{resolve, Preset};
use crate::preset::Preset;
use crate::spot::{Spot, SpotMode, SpotSet};
use crate::state::{EditState, FilmRebake};
/// Format version of the document itself.
///
@@ -197,8 +198,22 @@ pub struct Version {
}
impl Version {
/// A new version holding the non-default parameters of `graph`.
/// A new version holding everything `graph`'s edit consists of.
///
/// The destructuring is exhaustive on purpose — see [`crate::state`]. A
/// new part of an edit must not reach the file only by somebody
/// remembering to add a line here, which is how [`Self::update`] came to
/// write the masks and forget the film.
pub fn from_graph(uuid: impl Into<String>, name: impl Into<String>, graph: &EditGraph) -> Self {
let EditState {
params,
masks,
film,
spots,
} = graph.state();
let params = params.into_params();
let masks = (*masks).clone();
Self {
uuid: uuid.into(),
name: name.into(),
@@ -212,42 +227,40 @@ impl Version {
// it in the "not yet looked at" state a cull resumes from.
rating: 0,
flag: 0,
params: capture(graph),
masks: graph.masks().clone(),
film: graph.film().map(|f| FilmRef {
stock: f.stock.clone(),
print: f.print.clone(),
}),
spots: graph.spots().clone(),
params,
masks,
film,
spots,
unknown: BTreeMap::new(),
}
}
/// Apply this version's parameters to a graph.
/// Apply this version's edit to a graph, returning the film it still owes.
///
/// The graph is reset first, so loading is a *replacement* rather than an
/// overlay: a parameter absent from the file means default, and would
/// otherwise silently inherit whatever the graph happened to hold.
/// Loading is a *replacement* rather than an overlay: a parameter absent
/// from the file means default, and would otherwise silently inherit
/// whatever the graph happened to hold.
///
/// Unknown operations and parameters are skipped with a warning by
/// [`EditGraph::set_param`], and values are clamped there, so a corrupt
/// or newer file cannot reach a shader.
pub fn apply(&self, graph: &mut EditGraph) {
///
/// The [`FilmRebake`] is not a new obligation — restoring a stock always
/// needed the profile database this crate does not link (ARCH §6.5a), and
/// callers were already doing it from a comment. It is the same debt made
/// impossible to walk past.
pub fn apply(&self, graph: &mut EditGraph) -> FilmRebake {
// Reset first for the *viewport's* sake, and only that: `set_state`
// deliberately preserves the view so an undo does not read as
// navigation, whereas opening a photograph should show it fitted
// rather than at the zoom the previous one was inspected at.
graph.reset();
for ((op, param), value) in &self.params {
// `OpId` and `ParamId` hold `&'static str` because descriptors
// are statics, and a sidecar's strings are not. `resolve` matches
// the file's names against the descriptors and hands back the
// static ids, so no string read from disk is ever leaked to get
// a lifetime it did not earn.
let Some((op, param)) = resolve(graph, op, param) else {
log::warn!("sidecar: unknown parameter {op}.{param}; ignoring");
continue;
};
graph.set_param(op, param, *value);
}
*graph.masks_mut() = self.masks.clone();
*graph.spots_mut() = self.spots.clone();
graph.set_state(&EditState {
params: Preset::from_params(self.params.clone()),
masks: std::sync::Arc::new(self.masks.clone()),
film: self.film.clone(),
spots: self.spots.clone(),
})
}
/// Record `graph` into this version, bumping the revision.
@@ -256,9 +269,22 @@ impl Version {
/// setter: FR-NC-9 resolves conflicts by revision, so a local edit that
/// did not bump it is a local edit a remote one will silently win.
pub fn update(&mut self, graph: &EditGraph, device: &str, now: i64) {
self.params = capture(graph);
self.masks = graph.masks().clone();
self.spots = graph.spots().clone();
// Exhaustive, and this is the call site that proves why it has to be:
// this method wrote the parameters, the masks and the repairs and
// silently dropped the film, so saving an edit developed on a stock
// lost the stock. Nothing here can be forgotten now without failing to
// compile.
let EditState {
params,
masks,
film,
spots,
} = graph.state();
self.params = params.into_params();
self.masks = (*masks).clone();
self.film = film;
self.spots = spots;
self.revision = self.revision.saturating_add(1);
self.device = device.to_string();
self.modified = now;
@@ -583,20 +609,6 @@ fn merge_judgement(ours: u8, theirs: u8, remote_wins: bool) -> u8 {
}
}
/// Every non-default parameter in the graph, keyed by `(op, param)`.
///
/// Reads [`EditGraph::capabilities`] — the same list the UI builds controls
/// from — so an operation is persisted by virtue of being in the chain, with
/// nothing to register and nothing to forget.
///
/// Delegated to [`Preset::capture`] rather than reimplemented: a version's
/// parameters and a copied preset are the same values taken from the same
/// list, and two routines building the same map would be two places for the
/// non-default rule to drift.
fn capture(graph: &EditGraph) -> BTreeMap<(String, String), f32> {
Preset::capture(graph).into_params()
}
impl Sidecar {
pub fn new() -> Self {
Self::default()
@@ -1357,6 +1369,81 @@ mod tests {
Version::from_graph("uuid-1", "Default", graph)
}
/// A graph developing on a stock, with tables well-formed enough for the
/// film node to keep them. The emulsion is invented; what is under test
/// is whether the *choice* reaches the file.
fn on_film(stock: &str) -> EditGraph {
let mut g = edited();
g.set_film(Some(crate::graph::Film {
stock: stock.to_string(),
print: None,
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 saving_an_edit_keeps_the_film_it_was_developed_on() {
// `update` is the write path — the one an automatic save goes through
// — and it used to copy the parameters and the masks and say nothing
// about the film. A photograph developed on a stock was written back
// without it, so the next time it opened, the emulsion was gone and
// nothing had reported a failure.
//
// It reads as an oversight because it was one, and that is the point:
// three routines captured "the edit" and each captured a different
// subset. All three now destructure one `EditState`, so the next part
// of an edit cannot be forgotten by anybody writing a line too few.
let mut v = version_of(&on_film("kodak_portra_400"));
v.film = None;
v.update(&on_film("kodak_portra_400"), "device-a", 1000);
assert_eq!(
v.film,
Some(FilmRef {
stock: "kodak_portra_400".into(),
print: None,
}),
"the stock did not survive the save"
);
}
#[test]
fn a_film_written_by_update_comes_back_off_the_disk() {
// End to end, because the field being set is only half of it: the
// stock has to reach the text and parse back out of it.
let mut v = version_of(&EditGraph::default_chain());
v.update(&on_film("ilford_hp5"), "device-a", 1000);
let mut sidecar = Sidecar::new();
sidecar.put(v);
let parsed = Sidecar::parse(&sidecar.to_text()).expect("re-read");
let mut graph = EditGraph::default_chain();
let rebake = parsed
.default_version()
.expect("a version")
.apply(&mut graph);
assert_eq!(
rebake.wanted().map(|f| f.stock.as_str()),
Some("ilford_hp5"),
"reopening the photograph has to ask for its stock back"
);
}
#[test]
fn only_non_default_values_are_written() {
// The property the whole format rests on: a neutral operation is
@@ -1386,7 +1473,8 @@ mod tests {
parsed
.default_version()
.expect("a version")
.apply(&mut restored);
.apply(&mut restored)
.expect_no_film();
assert_eq!(restored.param(exposure::ID, exposure::EXPOSURE), Some(0.75));
assert_eq!(
@@ -1420,7 +1508,8 @@ mod tests {
parsed
.default_version()
.expect("a version")
.apply(&mut restored);
.apply(&mut restored)
.expect_no_film();
for cap in g.capabilities() {
for p in &cap.params {
@@ -1457,7 +1546,8 @@ mod tests {
parsed
.default_version()
.expect("a version")
.apply(&mut restored);
.apply(&mut restored)
.expect_no_film();
assert_eq!(restored.crop(), g.crop());
assert_eq!(restored.param(framing::ID, framing::ANGLE), Some(-1.5));
@@ -1497,7 +1587,8 @@ mod tests {
.expect("valid")
.default_version()
.expect("a version")
.apply(&mut sideways);
.apply(&mut sideways)
.expect_no_film();
assert_eq!(
sideways.framing().baseline(),
@@ -1515,7 +1606,11 @@ mod tests {
let parsed = Sidecar::parse(&sidecar.to_text()).expect("valid");
let mut g = edited();
parsed.default_version().expect("a version").apply(&mut g);
parsed
.default_version()
.expect("a version")
.apply(&mut g)
.expect_no_film();
assert!(g.is_neutral(), "a neutral version must clear the graph");
}
@@ -1542,7 +1637,11 @@ mod tests {
tone_curve.p1_y = 0.15\ntone_curve.p3_y = 0.85\n";
let parsed = Sidecar::parse(text).expect("valid");
let mut g = EditGraph::default_chain();
parsed.default_version().expect("a version").apply(&mut g);
parsed
.default_version()
.expect("a version")
.apply(&mut g)
.expect_no_film();
// The S-curve the file describes, on the master curve and nowhere
// else.
@@ -1609,7 +1708,8 @@ mod tests {
parsed
.default_version()
.expect("a version")
.apply(&mut restored);
.apply(&mut restored)
.expect_no_film();
assert_eq!(restored.param(curve::ID, blue), Some(0.08));
assert_eq!(restored.param(curve::ID, red), Some(0.92));
@@ -1638,7 +1738,11 @@ mod tests {
time_machine.year = 1994\n";
let parsed = Sidecar::parse(text).expect("valid");
let mut g = EditGraph::default_chain();
parsed.default_version().expect("a version").apply(&mut g);
parsed
.default_version()
.expect("a version")
.apply(&mut g)
.expect_no_film();
assert!(g.is_neutral());
}
@@ -1648,7 +1752,11 @@ mod tests {
exposure.exposure = NaN\nwhite_balance.temperature = 20\n";
let parsed = Sidecar::parse(text).expect("valid");
let mut g = EditGraph::default_chain();
parsed.default_version().expect("a version").apply(&mut g);
parsed
.default_version()
.expect("a version")
.apply(&mut g)
.expect_no_film();
assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(0.0));
assert_eq!(
@@ -1665,7 +1773,11 @@ mod tests {
exposure.exposure = 99\n";
let parsed = Sidecar::parse(text).expect("valid");
let mut g = EditGraph::default_chain();
parsed.default_version().expect("a version").apply(&mut g);
parsed
.default_version()
.expect("a version")
.apply(&mut g)
.expect_no_film();
assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(5.0));
}
@@ -1699,7 +1811,7 @@ mod tests {
assert_eq!(parsed.default_version().expect("default").name, "Colour");
let mut g = EditGraph::default_chain();
parsed.versions["u2"].apply(&mut g);
parsed.versions["u2"].apply(&mut g).expect_no_film();
assert_eq!(
g.param(saturation::ID, saturation::SATURATION),
Some(-100.0)
@@ -1744,7 +1856,7 @@ mod tests {
assert!(conflicts.is_empty(), "disjoint edits must not conflict");
let mut g = EditGraph::default_chain();
local.apply(&mut g);
local.apply(&mut g).expect_no_film();
assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(1.0));
assert_eq!(g.crop().width, 0.5);
}
@@ -1768,7 +1880,7 @@ mod tests {
assert_eq!(conflicts.len(), 1, "the same parameter, two values");
let mut g = EditGraph::default_chain();
local.apply(&mut g);
local.apply(&mut g).expect_no_film();
assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(2.0));
}
@@ -1793,7 +1905,7 @@ mod tests {
local.merge(&remote, Some(&base));
let mut g = EditGraph::default_chain();
local.apply(&mut g);
local.apply(&mut g).expect_no_film();
assert_eq!(
g.param(exposure::ID, exposure::EXPOSURE),
Some(1.0),
@@ -1815,7 +1927,7 @@ mod tests {
local.merge(&remote, Some(&base));
let mut g = EditGraph::default_chain();
local.apply(&mut g);
local.apply(&mut g).expect_no_film();
assert!(g.is_neutral(), "the remote reset must survive the merge");
}
@@ -2124,7 +2236,7 @@ mod tests {
assert_eq!(local.rating, 4, "the tablet's cull arrived");
let mut g = EditGraph::default_chain();
local.apply(&mut g);
local.apply(&mut g).expect_no_film();
assert_eq!(
g.param(exposure::ID, exposure::EXPOSURE),
Some(1.5),