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

Add temporary pivot for rotating multiple 2D nodes #58375

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Feb 21, 2022

Closes godotengine/godot-proposals#3684
godot windows tools 64_zJr9fwPUZV

You can set a temporary pivot, which will be used as rotation origin for the selected nodes, in global coordinates. It's very useful when e.g. placing objects on your level (using a parent node as rotation point is very inconvenient, because usually you don't need it after rotation). The pivot disappears when selection changes.

Submitting as draft, as I didn't know where to put it xd Right now it uses hard-coded P shortcut. I've been thinking about 3 options: Resolved.

  • add a "Rotation Pivot" tool that would set the temporary pivot
  • use the existing Pivot Tool and just make it so if you have multiple nodes selected, the temporary pivot would be set instead (which means you wouldn't be able to change pivot of multiple nodes)
  • add a modifier for Pivot Tool, e.g. Alt. Then Alt+V would set the temporary pivot in Select mode

I like the third one the best, but it's probably up to discussion. The code uses the easiest option until this is settled.

@KoBeWi KoBeWi added this to the 4.0 milestone Feb 21, 2022
@KoBeWi KoBeWi marked this pull request as draft February 21, 2022 00:31
@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 24fc10a to 6cdd5b6 Compare April 6, 2022 13:01
@KoBeWi KoBeWi marked this pull request as ready for review April 6, 2022 13:01
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 6, 2022

Ok I went with the third option. Alt with Pivot Tool moves temporary pivot (I had to make the tool active for any CanvasItem) and Alt + V changes temp pivot for selection tool.

@KoBeWi KoBeWi requested a review from a team April 6, 2022 13:03
@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 6cdd5b6 to f627d83 Compare May 16, 2022 18:02
@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from f627d83 to 1a57b7e Compare August 16, 2022 12:05
@clayjohn clayjohn requested a review from groud August 18, 2022 15:37
@clayjohn clayjohn modified the milestones: 4.0, 4.x Aug 18, 2022
@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 1a57b7e to 674ac2b Compare September 28, 2022 11:26
@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 674ac2b to 96d50b7 Compare November 5, 2022 20:55
@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 96d50b7 to 8c12c69 Compare March 7, 2023 02:01
@Calinou
Copy link
Member

Calinou commented Jun 1, 2023

Tested locally (rebased on latest master), it works. I think it's a good start, but there are further ways to improve usability:

  • When you have multiple nodes selected, there is no visual feedback for the pivot location in the 2D editor after setting its position, nor whether it's permanent or temporary.
  • The Alt modifier conflicts with window manipulation keys on many Linux window managers. I suggest adding an alternative.
  • When selecting multiple nodes, could there be a quick way to place the pivot in the center of all selected nodes? Likewise, in the rotate tool, there could be a modifier to ignore the rotation pivots and rotate all selected nodes individually.

@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 8c12c69 to 9044547 Compare June 1, 2023 16:12
@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 1, 2023

When you have multiple nodes selected, there is no visual feedback for the pivot location in the 2D editor after setting its position, nor whether it's permanent or temporary.

Fixed, I think. I couldn't really reproduce the problem, but I noticed the pivot did not appear when you click, only when you drag.

The Alt modifier conflicts with window manipulation keys on many Linux window managers. I suggest adding an alternative.

Not this again 🙄 I think we decided that we don't care about window managers doing stupid things with Alt.
What alternative would you suggest? Note that the temp pivot works with selection tool, i.e. you can use Alt+V in that mode. Which means that either the pivot tool needs a new shortcut (which would make it inconsistent with the select shortcuts) or both tools need an alternative. Ctrl is not possible (because Ctrl+V) and Shift... idk.

When selecting multiple nodes, could there be a quick way to place the pivot in the center of all selected nodes?

I added an Alt action when you press the Pivot tool. Kind of weird, but it works.

Likewise, in the rotate tool, there could be a modifier to ignore the rotation pivots and rotate all selected nodes individually.

I don't think it's worth it, as it complicates things. Shortcut before dragging (e.g. hold Ctrl before starting rotating) would make it inconsistent with select tool and shortcut while rotating (e.g. hold Shift during rotation action) would result in toggling the pivot mid-rotation, which would either teleport the nodes or remember their position, which again is a hassle to handle xd
Removing the pivot involves changing node selection, so I think it's not really a problem.

@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 9044547 to 00f50b3 Compare December 15, 2023 22:32
@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 15, 2023

Changed the modifier to Shift (it doesn't conflict with anything, so whatevs) and fixed a crash.

@KoBeWi KoBeWi removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 26, 2024
@KoBeWi KoBeWi force-pushed the temporary_pivot_is_temporary branch from 00f50b3 to f8cd3bb Compare April 26, 2024 09:13
@akien-mga
Copy link
Member

Tested and this seems to work well.

The discoverability isn't the best, but that might come from me not actually knowing much of the existing hotkeys and tools.

I'm not sure how to use the current Pivot tool xD
But I did manage to place the temp pivot after selecting it and clicking with Shift, though it's a bit of a bother to then have to select the rotation tool manually to use it.

And from reading the PR I saw that Shift+V when in Select mode does place the temp pivot, which is nice. But is just "V" supposed to place the main pivot? Because that doesn't seem to work for me. I guess it may not work in multi-select?

The Shift+V behavior could be added to the (already lengthy) tooltip for Select Mode.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 26, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Works well for the most part, might need some iteration on the UX / discoverability.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 26, 2024

But is just "V" supposed to place the main pivot?

Yes:

MYIFnPQoB5.mp4

Changing pivot involves changing node's position and setting the pivot so that the visual part stays in place. The pivot is not displayed when multiple nodes are selected, but setting it still works.

The Shift+V behavior could be added to the (already lengthy) tooltip for Select Mode.

Shift+V is "inherited" from pivot tool. It's the same as Shift in scale tool, which is only mentioned in the scale tooltip, but you can use it in select mode too.

@akien-mga
Copy link
Member

Ok changing the pivot with "V" seems to work with a Sprite2D, but doesn't work with a CharacterBody2D. How come?

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 26, 2024

Because CharacterBody2D doesn't have pivot or offset property. "Pivot" refers to rotation center, for nodes without size it's always equal to their position.

@akien-mga
Copy link
Member

Right, that makes sense. But that's still confusing to me, as if I multi-select 3 CharacterBody2D's, I can give them a temp pivot and rotate them fine :D
Likewise with a single selection, the temp pivot can be set, but the normal pivot can't.

@akien-mga akien-mga merged commit 610a9be into godotengine:master Apr 26, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An ability to set a temporary "pivot" for rotating selected nodes around it in editor.
4 participants