Bugfix/uum 141074 fix create shape gridsnap#671
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Review
There are two main issues in this PR that need to be addressed:
- Wrong Class Targeted: The fix has been applied to
PolyShapeTool(used for editing existing shapes), whereas the PR description and the new test suggest it should have been applied toDrawPolyShapeTool(used for drawing new shapes). - Ineffective Test: The new test activates
DrawPolyShapeToolinstead ofPolyShapeTooland does not actually assert thatgridSnapEnabledis enabled, meaning the test will pass even if the fix is not working.
Please review the inline feedback for details on how to resolve these issues.
🤖 Helpful? 👍/👎
| [EditorTool("Edit PolyShape", typeof(PolyShape))] | ||
| public class PolyShapeTool : EditorTool | ||
| { | ||
| public override bool gridSnapEnabled => true; |
There was a problem hiding this comment.
The PR description states this fix is for the Create PolyShape tool, but it was added here to PolyShapeTool, which handles editing an existing PolyShape (as indicated by the [EditorTool("Edit PolyShape", ...)] attribute).
If the intention was to fix the tool used for creating PolyShapes, this property likely needs to be added to DrawPolyShapeTool instead of (or in addition to) this class.
🤖 Helpful? 👍/👎 by bug_hunter
| // PolyShapeTool doesn't have a static instance like DrawShapeTool, | ||
| // so we just verify it was activated successfully | ||
| Assert.That(ToolManager.activeToolType, Is.EqualTo(typeof(DrawPolyShapeTool))); | ||
| } |
There was a problem hiding this comment.
This test activates and asserts against DrawPolyShapeTool, but it completely misses testing the actual fix because it never asserts the value of gridSnapEnabled (which is what the PR changed). It will pass even if the fix is reverted or fails.
Additionally, this test uses DrawPolyShapeTool (the tool for creating polyshapes), but the PR applied the fix to PolyShapeTool (the tool for editing polyshapes).
To properly test the property on the class that was modified, you can create an instance of it directly without needing ToolManager:
var tool = ScriptableObject.CreateInstance<PolyShapeTool>();
Assert.That(tool.gridSnapEnabled, Is.True);
Object.DestroyImmediate(tool);🤖 Helpful? 👍/👎 by bug_hunter
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## master #671 +/- ##
==========================================
+ Coverage 36.05% 38.02% +1.96%
==========================================
Files 277 278 +1
Lines 34909 38806 +3897
==========================================
+ Hits 12588 14755 +2167
- Misses 22321 24051 +1730
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1 participant
#Purpose of this PR
##Problem
The Create Shape tool (Cube, Sphere, Plane, etc.) was ignoring grid snapping when hovering the cursor over the scene view before starting to create a shape. Grid snapping would only activate after the first mouse click (MouseDown event). This inconsistent behavior differed from the Create PolyShape tool, which correctly snapped to the grid during hover.
Additionally, the Create PolyShape tool had gridSnapEnabled not set to true, preventing the tool from properly integrating with Unity's grid snapping system.
##Root Cause
In ShapeState_InitShape.cs, the m_IsOnGrid flag was only calculated during the MouseDown event (when clicking to start shape creation), not during hover/mouse move events. Since DrawShapeTool.GetPoint() requires both m_IsOnGrid && EditorSnapSettings.gridSnapActive to be true for snapping to work, the cursor position remained unsnapped during hover.
##Solutions
###Create Shape Tool Fix
Refactored ShapeState_InitShape.DoState() to calculate the plane and grid snapping state for ALL mouse events (hover + click), not just on MouseDown:
Key Changes:
Result: m_IsOnGrid is correctly set before GetPoint() is called during hover, enabling grid snapping throughout the shape creation workflow.
###Create PolyShape Tool Fix
Added gridSnapEnabled override to PolyShapeTool class:
public override bool gridSnapEnabled => true;This ensures the tool properly integrates with Unity's grid snapping system.
Links
Jira: https://jira.unity3d.com/browse/UUM-141074
Comments to Reviewers
[List known issues, planned work, provide any extra context for your code.]