Support undo/redo for control points in the Editor

[smoogipoo]

Works towards resolving #10314. Now handles control points.

... In the interim until we have binary object comparisons.

[smoogipoo]

It doesn't seem to be perfect in all cases:

• Changing scroll speed doesn't revert in osu! mode. It does revert in e.g. mania, where it actually has an effect.
• Changing slider velocity in the timeline doesn't revert.

These are issues elsewhere and relate to the way DifficultyControlPoint are extracted. It's a little bit of a mess, and this is a best effort.
More work will be required to actually apply new DifficultyControlPoints to hitobjects.

[peppy]

Probably fine / much better than nothing at all. Not sure the scroll speed setting should even be visible there.

[smoogipoo]

Sorry for the force push, I changed no earlier code, only removed a new commit that tried to fix DifficultyControlPoints not updating on HitObjects, then realised it caused damage on the actual timing screen :( Going to require more thought as previously said.

[bdach]

Are the assertion failures from the earlier runs here (example 1, example 2) related to the removed commit? As in, is this in a reviewable state right now?

[smoogipoo]

Nah, I'm currently fixing the assertion failures.

[smoogipoo]

Should be reviewable now.

[bdach]

Also not sure if related, but there's a weird bug with the bottom timeline display where control points are supposed to be shown:

2022-06-13.10-17-26.mp4

Appears that consecutive undoes hide and show the control point indicators... for some reason? Not sure if related, just bringing it up as something I noticed.

[smoogipoo]

The timeline issue could be related to incrementally adding every control point rather than giving a list with every control point populated at once. But probably a separate issue from this branch...?

[bdach]

Looks ok to me

[peppy]

This doesn't seem to handle the case where a new type is added to an existing group, is that known / expected?

And after doing this then adding a new control point... things get really weird...

osu.Game.Tests.2022-06-14.at.10.03.38.mp4

[smoogipoo]

Probably because the type isn't doing anything? It's only undoing/redoing based on what actually gets written to the file, so those points are probably becoming redundant & removed during write?

[peppy]

Hmm... that's... very awkward. Especially since it doesn't clear redo states. So you hit redo and suddenly you are in a completely broken looking state.

At very least I think the redo history needs to be purged in these cases.

[peppy]

Something has regressed with selected rows on the timing screen. After cloning a point, the newly cloned point is not selected visually (but seems to have the selection internally). I've checked that this works correctly on master

osu.Game.Tests.2022-06-14.at.10.08.08.mp4

• Select point
• Move to new time
• Clone

Observed row not selected.

[smoogipoo]

Admittedly I didn't thoroughly test redoing so I didn't notice it breaking it, though your video doesn't show it.

[peppy]

I had a video which has me redoing, but I didn't know what was going on and didn't think it would transfer well to a video 😅

Dismissing for now

[smoogipoo]

Please try this PR again and see if you can break it still. I think I might've resolved both the above issues by not replacing all control points.

There's still some weirdness (which I can't handle), that's best described with a video:

2022-06-20.15-35-15.mp4

In the video, I'm doing much the same actions as #18668 (comment).

1. I add an effect point to the first timing point.
   • No undo/redo state created (redundant point).
2. I duplicate an effect point later on.
   • No undo/redo state created (redundant point).
3. I change the duplicated effect point to adjust kiai time.
   • Creates an undo state.
4. I press undo.
   • Both the duplicated effect point and the effect point added in (1) are removed.
5. I press redo.
   • The effect point added in (1) does not return.
   • The duplicated effect point is merged into the one following it (which also starts kiai), because the one following it has been deemed redundant.

[peppy]

I can still get in a state where the row isn't selected. I'm not too concerned about certain changes remaining unsupported (like the scroll speed one in the video) but having the row non-selectable is quite breaking. Haven't looked at the code changes yet, but maybe a fix can be applied more locally to the table control rather than adding the overhead with equality etc?

osu.Game.Tests.2022-06-20.at.07.12.34.mp4

[smoogipoo]

Looks like the breaking is reproed by:

1. Select timing point.
2. Adjust BPM.
3. Select any other point.
4. Undo.

fix can be applied more locally to the table control rather than adding the overhead with equality etc

That's exactly what I was doing before, but it's even worse that way (breakage-wise) because performance starts to become a factor and you need to add layers of workarounds to resolve it. As it is, the current (/new) code is basically what we'd have to do with per-object diffing anyway.

I prefer equality, but let's see if the above issue changes my view on things.

[smoogipoo]

Above issue should be fixed (via ca287d0). Though I understand there's quite a lot of files changed here, I still believe that control point equality is the way to go.

I can split the changes out if it makes it easier to review - these changes are MVP and I'd also like to annotate ControlPointInfo with #nullable but that's a bit more involved, so I could bundle this in a separate PR.

[bdach]

I've checked this branch out again today, and I feel that this branch is as good a state as it can probably be. Which is to say, there is still breakage, but it's breakage that is definitely out of scope of this pull. See the following example:

2022-06-21.22-18-07.mp4

In the above video I manage to duplicate an "omit bar line" effect point by doing undo and redo. The reason why I am able to do this boils down to the abuse we have inflicted on LegacyBeatmap{Decoder,Encoder} to support per-hitobject control points. During the process of encoding, the per-hitobject points get converted back to global points, and the effect spec is applied onto legacy control points that are extracted from hitobjects. This breaks on decode, because OmitFirstBarLine = true effect points are never redundant, so they never get pruned, hence the magic cloning.

I don't see us fully fixing this without getting rid of .osu as the primary storage format for beatmaps. Which is mostly why I'm saying that this branch is in as good a state as probably can be at this stage.

[peppy]

Seems pretty good