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 the ability to move a skin element by 10 pixels when holding shift #22511

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

flustix
Copy link

@flustix flustix commented Feb 3, 2023

Allows you to move a skin element by 10px when holding shift and using the arrow keys.

switch (e.Key)
{
case Key.Left:
moveSelection(new Vector2(-1, 0));
moveSelection(new Vector2(holdingShift ? -10 : -1, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having this ternary done in half a dozen places wouldn't it be better to have the shift handled in a method and then use that for each case

Copy link
Author

Choose a reason for hiding this comment

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

Would something like this work?

int delta = e.ShiftPressed ? 10 : 1;

switch (e.Key)
{
    case Key.Left:
        moveSelection(new Vector2(-delta, 0));
        return true;
    case Key.Right:
        moveSelection(new Vector2(delta, 0));
        return true;
    case Key.Up:
        moveSelection(new Vector2(0, -delta));
        return true;
    case Key.Down:
        moveSelection(new Vector2(0, delta));
        return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably put it after the switch but yeah that reads a lot better to me now.

@flustix flustix force-pushed the skin-element-10px-movement branch from 565458c to 7a51e29 Compare February 3, 2023 14:01
@flustix flustix force-pushed the skin-element-10px-movement branch from 7a51e29 to 3f9cb17 Compare February 3, 2023 14:03
@mk56-spn
Copy link
Contributor

mk56-spn commented Feb 3, 2023

Also, to my knowledge force pushing is generally frowned upon here unless it's necessary for whatever reason ( cleaning up a very ugly and long commit history for example, and even it's sometimes easier to just close the PR and make a new one superseding it), Just a heads up

@flustix
Copy link
Author

flustix commented Feb 3, 2023

I still dont know how to use git properly and I've been using it for over a year

@NotGumballer91
Copy link
Contributor

NotGumballer91 commented Feb 3, 2023

Next time, I think you should just update/commit changes to the branch instead. Don't force push unless it's very necessary.

@bdach
Copy link
Collaborator

bdach commented Feb 4, 2023

Code looks trivially fine but the thing I'm not sure about is whether we want to be adding this for the skin editor only and if yes, whether it should be activated using Shift. Using Shift is pretty common UX elsewhere for big nudges, but in editor it is overloaded a bit (momentary rectangular grid snap toggle) and so I'm not sure how the two go together. Especially so that the rectangular grid snap may be eventually coming to skin editor too.

@bdach bdach requested a review from peppy February 4, 2023 15:54
@NotGumballer91
Copy link
Contributor

NotGumballer91 commented Feb 4, 2023

Since the editor uses Shift to toggle grid snap, How about make this PR as the default (requiring no additional keys) and set the current tiny steps as Ctrl instead?

@bdach
Copy link
Collaborator

bdach commented Feb 4, 2023

@NotGumballer91 i'm not entirely sure what you're suggesting but editor uses normal arrow keys without modifiers for seeking the track.

@NotGumballer91
Copy link
Contributor

NotGumballer91 commented Feb 4, 2023

@bdach Essentially: (In Skin layout editor)
Arrow keys = move element by 10px
Ctrl + Arrow keys = move element by 1px

And reserve the Shift button for grid snap which you've mentioned.

@peppy
Copy link
Member

peppy commented Feb 6, 2023

Whatever we choose, it needs to match between the skin and beatmap editor. I almost wonder if we should change grid snap to be ctrl and use shift to standardise things. Then again, that might piss off the whole mapping community to the point they won't use lazer 🤷.

@bdach
Copy link
Collaborator

bdach commented Feb 6, 2023

This is a stretch, but maybe the thing to do here is to add the grid to the skin editor, and have shift-nudges snap to the grid lines in both editors? As in, say you have an object unsnapped to the grid - when you shift-nudge it, it changes its X or Y component of position to the nearest gridline in the direction of the nudge?

Not sure this PR thread is the proper place to continue this discussion.

@peppy
Copy link
Member

peppy commented Feb 7, 2023

That sounds optimal and also will match user expectations.

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