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 hexgrid and circular grid to the osu editor #26310

Merged
merged 12 commits into from
Jul 3, 2024
Merged

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Jan 1, 2024

Part of #26303

Requires:

Adds hex grids and circular grids and the ability to toggle between them

This caused issues in rendering the outline of the grid because the outline was getting masked at some resolutions.
@bdach bdach self-requested a review June 6, 2024 12:08
@bdach
Copy link
Collaborator

bdach commented Jun 6, 2024

Initial five-minute impressions without looking at the actual code:

  • The triangular grid seems neat but the adjustment bounds (specifically rotation) seem weird. With triangular grid the rotation bounds should be [-30deg, 30deg] not [-45deg, 45deg] (because of central symmetry only 60deg of movement is required to get all possible spatial orientations)
  • I'm not sure how to feel about the mere existence of the circular grid. We already have a circular grid, it's just beat-snapped. Is a non-snapped circular grid even going to be useful to anyone? As in, a circular grid to me can seemingly only be used for either making circles of keeping distances equal between objects, at which point you probably want grid snap too for constant movement velocity.
  • When circular grid and beat snap grid are active with low spacing, the intersection snapping feature breaks down in that it's hard to even predict what is going to happen, especially when both sets of circles begin to be near-concentric:
2024-06-06.14-20-16.mp4

Will likely need general feedback from @ppy/team-client as to how much of this we actually want before proceeding with proper structural review.

@OliBomby
Copy link
Contributor Author

OliBomby commented Jun 6, 2024

  • The triangular grid seems neat but the adjustment bounds (specifically rotation) seem weird. With triangular grid the rotation bounds should be [-30deg, 30deg] not [-45deg, 45deg] (because of central symmetry only 60deg of movement is required to get all possible spatial orientations)

I think the rotation bounds should not change, in order to make the interaction more seamless when switching between grid types. For example when you set the rotation to 45 degrees on the square grid, change the grid type to hex and rotation gets clipped to 30 degrees, change the grid type back to square grid. In that scenario your grid rotation changes due to changing the grid type and the user has to manually set it back, which is not acceptable UX.

  • I'm not sure how to feel about the mere existence of the circular grid. We already have a circular grid, it's just beat-snapped. Is a non-snapped circular grid even going to be useful to anyone? As in, a circular grid to me can seemingly only be used for either making circles of keeping distances equal between objects, at which point you probably want grid snap too for constant movement velocity.

The distance snap grid is not suitable for any geometry snapping usecase besides its intended distance snap mapping. It can not snap slider control points, and you can only have the previous hit object as the origin.

The circular grid is very useful is many scenarios, like ensuring equal distance between some objects that is not the previous hit object, or making circular blanketing sliders. Combined with the 'grid from points' feature in the later PR, the circular grid will be very quick to set up, so it will definitely be more convenient than the distance snap grid.

  • When circular grid and beat snap grid are active with low spacing, the intersection snapping feature breaks down in that it's hard to even predict what is going to happen, especially when both sets of circles begin to be near-concentric:

The 'intersection snapping feature' seems to be working as intended by first snapping to the beat snap grid and then snapping to the PositionSnapGrid. This seems to approximate intersections for the grid types that consists just of points (square, hex) because these dont have many actual intersections between the points and circles, but for the circular grid where there do exist many circle-circle intersections (if the circles are not near-concentric) it does seem preferrable if it prioritized these actual intersections in the snapping. Though I'm not sure if its worth the trouble implementing.

@bdach
Copy link
Collaborator

bdach commented Jun 6, 2024

For example when you set the rotation to 45 degrees on the square grid, change the grid type to hex and rotation gets clipped to 30 degrees, change the grid type back to square grid

If such interactions are a worry (I'm not sure that they should be), then the way to fix that would be to store the grid settings local to the grid type. As in, you could change things on square grid, then go to triangle, change whatever and then go back to square which would continue to use original settings.

On a similar point, the rotation slider should be disabled on the circular grid because it does nothing useful.

@OliBomby
Copy link
Contributor Author

OliBomby commented Jun 6, 2024

If such interactions are a worry (I'm not sure that they should be), then the way to fix that would be to store the grid settings local to the grid type. As in, you could change things on square grid, then go to triangle, change whatever and then go back to square which would continue to use original settings.

On a similar point, the rotation slider should be disabled on the circular grid because it does nothing useful.

I don't want to change it in this direction. I think it makes the behaviour overly complicated and less seamless.

With grid type-specifc settings, the experience is not seamless. If the user wants a grid in some position and use different grid types, then he has to manually reset the position every time he switches type. That's not seamless.

The rotation slider for the circular grid is not irrelevant. If the settings carry over between types like it does now, then this would allow you to set the rotation before you switch to a grid type which requires the rotation.

I think its good UX that you can have the same behaviour with a different order of operations. Either you set the settings first and then change grid type, or you change the grid type and then change the settings. I want it to be identical either way.

@bdach
Copy link
Collaborator

bdach commented Jun 7, 2024

I don't want to change it in this direction. I think it makes the behaviour overly complicated and less seamless.

With grid type-specifc settings, the experience is not seamless. If the user wants a grid in some position and use different grid types, then he has to manually reset the position every time he switches type. That's not seamless.

Well if things are going to lean that way then the rotation slider should just have [-180deg, 180deg] range and not [-45deg, 45deg] which is specific to square grid.

The rotation slider for the circular grid is not irrelevant. If the settings carry over between types like it does now, then this would allow you to set the rotation before you switch to a grid type which requires the rotation.

Why would you want that though? The rotation slider does nothing for circular grid. You can't even see it do anything. Why would you want to adjust a grid based on it not moving at all?

@OliBomby
Copy link
Contributor Author

OliBomby commented Jun 7, 2024

Well if things are going to lean that way then the rotation slider should just have [-180deg, 180deg] range and not [-45deg, 45deg] which is specific to square grid.

With a full [-180deg, 180deg] range the precision of the slider bar is compromised. I'd say keep it on [-45deg, 45deg] range to increase precision since that range happens to be exactly enough for all grid types.

Why would you want that though? The rotation slider does nothing for circular grid. You can't even see it do anything. Why would you want to adjust a grid based on it not moving at all?

You're right the rotation slider does nothing for the circular grid, but it does do something for the other grid types. For example someone might attempt to first change rotation and then change grid type from circular to something else. Such an action is both reasonable and productive. Why go out of your way to take away that action? I want to give the users as much freedom as possible.

@bdach
Copy link
Collaborator

bdach commented Jun 10, 2024

Yeah I dunno. Will need a third opinion here because the visions for this feature are clearly divergent at this point.

My point is that I wish to guide users into understanding what does what and why things have constraints and limits that they do by interacting with them directly rather than giving them too many options and then having them wonder why some of them are redundant or useless.

@minetoblend
Copy link
Contributor

minetoblend commented Jun 10, 2024

After testing this, I think the experience is currently very frustrating. However I think that most of these issues come from the overall way the right side-menu works and less from the grid-specific menu.

I'll briefly include the frustrations I've had with editing the grid that aren't directly part of this PR as well, though they probably warrant their own discussion post where I go into more detail

Issues with the side menu/grid overall

  • The expand/collapse behaviour of the menu is very annoying since clicking it doesn't seem to trigger it and instead it expands on hover with a delay, making the menu feel very unresponsive
  • The grid is barely visible on 4k screens, I have to use background dim >75% to be able see it well Turns out this was my monitor struggling with contrast for this particular combination of colors, fixed by changing some monitor settings
  • The hamburger button toggle next to the menu section has no visible effect on click, leaving me confused about what it actually does at first
  • When the grid menu is collapsed, and my cursor touches the grid box on it's way to the rotate/scale buttons, the grid menu expands on hover and will push the buttons I actually wanted to click off-screen.
2024-06-10.12-01-05.mp4
  • The rectangular & triangle grid appear to have no anti-aliasing, while the circle grid does
  • The layout of the menu generally just completely changes on hover, my mouse cursor basically never lands on the thing I originally hovered on after it expanded
  • The button to toggle grid snap should be paired with the grid settings imo, I had to look for it when I wanted to turn it on only to find it in very bottom left of the screen (same thing goes for distance snap too)

Grid menu when sidebar is collapsed

image

  • I think the 1-letter abbreviations for the values are kinda confusing (R could mean Rotation, Radius, ...). There should be enough space to actually write out the words rotation and spacing, so no value in using these abbreviations. The angle should probably also have a degree symbol at the end.
  • The collapsed menu doesn't show the type of grid currently selected
  • The collapsed menu wastes a lot of space with margin between the individual lines
  • I think having these values shown at all is kind of pointless because I can just look at the actual grid which tells me so much more than those values.

Grid menu in expanded state

  • There should be text fields for each of the values. I don't wanna waste time micro-adjusting the sliders to get the correct values dialed in when I can just type it out
  • For the rotation limits, my suggestion is the following
    • The slider limits should be changed to match the range that makes sense for each grid type, and hidden for the circle grid
    • The sliders should have "soft" limits: when you change from a grid with -45/45 range to a grid with -30/30 range, the value should be kept, and the limits only applied once I touch the slider
    • The rotation text box (if there is one) should not try to somehow fit the values inside the given range (Software changing my values on enter is just super annoying and never ever feels good).
    • Buttons to increment/decrement the value by 1 would also be nice to have on the left/right side of the slider/text field
  • This is probably out of scope for this PR, but it would be nice to have draggable "handles" on the playfield while editing the grid. Using x/y/scale/rotation/whatever is generally a bit of a pain when making adjustments to the grid. I'd only really wanna use those if I want precise values, but not when just adjusting the grid by "vibe"
  • The grid type buttons use a lot of vertical space right now since they're arranged vertically. I think having either a horizontal button group would or a dropdown select would work better here.

Other thoughts/ideas

  • This should probably be inside a popup menu, don't think there's a need for it to use this much space in the side menu, especially since it currently leads to the issue of pushing other menus off-screen on hover
  • I would like a keyboard shortcut for bringing up the grid menu (so I can avoid having to use the sidebar)
  • It would be cool to have the option to completely disable the grid

@OliBomby
Copy link
Contributor Author

OliBomby commented Jun 14, 2024

I'm gonna limit it to stuff relevant to changes in this PR.

The collapsed menu doesn't show the type of grid currently selected

Just looking at the grid is enough to tell you which grid is selected.

The slider limits should be changed to match the range that makes sense for each grid type, and hidden for the circle grid
The sliders should have "soft" limits: when you change from a grid with -45/45 range to a grid with -30/30 range, the value should be kept, and the limits only applied once I touch the slider

I think the "soft" limits idea is a reasonable middle-ground. I'll just have to rewrite slider bars so they support soft limits and dont look ass when disabled.
osu Game Rulesets Osu Tests_enzYnOKDsr
@bdach lmk if you think this is okay and I should rewrite the slider bars for this. I kinda want to rewrite slider bars anyways to add custom precision so I can make slider bars default to integer values on a float bindable.

This is probably out of scope for this PR, but it would be nice to have draggable "handles" on the playfield while editing the grid. Using x/y/scale/rotation/whatever is generally a bit of a pain when making adjustments to the grid. I'd only really wanna use those if I want precise values, but not when just adjusting the grid by "vibe"

That's addressed in #26313

The grid type buttons use a lot of vertical space right now since they're arranged vertically. I think having either a horizontal button group would or a dropdown select would work better here.

I agree less vertical space would be better. If I go with a horizontal button group then I'd also add a label that says "Grid type" so you know what it controls.

@bdach
Copy link
Collaborator

bdach commented Jun 14, 2024

lmk if you think this is okay and I should rewrite the slider bars for this

Well put it this way if you can do it locally to the editor then maybe although I think it's going to be janky. Definitely would not want to see any changes to OsuSliderBar or even framework.

Not especially convinced by the proposal on a UX level but it's at least better than this branch right now.

@OliBomby
Copy link
Contributor Author

OliBomby commented Jun 14, 2024

Definitely would not want to see any changes to OsuSliderBar or even framework.

I checked and the only way to implement the features I need in a non-jank way is to change framework or OsuSliderBar, so that kinda kills the idea.

@OliBomby
Copy link
Contributor Author

@bdach I went and implemented your UX suggestions anyways. I just want to move this PR forwards.

The disabled sliderbar still doesn't look quite right because of the alpha showing the overlap, but I don't know how to fix that.
osu Game Rulesets Osu Tests_fD8UiK5YXf

@bdach
Copy link
Collaborator

bdach commented Jun 20, 2024

UX seems fine to me now for whatever that's worth.

OliBomby and others added 2 commits June 20, 2024 17:27
Co-authored-by: Bartłomiej Dach <[email protected]>
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 ok

@peppy requesting double-check to make sure you're fine with this in the current form

@bdach bdach requested a review from peppy June 21, 2024 06:39
@bdach
Copy link
Collaborator

bdach commented Jul 3, 2024

Actually I'm just gonna get this in to get it off of my review list.

@bdach bdach removed the request for review from peppy July 3, 2024 08:56
@bdach bdach enabled auto-merge July 3, 2024 08:57
@bdach bdach disabled auto-merge July 3, 2024 09:40
@bdach bdach merged commit 6313631 into ppy:master Jul 3, 2024
8 of 17 checks passed
@OliBomby OliBomby deleted the grids-2 branch July 3, 2024 10:19
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.

3 participants