mirror of
https://github.com/penpot/penpot.git
synced 2026-08-23 21:28:38 +00:00
🐛 Fix floating-point equality issues in path editing
Replace exact equality checks with tolerance-based comparisons in path editing functions to handle floating-point rounding differences after transforms, rotations, or curve fitting. Changes: - distribute-content: Round coordinates to 0.1 precision before grouping to ensure coincident nodes move together - separate-node: Use gpt/close? instead of exact equality to find nodes with floating-point imprecision - collision-step: Use mth/close? for tolerance-based comparison to detect paste collisions correctly - resolve-edit-fills: Add cycle detection to prevent infinite loops with corrupted parent chains Made collision-step, available-offset-step, and resolve-edit-fills public for better testability. Added comprehensive tests for all fixes covering both exact and floating-point coordinate scenarios. AI-assisted-by: qwen3.7-plus
This commit is contained in:
parent
6fedea0f4d
commit
7770aa807b
@ -946,7 +946,7 @@
|
|||||||
(let [cmd (nth content i)
|
(let [cmd (nth content i)
|
||||||
nxt (nth content (inc i) nil)
|
nxt (nth content (inc i) nil)
|
||||||
at-p? (and (not= :close-path (:command cmd))
|
at-p? (and (not= :close-path (:command cmd))
|
||||||
(= point (helpers/segment->point cmd)))]
|
(gpt/close? point (helpers/segment->point cmd)))]
|
||||||
(cond
|
(cond
|
||||||
;; Offset a subpath start.
|
;; Offset a subpath start.
|
||||||
(and at-p? (= :move-to (:command cmd)))
|
(and at-p? (= :move-to (:command cmd)))
|
||||||
|
|||||||
@ -9,6 +9,7 @@
|
|||||||
(:require
|
(:require
|
||||||
[app.common.data :as d]
|
[app.common.data :as d]
|
||||||
[app.common.geom.point :as gpt]
|
[app.common.geom.point :as gpt]
|
||||||
|
[app.common.math :as mth]
|
||||||
[app.common.types.path.helpers :as helpers]
|
[app.common.types.path.helpers :as helpers]
|
||||||
[app.common.types.path.impl :as impl]))
|
[app.common.types.path.impl :as impl]))
|
||||||
|
|
||||||
@ -180,29 +181,33 @@
|
|||||||
(let [content (vec content)
|
(let [content (vec content)
|
||||||
indices (set indices)
|
indices (set indices)
|
||||||
entries (selected-node-entries content indices)
|
entries (selected-node-entries content indices)
|
||||||
|
index->point (into {} entries)
|
||||||
horizontal? (= axis :horizontal)
|
horizontal? (= axis :horizontal)
|
||||||
coord (fn [p] (if horizontal? (:x p) (:y p)))
|
coord (fn [p] (if horizontal? (:x p) (:y p)))
|
||||||
groups (->> entries
|
groups (->> entries
|
||||||
(group-by (fn [[_ p]] [(:x p) (:y p)]))
|
(group-by (fn [[_ p]] [(mth/round (:x p) 0.1) (mth/round (:y p) 0.1)]))
|
||||||
(mapv (fn [[_ es]]
|
(mapv (fn [[_ es]]
|
||||||
{:point (second (first es))
|
{:point (second (first es))
|
||||||
:indices (mapv first es)})))]
|
:indices (mapv first es)})))
|
||||||
|
sorted (sort-by (comp coord :point) groups)]
|
||||||
(if (< (count groups) 3)
|
(if (< (count groups) 3)
|
||||||
(impl/from-plain content)
|
(impl/from-plain content)
|
||||||
(let [sorted (sort-by (comp coord :point) groups)
|
(let [lo (coord (:point (first sorted)))
|
||||||
lo (coord (:point (first sorted)))
|
|
||||||
hi (coord (:point (last sorted)))
|
hi (coord (:point (last sorted)))
|
||||||
step (/ (- hi lo) (dec (count sorted)))
|
step (/ (- hi lo) (dec (count sorted)))
|
||||||
deltas (into {}
|
deltas (into {}
|
||||||
(comp
|
(comp
|
||||||
(map-indexed
|
(map-indexed
|
||||||
(fn [k {:keys [point indices]}]
|
(fn [k {:keys [point indices]}]
|
||||||
(let [target (+ lo (* k step))
|
(let [target (+ lo (* k step))]
|
||||||
d (- target (coord point))
|
(map (fn [i]
|
||||||
|
(let [node-point (get index->point i)
|
||||||
|
d (- target (coord node-point))
|
||||||
dp (if horizontal?
|
dp (if horizontal?
|
||||||
(gpt/point d 0)
|
(gpt/point d 0)
|
||||||
(gpt/point 0 d))]
|
(gpt/point 0 d))]
|
||||||
(map (fn [i] [i dp]) indices))))
|
[i dp]))
|
||||||
|
indices))))
|
||||||
cat)
|
cat)
|
||||||
sorted)]
|
sorted)]
|
||||||
(impl/from-plain (translate-nodes content indices deltas))))))
|
(impl/from-plain (translate-nodes content indices deltas))))))
|
||||||
|
|||||||
@ -1564,6 +1564,18 @@
|
|||||||
(t/is (= (pts content)
|
(t/is (= (pts content)
|
||||||
(pts (path/distribute-content content #{1 2 3} :horizontal))))))
|
(pts (path/distribute-content content #{1 2 3} :horizontal))))))
|
||||||
|
|
||||||
|
(t/deftest segment-distribute-content-with-floating-point-coordinates
|
||||||
|
(t/testing "distribute-content groups nodes with floating-point rounding differences"
|
||||||
|
(let [content (path/content
|
||||||
|
[{:command :move-to :params {:x 0.0 :y 0.0}}
|
||||||
|
{:command :line-to :params {:x 3.0001 :y 7.0}}
|
||||||
|
{:command :line-to :params {:x 3.0002 :y 7.0}}
|
||||||
|
{:command :line-to :params {:x 10.0 :y 0.0}}])
|
||||||
|
pts (fn [c] (mapv (comp (juxt :x :y) :params) (vec c)))]
|
||||||
|
;; Nodes at ~3.0 should be grouped together
|
||||||
|
(t/is (= [[0.0 0.0] [5.0 7.0] [5.0 7.0] [10.0 0.0]]
|
||||||
|
(pts (path/distribute-content content #{0 1 2 3} :horizontal)))))))
|
||||||
|
|
||||||
(t/deftest helpers-curve-arc-length-t
|
(t/deftest helpers-curve-arc-length-t
|
||||||
(let [arc-len (fn [curve a b]
|
(let [arc-len (fn [curve a b]
|
||||||
(->> (range 1001)
|
(->> (range 1001)
|
||||||
@ -1865,6 +1877,18 @@
|
|||||||
;; separate-nodes should return a collection (vector or seq)
|
;; separate-nodes should return a collection (vector or seq)
|
||||||
(t/is (coll? result))))
|
(t/is (coll? result))))
|
||||||
|
|
||||||
|
(t/deftest segment-separate-nodes-with-floating-point-coordinates
|
||||||
|
(t/testing "separate-nodes finds nodes with floating-point rounding differences"
|
||||||
|
(let [content (path/content
|
||||||
|
[{:command :move-to :params {:x 0.0 :y 0.0}}
|
||||||
|
{:command :line-to :params {:x 10.0001 :y 0.0}}
|
||||||
|
{:command :line-to :params {:x 20.0 :y 0.0}}])
|
||||||
|
pt (gpt/point 10.0002 0.0)
|
||||||
|
result (path.segment/separate-nodes content #{pt})]
|
||||||
|
;; Should still find and separate the node
|
||||||
|
(t/is (coll? result))
|
||||||
|
(t/is (> (count result) (count content))))))
|
||||||
|
|
||||||
(t/deftest segment-make-corner-point
|
(t/deftest segment-make-corner-point
|
||||||
(let [content (path/content sample-content-2)
|
(let [content (path/content sample-content-2)
|
||||||
;; Take a curve point and make it a corner
|
;; Take a curve point and make it a corner
|
||||||
|
|||||||
@ -60,18 +60,18 @@
|
|||||||
|
|
||||||
(def ^:private paste-offset (gpt/point 10 10))
|
(def ^:private paste-offset (gpt/point 10 10))
|
||||||
|
|
||||||
(defn- collision-step
|
(defn collision-step
|
||||||
"Returns the non-negative paste-offset step that makes two nodes coincide."
|
"Returns the non-negative paste-offset step that makes two nodes coincide."
|
||||||
[pasted existing]
|
[pasted existing]
|
||||||
(let [delta (gpt/subtract existing pasted)
|
(let [delta (gpt/subtract existing pasted)
|
||||||
x-step (/ (:x delta) (:x paste-offset))
|
x-step (/ (:x delta) (:x paste-offset))
|
||||||
y-step (/ (:y delta) (:y paste-offset))]
|
y-step (/ (:y delta) (:y paste-offset))]
|
||||||
(when (and (not (neg? x-step))
|
(when (and (not (neg? x-step))
|
||||||
(= x-step y-step)
|
(mth/close? x-step y-step)
|
||||||
(= x-step (mth/floor x-step)))
|
(mth/close? x-step (mth/floor x-step)))
|
||||||
(long x-step))))
|
(long (mth/round x-step)))))
|
||||||
|
|
||||||
(defn- available-offset-step
|
(defn available-offset-step
|
||||||
"Returns the first paste-offset step with no node collisions."
|
"Returns the first paste-offset step with no node collisions."
|
||||||
[existing pasted]
|
[existing pasted]
|
||||||
(let [blocked
|
(let [blocked
|
||||||
|
|||||||
@ -867,20 +867,26 @@
|
|||||||
|
|
||||||
(declare stop-path-edit)
|
(declare stop-path-edit)
|
||||||
|
|
||||||
(defn- resolve-edit-fills
|
(defn resolve-edit-fills
|
||||||
"Resolves the fills inherited by the editing copy.
|
"Resolves the fills inherited by the editing copy.
|
||||||
Frames stop group fill inheritance."
|
Frames stop group fill inheritance."
|
||||||
[shape objects]
|
[shape objects]
|
||||||
(let [own (svg-fills/resolve-shape-fills shape)]
|
(let [own (svg-fills/resolve-shape-fills shape)]
|
||||||
(if (seq own)
|
(if (seq own)
|
||||||
own
|
own
|
||||||
(loop [parent-id (:parent-id shape)]
|
(loop [parent-id (:parent-id shape)
|
||||||
|
visited #{}]
|
||||||
|
(cond
|
||||||
|
(nil? parent-id) []
|
||||||
|
(visited parent-id) []
|
||||||
|
:else
|
||||||
(let [parent (get objects parent-id)]
|
(let [parent (get objects parent-id)]
|
||||||
(cond
|
(cond
|
||||||
(nil? parent) []
|
(nil? parent) []
|
||||||
(cfh/group-shape? parent) (svg-fills/resolve-shape-fills parent)
|
(cfh/group-shape? parent) (svg-fills/resolve-shape-fills parent)
|
||||||
(cfh/frame-shape? parent) []
|
(cfh/frame-shape? parent) []
|
||||||
:else (recur (:parent-id parent))))))))
|
:else (recur (:parent-id parent)
|
||||||
|
(conj visited parent-id)))))))))
|
||||||
|
|
||||||
(defn start-path-edit
|
(defn start-path-edit
|
||||||
[id]
|
[id]
|
||||||
|
|||||||
@ -11,6 +11,7 @@
|
|||||||
[app.common.test-helpers.shapes :as cths]
|
[app.common.test-helpers.shapes :as cths]
|
||||||
[app.main.data.shortcuts :as dsc]
|
[app.main.data.shortcuts :as dsc]
|
||||||
[app.main.data.workspace :as dw]
|
[app.main.data.workspace :as dw]
|
||||||
|
[app.main.data.workspace.path.edition :as path.edition]
|
||||||
[app.main.data.workspace.path.shortcuts :as psc]
|
[app.main.data.workspace.path.shortcuts :as psc]
|
||||||
[app.main.data.workspace.selection :as dws]
|
[app.main.data.workspace.selection :as dws]
|
||||||
[app.main.data.workspace.shortcuts :as wsc]
|
[app.main.data.workspace.shortcuts :as wsc]
|
||||||
@ -76,3 +77,21 @@
|
|||||||
(t/deftest test-enter-toggles-path-editing-mode
|
(t/deftest test-enter-toggles-path-editing-mode
|
||||||
(doseq [shape-type [:rect :circle :path :image]]
|
(doseq [shape-type [:rect :circle :path :image]]
|
||||||
(run-scenario shape-type)))
|
(run-scenario shape-type)))
|
||||||
|
|
||||||
|
(t/deftest resolve-edit-fills-with-normal-parent-chain
|
||||||
|
(t/testing "resolve-edit-fills resolves fills from parent chain"
|
||||||
|
(let [objects {1 {:type :group :parent-id 2 :fills [{:fill-color "#ff0000"}]}
|
||||||
|
2 {:type :frame :parent-id nil :fills []}}
|
||||||
|
shape {:type :path :parent-id 1 :fills []}
|
||||||
|
result (path.edition/resolve-edit-fills shape objects)]
|
||||||
|
;; Should inherit fills from group parent
|
||||||
|
(t/is (= [{:fill-color "#ff0000"}] result)))))
|
||||||
|
|
||||||
|
(t/deftest resolve-edit-fills-with-circular-parent-chain
|
||||||
|
(t/testing "resolve-edit-fills handles circular parent references gracefully"
|
||||||
|
(let [objects {1 {:type :rect :parent-id 2 :fills []}
|
||||||
|
2 {:type :rect :parent-id 1 :fills []}}
|
||||||
|
shape {:type :path :parent-id 1 :fills []}
|
||||||
|
result (path.edition/resolve-edit-fills shape objects)]
|
||||||
|
;; Should return empty fills instead of infinite loop
|
||||||
|
(t/is (= [] result)))))
|
||||||
|
|||||||
@ -202,3 +202,38 @@
|
|||||||
store done events
|
store done events
|
||||||
(fn [new-state]
|
(fn [new-state]
|
||||||
(t/is (empty? (page-paths new-state))))))))
|
(t/is (empty? (page-paths new-state))))))))
|
||||||
|
|
||||||
|
(t/deftest collision-step-with-exact-coordinates
|
||||||
|
(t/testing "collision-step detects collision with exact coordinates"
|
||||||
|
(let [pasted (gpt/point 10.0 10.0)
|
||||||
|
existing (gpt/point 20.0 20.0)
|
||||||
|
step (path.clipboard/collision-step pasted existing)]
|
||||||
|
;; Should detect collision at step 1
|
||||||
|
(t/is (some? step))
|
||||||
|
(t/is (= 1 step)))))
|
||||||
|
|
||||||
|
(t/deftest collision-step-with-floating-point-coordinates
|
||||||
|
(t/testing "collision-step detects collision with floating-point rounding differences"
|
||||||
|
(let [pasted (gpt/point 10.0 10.0)
|
||||||
|
existing (gpt/point 20.001 20.002)
|
||||||
|
step (path.clipboard/collision-step pasted existing)]
|
||||||
|
;; x-step = 1.0001, y-step = 1.0002
|
||||||
|
;; With tolerance, these should be considered equal
|
||||||
|
(t/is (some? step))
|
||||||
|
(t/is (= 1 step)))))
|
||||||
|
|
||||||
|
(t/deftest available-offset-step-with-exact-coordinates
|
||||||
|
(t/testing "available-offset-step finds first available step with exact coordinates"
|
||||||
|
(let [existing #{(gpt/point 20.0 20.0)}
|
||||||
|
pasted #{(gpt/point 10.0 10.0)}
|
||||||
|
step (path.clipboard/available-offset-step existing pasted)]
|
||||||
|
;; Should find step 0 (no collision at step 0)
|
||||||
|
(t/is (= 0 step)))))
|
||||||
|
|
||||||
|
(t/deftest available-offset-step-with-floating-point-coordinates
|
||||||
|
(t/testing "available-offset-step finds first available step with floating-point rounding differences"
|
||||||
|
(let [existing #{(gpt/point 20.0001 20.0002)}
|
||||||
|
pasted #{(gpt/point 10.0001 10.0002)}
|
||||||
|
step (path.clipboard/available-offset-step existing pasted)]
|
||||||
|
;; Should detect collision at step 1 and return step 0 as available
|
||||||
|
(t/is (= 0 step)))))
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user