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

Make editor flip, rotate, and scale tools revolve around the grid center #26311

Merged
merged 30 commits into from
Sep 19, 2024

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Jan 1, 2024

Part of #26303

Requires

Changes how the Ctrl+H and Ctrl+J flip mechanic works. It now takes into account the origin and rotation of the grid to flip around that axis. This makes it easy to map symmetric patterns which are off-angle or not around the center of the playfield.
Also the Ctrl+R rotation tool uses the grid origin as the center of the playfield.
The precision scale tool uses the grid origin as the center of the playfield and uses the grid rotation for axis constrained scaling (for scaling only along the X or Y axis)

@OliBomby OliBomby changed the title Make editor flip and rotate tools revolve around the grid center Make editor flip, rotate, and scale tools revolve around the grid center Jul 3, 2024
@bdach
Copy link
Collaborator

bdach commented Jul 4, 2024

I don't hate the idea, but I quite dislike the execution.

  • If you asked me how to implement this in a vacuum, I would say that this should be a third option of origin for flip/rotate operations, rather than replacing the playfield one. As in, it should still be possible to use playfield centre as a flip/rotate origin after the grid has been moved around.
  • The rotate/scale popover buttons still say "playfield centre". After the grid has been moved, they're clearly not that.
  • On any grid type other than square, flip respecting grid rotation and origin makes zero visual sense. Like why is that stuff respected still on a circular grid, where you only know where the origin is, but have no sense of the rotation of it? On a triangular grid, why do flips relative to grid continue to work as they did on square grid?

I will go ask around for feedback somewhere but this is not feeling great in my view. If you asked me how to implement it, it would be a third option rather than replacing playfield origin, and it would be limited to square grid for flipping.

@OliBomby
Copy link
Contributor Author

OliBomby commented Jul 4, 2024

I'm kind of surprised by this reaction because it felt pretty awesome and intuitive to use for me.

  • If you asked me how to implement this in a vacuum, I would say that this should be a third option of origin for flip/rotate operations, rather than replacing the playfield one. As in, it should still be possible to use playfield centre as a flip/rotate origin after the grid has been moved around.

I don't think a third option is necessary. If the user has moved the grid, then they are clearly not working with playfield center symmetry, so it wouldn't make sense to allow that. If the user specifically means to flip around the playfield center, then they can just reset the grid settings. Also there is no way to specify an origin option for the flip operations since that's only a hotkey.

  • The rotate/scale popover buttons still say "playfield centre". After the grid has been moved, they're clearly not that.

Fair enough, it's technically not the playfield centre. I kinda meant to stretch the definition of "Playfield centre" to equal "Grid centre", like you are able to control where the playfield centre is. I'm not against renaming the buttons to make them more accurate.

  • On any grid type other than square, flip respecting grid rotation and origin makes zero visual sense. Like why is that stuff respected still on a circular grid, where you only know where the origin is, but have no sense of the rotation of it? On a triangular grid, why do flips relative to grid continue to work as they did on square grid?

I always meant the grid types to only affect snapping and to just be different representations of the same underlying grid configuration. If the everything starts behaving differently based on grid type that would break that notion and just be more confusing I think.

I'll admit on the circular grid it does look a bit weird because there is no sense of rotation, but its nice to have that customizability. Maybe the mapper is using a specific rotated symmetry in their map and only temporarily enabled the circular grid. In that case its nice to be able to still use that rotation. That's another reason why I was against disabling the rotation slider in #26310

@OliBomby
Copy link
Contributor Author

@bdach
I went and implemented all your feedback.

The flip functionality on hexgrid now takes the axes on the grid that are closest to the direction that you want to flip, so you always flip along the axes of the grid and it makes sense visuallly. The circular grid defaults to 0 rotation flips. It felt wrong to disable flipping entirely for circular grids.

The precise rotation/scale popovers now show "Grid centre" as an origin option which is the first option.
I was considering disabling the grid centre origin if its equivalent to using playfield centre so it defaults to "Playfield centre", but that feels a bit off to disable it since it still works perfectly fine in that setting.

@bdach
Copy link
Collaborator

bdach commented Jul 16, 2024

Tests failing

@bdach
Copy link
Collaborator

bdach commented Jul 16, 2024

The flip functionality on hexgrid now takes the axes on the grid that are closest to the direction that you want to flip, so you always flip along the axes of the grid and it makes sense visuallly.

Dunno how to feel about this, feels essentially random:

2024-07-16.12-42-54.mp4

This is all going to be very dependent on mapper feedback. So the options are to either get this out and bear the possible backlash/hope there is none or get experienced mapper(s) on board to provide early feedback. I'll ask around.

@OliBomby
Copy link
Contributor Author

I noticed an error in how it determines the axis for the horizontal flip in your video. Ctrl+H is supposed to do a straight horizontal flip (mirroring over the vertical axis) when the grid rotation is 30/-30.
I fixed it so it should feel a bit less random now.

@bdach
Copy link
Collaborator

bdach commented Jul 22, 2024

When I asked the NAT the best idea of how to make flip work with triangle grid that I got was implementing a flow to actually select an axis for the flip rather than guessing / approximating and I do agree with that assessment.

Either that or just straight up disable flip with triangular grid I guess.

@OliBomby
Copy link
Contributor Author

When I asked the NAT the best idea of how to make flip work with triangle grid that I got was implementing a flow to actually select an axis for the flip rather than guessing / approximating and I do agree with that assessment.

What should that flow look like? Maybe you can hold down the flip key and move your cursor to one of the six partitions made by the axes of the grid and then when you release the key it flips to that paritition.

Either that or just straight up disable flip with triangular grid I guess.

I disagree with disabling flip entirely. I think a slightly confusing implementation is at least better than having no tools at all.

@bdach
Copy link
Collaborator

bdach commented Jul 22, 2024

Maybe you can hold down the flip key and move your cursor to one of the six partitions made by the axes of the grid

It shouldn't be hold. I'd see the editor entering a special "modal" state wherein the axes highlight as you move mouse. Left mouse / enter to do the flip, right mouse / escape to not.

@OliBomby
Copy link
Contributor Author

OliBomby commented Aug 8, 2024

It shouldn't be hold. I'd see the editor entering a special "modal" state wherein the axes highlight as you move mouse. Left mouse / enter to do the flip, right mouse / escape to not.

I'm not sure about this flow because mouse down ending the movement is unlike anything we've done elsewhere. I'd rather have the flip works as it does currently, or select an axis to flip on based on your mouse position when you press/release the flip hotkey.

@peppy
Copy link
Member

peppy commented Aug 9, 2024

When I asked the NAT the best idea of how to make flip work with triangle grid that I got was implementing a flow to actually select an axis for the flip rather than guessing / approximating and I do agree with that assessment.

Honestly I disagree with this. Just let the mapper use trial and error to get it right. It's such an edge case we shouldn't be overloading already complex user interactions just to save a few trial-until-succeed attempts.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Considering the UX issues are more or less resolved to my liking, here's a code review

osu.Game/Utils/GeometryUtils.cs Show resolved Hide resolved
osu.Game/Utils/GeometryUtils.cs Show resolved Hide resolved
@@ -20,21 +20,25 @@ public partial class PreciseScalePopover : OsuPopover
{
private readonly OsuSelectionScaleHandler scaleHandler;

private readonly Bindable<PreciseScaleInfo> scaleInfo = new Bindable<PreciseScaleInfo>(new PreciseScaleInfo(1, ScaleOrigin.PlayfieldCentre, true, true));
private readonly OsuGridToolboxGroup gridToolbox;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm somewhat uneasy with drawables being shoved into other drawables like this just for the sake of sharing data. Can the grid configuration be shipped into like a record, and then we can use bindables for this purpose rather than shoving components in wholesale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried implementing this with passing bindables but it just doesnt work. The bindables that you pass to the transform popovers get disposed when so you have to chain bindable updates, turning into a big mess. Also when I tried binding a bindable in the popover to a bindable you pass in the constructor, it would be null once the popover loads.

Maybe injecting drawables into other drawables isn't that bad. It's also being done with many other components and it seems to work fine.

osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

seems fine?

@peppy requesting UX pass

@bdach bdach requested a review from peppy September 19, 2024 09:03
@peppy peppy merged commit 9376ba3 into ppy:master Sep 19, 2024
7 of 9 checks passed
@OliBomby OliBomby deleted the grids-3 branch September 19, 2024 09:46
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.

5 participants