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 momentary shortcuts to toggle grid/distance snap #20828

Merged
merged 16 commits into from
Oct 26, 2022

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Oct 19, 2022

Matching osu!stable. I use these quite a lot while mapping and I'm sure others do as well.

Hold Shift = invert grid snap
Hold Alt = invert distance snap

Matching osu!stable. I use these quite a lot while mapping and I'm sure
others do as well.

Hold `Shift` = invert grid snap
Hold `Alt` = invert distance snap
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 21, 2022
@peppy
Copy link
Sponsor Member Author

peppy commented Oct 21, 2022

I've fixed all the known edge cases I could find.

Note that to make this work well, I limited the momentary toggles so only one can be utilised at once. If not for this, a case like the following could occur, requiring a stack of application history (and even then, it could go wrong if the mouse is used to toggle via a button):

  • Distance snap is enabled
  • Momentary enable grid snap (forces distance snap off)
  • Momentary enable distance snap
  • Let go on opposite order of momentary presses
  • Distance snap is now off

I want to get this as close as possible without getting too complex, and I think what is on this branch should work for now.

@bdach
Copy link
Collaborator

bdach commented Oct 21, 2022

Found one more edge case that wasn't mentioned anywhere above, osu!-specific:

  1. Turn on permanent grid snap by clicking the ternary button w/ mouse
  2. Press and release Alt to momentarily toggle distance snap
  3. After releasing Alt, grid snap is now permanently off
2022-10-21.22-51-49.mp4

Works the other way round too, obviously.

Unsure if you're aware of this, so holding off from doing anything further with this pull yet. I'm not hugely fussed, but it feels a little bad that this happens and I don't think it's not fixable with a reasonable amount of effort.

@peppy
Copy link
Sponsor Member Author

peppy commented Oct 22, 2022

I guess that's the same case I pointed out but reproduced much simpler. It's unfortunate and I'd like to fix it... but as you say it's going to take considerable thought.

@peppy
Copy link
Sponsor Member Author

peppy commented Oct 25, 2022

Should work better now (with the dependent PR).

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.

Implementation looks good, one slight problem, however...

@bdach bdach enabled auto-merge October 26, 2022 19:30
@bdach bdach merged commit 5c4ff87 into ppy:master Oct 26, 2022
@peppy peppy deleted the grid-momentary-shortcuts branch October 27, 2022 01:48
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.

None yet

2 participants