Refactor to command pattern for hit object change handling

[OliBomby]

This is a more minimal version of https://github.com/ppy/osu/pull/30255 for improved usage code and minimal API.

Changes compared to previous version

• Removed proxies
• Removed "mergeable" commands
• Renamed commands to "changes" to avoid confusion with commands which should represent one undo step. Instead we have atomic changes.
• Removed commands abstracted over generic lists
• Removed IHasMutablePosition
• Renamed EditorCommandHandler to HitObjectChangeHandler. This name because it only handles the hit object changes at the moment. Its supposed to take the name of BeatmapEditorChangeHandler once everything is replaced.

The baseline interface is now IRevertableChange. It represents a single atomic reversable change that will be recorded by the change handler to be reverted on undo. Every single bit of editor code that modifies the beatmap state must do so through a IRevertableChange otherwise it will not undo.

public interface IRevertableChange
{
    void Apply();
    void Revert();
}

The HitObjectChangeHandler batches changes between BeginChange() and EndChange() into transactions and each transaction is one undo step.

Usage code

Please don't despair! Porting code to the new change handler is quite straight forward.

Old code:

[Resolved]
private IEditorChangeHandler? changeHandler { get; set; }

changeHandler?.BeginChange();

hitObject.Path.ExpectedDistance.Value = null;
p.ControlPoint.Type = type;

editorBeatmap.Update(hitObject);

changeHandler?.EndChange();

New code:

[Resolved]
private HitObjectChangeHandler? changeHandler { get; set; }

changeHandler?.BeginChange();

new ExpectedDistanceChange(hitObject.Path, null).Apply(changeHandler);
new PathControlPointTypeChange(p.ControlPoint, type).Apply(changeHandler);

editorBeatmap.Update(hitObject);

changeHandler?.EndChange();

Debugging

Since the history of each operation is stored as a bunch of atomic changes its possible to build debug tools (like the Ctrl+F1 in test browser) that show you the individual changes in the history which you can step through one by one to check if its correct. This would allow you to quickly determine where exactly it goes wrong.

Multiplayer potential

The atomic changes allow for easy conflict resolution in a shared editor scenario.

Also the changes get submitted immediately to the central handler so its possible to send fine-grained updates to peers. For example you could see someone drag around an object smoothly rather than it teleport to the end position.

Commands coverage

• All operations on hit objects

Missing coverage (to be added in a later PR)

• Timingpoints
• Breaks
• Metadata

[OliBomby]

I added CompositeChange which is a more complex change which is made up out of smaller changes. It keeps it possible to iterate all the atomic changes in history, but its also nice because there is some higher level grouping in the history which makes debugging easier.

They differ a bit from the atomic changes in that they create the undo history on the first Apply() instead of on constructor. This is convenient because you can depend on the changed state while determining which changes to make next, just like normal edit code that changes state as it goes. This is used for the ReverseSliderPathChange.

Lastly I renamed the Submit(changeHandler) extension method to Apply(changeHandler). I think this fits better because its like an Apply() but it also records it to the changeHandler.

[OliBomby]

I removed the QueueUpdateHitObject command in a7273df59b523f29e64ccccc89d16bd9329ff6a9 and explained some reasoning in the commit description. I'd like some opinion on this approach before I try any of the alternative solutions listed in https://github.com/ppy/osu/pull/30314#discussion_r1806790153 which would actually change when hit objects are being updated.

[peppy]

@OliBomby is there a reason this is in draft state, or can we mark it ready for review?

[OliBomby]

@OliBomby is there a reason this is in draft state, or can we mark it ready for review?

It still needs more work until it can be functional and merge:

• Implement commands for the remaining hit object operations and gamemodes
• Implement compatibility with the old change handler so we can transition in steps. The old change handler should still handle undo for timing points and metadata for now.

[OliBomby]

This PR is now ready. I systematically replaced every bit of hitobject edit code with the new usage cod. I hope I didn't miss anything.

Every undo/redo interaction should works as it did before. The only difference is that Editor.HasUnsavedChanges will be true if you add and remove a hitobject. This must be so because we can no longer hash the full beatmap state because we move away from the old change handler.

I removed parts of the old change handler that handle hit objects, so now it only serialized metadata, timing points, and breaks. It no longer detects any changes in hit objects and only triggers an update state if something changes in the metadata, timing points, or breaks. This allows it to work together seamlessly with the new change handler until we replace all parts with the new change handler.

[peppy]

@OliBomby I'm going to take on review for this, but before I get too deep:

• There looks to be a couple of tests still failing, can you check on them?
• You mention in the OP that the naming of this will change when it's fully implemented (ie. dropping the New prefix) – are we there yet? I'm guessing "no" because non-hitobject changes are still handled by the old handler? If this is the case, I think this needs to be described by the class name and with some extra xmldoc on the class itself.

[OliBomby]

• There looks to be a couple of tests still failing, can you check on them?

Yes ill fix these. I dont think it will need much changes.

• You mention in the OP that the naming of this will change when it's fully implemented (ie. dropping the New prefix) – are we there yet? I'm guessing "no" because non-hitobject changes are still handled by the old handler? If this is the case, I think this needs to be described by the class name and with some extra xmldoc on the class itself.

You're right, we can only drop this prefix once the BeatmapEditorChangeHandler doesn't exist anymore. Right now we still need it to handle non-hitobject changes. I'll make the future direction of this name more clearly documented.

[OliBomby]

I resolved the merge conflicts and made sure to check every change that happened in between so to refactor any new code (precision move popup) to the new command pattern.

Would like to see this reviewed soon or labeled as the "next" editor thing to be worked on, so we can prevent further merge conflicts.

[OliBomby]

Thank you for reviewing my PR, despite the horrible time it caused you. I do my best to make it the least horrible as possible.

First I looked at the root classes - Editor, EditorChangeHandler et al. - and saw those classes peppered with a lot of (hopefully temporary) frankenstein code that attempts to somehow negotiate both approaches in a weirdly conjoined way.

Yes, that code joining the old change handler is temporary. It will be a lot cleaner once we don't need the old change handler anymore.

and it is being partially migrated, right? In an 80-file 2.5k-line diffstat godzilla?

Yes, it's being partially migrated. There was just no way to make the part any smaller. Due to how the old change handler works you can't cleanly separate hitobject changes between the old and new change handler, so all hitobject changes had to be ported in one go.

Even the parts that don't have the old change handler intertwined have weird API design such that I don't even know how the API is supposed to be used. What is a "revertible change" compared to a "transaction" compared to a "composite change"?

I hope we can work together to simplify the API and dispel any confusion.

• "revertible change" is a single atomic change in the state that can be reverted, like a variable assignment.
• "composite change" is functionally a revertible change but it is not atomic, it consists of multiple atoms. This class could in theory be removed.
• "transaction" is a collection of revertible changes that represents a single undo step. This is directly tied to the transactional commit component flow.

At this point I'm not sure what's a productive avenue to continue this in. I can't in good conscience go through review of the rest of this because I don't like how this looks too much to be fine with merging it. The only ways forward that I can see is that I either basically tell you to cook and just don't touch editor at all until you're done and hope all of the badness here somehow disappears, or try to redo this myself from scratch and see if I can fare any better.

What do you mean by letting me cook until it's done? Do you mean porting all change handling in this single PR? I was planning on merging this, and then port the next part and the next, and the skin editor, until its done.

I don't recommend redoing this from scratch yourself, because it will drive you insane and probably not make it much better.

I think a lot of your pain points can be resolved so please trust me a bit longer.

[andy840119]

I was planning on merging this, and then port the next part and the next, and the skin editor, until its done.

Have an idea, maybe start from the skin editor first can cause less code change and easier to be reviewed?

[OliBomby]

Have an idea, maybe start from the skin editor first can cause less code change and easier to be reviewed?

I'd rather not do all this again from scratch. I don't think it will be significantly smaller either.