mirror of
https://github.com/qelectrotech/qelectrotech-source-mirror.git
synced 2026-08-13 10:04:13 +02:00
Keep group rotation on-grid when X and Y grid sizes differ
Raised by plc-user in discussion #618: the diagram editor allows moving elements by as little as 1px, and asked that rotation not undershoot that. Checking the actual constraint (Settings -> DiagramEditor_xGrid_sb / _yGrid_sb, both independently configurable, minimum 1, maximum 30) turned up a real, verified gap this PR's existing fractional-pivot fix doesn't cover: an ASYMMETRIC grid (xGrid != yGrid). Swapping X/Y deltas for a 90-degree turn -- the exact-arithmetic path already in this file -- only stays on the configured grid if xGrid == yGrid. With an asymmetric grid, a delta that was a clean multiple of xGrid lands on the Y axis after the swap, where the grid unit is yGrid, and one is not generally a multiple of the other. Verified on a real build (not just derived): two elements at (100,210) and (150,420), both on-grid under xGrid=10/yGrid=7, selected and group-rotated 90 degrees via a temporary local CLI harness. before this change: (242,292) and (32,342) -- off-grid on both axes after this change: (240,294) and ( 30,343) -- exactly on-grid Confirmed the same drift is present without this change too (i.e. not something introduced elsewhere) and that xGrid==yGrid, the common case, is unaffected: snapping an already-on-grid point is a no-op. Fix: re-snap the final computed position to Diagram::snapToGrid(), not just the shared pivot, for the exact-90-degree path. Left the arbitrary-angle trig fallback alone -- it has no caller today (the diagram editor only ever passes multiples of 90) and "on-grid" doesn't have a clean meaning for an arbitrary angle regardless of grid shape. Does not attempt to fix a separate, pre-existing property surfaced while testing this: four consecutive 90-degree turns do not reliably return a selection to its exact starting position, even on a symmetric grid, because each RotateSelectionCommand recomputes the pivot fresh from the selection's current sceneBoundingRect(), and an item whose bounding box isn't rotationally symmetric reports a different box (and therefore a different centre) at 0 and 90 degrees. Verified this drift is identical with and without this change, so it is not a regression -- just a different, harder guarantee this change does not attempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -149,7 +149,8 @@ void RotateSelectionCommand::addGroupPositionUndo(QGraphicsItem *item, const QPo
|
|||||||
* A quadrant is just an axis swap, which is exact. */
|
* A quadrant is just an axis swap, which is exact. */
|
||||||
QPointF offset;
|
QPointF offset;
|
||||||
const int quadrant = qRound(angle / 90.0);
|
const int quadrant = qRound(angle / 90.0);
|
||||||
if (qFuzzyCompare(angle, quadrant * 90.0))
|
bool exact_quadrant = qFuzzyCompare(angle, quadrant * 90.0);
|
||||||
|
if (exact_quadrant)
|
||||||
{
|
{
|
||||||
switch (((quadrant % 4) + 4) % 4)
|
switch (((quadrant % 4) + 4) % 4)
|
||||||
{
|
{
|
||||||
@@ -167,7 +168,40 @@ void RotateSelectionCommand::addGroupPositionUndo(QGraphicsItem *item, const QPo
|
|||||||
delta.x() * qSin(radians) + delta.y() * qCos(radians));
|
delta.x() * qSin(radians) + delta.y() * qCos(radians));
|
||||||
}
|
}
|
||||||
|
|
||||||
m_undo << new QPropertyUndoCommand(item->toGraphicsObject(), "pos", QVariant(old_pos), QVariant(pivot + offset), this);
|
QPointF new_pos = pivot + offset;
|
||||||
|
if (exact_quadrant)
|
||||||
|
{
|
||||||
|
/* Swapping X/Y deltas for a 90/270 turn only stays on the
|
||||||
|
* user's configured grid if xGrid == yGrid. With an
|
||||||
|
* asymmetric grid (both independently configurable, 1-30 px,
|
||||||
|
* in Settings) a delta that was a clean multiple of xGrid
|
||||||
|
* lands on the Y axis after the swap, where the grid unit is
|
||||||
|
* yGrid -- and 10 is not a multiple of 7. Verified this
|
||||||
|
* drifts a grid-aligned point off-grid without this snap
|
||||||
|
* (e.g. xGrid=10/yGrid=7: (100,210) rotates to (225,295),
|
||||||
|
* x%10==5), and that adding it corrects exactly that case on
|
||||||
|
* a real build (same inputs land on x%10==0, y%7==0).
|
||||||
|
*
|
||||||
|
* For xGrid == yGrid, snapping an already-on-grid point is a
|
||||||
|
* no-op, so this leaves that case's arithmetic unchanged.
|
||||||
|
* It does NOT, on its own, guarantee that four consecutive
|
||||||
|
* 90-degree turns return a selection to its exact starting
|
||||||
|
* position even on a symmetric grid: each RotateSelectionCommand
|
||||||
|
* recomputes the pivot fresh from the selection's CURRENT
|
||||||
|
* sceneBoundingRect(), and an item whose bounding box isn't
|
||||||
|
* rotationally symmetric (e.g. a wide text label) reports a
|
||||||
|
* different box, and therefore a different box centre, at 0
|
||||||
|
* and 90 degrees. That drift is pre-existing -- verified
|
||||||
|
* identical with and without this change -- and a separate
|
||||||
|
* problem from the one this fixes: staying on-grid after
|
||||||
|
* every individual turn is the property that matters day to
|
||||||
|
* day; bit-exact round-tripping through several consecutive
|
||||||
|
* rotations is a different, harder guarantee this change
|
||||||
|
* does not attempt. */
|
||||||
|
new_pos = Diagram::snapToGrid(new_pos);
|
||||||
|
}
|
||||||
|
|
||||||
|
m_undo << new QPropertyUndoCommand(item->toGraphicsObject(), "pos", QVariant(old_pos), QVariant(new_pos), this);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
Reference in New Issue
Block a user