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 ability to bind "back" action; add default mouse binding #2472

Merged
merged 12 commits into from May 18, 2018

Conversation

3 participants
@UselessToucan
Contributor

UselessToucan commented Apr 29, 2018

resolves #2410

@peppy peppy added the resolves issue label May 2, 2018

@peppy peppy changed the title from Add back mouse button support to Add ability to bind “back” button; add default mouse binding May 2, 2018

@smoogipoo smoogipoo added this to the May 2018 milestone May 2, 2018

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 2, 2018

I don't think we want mb1 to trigger the pause screen during gameplay or cause the game to exit.

@peppy

This comment has been minimized.

Member

peppy commented May 2, 2018

Did you test this one? MouseButton1 isn't what you think it is.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 2, 2018

mb1 is correct.
screen shot 2018-05-02 at 2 38 04 pm

@UselessToucan

This comment has been minimized.

Contributor

UselessToucan commented May 2, 2018

Did you test this one? MouseButton1 isn't what you think it is.

Yes, on my mouse MouseButton1 corresponds to the back button.

I don't think we want mb1 to trigger the pause screen during gameplay or cause the game to exit.

I probably should handle it in concrete classes like SongSelect instead of OsuScreen.

@UselessToucan

This comment has been minimized.

Contributor

UselessToucan commented May 3, 2018

@smoogipoo where exactly should the mouse back button be handled? Are there any places besides song select screens and the button system in the main menu?

@peppy

This comment has been minimized.

Member

peppy commented May 4, 2018

It should probably be at an OsuScreen level, with an exposed protected virtual bool AllowBackButton for certain screens to disable.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 6, 2018

  1. This has broken escape key handling to exit from player / playerloader.
  2. I still don't think we should allow exiting the game via the back button.

Why not have two separate actions instead of grouping them together? One being specifically Exit (escape) and the other being Back?

@peppy

This comment has been minimized.

Member

peppy commented May 6, 2018

I’m against having two bindable actions as it will be confusing from the user’s perspective (when does exit work, when does back work?)

If anything the action should be called Exit/Back IMO.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 6, 2018

Don't have a separate "Exit" action, just keep it as escape button?

@peppy

This comment has been minimized.

Member

peppy commented May 6, 2018

Hmm, that should be fine, although we will need to consider what behaviour escape will have during gameplay then. Having it act (as it does in stable) as "toggle pause" may feel weird.

UselessToucan added some commits May 7, 2018

@peppy

This comment has been minimized.

Member

peppy commented May 14, 2018

Why is the back action no longer bindable?

@UselessToucan

This comment has been minimized.

Contributor

UselessToucan commented May 14, 2018

Looks like I did not understand latest comments correctly(strating from this).

  1. If both esc and mouse button are suitable for the usage scenario, input should be handled using GlobalAction.
  2. If only esc is suitable, input should be handled using OnKeyDown override.

Are these statements correct?

UselessToucan and others added some commits May 14, 2018

@peppy peppy changed the title from Add ability to bind “back” button; add default mouse binding to Add ability to bind "back" action; add default mouse binding May 18, 2018

@peppy

peppy approved these changes May 18, 2018

@peppy peppy merged commit cdd43f7 into ppy:master May 18, 2018

2 checks passed

CodeFactor No issues found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@UselessToucan UselessToucan deleted the UselessToucan:back_mouse_button_support branch May 18, 2018

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