Gamepad: Implement GamepadHapticActuator #32046
Merged
+652
−68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Also wondering if maybe @gterzian or someone has some insight, currently the subtests that test |
4 tasks
gterzian
requested changes
Apr 12, 2024
There was a problem hiding this comment.
Thanks for the start!
Bunch of small comments, plus a big one for the doing reset. I've also filed w3c/gamepad#200
| self.reset_result_promise.borrow().is_some() | ||
| } | ||
|
|
||
| pub fn resolve_reset_result_promise(&self) { |
There was a problem hiding this comment.
So to do this as per the spec we will have to restructure this via a listener on the IPC router thread and using trusted refs to the promises and also the actuator:
- Look at to setup a route over IPC, see for example
BroadcastListener, and do this insideReset. - Put the IPC sender in the
EmbedderMsg::StopGamepadHapticEffect - Put both the
playing_effect_promiseand thereset_result_promise, and also the actuator itself, as trusted on the "listener" in the route. - Add something in the reply about whether the effect was "successfully stopped"
- From the route, when you receive the reply, queue a task that runs all steps 4.3 and 4.4 of https://www.w3.org/TR/gamepad/#dom-gamepadhapticactuator-reset (the spec is wrong because it does 4.4. outside of a task).
- To do 4.3.1, I think you need to add a "playing effect generation counter", so that you can compare the one you stored on the IPC listener with the current one on the actuator by the time the task runs. Note this is necassary because it plays with the step 4 of
playEffect.
gterzian
reviewed
Apr 12, 2024
|
@msub2 Do you plan to continue working on this PR? |
|
Yes, there's a spec PR I've been waiting to merge for this before continuing but I do intend to finish it |
msub2
force-pushed
the
gamepad-haptics
branch
3 times, most recently
from
38cc0cb to
c42e092
Compare
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
Signed-off-by: Daniel Adams <[email protected]>
|
🔨 Triggering try run (#9973701020) for Linux WPT |
gterzian
reviewed
Jul 17, 2024
ports/servoshell/desktop/webview.rs
Outdated
| @@ -240,7 +245,10 @@ where | |||
| gamepad_event = Some(GamepadEvent::Disconnected(index)); | |||
| }, | |||
| EventType::ForceFeedbackEffectCompleted => { | |||
| gamepad_event = Some(GamepadEvent::HapticEffectCompleted(index)); | |||
| self.haptic_effects[&event.id.into()] | |||
There was a problem hiding this comment.
Should we not check for the presence of the effect? Is it possible for stop_haptic_effect to be called after EventType::ForceFeedbackEffectCompleted has already been enqueued but not handled yet(in which case the effect would have been removed already). Not sure what guarantees the stop calls give....
|
Test results for linux-wpt-layout-2020 from try job (#9973701020): Flaky unexpected result (15)
Stable unexpected results that are known to be intermittent (22)
|
|
✨ Try run (#9973701020) succeeded. |
…vent Signed-off-by: Daniel Adams <[email protected]>
gterzian
requested changes
Jul 18, 2024
|
|
||
| [GamepadHapticActuator interface: attribute effects] | ||
| expected: FAIL | ||
|
|
||
| [GamepadHapticActuator interface: operation playEffect(GamepadHapticEffectType, optional GamepadEffectParameters)] |
There was a problem hiding this comment.
I think we can fix these as well, as per my comments in zulip re setting up the promise.
|
I managed to address everything except checking for document visibility on playEffect and reset. It seems like the document visibility is hidden despite the page having loaded with visible content (I have a simple test page that gives me a readout of the gamepad state), so both promises are unexpectedly rejected. Should we try to track that down now or save it for a followup? |
Signed-off-by: Daniel Adams <[email protected]>
Let's note a follow-up in an issue. We can also open a separate issue about the weird IDL failure. |
gterzian
approved these changes
Jul 19, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: Daniel Adams <[email protected]>
Gae24
pushed a commit
to Gae24/servo
that referenced
this pull request
Jul 26, 2024
* Implement Servo side of GamepadHapticActuator Signed-off-by: Daniel Adams <[email protected]> * Get build working Signed-off-by: Daniel Adams <[email protected]> * Create effect handling on embedder side Signed-off-by: Daniel Adams <[email protected]> * Update tracing for GamepadHapticEffect Signed-off-by: Daniel Adams <[email protected]> * Update gilrs to point to commit with effect complete event Signed-off-by: Daniel Adams <[email protected]> * Implement playing and preempting haptic effects Signed-off-by: Daniel Adams <[email protected]> * Update IDL to add trigger rumble Signed-off-by: Daniel Adams <[email protected]> * Update WPT expectations Signed-off-by: Daniel Adams <[email protected]> * Handle stopping haptic effects from reset() Signed-off-by: Daniel Adams <[email protected]> * ./mach fmt, fix test-tidy issues Signed-off-by: Daniel Adams <[email protected]> * Add extra validity checks for trigger rumble Signed-off-by: Daniel Adams <[email protected]> * Retrieve supported haptic effects from embedder Signed-off-by: Daniel Adams <[email protected]> * Fix test expectations Signed-off-by: Daniel Adams <[email protected]> * Add missing spec link, pin gilrs commit Signed-off-by: Daniel Adams <[email protected]> * servoshell cargo formatting Signed-off-by: Daniel Adams <[email protected]> * Fix Cargo.toml Signed-off-by: Daniel Adams <[email protected]> * Additional comments, realm proof, naming Signed-off-by: Daniel Adams <[email protected]> * ./mach fmt Signed-off-by: Daniel Adams <[email protected]> * Update gilrs rev to gilrs-core 0.5.12 release Signed-off-by: Daniel Adams <[email protected]> * Implement sequence ids for gamepad haptic promises Signed-off-by: Daniel Adams <[email protected]> * Take playing effect promise instead of cloning Signed-off-by: Daniel Adams <[email protected]> * Implement listener for reset function Signed-off-by: Daniel Adams <[email protected]> * Fix Cargo.lock Signed-off-by: Daniel Adams <[email protected]> * Restructure IPC listeners, add comments, handle visibility change Signed-off-by: Daniel Adams <[email protected]> * Check that haptic effect still exists before handling ff completion event Signed-off-by: Daniel Adams <[email protected]> * Visibility steps, add InRealm bindings for promises Signed-off-by: Daniel Adams <[email protected]> * Add Gamepad EmbedderMsg arms to egl servo_glue Signed-off-by: Daniel Adams <[email protected]> --------- Signed-off-by: Daniel Adams <[email protected]>
atouchet
reviewed
Aug 19, 2024
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "3f226b8f4d9bc7da93de8efd8747c6b1086409ca3f4b6d51e9a7f5461a9183fe" | ||
| version = "0.10.6" | ||
| source = "git+https://gitlab.com/gilrs-project/gilrs?rev=eafb7f2ef488874188c5d75adce9aef486be9d4e#eafb7f2ef488874188c5d75adce9aef486be9d4e" |
There was a problem hiding this comment.
@msub2 Is this specific Git version necessary? Would it be fine to update gilrs to a newer published release?
There was a problem hiding this comment.
This specific revision is necessary because the currently published version on crates.io does not contain the changes I made to support events for force feedback effect completion. I've been meaning to ask the maintainer what's going on with that, as the last time I talked with them it sounded like it would've landed 2 or 3 releases ago
There was a problem hiding this comment.
Just left a message on the original gilrs PR, will see what they say
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses another item on #31362 by implementing GamepadHapticActuator, allow haptic effects to play on connected gamepads. Note that this currently implements the main spec version of GamepadHapticActuator and not what's present in the Gamepad Extensions.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors