Skip to content
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.js: Added reset() method #22972

Merged
merged 6 commits into from
Dec 8, 2021
Merged

TransformControls.js: Added reset() method #22972

merged 6 commits into from
Dec 8, 2021

Conversation

vHeemstra
Copy link
Contributor

@vHeemstra vHeemstra commented Dec 6, 2021

Description
There was no (easy) way to cancel/abort a transform action. The pull request adds that functionality.

Added method to reset object to state when the transform dragging started. Further transform action can be stopped immediately after resetting the object by using the optional parameter set to true.

Changes from outdated related prior pull request: #21661
The prior pull request involved two methods (cancel and stop). Looking at what the added functionality is, naming it reset seems more appropriate. Also, the added stop method was overly complex, so the optional unbinding of the pointermove event is included in the reset method.

Resolves #21655, closes #21661

Added method to reset object to state when the transform dragging started. The transform action can also be stopped immediately after by using the optional parameter set to `true`.
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 7, 2021

Also, the added stop method was overly complex, so the optional unbinding of the pointermove event is included in the reset method.

Would it be possible to remove the optional parameter and not touch the event listener?

Other controls also have reset() methods and they do not modify the event listeners, too. Would be good to be consistent in that regard.

vHeemstra and others added 2 commits December 7, 2021 11:51
Removed the optional parameter for stopping transform after reset.
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 8, 2021

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 8, 2021

After some tests I'am wondering if it isn't better to reset _positionStart, _quaternionStart and _scaleStart in reset(), too.

If you currently translate an object and call reset(), the object "jumps" to the last translated position when you proceed the transformation. That feels a bit strange.

@vHeemstra
Copy link
Contributor Author

vHeemstra commented Dec 8, 2021

The idea for this feature was that you had the possibility to cancel the current transform, as a sort of undo. So that you could revert the transform while you are still dragging. I'm not sure if this is similar to the reset methods of other controls, so maybe it needs to be renamed if it isn't consistent?

The jumping of the object is the desired result in this case. The previous (now removed) optional parameter would make it possible to immediately stop any further transform of that dragging action. (Mouse up and a new mouse down would start a new transform if needed.)

What would be the behavior you expected? Or what would feel less strange? Do you mean like a transition animation instead of snapping back? (Option A)
Or do you mean that after the reset the object would transform from that position/scale/rotation onward, like when you would start a new transform drag? (Option B)

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 8, 2021

I think option B would feel best.

@vHeemstra
Copy link
Contributor Author

Ok, I'll have a go at that.

Continue transform from the reset state.
@vHeemstra
Copy link
Contributor Author

Updated:
After reset is called, the transform continues from the new (reset) state.

Note:
The downside in ui experience here is that the transform magnitude might me smaller or larger than you would expect, because the pointer starts at a point further from the object. Although this was already the case when starting the dragging further away on the handles, but now it becomes more visible.

For example:

  1. Open the example
  2. Drag the red transform arrow to the left and keep the mouse button pressed.
  3. Press Esc
  4. Move the mouse back to the position where you started the dragging.
  5. Release the mouse button.

Result
The object will end up further to the right than it started. This is because the traveled mouse distance (after the reset) is scaled proportionally to the 3D position it points to at reset.

Updated the `js` version as well.
Added missing documentation for `setScaleSnap()` and moved `reset()` to the correct alphabetical place.
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 8, 2021

Thanks for highlighting the details! I think the implemented approach feels good now.

@mrdoob mrdoob added this to the r136 milestone Dec 8, 2021
@mrdoob mrdoob merged commit 098003f into mrdoob:dev Dec 8, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 8, 2021

Thanks!

@joshuaellis joshuaellis mentioned this pull request Dec 19, 2021
9 tasks
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.

Adding cancel method to TransformControls helper
3 participants