fix: canvas tool correctness, modifiers and missing controls - #52
fix: canvas tool correctness, modifiers and missing controls#52ProdigyRahul wants to merge 12 commits into
Conversation
be31d4d to
3dd2003
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.
The majority of this 10-commit PR is correct, well-tested fixes for real bugs — the mask-tile undo fix, bezier direction-only mirroring, guides/artboards riding crop/resize, brush flow/spacing/pressure dynamics with pixel-level tests, multi-stop ramp_at, the replace_layer_tiles FxHashSet fix, and red-eye selection gating all check out. But several of the fixes introduce new, verified bugs, two of which break core interactions in ordinary use. Requesting changes.
Blockers:
-
Transform pivot jump on the second drag of a session (
plugins/tools-transform/src/lib.rs:416):on_pointer_downre-anchorspivot_anchorper drag without compensatingoffset, so the accumulated transform is re-interpreted about the new pivot. Scale 2× with the right handle, release, then just click inside the box to move — the layer instantly jumps by half its original width. The added test only does single-drag sessions, so it can't see this. -
Empty wand-Replace leaves a "select nothing" trap (
plugins/tools-select/src/lib.rs:21,63): the new empty-Replacepath runsdeselect()then unconditionallysel.activate()with an all-zero mask. Sinceis_empty()is just!active, every brush, fill, gradient and retouch tool then silently does nothing document-wide until the user manually Deselects. The intent (empty replace clears the selection) needsdeselect()withoutactivate().
Major:
VanishingPointTool::on_deactivate(perspective.rs:414) callsself.on_commit(ctx)— but the tool never implementson_commit(strokes commit inon_pointer_up), so it dispatches to the ToolPlugin trait's empty default. The "fix" is a no-op with a comment claiming otherwise;self.strokeisn't even cleared, and switching tools mid-stroke still bakes preview pixels with no history entry — the exact bug commit 0250055 claims to fix.transform_mask(crates/core/src/resample.rs:421,448) samples the raw tile map (0 for missing tiles) instead ofLayerMask::valuesemantics (default_valueoutsidebounds). For a revealing mask this (a) densely allocates all-zero tiles across the whole clip on Image Size / Free Transform (~140 MB of garbage on a 12k×12k doc with a fresh empty mask) and (b) darkens a bilinear fringe along the old bounds. The added mask-resize test asserts on rawtiles.valuefor an effectively no-op mask, so it verifies the wrong semantics.- The elliptical marquee clips the drag rect to canvas before inscribing the ellipse (
tools-select/src/lib.rs:270), so a partly off-canvas drag commits a differently-proportioned ellipse than the preview showed — clip the mask, not the generating rect. - The feather box-blur rewrite (
crates/core/src/selection.rs:570-600) replaces the O(1)-per-pixel sliding window with a per-pixel radius loop — O(w·h·r), ~150× more work at radius 250, and it runs on every marquee commit with feather set. The fractional-radius fix only needed two extra taps on the existing window. on_cancelof an alt-duplicate drag (tools-basic/src/lib.rs:272) removes the copy viaLayerTree::remove, which doesn't cleardoc.active_layer— Esc mid-alt-drag leaves the active layer pointing at a deleted id and painting/move/transform silently no-op. The adjacent comment describesremove_layer, which is never called.- Exposure/Strength are applied as
a * x * 2.0(tools-paint/src/lib.rs:400): above 50%, Burn writes negative RGB unclamped into F32 tiles and Blur/Sharpen extrapolates past the convolved value (ringing). Clamp the factor to 1 or use a non-extrapolating model.
Minor: duplicated near-identical comment paragraphs in tools-transform (~486, ~545) and a fused doc-comment in liquify.rs:131 (render's docs lost); paintable_layer's "the caller reports it" comment is false — locked/hidden/non-raster layers now silently no-op with no feedback; the Alt scale-from-centre branch keeps the old half-speed handle rate the commit claims to fix; and three PR-body claims aren't implemented (vanishing point still ignores locked/selection, free transform still ignores the selection, map_geometry's doc cites rotate/flip commands that aren't wired to it). MoveTool also gets no on_deactivate, so an in-flight alt-duplicate leaks an unrecorded layer on tab-switch — the same session-scoping class this PR fixes elsewhere.
Findings 1–3 are quick, surgical fixes; 4 and 6 need a bit more care. With those addressed this merges — the underlying work is good.
d1d4105 to
0ca4063
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.
This was a strong round: all twelve findings were addressed and ten are cleanly fixed with meaningful tests — the empty-Replace trap, transform_mask default-value semantics (with the old test corrected to assert the right thing), the sliding-window feather with a direct-sum comparison at fractional radii, the vanishing-point finish_stroke, the alt-duplicate cancel restoring the active layer, tone clamping, the live-pivot scale rate, and the previously-unimplemented PR-body claims (locked/selection-aware vanishing point, selection-scoped free transform, map_geometry wired to rotate/flip, MoveTool::on_deactivate). Three verified items remain, all small:
-
Major —
repivotinverts the linear part in the wrong order (plugins/tools-transform/src/lib.rs:232-245): the compensation counter-rotates first, then divides by scale (the comment even says "Undo the rotation, then the scale"), butmatrix()composes scale-then-rotate, so the inverse must divide by scale first, then counter-rotate. Rotation-free repivots are exact (and the new multi-drag test covers those), but scale one axis with an edge handle, rotate, then press inside to move — the layer jumps (~17 px on a 40 px box at 2×/45°). Surgical fix: swap the two steps; add a rotated repivot test. -
Major — 90° canvas rotation destroys guides (
crates/core/src/document.rs:843): the guide branch maps the position but never swapsguide.horizontal, which the two 90° ops require. Under Cw90(x,y)→(h−y,x), a horizontal guide atposmaps(0,pos)→(h−pos,0)and keeps the y component — every horizontal guide collapses to position 0, every vertical guide lands atx = h. This branch was safe for its pre-existing axis-preserving callers (crop/resize); wiring it to Cw90/Ccw90 in this round exposed it. Flips and 180° are fine. The mapping needs an orientation swap for the 90° cases, plus a guide-rotation test. -
Minor — corner-sliver marquee recreates the select-nothing trap (
plugins/tools-select/src/lib.rs:289+Selection::clip_to): an elliptical drag whose bounding rect clips the canvas only at a corner the ellipse never reaches (e.g. (190,190)→(400,400) on a 200×200 canvas) passes the rect-intersect guard,apply_shapesetsactive = true, thenclip_tozeroes everything — the exact active-all-zero state finding 2 just eliminated, and with Replace it also wipes the prior selection. Deselect when the mask is empty afterclip_to.
One doc nit: lift's "reproduces the layer exactly" holds only for hard edges — feathered selections recombine with a slightly translucent seam (accepted float-composite semantics, but the comment overclaims).
With those three, this merges — the underlying work here is the best of the series.
… on canvas (cherry picked from commit 474a015)
(cherry picked from commit 94d5994)
(cherry picked from commit 3279f81)
(cherry picked from commit b3e795a)
…orms (cherry picked from commit 65375d3)
…rrectly (cherry picked from commit 1070c12)
(cherry picked from commit f490433)
(cherry picked from commit f52f5e4)
(cherry picked from commit c0e149e)
(cherry picked from commit 38fac8f)
Quarter turns swap guide orientation via map_geometry's new swap_guide_axes flag; repivot inverts the composed linear map in the right order; neural upscales rescale masks and document geometry like the classical path; an elliptical marquee that only clips the canvas with its bounding box deselects instead of leaving an all-zero mask. (re-applied from the review changes in merge ef18ea4, which this rebase flattens)
ef18ea4 to
6e4e336
Compare
|
Rebased onto current main (
Workspace build and full test suite pass locally (1423 tests). |
groups ten branches: the canvas tools.
tools acting on the wrong thing
selection maths
feather was quantised to
floor(radius / √3), so small radii did nothing and 2 px equalled 3 px;bordersat entirely outside the edge for odd widths; marquees were not clipped to the canvas.gestures that did not exist
shift-constrain and alt-duplicate on the move tool, and alt-from-centre for the marquee. the brush had no Flow, no adjustable spacing, and pressure changed only the dab's size.
alt-dragging with the move tool, and what the layers panel holds afterwards:
the gradient ramp was two colours
five styles but no way to put a third colour anywhere, and no per-stop opacity. the ramp is a stop list now; the two swatch-driven fills behave exactly as they did.
dodge and burn also gain Range and Exposure, blur and sharpen a Strength, and the smudge brush's mix stops being hard-coded — all defaulting to what those paths already applied.
previews that lied
the elliptical marquee previewed a rectangle:
the rotate handle's standoff was in document units, so at 800% it sat three screen pixels off the box and at 10% it floated 240 away; the brush cursor ignored pressure and was never cleared; the polygonal lasso had no way to drop an anchor; bring forward at the top of the stack recorded a no-op edit;
replace_layer_tileswas quadratic in tile count.one merge decision worth flagging
two branches independently added
on_deactivateto the paint tool — one cancelling the in-progress stroke, one committing it. i kept commit: a stroke is pixels the user already painted, switching tools should not throw it away, andfinishrecords it as one undoable edit.cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace— 667 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.