Save custom sound banks to beatmap

[kstefanowicz]

• Closes Custom sound banks don't save to beatmap #29312

PR-SaveCustomSoundBanks.webm

Saves custom sound banks to beatmap by writing a new section to .osu file "CustomSoundBanks", and writing any HitObjects with a custom sound bank as a new number > 3.

Issues

• Not compatible with Stable - trying to import a beatmap in Stable with a custom sound bank > 3 fails: Resolved with 785ef22
• Doesn't consistently work with sliders - the end of the video shows this, I'm not sure why it's inconsistent Resolved with a790127
• After loading, HitObjects with custom sample banks no longer fall back to Normal if a file for the sample doesn't exist
• May need to be refactored depending on Editor does not read properties ignored on BeatmapInfo correctly #20883

[bdach]

Saves custom sound banks to beatmap by writing a new section to .osu file "CustomSoundBanks"

Where is this idea even coming from? Why would it be a new ini section? What even?

This seems like a completely wrong direction to take.

[kstefanowicz]

To my knowledge, hitsounds and sound banks are only saved/loaded via the legacy encoder/decoder. If that is incorrect, then these definitely don't need to be in the .osu file.

[bdach]

I'm even more confused than before.

Is this adding new functionality that does not exist in stable?

If the answer to the previous question is "no," then why isn't this functionality encoded in the same way as stable, to the point of requiring new ini sections that make stable fail to parse the map?

[kstefanowicz]

Is this adding new functionality that does not exist in stable?

This is adding the functionality of saving custom sound banks (i.e. not "normal", "soft", or "drum") to the beatmap, as described in #29312. Lazer will already play hit sounds from a custom bank if the associated file exists, but when saving the map the existing encoder falls back to 0 when given a non-standard sound bank name.

make stable fail to parse the map?

In researching this today, I found that Stable only fails to parse the map if the section is added after [Colours]. Moving it to before lets Stable import without issues:

PR-29566-1.webm

I have pushed 785ef22 to resolve.

[bdach]

This is adding the functionality of saving custom sound banks (i.e. not "normal", "soft", or "drum") to the beatmap, as described in #29312. Lazer will already play hit sounds from a custom bank if the associated file exists, but when saving the map the existing encoder falls back to 0 when given a non-standard sound bank name.

You didn't answer the question directly - but it sure sounds like the answer is "yes this exists in stable".

In which case, I'll repeat my previous question, which you also evaded by focusing on the parsing failure: why is this PR inventing new .ini file sections for this that stable doesn't know of, or doesn't need? Why are you not making the stable way of encoding this information work?

[kstefanowicz]

Stable does not support custom sound banks - it will only look at file names starting with "normal-", "soft-", or "drum-" when handling custom hit sounds.

Lazer already supports custom sound banks for files starting with any name. This is demonstrated in the video attached to #29312:

custom-samplebanks-dont-save.webm

Custom hit sounds play the correct file in the editor preview, but not gameplay. They also do not persist after re-loading the beatmap.

This PR allows for them to persist by writing the custom sound banks to the .osu file through the legacy decoder. I did it that way because that's how existing sound banks are saved and loaded, so I did my best to follow that direction.

---

I see that this is failing several tests I didn't think to check for. I will investigate and attempt to resolve.

[cl8n]

(apologies if this is the wrong place to comment because I may be "too late" in the sense that this feature is already working but just not being saved properly until this PR)

I feel like using custom banks like this introduces a lot of new UX questions that don't have good answers. you allude to one here:

After loading, HitObjects with custom sample banks no longer fall back to Normal if a file for the sample doesn't exist

and further, what happens when a user purposely ignores beatmap hitsounds but expects their skin to fill in the samples? a skinner has no way to prepare in advance to handle arbitrary banks. if the solution here is to fall back to the "normal" bank for those lookups -- then isn't it functionally identical to using a "N:C2" sampleset, just with different filenames?

is there a previous discussion where this was brought up? if not I think this is important to get right before altering the file format to include it

[kstefanowicz]

is there a previous discussion where this was brought up?

Custom hitsounds hadn't been natively possible in Lazer until the "Edit External" function was added, so I don't think the topic has had much discussion. There is some in #29311 (which effectively makes all Lazer timing points create with "N:C1" sampleset) as well as #29280.

The functionality does overlap with Stable's custom sample index, in that both allow for more in-depth hitsounding than just replacing the stock 12 hit sounds. I believe that letting users set custom prefixes in file names makes more sense than the suffix indexing system, and allows for greater flexibility. Implementing the suffix indexes would also require adding them to Timing Points, which in my mind is going backwards. One of the best things about Lazer is that functionality is moved away from Timing Points (slider velocity, hitsounding).

Not having a fallback for missing files is a serious issue. I don't currently understand how the SkinnableSound code in general works, but I am hoping to investigate it and try to determine how to resolve. Abstractly, when loading the map the editor should check what sound files exist in the map directory, and if it attempts to write a hit sound that doesn't have a file it should instead write it as Normal.

and further, what happens when a user purposely ignores beatmap hitsounds but expects their skin to fill in the samples? a skinner has no way to prepare in advance to handle arbitrary banks.

This is an excellent point - there would need to be a check before entering gameplay, and it'd need to be changed on the fly. Or perhaps at the SkinnableSound level a fallback check could exist?

I personally do not see why adding to the .osu file is a problem. Maybe I'm just inexperienced and naive, but does it cause issues that I am not understanding?

[bdach]

none of this has been properly discussed. extending what is a legacy format we plan on replacing at some point with new hacks to support a feature nobody really seems to want while we still don't have stable parity with hitsounding means that i am not interested in this PR in the slightest.

if anything you could say the sample bank being a freeform textbox an overextension for now and just replace it with a dropdown.

[kstefanowicz]

extending what is a legacy format we plan on replacing at some point

I wasn't aware that the .osu format was being replaced. I see a comment in #12976 from peppy 7 years ago, but I don't see it as part of osu! client development focus or osu! development roadmap.

means i am not interested in this PR in the slightest

That's okay.

[bdach]

but I don't see it as part of osu! client development focus or osu! development roadmap.

it's not there because replacing the format is not an imminent concern when we have glaring usability issues with editor and still lack parity in areas like skinning or song select functionality

[cl8n]

I personally do not see why adding to the .osu file is a problem. Maybe I'm just inexperienced and naive, but does it cause issues that I am not understanding?

for me the issue is completing support for this new feature that has, by the looks of it, not been given proper consideration for whether it should exist in the current form at all. I also disagree with the logic in #29311 which relies on custom bank names being a suitable alternative to custom sample indexes. I think we should move all of this talk about sample indexes and custom banks back to #29280 (discussion) for now, I'll write something there soon