Let a photographer choose the film, and remember which one
The stock model rendered correctly and nothing could ask for it. This is the picker, and the sidecar key that makes the choice outlive the session. How the choice persists was the open question, and the answer was already written down twice in sidecar.rs: `rating` is a top-level key "because a rating is not an edit", and `masks` are one "because a layer is not a scalar". A stock is that kind of thing -- a choice of material, not a number a slider moves -- so it is a top-level key too. It stores the **id**, not an index. Stocks are files that users add, so an index would mean installing a profile silently changed which film every existing photograph had been developed on. A name this build has no profile for still round-trips untouched, because the alternative is that syncing to an older phone quietly un-develops the picture. Only the names travel. Turning one back into tables needs the profile database, which dr-pipeline deliberately does not link, so `Version::apply` clears the film and the session re-bakes -- after the parameters, because the bake reads the film's own exposure sliders and the print balance is solved against them. That is also why moving those sliders rebuilds the lookup where no other control in the panel does: an enlarger's filtration depends on how the negative was exposed. The panel keeps its rule. It still names no operation and still generates every control from a declared parameter kind; the stock gets a bespoke control beside those, exactly as the mask stack does, and for the same reason. The film's exposure and print exposure arrive as ordinary generated sliders. Two defaults worth stating. Picking a colour negative prints it, because an unprinted one is an orange strip and offering that as the first thing somebody sees after choosing Portra reads as a bug rather than as a choice -- the toggle is there for anyone who wants the scan. And a paste carries no film: a preset is a parameter map, and a stock is not a parameter, so pasting one would paste a choice the clipboard never took. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -1737,7 +1737,11 @@ mod tests {
|
||||
// largest shader the pipeline can actually generate, which is the one
|
||||
// with a film in it.
|
||||
let tables = film_test_tables();
|
||||
g.set_film_tables(Some(tables.clone()));
|
||||
g.set_film(Some(dr_pipeline::graph::Film {
|
||||
stock: "under_test".into(),
|
||||
print: None,
|
||||
tables: tables.clone(),
|
||||
}));
|
||||
pass.set_film(Some(&tables));
|
||||
|
||||
// Cropped, so the render is against an output size that is not the
|
||||
|
||||
@@ -68,6 +68,15 @@ fn tables(baked: &dr_film::Baked) -> FilmTables {
|
||||
}
|
||||
}
|
||||
|
||||
/// A name for the baked stock.
|
||||
///
|
||||
/// The id is not carried on `Baked` — it is the *recipe's*, and a bake is a
|
||||
/// pile of numbers. The tests here only need the graph to hold something, and
|
||||
/// what it holds is checked by the sidecar's own tests rather than by pixels.
|
||||
fn film_stock_of(_baked: &dr_film::Baked) -> &'static str {
|
||||
"under_test"
|
||||
}
|
||||
|
||||
/// Render a flat frame through a stock and return the centre pixel, 0..1.
|
||||
///
|
||||
/// The centre rather than a corner: a demosaic invents its edges, and the
|
||||
@@ -79,7 +88,11 @@ fn rendered(ctx: &GpuContext, level: u16, baked: &dr_film::Baked) -> [f32; 3] {
|
||||
.expect("demosaic");
|
||||
|
||||
let mut graph = EditGraph::default_chain();
|
||||
graph.set_film_tables(Some(tables(baked)));
|
||||
graph.set_film(Some(dr_pipeline::graph::Film {
|
||||
stock: film_stock_of(baked).to_string(),
|
||||
print: None,
|
||||
tables: tables(baked),
|
||||
}));
|
||||
let shader = graph.compose();
|
||||
|
||||
let mut adjust = AdjustPass::new(ctx);
|
||||
|
||||
@@ -93,6 +93,39 @@ pub struct EditGraph {
|
||||
/// list would make the list recursive and every consumer that walks it
|
||||
/// have to know that some entries are really sub-graphs.
|
||||
masks: MaskStack,
|
||||
/// TRACES: FR-DEV-3f
|
||||
/// The film stock this edit renders through, if any.
|
||||
///
|
||||
/// Apart from `ops` for the same reason `masks` is, and the reason
|
||||
/// [`crate::sidecar::Version::rating`] is a top-level key: a stock is not
|
||||
/// a scalar and not a slider. It is a *choice of material*, named by an
|
||||
/// id, from which the tables in `ops::film_sim` are derived.
|
||||
///
|
||||
/// Both halves live here together — the id that persists and the tables
|
||||
/// that render — because they are one fact, and holding them apart is how
|
||||
/// a sidecar comes to name one stock while the shader draws another.
|
||||
film: Option<Film>,
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3f
|
||||
/// A chosen stock, and what it bakes to.
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
pub struct Film {
|
||||
/// The stock's id, e.g. `kodak_portra_400`. **This is what persists.**
|
||||
///
|
||||
/// A name rather than an index into the stock list, because the list is
|
||||
/// data-driven: stocks are files, users add them, and an index would mean
|
||||
/// installing a profile silently changed which film every existing
|
||||
/// photograph was developed on.
|
||||
pub stock: String,
|
||||
/// The paper it is printed on, if it is printed. `None` views the film
|
||||
/// directly — right for a reversal stock, and for a negative it is the
|
||||
/// scan, orange mask and all.
|
||||
pub print: Option<String>,
|
||||
/// The baked tables. **Not persisted**: they are derived from the two ids
|
||||
/// above plus the node's own exposure parameters, and re-baking is
|
||||
/// milliseconds.
|
||||
pub tables: crate::ops::FilmTables,
|
||||
}
|
||||
|
||||
impl EditGraph {
|
||||
@@ -114,6 +147,7 @@ impl EditGraph {
|
||||
ops: ops::chain(),
|
||||
framing: Framing::new(),
|
||||
masks: MaskStack::new(),
|
||||
film: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -256,17 +290,24 @@ impl EditGraph {
|
||||
/// has to defend against an out-of-range value, and a corrupt sidecar
|
||||
/// cannot reach a shader.
|
||||
/// TRACES: FR-DEV-3f
|
||||
/// Load a baked film stock, or clear it.
|
||||
/// Develop on a film stock, or stop doing so.
|
||||
///
|
||||
/// One call for the id and the tables together, because they are one fact.
|
||||
/// Offered to every operation rather than to the one that wants it,
|
||||
/// because the graph holds `Box<dyn Operation>` and knowing which concrete
|
||||
/// type is which is exactly what it is organised not to know (ARCH §3.4).
|
||||
/// The default implementation ignores it, so this costs a virtual call per
|
||||
/// node on an action a user takes by hand.
|
||||
pub fn set_film_tables(&mut self, tables: Option<crate::ops::FilmTables>) {
|
||||
pub fn set_film(&mut self, film: Option<Film>) {
|
||||
for op in &mut self.ops {
|
||||
op.set_film_tables(tables.as_ref());
|
||||
op.set_film_tables(film.as_ref().map(|f| &f.tables));
|
||||
}
|
||||
self.film = film;
|
||||
}
|
||||
|
||||
/// The stock this edit is being developed on.
|
||||
pub fn film(&self) -> Option<&Film> {
|
||||
self.film.as_ref()
|
||||
}
|
||||
|
||||
pub fn set_param(&mut self, op: OpId, param: ParamId, value: f32) {
|
||||
@@ -322,6 +363,11 @@ impl EditGraph {
|
||||
// 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();
|
||||
// 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,
|
||||
// which this crate deliberately does not link (ARCH §6.5a).
|
||||
self.set_film(None);
|
||||
}
|
||||
|
||||
/// Set the crop rectangle. Clamped to keep it inside the frame.
|
||||
|
||||
@@ -108,14 +108,18 @@ mod tests {
|
||||
// it is deliberate rather than an oversight: a node that carries
|
||||
// measurements is a real second kind of node, and pretending otherwise
|
||||
// would mean silently leaving it out of every test that uses this.
|
||||
g.set_film_tables(Some(crate::ops::FilmTables {
|
||||
g.set_film(Some(crate::graph::Film {
|
||||
stock: "test_stock".into(),
|
||||
print: None,
|
||||
tables: crate::ops::FilmTables {
|
||||
exposure_matrix: [[5.0, 0.5, 0.2], [0.1, 5.0, 0.3], [0.2, 0.5, 4.0]],
|
||||
curves: vec![[0.5, 0.5, 0.5]; crate::ops::film_sim::CURVE_SAMPLES],
|
||||
curve_log_min: -3.0,
|
||||
curve_log_max: 4.0,
|
||||
lut: vec![[0.5, 0.5, 0.5]; 32 * 32 * 32],
|
||||
density_max: 3.0,
|
||||
lut_size: 32,
|
||||
density_max: 3.0,
|
||||
lut_size: 32,
|
||||
},
|
||||
}));
|
||||
g
|
||||
}
|
||||
|
||||
@@ -75,8 +75,15 @@ static DESCRIPTOR: OpDescriptor = OpDescriptor {
|
||||
/// - `exposure_matrix[l][c]` — layer `l`'s response to linear sRGB channel `c`.
|
||||
/// - `curves` — `CURVE_SAMPLES` density triples, uniform over
|
||||
/// `[curve_log_min, curve_log_max]`.
|
||||
/// - `lut` — `lut_size³` linear sRGB triples in x-major order, uniform over
|
||||
/// `[0, density_max]` on each axis.
|
||||
/// - `lut` — `lut_size³` linear sRGB triples, uniform over `[0, density_max]`
|
||||
/// on each axis, with the **red axis varying fastest**: index
|
||||
/// `(b * size + g) * size + r`. That is the order a 3D texture upload
|
||||
/// expects, so the consumer hands the slice straight to the driver. Filling
|
||||
/// it the other way round transposes red and blue in the finished picture —
|
||||
/// which is a plausible photograph of the wrong colour, and which the unit
|
||||
/// tests on both sides of this seam happily pass, because each side is
|
||||
/// internally consistent. `dr-film` pins it; `dr-gpu`'s `film_sim` test
|
||||
/// catches it end to end.
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
pub struct FilmTables {
|
||||
pub exposure_matrix: [[f32; 3]; 3],
|
||||
|
||||
@@ -106,6 +106,17 @@ pub struct Sidecar {
|
||||
unknown_blocks: Vec<String>,
|
||||
}
|
||||
|
||||
/// 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>,
|
||||
}
|
||||
|
||||
/// TRACES: FR-CAT-12 | FR-NC-8
|
||||
/// One named edit variant.
|
||||
#[derive(Debug, Clone, PartialEq, Default)]
|
||||
@@ -155,6 +166,22 @@ pub struct Version {
|
||||
/// per-field merge under FR-NC-9 is most of the reason the ids are stored
|
||||
/// as ids at all.
|
||||
pub masks: MaskStack,
|
||||
/// TRACES: FR-DEV-3f
|
||||
/// The film stock this version develops on, named by id.
|
||||
///
|
||||
/// A top-level key rather than an `op.param` line, for the reason
|
||||
/// [`Self::rating`] is one and [`Self::masks`] are: a stock is not a
|
||||
/// scalar. It is a choice of material, and the numbers that render it are
|
||||
/// derived from the choice rather than being the choice.
|
||||
///
|
||||
/// 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.
|
||||
///
|
||||
/// Only the names travel. Turning them back into tables needs the profile
|
||||
/// database, which this crate does not link, so [`Self::apply`] leaves the
|
||||
/// graph's film cleared and the caller re-bakes — see `EditGraph::set_film`.
|
||||
pub film: Option<FilmRef>,
|
||||
/// Keys this build did not recognise, kept verbatim.
|
||||
///
|
||||
/// An operation this build lacks would otherwise be deleted the moment an
|
||||
@@ -181,6 +208,10 @@ impl Version {
|
||||
flag: 0,
|
||||
params: capture(graph),
|
||||
masks: graph.masks().clone(),
|
||||
film: graph.film().map(|f| FilmRef {
|
||||
stock: f.stock.clone(),
|
||||
print: f.print.clone(),
|
||||
}),
|
||||
unknown: BTreeMap::new(),
|
||||
}
|
||||
}
|
||||
@@ -381,6 +412,21 @@ impl Version {
|
||||
self.rating = merge_judgement(self.rating, remote.rating, remote_wins);
|
||||
self.flag = merge_judgement(self.flag, remote.flag, remote_wins);
|
||||
|
||||
// TRACES: FR-DEV-3f
|
||||
// The film resolves wholesale to the higher revision, like a mask
|
||||
// layer and unlike a parameter. It is one decision with two names in
|
||||
// it: taking the stock from one device and the paper from the other
|
||||
// would print a negative on a paper nobody chose it for, which is a
|
||||
// combination neither photographer asked for and which renders as a
|
||||
// colour cast rather than as an obvious mistake.
|
||||
//
|
||||
// Unlike a rating, a cleared film *is* an edit — "develop this
|
||||
// normally again" — so `None` propagates where a zero rating does not.
|
||||
// The revision is what says whether it was cleared or never set.
|
||||
if remote_wins {
|
||||
self.film = remote.film.clone();
|
||||
}
|
||||
|
||||
// Masks merge by layer id, which is the same disjoint-survives rule
|
||||
// the parameters follow one level up: a layer added on the phone and
|
||||
// a layer added on the desktop are different ids, so both survive and
|
||||
@@ -506,6 +552,15 @@ impl Sidecar {
|
||||
if v.flag > 0 {
|
||||
let _ = writeln!(out, "flag = {}", v.flag);
|
||||
}
|
||||
// TRACES: FR-DEV-3f
|
||||
// Before the parameters, because it decides what they mean: the
|
||||
// film's exposure slider is a slider on *that stock's* curve.
|
||||
if let Some(film) = &v.film {
|
||||
let _ = writeln!(out, "film = {}", film.stock);
|
||||
if let Some(print) = &film.print {
|
||||
let _ = writeln!(out, "film_print = {print}");
|
||||
}
|
||||
}
|
||||
for ((op, param), value) in &v.params {
|
||||
let _ = writeln!(out, "{op}.{param} = {}", format_value(*value));
|
||||
}
|
||||
@@ -620,6 +675,21 @@ impl Sidecar {
|
||||
// out-of-range rating would sort above five stars forever and
|
||||
// no filter would reach it.
|
||||
"rating" => version.rating = value.parse::<u8>().unwrap_or(0).min(MAX_RATING),
|
||||
// TRACES: FR-DEV-3f
|
||||
// Not validated against the installed stocks here: this crate
|
||||
// does not link them, and a file naming a stock this device
|
||||
// lacks must round-trip unharmed rather than be silently
|
||||
// dropped. Whoever bakes it reports the miss.
|
||||
"film" => {
|
||||
version.film.get_or_insert_with(FilmRef::default).stock = value.to_string();
|
||||
}
|
||||
"film_print" => {
|
||||
// `get_or_insert` and not a plain field write: key order in
|
||||
// a hand-edited file is not guaranteed, and a paper line
|
||||
// above its film line must not be thrown away.
|
||||
version.film.get_or_insert_with(FilmRef::default).print =
|
||||
Some(value.to_string());
|
||||
}
|
||||
"flag" => version.flag = value.parse::<u8>().unwrap_or(0).min(MAX_FLAG),
|
||||
_ => match key.split_once('.') {
|
||||
// An `op.param` line whose value does not parse is a
|
||||
@@ -1572,6 +1642,128 @@ mod tests {
|
||||
assert!(local.revision > 9, "a merged result must not look stale");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_film_choice_survives_the_round_trip() {
|
||||
// TRACES: FR-DEV-3f
|
||||
let mut v = version_of(&edited());
|
||||
v.film = Some(FilmRef {
|
||||
stock: "kodak_portra_400".into(),
|
||||
print: Some("kodak_portra_endura".into()),
|
||||
});
|
||||
let mut side = Sidecar::default();
|
||||
side.versions.insert(v.uuid.clone(), v);
|
||||
|
||||
let text = side.to_text();
|
||||
let back = Sidecar::parse(&text).expect("parses");
|
||||
let film = back.versions.values().next().unwrap().film.clone().unwrap();
|
||||
assert_eq!(film.stock, "kodak_portra_400");
|
||||
assert_eq!(film.print.as_deref(), Some("kodak_portra_endura"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_film_with_no_print_round_trips_as_a_scan() {
|
||||
// Absent paper is a *choice* — the film as it comes, which for a
|
||||
// colour negative is the orange scan. It must not come back as the
|
||||
// stock's default paper, or "show me the negative" would be
|
||||
// unrepresentable.
|
||||
let mut v = version_of(&edited());
|
||||
v.film = Some(FilmRef { stock: "kodak_portra_400".into(), print: None });
|
||||
let mut side = Sidecar::default();
|
||||
side.versions.insert(v.uuid.clone(), v);
|
||||
|
||||
let back = Sidecar::parse(&side.to_text()).expect("parses");
|
||||
let film = back.versions.values().next().unwrap().film.clone().unwrap();
|
||||
assert_eq!(film.stock, "kodak_portra_400");
|
||||
assert_eq!(film.print, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn no_film_writes_no_film_line() {
|
||||
// The same non-default rule the parameters and the rating follow: a
|
||||
// library nobody has put on film does not grow a line per file.
|
||||
let mut side = Sidecar::default();
|
||||
let v = version_of(&edited());
|
||||
side.versions.insert(v.uuid.clone(), v);
|
||||
let text = side.to_text();
|
||||
assert!(!text.contains("film"), "{text}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_paper_line_above_its_film_line_is_not_lost() {
|
||||
// Key order in a hand-edited file is not guaranteed, and the reader
|
||||
// builds the film from two separate lines.
|
||||
let text = "drsc 1\n\n[version u1]\nname = Default\nrevision = 1\nmodified = 0\n\
|
||||
film_print = kodak_portra_endura\nfilm = kodak_portra_400\n";
|
||||
let side = Sidecar::parse(text).expect("parses");
|
||||
let film = side.versions.values().next().unwrap().film.clone().unwrap();
|
||||
assert_eq!(film.stock, "kodak_portra_400");
|
||||
assert_eq!(film.print.as_deref(), Some("kodak_portra_endura"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_stock_this_build_does_not_have_still_round_trips() {
|
||||
// A profile is a file a user can add. A device without it must hand
|
||||
// the name back untouched rather than drop it, or syncing to an older
|
||||
// phone would quietly un-develop the photograph.
|
||||
let text = "drsc 1\n\n[version u1]\nname = Default\nrevision = 1\nmodified = 0\n\
|
||||
film = ilford_hp5_plus\n";
|
||||
let side = Sidecar::parse(text).expect("parses");
|
||||
assert!(side.to_text().contains("film = ilford_hp5_plus"), "{}", side.to_text());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_film_resolves_wholesale_rather_than_field_by_field() {
|
||||
// TRACES: FR-DEV-3f | FR-NC-9
|
||||
// One decision with two names in it. Taking the stock from one device
|
||||
// and the paper from the other would print a negative on a paper
|
||||
// nobody chose for it -- a combination neither photographer asked for,
|
||||
// and one that renders as a colour cast rather than as an obvious
|
||||
// mistake.
|
||||
let mut base = version_of(&edited());
|
||||
base.revision = 1;
|
||||
|
||||
let mut local = base.clone();
|
||||
local.film = Some(FilmRef {
|
||||
stock: "kodak_portra_400".into(),
|
||||
print: Some("kodak_portra_endura".into()),
|
||||
});
|
||||
local.revision = 2;
|
||||
|
||||
let mut remote = base.clone();
|
||||
remote.film = Some(FilmRef { stock: "kodak_kodachrome_64".into(), print: None });
|
||||
remote.revision = 9;
|
||||
|
||||
local.merge(&remote, Some(&base));
|
||||
|
||||
let film = local.film.clone().expect("a film survived");
|
||||
assert_eq!(film.stock, "kodak_kodachrome_64");
|
||||
assert_eq!(
|
||||
film.print, None,
|
||||
"the loser's paper was grafted onto the winner's stock"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn clearing_the_film_elsewhere_propagates() {
|
||||
// Unlike a rating, a cleared film is an edit -- "develop this normally
|
||||
// again" -- so None must travel. A rating's zero does not, because
|
||||
// there "unset" and "set to zero" are indistinguishable; here the
|
||||
// revision says which happened.
|
||||
let mut base = version_of(&edited());
|
||||
base.film = Some(FilmRef { stock: "kodak_portra_400".into(), print: None });
|
||||
base.revision = 1;
|
||||
|
||||
let mut local = base.clone();
|
||||
local.revision = 2;
|
||||
|
||||
let mut remote = base.clone();
|
||||
remote.film = None;
|
||||
remote.revision = 9;
|
||||
|
||||
local.merge(&remote, Some(&base));
|
||||
assert_eq!(local.film, None, "a deliberate clear did not propagate");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_rating_survives_the_round_trip() {
|
||||
// The durability requirement: the catalog is disposable (ARCH §6.12),
|
||||
|
||||
Reference in New Issue
Block a user