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 toggles and hotkeys for configuring sample addition bank #28863

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

OliBomby
Copy link
Contributor

This PR adds a new left toolbox for configuring the sample addition bank of the selected hit objects.
The toggles for the sample bank now only affect the hitnormal sample of the selection.

The hotkeys are Alt-Q~R. This differs from stable's Ctrl-Q~R because those hotkeys are already taken by other tools in lazer.
Using Alt-Shift-Q~R sets both sample bank and sample addition bank simultaneously, like previous functionality.
The same hotkeys are also added to the sample edit popover on the timeline.

@bdach
Copy link
Collaborator

bdach commented Jul 16, 2024

We do need this but as usual mixed feelings about the execution.

Users will complain about the hotkey changing, not sure what to do about that. We probably need to do a hard pass over all hotkeys in editor and figure out a way to make them all match stable.

This feels like it's taking too much space:

1721118841

Also I dislike that there is no quick way now to assign the same bank to the normal sample and additions. Every exposed method now requires to input the same thing twice. I'm not a mapper but it feels like normal and addition banks being different is the 5% case not the 95% case.

Generally I'd maybe see the buttons redesigned entirely so that this can be worked with faster. Maybe they could be split down the middle or something:

1721119658

Idea is if you click the top part of the button you set both normal & additions; if you click either side you just do that side.

In addition, the addition bank toggle can be switched off but it will actually switch on again when the object is selected again (and deselecting it doesn't represent a valid operation anyway):

2024-07-16.10-49-43.mp4

@OliBomby
Copy link
Contributor Author

OliBomby commented Jul 16, 2024

Users will complain about the hotkey changing, not sure what to do about that. We probably need to do a hard pass over all hotkeys in editor and figure out a way to make them all match stable.

Not sure if users will complain. The current hotkey layout with Shift and Alt is more comfortable because you can have your pinky on the Shift key and the thumb on the Alt key simultaneously, allowing you to quickly change normal bank and addition bank without moving your hand.

Also I dislike that there is no quick way now to assign the same bank to the normal sample and additions. Every exposed method now requires to input the same thing twice.

Not true, I made it so holding Shift and Alt at the same time assigns the same bank to the normal sample and additions.

I'm not a mapper but it feels like normal and addition banks being different is the 5% case not the 95% case.

Normal and addition banks being different is a lot more common than you think. You can verify this by inspecting any random ranked map. Its common to set normal bank to soft while using various banks for the additions.

Generally I'd maybe see the buttons redesigned entirely so that this can be worked with faster.

I agree the buttons could be redesigned to be faster to work with and take up less space. However I think this should be addressed in a later PR because it would be a pretty big diff and the current implementation seems good enough for now. The main utility right now comes from the hotkeys.

In addition, the addition bank toggle can be switched off but it will actually switch on again when the object is selected again (and deselecting it doesn't represent a valid operation anyway):

Good catch, this must be fixed.

@bdach
Copy link
Collaborator

bdach commented Jul 16, 2024

Not true, I made it so holding Shift and Alt at the same time assigns the same bank to the normal sample and additions.

I said "Every exposed method", and I consider the hotkeys to not be an exposed method / be a power-user feature. Not to say that it's not possible, just inconvenient unless you know the hotkeys (and even then there are arguments that three-key chords are just not comfortable regardless of which modifier keys you use)

@OliBomby
Copy link
Contributor Author

I said "Every exposed method", and I consider the hotkeys to not be an exposed method / be a power-user feature. Not to say that it's not possible, just inconvenient unless you know the hotkeys (and even then there are arguments that three-key chords are just not comfortable regardless of which modifier keys you use)

Sorry for misinterpreting your definition of "exposed method".

Is it a blocking issue to not have a button on screen to assign the same bank to the normal and addition samples?
Considering:

  • The action can be done in 2 clicks instead of 1 click.
  • Mappers use differing addition and normal banks pretty often.
  • There is a hotkey to do this which is also hinted at on the button group itself.

@bdach
Copy link
Collaborator

bdach commented Jul 16, 2024

Dunno. Feels bad to me but is also a @peppy question (the entirety of this PR is, really, I don't see myself merging without getting second thoughts on the UX here).

@bdach bdach requested a review from peppy July 16, 2024 09:44
@peppy
Copy link
Sponsor Member

peppy commented Aug 8, 2024

I'm not a mapper but it feels like normal and addition banks being different is the 5% case not the 95% case.

Yes, this should be the default for mappers which are looking to make a beatmap without fuss (as it has always been). They shouldn't have to set the sample set twice.

Which is to say I agree with @bdach that this current implementation isn't what I'd want to see.

@peppy
Copy link
Sponsor Member

peppy commented Aug 8, 2024

Forgive my lack of effort in this mockup, but this is how I'd see it working:

Pixelmator Pro 2024-08-08 at 06 35 18

The first thing is a checkbox which exposes the advanced mode where a user will be able to assign different base/addition banks.

When the left bar is compacted, it would always apply to both at once (or have a compact split button as proposed above).

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

As commented.

@OliBomby
Copy link
Contributor Author

OliBomby commented Aug 8, 2024

Yes, this should be the default for mappers which are looking to make a beatmap without fuss (as it has always been). They shouldn't have to set the sample set twice.

I'm a bit confused about this statement because in stable it has always been separate buttons for normals and additions, unless if you've set the additions bank to auto.

Perhaps I should add back the auto bank functionality for additions so it matches stable? Typing "auto" in the additions bank will then always use the normals bank, and that will be the default value.

@peppy
Copy link
Sponsor Member

peppy commented Aug 8, 2024

Ignoring that, does the UI proposed by spaceman / myself not seem like a good direction though?

@OliBomby
Copy link
Contributor Author

OliBomby commented Aug 8, 2024

Ignoring that, does the UI proposed by spaceman / myself not seem like a good direction though?

I'm fine with having compact split buttons like suggested by spaceman so the whole thing takes up less vertical space.

However I don't think it's a good direction to overcomplicate the UI with additional buttons or 'advanced' modes just so you can assign normal bank and addition bank in one go. The assumption that 95% of the time mappers want the normal bank and addition bank to be equal is plain wrong. It's much more common that the hitsounder wants to control the normal bank and additions bank separately, so why hide that away behind an 'advanced' mode? In stable they have also been separate toggles all this time and I haven't heard complaints about it, so I think its best to follow stable's design as much as possible.

@peppy
Copy link
Sponsor Member

peppy commented Aug 8, 2024

All fine and good, but by default one there should be one button to adjust all banks at once. This is imperative.

@OliBomby
Copy link
Contributor Author

OliBomby commented Aug 8, 2024

All fine and good, but by default one there should be one button to adjust all banks at once. This is imperative.

I think I can achieve that with my previous suggestion:

Perhaps I should add back the auto bank functionality for additions so it matches stable? Typing "auto" in the additions bank will then always use the normals bank, and that will be the default value.

The bank for additions will be on "auto" by default when placing objects or when adding additions to objects without additions. The addition bank will then by "auto" by default which causes the normal bank to control both normals and additions. That satisfies the "by default one there should be one button to adjust all banks at once" requirement.

If you then manually override the addition bank to something other than "auto", it no longer has one button to adjust all banks at once. This would be like the 'advanced' mode you suggested.

I prefer this implementation because it doesnt require additions to the UI and it matches stable behaviour.

@peppy
Copy link
Sponsor Member

peppy commented Aug 9, 2024

As long as soemthing is done about the current UI on this PR taking up huge vertical space and also looking liek the same buttons are there twice. I'd still propose the dual-column setup.

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

4 participants