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 support for the auto sample addition bank in the editor #29648

Merged
merged 14 commits into from
Oct 25, 2024

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Aug 29, 2024

Requires:

UX works identical to osu! stable for addition samples only.

The implementation adds a new boolean property EditorAutoBank to HitSampleInfo. This seems like the best solution because it doesn't ever set the actual sample bank to auto, so the HitSampleInfo always has the correct bank and it doesn't need to inherit the bank before the deciphering the hitsound to play. With this the flow for gameplay is unchanged.

An alternative implementation where you set the HitSampleInfo.Bank to auto would require you to figure out the actual bank before playing the sample. This seems like a messier solution.
chrome_fEJHPxya9O
(idk how to embed the diff in a fancy way)

@peppy peppy self-requested a review October 1, 2024 07:24
@peppy
Copy link
Member

peppy commented Oct 1, 2024

Forcefully rebasing this on #28863 to avoid resolving conflicts twice. Honestly this could probably have been part of that PR since I have to review using this to test the full flow regardless 😅 .

@peppy
Copy link
Member

peppy commented Oct 1, 2024

Doesn't seem to work

osu.2024-10-01.at.07.54.31.mp4

@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 1, 2024

Doesn't seem to work

osu.2024-10-01.at.07.54.31.mp4

This is working as intended. You can't set Auto normal bank on selection because there is no fallback value, and you cant set addition bank on an object with no additions. You must add an addition first.

Maybe it should disable the options to better indicate this.

@peppy
Copy link
Member

peppy commented Oct 1, 2024

Yeah I dunno, it doesn't compute for me. Even with what you just said. Probably needs tooltips or better wording or UX.

@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 1, 2024

I've added a tooltip that explains why the addition banks are not usable when there are no addition samples in the selection.

Also I made it reset to auto sample bank whenever you clear the selection. This matches stable behaviour and prevents you from accidentally using a sample bank lingering from the last object you've selected.

Here's how it all works:

osu.Game.Rulesets.Osu.Tests_RPSpP15kLs.mp4

@peppy
Copy link
Member

peppy commented Oct 23, 2024

My remaining apprehension is that "auto" mode for non-additions has a meaning only during placement, and that feels a bit weird. Could that button do anything during selection, or if not, maybe it should show a tooltip that it's only valid during placement?

Copy link
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.

Code generally looks fine. One remaining concern as commented.

@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 23, 2024

My remaining apprehension is that "auto" mode for non-additions has a meaning only during placement, and that feels a bit weird. Could that button do anything during selection, or if not, maybe it should show a tooltip that it's only valid during placement?

There's no reasonable thing to inherit the bank from for non-additions that I can think of. There's no beatmap-wide sample bank default, and inheriting from the previous object (like during placement) wouldn't make sense at all after placement. So I'll just show a tooltip explaining how it works.

@peppy peppy self-requested a review October 24, 2024 09:21
@peppy
Copy link
Member

peppy commented Oct 24, 2024

Sure that works. I've also added a disabled state to better indicate that buttons are in a non-usable state:

osu.2024-10-24.at.09.37.12.mp4

peppy
peppy previously approved these changes Oct 24, 2024
@peppy peppy enabled auto-merge October 24, 2024 09:40
@peppy peppy self-requested a review October 24, 2024 10:52
@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 24, 2024

I don't see the disable state working, am I missing something? In the code I also don't see any code connecting the TernaryButton.Enabled to the enabled state of the drawable.

NVIDIA_Share_TDV4DH7wyz.mp4

@OliBomby OliBomby disabled auto-merge October 24, 2024 11:08
@peppy
Copy link
Member

peppy commented Oct 25, 2024

I don't see the disable state working, am I missing something

Seems like some of my changes didn't get committed properly. I had exactly what you added in 5b92a9f so all good 👍 .

@peppy peppy merged commit 47aa2c2 into ppy:master Oct 25, 2024
11 of 13 checks passed
@OliBomby OliBomby deleted the auto-addition2 branch October 25, 2024 09:40
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