Add ability to split sliders in osu! editor #19858
Conversation
| // Turn the control points which were split off into a new slider. | ||
| var samplePoint = (SampleControlPoint)HitObject.SampleControlPoint.DeepClone(); | ||
| samplePoint.Time = HitObject.StartTime; | ||
| var difficultyPoint = (DifficultyControlPoint)HitObject.DifficultyControlPoint.DeepClone(); | ||
| difficultyPoint.Time = HitObject.StartTime; | ||
|
|
||
| var newSlider = new Slider | ||
| { | ||
| StartTime = HitObject.StartTime, | ||
| Position = HitObject.Position + splitControlPoints[0].Position, | ||
| NewCombo = HitObject.NewCombo, | ||
| SampleControlPoint = samplePoint, | ||
| DifficultyControlPoint = difficultyPoint, | ||
| Samples = HitObject.Samples.Select(s => s.With()).ToList(), | ||
| RepeatCount = HitObject.RepeatCount, | ||
| NodeSamples = HitObject.NodeSamples.Select(n => (IList<HitSampleInfo>)n.Select(s => s.With()).ToList()).ToList(), | ||
| Path = new SliderPath(splitControlPoints.Select(o => new PathControlPoint(o.Position - splitControlPoints[0].Position, o == splitControlPoints[^1] ? null : o.Type)).ToArray()) | ||
| }; | ||
|
|
||
| // Increase the start time of the slider before adding the new slider so the new slider is immediately inserted at the correct index and internal state remains valid. | ||
| HitObject.StartTime += 1; |
There was a problem hiding this comment.
Of note, I haven't finished reviewing this because of this code. I'm very unsure of how correct this is (for instance, the ControlPoint creation here has slightly different Time specification).
If possible I'd like to see this moved into a method inside Slider itself or something else. It doesn't feel like this is a great place to being this, and may lead to diverging implementations of the same "clone" logic going forward.
There was a problem hiding this comment.
the ControlPoint creation here has slightly different Time specification
I'm not sure what you mean by this. I saw that the sample control point should be 1 ms after the end of the slider, but internal updating stuff already makes it so that the sample control point is updated to this correct time. I did some testing however and noticed that the sample set of the splitted sliders do not get retained after a save and reload, so maybe something is indeed wrong with this code, but I don't know why this happens, I don't know enough about how hitsounds work in lazer. I tried to copy as much as possible from the convert-to-stream code.
If possible I'd like to see this moved into a method inside Slider itself or something else. It doesn't feel like this is a great place to being this, and may lead to diverging implementations of the same "clone" logic going forward.
Do you think a Slider needs a clone method similar to With() in HitSampleInfo?
There was a problem hiding this comment.
I did some testing however and noticed that the sample set of the splitted sliders do not get retained after a save and reload, so maybe something is indeed wrong with this code, but I don't know why this happens
I now understand why this happens and it's just SampleControlPoint being broken, so I'm not gonna fix that in this PR.
There was a problem hiding this comment.
I saw that the sample control point should be 1 ms after the end of the slider, but internal updating stuff already makes it so that the sample control point is updated to this correct time
Then why are the control point times even set in the code above? It just seems misleading as written if those lines aren't even doing anything and the values assigned there aren't even final.
There was a problem hiding this comment.
ill remove those lines
|
As a bit of an aside, should this split operation be permitted for sliders that have repeats? I feel like it doesn't work intuitively at all with sliders that have repeats, since the way it works is that it splits the slider into parts like in the simple case, but each part has the same number of repeats that the original slider had and it's just... wonky to me? |
Slider splitting and merging are operations which really only concern themselves with the shape of the slider path. The number of repeats is not relevant for this operation and it's treated as just another parameter of the slider. |
| var newSlider = new Slider | ||
| { | ||
| StartTime = HitObject.StartTime, | ||
| Position = HitObject.Position + splitControlPoints[0].Position, |
There was a problem hiding this comment.
Related, but isn't splitControlPoints[0].Position always... zero? As I understand it, newSlider is the first part of the slider, not the second - so when would the first control point not be at (0,0)? It's confusing me as it looks like something you'd do when you wanted newSlider to become the second part of the slider rather than the first.
There was a problem hiding this comment.
Slider control points are removed from the start of HitObject to create newSlider, so at that point splitControlPoints[0].Position is not zero. There is nothing that really forces the first control point to be zero.
There was a problem hiding this comment.
But that would imply that the Position of newSlider is not the same as the position of the original slider? How can this be if newSlider is supposed to be the starting segment?
There was a problem hiding this comment.
My explanation was not completely right. splitControlPoints[0].Position is zero on the first split, but it can split in multiple points at the same time. On the second split, the HitObject is missing some of the control points, so splitControlPoints[0].Position is not zero, and the HitObject.Position has not changed, thus HitObject.Position + splitControlPoints[0].Position is the actual starting position of the new slider. It accounts for the HitObject.Position not updating in the previous splits.
…ain sample control point
|
@OliBomby I've made some minor changes just to help with my own understanding of how the process is working. Tests are still passing and I don't think I make any contentious changes so I'll go ahead with merge 👍 . |
I added a feature which lets you split sliders into multiple parts. It synergizes particularly well with the merging feature i added earlier, so for instance you can split a part of the slider, transform it however you want, and then merge it back to the whole. For now you can only split on control points which have a non-inherited type and are not the first or last control point of the slider. It is theoretically possible to make a splitting feature which can split on any part of the slider body, but that would be more involved and I'm not sure how to design the UX for that. For now this is a simple useful addition to the editor.
osu._yXwUicVEn0.mp4