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.

Sign up for GitHub

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

Jump to bottom

macOS: Bug fixes for game controllers #94580

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

macOS: Bug fixes for game controllers #94580

wants to merge 1 commit into from
+299 −501

Conversation

stuartcarnie
Copy link
Contributor

@stuartcarnie stuartcarnie commented Jul 21, 2024 •
edited
Loading

Game Controller support was refactored to resolve some bugs and such that it can be shared across macOS, iOS and tvOS. The code still lives in platform/macos until the team can advise where shared platform code like this might live.

Summary

  • Fixed bugs in haptic support, that was easy to break by sending rapid vibrations to the haptic engine
  • Simplified the connection / disconnection logic, and code for finding the next player index
  • Refactored to C++ classes rather than NSObjects to use Godot containers

stuartcarnie
stuartcarnie commented Jul 21, 2024
View reviewed changes
Copy link
Contributor Author

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Left some comments for reviewers.

platform/macos/joypad_macos.mm
print_verbose("Couldn't execute controller haptic pattern");
RumbleMotor(GCController *p_controller, GCHapticsLocality p_locality) {
engine = [p_controller.haptics createEngineWithLocality:p_locality];
engine.autoShutdownEnabled = YES;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haptics was buggy, as the previous implementation was continually stopping and starting the haptic engine. When playing haptics rapidly, requiring the existing player to stop, would ultimately cause the engine to enter an inconsistent state an haptics would stop working. This PR utilises the autoShutdownEnabled feature to let the OS handle it automatically.

platform/macos/joypad_macos.mm
Comment on lines +52 to +54
if ([p_controller.haptics.supportedLocalities containsObject:p_locality]) {
return memnew(RumbleMotor(p_controller, p_locality));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now only create the RumbleMotor if it is supported by the game controller

platform/macos/joypad_macos.mm
Comment on lines +159 to +162
gamepad.buttonA.pressedChangedHandler = BUTTON(JoyButton::A);
gamepad.buttonB.pressedChangedHandler = BUTTON(JoyButton::B);
gamepad.buttonX.pressedChangedHandler = BUTTON(JoyButton::X);
gamepad.buttonY.pressedChangedHandler = BUTTON(JoyButton::Y);
Copy link
Contributor Author

@stuartcarnie stuartcarnie Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Refactored to use the pressedChangedHandler callback API for button-like controls, so that we reduce the number of unnecessary events sent to the Godot input system.

Using a valueChangedHandler meant the buttons would receive continuous events as the pressure changed, whereas for most buttons, Godot only cares whether it is pressed or not.

platform/macos/joypad_macos.mm
Comment on lines -367 to -368
Joypad *joypad = [[Joypad alloc] init:controller];
[self.joypadsQueue addObject:joypad];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug, as it was adding a Joypad * to joypadsQueue, which was expecting a GCController *, per:

godot/platform/macos/joypad_macos.mm

Lines 255 to 257 in e25f3c0

for (GCController *controller in self.joypadsQueue) {
[self addMacOSJoypad:controller];
}

stuartcarnie
stuartcarnie commented Jul 21, 2024
View reviewed changes
platform/macos/joypad_macos.mm
}

- (NSArray<NSNumber *> *)getAllKeysForController:(GCController *)controller {
NSArray *keys = [self.connectedJoypads allKeys];
Copy link
Contributor Author

@stuartcarnie stuartcarnie Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Calling allKeys always results in a new allocation.

stuartcarnie
stuartcarnie commented Jul 21, 2024
View reviewed changes
platform/macos/joypad_macos.mm
Comment on lines -586 to -588
NSArray *keys = [observer.connectedJoypads allKeys];

for (NSNumber *key in keys) {
Copy link
Contributor Author

@stuartcarnie stuartcarnie Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Calling allKeys always returns a new NSArray, even if it is empty. This function is called from the main loop, resulting in allocations every frame.

@AThousandShips AThousandShips added bug platform:macos topic:porting topic:input cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 21, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 21, 2024
@stuartcarnie
Copy link
Contributor Author

@bruvzg perhaps we can merge your new features in #88590 with this version?

@stuartcarnie stuartcarnie marked this pull request as ready for review July 21, 2024 20:24
@stuartcarnie stuartcarnie requested a review from a team as a code owner July 21, 2024 20:24
@bruvzg
Copy link
Member

bruvzg commented Jul 21, 2024

perhaps we can merge your new features in #88590 with this version?

Probably, it makes sense to do it in one PR.

@stuartcarnie
Copy link
Contributor Author

perhaps we can merge your new features in #88590 with this version?

Probably, it makes sense to do it in one PR.

Sounds good – any ideas where the code might live, given we should be able to use this on macOS and iOS.

@bruvzg
Copy link
Member

bruvzg commented Jul 21, 2024

Sounds good – any ideas where the code might live, given we should be able to use this on macOS and iOS.

Probably in drivers, similarly to shared unix code.

@stuartcarnie
Copy link
Contributor Author

Probably in drivers, similarly to shared unix code.

Excellent – that was my thought too.

@stuartcarnie
Copy link
Contributor Author

Now what to name it – drivers/apple seems to be the most likely. We have some other shared Apple code for coremidi and coreaudio, so another possibility is drivers/gamecontroller, matching the framework name.

@akien-mga akien-mga changed the title macOS: bug fixes for game controllers macOS: Bug fixes for game controllers Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release platform:macos topic:input topic:porting
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
3 participants
@stuartcarnie @bruvzg @AThousandShips