[UUM-149791] Fix First Vertex pivot offset when resizing shapes or shift-duplicating - #696
Open
lopezt-unity wants to merge 5 commits into
Open
[UUM-149791] Fix First Vertex pivot offset when resizing shapes or shift-duplicating#696lopezt-unity wants to merge 5 commits into
lopezt-unity wants to merge 5 commits into
Conversation
…t Vertex pivot previewPivotPosition normalized the drag-captured corner offset and then rescaled it by its own original magnitude, which is a no-op and always reconstructs the offset from the size of the shape that was last dragged out rather than the size of the shape currently being placed. Resizing a shape (e.g. via Shape Settings) before shift-duplicating it with Pivot = First Vertex therefore placed the new shape's pivot using the previous shape's dimensions instead of its own. Now only the corner (sign per axis) is kept from the last drag, and the offset magnitude is scaled from the current bounds size.
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Harness Review
The revised offset correctly uses the current dimensions for positive-size shapes, but it loses the established drag-direction information when those dimensions are signed.
Reviewed commit 4ca7439
🤖 Helpful? 👍/👎
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## master #696 +/- ##
==========================================
+ Coverage 38.43% 38.55% +0.12%
==========================================
Files 279 279
Lines 39164 39195 +31
==========================================
+ Hits 15052 15112 +60
+ Misses 24112 24083 -29
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…ift-duplicating ProBuilderShape.UpdateShape() rebuilt a resized shape around its stale center (valid only for the previous size) instead of the pivot corner, so resizing a shape via Shape Settings left the mesh floating away from its pivot. The first attempt at this re-derived the pivot corner from the sign of the stale center, but that breaks for shapes with a negative size axis (e.g. Stairs dragged backwards): center = pivot + size/2 holds regardless of size's sign, so a corner "sign" derived independently of size can end up flipped relative to it. Fixed by recording, per axis, the ratio of the pivot-to-center offset to the half-size (0 for Center, 1 for First Vertex - a value immune to size's sign) once at rebuild time, and re-deriving the center from that ratio and the current size on every resize. DrawShapeTool.previewPivotPosition had the same class of bug: it reconstructed the shift-duplicate placement offset from the sign of the last-drawn shape's recorded corner vector and that shape's own (possibly stale or differently-signed) size. Since a First Vertex pivot is always exactly bounds.center - size/2, that reconstruction is unnecessary - the fix uses the current preview bounds directly.
lopezt-unity
changed the title
…acement Covers the reviewer-reported scenario directly: a shape drawn toward negative X (positive m_LastNonDuplicateCenterToOrigin.x, negative current size.x). Already passes - previewPivotPosition stopped using m_LastNonDuplicateCenterToOrigin entirely as of aa36ccd, in favor of computing the offset straight from the current (signed) bounds size.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[UUM-149791] Fixed the First Vertex pivot ending up offset from the shape after resizing an existing shape or shift-duplicating one with
the Shape tool.
Original bug:
DrawShapeTool.previewPivotPositionnormalized the drag-captured corner offset (m_LastNonDuplicateCenterToOrigin) and rescaled it by that same vector's own magnitude - a no-op that always reconstructed the offset using the size of whatever shape was last dragged out, ignoring the size of the shape actually being placed. Repro: draw a shape with Pivot = First Vertex, change its size (e.g. via Shape Settings), then shift-duplicate it - the new shape's pivot was offset using the previous shape's dimensions instead of its own.ProBuilderShape.UpdateShape()had the same class of bug one level down - it rebuilt a resized shape around its stale center (valid only for the previous size) instead of its pivot corner, so resizing an already-placed First-Vertex-pivoted shape via Shape Settings left the mesh floating away from its pivot/gizmo.Fix:
center = pivot + size/2holds exactly, regardless of the sign ofsize- there's no independent "corner choice" to reconstruct. (An intermediate fix that re-derived a corner sign from the stale center was still wrong for shapes with a negative size axis, e.g. a Stairs shape dragged backwards -ProBuilderShapenow instead records the pivot-to-center ratio, per axis, which is exactly0or1regardless of size's sign, once at rebuild time, and re-derives the center from that ratio and the current size on every resize.)Test plan
Executed by @lopezt-unity :
Tests/Editor/Editor/DrawShapeToolPivotOffsetTests.cs-previewPivotPositionagainst both an all-positive and a negative-size-axis "last drawn shape", proving stale drag data can't leak into the duplicate's placement.Tests/Editor/Editor/ProBuilderShapeResizePivotTests.cs-UpdateShape()resizing a First-Vertex-pivoted Stairs shape down, with both an all-positive and a negative-Z initial size, proving the shape stays anchored at its pivot after resize regardless of the original size's sign.