Fix/rotation recalc compound transforms (#1083)
* Fix rotation recalculation corrupting compound transforms Two bugs in the rotation center recalculation triggered by attribute changes on rotated elements: 1. The recalculation fires for ALL attribute changes (stroke-width, fill, opacity, etc.) even though only geometric attributes (x, y, width, height, d, points, etc.) can change the bbox center. This causes unnecessary and incorrect rotation center updates. 2. In undo.js, the center is computed through ALL remaining transforms via transformListToTransform(tlist).matrix, but for compound transform lists like [translate(tx,ty), rotate(angle)], the pre-rotation translate leaks into the center calculation, producing rotate(angle, bcx+tx, bcy+ty) instead of the correct rotate(angle, bcx, bcy). 3. In history.js (undo/redo), the code replaces the ENTIRE transform attribute with just rotate(...), completely destroying any other transforms (translate, scale, etc.) in the list. Fixes: - Guard recalculation with a BBOX_AFFECTING_ATTRS set so non-geometric attribute changes skip the rotation block entirely. - In undo.js, use transformListToTransform(tlist, n, max) to compute the center through only post-rotation transforms. - In history.js, replace string-based transform replacement with transform list API that finds and updates only the rotation entry, preserving all other transforms. Extract shared logic into relocateRotationCenter() helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add tests for rotation recalculation with compound transforms Tests demonstrate and verify fixes for three bugs: 1. Non-geometric attributes (stroke-width, fill, opacity) no longer trigger rotation center recalculation — the transform list is left untouched. 2. Geometric attribute changes on elements with compound transforms (e.g. translate + rotate) compute the rotation center using only post-rotation transforms, preventing the translate from leaking into the center calculation. 3. ChangeElementCommand.apply/unapply preserve compound transform structure instead of replacing the entire transform attribute with just rotate(...). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add visual demo and screenshot for rotation recalculation bug Includes an HTML demo showing both bugs side-by-side: - Bug 1: Non-geometric attribute changes (stroke-width) on compound transforms corrupt character positions (e.g., word art on curves) - Bug 2: Rotation center computed through ALL transforms instead of only post-rotation transforms, causing translate to leak into center Each panel shows Original / Buggy / Fixed comparisons with the actual SVG transform math applied. A Playwright script generates screenshots. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add sample SVG demonstrating compound transform corruption bug A pinwheel of 6 colored rectangles using translate(200,200) rotate(N). Loading this into SVG-Edit and changing stroke-width on the group triggers the rotation recalculation bug on unfixed builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix standard linting: object properties on separate lines Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Centralize BBOX_AFFECTING_ATTRS and fix bbox-before-remove safety Address two code review comments: 1. Export BBOX_AFFECTING_ATTRS from history.js and import it in undo.js so the attribute list is defined in one place. 2. Compute getBBox() before calling tlist.removeItem() so that a falsy bbox causes an early return/break without having already destroyed the rotation transform. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Geoff Youngs
@@ -8,6 +8,61 @@
|
||||
|
||||
import { NS } from './namespaces.js'
|
||||
import { getHref, setHref, getRotationAngle, getBBox } from './utilities.js'
|
||||
import { getTransformList, transformListToTransform, transformPoint } from './math.js'
|
||||
|
||||
// Attributes that affect an element's bounding box. Only these require
|
||||
// recalculating the rotation center when changed.
|
||||
export const BBOX_AFFECTING_ATTRS = new Set([
|
||||
'x', 'y', 'x1', 'y1', 'x2', 'y2',
|
||||
'cx', 'cy', 'r', 'rx', 'ry',
|
||||
'width', 'height', 'd', 'points'
|
||||
])
|
||||
|
||||
/**
|
||||
* Relocate rotation center after a bbox-affecting attribute change.
|
||||
* Uses the transform list API to update only the rotation entry,
|
||||
* preserving compound transforms (translate, scale, etc.).
|
||||
* @param {Element} elem - SVG element
|
||||
* @param {string[]} changedAttrs - attribute names that were changed
|
||||
*/
|
||||
function relocateRotationCenter (elem, changedAttrs) {
|
||||
const hasBboxChange = changedAttrs.some(attr => BBOX_AFFECTING_ATTRS.has(attr))
|
||||
if (!hasBboxChange) return
|
||||
|
||||
const angle = getRotationAngle(elem)
|
||||
if (!angle) return
|
||||
|
||||
const tlist = getTransformList(elem)
|
||||
let n = tlist.numberOfItems
|
||||
while (n--) {
|
||||
const xform = tlist.getItem(n)
|
||||
if (xform.type === 4) { // SVG_TRANSFORM_ROTATE
|
||||
// Compute bbox BEFORE removing the rotation so we can bail out
|
||||
// safely if getBBox returns nothing (avoids losing the rotation).
|
||||
const box = getBBox(elem)
|
||||
if (!box) return
|
||||
|
||||
tlist.removeItem(n)
|
||||
|
||||
// Transform bbox center through only post-rotation transforms.
|
||||
// After removeItem(n), what was at n+1 is now at n.
|
||||
let centerMatrix
|
||||
if (n < tlist.numberOfItems) {
|
||||
centerMatrix = transformListToTransform(tlist, n, tlist.numberOfItems - 1).matrix
|
||||
} else {
|
||||
centerMatrix = elem.ownerSVGElement.createSVGMatrix() // identity
|
||||
}
|
||||
const center = transformPoint(
|
||||
box.x + box.width / 2, box.y + box.height / 2, centerMatrix
|
||||
)
|
||||
|
||||
const newrot = elem.ownerSVGElement.createSVGTransform()
|
||||
newrot.setRotate(angle, center.x, center.y)
|
||||
tlist.insertItemBefore(newrot, n)
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Group: Undo/Redo history management.
|
||||
@@ -344,17 +399,7 @@ export class ChangeElementCommand extends Command {
|
||||
|
||||
// relocate rotational transform, if necessary
|
||||
if (!bChangedTransform) {
|
||||
const angle = getRotationAngle(this.elem)
|
||||
if (angle) {
|
||||
const bbox = getBBox(this.elem)
|
||||
if (!bbox) return
|
||||
const cx = bbox.x + bbox.width / 2
|
||||
const cy = bbox.y + bbox.height / 2
|
||||
const rotate = ['rotate(', angle, ' ', cx, ',', cy, ')'].join('')
|
||||
if (rotate !== this.elem.getAttribute('transform')) {
|
||||
this.elem.setAttribute('transform', rotate)
|
||||
}
|
||||
}
|
||||
relocateRotationCenter(this.elem, Object.keys(this.newValues))
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -388,17 +433,7 @@ export class ChangeElementCommand extends Command {
|
||||
})
|
||||
// relocate rotational transform, if necessary
|
||||
if (!bChangedTransform) {
|
||||
const angle = getRotationAngle(this.elem)
|
||||
if (angle) {
|
||||
const bbox = getBBox(this.elem)
|
||||
if (!bbox) return
|
||||
const cx = bbox.x + bbox.width / 2
|
||||
const cy = bbox.y + bbox.height / 2
|
||||
const rotate = ['rotate(', angle, ' ', cx, ',', cy, ')'].join('')
|
||||
if (rotate !== this.elem.getAttribute('transform')) {
|
||||
this.elem.setAttribute('transform', rotate)
|
||||
}
|
||||
}
|
||||
relocateRotationCenter(this.elem, Object.keys(this.oldValues))
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
*/
|
||||
import * as draw from './draw.js'
|
||||
import * as hstry from './history.js'
|
||||
import { BBOX_AFFECTING_ATTRS } from './history.js'
|
||||
import {
|
||||
getRotationAngle, getBBox as utilsGetBBox, setHref, getStrokedBBoxDefaultVisible
|
||||
} from './utilities.js'
|
||||
@@ -239,26 +240,39 @@ export const changeSelectedAttributeNoUndoMethod = (attr, newValue, elems) => {
|
||||
svgCanvas.selectorManager.requestSelector(elem).resize()
|
||||
}, 0)
|
||||
}
|
||||
// if this element was rotated, and we changed the position of this element
|
||||
// we need to update the rotational transform attribute
|
||||
// Only recalculate rotation center for attributes that change element geometry.
|
||||
// Non-geometric attributes (stroke-width, fill, opacity, etc.) don't affect
|
||||
// the bbox center, so the rotation is already correct and must not be touched.
|
||||
// BBOX_AFFECTING_ATTRS is imported from history.js to keep the list in one place.
|
||||
const angle = getRotationAngle(elem)
|
||||
if (angle !== 0 && attr !== 'transform') {
|
||||
if (angle !== 0 && attr !== 'transform' && BBOX_AFFECTING_ATTRS.has(attr)) {
|
||||
const tlist = getTransformList(elem)
|
||||
let n = tlist.numberOfItems
|
||||
while (n--) {
|
||||
const xform = tlist.getItem(n)
|
||||
if (xform.type === 4) {
|
||||
// remove old rotate
|
||||
// Compute bbox BEFORE removing the rotation so we can bail out
|
||||
// safely if getBBox returns nothing (avoids losing the rotation).
|
||||
const box = utilsGetBBox(elem)
|
||||
if (!box) break
|
||||
|
||||
tlist.removeItem(n)
|
||||
|
||||
const box = utilsGetBBox(elem)
|
||||
// Transform bbox center through only the transforms that come
|
||||
// AFTER the rotation in the list (not the pre-rotation transforms).
|
||||
// After removeItem(n), what was at n+1 is now at n.
|
||||
let centerMatrix
|
||||
if (n < tlist.numberOfItems) {
|
||||
centerMatrix = transformListToTransform(tlist, n, tlist.numberOfItems - 1).matrix
|
||||
} else {
|
||||
centerMatrix = svgCanvas.getSvgRoot().createSVGMatrix() // identity
|
||||
}
|
||||
const center = transformPoint(
|
||||
box.x + box.width / 2, box.y + box.height / 2, transformListToTransform(tlist).matrix
|
||||
box.x + box.width / 2, box.y + box.height / 2, centerMatrix
|
||||
)
|
||||
const cx = center.x
|
||||
const cy = center.y
|
||||
|
||||
const newrot = svgCanvas.getSvgRoot().createSVGTransform()
|
||||
newrot.setRotate(angle, cx, cy)
|
||||
newrot.setRotate(angle, center.x, center.y)
|
||||
tlist.insertItemBefore(newrot, n)
|
||||
break
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user