-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TransformControls: Derive from Controls
.
#29146
Conversation
@@ -89,7 +88,9 @@ | |||
scene.add( mesh ); | |||
|
|||
control.attach( mesh ); | |||
scene.add( control ); | |||
|
|||
const gizmo = control.getGizmo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrdoob Are you okay with getGizmo()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arodic How does this change look to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getGizmo()
is okay 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, getHelper()
would follow the current naming:
https://github.com/mrdoob/three.js/tree/dev/examples/jsm/helpers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransformControls.html
You forgot to delete these:
- <h3>[property:Object3D object]</h3>
- <p>
- The 3D object being controlled.
- </p>
@puxiao Actually I had to restore some deleted docs. The The thing is |
Rome wasn't built in a day. By the way, you forgot to delete these:
<h2>Methods</h2>
<p>See the base [page:Controls] class for common methods.</p>
- <h3>[method:undefined connect] ()</h3>
- <p>
- Adds the event listeners of the controls.
- </p>
- <h3>[method:undefined disconnect] ()</h3>
- <p>
- Removes the event listeners of the controls.
- </p>
- <h3>[method:undefined dispose] ()</h3>
- <p>
- Should be called if the controls is no longer required.
- </p>
<h2>Source</h2> |
@Mugen87 Will these PRs be released in r168? |
I'd like to wait for at least one feedback (see #29146 (comment)) so the API change is approved. Hence, the change might not be included in |
It has been a month since the PR was filed so I'm going ahead and merge it so we can finish the migration to Of course we can update the API at any point if things like |
* TransformControls: Derive from `Controls`. * Docs: Clean up. * TransformControls: Clean up. * Docs: Clean up. * Docs: Clean up.
* TransformControls: Derive from `Controls`. * Docs: Clean up. * TransformControls: Clean up. * Docs: Clean up. * Docs: Clean up.
Updated from 167 to 169. Traverse was part of |
Related issue: #29085
Description
Same as #29085.
This is a breaking change though.
TransformControls
was derived fromObject3D
so far which had pros and cons. You could add the controls directly to the scene to render the gizmo (the visual helpers) but theObject3D
methodsattach()
anddetach()
were overwritten with control specific methods.The new approach is conceptually cleaner and more consistent compared to all other controls. But since
TransformControls
is no 3D object anymore, developers must add the gizmo differently now. So instead of:It is now: