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

Fix controller vibration in macOS and migrate to GameController API #80709

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

JezerM
Copy link
Contributor

@JezerM JezerM commented Aug 17, 2023

Relates to #14634

This PR migrates the ForceFeedback implementation in favor of GameController and CoreHaptics frameworks, in order to add/fix controller vibration in macOS.

The GameController implementation was essentially copied/pasted from the iOS one.

@JezerM JezerM requested a review from a team as a code owner August 17, 2023 10:47
@AThousandShips AThousandShips added this to the 4.x milestone Aug 17, 2023
@bruvzg bruvzg self-requested a review August 17, 2023 10:50
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
@JezerM
Copy link
Contributor Author

JezerM commented Aug 17, 2023

Oh, some of this stuff is only available for macOS 10.15+. So, this wouldn't work as Godot supports macOS 10.13+... I'll have to rethink this.

@Calinou
Copy link
Member

Calinou commented Aug 17, 2023

Oh, some of this stuff is only available for macOS 10.15+. So, this wouldn't work as Godot supports macOS 10.13+... I'll have to rethink this.

Can the macOS 10.15+ stuff be made optional? I don't think vibration support (or even controller support) is essential on very old macOS versions. macOS 10.15 is 4 years old at this point.

@JezerM
Copy link
Contributor Author

JezerM commented Aug 17, 2023

I will see if the macOS 10.15+ stuff can be made optional, I'm not sure though.

@JezerM JezerM requested a review from a team as a code owner August 17, 2023 19:26
@JezerM
Copy link
Contributor Author

JezerM commented Aug 17, 2023

Controller support is available in macOS 10.9+, and Haptics in macOS 11+. So, vibrations are the only optional stuff for macOS 11+.

@MaddTheSane
Copy link

MaddTheSane commented Sep 16, 2023

What I did for SDL2 was, on the IOKit side:

  1. When a controller is plugged-in, check if the OS was at least 11.
  2. If it is, call +[GCController supportsHIDDevice:] to check if the GameController supports it.
  3. If it does, ignore: the GameController observer will pick up the controller.
  4. Otherwise, init it as regular.

On the GameController side, I made it so that if the OS isn't 11 or higher, the init would just fail (thus making SDL no longer try to use the GameController calls).

The reason for checking for macOS 11 is because it added the +[GCController supportsHIDDevice:] class method, making it easier to avoid duplicated controllers.

Yes, the GameController stuff can be made optional: You can weak-link frameworks and libraries so the program won't crash at launch if it's missing frameworks or libraries (It will, however, crash if you call the weak-linked functions).

@Alex2782
Copy link
Contributor

Alex2782 commented Oct 7, 2023

I wanted to try this 'PR'. (XBox Controller on macOS)

Unfortunately compiling was not possible. My master branch is 2 days old (commit d31794c).
Godot v4.2.dev (d31794c4a) - macOS 13.6.0 - Vulkan (Forward+) - integrated Apple M1 - Apple M1 (8 Threads)

Does this error mean that a 'rebase' would be necessary later?

thirdparty/zlib/zconf.h:450:1: error: import of C++ module 'Darwin.POSIX.sys.types' appears within extern "C" language linkage specification [-Wmodule-import-in-extern-c]
#    include <sys/types.h>      /* for off_t */
^
./thirdparty/minizip/unzip.h:47:1: note: extern "C" language linkage specification begins here
extern "C" {
^

https://hardwaretester.com/gamepad
With Google Chrome all keys are recognized on this page, test 'vibration' also works.

i was first going to suggest taking some sources from the chromium project, then found this 'PR'.
https://chromium.googlesource.com/chromium/src/+/refs/tags/116.0.5845.239/device/gamepad

@MaddTheSane
Copy link

Does this error mean that a 'rebase' would be necessary later?

That looks like an error in Apple's new C module parser. Try disabling modules in Xcode.

@Alex2782
Copy link
Contributor

Alex2782 commented Oct 8, 2023

I use VSCode with scons
scons dev_build=yes vulkan_sdk_path=/Users/alex/Develop/VulkanSDK/1.3.250.1
I have tried this time first with clean (-c), without success.

I have tested an example for GameController API, XBox controller has vibrated 👍
fox2-swift minigame
https://developer.apple.com/documentation/gamecontroller/supporting_game_controllers

@Alex2782
Copy link
Contributor

Alex2782 commented Oct 8, 2023

That looks like an error in Apple's new C module parser. Try disabling modules in Xcode.

On my MacM1, master branch also has problems with: "-fmodules", "-fcxx-modules"
Must be due to my development environment. At github-check older tools and SDKs are used.

There is a configuration for 'clang' to disable this error: clang-docs --> -Wno-module-import-in-extern-c

platform/macos/SCsub

env.Append(CCFLAGS=["-fmodules", "-fcxx-modules", "-Wno-module-import-in-extern-c"])

  • master branch works after that
  • this PR unfortunately not
[ 96%] Linking Program bin/godot.macos.editor.dev.arm64 ...
[ 99%] progress_finish(["progress_finish"], [])
[100%] 0  0x103073648  __assert_rtn + 72
1  0x102f9bc5c  ld::Fixup::applyFixup(ld::Atom const*, ld::LayoutLinkedImage const&, unsigned char*) const + 8268
....
....
(ld: Assertion failed: (extras.otherInstrOffset != 0 && "Kind::arm64_adrp_ldr missing extra info"), 
function applyFixup, file Fixup.cpp, line 793.
clang: error: linker command failed with exit code 1 )

On 'stackoverflow' some say switch to 'XCode 14' for similar linker error.

clang -v

Apple clang version 15.0.0 (clang-1500.0.40.1)
Target: arm64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

/usr/bin/xcodebuild -version

Xcode 15.0
Build version 15A240d

@bruvzg
Copy link
Member

bruvzg commented Oct 8, 2023

(ld: Assertion failed: (extras.otherInstrOffset != 0 && "Kind::arm64_adrp_ldr missing extra info")

That's Xcode 15 new linker error (seems to be related to embree module), it should be fixed in master, branch probably need a rebase.

Relevant buildsystem fix:

if not vanilla and cc_version_major == 15 and cc_version_minor == 0:
env.Prepend(LINKFLAGS=["-ld_classic"])

@Alex2782
Copy link
Contributor

Alex2782 commented Oct 8, 2023

@bruvzg thx, 'joypads' demo starts now.

Vibration is executed. It detects 1 button less (menu button), I'll try to check it in more detail in the next days.

Update:
https://hardwaretester.com/gamepad
Google Chrome recognizes up to 5 buttons more than Godot.
(2 sticks press down, XBox button, Menu button, Share button.)

@JezerM JezerM force-pushed the migrate-game-controller-api branch from b7c9860 to 58122c2 Compare October 8, 2023 20:08
@JezerM
Copy link
Contributor Author

JezerM commented Oct 8, 2023

Rebased with master branch. -Wno-module-import-in-extern-c flag was also applied.

@Alex2782
Copy link
Contributor

Alex2782 commented Oct 8, 2023

buttonHome and buttonOptions

I would have to debug the 2 buttons first.

buttonHome = XBox-Button? link with GUIDE would also be my suggestion, I searched all files and find only under "iOS", how is it under Windows and Linux?

core_constants.cpp JoyButton
image

@Alex2782
Copy link
Contributor

Alex2782 commented Oct 9, 2023

I found that my old Xbox 360 controller is recognized under Google Chrome, without macOS driver!
(via Godot demo / GameController API unfortunately not possible)

If this 'PR' / 'GameController API' is done, then maybe later I'll check the Chrome sources (effort and compatibility)

https://hardwaretester.com/gamepad
image


(Copyright 2013-2023) Statistics from the last 10 years maybe, XBox 360 is still No.1 there by far
image

float value = gamepad.rightTrigger.value;
Input::get_singleton()->joy_axis(joy_id, JoyAxis::TRIGGER_RIGHT, value);
}

Copy link
Contributor

@Alex2782 Alex2782 Oct 9, 2023

Choose a reason for hiding this comment

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

I have created macos_xbox-branch on my account

These are all extensions: 70bb7a3

Note: leftThumbstickButton and rightThumbstickButton require macOS 10.14.1+
The two if-queries above would have to be removed again.

@Alex2782
Copy link
Contributor

Alex2782 commented Oct 9, 2023

Tests

XBox buttonShare linked with JoyButton::MISC1. Godot demo already shows the correct position.

image

Screen-2023-10-09-184918.mp4

@Sauermann
Copy link
Contributor

We discussed this in the PR-review meeting. It would need testing and could be considered for 4.3.

platform/macos/joypad_macos.h Outdated Show resolved Hide resolved
platform/macos/joypad_macos.h Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.h Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
platform/macos/joypad_macos.mm Outdated Show resolved Hide resolved
@bruvzg
Copy link
Member

bruvzg commented Feb 20, 2024

#88590 - a bunch of extra stuff for the future (touchpad + gyroscope support).

@JezerM
Copy link
Contributor Author

JezerM commented Feb 20, 2024

Suggestions committed.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Style and docs look good, save for one grammar detail I missed!

Please squash your commits into one, see here

doc/classes/Input.xml Outdated Show resolved Hide resolved
@JezerM JezerM force-pushed the migrate-game-controller-api branch from e9906f4 to ae77fa3 Compare February 20, 2024 12:03
@JezerM
Copy link
Contributor Author

JezerM commented Feb 20, 2024

Squashed commits~

@AThousandShips
Copy link
Member

Needs a rebase to fix CI

@JezerM JezerM force-pushed the migrate-game-controller-api branch from ae77fa3 to 24625b4 Compare February 20, 2024 12:45
@AThousandShips
Copy link
Member

Must be something else in this PR that triggers this then, as it doesn't occur without it

@bruvzg
Copy link
Member

bruvzg commented Feb 20, 2024

The error looks like something related to the #87999, no idea why it's triggering now. Really strange.

@bruvzg
Copy link
Member

bruvzg commented Feb 20, 2024

Might be triggered by env.Append(CCFLAGS=["-fmodules", "-fcxx-modules", "-Wno-module-import-in-extern-c"]), the rest should not have any effect.

@bruvzg

This comment was marked as outdated.

@bruvzg
Copy link
Member

bruvzg commented Feb 20, 2024

Or do the same trick as Display Server do and redefine conflicting Key (which is probably a better way to handle it):

diff --git a/platform/macos/SCsub b/platform/macos/SCsub
index ab6ca236ce..355772fcd2 100644
--- a/platform/macos/SCsub
+++ b/platform/macos/SCsub
@@ -121,8 +121,6 @@ files = [
     "gl_manager_macos_legacy.mm",
 ]
 
-env.Append(CCFLAGS=["-fmodules", "-fcxx-modules", "-Wno-module-import-in-extern-c"])
-
 prog = env.add_program("#bin/godot", files)
 
 if env["debug_symbols"] and env["separate_debug_symbols"]:
diff --git a/platform/macos/joypad_macos.h b/platform/macos/joypad_macos.h
index 01949a8f37..e27f51b890 100644
--- a/platform/macos/joypad_macos.h
+++ b/platform/macos/joypad_macos.h
@@ -29,8 +29,10 @@
 /**************************************************************************/
 
 #include "core/input/input.h"
+#define Key _QKey
 #import <CoreHaptics/CoreHaptics.h>
 #import <GameController/GameController.h>
+#undef Key
 
 @interface JoypadMacOSObserver : NSObject
 

This should fix a lot of issues regarding to old controller API, such as vibration

Haptics (vibrations) are only available in macOS 11+, so haptics are now
processed in macOS 11+ only. Also, this doesn't interfere with
controller's input as controller support is available in macOS 10.9+.

Added a Note for macOS regarding vibration support
@JezerM JezerM force-pushed the migrate-game-controller-api branch from 24625b4 to 07313a0 Compare February 20, 2024 14:13
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Just in case, re-tested it another time, everything seems to be ok.

@akien-mga akien-mga merged commit b787fc6 into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@nikoladigi
Copy link

This PR fixed my ongoing xbox controller problems(wrong button mapping, some buttons not detected at al). But now my Nvidia shield controller inputs are no longer detected at all in 4.3 dev4.

They are detected in 4.2.1 stable.

@akien-mga
Copy link
Member

@nikoladigi Could you open a new issue so we can properly track the regression?

@ashtonmeuser
Copy link

Is there any intention of backporting this to Godot 3.x?

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