Refactor to command pattern for change handling

[OliBomby]

This is a MVP command pattern implementation made in collaboration with @minetoblend.

Commands approach

Commands are created for each individual property that can be modified, without code generators and minimal generics. They have hard references to the objects they modify so they can apply the changes without needing additional context or lookup. In the future we might change this in order to allow serialization.

Mergeable commands compress repeated changes within the same transaction. This results in ~1000x reduction of commands in the history for drag based events.

Commands coverage

• osu! hit object dragging
• Selection box
• Scale and rotate
• Slider path modifications

Missing coverage

• Hit object creation, deletion
• Other rulesets
• Timeline modifications
• Metadata

Proxies

To make it easier to use commands with minimal boilerplate we designed proxies. The proxy is a wrapper around an object that exposes custom setter methods which generate commands on use. The proxy setters are all defined in extension methods with generics that look at the wrapped type. This design allows for maximum flexibility so you can also use proxies for inexact types like T : OsuHitObject, IHasPath and all the expected methods will be there.

We went through a few designs for proxies before ending up at this design. Here's the requirements for proxies we came up with:

• Proxy must be defined for all types and hit object interfaces.
• Use of proxy must not incur additional allocations.
• Concise and simple interface so the proxy is effective at reducing boilerplate.

Migrating in parts

Migrating things across one by one to the new command system is quite problematic. We attempted to combine the old change handler with the new command system so the old change handler can handle undo on all that doesn't have command coverage yet. This does mean that the change handler is saving states even if you do a transaction with commands, because it needs to have the latest state saved in case you want to undo a change without command coverage next.

The problem is that the old change handler does not play nice with the command system, so it's hard to combine the two systems
When the old change handler modifies a hit object during undo, it creates a new object so all the hard links in the commands in the history no longer point to the correct object.
Making commands serializable wouldnt solve this either because the old change handler can not handle IDs because the legacy file format it uses does not support object IDs. Your IDs will be lost on undo and the links will be broken still.

Best we can do is break up the migration by object type:

• All hit objects in all rulesets. Can't be reasonably split because edit code is shared between rulesets and hitobjects of all types.
• Timing points
• Misc. metadata

[Susko3]

A customary look trough the code for theoretical correctness. I don't actually know how the editor works & I've not tested this code.

As I see it, doing [action, undo, redo] will likely break things on complex actions.

[Susko3]

This can't be a PropertyChangeCommand because the implementation of MergeWithPrevious() is incorrect.

Example:

IList<T> list;
T first;
T second;

var command1 = new Update(list, 1, first);
var command2 = new Update(list, 2, second);

var merged = command2.MergeWithPrevious(command1);

merged is command2, which is incorrect as it'll never do list[1] = first.

[Susko3]

Shouldn't this check for Beatmap as well?

[Susko3]

I'm not sure how editorBeatmap.Update(HitObject) works, but isn't this kinda wrong when undoed?

What the undo does:

editorBeatmap?.Update(HitObject);
HitObject.Path.ExpectedDistance.Value = previousDistance;
HitObject.SliderVelocityMultiplier = previousVelocity;

What it should actually do:

HitObject.Path.ExpectedDistance.Value = previousDistance;
HitObject.SliderVelocityMultiplier = previousVelocity;
editorBeatmap?.Update(HitObject);

[Susko3]

Suggested change

	new CommandProxy<PathControlPoint>(commandHandler, c).SetType(controlPoints[0].Type);
	new CommandProxy<PathControlPoint>(commandHandler, controlPoints[1]).SetType(controlPoints[0].Type);

[Susko3]

Weird how this class implements IHasMutablePosition, but these are only getters.

[Susko3]

Searching trough the whole command list for a merge feels wrong. It could lead to subtle ordering bugs.

Example:

https://github.com/ppy/osu/pull/30255/files#diff-ae62bacc01da64e9d3245713349cbda16a5dc13aae7f36e9879ae46917fab6d3L524-R540

This code does:

// Proxy.A() == initial
Proxy.SetA(first);
BeatmapProxy.Add(newSlider);
Proxy.SetA(second);

Equvalent to

HitObject.A = first;
Beatmap.Add(newSlider);
HitObject.A = second;

undoCommands would be build up as following:

1. [SetA(initial)]
2. [Remove(newSlider), SetA(initial)]
3. [Remove(newSlider), SetA(initial)]
   • SetA(first) merged into the previous command

Undo()ing this would put the following transaction on the redo stack:

• [SetA(second), Add(newSlider)]

Which is equivalent to

HitObject.A = second;
Beatmap.Add(newSlider);

Subtly different from the initial code. In a way that might cause weird breakages.

This is all theoretical and completely untested.

[Susko3]

Calling CreateUndo() like this is probably losing information. The xmldoc mentions that it's a delicate operation that takes the current state of the object into account.

I think it's easier to correctly implement undo and redo if IEditorCommand was structured slightly differently:

public interface IEditorCommand
{
    /// Stores necessary information for an undo operation
    /// and applies the command to the current state.
    /// This also functions as Redo()
    void Apply(); // alt-name: Do()
    
    /// Undoes the action done in Apply()/Do()
    void Undo();
}

This would also match the "command pattern" with undo in literature.

[Susko3]

Also, for redo, you're doing Transaction.Reverse().Reverse(). Why not store & use the original transaction?