Skip to content

Bugfix/uum 141074 fix create shape gridsnap#671

Open
lopezt-unity wants to merge 5 commits into
masterfrom
bugfix/uum-141074-fix-create-shape-gridsnap
Open

Bugfix/uum 141074 fix create shape gridsnap#671
lopezt-unity wants to merge 5 commits into
masterfrom
bugfix/uum-141074-fix-create-shape-gridsnap

Conversation

@lopezt-unity
Copy link
Copy Markdown
Collaborator

#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:

  • Moved plane calculation and m_IsOnGrid determination outside the MouseDown-specific branch
  • Grid alignment check now happens during hover, before GetPoint() is called
  • On MouseDown, the plane is persisted to tool members for subsequent states
  • Eliminated code duplication by unifying hover and click logic

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.]

Copy link
Copy Markdown
Contributor

@u-pr u-pr Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May require changes

Summary of Review

There are two main issues in this PR that need to be addressed:

  1. 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 to DrawPolyShapeTool (used for drawing new shapes).
  2. Ineffective Test: The new test activates DrawPolyShapeTool instead of PolyShapeTool and does not actually assert that gridSnapEnabled is 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? 👍/👎

Comment thread Editor/EditorCore/PolyShapeTool.cs
Comment thread Tests/Editor/Editor/ShapeToolGridSnapTests.cs Outdated
@codecov-github-com
Copy link
Copy Markdown

codecov-github-com Bot commented May 27, 2026

Codecov Report

Attention: Patch coverage is 2.63158% with 37 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Editor/StateMachines/ShapeState_InitShape.cs 0.00% 36 Missing ⚠️
Editor/EditorCore/PolyShapeTool.cs 0.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   36.05%   37.97%   +1.91%     
==========================================
  Files         277      278       +1     
  Lines       34909    38806    +3897     
==========================================
+ Hits        12588    14736    +2148     
- Misses      22321    24070    +1749     
Flag Coverage Δ
probuilder_MacOS_6000.3 35.64% <3.57%> (+1.06%) ⬆️
probuilder_MacOS_6000.4 35.64% <3.57%> (+1.06%) ⬆️
probuilder_MacOS_6000.5 35.64% <3.57%> (+1.07%) ⬆️
probuilder_MacOS_6000.6 35.64% <3.57%> (?)
probuilder_Windows_6000.0 35.56% <3.57%> (+0.32%) ⬆️
probuilder_Windows_6000.3 35.56% <3.57%> (+0.32%) ⬆️
probuilder_Windows_6000.4 35.56% <3.57%> (+0.31%) ⬆️
probuilder_Windows_6000.5 35.56% <3.57%> (+0.32%) ⬆️
probuilder_Windows_6000.6 35.56% <3.57%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
Editor/EditorCore/DrawShapeTool.cs 28.21% <100.00%> (+9.00%) ⬆️
Editor/EditorCore/PolyShapeTool.cs 4.72% <0.00%> (+0.72%) ⬆️
Editor/StateMachines/ShapeState_InitShape.cs 9.37% <0.00%> (+9.37%) ⬆️

... and 88 files with indirect coverage changes

ℹ️ Need help interpreting these results?

Copy link
Copy Markdown
Contributor

@modrimkus-unity modrimkus-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants