417cba8b4d7d6160bbd05c8d5fae9e7dcca10cbf
176
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 dtourolle
|
2fd7690b6f |
Mark a pixel the lens correction pushed off the sensor with alpha 0 in the camera-space tap
The fused shader stored black with alpha 1 for a pixel whose source coordinate left the frame, and the merge's warp averaged it in like any other: a dark, badly interpolated fringe along every frame's edge, visible as a seam wherever a frame ended and, later, as the edge the border fill continued. The display keeps its opaque black; CameraLinear stores alpha 0 and the warp weights each sample by the alpha it interpolated, dropping a sample that has none. |
||
|
dtourolle
|
42d11d919b |
cargo fmt and clippy across the panorama work, and one lint master carried
The dr-face comparison is master's: a negated partial-order test on the eye box's width, rewritten as the two conditions it meant. |
||
|
dtourolle
|
75d2ceb23c |
Provenance in the sidecar, a launch hook for the page, and where it stands
derived_from and merge are top-level sidecar fields (FR-MRG-6): one line per source in order, and how the composite was made. A build that predates them keeps the lines as unknown and writes them back. The job writes the sidecar beside the composite and stages it with its own record when the composite goes through the outbox. DARKROOM_START_MERGE=a.CR2,b.CR2 lands on the merge page at startup with the job running on local files, on the model of DARKROOM_START_IDENTITY, for looking at the page where synthetic clicks do not reach it. The fetch and the start are shared with the grid's button. panorama.md §11 records what exists, the fixture's figures, and the six things still open, auto-crop first. |
||
|
dtourolle
|
9b6b4942cf |
The camera-space tap: OutputMode::CameraLinear, composed with no operations
compose_camera_linear composes the fused pass with an empty operation list, the file's orientation as the baseline, a view rect for the tile, and a store of rgba32float. On the GPU, render_camera_linear is the only entry that accepts it: it fills the profile uniforms neutral — unit white balance, identity matrix, curve off — so what lands in the texture is the sensor's numbers after the lens warp and nothing else (FR-MRG-2). A third bind-group layout carries the format, as the linear one does, and the readback is generalised to any pixel width for the f32 copy. Thirty-two bits because the composite is written back at the sensor's scale: a 14-bit sensor has 16 384 steps to white and f16 keeps 2 048 of them in the top octave. |
||
|
dtourolle
|
ed4460cb9c |
Tag three requirements the code already meets
R5 says in its own note that zoom_resolution.rs establishes it as a pixel equality; that file was tagged FR-DSP-5 alone. FR-DEV-19's three sub-clauses carry eighty-three tags between them while the parent had none; MaskLayer, which is the thing they edit, now carries it. And NFR-R3 — a crash in decode does not take down the application, the image is marked failed — is exactly what the decoder's panic guard and the face sweep's unreadable mark do, tagged FR-RAW-4 and NFR-SEC-1 and not the clause that asked for them. |
||
|
dtourolle
|
696bafa9d5 |
Undefer AI subject masking, which shipped, and give it a clause
§7 still listed "AI subject masking — deferred per D11" while MaskSource::Subject and MaskSource::Category, backed by dr-segment's instance and semantic models, had been the primary way a local adjustment is made for weeks. The code was tagged FR-DEV-3, which names gradients and brushes and says nothing about a model. FR-DEV-3i now states what exists: a subject or a category found by a local model, stored as identity with the run's signature so that it merges per field and reads as stale rather than wrong, then treated as any other layer by the edge, stroke, composition and reveal clauses. The one place it departs from FR-DEV-19 — coverage written run-length coded beside the layer, so a stored subject renders without a model — is recorded in the clause instead of left for the next audit to find. The segmentation crate and the UI's selection module are tagged to it. |
||
|
dtourolle
|
d3b6127db6 |
Let a photographer name the state they liked, and go back to it or look at it
FR-DEV-5 asked for named snapshots of an edit state and FR-DEV-7 for a comparison against a chosen one, and neither existed. The history stack is per sitting and forgotten with it, on purpose — the gap that mattered was an automatically saved mis-drag with no way back, and that was closed first. What was left was the other half: a state the photographer wants to keep *because* it is worth keeping, which is a different thing from a step and is not served by making the steps last longer. A snapshot is an edit state, and an edit state is exactly what a sidecar version stores, so it is stored as one: a `[version]` block carrying `snapshot-of = <uuid>`. The parameters, the masks and their parts, the repairs and the film all arrive through the blocks that already carry them, a merge keys on the uuid as it does for any version, and a build that predates the key reads the block as a named version and keeps it — the right failure. Only the pointer is new. The one reader that has to know is `default_version`, which must never answer with a snapshot: a file whose edit is missing is not a file whose edit is one of its saved moments. The snapshots of an edit are listed by that pointer, oldest first, the same on every device. Writing them back removes what this sitting deleted and puts in what it holds, and leaves standing whatever it never saw — a snapshot the other device took since the photograph was opened here is not this device's to remove by not knowing about it. That is the rule the version merge already keeps, applied one level down, and it is why the save carries the deleted ids rather than replacing the list wholesale as the masks are. Each is re-pointed at the uuid the save settled on, because the default may have been fused onto its canonical identity since the snapshot was taken. Restoring is one history step, so undo takes it back whole, as a paste is. Taking and deleting are not steps: they change nothing about the photograph, and an undo that removed a snapshot would be undoing a decision to remember. Holding the eye beside one renders the snapshot and hands the edit straight back — the same suspension "Before" uses, against a point the photographer chose rather than the file. Two sessions on the same photograph get ids that cannot collide, stamped with the second and a random word, because the merge folds equal ids into one. |
||
|
dtourolle
|
4574c35236 |
Let a part be left out of a mask without being taken out of it
A layer built from parts was missing the one control a correction most often wants: seeing what it did. The question a subtracted gradient raises is whether it took only the sky, and the question a stroke raises is whether it filled the shoulder — and the only way to ask either was to remove the part and look, which answered the question and lost the part. The layer's own ring answers a different question, about the adjustment, and hiding eight layers to check one correction is not an A/B anybody performs. So a part carries `hidden`. It is an edit and a history step, as the layer's switch is, and it is folded into the render fingerprint because hiding a part changes the mask as surely as removing it does. Where the mask is built the shown parts are walked rather than the parts, which is what makes a hidden base hand the fold to the first part that is shown — and a revealed layer whose every part is hidden clears its slice rather than leaving whatever the last rasterisation put there to be read back. `covers` asks the same shown parts, so a layer whose only adding part is hidden costs no slice at all. In the sidecar the key is `hidden`, in the part's block or, for the base, in the mask block — under a word that cannot be confused with the layer's `enabled`, which has always meant the layer. Absent means shown, so no file written before the switch existed reads any differently. The row wears the same ring the layer does, one row down, because it is the same question about a smaller thing. |
||
|
dtourolle
|
a87139b838 |
Give every mask an eye and a colour, and put the brush where the mask is
The first build of seeing a mask showed the selected layer's, in one global style, from a strip at the top of the panel. It answered the wrong question and answered it somewhere nobody looked. What a photographer asks of two masks is how they meet — where the sky's edge sits against the building's — and that needs both on screen at once, in colours that can be told apart. So each row of the stack has an eye, drawn in the colour its mask is shown in, and each mask has six swatches to choose that colour from. Several can be open at once; a new one comes up open, in the first colour nothing else is using. The style — tint, alpha, outline — is the one setting that stays global, above the stack, because three styles at once are three pictures that cannot be read against each other. Alpha now draws every shown mask, each in its colour, on black. In the pipeline a `Reveal` is a list of `(layer, colour)` rather than one layer, and every reveal block carries its own colour. The brush moves too. Select, Paint and Erase and the three sliders under them sat at the top of the panel, appeared only once a row was selected, and said nothing about which mask they acted on — so "how do I paint" and "how do I correct the model's outline" both had the same answer and nobody found it. They sit under the selected mask's parts now, beside the swatches, and on a subject or a category the hint says what a stroke there does: it becomes a part of this mask, joined to the model's, and can be taken out again. Eyes and colours are viewing state, on the session and not on the layer, so a photograph reopened has every eye closed — the stored-mask round-trip test asserts it. |
||
|
dtourolle
|
c045702a47 |
Show the photographer the mask they are shaping
Nobody can refine an edge they are not being shown. The only thing drawn on the canvas was the region overlay — a false-coloured picture of what the model *detected* — which knows nothing of a layer's feather, its falloff, its morphology, its invert or its opacity, and nothing at all about a gradient, a range or a stroke. Every control added for mask editing therefore acted on something invisible, which is why the whole feature reads as absent rather than as unfinished. A layer's finished mask now draws over the photograph in one of three styles: a tint for whether the right thing is selected, an alpha for where the edge is, an outline for whether that edge is registered against the detail the other two hide. The hard part is not the shader. A selection with no adjustment on it changes no pixel, so it is not active, so it holds no slice of the mask array and is never rasterised — and that is exactly the layer somebody wants to look at, for the whole of the time between choosing a subject and deciding what to do to it. So `MaskStack::rendered` is `active()` plus the layer being looked at, and the rasteriser, the composer and the distance-field builder all index by position in it. Which is also why the design's "two uniforms, no recompile" is not available: a uniform can select a slot, it cannot conjure one. The reveal is never on the graph. It reaches the pipeline as an argument to `compose_revealing`, and `compose_for` — which the exporter, the thumbnail and the neutral probe all call — has no way to ask for one. A flag on the graph would have been shorter, would have type-checked, and would have been one forgotten reset away from a red tint baked into an exported file. And the tools that shape a mask now arm. `Masking.tool` is an `in` property only Rust may write, and the handler wrote nothing back, so the strip reported "Select" however many times Paint was pressed and the paint area was never enabled — the brush, the parts and the whole of FR-DEV-19b reachable from no control in the application. The region overlay stands down while a mask is being shown, and its button now says what it hides: two overlays that look alike and mean different things is worse than either. |
||
|
dtourolle
|
df741a8a49 |
Let one mask be built from more than one selection, and paint into it
A mask the model draws arrives approximately right — stopping inside a shoulder, leaking into the hair — and FR-DEV-3's edge controls move the *whole* boundary, so no value of feather or dilation fixes two errors that go opposite ways. What fixes them is a second selection joined to the first, and a layer that held exactly one source had nowhere to put one. The brush the core has had all along was reachable from no control in the application. A layer is now an ordered list of parts. Each names a source and how it joins the mask before it — added to it, or taken out of it — and carries its own edge treatment, because a model's soft coverage and a stroke painted where it stopped short do not want the same feather. Invert and opacity stay on the layer, where the composed shader already reads them. The sidecar grows `[part]` blocks and nothing else. A layer of one part writes exactly the bytes it always did; a mask block with no part blocks after it reads back as one part; and a stroke, a join or a source this build cannot read costs that part rather than the layer. So every sidecar in every library still parses to the edit it always was. On the device the parts fold into the layer's one slice, so eight layers still cost eight channels: union is a `max` blend and subtraction is the erase blend the brush already used. A part is drawn into a scratch texture before it is joined, and that is not incidental — an erase stroke means a hole in *that part*, not a hole in the mask, and drawn straight onto the accumulator it would punch through the subject underneath. A layer of one part skips all of it and takes the path it always took. In the interface: a part list under the selected layer with a chip saying which way each joins, Add and Subtract beside it, a Select/Paint/Erase strip with the brush's size, hardness and flow, and a drag on the photograph that paints. Pressing Paint on a mask that cannot hold a stroke joins a part that can, rather than explaining that a subject is not a brush. A whole stroke is one step in the history. The edge controls now shape the part that is selected rather than the layer, which is the one behaviour change to an existing control: with a correction selected, the feather slider softens the correction and leaves the model's mask alone. |
||
|
dtourolle
|
af89433aee |
Offer the lens profile as a tick box, since applying it silently reads as absent
The develop panel's Optics group is three manual sliders: distortion, chromatic aberration and lens vignetting. The automatic correction was already there — the file's EXIF lens is matched against the bundled Lensfun database on open and the coefficients are fanned out to all three — but nothing in the interface said so except a line of grey text under the camera reading "· corrected", and there was no way to decline it. From the outside that is indistinguishable from the feature not existing, which is how it was read. `dr-lens` states the rule this breaks: an automatic correction that silently does nothing is worse than one the user can see is unavailable. The caption satisfied the letter of it and not the point — a photographer looking for "apply the lens profile" found three sliders and no switch. So the profile is now a control. It is a capability rather than a flag on the session, because everything a photographer sets travels one road: the capability list feeds the generated panel, `Preset` captures it, the sidecar stores it and the undo stack replays it. A bool on the side would have needed adding to each of those four by hand and would have been forgotten in at least one — which is exactly how the mask stack came to be missing from the history. It is on by default, which is what `switch_on` is for: the coefficients are a measurement of the lens that took the photograph, so accepting them is neutral and declining them is the edit. The sidecar therefore stores nothing for the ordinary case and the correction still happens. The switch appears only where a profile was matched. A tick box on a photograph whose lens the database has never heard of would be a control that looks available and does nothing, which is the failure the rule above names rather than an instance of following it — those photographs are told "· no profile" in words instead, and one whose box is unticked now says "· profile off", which is a third fact and not either of the other two. Two things had to be built underneath. `ParamKind::Bool` was in the core's closed enum and mapped to a row kind here, and had no control behind it in `adjust.slint`: a parameter declaring itself a switch was flattened into a row that drew nothing at all. Nothing shipped had one until now, so the gap cost nothing and was invisible. And `Check` self-toggled, which is right for a settings page that owns its value and wrong for a panel row that is a view of the edit graph — the click would have answered by replacing the binding with a literal, and the next undo or pasted preset would have moved the value with the tick left where the finger put it. It now takes `controlled`, and the generated row uses it. The manual sliders are unchanged and still trim whatever the profile leaves, so switching it off is "correct this by hand" rather than "stop correcting". |
||
|
dtourolle
|
901f51e6c4 |
Point at something grey and let the pipeline work out the rest
FR-DEV-3 has asked for "white balance (temperature/tint, and picker)" since it was written, and only the first half existed. `WidgetKind::WhitePoint` was in the vocabulary and `develop::supported` answered false for it, so the node degraded to two sliders — correct behaviour that had quietly become the only behaviour. Sampling a neutral is the first move of the global tonal pass and every colour judgement afterwards is measured against where the grey was put, so guessing at two sliders until a wall stops looking green is the wrong way round. The awkward part is that a picker genuinely needs to know how far a hundred units of temperature move red against blue, and that number is declared in the node's own file. So the inversion lives in `dr_pipeline::neutral` rather than in the interface: the canvas hands over a colour, the core finds the operation that asked to be driven by a pixel and bisects its declared response until the sample comes back grey. Nothing in `ui/` names white balance, and nothing holds a second copy of a response that would be wrong the first time somebody adjusted the range. A bisection rather than a closed-form inverse because only monotonicity is part of the bargain — the expression is free to become a table tomorrow. The result is rounded to the precision the control is drawn at, which is not cosmetic: unrounded, sampling something already neutral lands a ten-thousandth off zero, and the photograph comes back modified with an undo step for a correction of nothing. On the panel side this needed one distinction the generated path was missing. `is_on_canvas` was being read as "and so the panel draws nothing for it", which is right for a crop — four edge fractions are not controls anyone drags in a list — and wrong for an eyedropper, which *writes* temperature and tint and leaves them exactly the controls a photographer reaches for next. So a sampling widget keeps its sliders and puts the affordance that arms the canvas in the group's heading, built like the reset beside it. One click, one sample, one history step: `Edit::Action` never coalesces, and there is no hover preview to fill the stack with temperatures nobody chose. Declaring the presentation also groups temperature and tint under one undo step, where they were two. That follows from what `Presentation` means and reads correctly — white balance is one decision — but it is a change, and worth saying so. |
||
|
dtourolle
|
68ebf5d78b |
Let a mask start from a tone or a colour, not only a shape
Every local adjustment began from a shape: painted, drawn with a handle, or found by a model. So the only way to hold back a sky was to draw a line near where it ended, and the only way to warm skin was to paint round it — both of which put the edit's edge where the photographer put a gesture rather than where the picture changes. A gradient across a treeline halos, and an adjustment traced round a face stops on the outline of a hand. MaskSource grows two variants that select by what a pixel *is*. Luminance carries two bounds on the perceptual tone scale plus a softness; Colour carries an arc of hue, a range of chroma, and one softness for every edge of both. Five floats and three, so they diff, sync and merge per field under FR-NC-9 exactly as a gradient's geometry does — the property a stored raster has none of, and the reason the model's coverage had to sit beside its source rather than inside it. The pixels are the shader's business and nowhere else's. `mask.wgsl` takes the demosaiced source as a sixth binding and two new modes read it: decode, balance, pull a clipped photosite back to neutral, apply the camera matrix, then weigh the band. Nothing crosses to the CPU but the numbers and the matrix, and each mask texel averages its own footprint in the source, so a band lands on the tone an area is rather than on whichever texel a proxy grid happened to land on. The photograph it measures is the one the camera recorded, before this edit. A band over the edited result would slide out from under the edit as the edit was made — raising the highlights would change which pixels counted as highlights, and the slider would chase its own mask. Feather, falloff and morphology stay off a range layer, which is what `shapeable` already meant. All three are functions of the signed distance from a boundary, and a range has no boundary to be at a distance from; its edge is the softness of its own band, in the band's units. Offering them would be four controls that move and change nothing. |
||
|
dtourolle
|
81b1ae8c42 |
Measure the haze from the picture, and divide it back out
Four files named dehaze as a member of the compositional detail family — `detail.rs` twice, `dr-gpu`'s detail module, `ops/README.md` and `capture_sharpen.rs` — and no such node existed. Every one of them was describing the family by listing clarity, texture and a control the photographer could not reach. Haze is the one degradation the controls already in the chain cannot remove, and the reason is spatial rather than tonal. Scattering composites an airlight over the scene in proportion to distance, so the lift is per-pixel: a black point that clears the mountains crushes the foreground, and a contrast curve that clears the mountains does the same. So the node has to estimate the transmission at every pixel, which is the dark-channel prior — the local minimum over the channels and over a patch is the airlight that has been added there — and then invert the scattering model with it. The airlight is taken as neutral and as unit, which removes the one part of the published method this stage cannot perform. Estimating it properly is a whole-frame reduction, and the detail chain has none: it hands each pass the pass before it. It is also unnecessary, because white balance is the first node in the chain and has already driven the illuminant to grey, so only the magnitude is unknown — and an unknown magnitude on the veil is a scale factor on the amount slider, which the photographer is setting by eye regardless. The patch is a fraction of the frame's shorter edge, through `RenderScale::frame_fraction`, and never a count of pixels. It has to be wide enough to contain something dark and narrow enough that what it measures is still local, and both of those are statements about how much of the composition it covers — so it must cover the same proportion of the picture on a proxy as in the export, or the file is sharpened for a patch three times narrower than the one that was tuned on screen. Affording it needs an identity a Gaussian does not have. Erosions compose by adding their structuring elements, so the minimum over a run of d followed by the minimum over k points spaced d apart is the exact minimum over the whole kd window. At the square root that is 16 taps rather than 61 at 4K, and it is the same filter rather than an approximation of one — which is the difference from the strided kernel `local_contrast` refuses, where sampling an image that is not band-limited aliases into the base and comes back as mottling. It runs first among the compositional detail nodes, at order 125: after noise reduction, because dividing by a transmission below one amplifies the noise in the veiled distance by exactly the factor it recovers the contrast by, and before clarity and texture, coarse before fine, so that their base is computed on the picture the veil has left rather than on a modelling about to be divided out. What it cannot honour is the placement dehaze most wants. It shifts colour — it subtracts a grey term and rescales, so saturation changes wherever the veil is thick — and the colour work would ideally be correcting the picture that leaves here. The detail stage runs as a group after every point operation, because a neighbourhood pass is a separate dispatch over a texture the fused pass has finished writing, so an order placing this node ahead of `vibrance` would be a lie the chain cannot tell. Interleaving would mean splitting the fused pass in half around it, at the cost of a second full-frame dispatch and intermediate for every edit in the catalogue whether it dehazes or not. The declaration records that rather than leaving it to be rediscovered. FR-DEV-18 is added to the requirements register alongside it. The tag had nowhere to point, and an orphan tag fails the traceability gate rather than quietly counting for nothing. |
||
|
dtourolle
|
7c3e1d2c54 |
Let the shadows and the highlights carry a colour the picture never had
The colour mixer is the only chromatic control in the chain, and it can only turn a hue that is already in the frame. Ask it for cool shadows against warm highlights and it has nothing to take hold of: the shadows of a correctly balanced photograph are near enough neutral that there is no band there to turn, and a monochrome conversion hands it a picture with no hue in it at all. Split toning is the oldest look in the book and every developer worth comparing against ships it; there was no way to reach it from here. So colour_grading, declared like any other node — a hue and a strength for the shadows, the midtones and the highlights, and a global cast over the frame. It targets a tonal range rather than a hue, which is the whole difference between the two controls: it puts colour where none was rather than turning what it finds. It sits at 105, after the mixer has had the last word on the colours that are in the picture and before the detail stage. The mechanism is one helper. Three cosines 120 degrees apart are the hue wheel written directly as an RGB direction, and their sum is zero at every angle, so exp2 turns them into three gains whose product is exactly one — a cast tilts the balance without moving the level. A grade that doubled as an exposure change is the failure that has the photographer chasing brightness with a colour slider, and it is corrected with a control that cannot reach it. The three tonal weights partition the scale rather than overlapping, the midtones being whatever the two ends leave, so setting all three to one hue is exactly the global cast and a split tone does not colour its own midtones as a side effect of its halves meeting. Full strength is half a stop on the leading channel, the ceiling white balance already holds itself to. Neutral is declared rather than inferred, which is what `active:` is for. A hue with no strength behind it is a direction with no distance, so under the default rule nudging one would have put the node into every fused shader for a change nobody can see. Summing the strengths is zero exactly when all four are, and they cannot go negative to cancel each other. The opposite reading — neutral as "nothing has been touched" — fails the other way round: red is hue zero, so a grade toward red never moves a hue off its default and would never have been applied at all. It asks for a colour wheel, the widget the descriptor vocabulary has been carrying with no operation behind it. Nothing draws one yet, and that is fine by construction: the panel takes the first widget it implements and falls through to sliders otherwise, so this arrives as eight ordinary controls that work. Each parameter is named for its own range for exactly that reason — in a flat list, four sliders called "Hue" are four controls nobody can tell apart. FR-DEV-12 is written into requirements.md beside it. A TRACES tag naming a requirement that is not defined there is an orphan, and the traceability gate fails on those rather than quietly counting them. The label catalogue gets one line for the operation's display name; the eight parameters derive correctly and are left to. |
||
|
dtourolle
|
efa9d84aad |
Correct the lens first and settle the grain last
`Attribute::ALL` has claimed since it was written to be roughly the order a photographer works in, and |
||
 dtourolleandClaude Opus 5
|
e235e99cce |
Move the film to Effect in the descriptor that is actually read
An earlier commit claimed to move `film_sim` from `[tone, colour]` to `[effect]` and did not. It edited `ops/film_sim.yaml`, where `attributes:` is read, validated against the vocabulary, and then dropped: a `rust:` node publishes its own descriptor, and the type still said tone and colour. The stock went on appearing in the Light group beside exposure and again in Colour beside white balance, exactly as before, and every test passed. Nothing caught it because nothing could. The declaration parsed, the parity tests compare ids rather than attributes, and an operation filed under the wrong groups renders perfectly. It surfaced only on screen, as a missing Effects tab — which is indistinguishable from a category that genuinely has nothing in it, and is precisely how `Optics` looked for as long as it was empty. So three changes rather than one: `FilmSim`'s descriptor declares `Attribute::Effect`, which is the move the earlier commit described. `attributes:` joins the keys a `rust:` node may not carry, beside `params`, `uniforms`, `wgsl`, `helpers`, `define` and `label`. The rule was already written — "its descriptor comes from the type" — and attributes were the one field that slipped past it. A key that is silently ignored is worse than one that is rejected, because it reads as though it worked; the eight hand-written declarations lose a line that never did anything. And a test asserts that every attribute the chain carries reaches the tab strip. That is the property that was actually broken, and its failure mode is invisible from every direction: the controls exist, they are in the shader, and there is no way to filter to them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
474dcf0bf6 |
Let Compose lead the attributes, as their own doc always said
`Attribute::ALL` claims to be "roughly the order a photographer works in" and then listed framing fifth, behind tone, colour and detail. Framing is the first decision made about a photograph and the one every later judgement is made inside — there is no sense balancing tones across a frame about to lose a third of its width. The contradiction was harmless while the list only fed a row of chips nobody reads in order. It stops being harmless now that the same list drives a column read top to bottom. `declared::Attr::ALL` moves with it. The two are separate spellings of one vocabulary and a test asserts they agree, which is what caught this rather than the order silently disagreeing between the YAML front end and the crate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
601c984894 |
Fold a photograph's rival default versions back into one
Picking the newer of two default versions stopped the wrong edit being shown, but it did not close the split: the losing version stayed in the file, and a device holding disjoint work — a crop made here, an exposure change made there — still contributed only one of the two. Worse, the next write made it larger. `amend` looks its version up by uuid, neither of the two was ours, so the miss minted a *third* `default = 1` block and the file grew one rival per device per photograph. `Sidecar::fuse_default_versions` folds them down. The version with the highest `(revision, modified)` is the accumulator and every other default is merged into it as the remote, which is what makes the fold order-independent — `Version::merge` raises its own revision to `max + 1` as it goes, so merging a chain in ascending order stops being ascending after the first step and a third device would be dropped. Contested values resolve to the winner, disjoint keys survive from both sides because the merge is key-wise, and ratings come across under `merge_judgement`, so a device that never judged the frame cannot erase one that did. The result is a function of the file's bytes alone, so two devices that fuse independently reach the same document and converge instead of overwriting each other. Called wherever a sidecar is parsed: - `amend`, with the write's own uuid, so the fold lands on the identity this device is about to use and the lookup below it hits instead of missing. - `spawn_sidecar_fetch`, so opening a photograph shows everything done to it rather than whichever half won. - `drain_one`, because `merge_into` reconciles by uuid and would otherwise publish the split rather than resolve it. - `presets::load_local` and `save_local` — a local sidecar's folder may be synced by something else entirely, and gets the same split. A file with one default under the expected uuid comes back byte-identical, so this costs nothing on the ordinary write and no sidecar is uploaded merely for having been read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
98fcf8e98e |
Ask which edit is newer, not which uuid sorts first
A photograph edited on two devices ends up with two `[version]` blocks in one sidecar, both marked `default = 1`. Five of the thirty-eight sidecars in the local cache are in that state right now. `default_version` answered with the first `is_default` it met in map order, and the map is keyed on uuid — so which device's work the photographer saw was decided by which randomly minted uuid happened to sort lower. On `IMG_20130625_0033` that is `0545c20a` over `679fe872`: a four-star rating from the twenty-first of August standing in front of the one-star made on the thirtieth, with nothing anywhere saying the newer judgement existed. Resolved by `(revision, modified)` instead, which is the discriminator `Version::merge` already uses — revision first so that a device with a skewed clock cannot win by claiming a later timestamp (FR-NC-8), and the timestamp only to break an exact tie. This makes the reader pick the right one. It does not make the two converge: the edit that lost is still in the file, and a device that holds disjoint work — a crop here, an exposure change there — still only contributes one of them. That is the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
a56baa9042 |
Let rustfmt have the assertion it reflowed
The closure taken down to `&dyn Operation` needed its call site re-wrapped, and I wrapped it by hand rather than letting rustfmt decide: it fits on one line at the workspace width. `cargo clippy` was run on the change and `cargo fmt --check` was not, which is the whole of how it got through — the two catch different things and CI runs both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
2841eaf9a1 |
Take the layer-chain test's closure down to &dyn Operation
`clippy::borrowed_box` is denied by the workspace lint set, and the closure added with the optics exclusion took `&Box<dyn Operation>` — a borrow of the box rather than of the thing in it, which says nothing the plain trait object does not. Caught by `cargo clippy --workspace --all-targets -- -D warnings`, which is what CI runs and what the workspace tests do not: a lint on test code only appears when the tests are compiled as a clippy target. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
c4ddcbe0f7 |
Look the lens up and say plainly whether one was found
`dr-lens` has held a complete Lensfun lookup — distortion, TCA and vignetting coefficients from a lens name, a focal length and an aperture — with no dependents anywhere in the workspace. The three corrections it feeds now exist in the graph, so this connects the two and finishes the chain. The coefficient structs stay duplicated. `dr-pipeline` is organised around having no dependencies so its codegen is testable without a device or a database (ARCH §6.5a), and `dr-lens` carries an XML parser and 5.5 MB of profile data. Neither crate can convert to the other, so the conversion goes above both, in `develop.rs`, which is the only place that sees them together. Both traits grow the same defaulted door. The optical corrections do not sit on the same side of the fetch — distortion and CA rewrite coordinates and are `Warp`s, vignetting applies a gain to the pixel already there and is an ordinary node — and fanning a profile out by which trait each happens to implement would make the caller reason about that distinction. Each correction takes its own share of the whole profile instead, and `set_lens_profile` walks both lists identically. The lookup happens in `set_source_metadata` rather than in its caller, because that is the one place a session is told which file it came from. Doing it there makes it unforgettable, in the shape `FilmRebake` already uses for the other derived thing — and, more to the point, makes *clearing* unforgettable: a session that opened a second photograph while still holding the first one's profile would correct it for the wrong optics, invisibly, in a way that looks exactly like the lens. It needs the whole shot and not just a name. Distortion is interpolated across a zoom's focal range and vignetting depends strongly on aperture — a fast prime can be two stops down in the corners wide open and clean by f/8 — so a lookup missing either returns coefficients measured for a shot nobody took. Missing any of the three refuses rather than guesses. A profile is derived, not persisted: it comes from the file's EXIF and a database, so it is not a parameter, not in the sidecar and not undoable. What is an edit is the manual trim beside it, which each correction composes with the measurement — so a photographer can lean on it, override it, or work without one. `InfoPanel` gains a lens line, and it distinguishes three cases rather than two. `dr-lens` states the rule it exists for: an automatic correction that silently did nothing is worse than one the user can see is unavailable. A session with no header draws nothing, a header naming no lens reads "Lens not recorded", and a lens the database has never heard of reads "· no profile". Collapsing the last two would send somebody hunting for a profile that was never missing — which, for third-party and adapted glass, is the ordinary case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
a1165ef182 |
Put the coordinate-domain lens corrections into the graph
`lens.rs` has held a `Warp` trait, a composer and two implementations — distortion and lateral chromatic aberration — since they were written, and `compose_warps` was called by nothing outside its own tests. The corrections existed, were correct, and never touched a photograph. `EditGraph` now holds them, and `compose_full` emits them between the framing prologue and the fetch. Distortion first, then CA: each warp receives the position the previous one produced, and lateral CA is a magnification about the optical axis of the *undistorted* frame, so measured on a barrel-distorted one it would be fitted to a radius no profile describes. They reach the panel the way framing already does — through `capabilities`. That was the one open question and existing practice answered it: framing is also not an `Operation`, also has parameters a photographer sets, and also arrives through that list. Because `Preset::capture` walks the same list, the sidecar, the clipboard and the undo stack carry a warp's parameters with nothing registered anywhere, and no file under `ui/` names one (FR-DEV-3a). `state()` destructures `EditGraph` field by field precisely so that a new field cannot be forgotten, and it was not. Chromatic aberration is the only thing that samples per channel, and `splits_channels` is what keeps everything else from paying for it. Red and blue are fetched from positions green is not — green is the reference and never moves, so a wrong correction still leaves one channel sharp rather than softening all three. With no CA in the chain the single-fetch path is emitted instead. The interpolating sampler is now chosen by framing *or* an active warp. Asking framing alone would have nearest-neighboured a distortion correction on an unstraightened frame, and that aliasing reads as a bad profile rather than as a missing filter. The warps go in the geometry invalidation key rather than the colour one: they decide which source pixel a colour is read from, so a tile cached across a distortion change would keep drawing the previous correction. The pipeline cache needs nothing new — `hash_source` already covers the generated body, and uniform values never enter it, so arming a warp recompiles and dragging it does not. Both are asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
e17b909d41 |
Connect the lens vignetting correction to the pipeline
`ops/vignetting.rs` has carried a complete descriptor, polynomial, helper and test suite without an entry in `ops/`, so it was never in `chain()`. It reached no photograph and no panel, and `Attribute::Optics` was an empty category in consequence — filtered out of the tab strip for having no rows, by a chain that had never been given its only member. Declaring it needs the one thing the operation was written against and which did not exist. `wgsl_body` reads `radius`, and the module claimed "the composer publishes `radius` in the shader prologue for exactly this reason". It did not. `sample_source` now does, in both sampling branches, beside the `source_px` it already published for the same class of caller. It is corner-normalised there, which is the part that is easy to leave out. `p` spans ±0.5·aspect, so its length at the corner is 0.5·length(aspect) — about 0.901 on a 3:2 frame, not 1. Lensfun's polynomials are fitted against a corner radius of 1, so passing `length(p)` straight in evaluates every one of them short of where it was measured, by a factor that changes with the aspect ratio. It would have read as a correction that is simply too weak, which is indistinguishable from a bad profile. Both `lens.rs` and `framing.rs` asserted the normalisation `p` does not have; corrected. `order: 5` puts the correction ahead of the tonal stages, and the ordering is load-bearing rather than tidy. Recovering a corner means dividing by an attenuation below one — about two stops for a fast prime wide open — so run after the highlights have been rolled off and clipped, the lift has nowhere to go and the corners posterise instead of brightening. `layer_chain` now drops `Optics` as well as the neighbourhood operations. A local vignetting slider would have worked, which is what makes it worth excluding: `radius` measures from the centre of the whole photograph and a mask cannot move the optical axis, so it would lay a frame-centred radial ramp across the picture and multiply it by the mask. The existing exclusion covers operations that move and do nothing; this one covers an operation that moves and does something its name does not promise. The rule both share is now written down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
8e7b1350bf |
Name the frame's category for the decision, not the maths
`Attribute::Geometry` becomes `Attribute::Compose`, and `film_sim` moves from `[tone, colour]` to `[effect]`. Two categories were doing the wrong job. "Geometry" describes what crop, straighten and the quarter turns do to coordinates — but it describes lens distortion correction exactly as well, and that is not a compositional choice at all. Naming the attribute for the photographer's decision is what separates it from `Optics`: one is what the lens did, the other is what they chose. The maths the two have in common is not the thing worth filing them under. A film stock declared both `tone` and `colour`, so "Kodachrome" appeared in the Light group beside exposure and again in Colour beside white balance — two places, neither of which is where anyone looks for it. It is neither: `Effect` is defined in this same file as "applied rather than corrected — a look, not a fix", which is what a stock is. That it moves tone and colour is true of every look, and is not what the attribute is for. `from_name` still accepts "geometry" on the way in. That string is persisted in `develop.copy_attributes`, and an entry it fails to parse is not an error — `presets::scope_for` logs it and drops it — so without the alias an existing settings file would have quietly narrowed what a paste carries. `name` writes the current spelling, so the file migrates itself the first time it is saved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolle
|
89c4ff1820 |
Store what the model found, so a reopened photograph keeps its masks
A subject or category layer was written to the sidecar as identity alone —
which run, which instance, which category — on the reasoning that the pixels
are reproducible by running the same model over the same image. They are, but
only by *running the model*, and nothing runs one except a photographer
pressing "find subjects". So on every path that did not already have a run in
memory the layer resolved to no coverage, `MaskPass::render` logged "has no
distance field; skipping", and the adjustment was silently absent:
- reopening an edited photograph rendered it without its local adjustments,
and then saved that state back on the way out;
- a batch export from the grid could not have them at any point, because
`render_from_library` opens a session, applies a version and renders, and
there is no model anywhere on that path. Three hundred files written
without the edits their photographer made, over a log warning.
Neither failure announced itself. The generated shader still emits the layer's
block and the empty placeholder multiplies it by zero, so the result is a
well-formed frame that is simply missing an edit — `mask_is_stale` already
named the state and called it "not stale, just unrenderable".
The coverage now travels in the file, as one `coverage = w h levels payload`
line at the end of the layer's block.
Two levels, and that is not a compromise. The model hands out a byte per pixel
but `Shaped::build` measures its distance field from `coverage >= 128` and
throws the shoulder away on the first line; everything soft about the rendered
edge comes afterwards from the layer's feather and falloff, which are read off
the distance. So one bit per pixel is not an approximation of what the model
said — it is exactly the part of it that reaches a pixel, and the stored mask
renders the identical frame. Storing all 256 levels would have stored 1.7 MB
of bilinear interpolation to reconstruct a predicate, and would not even have
compressed: a model mask is a bilinear upsample of a coarse grid, so almost no
two adjacent bytes are alike. Measured on a simulated sky and a simulated
figure at 1600x1067, against 1.71 MB raw: 4.0 kB and 6.5 kB at two levels,
46 kB and 76 kB at sixteen, 835 kB and 1.43 MB at all 256. The level count is
still written into the line, so a later build that finds a use for the
shoulder can write sixteen and this one will read them rather than misreading
a stream of lengths as pairs.
The coder is hand-rolled — run-length pairs in a base-64 varint — because
`dr-pipeline` links nothing, which is the property that lets the descriptor
and codegen logic be tested without a device. `flate2` would have been fewer
lines and a dependency in the one crate that has none.
Where it lives matters more than how it is coded. The raster sits on
`MaskLayer` beside the source, not inside `MaskSource::Subject`: the source is
*identity*, which is what makes it diff as a handful of numbers and merge per
field under FR-NC-9, and a raster in there would have given the merge a binary
blob to arbitrate. It takes no part in `MaskLayer`'s equality for the same
reason — a device that has run the model and one that has not hold the same
edit, and counting the difference would raise a conflict over a cache and let
`remote_wins` answer it by discarding the only copy of the pixels.
Encoding happens in `masks_for_storage`, on the save path, rather than in
`ensure_subject_fields` where every coverage already funnels through.
`ensure_subject_fields` runs on a drag — dilating a mask with a compound
morphology rebuilds the field every frame — and encoding a megapixel raster
per frame is the kind of work NFR-P5 exists to keep off a gesture. Saving
happens once, when the photograph stops being the open one, and already costs
a network round trip.
Version skew holds both ways. A file with no `coverage` line reads exactly as
it did before, which is a layer that needs the model run; an unreadable one
costs the pixels and not the layer, because the layer is the edit and the
raster is a cache of it. An old build reading a new file drops the key it does
not understand, which costs a model run and no work. And a payload that will
not compress is refused rather than truncated: a checkerboard would encode to
twice the raster it came from, so past 64 kB nothing is stored and the
behaviour falls back to what it was — half a mask would render as a mask that
is confidently wrong, which is the failure that tells nobody.
|
||
|
dtourolleandClaude Opus 5
|
3d248cfb79 |
Export the photograph, not the canvas
Zooming the develop view changed the exported file. `Framing::view` is kept out of the sidecar, out of `is_active` and out of `output_size` precisely so that it cannot — but those exclusions keep it out of the *edit*, and an export is a *render*. `visible_rect` deliberately folds the view into the single rect the fused shader's prologue samples, so `render_for_export` inherited it: at 4:1 it wrote the middle of the frame, magnified to fill the file at the full output size, with the detail kernels scaled four times over because `render_scale` folds the view in as well. `render_thumbnail` did the same to the grid. `render_uncropped` already suspends the view for this exact reason, so the fix is its pattern: one `render_the_file` that both file-producing paths go through, composing inside the suspension since the view reaches the shader as a uniform baked at composition time. Restored whatever happens — leaving the graph un-zoomed after a failed export would throw away where the photographer was looking. Nothing caught it because the guard checked the wrong things. `zooming_does_not_change_the_exported_image` asserted the output size and the crop; both held perfectly throughout. Renamed to `zooming_does_not_change_the_size_or_the_crop`, which is what it tests, and the pixels are now guarded where pixels exist. The new test uses a ramp rather than quadrants deliberately: a four-quadrant frame is self-similar under a centred zoom, and the first version of this test passed against the bug because of it. Traces FR-EXP-9, which asks for the full-quality pipeline "regardless of what the display was showing". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dtourolleandClaude Opus 5
|
4efab496c2 |
Put the refinement on a slider, per layer
Benchmarks / CPU and I/O (per commit) (push) Successful in 3m27s
Benchmarks / Frame budget (on demand) (push) Skipped
🐳 Android image / Build and push (push) Successful in 3s
Build and test / android-image (push) Successful in 3s
Build and test / Desktop (Linux) (push) Failing after 1h14m30s
Build and test / Layer separation (push) Successful in 43s
Traceability / Requirement traces (push) Failing after 46s
Build and test / Android (aarch64) (push) Successful in 1h8m56s
The evidence was being gathered and spent immediately at one strictness nobody could see or change. This makes it a control. `MaskLayer::refine` is shaping, like the feather, and it lives on the layer for the layer's reason: two layers may sit on the same category and want different amounts of it, and the model ran once for both. ## What the segmentation now stores `CategorySummary` keeps the **coarse** mask and the `Refinement` beside it, rather than a refined mask. That is what gives the control an off position that is bit-for-bit the model's own weighting, and what stops a strictness change from needing the model again. `category_mask_at` borrows at zero and wherever no refinement could be fitted, so a layer nobody has touched costs nothing over the old path. The fit moved to the far side of the orientation permutation. The verdict is a per-pixel field over the same grid as the mask it gates, so fitting it upright would mean permuting a proxy-sized buffer afterwards to match — a second rotation, and a second chance to get one wrong. One logit cell is the same number of pixels either way: the letterbox scales by the longer edge and a permutation does not change which edge that is. The two frames became a `Frames` struct rather than six parameters. This function reads the picture twice for opposite purposes — the model needs it upright or it recognises far less, the refinement needs the sensor's grid — and a transposed pair produces a plausible mask over slightly the wrong pixels, which is the failure this module is most prone to. ## It rebuilds the field, and the signature says so `refine` is mixed into `subject_signature`. Unlike a feather, which is read off a field that is already correct, this changes which pixels are in the mask at all — so it changes the coverage the field is measured from. Omitting it is the bug where the slider moves and nothing happens until some unrelated control invalidates the cache. That puts it in the same cost class as a close or an open, which is why the row takes `SliderRow::changed` — already once-per-gesture, since that row takes `SliderTrack`'s `committed` internally — rather than a live stream. The slider is offered only where there is something to move: a category source, *and* a refinement the frame actually gave enough to fit. A control that moves and does nothing is worse than an absent one. ## Two defaults that are deliberately different A layer added from the panel starts at 4.0, because a category's edges are twenty proxy pixels wide before anything is done to them and a photographer adding a sky mask wants the sky rather than the sky plus every chimney in it. A layer read from a sidecar with no `refine` key starts at **zero**. A file written before this control existed has to render as it did then, and a default of 4 on absence would quietly re-grade every stored category mask in the catalogue. `a_categorys_refine_strictness_survives_and_defaults_off` holds both halves, and `an_out_of_range_refine_is_clamped` holds the file to the scale — past the top of it every colour fails and the mask deletes itself, which reads as lost work rather than as a bad file. `MAX_REFINE` is dr-pipeline's own constant mirroring `dr_segment::STRICTNESS_MAX`, following `Falloff` and `Morphology`: this crate holds the description of an edit and must not depend on the crate that runs a model. dr-ui is where the two meet, and the only place that converts. Verified: fmt clean, clippy --workspace -D warnings clean, 488 dr-pipeline and 60 dr-segment tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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 |
||
|
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>
|
||
|
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> |
||
|
dtourolle
|
91fe6cf300 |
Merge master into partial-preset-scope
🐳 Android image / Build and push (push) Successful in 8s
Build and test / android-image (push) Successful in 8s
Build and test / Desktop (Linux) (push) Failing after 1h17m12s
Build and test / Layer separation (push) Successful in 56s
Traceability / Requirement traces (push) Successful in 1m33s
Build and test / Android (aarch64) (push) Successful in 1h0m38s
# Conflicts: # docs/traceability.md # ui/dr-ui/ui/app.slint |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
dtourolleandClaude Opus 5
|
5133e53bc8 |
Give back what a straighten took, when the angle comes back
Build and test / Desktop (Linux) (push) Successful in 2h16m6s
Build and test / Layer separation (push) Successful in 52s
🐳 Android image / Build and push (push) Successful in 4s
Build and test / android-image (push) Successful in 4s
Traceability / Requirement traces (push) Successful in 51s
Build and test / Android (aarch64) (push) Successful in 47m10s
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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>
|
||
|
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> |
||
|
dtourolleandClaude Opus 5
|
d7a81375ee |
List the steps, and let a photographer step straight to one
Build and test / Desktop (Linux) (push) Successful in 19m30s
Build and test / Layer separation (push) Successful in 25s
🐳 Android image / Build and push (push) Successful in 1s
Build and test / android-image (push) Successful in 1s
Traceability / Requirement traces (push) Successful in 23s
Build and test / Android (aarch64) (push) Failing after 33m5s
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
dtourolleandClaude Opus 5
|
4a82753d22 |
Show the detector the photograph, not the sensor's scanlines
🐳 Android image / Build and push (push) Successful in 1s
Build and test / android-image (push) Successful in 1s
Build and test / Desktop (Linux) (push) Successful in 19m7s
Build and test / Layer separation (push) Successful in 25s
Traceability / Requirement traces (push) Successful in 23s
Build and test / Android (aarch64) (push) Failing after 33m10s
"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> |