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

Improve mouse-based load inhibition at player loading screen #2078

Merged
merged 6 commits into from Mar 16, 2018

Conversation

3 participants
@UselessToucan
Contributor

UselessToucan commented Feb 16, 2018

We probably do not want to move from PlayerLoader to Player if the user pressed mouse button down on one of the sliderbars and did not release it yet.

I can't provide the video right now, but here are the steps to reproduce:

  1. Perform MouseDown on one of the VisualSettings overlay sliderbars.
  2. Move the cursor outside of the overlay while holding mouse button pressed.

@UselessToucan UselessToucan changed the title from Do push Player while user is interacting with SliderBars to Do not push Player while user is interacting with SliderBars Feb 16, 2018

@peppy peppy added this to the Candidate Issues milestone Feb 18, 2018

@peppy

Still not a huge fan of exposing IsMouseDown.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Feb 19, 2018

How about just using Drawable.IsDragged on the sliderbars?

@UselessToucan

This comment has been minimized.

Contributor

UselessToucan commented Feb 19, 2018

@smoogipoo I tried to use Drawable.IsDragged. It does not solve the problem since it becomes false earlier than I release the mouse button.

@peppy

This comment has been minimized.

Member

peppy commented Feb 19, 2018

According to its implementation, it shouldn't.

@peppy

This comment has been minimized.

Member

peppy commented Mar 5, 2018

Two potential better solutions:

In PlayerLoader.cs

var mouseDownPos = GetContainingInputManager().CurrentState.Mouse.NativeState.PositionMouseDown;
if (player.LoadState != LoadState.Ready || visualSettings.IsHovered || (mouseDownPos != null && visualSettings.ReceiveMouseInputAt(mouseDownPos.Value)))
{
    Schedule(pushWhenLoaded);
    return;
}

In VisualSettings.cs

private bool mouseDown;

public override bool ReceiveMouseInputAt(Vector2 screenSpacePos) => mouseDown || base.ReceiveMouseInputAt(screenSpacePos);

protected override bool OnMouseDown(InputState state, MouseDownEventArgs args)
{
    mouseDown = true;
    return base.OnMouseDown(state, args);
}

protected override bool OnMouseUp(InputState state, MouseUpEventArgs args)
{
    mouseDown = false;
    return base.OnMouseUp(state, args);
}

Both result in very localised implementations, which handle more than just sliders at the same time. Not sure which I prefer.

@UselessToucan

This comment has been minimized.

Contributor

UselessToucan commented Mar 5, 2018

I prefer the first one. In my opinion, it is easier.

@peppy peppy requested a review from smoogipoo Mar 8, 2018

@peppy

This comment has been minimized.

Member

peppy commented Mar 8, 2018

@smoogipoo would like a second opinion on this. it's likely going to pop up in more places going forward if anything (volume overlay being one such example).

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Mar 8, 2018

At the very least I believe these should be inverted. I.e. instead of visualSettings.ReceiveMouseInputAt(...) it should use PlayerLoader.ReceiveMouseInputAt. Same thing with IsHovered.

Furthermore, why does IsDragged not work? Sounds like an issue...

@peppy

This comment has been minimized.

Member

peppy commented Mar 8, 2018

I checked on IsDragged: you can potentially leave the visual settings area before the drag lenience has been hit, at which point there's no dragged state activated.

Inverting could have unwanted side effects, like hovering the toolbar or chat or something. Not sure if we want that.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Mar 8, 2018

The idea being that those places should not handle OnHover if they don't want player's loading to stop.

I mean there really isn't any other way - either you have PlayerLoader check for each component that it needs, or you have some "hey I'm currently handling input" state be passed down to the PlayerLoader that it can check locally.

I believe the chat is an overlaycontainer, yes? In which case it probably already does block passthrough hover and I think that makes more sense than pushing a player while chat is open?

@peppy

This comment has been minimized.

Member

peppy commented Mar 8, 2018

It's definitely worth a try to see how it feels.

@peppy peppy force-pushed the UselessToucan:DoNotMoveToPlayerWhileDragging branch from 524b637 to d3e9102 Mar 8, 2018

Add debouncing to player loading
Allows the mouse to temporarily exit and re-enter overlay elements without triggering a load

@peppy peppy changed the title from Do not push Player while user is interacting with SliderBars to Improve mouse-based load inhibition at player loading screen Mar 8, 2018

@peppy

This comment has been minimized.

Member

peppy commented Mar 8, 2018

Reworked to add debounce and work in a more general fashion. @smoogipoo requesting review as this is now mostly my code ;x

@peppy peppy modified the milestones: Candidate Issues, March 2018 Mar 9, 2018

private void pushWhenLoaded()
{
if (player.LoadState != LoadState.Ready || visualSettings.IsHovered)
Schedule(pushWhenLoaded);

This comment has been minimized.

@smoogipoo

smoogipoo Mar 14, 2018

Contributor

This scheduling is still going on during gameplay

contentOut();

this.Delay(250).Schedule(() =>
if (pushDebounce == null) pushDebounce = Scheduler.AddDelayed(() =>

This comment has been minimized.

@smoogipoo

smoogipoo Mar 14, 2018

Contributor

Put this on a separate line please.

@peppy peppy dismissed their stale review via 08ed026 Mar 14, 2018

@peppy peppy force-pushed the UselessToucan:DoNotMoveToPlayerWhileDragging branch from 08ed026 to ea649f9 Mar 14, 2018

have been resolved

@smoogipoo smoogipoo merged commit b39c04e into ppy:master Mar 16, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@UselessToucan UselessToucan deleted the UselessToucan:DoNotMoveToPlayerWhileDragging branch Mar 22, 2018

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