fix: make colour profiles and compositing correct - #50
Conversation
4159515 to
1a885f0
Compare
IAmJSD
left a comment
There was a problem hiding this comment.
This review was written by Claude (Fable 5), acting on Astrid's behalf.
Four real bugs, fixed well: built-in profiles serialize so Assign/Convert actually tag the document, profile changes ride the history, group fill opacity reaches the isolated and pass-through paths, and adjustment layers honour their blend mode — with the CPU blend_pixel path and the GPU blend_px shader in verified parity and all seven tests failing on main as they should. Two blockers before this can land:
-
Undo of a profile edit never reaches the screen (
crates/core/src/document.rs:427+crates/app/src/workspace.rshistory_jump): the newEditOp::IccProfileSetarm restoresicc_profilebut records no damage and no revision bump (compare theSelectionSetarm directly above it), andhistory_jumpnever callsrebuild_color_transforms— that's only done inreset_per_document_caches,assign_profile,convert_to_profileandtoggle_proof. So: convert sRGB → Display P3, press Ctrl+Z — the restored sRGB pixels keep rendering through the cached P3→display transform for the rest of the session; undo a pure Assign and the canvas doesn't repaint at all. The headline "undoable" feature is broken at the view layer. Fix: rebuild transforms + damage inhistory_jumpwhen the edit touched the profile. -
The PR body claims a soft-proofing fix that isn't in the diff: "the display hop was built with the document profile as its source" — but at this head
to_displaystill applies doc→proof then doc→display viaColorSettings::transform_for, so Proof Colors still double-converts whenever display ≠ document profile. Onlytoggle_proof's toggle-vs-switch semantics changed. Either include the fix or correct the description.
Smaller items:
set_icc_profilewas spliced betweenchange_selectionand its doc comment (document.rs:793), so the docs now describe the wrong function;ColorTransform::applysimilarly carries two stitched summary lines (colormgmt/src/lib.rs:195).toggle_proofcompares proof profiles byname(), andProfile::from_bytesnames every embedded profile "Embedded profile" — switching between two loaded device profiles would turn proofing off instead of switching. Latent today (the only caller passesProfile::srgb()), but worth an identity check.- On a plain PSD open→save round-trip the writer now relocates the ICC block to a synthesized one at the end of the resource section (reader always mirrors ICC into
doc.icc_profile, so the skip always fires). Byte-identical content, changed layout — writing it in place of the preserved block would keep the file stable.
Happy to re-review once the undo repaint and the description/diff mismatch are sorted.
d67c674 to
04bb109
Compare
|
fixed |
IAmJSD
left a comment
There was a problem hiding this comment.
This follow-up review was written by Claude (Fable 5), acting on Astrid's behalf.
Five of the six findings are cleanly fixed with meaningful regression tests: IccProfileSet now damages the canvas symmetrically (with a test that fails on the old code), the soft-proof hop moved here and is single-hop-correct (the_display_hop_starts_where_the_proof_hop_ended proves it), toggle_proof compares bytes, the doc-comment splices are gone, and the PSD writer now substitutes the ICC bytes in place so a round-trip is byte-identical. Nice work.
One thing keeps this from merging — the blocker fix only covers the history-panel buttons, not Ctrl+Z:
history_jump now checks the profile and calls rebuild_color_transforms(), but Ctrl+Z / Cmd+Z and the Edit menu are bound to the plugin command edit.undo (plugins/commands-core/src/lib.rs:418 — a bare ctx.doc.undo()), dispatched through Workspace::run_command (workspace.rs:2891), which ends with after_change and never rebuilds transforms. So the original repro still works verbatim on the keyboard path: convert sRGB → Display P3, press Ctrl+Z — apply_op restores the tag and damages everything, after_change drops the display tiles, and they're rebuilt through the still-cached P3→display transform. The same profile-before/after check history_jump got needs to run in run_command (or centralize it in after_change, which both paths share — that would also future-proof any other command that touches the profile).
One latent nit in the new code, non-blocking: in transform_for's proofing branch (colormgmt/src/lib.rs:279), if proof_transform fails to build (.ok() → None) the proof hop is silently skipped while the display hop still starts from the proof profile — document pixels through the wrong source hop. Unreachable today (only sRGB is offered as proof), but a CMYK proof device would hit it; degrading proofing off as a unit would be safer.
Fix the Ctrl+Z path and this merges.
groups two branches: colour management and compositing correctness.
profile changes were wrong and not undoable
soft proofing converted twice
the display hop was built from the document profile, but with proofing on the pixels have already been through the proof hop. proof colors therefore ran the document-to-display conversion a second time over pixels that had left that space, and people make colour decisions against that view. the second hop now starts where the first ended.
toggle_proofalso compares profiles rather than names, so switching between two embedded profiles switches instead of turning proofing off.compositing ignored two things it should not have
cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace— 632 passed, 0 failed.how this relates to my other open prs
these seven are independent of each other — each branches off
mainand each is green on its own. they do share files with my five open prs (#36, #38, #42, #44, #46), mostlyworkspace.rs, so whichever lands first will leave the others needing a rebase. happy to rebase in whatever order suits you, or to split any of these further if one is too big to review in a sitting.