github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-07-23 22:48:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Fix propagation of geometric changes to rotated component copies (#10574)

Browse Source 
* 🐛 Fix geometry sync between mains and rotated component copies

Rotating a copy instance as a whole marked every shape inside it as
touched for geometry, so later geometric changes in the main (e.g. a
resize) were no longer propagated to that copy, while non-geometric
ones (e.g. fills) still were. And on paths where geometry did get
written to a rotated copy (e.g. resetting overrides), the sync engine
compensated only the roots' position delta, so the written values wiped
the copy's rotation back to 0.

Model the instance root's transformation as inherited, overridable
content, asymmetric to position (which remains free per-instance
placement):

- An untouched copy follows the main's transformation verbatim,
  including rotation and flips (preserving the BUG #13267 semantics
  that rotating a main propagates to its copies).

- Transforming a copy as a whole overrides only its ROOT: check-delta
  compares the root's rotation/flips absolutely, but the descendants
  relative to their root, so they merely follow and stay untouched.

- When a copy root's geometry is overridden, update-attrs expresses the
  main's geometry in the copy's own frame: reposition-shape applies the
  roots' relative transformation (rotation/flips) around the dest root
  center in addition to the position delta. Geometric changes from the
  main then keep propagating to the rotated copy, landing correctly in
  its rotated frame instead of destroying its placement.

Covered by the new composable test case
case-n-geometry-sync-with-rotated-instances: an 8-variant sweep over
optional copy rotation, optional main rotation, and one of a fills or
height edit on the main child, asserting the whole model through the
real workspace events (the new rotate operation dispatches
dwt/increase-rotation, whose apply-modifiers step runs the check-delta
classification under test; change-height dispatches
dwt/update-dimensions and implements IPropertyCheck so one-of sweeps
can mix property and geometry edits). Verified by temporarily reverting
the fix: the case then fails with 6 assertion failures and passes again
with the fix restored.

Fixes #10109

AI-assisted-by: claude-fable-5

* 🐛 Fix synchronization problems

---------

Co-authored-by: alonso.torres <alonso.torres@kaleidos.net>
This commit is contained in:
Dr. Dominik Jain 2026-07-21 10:05:31 +02:00 committed by GitHub
parent 27392abd49
commit 4ea56e0b89
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 271 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
common/src/app/common/logic/libraries.cljc
View File

@ -2505,15 +2505,49 @@
(pcb/concat-changes changes new-changes)))
(defn- reposition-shape
[shape origin-root dest-root]
(let [shape-pos (fn [shape]
(gpt/point (get-in shape [:selrect :x])
(get-in shape [:selrect :y])))
"Expresses the shape (belonging to the origin-root instance) in the frame of the
dest-root instance, making the geometry of both instances directly comparable —
and copyable.
origin-root-pos (shape-pos origin-root)
dest-root-pos (shape-pos dest-root)
delta (gpt/subtract dest-root-pos origin-root-pos)]
(gsh/move shape delta)))
If the dest root's geometry is NOT overridden (touched), the instance follows
the origin's transformation verbatim (including rotation and flips), so a
translation by the roots' (untransformed) position delta suffices — position is
free per-instance placement.
If the dest root's geometry IS overridden (e.g. the user rotated the copy as a
whole), the instance keeps its own placement transform, so the origin shape is
additionally transformed by the roots' relative transformation (rotation /
flips) around the dest root center — geometric changes then land expressed in
the dest instance's own frame instead of wiping its placement."
[shape origin-root dest-root]
(let [shape-pos (fn [shape]
(gpt/point (dm/get-in shape [:selrect :x])
(dm/get-in shape [:selrect :y])))
origin-root-pos (shape-pos origin-root)
dest-root-pos (shape-pos dest-root)
delta (gpt/subtract dest-root-pos origin-root-pos)
shape (gsh/move shape delta)]
(if-not (ctk/touched-group? dest-root :geometry-group)
shape
(let [origin-transform (d/nilv (:transform origin-root) (gmt/matrix))
dest-transform (d/nilv (:transform dest-root) (gmt/matrix))
rel-transform (gmt/multiply dest-transform (gmt/inverse origin-transform))]
(if ^boolean (gmt/unit? rel-transform)
shape
;; The roots differ in rotation/flips: rotate the whole (already moved)
;; shape around the dest root center by the roots' relative transform.
;; The :rotation attribute delta is fed through the :modifiers path so
;; apply-transform keeps it consistent with the resulting matrix.
(let [center (grc/rect->center (:selrect dest-root))
rel-rotation (mod (- (d/nilv (:rotation dest-root) 0)
(d/nilv (:rotation origin-root) 0))
360)]
(-> shape
(assoc-in [:modifiers :rotation] rel-rotation)
(gsh/apply-transform (gmt/transform-in center rel-transform))
(dissoc :modifiers))))))))
(defn- make-change
[container change]

62
frontend/src/app/main/data/workspace/modifiers.cljs
View File

@ -117,28 +117,54 @@
;; geometric attributes of the shapes.
(defn- check-delta
"If the shape is a component instance, check its relative position and rotation respect
the root of the component, and see if it changes after applying a transformation."
[shape root transformed-shape transformed-root]
(let [shape-delta
(when root
(gpt/point (- (gsh/left-bound shape) (gsh/left-bound root))
(- (gsh/top-bound shape) (gsh/top-bound root))))
"If the shape is a component instance, check whether the transformation is a
free change that must not mark the shape as touched.
transformed-shape-delta
(when transformed-root
(gpt/point (- (gsh/left-bound transformed-shape) (gsh/left-bound transformed-root))
(- (gsh/top-bound transformed-shape) (gsh/top-bound transformed-root))))
For the instance ROOT, position is free placement, but its rotation and flips
are inherited content: transforming the copy as a whole overrides them, so a
change there marks the root as touched.
For DESCENDANTS, position, size, rotation and flips are compared RELATIVE TO
THE ROOT: a transformation of the whole instance preserves them all and does
not mark the descendants as touched; editing an individual shape does."
[shape root transformed-shape transformed-root]
(let [is-root? (and (some? root)
(= (dm/get-prop shape :id) (dm/get-prop root :id)))
center (fn [shape]
(grc/rect->center (:selrect shape)))
rel-pos (fn [shape root]
(when root
;; vector from the root center to the shape center,
;; expressed in the root's local (untransformed) axes
(-> (gpt/subtract (center shape) (center root))
(gpt/transform (:transform-inverse root (gmt/matrix))))))
orientation (fn [shape root]
;; the shape's rotation/flip orientation taken from its
;; transform matrix, so it is reliable regardless of how the
;; transform was applied (the WASM apply-transform path does
;; not refresh the :rotation attribute). For the ROOT the
;; absolute orientation; for a DESCENDANT the orientation
;; relative to the root — invariant when the whole instance
;; is rotated or flipped as a unit.
(let [t (:transform shape (gmt/matrix))]
(if is-root?
t
(gmt/multiply (:transform-inverse root (gmt/matrix)) t))))
pos-before (rel-pos shape root)
pos-after (rel-pos transformed-shape transformed-root)
distance
(if (and shape-delta transformed-shape-delta)
(gpt/distance-vector shape-delta transformed-shape-delta)
(if (and pos-before pos-after)
(gpt/distance-vector pos-before pos-after)
(gpt/point 0 0))
rotation-delta
(if (and (some? (:rotation shape)) (some? (:rotation shape)))
(- (:rotation transformed-shape) (:rotation shape))
0)
orientation-unchanged?
(gmt/close? (orientation shape root)
(orientation transformed-shape transformed-root))
selrect (:selrect shape)
transformed-selrect (:selrect transformed-shape)]
@ -152,7 +178,7 @@
(and (and (< (:x distance) 1) (< (:y distance) 1))
(mth/close? (:width selrect) (:width transformed-selrect))
(mth/close? (:height selrect) (:height transformed-selrect))
(mth/close? rotation-delta 0))))
orientation-unchanged?)))
(defn calculate-ignore-tree
"Retrieves a map with the flag `ignore-geometry?` given a tree of modifiers"

84
frontend/test/frontend_tests/composable_tests/comp/nodes.cljs
View File

@ -26,15 +26,19 @@
[app.common.data :as d]
[app.common.files.changes-builder :as pcb]
[app.common.files.helpers :as cfh]
[app.common.geom.point :as gpt]
[app.common.geom.shapes :as gsh]
[app.common.logic.libraries :as cll]
[app.common.logic.shapes :as cls]
[app.common.logic.variants :as clv]
[app.common.math :as mth]
[app.common.test-helpers.components :as thc]
[app.common.test-helpers.compositions :as tho]
[app.common.test-helpers.files :as thf]
[app.common.test-helpers.ids-map :as thi]
[app.common.test-helpers.shapes :as ths]
[app.common.types.container :as ctn]
[app.common.types.modifiers :as ctm]
[frontend-tests.composable-tests.core :as tm]))
;; ---------------------------------------------------------------------------
@ -127,6 +131,86 @@
(def ^{:doc "Alias of `has-property-of`."} has-attr? has-property-of)
(def ^{:doc "Alias of `applied-property`."} applied-attr applied-property)
;; ---------------------------------------------------------------------------
;; Geometry operations
;;
;; Unlike `change-property` (a raw attribute write), geometric changes must go
;; through the TRANSFORM pipeline: on the frontend, the interpreter dispatches
;; the real sidebar events (`dwt/increase-rotation`, `dwt/update-dimensions`),
;; whose apply-modifiers step also runs the placement-vs-override classification
;; for component copies (calculate-ignore-tree / check-delta) — which is part of
;; what these operations exist to exercise. The synchronous `apply-to` fallback
;; below performs the geometrically equivalent transform through the production
;; math, but cannot classify placement (that code is frontend-only), so cases
;; using these operations are meant to run through the interpreter.
(defrecord Rotate [target angle]
tm/IOperation
(apply-to [this situation]
(let [the-file (tm/file situation)
shape-id (tm/target-shape-id situation target)
page (thf/current-page the-file)
objects (:objects page)
shape (get objects shape-id)
;; rotating a container rotates its whole subtree, as the real event does
ids (into #{shape-id} (cfh/get-children-ids objects shape-id))
center (gsh/shape->center shape)
rotate1 (fn [s] (gsh/transform-shape s (ctm/rotation-modifiers s center angle)))
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
ids
rotate1
objects
{})
file' (thf/apply-changes the-file changes)]
(-> situation
(tm/with-file file')
(tm/record-application this {:target target :angle angle})))))
(defn rotate
"Constructor for the rotation operation: rotate the shape named by `target`
(a role, a label, or a `(situation -> id)` fn) — including its whole subtree —
by `angle` degrees around its center. On the frontend this dispatches the real
`dwt/increase-rotation` event."
[target angle]
(tm/assign-id (->Rotate target angle)))
(defrecord ChangeHeight [target value]
tm/IOperation
(apply-to [this situation]
(let [the-file (tm/file situation)
shape-id (tm/target-shape-id situation target)
page (thf/current-page the-file)
objects (:objects page)
shape (get objects shape-id)
resize1 (fn [s] (gsh/transform-shape
s
(ctm/resize-modifiers (gpt/point 1 (/ value (:height s)))
(gpt/point (:x s) (:y s)))))
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id shape)}
resize1
objects
{})
file' (thf/apply-changes the-file changes)]
(-> situation
(tm/with-file file')
(tm/record-application this {:target target :value value})))))
(defn change-height
"Constructor for the height-change operation: resize the shape named by `target`
to height `value` (width unchanged). On the frontend this dispatches the real
`dwt/update-dimensions` event. Implements `IPropertyCheck`, so a `one-of` over
property and geometry edits can assert uniformly via `has-property-of`."
[target value]
(tm/assign-id (->ChangeHeight target value)))
(extend-type ChangeHeight
IPropertyCheck
(applied-property [_node] :height)
(applied-value [node] (:value node))
(has-property-of [node shape]
(mth/close? (:value node) (:height shape))))
;; ===========================================================================
;; Synchronisation-scenario building blocks
;;

87
frontend/test/frontend_tests/composable_tests/comp/sync_test.cljs
View File

@ -27,6 +27,10 @@
Async: each deftest uses `t/async`; `ftm/check` drives the store and calls
`done` when finished."
(:require
[app.common.geom.matrix :as gmt]
[app.common.geom.point :as gpt]
[app.common.geom.rect :as grc]
[app.common.math :as mth]
[app.common.types.component :as ctk]
[app.common.types.shape-tree :as ctst]
[cljs.test :as t :include-macros true]
@ -343,4 +347,87 @@
(t/is (= (expected-at s i) (level-color s m i))
(str "level " i)))))]))}))))
(t/deftest case-n-geometry-sync-with-rotated-instances
;; Regression sweep for #10109, with #13267's semantics as its complement: an
;; instance root's transformation is inherited, overridable content —
;; asymmetric to position, which is free per-instance placement.
;;
;; On the simple component-with-copy, sweep three axes: optionally rotate the
;; COPY as a whole (an override of its root's placement — must not block
;; propagation), optionally rotate the MAIN as a whole (inherited content —
;; an untouched copy must follow it), and apply ONE of a property edit
;; (fills) or a geometry edit (height) to the main child. In EVERY variant
;; the chosen edit must arrive at the copy child; the copy's rotation is its
;; own 45° if the copy was rotated (override wins), else the main's 45° if
;; the main was rotated (clean copy follows), else 0; and only a rotated
;; copy's ROOT is touched (:geometry-group) — its child merely follows and
;; stays untouched.
(t/async
done
(let [rotate-copy (n/rotate :copy-root 45)
rotate-main (n/rotate :main-root 45)
edits (tm/one-of
[(n/change-attr :main-instance :fills red)
(n/change-height :main-instance 80)])]
(ftm/check
done
{:setup setup/simple-component-with-labeled-copy
:operation (tm/in-sequence [(tm/optional rotate-copy)
(tm/optional rotate-main)
edits])}
(fn [situation]
(let [chosen (tm/get-choice situation edits)
copy-root (setup/copy-root situation)
copy-child (setup/copy-instance situation)
expected-rotation (cond
(tm/applied? situation rotate-copy) 45
(tm/applied? situation rotate-main) 45
:else 0)]
;; the chosen main edit arrived at the copy child, in every variant
(t/is (some? chosen))
(t/is (n/has-property-of chosen copy-child))
;; the copy shows the expected rotation (own override > followed main > none)
(t/is (mth/close? (or (:rotation copy-root) 0) expected-rotation))
(t/is (mth/close? (or (:rotation copy-child) 0) expected-rotation))
;; only a rotated copy's ROOT is an override; the child merely follows
(t/is (= (if (tm/applied? situation rotate-copy) #{:geometry-group} nil)
(:touched copy-root)))
(t/is (nil? (:touched copy-child)))))))))
(t/deftest case-touched-copy-child-survives-main-rotation
;; A copy whose ROOT is rotated (a geometry override) AND whose CHILD has its
;; own geometry override (resized) must keep the child exactly in place when the
;; MAIN is later rotated: the child's placement is its own, shielded by its
;; touched :geometry-group. We capture the child's position relative to the copy
;; root before the main rotation and assert it is unchanged after.
(t/async
done
(let [before (atom nil)
rel-pos (fn [situation]
(let [child (setup/copy-instance situation)
root (setup/copy-root situation)]
(-> (gpt/subtract (grc/rect->center (:selrect child))
(grc/rect->center (:selrect root)))
(gpt/transform (:transform-inverse root (gmt/matrix))))))
capture (tm/test-that (fn [s] (reset! before (rel-pos s)) (t/is (some? @before))))]
(ftm/check
done
{:setup setup/simple-component-with-labeled-copy
:operation (tm/in-sequence [(n/rotate :copy-root 30)
(n/change-height :copy-instance 60)
capture
(n/rotate :main-root 40)])}
(fn [situation]
(let [copy-child (setup/copy-instance situation)
copy-root (setup/copy-root situation)
after (rel-pos situation)]
;; both overrides are recorded as geometry touches
(t/is (contains? (:touched copy-root) :geometry-group))
(t/is (contains? (:touched copy-child) :geometry-group))
;; the child keeps its own overridden height
(t/is (mth/close? (:height copy-child) 60))
;; and its position relative to the copy root is unchanged by the main rotation
(t/is (mth/close? (:x @before) (:x after) 0.5) (str "rel-x " (:x @before) " -> " (:x after)))
(t/is (mth/close? (:y @before) (:y after) 0.5) (str "rel-y " (:y @before) " -> " (:y after)))))))))

14
frontend/test/frontend_tests/composable_tests/interpreter.cljs
View File

@ -44,6 +44,7 @@
[app.main.data.workspace.libraries :as dwl]
[app.main.data.workspace.shapes :as dwsh]
[app.main.data.workspace.thumbnails :as dwth]
[app.main.data.workspace.transforms :as dwt]
[app.main.data.workspace.undo :as dwu]
[app.main.data.workspace.variants :as dwv]
[app.main.store :as st]
@ -140,6 +141,19 @@
[(dwsh/update-shapes #{(tm/target-shape-id situation target)}
(fn [shape] (n/set-property shape property value)))])
(instance? n/Rotate op)
;; The real sidebar rotation event, absolute mode: rotates the shape (and its
;; whole subtree) around its center. Its apply-modifiers step runs the
;; placement-vs-override classification for component copies (check-delta),
;; which is part of what a case using `rotate` exercises.
(let [{:keys [target angle]} op]
[(dwt/increase-rotation [(tm/target-shape-id situation target)] angle)])
(instance? n/ChangeHeight op)
;; The real sidebar dimension event.
(let [{:keys [target value]} op]
[(dwt/update-dimensions [(tm/target-shape-id situation target)] :height value)])
(instance? n/SwapComponent op)
;; Swap lineage `name`'s nesting level `level` for lineage `target`'s component
;; via the REAL swap event (dwl/component-swap), so it commits through the