Fix multi-segment-type sliders getting mangled on legacy export #31734
Conversation
The logic in `LegacyBeatmapEncoder` that was supposed to handle the lazer-exclusive feature of supporting multiple slider segment types in a single slider was interfering rather badly with the Bezier converter. Generally it was a bit difficult to follow, too. The nice thing about `BezierConverter` is that it is *guaranteed* to only output Bezier control points. In light of this, the same double-up- -the-control-point logic that was supposed to make multiple slider segment types backwards-compatible with stable can be placed in the Bezier conversion logic, and be *much* more understandable, too.
bdach
added
ruleset/osu!
area:beatmap-parsing
|
I'm not a fan of doing the double-up-the-control-point logic in Removing the backwards compatibility logic from |
Genuine question, do you see any future use for this method other than the current one? I'm not opposed to moving the logic out as you propose, but it'll be a little annoying to write and I kinda wonder if it's worth the added code overhead. |
One niche use-case I see is being able to do affine transformations on the shape of perfect curve type sliders. For example converting to bezier and then scaling it on the X axis to turn the circle into an ellipse. Besides that I just think it will make the code less confusing, because all the backwards compatibility logic will be localized to |
Done for two reasons: - During review it was requested for the logic to be moved out of `BezierConverter` as `BezierConverter` was intended to produce "lazer style" sliders with per-control-point curve types, as a future usability / code layering concern. - It is also relevant for encode-decode stability. With how the logic was structured between the Bezier converter and the legacy beatmap encoder, the encoder would leave behind per-control-point Bezier curve specs that stable ignored, but subsequent encodes and decodes in lazer would end up multiplying the doubled-up control points ad nauseam. Instead, it is sufficient to only specify the curve type for the head control point as Bezier, not specify any further curve types later on, and instead just keep the double-up-control-point for new implicit segment logic which is enough to make stable cooperate (and also as close to outputting the slider exactly as stable would have produced it as we've ever been)
|
Turns out I needed to do another thing to fix decode-encode stability that got broken with this, so it works out. Logic moved out in b4f63da. |
| // however, the Bézier path as output by the converter has multiple segments. | ||
| // `LegacyBeatmapEncoder` will attempt to encode this by emitting per-control-point curve type specs which don't do anything for stable. | ||
| // instead, stable expects control points that start a segment to be present in the path twice in succession. | ||
| if (convertedPoint.Type == PathType.BEZIER) |
There was a problem hiding this comment.
This should not double the control point for i == 0. The first control point is always considered a new segment.
Previously, previously, previously.
While I don't see immediate breakage in light testing, I accept that this may turn out to be entirely incorrect from the start line, so please treat with as much skepticism as you see warranted.
The logic in
LegacyBeatmapEncoderthat was supposed to handle the lazer-exclusive feature of supporting multiple slider segment types in a single slider was interfering rather badly with the Bezier converter. Generally it was a bit difficult to follow, too.The nice thing about
BezierConverteris that it is guaranteed to only output Bezier control points. In light of this, the same double-up-the-control-point logic that was supposed to make multiple slider segment types backwards-compatible with stable can be placed in the Bezier conversion logic, and be much more understandable, too. So I did that instead of attempting to jam more exceptions into the existing encoder code.@OliBomby tagging in, as you may be interested in checking out this proposal, and maybe you can spot if I'm missing something in all this as well.
This change fixes some sliders which use mutliple curve types not getting correctly exported to the stable
.oszbeatmap package format.