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

Clean up OSX input selection #5954

Merged
merged 4 commits into from Sep 1, 2017
Merged

Clean up OSX input selection #5954

merged 4 commits into from Sep 1, 2017

Conversation

khg8m3r
Copy link
Contributor

@khg8m3r khg8m3r commented Aug 21, 2017

Because Quartz handles both keyboard and mouse inputs on OSX, is the OSX keyboard and mouse really needed anymore?
This removes the Keyboard/0 input from Dolphin while still allowing for the OSX controller interface to handle OSXJoystick controllers, I think. I'm still able to see my Logitech F310 in D mode in OSX, but I can't say for sure if other controllers emulate a keyboard.
As for the extra USB inputs, I have no idea how to get rid of those......

@khg8m3r khg8m3r changed the title Depreciate old OSX Keyboard and Mouse? Remove old OSX Keyboard and Mouse? Aug 21, 2017
@MayImilae
Copy link
Contributor

MayImilae commented Aug 21, 2017

This sounds great! macOS has waaaaay too many devices that appear, and half of them don't even work! Here's how it looks on my macbook:

screen shot 2017-08-20 at 11 45 17 pm

Which ones of those work and which don't? Let's find out! (EDIT: note that the items don't totally match the order and items below, because it friggin changes every single time and whaaatever!)

✖️ Keyboard/0/Apple Internal Keyboard / Trackpad - Only the trackpad works.
❌ Input/0/Apple IR - Doesn't work, obviously, it's the IR remote receiver! Why does this appear here anyway?
❌ Input/0/Apple Internal Keyboard / Trackpad - Doesn't work.
❌ Input/1/Apple Internal Keyboard / Trackpad - Doesn't work either. Not sure what device this is meant to represent... I haven't used an external keyboard in ages.
❌ Input/0/Apple Mikey HID Driver - Doesn't work, obviously, since this is for inputs from headphone/headset buttons! Why is this in the list again?
❌ Keyboard/0/Karabiner VirtualHIDKeyboard - Obviously this doesn't work, because it's a virtual keyboard used for keyboard rearranging by Karabiner. Proooobably shouldn't be appearing either.
✖️ Keyboard/0/Keyboard - Well the trackpad works, but the keyboard doesn't, even though it's called "keyboard"? ¯_(ツ)_/¯
✖️ Keyboard/1/Keyboard - Um, ditto I guess?
✅ Quartz/0/Keyboard & Mouse - Works!

So um, my laptop has all of these options, and only one of them works the way it should! Like, OMG, we need to fix this.

(@khg8m3r sorry for taking over your PR a little bit with a rant!)

EDIT

Tested the PR, and it's much better!

screen shot 2017-08-20 at 11 54 40 pm

It still has some of the nonsense, but at least it's less! I hope this PR or subsequent PRs can go even further, and get rid of more of these not working input methods! As long as it doesn't impact gamepad functionality, of course.

@ligfx
Copy link
Contributor

ligfx commented Aug 21, 2017

I've been using the Quartz input since it was put in (by me, hehe) and it works fine. OSXJoystick also works with both of my wired Xbox One Controllers (one appears as an Xbox 360 Controller for some reason, with a different layout, but still works).

The only reason I can see to keep this around would be to support an arbitrary number of keyboards. Maybe someone wants to do local multiplayer with many local keyboards? Idk, seems weird.

Anyways, I'm all for this. Like, I'd vote a +2 if I could.

@khg8m3r
Copy link
Contributor Author

khg8m3r commented Aug 21, 2017

@MayImilae That's exactly the problem! Apple has way to many input devices that aren't actually input devices. I'm only on a Hackintosh so I don't have as many inputs as your does, but it's still to many. It even wants to use my USB headset as an input, even on this PR!

screen shot 2017-08-21 at 10 40 32 am

@ligfx Yes, your Quartz implementation was very helpful in pointing me to the files I needed to butcher in order to remove the keyboard :P
As for multiple keyboards, I'm not sure how OSX handles it, but at least on Windows they all appear as the same keyboard with no way to tell them apart. I'll grab an extra keyboard today and double-check to see what multiple keyboards look like to Apple in both OSX and Quartz.

Copy link
Contributor

@ligfx ligfx left a comment

Choose a reason for hiding this comment

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

If the Windows backend combines all physical keyboards into a single logical one (just like Quartz should), then this seems like an easy decision: let's merge this PR and remove the ability to use specify separate physical keyboards, since 1) it's not supported on Windows (far more popular than macOS) and 2) it's confusing to users.

IMO this is fine to be merged once the buildbot complaints are fixed.

@MayImilae
Copy link
Contributor

If the Windows backend combines all physical keyboards into a single logical one

Windows takes care of that itself! Even if you have a dozen keyboards plugged in, it will only present one to software, Dolphin included.

@khg8m3r
Copy link
Contributor Author

khg8m3r commented Aug 22, 2017

If this can get rebuilt and re-tested, I flipped an if statement in OSX.mm that should get rid of all the excess inputs!
Plus I think I fixed the clang-format issues.

Copy link
Contributor

@ligfx ligfx left a comment

Choose a reason for hiding this comment

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

The new change seems like a fine direction: I was just talking with @MayImilae about the same idea on IRC yesterday. It should also allow kHIDUsage_GD_GamePad and kHIDUsage_GD_MultiAxisController, which also represent the general "game controller" idea. Both SDL and Google Chrome limit it to those three usages, so that should be fine.

Also if you could update the name of the PR and change the commit message to reflect the additional change in logic, that would be great!

(Ah, btw, the word you're looking for is "deprecate": it has a similar meaning to "depreciate", but is the one used when talking about software.)

}
#endif
else

This comment was marked as off-topic.

@lioncash
Copy link
Member

lioncash commented Aug 22, 2017

By the way, you may want to set your commit email to the one you use on github, so you can be more properly identified as the author (this can also make it less of a pain if, for whatever reason, we do any licensing related changes way down the line in the distant future and need to contact commit authors).

@khg8m3r
Copy link
Contributor Author

khg8m3r commented Aug 22, 2017

@ligfx are you saying that the kHIDUsage_GD_Joystick includes the GamePad and MultiAxisController, or that it needs to be added so it can recognize them?

@lioncash set my email in GitHub preferences, or in my Terminal where I push the commits?

@ligfx
Copy link
Contributor

ligfx commented Aug 22, 2017

It needs to be added so it can recognize them.

(And pretending I'm @lioncash here for a sec: in the Terminal. See https://help.github.com/articles/setting-your-commit-email-address-in-git/ )

@khg8m3r khg8m3r changed the title Remove old OSX Keyboard and Mouse? Clean up OSX input selection Aug 23, 2017
@khg8m3r
Copy link
Contributor Author

khg8m3r commented Aug 23, 2017

Well I've cleaned up the code and fixed the name, but I have no idea how to properly implement GamePad and MultiAxisController. IOKit documentation is far above what I can understand in a single night....

@ligfx
Copy link
Contributor

ligfx commented Aug 23, 2017

@khg8m3r Don't worry about implementing anything new, the Joystick class should be able to handle most devices. We just need to make sure we allow the devices though: adding those two enums in the same if-statement as the other ones should be enough.

@khg8m3r
Copy link
Contributor Author

khg8m3r commented Aug 23, 2017

@ligfx Unfortunately it is not. Adding an if case for IOHIDDeviceConformsTo(inIOHIDDeviceRef, kHIDPage_GenericDesktop, kHIDUsage_GD_GamePad)) and MultiaxisController in OSX.mm throws compile errors. To fix that compile error you need to add a section for const GamePad* gamepad = dynamic_cast<const GamePad*>(device);, which throws more compile errors. To fix those errors, I copied Joystick.h and Joystick.mm, changed their name to GamePad, and changed all references from joystick to gamepad, but it still threw compile issues. That was the point at which I went online to try and figure out what IOKit wants and to try and find some examples.

From what I could gleam online, GamePad and MultiAxisController have to be added to Joystick.mm. They turn Joystick, Gamepad, and MAC into an array of some sort and then use that. But translating that to Dolphin is what I'm currently struggling to wrap my head around.

@ligfx
Copy link
Contributor

ligfx commented Aug 23, 2017

@khg8m3r It sounds like you're trying to add new if-cases for each usage page, and new classes. Can you try adding only this?

--- a/Source/Core/InputCommon/ControllerInterface/OSX/OSX.mm
+++ b/Source/Core/InputCommon/ControllerInterface/OSX/OSX.mm
@@ -167,7 +167,10 @@ static void DeviceMatchingCallback(void* inContext, IOReturn inResult, void* inS
   std::string name = GetDeviceRefName(inIOHIDDeviceRef);
 
   // Add a device if it's of a type we want
-  if (IOHIDDeviceConformsTo(inIOHIDDeviceRef, kHIDPage_GenericDesktop, kHIDUsage_GD_Joystick))
+  if (IOHIDDeviceConformsTo(inIOHIDDeviceRef, kHIDPage_GenericDesktop, kHIDUsage_GD_Joystick) ||
+      IOHIDDeviceConformsTo(inIOHIDDeviceRef, kHIDPage_GenericDesktop, kHIDUsage_GD_GamePad) ||
+      IOHIDDeviceConformsTo(inIOHIDDeviceRef, kHIDPage_GenericDesktop,
+                            kHIDUsage_GD_MultiAxisController))
   {
     g_controller_interface.AddDevice(std::make_shared<Joystick>(inIOHIDDeviceRef, name));
   }

That compiles and works fine on my machine.

@khg8m3r
Copy link
Contributor Author

khg8m3r commented Aug 23, 2017

@ligfx I'll double check when I get home, but you're suggesting that all three of them call the same interface? I don't have anything that shows up as GamePad or MAC, so even if it may compile I'm cant test if it actually will work.

@ligfx
Copy link
Contributor

ligfx commented Aug 24, 2017

Yep, exactly. You shouldn't need to worry about testing those, since this doesn't change how they're treated: currently, they're handled by the Joystick class along with every device that happens to be installed.

@khg8m3r
Copy link
Contributor Author

khg8m3r commented Aug 24, 2017

@ligfx Well it compiled fine and still let me use my Logitech F310, so hurray! Plus I installed some xbox 360 drivers and that works as well.

Copy link
Contributor

@ligfx ligfx left a comment

Choose a reason for hiding this comment

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

Cool. This whittles my device list down to the Quartz Keyboard & Mouse and whichever Xbox One controllers I have plugged in. @MayImilae how's your device list look?

@leoetlino leoetlino merged commit 8e9857d into dolphin-emu:master Sep 1, 2017
@khg8m3r khg8m3r deleted the OSXKeyboard branch September 2, 2017 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants