Modifying sliders in editor resets the hitsound type and volume

[bdach]

Discussed in https://github.com/ppy/osu/discussions/15615

^{Originally posted by Coppertine November 14, 2021} Self explanitory, happens on any standard beatmap in the editor. This happens when you move any anchor, press CTRL+G, using the Rotate buttons, using the Horizontal or vertical flips, or moving the slider on the playfield. My guess is that the slider is replaced by a default slider and does not respect the current hitsounds on said slider.

[bdach]

So I'm going to request a bit of a design discussion on this because this will be a problem going forward even after ditching legacy sample points. Warning, essay incoming.

When the editor beatmap initially loads via WorkingBeatmap/conversion/the normal paths, all objects, including nesteds, receive their sample points via the following "legacy" path:

https://github.com/ppy/osu/blob/dbe76b529b26bee32d279167d9f70244021b8b31/osu.Game/Rulesets/Objects/HitObject.cs#L128-L132

However, the editor then discards the legacy info to allow the new per-object logic to work:

https://github.com/ppy/osu/blob/dbe76b529b26bee32d279167d9f70244021b8b31/osu.Game/Screens/Edit/EditorBeatmap.cs#L75-L77

This is the root cause of this issue. Because nested hit objects are considered ephemeral (by the nature of their position/timing/everything else being tightly coupled to the parent), each parent ApplyDefaults() call - so any modification of the parent objects - will clear and recreate the nested objects. But at the point of the recreation, the legacy sample info is no longer there, and so in the nested objects ApplyDefaults takes the following code path wrt sample points:

https://github.com/ppy/osu/blob/dbe76b529b26bee32d279167d9f70244021b8b31/osu.Game/Rulesets/Objects/HitObject.cs#L133-L134

causing the old legacy information to get dropped, and causing each nested objects to get the default sample point instead.

This is actually a larger consideration going forward. Consider that we want to allow beatmap creators to customise samples per node/nested objects. But if nested objects are considered to be throwaway like that, what should happen e.g. if a mapper assigns samples to nodes, but then drags the slider to add an extra repeat? This is a similar scenario in that almost everything about the nested object is dependent on the parent, with the exception of sample control points, which are the property of the nested object itself. This didn't use to be a problem in the legacy way of doing things, because sample points were completely independent from objects.

To that end, I have two draft proposals of fixes to link:

• The first (https://github.com/ppy/osu/compare/master...bdach:sample-resetting-on-edit-ugly?expand=1) is basically an ad-hoc targeted condition that is supposed to address the bug. Namely, if a DHO's nested objects have just been cleared via default application, and there is no legacy info, then it is assumed that the nested objects should inherit the parent object's sample point.
• The second (https://github.com/ppy/osu/compare/master...bdach:sample-resetting-on-edit-nice-question-mark?expand=1) paves a possible way to address the concern above. A new TransferFromPreviousNestedHitObjects() virtual method is added, that allows the new nested objects to "salvage" the sample points from the old ones (via a heuristic based on object type and start time). This could potentially be used later to e.g. ensure that when a slider is lengthened, new repeat nodes will use the samples from previous nodes, or something like that.

[peppy]

I haven't looked at the branches yes, but from reading your investigation it seems like the larger issue here is that due to nested hitobjects being considered ephemeral, we cannot store any important information at that level. ie. all persistent data should always be at the top level object. Maybe it can also be argued that nested objects shouldn't be serialised if this is the case..?

Definitely needs some serious thought as it will change how we structure the data here - potentially more than just samples.

---

As for the presented solutions, the second seems to be what we want - at least temporarily - to ensure things work as we expect. The above discussion point can potentially be deferred until more cases come up and that solution becomes unwieldy.

[bdach]

I've been reluctant to PR any of the above solutions as I consider them hacky at best, but as an alternative - how would you feel if every slider had "node control points", e.g. a list of control points that apply for each repeat/span of the slider? Similarly to how NodeSamples works for actual sample info. That would be the long-term solution as it avoids storing important data in nested objects which as discussed above currently are deemed entirely disposable and not persisted with the rest of the object.

[peppy]

I think that approach sounds amicable.

[OliBomby]

I recently ran into this issue myself and I have some opinions about what should be done to fix this. I think the SampleControlPoint should be removed (almost) entirely.

1. They are almost entirely redundant. The HitSampleInfo already contains properties for the bank name and volume. If there are other properties unaccounted for, then those should also be included in HitSampleInfo. The only real use I can think of to have sample control points is to control the volume of slider slide sounds during the slider.
2. The way the SampleControlPoint are serialized seperately from the hitobjects with time being the only thing binding them causes all sorts of issues like #19483. Also the SampleControlPoint for a Slider and a SliderTailCircle have the same time so they end up overwriting eachother.

If all the info for playing a hitsound is contained in the list of HitSampleInfo that would be so much easier to work with. The Slider could still have a list of SampleControlPoint just to control the volume of the slider slider sample if thats wanted. The beatmap file format would have to be updated so it can actually store all the information of samples.

[peppy]

SampleControlPoint / DifficultyControlPoint were only recently moved into hit objects as a generic and potentially interim method of more easily setting these per-hitobject, and to provide some degree of compatibility with serialising back into legacy formats.

I think aiming to remove them eventually is not a bad direction.

[peppy]

This should be resolved via #23308!