96 Commits

Include renames

Author	SHA1	Message	Date
dtourolle	328fda6f7c	Merge: mask a whole category, not just one instance ... Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> # Conflicts: # apps/darkroom-desktop/Cargo.toml # docs/traceability.md	2026-08-30 16:37:31 +02:00
dtourolleandClaude Opus 5	485297f0c6	Let a mask cover a whole category, not just one instance ... `MaskSource` could say "instance 3 of that segmentation run" but had no way to say "the sky". Adding `Category { signature, name }` beside `Subject` is what lets a local adjustment attach to a semantic category at all. Stored as identity like a subject, and for the same reason: the coverage is megabytes and is reproducible by running the same model over the same image, so the sidecar carries what finds it again and the session carries pixels. ## A name rather than an index An index would be smaller and would match `Subject`. It would also be a bug. The grouping lives in `models/scene/categories.txt`, which is editable by design — adding one category to it renumbers every category after it, and every stored layer would silently start grading something else. A name that no longer exists is simply not found and the layer reads as stale, which is the failure that announces itself. Staleness is otherwise identical to a subject's: the coverage buffer is in the session, never the sidecar, so a signature from another run points at pixels that were never computed. ## Two tests, and the second one caught a real shape Round-tripping the name matters more than usual here, because the whole argument for storing a name instead of an index is worthless if the sidecar is what drops it. The multi-word case is the one worth having: `category = swimming pool` is written on one line, and a reader splitting on whitespace would have truncated it to a category no model has — a layer that silently masks nothing. `category` is also its own key rather than a reuse of `class`, because a file conflating them would round-trip a subject into a category. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-30 14:02:08 +02:00
dtourolleandClaude Opus 5	99563b33f7	Record the one clause of FR-PLG-8 that is built, and say what it is short of ... FR-PLG-8 reads as unbuilt, and most of it is: no aggregated missing-plugin notice, no catalog-wide view, no export gate, no mark on an image whose operation is unavailable. But its opening sentence is a claim about existing behaviour — "the sidecar already preserves lines it does not understand verbatim and writes them back untouched" — and it goes on to say that property "is now load-bearing and shall be treated as such". Two tests treat it as such, and neither was tagged. `sidecar.rs` keeps an unknown operation across a parse-and-write, and keeps it out of the edit graph so that preserving it is safe rather than merely tidy. Both would fail if the verbatim path were removed, which is what CONTRIBUTING.md asks a tag to mean. The tag sits on the two tests rather than on the module, so the matrix points at the clause that is closed rather than at the requirement as a whole. It is still short of the requirement's own acceptance criterion, and the tag comment says so: FR-PLG-8 asks that a sidecar written with a plugin, opened and saved without it, be byte-identical to the original, and the test asserts `contains`. Both halves exist separately — `writing_the_same_state_twice_is_byte_identical` proves byte identity for content this build understands — and nothing joins them into the single claim the requirement makes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-30 10:20:22 +02:00
dtourolle	91fe6cf300	Merge master into partial-preset-scope ... # Conflicts: # docs/traceability.md # ui/dr-ui/ui/app.slint	2026-08-29 22:02:03 +02:00
dtourolleandClaude Opus 5	754ab91347	Bring a Lightroom library across, and start with something in the list ... Two halves of the same complaint: a preset sheet that opens on "No presets yet" is homework, and a photographer with ten years of presets in Lightroom has no way to bring them. `dr-preset-xmp` reads Camera Raw `.xmp`. The mapping turned out to be mostly a rename rather than a conversion, because Adobe and this pipeline already agree: exposure is in stops in both, and contrast, the four recovery controls, clarity, texture, vibrance and saturation are all ±100 in both. That is not imitation, it is the convention raw developers converged on — `highlights_shadows.yaml` cites it in as many words. Only sharpening needed arithmetic, Adobe's 0…150 against our 0…100. The white balance does not come across, and says so rather than guessing. Adobe writes absolute Kelvin for a raw file where ours is a relative nudge from what the camera recorded, so converting needs the *target image's* as-shot white balance — exactly what a preset cannot carry, since the same preset lands on a frame shot at 3200K and one shot at 7000K. A guess would be wrong on most images and invisibly so. A folder is read as readily as a file, nested, because that is the shape an exported preset folder is in and importing ninety files one at a time is asking someone not to bother. `dr_pipeline::starter` is six presets a first run begins with, written against this pipeline in its units and deliberately mild — a starting point, not a caricature. They are seeded when the library *file* does not exist rather than when the library is empty, so deleting all six does not hand them back on the next launch. Both of these name operations, and `ui_names_no_operation` was right to stop them living in `ui/`. That test exists because the failure is silent and cumulative, and it caught exactly what it was written for: a preset called "Punch" is a statement about contrast, clarity and vibrance, and a table mapping Adobe's vocabulary to ours is a statement about the pipeline. Neither is a fact about an interface. So the starter set went into `dr-pipeline`, and the importer into its own crate — between two walls, since `dr-pipeline` depends on nothing on purpose and XMP is real XML not worth hand-rolling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-29 22:00:53 +02:00
dtourolleandClaude Opus 5	bb35665bd2	Let a paste carry some kinds of edit and not others ... FR-DEV-6 asks for presets "covering a subset of the edit graph". What landed with the named presets covered two subsets: everything, and everything but the crop. "Match the colour but not the sharpening" had no way to be said. `Scope` is now a set of `Attribute` — the same six kinds every operation already declares and the develop panel already builds its tabs from. The photographer ticking "tone and colour" is naming the groups they navigate by, and neither this module nor the interface has to name an operation to do it (FR-DEV-3c). The pleasing part is what left. Framing used to be excluded by an explicit test against one operation's id; it is now excluded because Geometry is not in the default set. The special case dissolved into the general rule, and the argument for it — a crop is a decision about *this* photograph, and carrying it across forty destroys forty compositions — is now a statement about a kind of edit rather than about a node. All thirty-three existing preset tests pass unchanged, which is the evidence that the generalisation kept its promises. One decision that is a field rather than a rule, because the two cases genuinely differ. An operation this build cannot classify — from a newer version, arriving over sync — travels under "everything" and "everything but the crop", because those are claims about the whole edit and an unrecognised operation is part of it (FR-NC-8). It does not travel under a hand-picked set, because that is a claim about kinds, and an unknown kind is not one of the kinds that were ticked. The settings page's "Copy crop and rotation" checkbox is gone, replaced by the same chips the preset sheet draws. It asked the right first question — geometry is the kind whose accidental travel destroys work — but it was the only question a boolean could ask. The field stays in `Settings`, read exactly once to seed the new set, so anyone who had ticked it keeps their behaviour. The chips are deliberately not in the develop column. Six of them there would set the width of the whole sidebar, which is the bug `ChipGrid`'s comment records at length. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-29 20:38:19 +02:00
dtourolleandClaude Opus 5	ab0ef6a26d	Say which requirements the code was already satisfying ... Thirteen requirements were surveyed as built but untagged. Eight of them were: R3, R6, FR-DEV-1, FR-UI-6, FR-NC-6d, NFR-OPS-3, NFR-PORT-2 and NFR-SEC-3. Each was read against its full text in requirements.md and against the code before the tag was added, because a tag that is wrong is worse than an absent one — it turns a visible gap into an invisible one. The five that were refused, and why, because the reasoning is the part worth keeping: R2 carries "(figure TBD)" in its own acceptance criterion and asks for a stated prefetch margin and cache-hit rate; neither figure exists anywhere in the tree and neither quantity is measured, while TD-2 and TD-3 both describe the thumbnail path falling short of it. R5 asks for three things and the code does one. The display pipeline does run at viewport resolution, but "only visible tiles are computed" and "panning recomputes only newly exposed tiles" need a tile scheduler that does not exist — and frame_budget.rs currently argues for striking tiled computation from the interactive path rather than building it. FR-RAW-2 asks for a trait taking a SourceRef, so that a second decoder can be added without changing callers. What exists is free functions over &[u8]. That meets the requirement's stated *purpose* — the same decoder serves a local file, a SAF document and a byte range, which is exactly why it takes bytes — but there is no trait and no second implementation seam, so the requirement should probably be amended rather than tagged. NFR-ARCH-1 asks for named executors with stated thread counts. architecture.md §7.1 states the table; nothing implements it. Workers are twenty-odd ad-hoc std::thread::spawn sites, each building its own one-worker tokio runtime, with no decode pool, no GPU-submit executor and no I/O pool. The requirement's own text says R4 and NFR-P9 "assert an outcome with no stated means", and that is still true. NFR-SEC-4 is satisfied by absence — there is no telemetry — and absence has no module to tag. A tag would point at nothing. NFR-OPS-3 was the closest call of the eight taken. The store is single, separate from the catalog, survives a catalog rebuild and does not sync between devices; it has no version *field*, deliberately, and settings.rs argues why and names the condition that would need one. The substance is met and the reasoning is recorded where it belongs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-29 20:23:16 +02:00
dtourolleandClaude Opus 5	5a8327824f	Keep an edit under a name, not just on the clipboard ... FR-DEV-6 asks for three things — named presets, copy/paste between images, and batch-apply to a selection. The last two have been here for a while; this is the first. The format is the sidecar's, deliberately. A preset *is* the non-default half of a version, so the lines are the same lines keyed the same way, which makes the two files diffable against each other and lets someone debugging an edit paste a block from one into the other. One file rather than one per preset: a preset per file makes the name a path, and every name then has to survive a filesystem — a `/` becomes a directory, a name differing only in case collides on one platform and not another, and renaming becomes two operations that can half-fail. As a key in a document it is none of those. Unknown *parameters* needed no machinery. `Preset` already holds whatever keys it is given and resolves them against the descriptors only at apply time, so one written by a newer build survives by being stored. Only lines that are not `op.param = float` at all are preserved verbatim, which is the sidecar's version-skew promise made here too. Applying is the paste path with a different source, so a preset reaches a selection through the sidecar read-modify-write that was already there: no graph, no decode, no GPU, forty files or one. Two smaller decisions worth the record. A library that fails to parse is held empty in memory and *not* written back over — settings regenerate themselves and this is work, so a parse failure must not be the moment it is destroyed. And every save persists immediately and rolls the in-memory copy back if the write fails, so the sheet never lists a preset the file does not have. The grid's "Presets" button is gated on the selection alone, unlike the "Paste to 40" beside it. That button needs a clipboard armed this session; the preset list is whatever was saved last month, and hiding it behind an unrelated action is what makes a feature only its author knows about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-29 20:07:14 +02:00
dtourolleandClaude Opus 5	5133e53bc8	Give back what a straighten took, when the angle comes back ... The auto-crop only ever shrank. Straighten to 20 degrees and the corners are cropped away correctly; come back to 3, or all the way to zero, and the crop stays at the size 20 degrees demanded. Nothing on screen explains why the photograph is still small, and the only way back was undo. The cause was that each correction was computed from the previous correction's output, so it accumulated: every angle the slider rested at took its cut and none was ever returned. The fix is to stop accumulating and recompute. The applied crop is now always the user's own rectangle fitted into the current angle's safe area, so as the angle falls and that area opens up the crop grows back — and stops, exactly, at the rectangle they chose. At zero the safe area is the whole frame and the fit is the identity, which is what carries it the last of the way home. There is deliberately no early exit for the upright case now: that exit is precisely what would strand the crop small. **The intent is remembered as a pair, so it repairs itself.** The session keeps `(applied, intended)` — what the correction wrote, and what it was derived from — and trusts the remembered intent only while the graph still holds `applied`. Every other route to the crop leaves something else there: a handle dragged, a ratio chosen, a sidecar loaded, a paste, an undo. That mismatch is the signal the memory is stale, and the current rectangle becomes the new intent. The alternative was a write into this field from each of those paths, which is the kind of bookkeeping that is correct until someone adds a seventh path. Dragging a handle therefore *is* the user choosing, including at a non-zero angle: the correction will not later grow the crop past what they dragged it to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-29 19:16:43 +02:00
dtourolleandClaude Opus 5	d9eb8faffd	Crop away the corners a straighten exposed, once the slider is let go ... Turning a rectangle inside its own bounds exposes its corners: there is no source pixel out there, and the shader renders it black. Nothing in the render prevents that, deliberately — a free angle does not change the output size, which is what leaves the frame where the user put it while the slider moves. Correct during the drag; four black wedges on the finished photograph. Letting go of the slider now pulls the crop inside the area the angle leaves defined. `Framing::max_inscribed_crop` already computed that bound and had no caller; this is the caller its doc comment described. **Once, at the end of the gesture.** Applied per frame it would shrink the crop on every step of the slider and never grow it back, so a user who overshot to 20° and came back to 3° would be left with a crop ratcheted down by the excursion rather than by the angle they settled on. Per gesture it is bounded by the angles actually rested at, and undo steps back through them. **The crop is fitted into the bound, not replaced by it.** A crop placed deliberately off-centre is a decision, and an automatic correction that recentred it would undo the user's work to fix a problem they did not have. `CropRect::fitted_into` scales only as far as the bound demands and then slides the rect the shortest distance needed to be inside — so a ratio locked in the crop panel survives the straighten too, since the shape is never touched. It returns the rect unchanged, bit for bit, when nothing needed to move. That matters more than it looks: this runs on every release of the slider, including releases at zero, and a rect that drifted by a rounding error each time would be an edit recorded for no reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-29 13:49:19 +02:00
dtourolleandClaude Opus 5	e6ad906bc1	Let the crop be held to a ratio while it is dragged ... A photographer cropping for a print, a phone wallpaper or a 16:9 frame is not choosing four edges — they are choosing one edge and a known shape. Free-dragging every corner made them do that arithmetic by eye on every drag, and get it slightly wrong. The panel now offers Free, Original, 1:1, 3:2, 4:3 and 16:9, with a Portrait switch for the ones that have two orientations. Original follows the frame rather than naming a number, so it stays right on the next photograph from another body and after a quarter turn. **The ratio is of output pixels, and the rect is not.** `CropRect` is stored in fractions of a frame that is not itself square, so holding a shape needs the frame's size — `ratio * height / width` of the frame. Skipping that gives a "1:1" crop that is square only on a square photograph, which is the one case nobody would test on, so the conversion lives in `CropRect::with_aspect` where it is explained and pinned by a test that asserts the fractions are *not* equal. Two decisions worth recording: The reshaped rect **grows** onto the ratio rather than shrinking onto it, then scales down only as far as the frame's edge demands. Fitting inside instead makes a one-axis drag do nothing at all — the other axis clamps the first straight back, and the handle simply refuses to move. The overlay now reports **which corner the drag is holding**, because reshaping onto a ratio has to know which corner is nailed down and only the handle that took the press knows that. A move reports no corner and keeps its shape: reshaping about a centre would pull an over-moved rect smaller instead of sliding it along the edge. The lock lives with the window rather than the session. A `DevelopSession` is per image, and cropping a set of frames to one shape is exactly when the lock earns its place. It is not an edit and reaches no sidecar — what is saved is the rectangle it produced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-29 13:48:51 +02:00
dtourolleandClaude Opus 5	bff95e25ad	Let clarity's base be computed where it is still fully determined ... Clarity's Gaussian sigma is 1.2% of the frame's shorter edge, so its radius is a property of the viewport: 52 render pixels at 4K, two separable passes of 105 taps each over 8.3 M pixels. That measured 33.9 ms — seven times the entire fused point chain, for one slider — and is docs/technical-debt.md TD-4. A detail pass may now declare `output_scale`, and clarity's base is computed on a grid a quarter the size on each axis. The pass that combines needs the blur *and* the full-resolution colour, and a colour that has been through a quarter-scale target is no longer full resolution. So a scaled pass cannot simply join the ping-pong: there are two chains now. The full-resolution one carries the colour and no scaled pass touches it; the reduced one carries the base and reaches the combining pass through a second binding as `reduced_at()`. The reduce is a dispatch of its own rather than something the first blur half does on the way past, and that is the whole difference between this and the strided kernel the module documentation rules out. A stride samples an image that is not band-limited and aliases high-frequency content down into the base, which is then subtracted, and arrives in the output as mottling across smooth gradients. This band-limits first and samples after. What is discarded is content the base could not represent at any resolution, because a Gaussian at sigma = 26 px holds nothing above one cycle per 26 px and the quarter-scale grid carries one per 8 — so the reduced base is not an approximation of the full-resolution one, it is the same function sampled where it is still determined. Which is also why the scale belongs to the band rather than to the stage. Texture's sigma is a decade finer, so the reduce pass's own box would be wider than the Gaussian it was prefiltering; texture never reduces. And clarity steps 4 -> 2 -> 1 as sigma falls, because a quarter of a small sigma is not a Gaussian either — the case that gives up is the one that was already cheap. `radius` stays in each pass's own pixels and `ComposedDetail::radius` multiplies it back up, so 13 reduced pixels at scale 4 still report the 52 render pixels a tile would have to be grown by. The halo a scheduler sees does not move. The halo tests pass unchanged, which was TD-4's stated bar; they render at 1024 px and so exercise the reduced path rather than stepping around it. Added `crossing_the_reduction_threshold_does_not_change_the_picture`, because nothing yet compared the reduced form against a *less* reduced one — every other test measures one form against itself. It renders the same edit either side of the 4 -> 2 step-down and holds the peak excursion to 0.03 stops and the reach to 2% of the frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-29 13:12:29 +02:00
dtourolleandClaude Opus 5	0cd3ef1b3f	Run a node declaration without compiling it ... `ops/*.yaml` plus `build.rs` has been the class-1 plugin format since the declarative nodes landed — it was simply resolved at build time. Nothing about a declaration requires the compiler: everything it produces is data plus a WGSL string, and the composer already assembles WGSL at run time from whatever operations are active. So this is not a new mechanism. It is the existing one, loaded later (FR-PLG-2). `DeclaredOp` implements `Operation` from an owned `Declaration` — one interpreter over many declarations, where `build.rs` emits generated code per node. The generated path stays, as FR-PLG-2 says it should: a generated `match` is faster than an interpreted one, the built-ins' declared `tests:` have to run under `cargo test`, and generated source is inspectable in a way an interpreter's state is not. **The reader is now one file, read by both.** `src/declared/decl.rs` and `src/declared/expr.rs` are `#[path]`-included by `build.rs` as well as being modules of the crate, and they produce a neutral `Declaration` that names no Rust type. The build script's job is reduced to *rendering* that declaration as Rust; `DeclaredOp` converts the same declaration into descriptors and `Expr::eval` walks the same tree the renderer writes out. There is one grammar, one set of validations and one set of error messages, so "a plugin is the same kind of thing as a built-in" is structural rather than aspirational. What remains genuinely written twice is the pair of backends — an arithmetic node rendered as Rust here and evaluated there — and that is what the parity test stands between. `tests/declared_parity.rs` parses every built-in declaration at run time and asserts the composed WGSL is byte-for-byte what the generated implementation produces, with the uniform block bit-for-bit identical, at both ends of every parameter's range and at four interior points; then again over the whole develop chain with the declared nodes swapped in, which is what covers uniform slot ordering and helper de-duplication between operations. A third test asserts the declared and hand-written nodes partition `ops/` between them, so coverage cannot shrink silently. Bit-for-bit rather than within a tolerance, because a tolerance is where a real divergence hides. The one thing that had to be got right for that to hold is number literals: `expr::as_f32` rounds a decimal exactly once, through the same shortest-round-trip text the compiler is handed, rather than rounding an `f64` a second time. Not in scope, and deliberately untagged: load-time WGSL validation (FR-PLG-11), id namespacing, a plugin directory read at startup, and pass nodes (FR-PLG-2a). Those are separate work, and tagging them from here would be the overstatement the spec's own §7 warns about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-27 19:08:34 +02:00
dtourolleandClaude Opus 5	0c3b8cb1c4	Hand out descriptors a declaration could produce ... `Operation::descriptor()` returned `&'static OpDescriptor`, and that lifetime is the whole reason a build-time node is free and a run-time node is impossible: only a compile-time literal can satisfy it, so no amount of reading `ops/*.yaml` at startup could ever produce a descriptor the rest of the application would accept. FR-PLG-2 says a bundled operation and a third-party plugin are the same kind of thing, differing only in where the file was found — and a lifetime outsiders cannot meet is exactly the second, weaker format that requirement forbids. So a descriptor is now owned and handed out as `Arc<OpDescriptor>`, with `Vec` where it held `&'static` slices. `Arc` rather than a `&self`-borrowed reference because the callers want to *keep* it: the develop panel collects descriptors and then mutates the graph, and a borrow would tie the descriptor's lifetime to a borrow of the operation it came from, which is the one thing `&'static` was doing right. The identifier newtypes deliberately did not follow. `ParamId` is `Copy`, is compared in `match` arms against generated constants, is a map key in the sidecar and history, and reaches Slint model rows; an `Arc<str>` there would cost a refcount on every one of those and would take `match id { EXPOSURE => .. }` away from the generated code. They gain an interner instead, which is honest about its lifetime rather than pretending to one — the set of ids is bounded by deduplication and is process-lifetime by construction, because the sidecar on disk names its parameters and an id has to stay resolvable for as long as any edit naming it can be opened. No behaviour changes. Every descriptor that was a `static` is a `LazyLock` initialiser now, `Operation::helpers` borrows from `self` instead of being `'static` so a future run-time node can own its list, and `Warp` and `Framing` follow `Operation` so there is one shape rather than two. The one place a descriptor is read per frame is `compose_full`, which takes `descriptor().id` to prefix each active operation's uniforms, and `dr-ui` composes on every frame it draws. That is a dozen atomic increments beside a composition that is already building several kilobytes of WGSL on the same call; it is noted at the trait method rather than left for a profiler to find. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-27 19:02:25 +02:00
dtourolleandClaude Opus 5	ce201c7dd6	Name the two spaces a photograph lives in, so a turn cannot go the wrong way ... Every orientation bug this codebase has had has been the same bug: a turn of the right size applied in the wrong direction. That failure is worth naming precisely, because it does not look like one — a quarter turn applied backwards lands 180 degrees from right, so the result is a plausible transform of the picture rather than anything obviously broken, and on landscape frames it is not wrong at all. It was the straighten shear, and it was the segmentation overlay, and each time it was found by eye rather than by a test. The reason it keeps happening is that "rotate 90 degrees clockwise" cannot be checked by reading it. The reader has to hold in their head which of the two images is being rotated and which way the y axis runs, and there were four hand-written copies of the permutation to hold it for: the shader prologue, its CPU twin, the thumbnail path, and the segmentation. So nothing added here says clockwise, anticlockwise, horizontal or vertical. The functions say *which space they take and which space they return* — `into_shown` and `into_stored`, `source_pixel` and `shown_pixel`, `into_shown_rect` and `into_stored_rect` — and each takes the dimensions of the space it reads from, so no caller has to work out which pair it is holding. `StoredRect` and `ShownRect` are separate types because they are the same four numbers meaning different things, which is exactly the case where a mistake is silent: a shown rect measured against stored dimensions produces a rectangle in the wrong place, not an error. Underneath there is one permutation. `source_pixel` was already shared by the prologue and the thumbnails; `source_point` is its normalised twin, written beside it so the two cannot drift, and everything else is those two read forwards or backwards. `Orientation::inverse` is the group inverse rather than `4 - turns`: mirrors apply after the turn, so undoing means undoing them first, and a mirror seen from the far side of an odd turn is about the other axis. That is the diagonal-mirror case, tags 5 and 7, and getting it wrong renders as — again — 180 degrees. Three call sites lose their own copy: the thumbnail path, `dr-ui`'s segmentation, and `dr-gpu`'s `local` example. "Upright" now means one thing across the application rather than one thing per caller. The gate that matters most is `the_render_and_the_orientation_map_agree`. The shader prologue and `Orientation` answer the same question by different routes, and until now nothing checked that they answered it the same way. It now checks every EXIF tag against every user rotation and mirror on top of it, because the composition is where the two could agree singly and disagree together. The rest earn their place by having caught something. Writing these found two real errors in this commit's own new code before it ran anywhere: `shown_pixel` was handed the dimensions of the wrong space and overflowed, and the rect map turned the wrong way for the diagonal mirrors. A round trip that returns what went in is the only check worth having here, since every wrong answer is still a picture. No behaviour changes. The permutations are the ones that were already being applied; they are simply applied from one place now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-27 09:46:24 +02:00
dtourolleandClaude Opus 5	d7a81375ee	List the steps, and let a photographer step straight to one ... Undo answers "take back the last thing", which is the question asked about a mistake just noticed. It is the wrong instrument for one noticed six adjustments later: eight presses, each changing the picture, with no way to see how far back the mistake is without passing through it. A step is a whole state, so arriving from six away costs what arriving from one does — which is what makes a row worth making clickable rather than decorative. `Edit::Discrete` had to go for the list to be worth drawing. Seventeen call sites recorded the same anonymous step, which is fine for deciding whether two changes are one gesture and useless for a panel: seventeen rows reading "Discrete" is not a history. Every variant now carries enough to name itself, and the compiler enumerated the sites that had to start saying so. A step that moved a parameter is still named out of the descriptor, so an operation added as a YAML declaration appears in the history correctly named with nothing written for it (FR-DEV-3c). Choosing a film stock was not undoable at all. The pick went straight to `choose_film`, which nothing on the history's path ever sees. `pick_film` records it, and is separate because the same call is also how a *restored* edit gets its tables back — recording that would push a step for the undo the photographer had just asked for. The list is rebuilt off a revision rather than off every redraw. A drag ends in a redraw per frame while folding into one step, so the unconditional version would tear down and recreate every row sixty times a second to arrive back at the list already on screen. The counter is process-wide: a per-instance one starts every photograph at the same number, so a frontend holding "the revision I last drew" would keep the previous image's steps on screen — invisible while every image opens with one identical row, and a wrong-photograph bug the moment persisted history means it does not. The step names that no descriptor can supply are constants with a roll, and a test walks the roll rather than a second copy of it. `resolve` splits so that "is this catalogued?" can be asked: `derive` turns `history.mask_toggled` into "Mask Toggled", which names a field rather than an act and, being perfectly readable, is a mistake nobody would look at twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 23:04:35 +02:00
dtourolleandClaude Opus 5	b89f1cfece	Snapshot the whole edit in the history, so a drawn mask can be taken back ... The undo stack snapshotted a `Preset` — the parameter map — and a mask layer is deliberately not a parameter. So drawing one changed nothing the history could see: `record` returned `false`, no step opened, and the layer the photographer had just painted had no way back. The interface went on calling `record` in good faith, including from the mask controls, and nothing failed. A film stock went missing the same way. The snapshot is an `EditState` now, so the history is complete by construction rather than by anyone keeping a list in their head. `undo` and `redo` return a `Step` rather than a `bool`. Stepping is not the only outcome a caller has to act on — a step across a change of film leaves the graph without its tables, and only the caller can bake them — and a `bool` would let that be dropped by writing nothing at all, which is the shape of mistake this module had already made once. `DevelopSession` settles the debt either way; a step that found nowhere to go is left alone, since clearing the film because undo hit the floor would take the stock off the picture. Five tests, all of which fail against the old snapshot: a drawn layer is undoable and redoable, a layer's own settings are a step of their own, a change of stock is a step and names what it needs baked back, clearing the film is undoable, and an exposure move does not deep-copy the mask stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 22:54:55 +02:00
dtourolleandClaude Opus 5	93efdf27a6	Keep the film stock when an edit is saved ... `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>	2026-08-26 22:53:00 +02:00
dtourolleandClaude Opus 5	5622a58ce3	Give an edit one complete state, and make omitting part of it a compile error ... 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>	2026-08-26 22:50:11 +02:00
dtourolleandClaude Opus 5	4a82753d22	Show the detector the photograph, not the sensor's scanlines ... "Find subjects" was handed the proxy in the sensor's own orientation, so every frame shot on a body held sideways reached the model lying on its side — and a model trained on upright photographs is very bad at those. Measured end to end on a 22 MP frame of two people and a dog: `person 0.36` and nothing else, against `dog 0.82, person 0.61, person 0.49` for the same pixels stood up. Nothing failed; the panel simply offered one poor subject where there were three good ones. The orientation was never dropped on purpose. The proxy is deliberately rendered through a *neutral* graph — the detection has to survive an exposure change, or every slider would invalidate the masks built on it — and neutral took the file's orientation with it along with everything else. Landscape frames were unaffected, which is why it stood for as long as it did. The turn is `Orientation::source_pixel`, the same function the grid's thumbnails already go through, so the detector and the thumbnailer now agree about which way is up rather than holding two opinions. What it is turned by is `Framing::effective_orientation` — the file's EXIF tag and the photographer's own rotations composed into one permutation, by the group law rather than by adding the turns, which is a distinction `Framing` already had to make and had already tested. Rotating the picture and pressing the button again therefore does what it looks like it does. The proxy stays in sensor space and the masks come back into it. That is not a detail to be tidied later: the generated shader samples the mask array at `uv_src`, *after* the framing map, so a mask stored upright would sit a quarter turn off the subject it was drawn around. That is a wrong mask rather than a weak one, and nothing announces it. So the picture is stood up for the model and laid back down for everything else, and `upright`/`lay_down` are returned as a pair because calling one and forgetting the other is silent. Both directions are the one function: `upright` gathers through `source_pixel` and `lay_down` scatters through it. A quarter turn is a bijection of the pixel grid, so the round trip is exact — no filter, no resampling, and no hole to fill — and an inverse written out by hand would be a second thing to keep in step, whose way of being wrong is a mask mirrored about the wrong axis, which still looks like a mask. The orientation joins the confidence and the tiling flag in the segmentation signature, and for the same reason: turning the photograph changes what the model recognises, so two runs either side of a rotation are different instance lists. Two that happened to come out the same length would otherwise share a signature and a stored layer would be silently re-indexed from one into the other. The refine pass had it too — it re-runs the model over a crop rendered in the same sensor space — so it makes the same turn, and would otherwise have handed back a worse mask than the one it was asked to improve, on the subject the photographer had just pointed at. `dr-gpu`'s `local` example is fixed with it. It exists to be the shipping path with pictures attached, and a diagnostic that reproduces the bug it is meant to catch is a trap for whoever reads it next. Seven tests. The round trip is the identity over all eight EXIF tags on a non-square asymmetric grid; a turn carries whole pixels rather than shearing the channels apart; a sideways frame reaches the model upright; a box comes back in sensor pixels, worked out by hand for the one turn a portrait frame actually writes; a restored box still reads low-to-high for every tag, since the rest of the pipeline takes `x1 - x0` without checking the sign; and the eight tags cannot collapse into one signature key. The existing composition test now runs against `effective_orientation` itself, over all 8 x 16 baseline-and-user pairs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 22:40:58 +02:00
dtourolle	e0cb968e04	Merge remote-tracking branch 'origin/master' into worktree-spot-removal ... # Conflicts: # docs/traceability.md	2026-08-26 21:56:32 +02:00
dtourolleandClaude Opus 5	e14bc34a9e	Ask which frame this was taken on, because grain is enlargement ... A crystal is a fixed size in micrometres. How grainy a photograph looks is therefore not a property of the emulsion alone -- it is film size against output size, and the frame is the half a digital file cannot supply. This assumed 35 mm for everything. The same emulsion on 4x5 averages about 3,800 crystals into the pixel that holds 300 on 35 mm, so it renders roughly 3.5 times smoother at the same print; every large-format photograph was being rendered as grainy as a half-frame. `Format` now carries the real image widths -- the gate, not the nominal inches, since a "4x5" exposes about 121 mm -- and the film node asks for it. It is a genuinely fixed list, unlike the stocks, so it is a declared `enum` parameter and gets its control, its sidecar entry and its undo step for nothing. It is also the first enum in the develop chain, and it broke two tests by being one. A row has to compare equal to itself across two builds or `sync_rows` replaces it on every parameter event -- destroying the elements built from it, including whichever TouchArea holds the current gesture, so the format picker would have fought every slider drag in the panel. `ModelRc` compares by identity and the row built a fresh choices model each call. `no_choices` already shares one empty model for exactly this reason, and the build site already said "see no_choices for why the identity matters". The fix follows it: memoise the model per variant list. Curve rows solve the same problem the other way, writing values through the existing model, which is not needed here -- a variant list is fixed at compile time, so one model can serve forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-26 21:47:19 +02:00
dtourolleandClaude Opus 5	8ab9440190	Put the repair tool on the photograph ... A third chip beside Crop and Local, and the mode strip's own comment predicted the shape: a mode that arms a gesture on the canvas and scopes the column. Click a mark to cover it, drag the disc to move the repair, drag the source circle to say where the patch comes from, Delete to remove it. The source starts two and a half radii towards the middle of the frame, which is FR-DEV-8's automatic placement in its cheap form — dust sits on skies and skies are smooth, so it is usually right and always one drag from fixed. Two things are drawn deliberately. The circles are the size the repairs actually are, because whether a disc covers a speck is the whole judgement being made and a fixed-size dot would say nothing about it; the reach around them is padded to a touch target so a spot on a dust mark can still be picked up on a phone. And only the selected repair shows its source: a dusty sky carries a dozen, and two dozen circles with nothing saying which belongs to which is less information rather than more. The panel edits what is stored while the canvas draws what is mapped, and the two are pushed separately for that reason — a slider deriving its value from the drawn radius would move differently at different zoom levels. It is also the one panel built from SliderRow rather than a live track: a repair has no OpId to coalesce a drag under, so a row that fires once per gesture is what keeps undo one step per decision. Verified as far as this environment allows: the strip renders and the column re-scopes, photographed under XWayland. Synthetic clicks do not reach this application, so the gestures are as-written rather than as-felt, and docs/spot-removal.md says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 20:58:13 +02:00
dtourolleandClaude Opus 5	2958444835	Let undo reach the repairs before the tool can make one ... A history state was a Preset — a map of scalars — which was right while every edit in the graph was a parameter. A spot is not one, and undo is the first thing anyone does with a repair: place it, dislike it, take it back. Left as it was, that press would have stepped some unrelated slider and left the spot on the photograph, which reads as undo being broken rather than as undo being absent. So a state is now the pair, params and spot set. The same door is the one the mask stack will come through: mask edits are outside undo today for exactly this reason, and FR-DEV-5 is not finished until they are not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 20:27:34 +02:00
dtourolleandClaude Opus 5	f00b3ae924	Take the tone from the hole and the texture from beside it ... A clone gets the texture right and the level wrong. Dust on a gradient sky is copied from a patch a little lighter than the hole it fills, and the repair reads as a disc even though every grain in it is correct — which is why FR-DEV-8 asks for heal and not only for clone. Heal adds the membrane: the difference between the two neighbourhoods, sampled at twenty-four points around the rim and interpolated across the disc by inverse square distance. Solving the Poisson problem properly is tens of Jacobi iterations, and an iteration here is a dispatch — sixty dispatches to remove a dust spot is not a frame budget. The closed form costs one loop over the rim, no state, and no second pass. The spec called for mean-value weights; inverse squares are two transcendentals per sample cheaper and agree wherever the boundary difference varies smoothly, which is every repair anyone makes. What decides whether that trade holds is the measurement, so the measurement is the test: on a ramp steep enough to leave a clone wrong by 38 levels out of 255, the heal is wrong by 0. docs/spot-removal.md §6.1 records what shipped and what it would take to go back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 20:22:10 +02:00
dtourolleandClaude Opus 5	5323608051	Draw the repairs, before anything sharpens what they removed ... A spot set now composes detail passes of its own, one per round, and they go ahead of every operation's kernel. That placement is the decision worth recording: a sharpening pass reads a neighbourhood, so sharpening a dust mark before removing it smears its edge into pixels the repair's disc does not cover, and what survives is a faint over-sharpened ring around an otherwise perfect patch. It also disagrees with ARCH §5.2, which draws spot removal after clarity — docs/spot-removal.md §5.1 is where that is argued out. Every length reaching the shader is in render pixels, converted here where the framing is in scope. Both the centre and the source go through `Framing::output_at` — the same map the fused pass applies to every pixel — so a rotated photograph rotates the offset with no trigonometry, and the radius is found by mapping a point one radius above the centre and measuring, rather than by multiplying by a ratio this function has no business knowing about. The tests turn and crop the frame and expect the mark to stay gone, which is the property that arrangement buys. compose_full now takes the spot set, because a photograph with a repair and no sharpening still has a detail stage: a fused pass that encoded its own output there would quantise twice and bind to a texture of the wrong format. compose_detail_for takes the source size for the same kind of reason — a RenderScale describes the region on screen, and a spot is stored against the photograph. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 20:17:51 +02:00
dtourolleandClaude Opus 5	6997c0f7ac	Let a detail pass carry a list, not only a kernel ... Every neighbourhood pass so far has been a convolution, whose whole description fits in the uniform block because its structure fixes how many numbers it needs. Spot removal is not that shape: sixty-four repairs and one repair are the same shader with a different buffer behind it. So a pass may declare `storage`, which arrives at binding 3 as `array<vec4<f32>>` with `arrayLength` in scope. The alternative — packing the list into uniforms — needs a fixed maximum paid for on every frame, a composer that can emit vec4 fields because a uniform array's stride is 16 whatever it holds, and it gives the next operation that wants a table nothing to build on. The property worth having is what stays out of the generated source: the count is in the buffer, so placing the tenth spot uploads 512 bytes and reuses the compiled pipeline, exactly as moving a slider does for the fused pass. `changing_the_list_does_not_recompile` is that, asserted. One bind group entry rather than two more layouts, and one placeholder buffer allocated in `new` rather than sixteen bytes per pass per frame — a zero-length storage buffer cannot be bound, and per-frame allocation is what this module's documentation exists to refuse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 20:00:24 +02:00
dtourolleandClaude Opus 5	44444f7768	Write the repairs down, one line each, and merge them by id ... A spot is eight numbers, so it goes in the version block as a line rather than in a block of its own the way a mask does — 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 what a diff shows and what a merge resolves. The parse arm sits ahead of the op.param arm deliberately: without it, `spot.abc123 = 0.4 0.6 …` reads as an operation called `spot` whose value will not parse, and the repair is dropped with a warning about a corrupt number. The prefix is safe precisely because a spot is not an operation, so no ops/ declaration can claim the name. Merging is merge_masks by id, one level down, and it is where the derived ids earn their keep: two devices that removed different marks hold different ids and both survive, while two that removed the same piece of dust hold the same id and the merge sees the one repair it is. A spot both sides dragged resolves whole to the higher revision — eight numbers describe one disc, and half of each is a repair neither photographer made. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 19:52:54 +02:00
dtourolleandClaude Opus 5	97479a0512	Hold the repairs a photographer makes, and say which may share a pass ... A spot is a disc, a source offset and four numbers, and it lives beside `ops` for the reason `masks` and `film` do: the operation trait is ParamId -> f32, and a list of repairs is neither scalar nor fixed. Two decisions here are not obvious. The id is derived from the position rather than counted, because two devices editing offline would each mint `spot3` for different marks and the sidecar merge would then treat two repairs as one — from the position, two devices that removed the same piece of dust agree, and two that removed different ones do not. And every length is in the frame's isotropic units, not a mixture of those and shorter-edge fractions: one unit for the radius, the feather and the offset agrees on a landscape frame and on a portrait one, where a mixture only agrees on the first. `rounds` is the arithmetic that keeps a source from reading a destination. Every spot in one pass reads the photograph as it stood before that pass, so a spot sourcing from an earlier spot's destination would copy the mark that spot was removing. Grouping is not a pass per spot — that is sixty-four dispatches for a case that almost never arises — it is a new round only when the sources actually collide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 19:50:00 +02:00
dtourolleandClaude Opus 5	ce6458547a	Develop longer, from the measurements rather than from a contrast slider ... Pushing was not a thing to simulate. It was measured data being thrown away: Double-X and 2302 each ship five characteristic curves, one per development time, and this shipped the 6.5-minute column and discarded four. All five now ship and interpolate. The axis is real. Double-X runs 4 to 12 minutes, and across it the average gradient goes 0.472 to 1.034 while Dmax goes 1.19 to 2.56. The control is in stops, because that is what a photographer means, and one stop is a factor of about 1.41 in time. That mapping is checked rather than assumed: against Double-X's own axis it lands within 2% of the 9-minute column for +1, and near 12 minutes for +2, which are the times the datasheet gives for exactly that. There is a test. **Pushing must not recover shadow detail, and this does not.** Across the whole measured range the speed point moves about a third of a stop while the gradient doubles; three stops under mid-grey, density goes from 0.008 to 0.035, which is still nothing. Developing longer multiplies what was already recorded and cannot record what never hit the film. A push built as added exposure or global contrast brightens those shadows instead and looks convincing until someone who shoots film sees it, so that property has a test of its own. Interpolated in *log* time, because development is multiplicative: 4 to 5 minutes is the same amount of push as 9 to 12, and interpolating linearly would bunch the control at one end. Clamped at both ends, because past the published range there is no data and extrapolating a contrast curve invents an emulsion nobody tested. A stock measured at one process ignores the control entirely rather than inventing a curve for it -- Portra 800's pushes are separate *measured* profiles, which is the honest way to offer those. Costs nothing per pixel and changes no shader. The curves are a per-stock table, so the interpolation happens on the CPU at bake time, where choosing a stock and moving its sliders already rebakes. The Vulkan shader is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-26 17:35:25 +02:00
dtourolleandClaude Opus 5	4b2ee0ac50	Count the silver instead of adding noise ... An emulsion is a suspension of crystals. Light sensitises some; development turns a sensitised one opaque, all or nothing. So a patch of film's density is a *count* of developed grains, and a count of independent yes/no events has a variance whether or not anyone wanted texture: mean = D variance = D * (Dmax - u * D) / N That expression is the whole feature. It peaks in the middle of the density range and vanishes at both ends -- clear film has nothing developed to vary, black film has nothing left to develop -- so grain lives in the midtones as a consequence rather than as a "midtone bias" slider. I was wrong earlier that this needs the detail stage. Nothing in it reads a neighbouring pixel; the only reason to move it was that grain must be fixed in film space rather than screen space, and that solves itself: N is grains *per pixel*, so it scales with the film a pixel covers. Zoom out, each pixel averages more grains, less variance -- correct, with nothing super-sampled and nothing filtered. It stays in the fused pass. Grain goes on the density and *before* the dye, which is the physical order and not cosmetic. Perturbing the finished colour -- what an effect does -- tints highlights wrong, because that noise never passes through the dye. Crystal habit lives in `rms_granularity`, the number every datasheet publishes, now a profile field. It measures exactly what differs between a cubic emulsion and a tabular one: at equal speed, tabular crystals present more area per unit silver, so the film reads finer. Delta 100 is quoted near 9 where HP5 is near 12, and that gap *is* the habit. Adding a stock whose grain is its whole reputation is therefore editing one line, not writing a model. Three things this cost, all of them worth writing down: - The default granularity is a colour negative's, blue coarsest. Applied to Tri-X it put *colour* speckle on a black and white photograph. Monochrome stocks collapse it at parse, where every other per-layer table is already replicated from the one measured channel. - Helpers cannot read uniforms. The composer prefixes a uniform with its operation's id and rewrites references inside a fragment body only; helpers are shared and deduplicated, so a bare `gn0` names nothing. `film_lut` already took its size as an argument for this reason, and now says so. - The end-to-end test compares the shader against the CPU model, and grain is stochastic, so that comparison now runs with grain off. Which means a grain that never left the CPU would look exactly like a passing suite -- hence a second test that grain off is bit-identical, one grain per pixel moves it, and ten thousand move it less. Not here, deliberately: no grain slider. The parameters are physical and `rms_granularity` is the honest place to scale one from, but its range wants choosing rather than guessing. Nor a film format -- 35 mm is assumed, and medium format at the same stock is far less grainy per unit of picture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-26 10:08:51 +02:00
dtourolleandClaude Opus 5	56978fdf35	Clear the clippy warnings that were failing CI before this branch ... Nothing here is film simulation. These are lints that fail master today, under the -D warnings CI runs with, mostly from a toolchain that learned new ones rather than from anybody's code -- `is_multiple_of` and the derivable `Default` did not exist as lints when this was written. They are fixed rather than allowed, and by hand rather than by trusting `cargo clippy --fix` wholesale: its automatic pass split a derive in two and left a stray blank line, which is the sort of thing that is correct and still wrong to commit. The four that needed a decision rather than a rewrite: - The distance transform's inner loop writes through its iterator now. `q` stays, because it is the position the parabola is evaluated at as well as the index it is written to -- the lint is about the write. - `to_source` and `to_proto` take `self` by value. Their receiver is `Copy`, so this is the same machine code and the honest signature. - The export path's return type is five levels deep and now has a name, plus a line saying why the `Option` wraps the `Result`: `None` is cancellation, which is not a failure and has no error to report. - A test fills a range instead of looping over one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-25 22:35:02 +02:00
dtourolleandClaude Opus 5	3b5952769b	Emit floats an f32 can hold, and drop the format! that formats nothing ... CI runs cargo fmt --check and clippy -D warnings, and this branch had never been through either. Both would have failed it. The bulk was the generated colour tables: eight significant figures where an f32 carries about 7.2, so the eighth is noise that rounds away at compile time and clippy's excessive_precision says so 109 times over. Fixed in the generator rather than only in the file, so it stays fixed -- and the file is trimmed in place rather than re-derived, because regenerating it needs a colour-science stack that has nothing to do with the defect. The format! in the composer is mine too, from extracting the rendering tail: the braces in it were escaped because the text used to live inside a larger template, and once extracted the escapes are noise and the call formats nothing. Also here, and clearly not mine: an unused import and a shadowed binding in dr-gpu, and an unused import in a test. They are pre-existing -- clippy has been failing on master before this branch existed, on lints like is_multiple_of that arrived with a toolchain rather than with anyone's code. Fixed because CI cannot go green around them, and called out because a merge commit is a bad place to quietly edit someone else's crate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-25 22:28:14 +02:00
dtourolleandClaude Opus 5	baa8957e80	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>	2026-08-25 19:57:41 +02:00
dtourolleandClaude Opus 5	4a2fcb6d22	Render a film stock on the GPU, and let it take over the rendering ... The stock model landed in dr-film with no way to see it. This is the pipeline node, the two texture bindings it reads, and the end-to-end test that proves the shader agrees with the model. The design point is that a film simulation is not an adjustment. Every other node changes a picture; this one makes it. A stock's characteristic curve does the camera profile's base curve's job -- from measurements rather than from a curve somebody drew -- so running both renders the scene twice: the camera's rendering, and then a film's rendering of that. It looks like neither, and it reads as a colour-management bug with no colour-management bug to find. So `Operation::renders` is new. A node declaring it takes camera RGB and hands back linear sRGB, and the composer emits neither the base curve nor the conversion out of camera space. Both halves move together, and the composer keeps them as one string precisely so that getting half of it right is impossible. The tables are not parameters, for the reason vignetting's coefficients are not: they are measurements. dr-pipeline declares the layout as a plain struct and keeps its no-dependency property; the two crates share no types on purpose. `EditGraph::set_film_tables` offers them to every node rather than to the one that wants them, because knowing which concrete type is which is what the graph is organised not to know. Bindings 4 and 5 follow the masks precedent: declared unconditionally so one bind group layout serves every generated shader, bound to 1x1 placeholders when no stock is loaded. Both are interpolated by hand with textureLoad -- this pipeline binds no sampler, and adding one for two lookups would cost a binding in every shader. Uploads are keyed on content so an unchanged stock does not push half a megabyte across the bus per frame. The end-to-end test earned its place immediately: it found the density lookup being filled z-fastest while a 3D texture upload wants x-fastest, so the red and blue axes were transposed. Green matched exactly, which is what that bug looks like -- a plausible photograph of the wrong colour, and one that every unit test on either side of the seam passes. dr-film now pins the layout in a test that needs no device, and states it where the field is declared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-25 15:12:56 +02:00
dtourolleandClaude Opus 5	c75849040c	Format the tree the way the gate asks for it ... `cargo fmt --check` is a required step and had drifted across 45 files. Most of it arrived this week: several operations were written in parallel worktrees and merged by hand, and a hand-merge resolves conflicts without ever running the formatter over the result. No behaviour changes — this is `cargo fmt --all` and nothing else, kept as its own commit so the next reader can skip it wholesale rather than search it for one that matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-22 21:16:34 +02:00
dtourolle	ec4283e37f	Finish reconciling the whole-chain tests the three kernels each rewrote ... Sharpening, noise reduction and clarity were written in parallel and each rewrote the same two tests, which had counted one fused block per operation — true only while every operation was a point function. Kept the exclusive-or formulation: each operation must reach exactly one of the two stages. A count cannot tell "moved to the detail stage" from "vanished from both", and that ambiguity is what broke these tests three times over. The merge left two fragments of the versions it replaced — a loop over a set that no longer exists, and the tail of an assertion whose head was gone. The loop is not restored: `point ^ neighbourhood` already asserts per operation what it checked over the set. The assertion is, because it catches a different fault from the exclusive-or — a block in the shader that nothing in the chain asked for, rather than an operation in the wrong stage.	2026-08-22 19:34:24 +02:00
dtourolle	35b126449b	Order the detail stage so the repair runs before the enhancements ... Capture sharpening and noise reduction were written in parallel and both claimed `order: 110`; the codegen refuses that, which is the guard working — two nodes at one order is an ambiguous pipeline and operation order changes the result. Resolved in noise reduction's favour, for the reason its own `placement:` block already gives: denoising is a repair and everything else in this stage is an enhancement. Sharpening or adding clarity to a noisy frame amplifies the grain along with the detail, and no later pass can separate them again. So the detail stage now runs noise reduction, capture sharpening, clarity, texture, and the other three shift up a slot to keep the multiple-of-ten convention the rest of the chain uses. Noise reduction was also missing the required `attributes:` key. The order collision aborted the build before the attribute check could report it, so it arrived looking like one fault and was two.	2026-08-22 19:32:59 +02:00
dtourolle	d800af049b	Merge branch 'worktree-agent-acd27f9b2974c67eb' into integration ... # Conflicts: # core/dr-gpu/src/adjust.rs # core/dr-pipeline/src/detail.rs # core/dr-pipeline/src/ops/mod.rs	2026-08-22 19:31:15 +02:00
dtourolle	5852e14a5c	Merge branch 'worktree-agent-a75c901968abfa183' into integration ... # Conflicts: # core/dr-gpu/src/adjust.rs # core/dr-pipeline/ops/README.md # core/dr-pipeline/src/lib.rs # core/dr-pipeline/src/ops/mod.rs # ui/dr-ui/src/develop.rs	2026-08-22 19:29:39 +02:00
dtourolle	7031352e85	Keep neighbourhood operations out of a mask layer's chain ... A layer holds a full chain and fuses it into the colour dispatch, so the panel - which names no operation - would have offered a noise reduction slider inside a local adjustment. It could not have worked: the detail stage is its own dispatch, running after the masks are already applied, with nowhere to be handed one layer's mask. The control would have moved and done nothing. Filter the layer's chain to the operations that can honour it.	2026-08-22 19:23:43 +02:00
dtourolle	690e76a51f	Let the fully-active chain test account for both stages ... Activating every operation now activates a kernel too, and a kernel emits no block in the fused shader. Assert that each operation reaches exactly one of the fused pass and the detail chain, rather than counting fused blocks against the length of the chain.	2026-08-22 19:21:59 +02:00
dtourolleandClaude Opus 5	c9305fd0e6	Write out the derivation behind every kernel width these tests assert ... A test that cannot be run cannot be checked by running it, and a number copied out of a test run agrees with whatever the code did on the day. Each asserted kernel width, tolerance and overshoot bound now carries the arithmetic that produces it -- the shorter edge, the sigma, the truncation at two sigmas, and where the rounding falls -- so a reader can verify the expectation against the recipe without a GPU or a compiler. Also records the two places where a bound is a bound and not a measurement: the tolerance in the frame-fraction test is exactly what rounding a kernel to a whole pixel costs on the smallest frame it uses, and the halo test's floor and ceiling bracket a peak derived from the step, the soft limit and the midtone taper rather than from a run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-22 19:18:13 +02:00
dtourolleandClaude Opus 5	e44c929afc	Guard the sharpening kernel against WGSL reserved keywords ... The fused-fragment check in lib.rs cannot see a detail pass: it is a separate shader composed at a resolution compose() never knows. The kernel's own test now scans the block the composer wrapped, with comments stripped so prose about the keyword cannot fail a test about the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-22 19:17:05 +02:00
dtourolle	df3fe660e5	Finish the render when a kernel is too small to draw ... An active detail operation may emit no pass at a given render scale - the honest answer for a sensor-sized radius on a heavy proxy. The fused composer cannot see that, having no resolution to consult, so it had already stopped short of the output transform and the frame died on a storage-format mismatch. Compose a bodyless resolve pass in that case so the output transform still happens exactly once.	2026-08-22 19:14:57 +02:00
dtourolleandClaude Opus 5	00663a870b	Teach the chain test that a detail node has no fused fragment ... every_operation_can_be_activated_together counted one block per operation in the chain, which was true only while every operation was a point function. A neighbourhood operation is a dispatch of its own and emits no fused block, so the count now excludes the operations the detail chain names, and each of them is separately asserted absent rather than the comparison being loosened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-22 19:14:54 +02:00
dtourolleandClaude Opus 5	b321556dbe	Sharpen the capture with a separable unsharp mask ... Capture sharpening as a two-pass unsharp mask in the detail stage: blur along x, then along y, each pass applying a one-dimensional high-pass to luminance so the composite preserves a flat field exactly and matches the textbook kernel on any locally one-dimensional edge. The radius is stated in source pixels and converted once per render, so a radius tuned on a fit view is the radius the exported file gets. Below one render pixel the operation declines to draw rather than showing sharpening the file will not contain, and emits a single pass-through that still carries the output transform. The develop session now renders through render_detailed, which is what lets an active neighbourhood operation reach the screen at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-22 19:13:00 +02:00
dtourolle	a9baebc396	Sync with integration	2026-08-22 19:04:27 +02:00
dtourolle	5db234bdf3	Sync with integration	2026-08-22 19:04:24 +02:00
dtourolle	f78c8b6959	Sync with integration	2026-08-22 19:04:13 +02:00