Skip to content

Conversation

@varinotmUnity
Copy link
Contributor

@varinotmUnity varinotmUnity commented Nov 14, 2025

DO NOT FORGET TO INCLUDE A CHANGELOG ENTRY

Purpose of this PR

Fix UV Editor distortion when dragging connected texture group faces

Problem

When you select one face in a subdivided/connected texture group and drag it in the UV Editor, all connected faces would visually jump/distort during the drag. On mouse release everything snaps back to correct position, so it's visual bug only.

Root Cause

OnBeginUVModification() auto-selects all faces in the texture group, then calculates new pivot as center of ALL faces. The move math then reprojects each vertex relative to this new center, causing the visual jump.

Fix

Store handlePosition BEFORE the texture group auto-selection loop runs. Use this original position as uvOrigin instead of the recalculated center.

This way connected faces maintain their spatial relationship to the original pivot point, meaning no distortion.

Changes

  • UVEditor.cs: Save original handle position before SelectTextureGroups(), use it for uvOrigin
  • Cleanup: Use EditorGUI.actionKey instead of platform-specific Control/Command key check. This is only because Unit tests were failing.
  • Tests: Added MoveSingleFace_PreservesRelativePositions() and MoveConnectedFaces_PreservesRelativePositions() to verify no distortion happens. The unit tests do not test the original issue, as it is ONLY happening in the editor UV window. The editor calls MoveTool(), while the unit tests calls SceneMoveTool. The tests were mostly added to make sure I didn't break the API.

Links

https://jira.unity3d.com/browse/PBLD-276

Comments to Reviewers

Though I did not test what would happen with the rotate and scale tool. From my testing, it looks fine, but thorough testing from QA might be necessary to make sure we don't cause regression

… then moving it. this was due to bad handle position set, which would then overwrite the uv center badly. this only happens in the editor, not with the API
@unity-cla-assistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


varinotmUnity seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@u-pr-agent
Copy link

u-pr-agent bot commented Nov 14, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

PBLD-276 - Partially compliant

Compliant requirements:

  • The PR addresses the snapping issue by correctly preserving the handle position when the selection is programmatically expanded to include the entire texture group.

Non-compliant requirements:

[]

Requires further human verification:

  • As noted by the author, the impact of this change on the Rotate and Scale tools in the UV Editor has not been thoroughly tested and requires manual QA to ensure no regressions were introduced.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

The PR includes a small, targeted logic change and well-structured unit tests, making it relatively easy to understand and review.
🏅 Score: 95

The code provides a clean fix for the reported bug, significantly improves test hygiene, and adds excellent regression tests, but manual testing on related tools is still required.
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Handle Position Logic

The core fix involves caching the handle position before the selection is expanded and the editor is refreshed, then restoring it. Please validate that this logic correctly prevents the "snapping" behavior described in the ticket without introducing side effects during UV modification.

Vector2 originalHandlePosition = handlePosition;

// Make sure all TextureGroups are auto-selected
for (int i = 0; i < selection.Length; i++)
{
    if (selection[i].selectedFaceCount > 0)
    {
        int fc = selection[i].selectedFaceCount;
        selection[i].SetSelectedFaces(
            SelectTextureGroups(selection[i], selection[i].selectedFacesInternal)
        );

        // kinda lame... this will cause setSelectedUVsWithSceneView to be called again.
        if (fc != selection[i].selectedFaceCount)
            update = true;
    }

    selection[i].ToMesh(); // Reset the Mesh to PB data only.
    selection[i].Refresh();
}

if (update)
{
    // UpdateSelection clears handlePosition
    ProBuilderEditor.Refresh();
    SetHandlePosition(originalHandlePosition, true);
}
Test Cleanup

The TearDown method has been significantly improved for better test isolation. This is a great addition that improves the stability of the test suite.

[TearDown]
public void Cleanup()
{
    // Close the UV Editor window first
    if (UVEditor.instance != null)
    {
        UVEditor.instance.Close();
    }

    // Clear ProBuilder selections
    MeshSelection.ClearElementSelection();
    Selection.activeGameObject = null;

    // Reset tool context
    ToolManager.SetActiveContext<GameObjectToolContext>();

    // Destroy the cube
    if (m_cube != null && m_cube.gameObject != null)
    {
        UObject.DestroyImmediate(m_cube.gameObject);
    }

    m_cube = null;

    // Clear undo to prevent resurrection
    Undo.ClearAll();
}
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

Comment on lines +158 to +160
// Simulate a move operation
Vector2 moveDelta = new Vector2(0.1f, 0.2f);
UVEditor.instance.SceneMoveTool(moveDelta);
Copy link

Choose a reason for hiding this comment

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

Suggestion: The new tests MoveSingleFace_PreservesRelativePositions and MoveConnectedFaces_PreservesRelativePositions are missing a call to OnBeginUVModification() before SceneMoveTool(). The bug fix in this PR is located within OnBeginUVModification, which handles auto-selecting grouped faces and preserving the handle position. Without this call, the tests do not accurately simulate the user workflow and fail to validate the actual fix. [possible issue, importance: 9]

Suggested change
// Simulate a move operation
Vector2 moveDelta = new Vector2(0.1f, 0.2f);
UVEditor.instance.SceneMoveTool(moveDelta);
// Simulate a move operation
Vector2 moveDelta = new Vector2(0.1f, 0.2f);
UVEditor.instance.OnBeginUVModification();
UVEditor.instance.SceneMoveTool(moveDelta);

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SceneModeTool called OnBeginModification already

@codecov-git.colasdn.top
Copy link

codecov-git.colasdn.top bot commented Nov 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
+ Coverage   35.55%   36.02%   +0.47%     
==========================================
  Files         277      277              
  Lines       34889    34888       -1     
==========================================
+ Hits        12406    12570     +164     
+ Misses      22483    22318     -165     
Flag Coverage Δ
mac_trunk 34.59% <100.00%> (-0.75%) ⬇️
win_trunk 34.49% <100.00%> (-0.75%) ⬇️

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

Files with missing lines Coverage Δ
Editor/EditorCore/UVEditor.cs 39.78% <100.00%> (+6.08%) ⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thomas-tu thomas-tu self-requested a review November 17, 2025 15:49
m_cube = null;

// Clear undo to prevent resurrection
Undo.ClearAll();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the cube was not getting deleted at the end of the test. the changes here is to clean the test. only this line was necessary to fix the issue though

get { return Event.current.modifiers == EventModifiers.Control; }
get { return EditorGUI.actionKey; }
}
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was changed or else the test would not pass

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.

3 participants