-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 support for Apple's GameController.framework as an input device #9745
Conversation
|
||
for (GCControllerButtonInput* item in controller.physicalInputProfile.allButtons) | ||
{ | ||
pairs.push_back(std::make_pair(std::string([item.aliases.allObjects[0] UTF8String]), item)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pairs.push_back(std::make_pair(std::string([item.aliases.allObjects[0] UTF8String]), item)); | |
pairs.emplace_back(std::string([item.aliases.allObjects[0] UTF8String], item); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is item.aliases.allObjects[0]
guaranteed to be valid?
Feels like it should probably be checked, unless it's a c array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://developer.apple.com/documentation/gamecontroller/gccontrollerelement, the aliases object is always present, but I will add a check if the array contains more than 0 entries
pairs.push_back(std::make_pair(std::string([item.aliases.allObjects[0] UTF8String]), item)); | ||
} | ||
|
||
sort(pairs.begin(), pairs.end(), [=](std::pair<std::string, GCControllerButtonInput*>& a, std::pair<std::string, GCControllerButtonInput*>& b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort(pairs.begin(), pairs.end(), [=](std::pair<std::string, GCControllerButtonInput*>& a, std::pair<std::string, GCControllerButtonInput*>& b) | |
std::sort(pairs.begin(), pairs.end(), [](const auto& lhs, const auto& rhs) |
AddInput(new Button(pairs[i].second)); | ||
} | ||
|
||
if (controller.motion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (controller.motion) | |
if (!controller.motion) | |
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference I guess, but I'd use m_controller
instead of controller
inside this function.
|
||
class Controller : public Core::Device | ||
{ | ||
private: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Private interface should be below the public interface.
class Motion : public Input | ||
{ | ||
public: | ||
enum button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum button | |
enum class ButtonAxis |
INFO_LOG_FMT(SERIALINTERFACE, "Controller '{}' connected with motion={}", vendorName, controller.motion.sensorsActive); | ||
NOTICE_LOG_FMT(SERIALINTERFACE, "Controller '{}' connected with motion={} and id=", vendorName, controller.motion.sensorsActive); | ||
|
||
std::shared_ptr<ciface::GameController::Controller> j=std::make_shared<Controller>(controller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just collapse into:
std::shared_ptr<ciface::GameController::Controller> j=std::make_shared<Controller>(controller); | |
g_controller_interface.AddDevice(std::make_shared<Controller>(controller)); |
{ | ||
controller.motion.sensorsActive = true; | ||
|
||
std::string vendorName = std::string([controller.vendorName UTF8String]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::string vendorName = std::string([controller.vendorName UTF8String]); | |
std::string vendor_name = std::string([controller.vendorName UTF8String]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly care but I think this string could be const
|
||
void Init(void* window) | ||
{ | ||
CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), s_observer, &onControllerConnect, CFSTR("GCControllerDidConnectNotification"), NULL, CFNotificationSuspensionBehaviorDeliverImmediately); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer using nullptr
over NULL. Ditto for other applicable cases elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this trigger the callback for devices that are connection on Init, or does it only call it for devices that will be connected after binding to it? I guess it's the second case, but please specify it in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also there is a space between the two functions here, but there isn't in DeInit()
|
||
namespace ciface::GameController | ||
{ | ||
using MathUtil::GRAVITY_ACCELERATION; // 9.80665 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this into the constructor, given it's only used in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not like GRAVITY_ACCELERATION is likely to change, but why copy its value in the comment?
// I was unable to use [GCController controllers] in Objective-C++ and therefore needed to access this class method using objc_msgSend instead | ||
typedef NSArray<GCController*>* (*send_type)(id, SEL); | ||
send_type func = (send_type)objc_msgSend; | ||
NSArray<GCController*>* controllers=(NSArray<GCController*>*)func(objc_getClass("GCController"), sel_getUid("controllers")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NSArray<GCController*>* controllers=(NSArray<GCController*>*)func(objc_getClass("GCController"), sel_getUid("controllers")); | |
auto* controllers = func(objc_getClass("GCController"), sel_getUid("controllers")); |
namespace ciface::GameController | ||
{ | ||
void Init(void* window); | ||
void PopulateDevices(void* window); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the window ptr is never used.
It would be better without it, especially after the changes I made to the controller interface in my pending PR:
#9702
} | ||
|
||
void DeInit() | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some safety measure to make sure DeInit doesn't break if called when Init hasn't been called? Or is that the case already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked this and the CFNotificationCenterRemoveObserver methods in DeInit() can already be called multiple times without any problems, so it looks like an explicit safety is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe specify that in a comment, it wouldn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does s_observer do? Is it assigned in Init? Maybe you could set it to null after DeInit?
Edit: nevermind.
|
||
std::string Controller::GetSource() const | ||
{ | ||
return "GCF"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constexpr somewhere
|
||
namespace ciface::GameController | ||
{ | ||
using MathUtil::GRAVITY_ACCELERATION; // 9.80665 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not like GRAVITY_ACCELERATION is likely to change, but why copy its value in the comment?
@@ -16,6 +16,7 @@ | |||
#include "InputCommon/ControllerInterface/Xlib/XInput2.h" | |||
#endif | |||
#ifdef CIFACE_USE_OSX | |||
#include "InputCommon/ControllerInterface/GameController/GameController.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR a Draft (work in progress)? Init/PopulateDevices/DeInit aren't plugged in in the ControllerInterface.
I don't understand how you even tested this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for the confusion. I own an M1 Mac and had to test/develop in skylersaleh's fork as I am unable to build Dolphin on the main branch.
I forgot to manually copy the other changes I made in ControllerInterface.cpp
|
||
namespace ciface::GameController | ||
{ | ||
static char *s_observer=""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why assigning a "" to a ptr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to ensure s_observer is an actual pointer to something. Otherwise the pointer will always be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand.
I've never allocated a ptr like that.
Does that create a new object and assign it to the pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand what this is, you should probably assign "dolphin" to that char array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this a constexpr or a const which you then pass as a reference to the callbacks bindings functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this does is to ensure that there is an empty string in the constant data segment of the binary and make the pointer point to it. If this is to be done, the variable should be marked const char*
instead of just char*
, because writing to such a string is not allowed. Or you can use Filoppi's suggestion of defining some simple variable (like an int
) and then taking a pointer to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked the manual (https://developer.apple.com/documentation/corefoundation/1543316-cfnotificationcenteraddobserver?language=objc) and it looks like I can completely remove the s_observer variable.
I will remove it and pass nullptr to the callback functions!
|
||
std::string vendorName = std::string([controller.vendorName UTF8String]); | ||
INFO_LOG_FMT(SERIALINTERFACE, "Controller '{}' connected with motion={}", vendorName, controller.motion.sensorsActive); | ||
NOTICE_LOG_FMT(SERIALINTERFACE, "Controller '{}' connected with motion={} and id=", vendorName, controller.motion.sensorsActive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is CONTROLLERINTERFACE now, I've recently added a new log type. Same for any other log you might have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ID for the notice log doesn't seem to be specified?
Do we really need two different logs of different verbosity?
|
||
g_controller_interface.RemoveDevice([&inController](const auto* obj) | ||
{ | ||
const Controller* controller = dynamic_cast<const Controller*>(obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought dynamic_cast was forbidden in dolphin, how does this even build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I borrowed this code from the already existing OSX.mm, so I guess it is not a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I know it's there as well. Not a problem to me, but on MSVC if you try to add a dynamic_cast it refused to build as it's been disabled.
|
||
sort(pairs.begin(), pairs.end(), [=](std::pair<std::string, GCControllerButtonInput*>& a, std::pair<std::string, GCControllerButtonInput*>& b) | ||
{ | ||
return a.first.compare(b.first)<0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return a.first.compare(b.first) < 0;
AddInput(new Button(pairs[i].second)); | ||
} | ||
|
||
if (controller.motion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference I guess, but I'd use m_controller
instead of controller
inside this function.
|
||
for (GCControllerButtonInput* item in controller.physicalInputProfile.allButtons) | ||
{ | ||
pairs.push_back(std::make_pair(std::string([item.aliases.allObjects[0] UTF8String]), item)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is item.aliases.allObjects[0]
guaranteed to be valid?
Feels like it should probably be checked, unless it's a c array.
AddInput(new Motion(controller, "Gyro Yaw Left", Motion::gyro_z, 1)); | ||
AddInput(new Motion(controller, "Gyro Yaw Right", Motion::gyro_z, -1)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space
|
||
void Init(void* window) | ||
{ | ||
CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), s_observer, &onControllerConnect, CFSTR("GCControllerDidConnectNotification"), NULL, CFNotificationSuspensionBehaviorDeliverImmediately); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also there is a space between the two functions here, but there isn't in DeInit()
// Licensed under GPLv2+ | ||
// Refer to the license.txt file included. | ||
|
||
#include <random> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be used?
|
||
void addController(GCController* controller) | ||
{ | ||
controller.motion.sensorsActive = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? Why is it not "deactivated" on disconnection? Is it relevant? Persistent?
|
||
std::string vendorName = std::string([controller.vendorName UTF8String]); | ||
INFO_LOG_FMT(SERIALINTERFACE, "Controller '{}' connected with motion={}", vendorName, controller.motion.sensorsActive); | ||
NOTICE_LOG_FMT(SERIALINTERFACE, "Controller '{}' connected with motion={} and id=", vendorName, controller.motion.sensorsActive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ID for the notice log doesn't seem to be specified?
Do we really need two different logs of different verbosity?
|
||
for (GCController* controller in controllers) | ||
{ | ||
addController(controller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove all devices which have the same source name as ours before re-adding them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Filoppi during my tests, it seems like this is working properly. When I click 'Refresh' in the Device popup screen the controllers are added properly again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work properly in the way the controller interface is currently setup, meaning that it cleans all devices before calling PopulateDevices() on each input backend, but it doesn't hurt to also remove your devices manually, and some input backends do. It would avoid duplicates if the controller interface design ever changed.
@dhannema Could you please rebase your PR to the latest master to pick up the changes to support M1 builds, so that the osx-universal buildbot will be able to run? |
{ | ||
static void AddController(GCController* controller) | ||
{ | ||
if (!@available(macOS 11.0, *)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't negate an @available
check. An alternative might be
if (@available(macOS 11.0, *))
{
}
else
{
INFO_LOG_FMT(...);
return;
}
But admittedly, it is a bit ugly.
This reverts commit c62029b.
I tested this on my M1 MacBook and it seems to work well, both when running native (arm) and under Rosetta2 |
Tested this on my m1 mac and a DualShock4, can confirm it works well |
Can confirm this works even with a DS5 (DualSense) controller! |
Protip: When making small changes, is always better to ammend the main commits than create a new one... Also hit the weak spot for massive damage! |
SDL should now provide accelerometer/gyroscope input. |
You are correct. We are 3 years later and indeed SDL also provides this functionality now. |
Add support for input devices to be detected from Apple's GameController.framework.
So far, I have only tested this with a DualShock 4 and an Xbox One S controller.
This will allow Mac users to directly use accelerometer and gyroscope motion input (if available) without need for external DSU applications:
![image](https://user-images.githubusercontent.com/13510885/119250672-f9cca800-bba1-11eb-8ee6-c4d21f043090.png)
And will also allow Mac users to use the touchpad of the DS4 controller:
![image](https://user-images.githubusercontent.com/13510885/119250680-07822d80-bba2-11eb-9f10-e959c96d853a.png)