Skip to content

Making some Types and Properties public to allow building of custom tools with it#629

Open
CortiWins wants to merge 5 commits into
Unity-Technologies:masterfrom
CortiWins:master
Open

Making some Types and Properties public to allow building of custom tools with it#629
CortiWins wants to merge 5 commits into
Unity-Technologies:masterfrom
CortiWins:master

Conversation

@CortiWins
Copy link
Copy Markdown

Purpose of this PR

The changes make three types public

  • PositionToolContext
  • TextureToolContext
  • CutTool

This allows to build shortcuts that probuilder into the respective tools and edit mode.

The changes one property public

  • MeshSelection.totalVertexCountOptimized

it is calculated and updated in the same way all other public statistics are, so making it public allows to build UI that shows statistics.

All other statics about the current selection are also public.
Useful to set it via ToolManager.SetActiveTool
Useful to set via ToolManager.SetActiveContext
@unity-cla-assistant
Copy link
Copy Markdown

unity-cla-assistant commented Oct 12, 2025

CLA assistant check
All committers have signed the CLA.

@u-pr
Copy link
Copy Markdown
Contributor

u-pr Bot commented Oct 12, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

The PR is very small and only changes access modifiers to make certain classes and properties public, with no logic changes.
🏅 Score: 100

The PR correctly implements its stated goal of exposing specific types and properties, follows good practices by updating the changelog, and introduces no new logic or complexity.
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

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

@u-pr
Copy link
Copy Markdown
Contributor

u-pr Bot commented Oct 12, 2025

PR Code Suggestions ✨

No code suggestions found for the 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 acutual snap settings are applied.
@lopezt-unity lopezt-unity self-requested a review June 1, 2026 19:43
{
[EditorTool("Cut Tool", typeof(ProBuilderMesh), typeof(PositionToolContext))]
class CutTool : EditorTool
public class CutTool : EditorTool
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should make that sealed, so that it's possible to reference it but that should be all.

Suggested change
public class CutTool : EditorTool
public sealed class CutTool : EditorTool

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm a fan of sealing things that are not made to be extended. So thumbs up for that.

[Icon("Packages/com.unity.probuilder/Content/Icons/EditableMesh/EditMeshContext.png")]
[EditorToolContext("ProBuilder", typeof(ProBuilderMesh))]
class PositionToolContext : EditorToolContext
public class PositionToolContext : EditorToolContext
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should make that sealed, so that it's possible to reference it but that should be all.

Suggested change
public class PositionToolContext : EditorToolContext
public sealed class PositionToolContext : EditorToolContext

}

class TextureToolContext : EditorToolContext
public class TextureToolContext : EditorToolContext
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should make that sealed, so that it's possible to reference it but that should be all.

Suggested change
public class TextureToolContext : EditorToolContext
public sealed class TextureToolContext : EditorToolContext

Comment on lines +50 to +68
// Disable Snap for the individual handles. Snap is applied if movement is detected.
const float SnapFactor = 0.0f;

m_Position = Handles.Slider2D(m_Position,
Vector3.forward,
Vector3.right,
Vector3.up,
HandleUtility.GetHandleSize(m_Position) * .2f,
Handles.RectangleHandleCap,
0f,
SnapFactor,
false);

Handles.color = Color.green;

m_Position = Handles.Slider(m_Position, Vector3.up);
m_Position = Handles.Slider(m_Position, Vector3.up, HandleUtility.GetHandleSize(m_Position), Handles.ArrowHandleCap, SnapFactor);

Handles.color = Color.red;

m_Position = Handles.Slider(m_Position, Vector3.right);
m_Position = Handles.Slider(m_Position, Vector3.right, HandleUtility.GetHandleSize(m_Position), Handles.ArrowHandleCap, SnapFactor);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These can be removed as I'm landing a fix on another PR :D

@lopezt-unity
Copy link
Copy Markdown
Collaborator

Hey @CortiWins,
I will have to spend a little more time on this.
Making these class public is also exposing API from inside these class that does not need to be exposed 😓
I will have to take a look at that and make some additional changes.

@CortiWins
Copy link
Copy Markdown
Author

@lopezt-unity I am not that surprised by the answer.

The whole concept of tools and contexts is a bit...semi public right now. I helped on an addon called "ProBuilderPlus" by ex-Unity ProBuilder creator GabrielW and that required a bit of reflection to use the ToolManager.Tool and ToolManager.Context functions.
Setting them via Types and reading and comparing the Type works fine as soon as yo have them.
But the ones Unity uses for different editor functions, including ProBuilder, are not public so it's having a public API, but lacking the access to really use it.

Maybe a compromise is a property that returns the type like

public static ProBuilderContexts
{
  System.Type CutToolType => typeof(CutTool);
}

Because the use of knowing the exact Types is not changing something about their instances, but using them for setting and comparing ToolManager.Tool and ToolManager.Context throught ToolManager.SetActiveTool(Type)/ToolManager.SetActiveContextType).

@lopezt-unity
Copy link
Copy Markdown
Collaborator

@lopezt-unity I am not that surprised by the answer.

The whole concept of tools and contexts is a bit...semi public right now. I helped on an addon called "ProBuilderPlus" by ex-Unity ProBuilder creator GabrielW and that required a bit of reflection to use the ToolManager.Tool and ToolManager.Context functions. Setting them via Types and reading and comparing the Type works fine as soon as yo have them. But the ones Unity uses for different editor functions, including ProBuilder, are not public so it's having a public API, but lacking the access to really use it.

Maybe a compromise is a property that returns the type like

public static ProBuilderContexts
{
  System.Type CutToolType => typeof(CutTool);
}

Because the use of knowing the exact Types is not changing something about their instances, but using them for setting and comparing ToolManager.Tool and ToolManager.Context throught ToolManager.SetActiveTool(Type)/ToolManager.SetActiveContextType).

Sorry, I should express a little better :)
I totally agree that these class should be public :) but it should be done the right way and that might involve to have a look inside these classes and be certain that the correct APIs are exposed/internal :)
It's just some extra work we'll have to do :) I need to plan for some extra time on that, but it'll be done :D

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