Refactor rotation handling in editor to facilitate reuse

[bdach]

RFC. While everything was manually tested to work, opening as draft, as this is a long shot to me direction-wise.

The primary goal of this change is to build on top of it to ship #12045. A preview branch with the implementation of aforementioned issue can be seen here: https://github.com/ppy/osu/compare/master...bdach:osu:precise-rotation-2?expand=1

There are a few core hang-ups in the current editor selection handling flows that made me try this particular direction:

• While SelectionHandler defines HandleMovement(), HandleRotation(), HandleScale(), etc., it cannot by itself tell if it is even possible to perform those actions, because so far, the only singular component that knows this and uses this knowledge is SelectionBox. This is an issue when wanting to reuse the rotation/movement availability & performance logic somewhere else, such as in the rotation control mentioned in the issue above.

• Delivering selection edit flows with continuous preview that the rotation control requires is difficult, because for complete correctness, it is required to store the initial state of the selection when a continuous edit flow is initiated, so that changes made to the rotation operation performed when the rotation popover is opened can be previewed correctly. The one particular problematic case is switching rotation origin from playfield centre to selection centre when the popover is open.

  At this point you could say that EditorChangeHandler already can do this, via undo. Unfortunately the way EditorChangeHandler works, at least in beatmap editor, is that it will just remove and recreate objects which were moved, therefore discarding selection. This is also quite noticeable UI-wise; if you're unlucky, you can see the object disappear for a moment. Trying to leverage undo in the continuous edit flow makes that much more apparent.

  Fixing this so that undo preserves object instances feels like an objectively worse solution, since it's combining the worst aspects of the snapshot vs delta dichotomy of implementing undo (not only would we be storing lots of data, we would also have to remember to unapply everything correctly, too).

To this end, the proposed direction is probably best summed up by reading SelectionRotationHandler and its xmldocs. It offers a bindable to be able to tell if you can perform a rotation, as well as two different APIs to actually perform one: one simple, to perform an instant rotation, and a "continuous" one, which allows the parameter of the operation to be changed to the user's heart's content so long as Commit() isn't called.

Now, a few caveats:

• This change is ruleset API breaking in a major way. There is probably a way to make it not so, but I decided not to worry about this that much yet, because I want to see if I will even get agreement on this direction.

• If this direction takes, I will incrementally apply it onto all remaining editor operations across all rulesets. I just want to see the reaction this gets first.

• Reviewers will likely notice lots of boilerplate in the implementation of SelectionRotationHandler, like the fact that 2 of 3 implementations accept IEditorChangeHandler, and they also have their own BindableLists with selected objects, and pretty much all manage slightly different pieces in Begin()/Commit().

  Reviewers may also be poised to suggest an overarching interface for this, something similar to

  public interface ISelectionOperationHandler<TObject, TParams>
  {
       BindableList<TObject> SelectedObjects { get; }
  
       IBindable<bool> CanPerform { get; }
  
       void Begin();
       void Update(TParams params);
       void Commit();
  }

  I would ask to at least hold off on this until migration of all operations is completed, but am also relatively certain that doing this would be likely impractical, if not straight up impossible. Some of the constraints in place such as:

  • the fact that if I do SelectionScaleHandler, it will have CanScaleX and CanScaleY
  • if I do SelectionFlipHandler, it will not have the continuous interface, because it won't need it
  • SelectionBox, which now accepts the SelectionRotationHandler, is generic-unaware and I'd like to keep it that way

  mean that I think there is likely no appropriate common abstraction.

• The diff is huge, which I apologise for; if it helps any, 4622255 is menial, takes up ~300 lines of diffstat, and is trivially splittable into its own PR. I just don't want to overdo ceremony on a long shot PR that I may end up closing pretty much out of the gate if the direction doesn't take.

[peppy]

I'm pretty happy with this direction, would say you should carry on.

[peppy]

One thing to note is that as SelectionRotationHandler isn't a drawable, this binding won't be eagerly cleaned up. I think it's fine as long as only one RotationHandler is created ever (seems to be the case) and the lifetime is generally the same as the source bindable.

[bdach]

That is a good point, and which has been in the back of my mind (as non-drawables binding to drawable bindables has been a direct cause of many ugly bugs in days past), but one I didn't want to spend too much attention on before getting any feedback on the overall direction. I will reconsider making SelectionRotationHandler a Component that lives in the hierarchy instead to remedy this; it may even open up opportunities for DI usage. I'll see.

[bdach]

I've changed to Component, which opened up several avenues for DI usage in 262f25d and ebe5dd2. I think those changes cleaned up a lot of rough edges, but I guess mileage may vary, so interested to see if you agree.

[bdach]

would say you should carry on

I guess two main questions from me as to how to proceed here:

• Should I attempt to make this change non-API-breaking for rulesets' sake, or is breaking rulesets OK? I'm not even sure we know which custom editors have rulesets at this point, and unfortunately our naming of the broken methods is generic enough that github code search turns up mostly garbage...
• I'm guessing it will be okay to do just rotation in this pull, and do the rest in subsequent pulls similar in appearance to this one? Or would you want to see the completed result with all operation handlers factored out first?

[peppy]

• Breaking API is fine. Ruleset editors are not really usable now (pretty bad) due to not being able to save so it's fine.
• Yep, just rotation is fine.

[OliBomby]

I'm a big fan of this change. Not only does this allow changing rotation origin, it will also fix the scale operation losing precision when scaling to 0x and back in the same operation, and allow new scaling features such as preserving the original aspect ratio when pressing shift (like many drawing applications).

[bdach]

Correct, all of those were among the underlying reasons to take this direction.

Hopefully I'll get this out of draft today. No promises though.

[bdach]

I don't have any more changes to make here for now I don't think, so I took the PR out of draft. Further changes will come in separate PRs.