Skip to content

Conversation

@CortiWins
Copy link

Purpose of this PR

Class: TextureMoveTool
Method: DoToolUI

Snapping is done by using EditorSnapping.MoveSnap on the moved position.

There are three handles

  • Slider2D
  • Slider1D Up ( Green arrow)
  • Slider1D Right (RedArrow)

The Slider2D has an explicit individual snap value of 0.0f. The Slider1Ds use a method override that internally applies -1.0f as a snap factor.

The position done via Slider1D is snapped to 1.0 increments before getting to the MoveSnap line where the actual snap settings are applied.

Class: TextureMoveTool
Method: DoToolUI

Snapping is done by using EditorSnapping.MoveSnap on the moved position.

There are three handles
- Slider2D
- Slider1D Up ( Green arrow)
- Slider1D Right (RedArrow)

The Slider2D has an explicit individual snap value of 0.0f.
The Slider1Ds use a method override that internally applies -1.0f as a snap factor.

The position done via Slider1D is snapped to 1.0 increments before getting to the MoveSnap line where the acutual snap settings are applied.
@u-pr
Copy link

u-pr bot commented Oct 23, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

The PR is a small, localized bug fix with a clear explanation, making it very straightforward to review.
🏅 Score: 95

The code change correctly fixes the snapping issue and improves readability, but there is a minor typo in the changelog entry.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Changelog Entry

There appears to be a typo ("big" instead of "bug") in the new changelog entry. Also, the line starts with a + character which should probably be removed.

- Fixed a big where in the TextureMoveTool, a snapping of 1.0f was applied to handle movement before the actual snapping is applied

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

@thomas-tu
Copy link
Collaborator

Hi @CortiWins, thanks for your interest in improving ProBuilder.
Does this issue affect the user experience with regard to the TextureMoveTool? If so, please provide a reproduction case. I'm under the impression that these changes are an attempt to make a small optimization on the execution of the TextureMoveTool. Thanks!

@CortiWins
Copy link
Author

Hello @thomas-tu

it was not an optimization, it was a fix for an issue i had.

In the unity version of the code:

  • Handles.Slider2D() is given a snap value of 0.0
  • Handles.Slider() is not given a snap value, so it defaults to -1.0
  • The position is snapped to grid with the later line EditorSnapping.MoveSnap(m_Position)

So Slider2D returns "unsnapped" position while Slider returns already snapped positions and i had some case where that lead to an issue which i noticed when the 2D Slider worked fine with snapping and the 1D Sliders did not. And so it tried giving the Slide1D gizmos the same treatment the Slider2D already had and that fixed it.

Currently, i can not reproduce the bug any more, which means that something is different from what it was in october last year and that bothers me a lot to be honest. Anyways, that is it for now.
And whoever at Unity once decided that Handles.Slider2D() should be called with 0.0 snapping, i think they where right about it.

@thomas-tu
Copy link
Collaborator

Hello @CortiWins, thank you for the clarification.
If the issue doesn't not occur anymore so I'm going to close this pull request.

Feel free to open it back or file a Bug Report through the Editor (Help > Report a Bug...) if it happens again. Thank you!

@thomas-tu thomas-tu closed this Jan 26, 2026
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