Enhance cut tool functionality and code refactor

[singam96]

This pull request introduces significant new functionality to the ProBuilder CutTool in the Unity Editor by adding support for rectangle-based cuts and improving snapping behavior. The main changes are the implementation of a rectangle cut mode, enhanced snapping logic for precise placement, and supporting infrastructure for these features.

Rectangle Cut Mode:

• Added a new rectangle cut mode in CutTool that allows users to click and drag to define a rectangular cut on a face, with visual previews and auto-placement of corners. The cut path is built from the rectangle's four corners, and the user can finalize the cut interactively. (Editor/EditorCore/CutTool.Rectangle.cs)
• Implemented a visual preview for the rectangle during drag operations, including filled and outlined rectangle rendering in the scene view. (Editor/EditorCore/CutTool.Rectangle.cs)

Snapping and Placement Enhancements:

• Refactored and extended snapping logic to support grid snapping, snapping to existing vertices and edges, and associating cut points with mesh geometry or the current cut path. This ensures precise placement of cut points and seamless integration with the mesh. (Editor/EditorCore/CutTool.Snapping.cs)
• Improved mesh connection logic to intelligently connect the cut path to the mesh, handling both loop and non-loop cases, and ensuring robust integration with face borders and vertices. (Editor/EditorCore/CutTool.Snapping.cs)

Supporting Files:

• Added .meta files for the new source files to ensure proper Unity asset tracking. (Editor/EditorCore/CutTool.Geometry.cs.meta, Editor/EditorCore/CutTool.Rectangle.cs.meta, Editor/EditorCore/CutTool.Snapping.cs.meta) [1] [2] [3]

These changes collectively provide a more powerful and user-friendly cut tool in ProBuilder, enabling both freeform and rectangle-based cuts with accurate snapping and visual feedback.

[cla-assistant-unity]

All committers have signed the CLA.

[u-pr]

We have reviewed this pull request and found several critical bugs, performance issues, and high memory allocations in hot code paths that should be addressed before merging.

Summary of Key Issues:

• Bugs & Logic Errors: A coordinate space mismatch in the rectangle preview (CutTool.Rectangle.cs), a bitwise OR operator typo (| instead of &) in CutTool.Snapping.cs, and a regression causing incorrect face selection during loop cuts in CutTool.Geometry.cs.
• Performance & GC Allocations: Heavy heap allocations via GetVertices().ToList() inside OnToolGUI repaints and vertex insertion loops, plus highly expensive operations (like HandleUtility.WorldToGUIPoint and List.IndexOf) executed inside nested loops.
• Refactoring: Code duplication for generating local axes on the face plane in CutTool.Rectangle.cs.

Detailed feedback and suggestions have been provided inline.

_{🤖 Helpful? 👍/👎}

[singam96]

Updated
master...singam96:com.unity.probuilder:feature/cut-tool-improve

[u-pr]

Here is a summary of the new review findings for this PR:

⚠️ Geometry & Snapping Issues

• Rectangle Cut Distortion (CutTool.Rectangle.cs): Snapping all four corners to a 3D grid independently can pull vertices off the face's plane and distort the rectangle into an irregular, non-planar quadrilateral. Additionally, hardcoding VertexTypes.NewVertex bypasses the geometry snapping checks, preventing the corners from topologically connecting to existing vertices or edges.

⚙️ Performance & Memory Optimizations

• Double List Traversal (CutTool.Geometry.cs): Using both Exists and Find with the same predicate results in traversing m_MeshConnections twice. This can be streamlined using FindIndex.
• Garbage Collection Allocations (CutTool.Geometry.cs, CutTool.UI.cs): Calling .Select().ToArray() inside TryGetFaceArea and DrawCutLine creates unnecessary heap allocations on every execution/repaint frame. Using pre-cached arrays or drawing segment-by-segment is highly recommended to prevent frame spikes.

_{🤖 Helpful? 👍/👎}

[u-pr]

Applying grid snapping independently to all four corners will distort the cut into an irregular, non-planar quadrilateral if the target face isn't perfectly axis-aligned.

Additionally, because end (m_RectEndPoint) was grid-snapped during the drag interaction, it might possess an elevation offset relative to the face's plane. Using corner2 = end incorporates this offset, making corner2 non-coplanar with the other three corners.

Have you considered deriving corner2 strictly on the plane (e.g., start + faceRight * rightDot + faceUp * upDot) and removing this secondary grid-snap block so the resulting shape remains a valid, planar rectangle?

_{🤖 Helpful? 👍/👎 by bug_hunter}

[u-pr]

Hardcoding VertexTypes.NewVertex for the rectangle's corners bypasses the m_SnapToGeometry logic. If a user starts or ends the rectangle exactly on an existing edge or vertex, the cut tool will insert disconnected floating geometry instead of topologically connecting to it.

Have you considered updating the point state dynamically before adding it to the path, similar to point mode? For example:

m_CurrentPosition = corner0;
m_CurrentPositionNormal = faceNormal;
m_CurrentFace = m_TargetFace;
CheckPointInMesh();
corner0 = m_CurrentPosition; // Save potentially snapped position for closing the loop
AddCurrentPositionToPath(false);

_{🤖 Helpful? 👍/👎 by bug_hunter}

[u-pr]

Have you considered using FindIndex to avoid a double-iteration over m_MeshConnections? Currently, calling both Exists and Find with the same predicate traverses the list twice.

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

Have you considered using m_Mesh.positionsInternal directly here? Calling .Select(...).ToArray() on meshVertices inside TryGetFaceArea allocates a new Vector3[] array on every invocation. Since TryGetFaceArea is executed inside a loop within ComputeFaceClosure, using the cached positionsInternal array can significantly reduce GC allocations.

Suggested change

	Vector3[] vertices = meshVertices.Select(vertex => vertex.position).ToArray();
	Vector3[] vertices = m_Mesh.positionsInternal;

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

Since DrawCutLine is executed on every Repaint frame, calling .Select(...).ToArray() creates a high volume of short-lived array allocations. Have you considered drawing the polyline segment-by-segment using Handles.DrawLine to avoid any allocations in the rendering loop?

Suggested replacement omitted because it is too large for an inline review comment.

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

Here is a summary of the new verified findings discovered in this review run:

• Performance & Disk I/O: Unconditional writes to EditorPrefs on every GUI event causes unnecessary disk activity and micro-stutters.
• Redundant Processing: Unused vertex population loop and parameter inside geometry computation (TryGetFaceArea).
• High GC Allocations: Sorting edges inside hot path snapping checking (CheckPointInMesh) can be replaced with direct iterations.

Detailed comments have been posted inline.

_{🤖 Helpful? 👍/👎}

[u-pr]

Have you considered using EditorGUI.BeginChangeCheck() and EditorGUI.EndChangeCheck() to only write to EditorPrefs when the values are actually changed by the user? Currently, EditorPrefs.SetBool and EditorPrefs.SetFloat are called unconditionally on every GUI event (including Repaint and Layout), which triggers redundant writes and can cause micro-stutters in the editor.

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

The meshVertices list is cleared and populated via m_Mesh.GetVerticesInList(meshVertices) on every iteration, but it is completely unused. Since GetVerticesInList iterates over all vertices of the mesh, removing this redundant call will significantly improve performance, especially on complex meshes.

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

The meshVertices parameter is unused in this method and can be safely removed from the signature.

Suggested change

	bool TryGetFaceArea(Face face, List<Vertex> meshVertices, IList<SharedVertex> uniqueIdToVertexIndex,
	bool TryGetFaceArea(Face face, IList<SharedVertex> uniqueIdToVertexIndex,

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

Since CheckPointInMesh is called on every mouse move in the Scene View, using WingedEdge.SortEdgesByAdjacency here introduces significant garbage collection and performance overhead. Since the subsequent loop only searches for the edge with the minimum distance, sorting the edges by adjacency is unnecessary. Using the face's direct edges property (e.g., m_CurrentFace.edges and m_TargetFace.edges) instead would eliminate this overhead entirely.

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

This review identifies several critical bugs and structural issues in the new grid snapping and rectangle cut code that need addressing:

⚠️ Critical Snapping & Planarity Issues

• Grid Snapping Planarity Drifts (f1 & f3): 3D grid snapping applied directly to the current point or corner0 can pull vertices off the face plane, causing non-planar geometry and triangulation failures. Projecting snapped points back onto the face plane is required.
• Bypassed Rectangle Connections (f2): A hardcoded < 2 threshold prevents the generation of all 4 required connections in rectangle mode when 2 or 3 connections already exist.

🧹 Refactoring & Code Quality

• Duplicated Axes Calculation (f5): Reuses the newly introduced GetFacePlaneAxes helper to simplify duplicated local axes calculations in validation logic.

_{🤖 Helpful? 👍/👎}

[u-pr]

When grid snapping is enabled, ProBuilderSnapping.Snap rounds coordinates to the 3D grid, which can pull the point off the face's 2D plane (unless the face is perfectly aligned with the grid).

Since ProBuilder requires cut points to be coplanar with the face, using these drifting floating points can result in non-planar geometry or failed triangulations. Have you considered projecting the snapped point back onto the face plane?

if (m_SnapToGrid)
{
    Vector3 snapped = ProBuilderSnapping.Snap(m_CurrentPosition, EditorSnapping.activeMoveSnapValue);
    Plane facePlane = new Plane(m_CurrentPositionNormal, m_CurrentPosition);
    m_CurrentPosition = facePlane.ClosestPointOnPlane(snapped);
}

Suggested change (apply manually):

                if (m_SnapToGrid)
                {
                    Vector3 snapped = ProBuilderSnapping.Snap(m_CurrentPosition, EditorSnapping.activeMoveSnapValue);
                    Plane facePlane = new Plane(m_CurrentPositionNormal, m_CurrentPosition);
                    m_CurrentPosition = facePlane.ClosestPointOnPlane(snapped);
                }

_{🤖 Helpful? 👍/👎 by bug_hunter}

[u-pr]

The else if condition is hardcoded to < 2, but the logic inside dynamically calculates requiredConnections up to 4 for rectangles.

If m_RectangleMode is enabled and the user snapped exactly 2 or 3 corners to existing geometry, connectionsToBordersCount will be >= 2. This block will be completely skipped, and the remaining artificial connections needed to reach requiredConnections won't be generated.

Have you considered evaluating requiredConnections before the condition?

int requiredConnections = m_RectangleMode ? 4 : 2;
// ... 
else if(isALoop && connectionsToBordersCount < requiredConnections)

_{🤖 Helpful? 👍/👎 by bug_hunter}

[u-pr]

While corner1, corner2, and corner3 are derived properly to form a rectangle, corner0 is snapped to the 3D grid here without being projected back onto the face.

Because all other corners are offset from corner0, the entire rectangle will be translated off the face plane by the 3D snapping offset (unless the face is perfectly grid-aligned). This will cause ProBuilder to generate skewed, non-planar connection polygons.

Have you considered projecting corner0 back onto the face plane after snapping?

if (m_SnapToGrid)
{
    Vector3 snapped = ProBuilderSnapping.Snap(corner0, EditorSnapping.activeMoveSnapValue);
    Plane facePlane = new Plane(faceNormal, corner0);
    corner0 = facePlane.ClosestPointOnPlane(snapped);

    corner1 = corner0 + faceRight * rightDot;
    corner2 = corner0 + faceRight * rightDot + faceUp * upDot;
    corner3 = corner0 + faceUp * upDot;
}

_{🤖 Helpful? 👍/👎 by bug_hunter}

[u-pr]

The logic to compute the face plane axes (faceRight and faceUp) is duplicated here and in CutTool.Rectangle.cs. Since both files are part of the same partial class CutTool, you can reuse the GetFacePlaneAxes static helper method to avoid duplication and improve maintainability.

Have you considered refactoring this to use the helper?

csharp\nVector3 faceRight, faceUp;\nGetFacePlaneAxes(faceNormal, out faceRight, out faceUp);\n

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

This pull request introduces two critical correctness/topology issues and several efficiency/garbage collection bottlenecks that should be addressed.

Summary of New Findings

Critical Bugs:

• Topological Snapping Mismatch (CutTool.Snapping.cs): Visual snapping to internal/non-perimeter edges (due to using m_CurrentFace.edges directly) causes a mismatch with the actual cut execution, forcing the visual snapping points onto different perimeter edges.
• Mesh Topology Corruption (CutTool.Geometry.cs): The dictionary lookup optimization loop overwrites coincident/duplicate vertex keys, returning the last match rather than the first index. This can connect cuts to incorrect faces sharing coincident vertices, corrupting mesh structure.

Performance & Allocations:

• Loop-Invariant Hoisting (CutTool.Geometry.cs): Dictionary lookups inside ComputeFaceClosure can be hoisted out of the reverse/forward iteration loops to avoid redundant lookups.
• Repaint Frame GC Allocations (CutTool.Rectangle.cs): Creating new Vector3[] arrays during repaint events in DoRectanglePreview leads to garbage collector pressure and stutters.
• Closure Delegate Allocations (CutTool.Snapping.cs): Lambda-based searches (Exists) inside loops allocate closures on every iteration and can be easily optimized into standard for loops.

_{🤖 Helpful? 👍/👎}

[u-pr]

Using m_CurrentFace.edges instead of WingedEdge.SortEdgesByAdjacency(m_CurrentFace) includes the internal edges of multi-triangle faces (e.g., the diagonal of a quad). This causes the tool to visually snap to internal edges during CheckPointInMesh().

However, the actual insertion logic in InsertVertexOnExistingEdge() still uses WingedEdge.SortEdgesByAdjacency(), which only contains the perimeter edges. If a user snaps to an internal edge, the visual point will mismatch the actual cut execution, because the cut point will be forced onto the nearest perimeter edge instead. You should revert to using WingedEdge.SortEdgesByAdjacency to ensure visual snapping matches the execution logic.

Suggested change (apply manually):

            IList<Edge> peripheralEdges = WingedEdge.SortEdgesByAdjacency(m_CurrentFace);
            if (m_TargetFace != null && m_CurrentFace != m_TargetFace)
                peripheralEdges = WingedEdge.SortEdgesByAdjacency(m_TargetFace);

_{🤖 Helpful? 👍/👎 by bug_hunter}

[u-pr]

Converting IndexOf() to a Dictionary assignment loop changes how duplicate elements are resolved. IndexOf() returns the index of the first matching vertex, while this loop overwrites the entry and stores the last index.

Meshes frequently contain multiple structurally identical vertices (e.g., coincident vertices shared across different faces). If the dictionary returns the last match instead of the first, vertexIndexMap[formerVertices[i]] may connect the new cut to a vertex belonging to a completely different face rather than the target face, which can corrupt the mesh topology.

To safely optimize the lookup while preserving the original IndexOf behavior, only add the index if the key isn't already present.

Suggested change (apply manually):

                for (int i = 0; i < meshVertices.Count; i++)
                {
                    if (!vertexIndexMap.ContainsKey(meshVertices[i]))
                        vertexIndexMap.Add(meshVertices[i], i);
                }

_{🤖 Helpful? 👍/👎 by bug_hunter}

[u-pr]

Have you considered hoisting the loop-invariant dictionary lookups out of the loop?

Currently, sharedToUnique.ContainsKey(connection.item1) and sharedToUnique[connection.item1] are looked up on every iteration of the loops (both forward and reverse). Since connection.item1 does not change during the iterations, we can pre-compute this value outside the loops to avoid redundant dictionary lookups:

            int connectionSharedItem1 = -1;
            bool hasConnectionSharedItem1 = hasConnection && sharedToUnique.TryGetValue(connection.item1, out connectionSharedItem1);

Then, the condition inside the loops simplifies to:

                if(sharedToUnique[vertexIndex] == polygonFirstSharedIndex ||
                   (hasConnectionSharedItem1 && sharedToUnique[vertexIndex] == connectionSharedItem1))

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

Have you considered avoiding allocation of new Vector3[] arrays inside DoRectanglePreview()? Since this method is executed on every Repaint event during drag operations, allocating new arrays per frame generates garbage that can lead to garbage collection stuttering in the Unity editor.

You can declare reusable fields on the class level to avoid allocation completely:

private readonly Vector3[] m_RectConvexPolygon = new Vector3[4];
private readonly Vector3[] m_RectPreviewPath = new Vector3[5];

And then populate and pass them to the Handles methods.

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

Have you considered replacing the lambda-based Exists call with a simple for loop?

Using .Exists(vert => Math.Approx3(...)) inside the outer foreach loop allocates a delegate (lambda closure) on every iteration. We can avoid this allocation entirely by using a standard for loop to check if the vertex matches:

                    bool alreadyExists = false;
                    for (int j = 0; j < existingVerticesInCut.Count; j++)
                    {
                        if (Math.Approx3(verticesPositions[vertexIndex], existingVerticesInCut[j]))
                        {
                            alreadyExists = true;
                            break;
                        }
                    }
                    if (alreadyExists)
                        continue;

_{🤖 Helpful? 👍/👎 by guardian}

[u-pr]

Have you considered optimizing the check and retrieval of connections here to avoid double lookup and delegate allocations?

Calling m_MeshConnections.Exists(...) followed immediately by m_MeshConnections.Find(...) with lambdas iterates through the connections twice and allocates two delegate instances. We can find the index first using a standard loop to avoid allocations and get the connection directly:

                        int connectionIndex = -1;
                        for (int k = 0; k < m_MeshConnections.Count; k++)
                        { 
                            if (m_MeshConnections[k].item1 == pathIndex)
                            { 
                                connectionIndex = k;
                                break;
                            }
                        }

                        if (connectionIndex >= 0)
                        { 
                            var tuple = m_MeshConnections[connectionIndex];
                            if (Vector3.Distance(m_CutPath[tuple.item1].position, verticesPositions[tuple.item2])
                                > Vector3.Distance(m_CutPath[pathIndex].position, verticesPositions[vertexIndex]))
                            {
                                m_MeshConnections.RemoveAt(connectionIndex);
                                m_MeshConnections.Add(new SimpleTuple<int, int>(pathIndex, vertexIndex));
                            }
                        }

Using RemoveAt with the pre-determined index is also faster than Remove(tuple) because it doesn't need to perform a second search to remove the item.

_{🤖 Helpful? 👍/👎 by guardian}