Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to command pattern for change handling V2 #30314

Draft
wants to merge 71 commits into
base: master
Choose a base branch
from

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Oct 16, 2024

This is a more minimal version of #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 NewBeatmapEditorChangeHandler. This name kinda sucks rn but 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 NewBeatmapEditorChangeHandler 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;

changeHandler?.EndChange();

New code:

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

changeHandler?.BeginChange();

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

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

  • Hit object dragging
  • Selection box
  • Scale and rotate
  • Slider path modifications
  • Split and merge

Missing coverage

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

minetoblend and others added 30 commits October 8, 2024 20:18
Implement variant type generic proxies without heap allocations
…ature/command-handler

# Conflicts:
#	osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs
#	osu.Game.Rulesets.Osu/Edit/Commands/OsuHitObjectCommandProxy.cs
#	osu.Game/Screens/Edit/Commands/SetPositionCommand.cs
@@ -120,7 +124,7 @@ protected override void DragOperationCompleted()

// handle positional change etc.
foreach (var blueprint in SelectionBlueprints)
Beatmap.Update(blueprint.Item);
changeHandler?.SafeSubmit(new QueueUpdateHitObject(Beatmap, blueprint.Item));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using ?.
So .SafeSubmit() is not called if the changeHandler is null, right? Maybe having the reverse extension method is less prone to footguns: new BlahChange(Target).SafeSubmit(changeHandler).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is an error. It's not supposed to have the ?
The reverse extension method would be less error prone yeah.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version much easier to understand for grug. Grug no want pull out club with this one.

Comment on lines +305 to +306
newChangeHandler = new NewBeatmapEditorChangeHandler(editorBeatmap);
dependencies.CacheAs(newChangeHandler);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work? Does this just completely break undo on rulesets that haven't fully migrated to commands?

Not something we can do in a really real mergeable version of this, if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lined out a bit on how to migrate this in parts in #30255 and that's unchanged for this PR as well.

I can implement something that allows the undo history of the old and new change handlers to be interleaved, so we can have some parts undo with the new system and some parts with the old system.

However we'll have to migrate all the hitobject stuff for all rulesets at once because its not possible to undo a hitobject with the old system and then with the new system. The old system replaces the hitobject with a new object on undo so references break.

Comment on lines 22 to 30
public static class IRevertibleChangeExtension
{
public static void Submit(this IRevertibleChange change, NewBeatmapEditorChangeHandler? changeHandler, bool commitImmediately = false)
{
if (changeHandler != null)
changeHandler.Submit(change, commitImmediately);
else
change.Apply();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit weird to me in general. Why do changes submit themselves to the handler like this? Reads backwards. changeHandler.Submit(command) reads much more natural to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I eventually went with this because of what's pointed out in #30314 (comment) its the least likely to shoot yourself in the foot.

We have a weird situation because commands/changes have to Apply to do anything or Submit to a change handler which in turn calls Apply.
The change handlers are often nullable so we want to Submit if it's not null and Apply if it's null.

I previously had an extension method like below that handles this logic and follows the reading direction you say feels more natural.

        public static void SafeSubmit(this NewBeatmapEditorChangeHandler? manager, IRevertableChange command, bool commitImmediately = false)
        {
            if (manager != null)
                manager.Submit(command, commitImmediately);
            else
                command.Apply();
        }

The issue is that you can accidentally add in a ?. operator to the nullable change handler which would cause it to not run anything if the change handler is null.

// Wrong, does not apply the command when null
changeHandler?.SafeSubmit(...);

I also personally prefer writing code with the new Command(...).Submit(changeHandler) syntax because you can focus on the important bits first which is defining the command and then write the boilerplate, and its less nesting of parentheses.

/// </summary>
/// <typeparam name="TTarget">Type of the object owning the property</typeparam>
/// <typeparam name="TValue">Type of the property to update</typeparam>
public abstract class PropertyChange<TTarget, TValue> : IRevertibleChange where TTarget : class
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have a fair bit of skepticism towards this thing still. Generic usage will always have me stop and think "is it really necessary though?"

I recall you saying something that this "helps alleviate conflicts" when talking about collaborative editing or something. Can you elaborate on this? Because to my naive mind, I'm not convinced granularity at property level is required. I'd say the 'unit of operation' in the editor is a single hitobject, timing point, etc.

The point I'm trying to make is that it seems this is trying to make flows like "user A moves slider, user B changes its path" work without incurring conflicts - and I guess what I'm trying to say to that is I'm not sure that sort of thing would make sense? Those two edits are something that should conflict to me.

Copy link
Contributor

@minetoblend minetoblend Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two edits being conflicting makes sense, however it's more about how you deal with the conflict. That particular case, while probably being a bit jank if it happens, shouldn't really break anything. Dealing with conflicts on a per-property level allows just doing a simple last-write-wins appraoch for everything. I think the moment you introduce custom handling for every edge case like this complexity is just gonna skyrocket.

And what would the conflict resolution for this case even look like? Whoever touched the slider last will have their drag operation canceled? Everyone sees their local changes until they're done dragging and then it snaps to whichever one finished last? Will all changes to the same object be considered a conflict? What if someone selects all objects & i.e. modifies their hitsound, will that create conflicts with someone moving an object at that moment?
Those updates can of course be done on a per-object level too with i.e. a UpdateHitObjectCommand like I'm doing in osucad but they should still be able to pick which properties should be updated & I wanted to avoid that to have stuff like this (these are some modified osucad code snippets)

// its fine to do stuff like this in js since it's all dynamic anyways but not so sure about c#
function applyPatch(circle: HitCircle, patch: Partial<SerializedHitCircle>) {
  for (const key in patch) {
    let value = patch[key] as any
    if (key in circlePropertyDeserializers)
      value = circlePropertyDeserializers[key](value)
    
    circle[key] = value
  }
}

or this

function applyPatch(circle: HitCircle, patch: Partial<SerializedHitCircle>) {
  if (patch.startTime !== undefined)
    circle.startTime = patch.startTime;
  if (patch.position !== undefined)
    circle.position = Vec2.from(patch.position);
  if (patch.comboOffset !== undefined)
    circle.comboOffset = patch.comboOffset;
  if (patch.newCombo !== undefined)
    circle.newCombo = patch.newCombo;
  if (patch.hitSound !== undefined)
    circle.hitSound = deserializeHitSound(patch.hitSound);
}

/// Queues the update of a <see cref="HitObject"/> in an <see cref="EditorBeatmap"/> for undo/redo.
/// The order of the updates in the transaction does not matter, because the updates are aggregated and applied on the next frame.
/// </summary>
public class QueueUpdateHitObject : IRevertibleChange
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated previously, HitObject.Update() doesn't really make sense as a command / "revertible change" or however you put it. It is always supposed to be part of some larger operation.

Copy link
Contributor Author

@OliBomby OliBomby Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it doesnt truly fit in the category of "revertible change". It was put here as sort of a hack to make sure the correct hit objects get updated on undo. I need some way to know which objects to update on undo/redo.

I think a better approach would be to store the set of hit objects to update in Transaction directly adjacent to the list of changes, and then either providing these on the BeginChange() call or capture them from whenever EditorBeatmap.Update() is called.

The alternative solution is to make it so you never have to call EditorBeatmap.Update at all. I think that sort of change is better left for later though. I have some ideas on how it could be done but they all have downsides:

  • Have the change Apply() call the update. Would require all changes to have the EditorBeatmap and the relevant hit object. Probably calls the update way more than necessary which adds a bit of overhead.
  • Have EditorBeatmap listen for submitted changes that target hit objects. Would need some interface to get the Target object from the change.
  • Make everything in HitObject bindable with HitObjectProperty. Might add a lot of overhead.

}

/// <summary>
/// Reverse the direction of this path.
/// </summary>
/// <param name="sliderPath">The <see cref="SliderPath"/>.</param>
/// <param name="positionalOffset">The positional offset of the resulting path. It should be added to the start position of this path.</param>
public static void Reverse(this SliderPath sliderPath, out Vector2 positionalOffset)
/// <param name="changeHandler">Change handler to submit changes to.</param>
public static void Reverse(this SliderPath sliderPath, out Vector2 positionalOffset, NewBeatmapEditorChangeHandler? changeHandler = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line with what I said previously, the feeling I have here is that this helper method should really eventually just become a command.

Same thing goes for every user operation that ends up submitting 2+ commands.

Copy link
Contributor Author

@OliBomby OliBomby Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll turn these into some kind of commands.

This one was a bit weird to convert the out parameter, but I figured it out with the power of OOP

Comment on lines 108 to 124
/// <summary>
/// Removes a range of <see cref="PathControlPoint"/>s from the provided <see cref="BindableList{T}"/>.
/// </summary>
public static void SubmitRemoveRange(this BindableList<PathControlPoint> controlPoints, int startIndex, int count, NewBeatmapEditorChangeHandler? changeHandler)
{
for (int i = 0; i < count; i++)
new RemovePathControlPointChange(controlPoints, startIndex).Submit(changeHandler);
}

/// <summary>
/// Adds a range of <see cref="PathControlPoint"/>s to the provided <see cref="BindableList{T}"/>.
/// </summary>
public static void SubmitAddRange(this BindableList<PathControlPoint> controlPoints, IEnumerable<PathControlPoint> points, NewBeatmapEditorChangeHandler? changeHandler)
{
foreach (var point in points)
new InsertPathControlPointChange(controlPoints, controlPoints.Count, point).Submit(changeHandler);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't like these at all, they give me a faint waft of the proxy stuff. Just submit the commands directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see how it resembles the proxy stuff. I added these because it was easy to make mistakes in the implementation of these routines. I think I'll turn these methods into commands instead.

I think this fits better because it makes clear that this applies the change
@OliBomby
Copy link
Contributor Author

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.

Copy link
Contributor

@andy840119 andy840119 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better
Excited to see lazer apply the change ❤️

{
public class RemoveHitObjectChange : IRevertableChange
{
public EditorBeatmap Beatmap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:
Can think about should be better to provide the EditorBeatmap in the Apply() or Revert() instead?
Because:

  1. All cases of changes only write the value to the Beatmap.
  2. The change executer(NewBeatmapEditorChangeHandler) already has the instance of EditorBeatmap, and it should be possible to make any changes only expose the NewBeatmapEditorChangeHandler.

Also, should it be better to check if the change hit-object or property in the EditorBeatmap before change? I guess this PoC might works without check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can think about should be better to provide the EditorBeatmap in the Apply() or Revert() instead?

Not every change requires the EditorBeatmap so I dont see the reason to put it in the Apply() or Revert() method.

Also, should it be better to check if the change hit-object or property in the EditorBeatmap before change? I guess this PoC might works without check.

What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

// hit-circle is not in the beatmap.
var circle = new HitCircle();

// should throw the exception if object is not in the editor beatmap while apply the change?
changeHandler.Apply(new StartTimeChange(circle , circle .StartTime + offset))

Copy link
Contributor Author

@OliBomby OliBomby Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think such a check is unnecessary overhead. The changes have to be functionally the same as normal assignments.

Maybe such a check becomes necessary when serialising changes.

Comment on lines 95 to 96
changeHandler.SafeSubmit(new StartTimeChange(obj, obj.StartTime + offset));
changeHandler.SafeSubmit(new QueueUpdateHitObject(Beatmap, obj));
Copy link
Contributor

@andy840119 andy840119 Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q:
QueueUpdateHitObject command will be called before StartTimeChange while undo?
Guess expected to update the hit-object after undo the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EditorBeatmap.Update schedules the update for the next frame so it always runs last.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK
seems reasonable 👌🏼

Comment on lines +24 to +26
public void Apply() => Beatmap?.Update(HitObject);

public void Revert() => Beatmap?.Update(HitObject);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q:
why beatmap can be nullable in here, but not nullable in AddHitObjectChange?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happened to be nullable where Update was called.
I will probably remove this class entirely later.

Copy link
Contributor

@andy840119 andy840119 Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if this command is used, than developer might expected the triggers will call the update.
If EditorBeatmap might be nullable, maybe should be better to check outside?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right about that

{
var controlPoints = sliderPath.ControlPoints;

var inheritedLinearPoints = controlPoints.Where(p => sliderPath.PointsInSegment(p)[0].Type == PathType.LINEAR && p.Type == null).ToList();

// Inherited points after a linear point, as well as the first control point if it inherited,
// should be treated as linear points, so their types are temporarily changed to linear.
inheritedLinearPoints.ForEach(p => p.Type = PathType.LINEAR);
inheritedLinearPoints.ForEach(p => new PathControlPointTypeChange(p, PathType.LINEAR).Apply(changeHandler));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q:
Should create a class inherit CompositeChange for list of changes?
I noticed that CompositeChange can do list of changes inside a change but have no idea when to use this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to remove the old SliderPathExtensions.

Every bit of edit code that you want to put into history should be an IRevertibleChange. If your change is not atomic then you use CompositeChange.
We could technically also make CompositeChange for other methods like splitControlPoints.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little bit hard to distinguish them.
e.g.
why add list of control points use the AddRangePathControlPointChange
but update list of control point use the ForEach

Also, guess should be OK to remove the CompositeChange if InsertPathControlPointChange can accept add list of control points?

e.g.

public class InsertPathControlPointChange : IRevertibleChange
{
    public readonly IList<PathControlPoint> Target;

    public readonly int InsertionIndex;

    public readonly IList<PathControlPoint> Items;

    public InsertPathControlPointChange(IList<PathControlPoint> target, int insertionIndex, PathControlPoint item)
      : this(target, insertionIndex. new List<PathControlPoint> item)
    {
    }

    public InsertPathControlPointChange(IList<PathControlPoint> target, int insertionIndex, IList<PathControlPoint> items)
    {
        Target = target;
        InsertionIndex = insertionIndex;
        Items = items;
    }

    public void Apply() => Target.Insert(InsertionIndex, Items);

    public void Revert() => Target.RemoveAt(InsertionIndex, items.count);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddRange and RemoveRange are standard methods on any IList. It makes sense to have commands for these.

ForEach is a Linq extension method that takes a lambda for what to do. It's not very standard API and lambdas can't be serialized, so it doesn't make sense to make it a command.

I guess AddRange and RemoveRange could also be primitive changes (not CompositeChange) but for now I've left the set of primitive changes minimal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i see.

Another question:

foreach (var h in hitObjects)
  new PositionChange(h, h.Position + localDelta).Apply(changeHandler);

seems can be:

new RelativePositionChange(hitObjects, localDelta).Apply(changeHandler);

Are you recommend to give the change a relative value(e.g. x += 3), or only accept the absolute value(x = 3).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer absolute changes over relative ones because they are more robust to changes in the history. If you have multiple move changes in history and remove one in the middle, then it wont matter for absolute changes, but with relative changes the result changes.

I'd need a good reason to add this beyond just saving one line of code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with preferring absolute change commands over relative ones.


public static class RevertibleChangeExtension
{
public static void Apply(this IRevertibleChange change, NewBeatmapEditorChangeHandler? changeHandler, bool commitImmediately = false)
Copy link
Contributor

@andy840119 andy840119 Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that it's easy not to notice the Apply():

new SliderVelocityMultiplierChange(HitObject, proposedVelocity).Apply(changeHandler);

Maybe can be:

// note: only for the case that NewBeatmapEditorChangeHandler  is nullable.
// use drawable because Drawable support the DI.
public static void ApplyChange(this Drawable drawable, NewBeatmapEditorChangeHandler? changeHandler, IRevertibleChange change, bool commitImmediately = false)
{
}

And use like

public class HitObjectComposer: CompositeDrawable
{
    [Resolved(canBeNull: true)]
    private NewBeatmapEditorChangeHandler changeHandler { get; set; }

    public void EndPlacement(HitObject hitObject, bool commit)
    {
       ApplyChange(changeHandler, new AddHitObjectChange(EditorBeatmap, hitObject));
    }
}

Maybe can ask other developer's opinion(i know it's hard to make the decision, all of the solution to deal with nullable case until now is not satisfied).

Copy link
Contributor Author

@OliBomby OliBomby Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extending Drawable for this seems weird.
I think the current Apply() is alright. If you forget the Apply() altogether you'll get a warning, so you won't miss it. The worst that could happen is you forget to add the changeHandler but I think AI autocomplete will get this right most of the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants