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

Add hold-to-quit button for gameplay/replays #2430

Merged
merged 35 commits into from May 22, 2018

Conversation

4 participants
@UselessToucan
Contributor

UselessToucan commented Apr 20, 2018

I want to be able to exit replay using mouse.
https://www.youtube.com/watch?v=LnXfhS0o_PQ

@peppy

This comment has been minimized.

Member

peppy commented Apr 20, 2018

We really need something like this for play modes too. @arflyte anything in mind?

@peppy peppy added the design label Apr 20, 2018

@arflyte

This comment has been minimized.

arflyte commented Apr 20, 2018

An "exit" button for play mode and replay?

@peppy

This comment has been minimized.

Member

peppy commented Apr 20, 2018

Some mechanism to exit gameplay, for touch users for instance.

@arflyte

This comment has been minimized.

arflyte commented Apr 20, 2018

Haven't worked on touch interface elements yet. Use this for now, will revisit the buttons again later.

spectator-screen

To quit, player have to hold the button for 1s. "Hold to Quit" fades away after 5s. It's either "Quit" or "Resign", depends on the mode.

On hold, the action icon becomes bigger, "Hold to Quit" is visible again, and progress ring appears.

ee3397bea7

@peppy

This comment has been minimized.

Member

peppy commented Apr 20, 2018

Note that this should be displayed in both play and replay, but probably doesn't need to completely fade during replay.

During play it should fade out until the cursor is hovering it and then appear again.

@peppy peppy added this to the Candidate Issues milestone Apr 20, 2018

@peppy peppy added the gameplay label Apr 20, 2018

@UselessToucan

This comment has been minimized.

Contributor

UselessToucan commented Apr 21, 2018

Looks ok to me
https://www.youtube.com/watch?v=lg4_EXMZiSs

However I don't know what to do with this new HoldToQuit UI element during actual gameplay.

  1. As osu! standard player I just don't need this thing on the screen during gameplay.
  2. During gameplay Exit call causes player to pause(pause != quit). https://www.youtube.com/watch?v=RiqKzdcun48
@peppy

This comment has been minimized.

Member

peppy commented Apr 21, 2018

Where's the test case?

@UselessToucan

This comment has been minimized.

Contributor

UselessToucan commented Apr 21, 2018

Not written yet. :(

@peppy

This comment has been minimized.

Member

peppy commented Apr 21, 2018

How did you even make this component without a test case? Please read the wiki regarding how to develop for this project before going any further.

@peppy

This comment has been minimized.

Member

peppy commented Apr 21, 2018

This whole thing can be done using transforms and without a clock or whatever.

onMouseDown
{
    ring.FillTo(1, duration).OnComplete(_ =>
    {
        // exit
    });
}

onMouseUp
{
    ring.FillTo(0, duration/4); // interrupts the other transform and causes the exit to never occur.
}
@peppy

This comment has been minimized.

Member

peppy commented Apr 28, 2018

Did you see my comment above? It would clean up the implementation considerably.

UselessToucan added some commits Apr 28, 2018

@smoogipoo

Needs some work on the design, but let's go with these reviews for now.

@@ -216,6 +216,8 @@ private void load(AudioManager audio, APIAccess api, OsuConfigManager config)
}
};
hudOverlay.HoldToQuit.Button.ExitAction = Exit;

This comment has been minimized.

@smoogipoo

smoogipoo May 2, 2018

Contributor

Would rather that the button wasn't exposed. Make an Action property in HoldToQuit which proxies the passed value into the button. Make the button itself private.

{
Anchor = Anchor.BottomRight,
Origin = Anchor.BottomRight,
Margin = new MarginPadding { Bottom = Progress.Size.Y * 1.25f, Right = 5 }

This comment has been minimized.

@smoogipoo

smoogipoo May 2, 2018

Contributor

Shouldn't be relative to progress. Just make it 70 or something.

namespace osu.Game.Screens.Play.HUD
{
public class HoldToQuit : FillFlowContainer

This comment has been minimized.

@smoogipoo

smoogipoo May 2, 2018

Contributor

Rename to QuitButton.

AutoSizeAxes = Axes.Both;
}
public class HoldToQuitButton : CircularContainer

This comment has been minimized.

@smoogipoo

smoogipoo May 2, 2018

Contributor

Rename to Button, make private.

@@ -205,6 +207,13 @@ protected virtual ScoreCounter CreateScoreCounter() => new ScoreCounter(6)
RelativeSizeAxes = Axes.X,
};
protected virtual HoldToQuit CreateHoldToQuit() => new HoldToQuit

This comment has been minimized.

@smoogipoo

smoogipoo May 2, 2018

Contributor

Rename to CreateQuitButton.

@smoogipoo

as commented

UselessToucan and others added some commits May 9, 2018

@peppy peppy changed the title from Add 'End replay' button to Add hold-to-quit button for gameplay/replays May 21, 2018

@peppy

This comment has been minimized.

Member

peppy commented May 21, 2018

@UselessToucan highly recommend you check my code changes for this. look how elegant the shared logic is now

hudOverlay = new HUDOverlay(scoreProcessor, RulesetContainer, working, offsetClock, adjustableClock)
{
Clock = Clock, // hud overlay doesn't want to use the audio clock directly
ProcessCustomClock = false,
Anchor = Anchor.Centre,
Origin = Anchor.Centre
},
RulesetContainer.Cursor?.CreateProxy() ?? new Container(),

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

You moved the cursor under the overlay?

This comment has been minimized.

@peppy

peppy May 22, 2018

Member

Yeah, I think it feels better and makes more physical sense

image

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

So you're fine with the cursor being below:

  1. Leaderboard
  2. Key counters

?

I think we need to discuss what the scope of a hud is going to be. Should the hud handle combo pop-in images?

{
base.Update();
float adjust = Vector2.Distance(GetContainingInputManager().CurrentState.Mouse.NativeState.Position, button.ScreenSpaceDrawQuad.Centre) / 200;

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

We surely don't want to be querying this every frame. Can we do it once on LoadComplete(), until the point (hopefully never) that it's required to be refreshed?

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

As discussed, let's use ReceiveMouseInputAt() => true here.

{
base.Confirm();
// temporarily unbind as to not look weird during flash animation.

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

This needs to explain how this would look weird. Shouldn't have to go in and find that it would look weird if the button is released after confirming.

This comment has been minimized.

@peppy

peppy May 22, 2018

Member

not sure i follow

// temporarily unbind as to not look weird during flash animation.
Progress.UnbindAll();
pendingAnimation = true;

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

Is this pendingAnimation thing done solely for the testcase? This needs a comment.

This comment has been minimized.

@peppy

peppy May 22, 2018

Member

no, it's used to stop a second confirmation while animatng (see it's only usage)

Progress.UnbindAll();
pendingAnimation = true;
innerCircle.ScaleTo(0, 100)

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

Please add a comment explaining what innerCircle is. I had to go through and change the transforms to figure out that it's adding the shutter effect as an element over the top of the circular progress...

Perhaps even rename innerCircle to shutterCircle.

This comment has been minimized.

@peppy

peppy May 22, 2018

Member

will rename to overlayCircle, shutter feels weird since it comes out from the middle.

innerCircle.ScaleTo(0, 100)
.Then().FadeOut().ScaleTo(1).FadeIn(500)
.OnComplete(a => circularProgress.FadeOut(100).OnComplete(_ =>

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

This fadeout seems to cause some weird flashing of the border: https://streamable.com/3pzol .

This comment has been minimized.

@peppy

peppy May 22, 2018

Member

that's intended (it's not flashing, just a fade out). i guess we should add special behaviour for a user continuing to hold the mouse button down

base.Confirm();
// temporarily unbind as to not look weird during flash animation.
Progress.UnbindAll();

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

Due to this unbind, the icon scale doesn't get reset properly: https://streamable.com/62klb

bool stayVisible = text.Alpha > 0 || button.Progress.Value > 0 || IsHovered;
Alpha = stayVisible ? 1 : Interpolation.ValueAt(elapsed, Alpha, MathHelper.Clamp(1 - adjust, 0.04f, 1), 0, 200, Easing.OutQuint);

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

We'll want a debounce, otherwise it's going to trigger readily for 4:3 players.

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

Also this affects the alpha while the pause screen is active, looks pretty weird.

namespace osu.Game.Graphics.Containers
{
public abstract class HoldToCofirmContainer : Container

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2018

Contributor

HoldToCo n firmContainer

@peppy

This comment has been minimized.

Member

peppy commented May 22, 2018

All fixed 👍

@smoogipoo

lgtm

@smoogipoo smoogipoo merged commit c0802b5 into ppy:master May 22, 2018

1 of 2 checks passed

CodeFactor 1 issue found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@UselessToucan UselessToucan deleted the UselessToucan:exit_replay_button branch May 25, 2018

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