Skip to content
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

Pad Refactoring #3283

Merged
merged 2 commits into from
Sep 8, 2017
Merged

Pad Refactoring #3283

merged 2 commits into from
Sep 8, 2017

Conversation

RipleyTom
Copy link
Contributor

@RipleyTom RipleyTom commented Aug 20, 2017

Adds a window to setup multiple input types as once
All controllers are now handled by a single thread
[hcorion] evdev refactor

@mention-bot
Copy link

@RipleyTom, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jarveson, @mputters and @georgemoralis to be potential reviewers.

pad->m_vibrateMotors.emplace_back(true, 0);
pad->m_vibrateMotors.emplace_back(false, 0);

bindings.push_back(std::make_pair(device_number, pad));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not emplace here too?

dialog_layout->addLayout(all_players);

QHBoxLayout *buttons_layout = new QHBoxLayout();
QPushButton *ok_button = new QPushButton("OK");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tr("OK")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not really needed

Copy link
Contributor

@danilaml danilaml Aug 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Megamouse why? isn't the code here is just doing poor man's qdialogbuttonbox? With it, the dialog would have the standard buttons with the standard (to the user OS) button layout and translations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well i don't care. I'm fine with it either way.

QHBoxLayout *buttons_layout = new QHBoxLayout();
QPushButton *ok_button = new QPushButton("OK");
buttons_layout->addWidget(ok_button);
QPushButton *cancel_button = new QPushButton("Cancel");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tr("Cancel")

@RipleyTom RipleyTom force-pushed the PadMagic branch 3 times, most recently from 7f8346c to 0c9dd10 Compare August 23, 2017 00:27
@hcorion hcorion mentioned this pull request Aug 23, 2017
@toccata10
Copy link
Contributor

Nice improvement. A few tests with evdev (linux):

  1. I launch rpcs3, and configure my pads like this for 2 players:
    gamepads settings_002
    I click ok: it works.
  2. I open again the gamepad config:
    gamepads settings_003
    It attributes the same device to player 1 and 2

Suggestions:

  1. When there are several devices, give the lowest to the 1st player, then another one for player 2, ...
  2. If player 1 and player 2 are configured with evdev, but only 1 device is found, disable input for player 2: it doesn't make sense to me to have the same joystick controlling 2 different players. With Castle crashers for example, if I want to play alone, I plug 1 controller. If I want to play with a friend, I plug 2 controllers. It should work without having to re-open the pad config. With the present code, if I plug only 1 joystick, it'll create and control 2 players. Some games (street fighter HD remix) don't allow the same controller to play 2 fighters, but it's not universal.

@RipleyTom
Copy link
Contributor Author

RipleyTom commented Aug 24, 2017

Yeah there is a bug(set the connects too early), fixing it
Current behavior is:
-On Pad Window: It will keep the input selected but revert to default device if it can't find the device
-On Game: if it can't find the device it reverts to null handler

@toccata10
Copy link
Contributor

It's better, but there's another pb: the /dev/input/event-numbers don't survive a reboot.
So, I have this on the log after a reboot, as my new number is 19.
E Joystick /dev/input/event14 is not present or accessible [previous status: 0]
E Joystick /dev/input/event13 is not present or accessible [previous status: 0]
although the pad configuration shows a device at number 19.
This results in a non-working stick (test with castle crashers).

@toccata10
Copy link
Contributor

So, I suggest to modify the current behaviour:
-On Pad Window: It will keep the input selected but revert to default device if it can't find the device. It tries to assign a different device to each player.
-On Game: if it can't find the device or the device is already used by another player, it reverts to null handler

@RipleyTom RipleyTom force-pushed the PadMagic branch 5 times, most recently from b05b73e to 54fe399 Compare August 26, 2017 22:57
@RipleyTom
Copy link
Contributor Author

RipleyTom commented Aug 26, 2017

Evdev issue should be fixed(hcorion).
Also fixed a small issue with keyboard pad handler that crashed on SetRumble.

@toccata10
Copy link
Contributor

The joystick is not recognized. I send you 2 logfiles: 1 with your PR, the 2nd on master (which is working)
RPCS3-PadPb.zip
RPCS3-PadMaster.zip

@hcorion
Copy link
Member

hcorion commented Aug 27, 2017

@toccata10 Make sure to delete ~/.config/rpcs3/config_input.yml before re-testing PadMagic, and then re-configuring your pads.

@toccata10
Copy link
Contributor

I deleted my config_input.yml and tested. My tests were fine: unplug 1 stick, reboot,... everything worked as expected. Nice !

Adds a window to setup multiple input types as once
All controllers are now handled by a single thread
[hcorion] evdev refactor
Copy link
Member

@hcorion hcorion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've poured over every inch of this PR and can confirm it is completely bug-free and ready for merge.

@AniLeo AniLeo merged commit 0457f23 into RPCS3:master Sep 8, 2017
@RipleyTom RipleyTom deleted the PadMagic branch June 4, 2019 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants