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 precise rotation control to osu! editor #24567

Merged
merged 9 commits into from
Aug 19, 2023
Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Aug 16, 2023

2023-08-16.11-42-13.mp4

A few notes:

  • Rotation is clamped to [-180deg, 180deg] range. Textbox inputs above that will be clamped to the range on focus loss/commit (not shown on above video)
  • Stable also has a direction toggle (clockwise/counterclockwise). I didn't add it back as it seemed pointless and led to ambiguity (180deg CCW = -180deg CW)
  • The hotkey is Ctrl-R to avoid clash with the global "random skin" binding.

@Walavouchey
Copy link
Member

i haven't tried this branch but instead of clamping, wouldn't it be better to conveniently convert the angle to the equivalent one within -180 and 180? then if you have say 240 in your head you can type that instead of doing mental arithmetic (to get -120)

@peppy peppy self-requested a review August 17, 2023 08:59
@peppy
Copy link
Member

peppy commented Aug 17, 2023

i haven't tried this branch but instead of clamping, wouldn't it be better to conveniently convert the angle to the equivalent one within -180 and 180? then if you have say 240 in your head you can type that instead of doing mental arithmetic (to get -120)

The best compromise if we want this would be to change the allowed range to -360 .. 360:

diff --git a/osu.Game.Rulesets.Osu/Edit/PreciseRotationPopover.cs b/osu.Game.Rulesets.Osu/Edit/PreciseRotationPopover.cs
index 86112d9751..f09d6b78e6 100644
--- a/osu.Game.Rulesets.Osu/Edit/PreciseRotationPopover.cs
+++ b/osu.Game.Rulesets.Osu/Edit/PreciseRotationPopover.cs
@@ -45,8 +45,8 @@ private void load()
                     {
                         Current = new BindableNumber<float>
                         {
-                            MinValue = -180,
-                            MaxValue = 180,
+                            MinValue = -360,
+                            MaxValue = 360,
                             Precision = 1
                         },
                         Instantaneous = true

@bdach thoughts on this? I think it's probably okay, and may lead to a better UX (more flexibility at the cost of less precision when dragging the slider, but let's be real: this control is mainly for typing a value in).

@bdach
Copy link
Collaborator Author

bdach commented Aug 17, 2023

No objections to changing to [-360, 360]. While @Walavouchey's proposal is technically feasible, it'd be rather finicky to implement given the structure of things.

Will apply when able (unless I get preempted).

@peppy
Copy link
Member

peppy commented Aug 17, 2023

I can make that change, but curious if you had anything lined up to fix the popover tests already?

@bdach
Copy link
Collaborator Author

bdach commented Aug 17, 2023

Hmm, wasn't expecting them to fail. Pretty sure they were green locally as is tradition. You can leave be, I'll look closer on Saturday.

@Theighlin
Copy link

I'm not a mapper, but have a couple of suggestions upon trying the branch:

  • Is there a keybind to change from playfield centre to selection centre? I instinctively tried using TAB, but it didn't work.
  • Pressing enter does not close the popover. This is helpful if the value inserted is not correct and needs to be changed, but i'd say it's better to just have it close (for when there is no doubt about the operation) and undo and rotate again via keybinds if needed (enter should close the popover but not cancel the selection).
  • Textbox's "0" is not selected when opening the popover, typing the numbers after the 0 directly works fine, but looks weird and doesn't work for negative numbers. (waiting on Allow selecting all text in a textbox programatically osu-framework#5823 ?)

Sorry if these were already known / out of scope of this PR / there are reasons i don't know it was done this way.

@bdach
Copy link
Collaborator Author

bdach commented Aug 19, 2023

Test failures should be resolved. Allowed rotation range expanded to [-360, 360].

Is there a keybind to change from playfield centre to selection centre? I instinctively tried using TAB, but it didn't work.

There is not. I intentionally did not make one because reproducing winforms tab behaviour would have been a massive pain (because presumably people would want to be able to tab between the textbox and the toggle or whatever).

Pressing enter does not close the popover. This is helpful if the value inserted is not correct and needs to be changed, but i'd say it's better to just have it close

Need more opinions on this because I prefer it the way it is. Compare: photoshop free transform tool.

Textbox's "0" is not selected when opening the popover, typing the numbers after the 0 directly works fine, but looks weird and doesn't work for negative numbers.

This requires the PR you mention yes.

@peppy peppy merged commit a7332cc into ppy:master Aug 19, 2023
@bdach bdach deleted the precise-rotation-2 branch August 20, 2023 16:53
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.

Mappers coming from stable expect key binding brings up a rotate control
4 participants