Skip to content
This repository has been archived by the owner on Mar 19, 2021. It is now read-only.

GUI: Added configurable keyboard mappings to the project #174

Merged
merged 2 commits into from
Feb 29, 2020

Conversation

flplv
Copy link
Contributor

@flplv flplv commented Feb 26, 2020

This PR adds the support to configure the keybindings using the settings dialog.

It also rollback a recent commit that bound mouse button to the touch pad of the PS4 controller, now just go the settings screen and bind a key to the pad (@3kinox, please let me know if you accept this change).

Next feature I will develop is an autoclick functionality. Let me know where do you believe that should be, at GUI or chiaki library?

I suggest you review this PR commit by commit, so you can avoid reading some of the formatting fixes in the settings.cpp file.

@juicypop
Copy link

@flplv would this solve my issue #169?

@flplv
Copy link
Contributor Author

flplv commented Feb 26, 2020

I think yes, if the controllers are treated as digital keys. But no actual analog channel are supported. Sticks and L2, R2 are fixed values.

Does QT can read analog keys from usb or Bluetooth controllers?

Copy link
Owner

@thestr4ng3r thestr4ng3r left a comment

Choose a reason for hiding this comment

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

Very nice, I like the overall approach.
Please make sure to closely follow the formatting style of the rest of the code, especially the changes in settingsdialog.cpp are still wrongly formatted. You can use this as a reference: https://gist.github.com/thestr4ng3r/d0b1de10852ca3d119e8c83375211e4b

CHIAKI_CONTROLLER_ANALOG_STICK_RIGHT_X_DOWN = (1 << 23),
CHIAKI_CONTROLLER_ANALOG_STICK_RIGHT_Y_UP = (1 << 24),
CHIAKI_CONTROLLER_ANALOG_STICK_RIGHT_Y_DOWN = (1 << 25),
} SettingsExtendedAnalogStickEnum;
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be better to differentiate these a bit more from the original definitions. I would do it like this (no typedef needed in C++):

enum class ControllerButtonExt {
	ANALOG_STICK_LEFT_X_UP = (1 << 18),
	...
};

Then later use it like static_cast<int>(ControllerButtonExt::ANALOG_STICK_LEFT_X_UP)

Q_OBJECT

private:
private:
Copy link
Owner

Choose a reason for hiding this comment

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

Unnecessary leading space

@@ -41,14 +52,22 @@ class Settings : public QObject
void LoadManualHosts();
void SaveManualHosts();

public:
public:
Copy link
Owner

Choose a reason for hiding this comment

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

leading space


bool GetLogVerbose() const { return settings.value("settings/log_verbose", false).toBool(); }
void SetLogVerbose(bool enabled) { settings.setValue("settings/log_verbose", enabled); }
bool GetDiscoveryEnabled() const {
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep the one-line formatting for these

void KeyCaptured(Qt::Key);

protected:
void keyPressEvent(QKeyEvent *event) override;
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't used, so can be removed.

void keyReleaseEvent(QKeyEvent *event) override;

public:
SettingsKeyCaptureDialog(QWidget *parent = nullptr);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
SettingsKeyCaptureDialog(QWidget *parent = nullptr);
explicit SettingsKeyCaptureDialog(QWidget *parent = nullptr);

@@ -47,6 +48,7 @@ class ChiakiException: public Exception

struct StreamSessionConnectInfo
{
Settings * settings;
Copy link
Owner

Choose a reason for hiding this comment

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

This struct shouldn't depend on the actual Settings class. Better add a member of type QMap<Qt::Key, int> and extract the value in the StreamSessionConnectInfo(Settings *settings, QString host, QByteArray regist_key, QByteArray morning) constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you feel about passing Settings * to StreamWindow constructor, in order to let it connect to a signal in Setttings that would be called KeyboardMapChanged? That would enable the user to change the mapping without having to reopen the stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or I can do what you say, but then at the same time connect the slot externally to StreamWindow. I think that would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

humm.. I realized that the StreamWindow is consuming all keypress, and not forwarding to other windows, so changing the key mapping while stream is on is not possible. Not sure if want to change this behavior.


void SettingsKeyCaptureDialog::keyReleaseEvent(QKeyEvent* event) {
KeyCaptured(Qt::Key(event->key()));
reject();
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
reject();
accept();

Doesn't change the behavior in this case, but accept makes more sense.

bool press_event = event->type() == QEvent::Type::KeyPress;

if (press_event)
keyboard_state.buttons |= button;
Copy link
Owner

Choose a reason for hiding this comment

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

keyboard_state.buttons should not be touched for the extended buttons. I think this whole if(press_event) could be moved into the default case of the switch below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you caught a bug, thanks.

@@ -56,6 +56,7 @@ typedef enum chiaki_controller_analog_button_t
CHIAKI_CONTROLLER_ANALOG_BUTTON_R2 = (1 << 17)
} ChiakiControllerAnalogButton;


Copy link
Owner

Choose a reason for hiding this comment

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

Unnecessary change

@thestr4ng3r
Copy link
Owner

The reason for the mouse click for touchpad button was that Linux by default handles the Dualshock's touchpad as a mouse-like touchpad so if you use the actual controller you could use the real touchpad button. Mapping it to a keyboard key wouldn't help here, so please keep it for now.

What exactly do you mean by "autoclick"? Something like a turbo button that automatically presses a button in a fast interval?

@flplv
Copy link
Contributor Author

flplv commented Feb 26, 2020

Ah got it, just paired my PS4 controller to the PC and saw the issue.

Where do you keep user manual? in the README? I believe a section speaking how the Desktop client can be used could be useful, specifically to point out that PS4 can be paired and or keyboard can be mapped.

Autoclick is a feature that I particularly use, and that was what drove me to this project. It is a timed automatic clicker for AFK gaming, it just periodically click a given button or sequence of buttons. It is useful for games like Minecraft. In Minecraft we use for AFK Fishing to level up and collect enchantment books. If you find it too specialized to the project, that is fine, I can keep it on my fork. But If you want the feature in the project, I'd add another entry in the settings menu where the user can configure the timer and the button to auto click, then the internals of the autoclick could go either in the Gui or in the shared library.

I will make some commits to address your inputs and I'll let you know.

@3kinox
Copy link
Contributor

3kinox commented Feb 26, 2020

@flplv For the setting + touchpad fix, one way is simply to give the option to use any input types (key, controller button, mouse, or whatever) for binding. Not sure if you can do it straight in QT or not. Otherwise, a cleaner and more versatile fix could be to use SDL or evdev for all inputs, they should also give you control over analog vs numeric sticks/ R2-L2 (and rumble).

@flplv
Copy link
Contributor Author

flplv commented Feb 26, 2020

@flplv For the setting + touchpad fix, one way is simply to give the option to use any input types (key, controller button, mouse, or whatever) for binding. Not sure if you can do it straight in QT or not. Otherwise, a cleaner and more versatile fix could be to use SDL or evdev for all inputs, they should also give you control over analog vs numeric sticks/ R2-L2 (and rumble).

That is a good idea. I will keep in mind for the next improve.

@flplv
Copy link
Contributor Author

flplv commented Feb 26, 2020

@thestr4ng3r I believe I addressed all your comments.

@flplv flplv force-pushed the flplv/settings branch 2 times, most recently from f956fbc to 9f76359 Compare February 26, 2020 22:11
@thestr4ng3r
Copy link
Owner

Where do you keep user manual? in the README?

Yes, for now there is only the readme, but more extensive documentation is planned.

Autoclick is a feature that I particularly use, and that was what drove me to this project. It is a timed automatic clicker for AFK gaming, it just periodically click a given button or sequence of buttons. It is useful for games like Minecraft. In Minecraft we use for AFK Fishing to level up and collect enchantment books. If you find it too specialized to the project, that is fine, I can keep it on my fork. But If you want the feature in the project, I'd add another entry in the settings menu where the user can configure the timer and the button to auto click, then the internals of the autoclick could go either in the Gui or in the shared library.

I see, well I'm not entirely sure I want that as part of the main project. But you can open an issue about it so we can discuss it in detail there.

@thestr4ng3r thestr4ng3r merged commit a605ba0 into thestr4ng3r:master Feb 29, 2020
@thestr4ng3r
Copy link
Owner

Thank you very much!
If you want to continue on this, a button to reset the key bindings to the default would be nice.

@flplv flplv deleted the flplv/settings branch March 4, 2020 18:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants