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

[GameController] Correctly handle magnitude values passed to GamepadHapticActuator.playEffect() #8653

Merged
merged 1 commit into from
Jan 17, 2023
Merged

[GameController] Correctly handle magnitude values passed to GamepadHapticActuator.playEffect() #8653

merged 1 commit into from
Jan 17, 2023

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Jan 14, 2023 •
edited by webkit-early-warning-system

7ad0df2

[GameController] Correctly handle magnitude values passed to GamepadHapticActuator.playEffect()
https://bugs.webkit.org/show_bug.cgi?id=250616

Reviewed by Youenn Fablet.

Correctly handle magnitude values passed to GamepadHapticActuator.playEffect()
when passing them to the GameController framework.

We noticed different Gamepad vibration results between Safari and Chrome for
the same magnitude values. One obvious difference was that values below 0.1
didn't result in any vibration in Safari but did in Chrome.

In 258874@main, I made an attempt to address this by scaling the input
magnitude (in the range [0; 1]) to be in the range [0.1; 1] in order to
always get a vibration if the magnitude value is not zero.

However, I have since heard back from GameController developers who have
indicating that we should be using the square root of the magnitude,
instead of the raw magnitude value. This is due to the fact that the
GameController framework doesn't use the input value as-is internally.

I am therefore updating our code to use the suggested approach. I have
validated on XBox Cloud that the level of vibration feels similar between
Safari and Chrome.

* Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEffect.mm:
(WebCore::magnitudeToIntensity):

Canonical link: https://commits.webkit.org/258988@main

83d3fa9

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim
✅ 🛠 🧪 unsafe-merge

@cdumez cdumez self-assigned this Jan 14, 2023
@cdumez cdumez added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Jan 14, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 83d3fa9)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim
✅ 🛠 🧪 unsafe-merge

youennf
youennf approved these changes Jan 16, 2023
View reviewed changes
Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

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

LGTM.
By any chance, do you know what Chrome is doing on macOS?

@cdumez
Copy link
Contributor Author

cdumez commented Jan 16, 2023

LGTM.
By any chance, do you know what Chrome is doing on macOS?

They are using low level HID directly, not the GameController framework as far as I could tell.

@cdumez cdumez added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 17, 2023
@cdumez
[GameController] Correctly handle magnitude values passed to GamepadH…
7ad0df2 
…apticActuator.playEffect()

https://bugs.webkit.org/show_bug.cgi?id=250616

Reviewed by Youenn Fablet.

Correctly handle magnitude values passed to GamepadHapticActuator.playEffect()
when passing them to the GameController framework.

We noticed different Gamepad vibration results between Safari and Chrome for
the same magnitude values. One obvious difference was that values below 0.1
didn't result in any vibration in Safari but did in Chrome.

In 258874@main, I made an attempt to address this by scaling the input
magnitude (in the range [0; 1]) to be in the range [0.1; 1] in order to
always get a vibration if the magnitude value is not zero.

However, I have since heard back from GameController developers who have
indicating that we should be using the square root of the magnitude,
instead of the raw magnitude value. This is due to the fact that the
GameController framework doesn't use the input value as-is internally.

I am therefore updating our code to use the suggested approach. I have
validated on XBox Cloud that the level of vibration feels similar between
Safari and Chrome.

* Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEffect.mm:
(WebCore::magnitudeToIntensity):

Canonical link: https://commits.webkit.org/258988@main
@webkit-commit-queue
Copy link
Collaborator

Committed 258988@main (7ad0df2): https://commits.webkit.org/258988@main

Reviewed commits have been landed. Closing PR #8653 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 7ad0df2 into WebKit:main Jan 17, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youennf youennf youennf approved these changes

@achristensen07 achristensen07 Awaiting requested review from achristensen07

@beidson beidson Awaiting requested review from beidson

@geoffreygaren geoffreygaren Awaiting requested review from geoffreygaren

Assignees

@cdumez cdumez

Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
Milestone
No milestone
4 participants
@cdumez @webkit-early-warning-system @webkit-commit-queue @youennf