ppy
Change non-legacy control points to have CustomSampleBank 1
- Closes #29028
Passes test osu.Game.Tests.Beatmaps.Formats.LegacyBeatmapEncoderTest, which previous implementation #29084 failed (thanks @smoogipoo!)
See also #29280 regarding long-term implementation of custom samplesets - this PR implements the third option proposed.
Maybe @OliBomby or @peppy can provide feedback on whether this is the direction we want to go? From my reading of it this means any lazer-added sample bank will be CSS1 i.e. it will be overridden by whatever hitsample file exists in the beatmap.
I do agree with the direction of allowing sample banks to be overriden this way. There is no need for custum sample set indices because you can use custom bank names instead (though these cant be saved yet).
However this implementation seems to still be somewhat broken. When I add a custom sample, I still need to cut and repaste the objects for the custom sound to play, and when I reload the editor or go in play the sounds are gone again.
However this implementation seems to still be somewhat broken. When I add a custom sample, I still need to cut and repaste the objects for the custom sound to play, and when I reload the editor or go in play the sounds are gone again.
Are you testing on an existing beatmap? This change only affects creating new timing points, it will not change existing ones that are already saved as 0.
Are you testing on an existing beatmap? This change only affects creating new timing points, it will not change existing ones that are already saved as 0.
I'm testing on a completely new beatmap. It's still very much broken.
https://github.com/user-attachments/assets/ef2c3d25-83bb-46b2-bbd8-9e45e63bcc09
https://github.com/user-attachments/assets/c3ae2ac6-9705-45c9-93f6-c5e510e78a55
I have changed this logic in getLegacyControlPointProperties to create LegacyHitSampleInfo with customSampleBank 1 if the sample point isn't already legacy. Still passes LegacyBeatmapEncoderTest.
This appears to work at first, but after placing a hit object the timing point reverts back to 0:
Looking at debug, after the encoder is called as part of EditorChangeHandler, samplePoint is being passed with CustomSampleBank -1:
I was not able to determine why this is - I will continue investigating
Found it - the encoder goes through each HItObject to add a SampleControlPoint to it, which was defaulting to CustomSampleBank -1:
SampleControlPoint createSampleControlPointFor(double time, IList<HitSampleInfo> samples)
{
int volume = samples.Max(o => o.Volume);
int customIndex = samples.Any(o => o is ConvertHitObjectParser.LegacyHitSampleInfo)
? samples.OfType<ConvertHitObjectParser.LegacyHitSampleInfo>().Max(o => o.CustomSampleBank)
: -1;
return new LegacyBeatmapDecoder.LegacySampleControlPoint { Time = time, SampleVolume = volume, CustomSampleBank = customIndex };
}
After changing that fallback to 1, custom hit sounds appear to be persisting:
Before further review happens here I will link back https://github.com/ppy/osu/discussions/29280 here, and particularly https://github.com/ppy/osu/discussions/29280#discussioncomment-10506039 which seems to indicate that this implementation is much more opinionated about what should happen than it probably should be.
Would be good to set some expectations in stone first before we go in any direction what is this. I'm not sure I have a valid opinion to give on that, though.
As pointed out by @bdach and https://github.com/ppy/osu/discussions/29280#discussioncomment-10506039 there are some fundamental issues with this implementation that I failed to notice earlier, and I think we should go in a different direction.
Going to close this for now based on https://github.com/ppy/osu/pull/29311#issuecomment-2385563246 and the fact that it's only a few lines of change.
