Fix sliders being reversed incorrectly in the editor

[Pasi4K5]

Fixes #18042

• Depends on Fix SliderPath.GetSegmentEnds #24581

This updated Reverse() method now includes 2 new things:

1. Removing unnecessary PathControlPoints at the end of a SliderPath

Let's use this slider as an example:

Previously, the control points at the end of the slider, which don't visibly affect the the path, were reverse just like all the other control points, which made the reversed slider look like this:

With the updated method, the control points at the end will be removed and the slider will be reversed as expected.

2. Recalculating the last perfect curve section of a slider

To fix the issue pointed out in #18042, Reverse() now takes the slider path of the last perfect curve and recalculates the middle control point to make sure it's always positioned between the perfect curve control point and the tail of the slider, so the slider shown in #18042 is now being reversed correctly:

By the way, stable reverses perfect curve sliders in a similar way. It doesn't ever remove any control points though.

Please note that this doesn't fix Bezier and Catmull sliders from being reversed incorrectly.
For example, this slider

when reversed, will still look like this

But, with the way things are now, I'm not sure if it's even possible to fix this due to how Bezier sliders work, but to be honest I don't think it's worth worrying about it since this doesn't majorly affect most sliders and mappers are used to this because the same issue exist in stable as well.

[bdach]

Before I even go into reviewing I would at least appreciate test cases for this. Should be simple enough to write, just check that the start and end positions of the sliders in the cited cases swap places rather than doing whatever else they used to do until now.

That said:

Removing unnecessary PathControlPoints at the end of a SliderPath

This makes path reversal a destructive operation. I'm not sure how to feel about that.

Maybe if a reversal would truncate control points, it should prompt for confirmation or something. Not sure.

[Pasi4K5]

Before I even go into reviewing I would at least appreciate test cases for this. Should be simple enough to write, just check that the start and end positions of the sliders in the cited cases swap places rather than doing whatever else they used to do until now.

Will do soon.

That said:

Removing unnecessary PathControlPoints at the end of a SliderPath

This makes path reversal a destructive operation. I'm not sure how to feel about that.

Maybe if a reversal would truncate control points, it should prompt for confirmation or something. Not sure.

You can still use Ctrl+Z to restore the removed control points, so it's not really destructive, especially considering that with this change the shape of the slider stays the same when reversing it twice, which wasn't always the case previously.

[bdach]

You can still use Ctrl+Z to restore the removed control points, so it's not really destructive

That argument doesn't make much sense given that we mark e.g. slider-to-stream operation, or slider merge, as destructive, and you can Ctrl+Z those too (you can Ctrl+Z pretty much anything, really). This change makes it kind of in-between, since there are no objects being deleted, but some "data" is still lost (even if it has no value).

Maybe it's fine for it to be that way. Just a loose UX thought I guess, probably need more opinions on it. @OliBomby curious what you'd say about that.

[OliBomby]

I cant immediately come up with reasonable use cases where you'd want to keep the extra bit of length, so I guess in most cases it would be fine to corrupt the control points a bit to keep the slider path the same.
For the rare case where you'd want the current method of reverse it would be nice to have a workaround like being able to make the slider end fall exactly on the last control point so this new reverse method does the same as the old.

Also I think you should implement proper shortening of the bezier too. Its actually quite easy to do with Casteljau's algorithm. See the perfect curve to bezier converter.

[bdach]

I cant immediately come up with reasonable use cases where you'd want to keep the extra bit of length, so I guess in most cases it would be fine to corrupt the control points a bit to keep the slider path the same.

Let's just keep as is for now then (make it discard the control points that don't do anything). In fact it could be a nice kind of conveyance for mappers that these points were bogus and not doing anything at all.

Also I think you should implement proper shortening of the bezier too. Its actually quite easy to do with Casteljau's algorithm. See the perfect curve to bezier converter.

Oh interesting, if it's not prohibitively long LoC-wise then I'd like to see that done in this pull too.

[OliBomby]

Actually I take back what I said about the bezier. To find the right T value for a given length requires a more complicated algorithm so better to do that in a separate PR.

[Pasi4K5]

For the rare case where you'd want the current method of reverse it would be nice to have a workaround like being able to make the slider end fall exactly on the last control point so this new reverse method does the same as the old.

Actually, this is kind of possible already. Kind of. By changing the curve type of the last control point, the slider can be extended to the last point, ignoring the beat snap. You could then reverse the slider and resnap the sliderend. But that's a pretty hacky way to do it, and probably not intentional, so it might even get removed at some point.

Actually I take back what I said about the bezier. To find the right T value for a given length requires a more complicated algorithm so better to do that in a separate PR.

I can try looking into that to see if I can come up with a solution for this myself, but IMO it's questionable if it's even worth giving it any attention in the near future, because, again, it doesn't significantly affect most sliders and it's been like this for years.

[OliBomby]

I can try looking into that to see if I can come up with a solution for this myself, but IMO it's questionable if it's even worth giving it any attention in the near future, because, again, it doesn't significantly affect most sliders and it's been like this for years.

If you don't want to then I'll be happy to add the algorithm myself. I already have all the source code for the algorithm you need from the Mapping Tools repo.

[Pasi4K5]

If you don't want to then I'll be happy to add the algorithm myself. I already have all the source code for the algorithm you need from the Mapping Tools repo.

I'm definitely not against it, so sure, go ahead if you want to.

[Pasi4K5]

While writing the test, I noticed a little oversight where linear sliders with uninherited points at the end were not reversed properly, which is also fixed now.

Code should be ready for review.