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
Implement non-XR Gamepad discovery and input #31200
Conversation
Okay, it's not 100% perfect yet but I think ready for a more complete review now. Some caveats of the current implementation:
|
I've added spec comments where applicable. Here's the full flow for reference: EmbedderIn ConstellationConstellation receives these EmbedderEvents and handles them in Script threadConstellation sends the gamepad events to the script thread, where it then gets dispatched to the document via |
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.
Great start, thanks! In general, instead of adding comments for steps, I prefer to try to break things down into method mapping to a link in the spec(because the step numbers usually change). Which steps are taken inside the method is usually clear enough that no comment is necessary. Comments for TODO's are useful on the other hand.
This is more like a first pass, with some suggested structural changes.
ports/servoshell/app.rs
Outdated
@@ -437,6 +437,7 @@ impl App { | |||
} | |||
|
|||
// Catch some keyboard events, and push the rest onto the WebViewManager event queue. | |||
webviews.handle_gamepad_events(); |
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.
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've gone ahead and made this change
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.
Hey sorry for the late reply. Yeah I think it's better to match all event variant into same method.
Btw I have some thoughts about some input events like keyboard and mouse should be more direct.
Right now it has to be passed from embedder to constellation and the to script thread.
In general, it should be fine. But for gaming usage, it usually requires more precision checks. Even a few nanoseconds lost may cause the trigger to miss the previous tick frame.
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 thanks for the info! I think performance can be investigated in a follow-up. I agree there appears to be some redundant message passing.
components/script/dom/document.rs
Outdated
if let Some(gamepad) = gamepad_list.IndexedGetter(index.0 as u32) { | ||
// 1. Let now be the current high resolution time. | ||
let current_time = time::get_time(); | ||
let now = (current_time.sec * 1000 + current_time.nsec as i64 / 1000000) as f64; |
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.
we are trying to remove time
create, can you use std::time
?
cc @mrobinson
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, done.
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.
Thanks for the changes, it is much easier to follow-along now.
I have first a few more "structural" comments, and after they have been addressed I will review the details of the spec algorithms.
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.
Thanks for the work, looks like very good progress. Couple of changes on the detailed algorithms.
@wusyong Could you please take a look at the embedding layer?
We also need to start looking at WPT updates, see https://github.com/servo/servo/tree/main/tests/wpt#updating-test-expectations
In addition to the WPT changes there's one notable remaining issue that still needs fixing, which is that button inputs don't seem to register on their very first input, but respond without issue afterwards. Going to try and track that down later today |
Interesting. Is that after the connected event has been fired? I can imagine some input coming-in before that and being ignored. |
It is, that's what confuses me about it. Axes values seem to update immediately so I'll need to track down where it seems to be getting lost |
I've figured out the button issue! Turns out it stems from how I'm handling events from GilRs. There are ButtonPressed, ButtonReleased, and ButtonChanged events. I've been listening for ButtonChanged, but the very first time you press a button it only emits the ButtonPressed event, I assume because there's no prior state to identify a change with. Fixing this shouldn't be too difficult now, just need to add some checks on the embedder side |
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 to script all LGTM, would be good to:
- Update test expectations so that it can pass the merge queue.
- File an issue re TODO'(one issue can just give a sense of what needs to be addressed later).
The frozen array stuff can be done in a follow-up I would say(unless it makes new tests fails, but I don't think it does).
@wusyong All ok on the embedding side?
@sagudev I will be away next week, could you help to get this to merge?
- Small change but is more in line with spec
@sagudev Wu Yu Wei mentioned on Zulip already that it looks good |
I see. |
Some more passing test on legacy-layout:
|
I think this is wrong:
so we should get even more PASS on legacy layout. |
🔨 Triggering try run (#7940297235) for Linux WPT legacy-layout |
Test results for linux-wpt-layout-2013 from try job (#7940297235): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (16)
Stable unexpected results (1)
|
|
Implement discovery and input updating for non-XR controllers for use with the Gamepad API.
./mach build -d
does not report any errors./mach test-tidy
does not report any errors