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

Rewrite editor selections to better handle selection and movement logic #2329

Merged
merged 51 commits into from Apr 6, 2018

Conversation

2 participants
@smoogipoo
Contributor

smoogipoo commented Mar 29, 2018

Fixes #2211
Fixes hitobjects not moving on drag.

It'd be a good idea to go through this in code, rather than looking at the diff, since most of everything besides HitObjectMask doesn't work the same way anymore.

The general idea behind this is that we need the masks to handle mouse down events, as they need to handle the drag (mousedown -> drag immediately).

I've rewritten the editor selections to use HitObjectMask events, as there are some 3-4 different components that handle/trigger selections in different ways.

  1. All selections/deselections now propagate through HitObjectMask.Select()/HitObjectMask.Deselect().
  2. Components that react to changes in the selection bind to the masks' Selected/Deselected events, and track them/change their states locally.
  3. SelectionBox renamed to Selection (with an internal box), and now handles all single-click-selection + drag-movement logic.
  4. DragBox now handles all drag input locally. It triggers Select/Deselect on the masks it needs to.
  5. DragBox handles the display of itself.
  6. Selection handles the display of its box itself.
  7. Selection handles movement of selections.

Note that the HitObjectMaskLayer.Select() and HitObjectMaskLayer.Deselect() methods have been removed. We'll probably want these back in the future, but it's too early for them. They'd be easy to implement anyway ^^.

smoogipoo added some commits Mar 29, 2018

Rewrite the way drag + click selections happen
The general idea here is that we need the masks to handle mouse down events, as they need to handle the drag (mousedown -> drag immediately).

I've rewritten the editor selections to use events, as there are some 3 different components that handle/trigger selections in different ways.

1. All selections/deselections now propagate through `HitObjectMask.Select()`/`HitObjectMask.Deselect()`.
2. Components that react to changes in the selection bind to the masks' `Selected`/`Deselected` events, and track them/change their states locally.
3. Masks provide a `SingleSelectionRequested` event which is invoked on the mouse-down event. Various components bind to this event to perform state changes locally in this scenario.
4. `DragBox` now handles all drag input locally. It triggers `Select`/`Deselect` on the masks it needs to.
5. `SelectionBox` handles the display of itself locally.
6. `SelectionBox` handles movement of groups of masks locally.
7. `HitObjectMasks` handles movement of itself locally.
@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Mar 29, 2018

Also this has allowed us to move the SelectionPoint/SelectionQuad stuff from DrawableHitObject to HitObjectMask, no more pollution of hitobjects with editor stuff :D/

@peppy peppy changed the title from Selectionlayer rewrite to SelectionLayer rewrite Mar 30, 2018

@smoogipoo smoogipoo changed the title from SelectionLayer rewrite to Rewrite editor selections to better handle selection and movement logic Apr 2, 2018

peppy added some commits Apr 2, 2018

/// <summary>
/// A box that handles and displays drag selection for a collection of <see cref="HitObjectMask"/>s.
/// </summary>
public class DragBox : CompositeDrawable

This comment has been minimized.

@peppy

peppy Apr 3, 2018

Member

Should probably be called something like DragLayer, as it itself is not a box but contains a box.

box.Position = topLeft;
box.Size = bottomRight - topLeft;
foreach (var mask in maskContainer.AliveMasks)

This comment has been minimized.

@peppy

peppy Apr 3, 2018

Member

might be nicer if we can just call maskContainer.SelectWithin(rectangle).

This comment has been minimized.

@smoogipoo

smoogipoo Apr 3, 2018

Contributor

I dunno about this. In the future we're probably going to want to override this somehow - not sure if the best way to do that is overriding the DragLayer (as you would for the Selection), overriding the mask container, or overriding some other place in code. I want to leave it until we get to a point where we need to consider that (i.e. mania/taiko editors).

To me, maskContainer is just a container that exposes the alive children and ensures that events are bound correctly.

This comment has been minimized.

@peppy

peppy Apr 4, 2018

Member

Okay, let's leave it, but definitely keep this in mind.

@@ -24,14 +25,22 @@ public class HitObjectMask : VisibilityContainer
/// </summary>
public event Action<HitObjectMask> Deselected;
/// <summary>
/// Invoked when this <see cref="HitObjectMask"/> has rqeuested selection.

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

xmldoc typo

/// <summary>
/// The <see cref="DrawableHitObject"/> which this <see cref="HitObjectMask"/> applies to.
/// </summary>
public readonly DrawableHitObject HitObject;
protected override bool ShouldBeAlive => HitObject.IsAlive || State == Visibility.Visible;
public override bool RemoveWhenNotAlive => false;
public override bool HandleMouseInput => HitObject.IsPresent;
public override bool HandleMouseInput => ShouldBeAlive;

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

Why was this changed from IsPresent? This doesn't actually do anything anymore - drawables can't handle input if they're not alive anyway.

The purpose of this was, for example, if a hitobject disappears but hasn't been expired yet - something like:

myHitObject.FadeOut(100).ScaleTo(2f, 300).Expire()

Will expire at t = 300, but I didn't want this hitobject to be selectable beyond 100ms since it's invisible by that point.

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

Changing this back to how it was would also put it back in line with dragselection (see maskContainer.Select(RectangleF))

{
this.maskContainer = maskContainer;
this.performSelection = performSelection;

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

Newline between this and the rest, looks nicer.

@@ -33,23 +33,29 @@ private void load()
{
maskContainer = new MaskContainer();
selectionBox = composer.CreateSelectionBox(maskContainer);
maskSelection = composer.CreateSelectionBox(maskContainer);

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

While we're here, how about we rename the composer method too? Hasn't been a selection box since the beginning of this PR :P

@@ -22,17 +23,25 @@ public class MaskContainer : Container<HitObjectMask>
/// </summary>
public event Action<HitObjectMask> MaskDeselected;
public event Action<HitObjectMask> MaskSelectionRequested;

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

Needs an xmldoc

{
// todo: remove this

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

todo?

@@ -63,39 +63,23 @@ private void load(OsuColour colours)
#region User Input Handling
// Only handle input on selectable or selected masks
public override bool ReceiveMouseInputAt(Vector2 screenSpacePos) => selectableMasks.Reverse().Concat(selectedMasks).Any(m => m.ReceiveMouseInputAt(screenSpacePos));
public override bool ReceiveMouseInputAt(Vector2 screenSpacePos) => true;

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

Is this required? What is this doing?

DeselectAll();
selectableMasks.Reverse().First(m => m.ReceiveMouseInputAt(state.Mouse.NativeState.Position)).Select();
protected override bool OnDragStart(InputState state) => handleInput(state);

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

Because drag is delayed by a number of pixels, this will cause problems if you drag out from the border of a mask (no masks will be receiving mouse input).

public void DeselectAll() => selectedMasks.ToList().ForEach(m => m.Deselect());
private void onSelectionRequested(HitObjectMask mask)
{
if (GetContainingInputManager().CurrentState.Keyboard.ControlPressed)

This comment has been minimized.

@smoogipoo

smoogipoo Apr 4, 2018

Contributor

No way, if anything SelectionRequested should give us an input state.

@smoogipoo

Looks good apart from 2 more comments.

return false;
selectionRequested = false;
if (State == SelectionState.NotSelected && !selectionRequested)

This comment has been minimized.

@smoogipoo

smoogipoo Apr 5, 2018

Contributor

We're guaranteed to be selectionRequested == false here.

{
if (mask.IsSelected)
return;

This comment has been minimized.

@smoogipoo

smoogipoo Apr 5, 2018

Contributor

Extra whitespace.

peppy added some commits Apr 5, 2018

Remove TestTestCase
No longer necessary as we have restructured tests considerably.
@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 6, 2018

Input is no longer ordered correctly: https://streamable.com/3e8lo

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 6, 2018

This feels weird: https://streamable.com/kcvwy Click+dragging on a mask that is not selected but rests on top of a selected mask will transfer the selection to the clicked mask. Regressed by the previous change.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 6, 2018

Looks good to me

@peppy peppy dismissed their stale review via ae2dce2 Apr 6, 2018

@peppy

peppy approved these changes Apr 6, 2018

@peppy peppy merged commit b91f538 into ppy:master Apr 6, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@smoogipoo smoogipoo deleted the smoogipoo:selectionlayer-rewrite branch Jun 15, 2018

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