[UUM-133529] ProBuilder GameObjects not changing back to 3D shapes after Plane/Sprite transition#670
[UUM-133529] ProBuilder GameObjects not changing back to 3D shapes after Plane/Sprite transition#670lopezt-unity wants to merge 3 commits into
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for this pull request! We've completed our review and identified one area that can be improved:
- Floating-point Tolerance & Property Setter Bypass: The check on
m_Size.yusesMathf.Epsilonwhich is too small for general zero-tolerance comparisons, and it directly modifies the backing field instead of updating through thesizeproperty. This can cause state desynchronization and logic failure.
Please see the inline comment for detailed explanations and suggested code replacements.
🤖 Helpful? 👍/👎
| else if(wasFlat && !isFlat) | ||
| { | ||
| // Transitioning FROM a 2D shape TO a 3D shape - restore Y dimension | ||
| if(Mathf.Abs(m_Size.y) < Mathf.Epsilon) |
There was a problem hiding this comment.
Have you considered using a small absolute tolerance threshold (like 0.0001f) instead of Mathf.Epsilon, and updating the dimension through the public size property rather than modifying the backing field m_Size.y directly?
1. The Mathf.Epsilon Precision Trap
In Unity, Mathf.Epsilon is defined as float.Epsilon (1.4e-45), which is the absolute minimum denormalized positive float rather than the standard machine epsilon (1.19e-7). If the Y size undergoes any minor floating-point calculations, scaling, or serialized rounding/precision loss, it might be a tiny positive non-zero value. Because of this, the Mathf.Abs(m_Size.y) < Mathf.Epsilon check would evaluate to false, and the Y dimension would fail to restore. Using a small practical tolerance (like 0.0001f) is much more robust.
2. Bypassing the Property Setter
Modifying the backing field m_Size.y directly bypasses the size property setter. If the size setter handles internal validation, clamping, component dirtying, or undo/redo registration, direct field manipulation can lead to state desynchronization. The idiomatic C# way to update a field on a struct property is to retrieve it, modify the component, and write it back.
Suggested change (apply manually):
if (Mathf.Abs(size.y) < 0.0001f)
{
Vector3 currentSize = size;
currentSize.y = 1f;
size = currentSize;
}🤖 Helpful? 👍/👎 by guardian
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## master #670 +/- ##
==========================================
+ Coverage 36.05% 36.77% +0.71%
==========================================
Files 277 278 +1
Lines 34909 35439 +530
==========================================
+ Hits 12588 13033 +445
- Misses 22321 22406 +85
Flags with carried forward coverage won't be shown. Click here to find out more.
|
modrimkus-unity
left a comment
modrimkus-unity
left a comment
There was a problem hiding this comment.
LGTM, though AI might have a point on Mathf.Epsilon. I'd check what has more precedent in the codebase (are we checking more against Mathf.Epsilon or 0.0001f for example).
Unity typically uses tolerances like 0.0001f for geometric comparisons (e.g., in collision detection, mesh validation)
2 participants
#Purpose of this PR
##Problem
When a ProBuilder GameObject (e.g., Arch, Cube) had its shape changed to Plane or Sprite, it could not be changed back to a 3D shape. The GameObject would remain flat instead of becoming a proper 3D mesh.
##Reproduction steps:
##Root Cause
The ProBuilderShape.SetShape() method had logic to flatten the Y dimension when transitioning TO Plane/Sprite shapes, but no corresponding logic to restore the Y dimension when transitioning FROM Plane/Sprite TO 3D shapes. This left m_Size.y at 0, causing the 3D shape to be built with zero height.
##Solution
Added detection for 2D→3D shape transitions in SetShape(). When transitioning from a flat shape (Plane/Sprite) to a 3D shape, the Y dimension is now restored to 1.0 if it was previously zero.
Links
Jira: https://jira.unity3d.com/browse/UUM-133529
Comments to Reviewers
[List known issues, planned work, provide any extra context for your code.]