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 keybind for cycling grid type #26303

Merged
merged 14 commits into from
Oct 16, 2024
Merged

Add keybind for cycling grid type #26303

merged 14 commits into from
Oct 16, 2024

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Jan 1, 2024

Requires:

Using the tool boxes on the right frequently is quite annoying, so I also added a keybind for cycling the grid type (H). I chose this key because its right next to the grid spacing cycle hotkey (G) and unused. It makes sense to group the grid property changing hotkeys physically close on the keyboard. An alternative hotkey could be like Shift+G or Shift+G, Ctrl+G is taken.

There's also a test for the grid placement tool in here which apparently got left behind when I was splitting up this PR into parts. I guess it can't hurt to have it so here it is.

2023-12-31.21-07-56.mp4

@bdach
Copy link
Collaborator

bdach commented Jan 1, 2024

1.3k line diffstat moves this even lower on review priority list than it already was.

Did everything here need to be in a single pull?

@OliBomby
Copy link
Contributor Author

OliBomby commented Jan 1, 2024

Right I forgot to split it up to make review more palatable.

bdach added a commit to bdach/osu that referenced this pull request Jun 20, 2024
Addresses ppy#19970.

While yes, ppy#26303 is also a thing,
in discussing with users I don't think that grids are going to be able
to deprecate this feature.

Logic transcribed verbatim from stable.
@OliBomby OliBomby changed the title Add customization to positional snap grids in the editor Add keybinds for 'Grid from points' and cycling grid type Sep 19, 2024
@bdach
Copy link
Collaborator

bdach commented Oct 11, 2024

What's with the history here?

Please force-push that away because whatever that diffstat is, it's not worth 50 commits. If you chain PRs correctly the commits from previous PRs in the chain should go away when merging master into PRs later down the chain; and here it appears they didn't. So I'd rather this was fixed up, even if by squashing, because I don't want to see all that stuff in blame.

@OliBomby OliBomby changed the title Add keybinds for 'Grid from points' and cycling grid type Add keybind for cycling grid type Oct 11, 2024
@peppy peppy self-requested a review October 11, 2024 11:55
@OliBomby
Copy link
Contributor Author

I'm struggling with git here to fix this history

@peppy
Copy link
Member

peppy commented Oct 11, 2024

I dunno what's going on with the new commits but this should be fixed by squashing down to a single commit.

@OliBomby
Copy link
Contributor Author

Ok its fixed

@peppy
Copy link
Member

peppy commented Oct 11, 2024

I guess cycling is okay and saves on used hotkeys, but I'd bet the first thing we get asked for next is per-grid hotkeys rather than a cycle. Any thoughts on this?

@OliBomby
Copy link
Contributor Author

idk per-grid hotkeys seems overkill for something with 3 options because you're only ever at most 2 keypresses away from the grid type you want with cycling

@peppy
Copy link
Member

peppy commented Oct 11, 2024

I've pushed some changes and fixes. @OliBomby please confirm you're okay with them.

Importantly, you have to make sure to add new bindings to the end of the enum, else it offsets game bindings game-wide and breaks everything (see b62d9f8).

AddUntilStep("rectangular grid visible", () => this.ChildrenOfType<RectangularPositionSnapGrid>().Any());
gridActive<RectangularPositionSnapGrid>(true);

nextGridTypeIs<TriangularPositionSnapGrid>();
Copy link
Member

Choose a reason for hiding this comment

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

this generic test logic is shoddy. a failure results in invalid operation exception on a linq call. tests should fail on the asserts, not random link.

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've added an assert to explicitly assert the specific grid type. Now it should fail on the assert if it has the wrong grid type.

Not sure why inspectcode is quiet about this?
@bdach bdach merged commit 4fa101d into ppy:master Oct 16, 2024
11 of 13 checks passed
@OliBomby OliBomby deleted the grids branch October 16, 2024 17: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