Add toggles and hotkeys for configuring sample addition bank

[OliBomby]

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]

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:

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:

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]

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]

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]

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]

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).

[peppy]

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]

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

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).

[peppy]

As commented.

[OliBomby]

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]

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

[OliBomby]

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]

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

[OliBomby]

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]

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.

[bdach]

I can probably live with this

[peppy]

How do I make auto button work in this PR?

[OliBomby]

How do I make auto button work in this PR?

I haven't implemented that yet, because it needs more consideration than I expected, so it may be fit for a separate PR. I apologise for my lack of communication on this.

I tried allowing the auto bank on HitSampleInfo to inherit the bank from the normal sample, but this doesn't really make sense because it looks like the HitSampleInfo shouldn't depend on any other samples for its representation.

The alternative I want to try is adding an auto boolean property to HitSampleInfo that is purely for editor flow. Any such samples will get their bank set whenever the normal bank is set in the editor.

[peppy]

@OliBomby Please double-check my merge conflict resolution is correct at 1b42155.

[OliBomby]

merge conflict resolution was a bit hard to review with 400 files changed but it all seems good.

[peppy]

merge conflict resolution was a bit hard to review with 400 files changed but it all seems good.

For future reference, you can just look at the github link (1b42155) to see the relevant changes (ie. the parts i changed from the actual merge portion)

[bdach]

For future reference, you can just look at the github link (1b42155) to see the relevant changes (ie. the parts i changed from the actual merge portion)

That shows the full merge diff again. This works better. I have to explicitly put it in a markdown link rather than just paste it in because github's automagic "make this ugly link pretty" seems to mangle this particular scheme so that https://github.com/ppy/osu/pull/28863/commits/1b4215576d3cde9928a3c71b6101549cf2e68107 becomes https://github.com/ppy/osu/commit/1b4215576d3cde9928a3c71b6101549cf2e68107 which does not have the function you speak of.

Alternatively, git show 1b4215576d3cde9928a3c71b6101549cf2e68107 in terminal does the same thing. Or just use lazygit (shameless plug).

[peppy]

Oh that is silly.

[peppy]

Going to get this in to continue forward momentum.