From 4b506488702661809e755c83b93fdec9ea4615a7 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Wed, 26 Aug 2026 15:24:19 +0200 Subject: [PATCH] Rebuild the film tables when the film's own sliders move Push and print exposure did nothing at all. The parameter was set and the tables were never rebuilt, so the shader went on running the stock as it had been baked. The check asked the wrong list: self.rows() // one entry per *parameter*, tab-filtered .get(op_index as usize) // indexed by an *operation* index .map(|_| ()) .and( ...the real check... ) `op_index` counts over the scoped capabilities -- it is what `lookup` resolves a slider through -- so capabilities is the only list to ask. Indexing `rows()` served no purpose, and past its end the `and` short-circuited to None and the rebake silently never happened. Both halves of that are worth saying. It was wrong, and it was convoluted, and the convolution is what hid the wrongness: a one-line check would have been obviously right or obviously broken. This is a class of bug the suite cannot reach. The tables are rebuilt in the interface layer, in response to a control, and every test either side of it passed throughout -- dr-film computed the pushed curves correctly and the shader rendered whatever it was handed. Only moving the slider showed it. Co-Authored-By: Claude Opus 5 --- ui/dr-ui/src/develop.rs | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index b8109f9..c574993 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -2449,35 +2449,28 @@ impl DevelopSession { .map(|f| (f.stock.as_str(), f.print.is_some())) } - /// Re-bake if `op` is the film, and do nothing otherwise. + /// Re-bake if `op_index` names the film, and do nothing otherwise. /// - /// The test is here rather than at the call site so that the callback in + /// `op_index` counts over [`Self::scoped_capabilities`] โ€” the same list + /// [`Self::lookup`] resolves a slider through โ€” so that is the only list to + /// ask. An earlier version also indexed `rows()`, which is one entry per + /// *parameter* and filtered by the active tab: past its end the check + /// short-circuited, the tables were never rebuilt, and the film's own + /// sliders moved nothing at all. + /// + /// The test is here rather than at the call site so the callback in /// `lib.rs` goes on naming no operation, which is the rule the whole panel /// is built on (ARCH ยง4.3a). - pub fn rebake_film_if_affected(&mut self, op: i32) { - let is_film = self - .rows() - .get(op.max(0) as usize) - .map(|_| ()) - .and( - self.scoped_capabilities() - .get(op.max(0) as usize) - .map(|c| c.id == dr_pipeline::ops::film_sim::ID), - ) - .unwrap_or(false); + pub fn rebake_film_if_affected(&mut self, op_index: i32) { + let is_film = usize::try_from(op_index) + .ok() + .and_then(|i| self.scoped_capabilities().get(i).map(|c| c.id)) + .is_some_and(|id| id == dr_pipeline::ops::film_sim::ID); if is_film { self.rebake_film(); } } - /// Re-bake the current stock. - /// - /// Called when the film's own exposure sliders move: those ride *inside* - /// the baked tables rather than arriving as uniforms, because the print - /// balance is solved against them โ€” an enlarger's filtration depends on - /// how the negative was exposed. So a slider drag has to rebuild the - /// lookup, which is why the bake is measured in milliseconds and not in - /// frames. /// The stock, the paper, and how far it was developed. /// /// Push rides with the other two through every path that re-bakes, because