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

Add support for XRInputSource and target ray spaces #23292

Merged
merged 11 commits into from May 8, 2019

Conversation

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Apr 30, 2019

Untested, but compiles.

r? @jdm or @asajeffrey


This change is Reviewable

@Manishearth Manishearth requested review from jdm and asajeffrey Apr 30, 2019
@highfive
Copy link

@highfive highfive commented Apr 30, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/XRSession.webidl, components/script/dom/vrdisplay.rs, components/script/dom/xrreferencespace.rs, components/script/dom/mod.rs, components/script/dom/xrstationaryreferencespace.rs and 5 more
  • @paulrouget: ports/glutin/Cargo.toml
  • @KiChjang: components/script/dom/webidls/XRSession.webidl, components/script/dom/vrdisplay.rs, components/script/dom/xrreferencespace.rs, components/script/dom/mod.rs, components/script/dom/xrstationaryreferencespace.rs and 5 more

@highfive
Copy link

@highfive highfive commented Apr 30, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 30, 2019

I'm getting a link error when compiling for android, but it bisects down to ee0bb1c , which makes no sense since that PR just introduces a new interface

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 1, 2019

The link error is:

> = note: /home/manishearth/mozilla/voser/android-toolchains/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../arm-linux-androideabi/bin/ld.gold: error: /home/manishearth/mozilla/voser/target/android/armv7-linux-androideabi/debug/deps/libscript-baf7c7e550275eab.rlib: bad extended name index at 8

@Manishearth Manishearth marked this pull request as ready for review May 4, 2019
@Manishearth Manishearth changed the title [WIP] Add support for XRInputSource and target ray spaces Add support for XRInputSource and target ray spaces May 4, 2019
@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 4, 2019

This works! I've got a test program at https://manishearth.net/sand/webgl-to-webvr/webxr-input.html. It doesn't show a ray but the triangle tracks the input device pretty well.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 4, 2019

Copy link
Member

@asajeffrey asajeffrey left a comment

Mostly looks good, my only real worry is that by making the gamepad state synchronous we'll end up introducing the same deadlock as for frames (which is why frames are async).

#[derive(Deserialize, Serialize)]
pub struct WebVRPoseInformation {
pub frame: WebVRFutureFrameData,
pub gamepads: Vec<(u32, WebVRGamepadState)>,
Copy link
Member

@asajeffrey asajeffrey May 6, 2019

Choose a reason for hiding this comment

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

nit: A comment about what that u32 is for would be nice.

Copy link
Member

@asajeffrey asajeffrey May 6, 2019

Choose a reason for hiding this comment

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

The frame is a asynchronous, but the gamepad state is synchronous, which seems odd, at least worth commenting on. IIRC the frame is async to deal with the case that the frame data is only available on the main thread, to avoid having the webgl thread block on the main thread, since that can cause deadlock. Are there platforms where the gamepad state is also only available on the main thread?

Copy link
Member Author

@Manishearth Manishearth May 6, 2019

Choose a reason for hiding this comment

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

Not that I know of.

Copy link
Member

@asajeffrey asajeffrey May 6, 2019

Choose a reason for hiding this comment

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

I think the magicleap assumes that gamepad state is queried on the main thread, but I'm not sure if this is a requirement, or just the way their example code is written.

components/webvr_traits/webvr_traits.rs Show resolved Hide resolved
components/webvr/webvr_thread.rs Show resolved Hide resolved
gamepads: vec![],
};
if get_gamepads {
let gamepads = unsafe { (*compositor.0).fetch_gamepads() };
Copy link
Member

@asajeffrey asajeffrey May 6, 2019

Choose a reason for hiding this comment

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

Can fetch_gamepads() block?

Copy link
Member Author

@Manishearth Manishearth May 6, 2019

Choose a reason for hiding this comment

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

Depends on the device impl, I guess, right now I don't think it does for googlevr.

})
.collect();
sender.send(Ok(data)).unwrap();
},
Copy link
Member

@asajeffrey asajeffrey May 6, 2019

Choose a reason for hiding this comment

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

Nit: can we not use the impl of collect() for iterators where Item=Result<T,E> to simplify this?

Copy link
Member Author

@Manishearth Manishearth May 6, 2019

Choose a reason for hiding this comment

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

Item isn't a result here, the entire list is. We could simplify this a bit with a .map, maybe

components/script/dom/vrdisplay.rs Show resolved Hide resolved
@@ -30,3 +30,9 @@ pub use rust_webvr_api::VRLayer as WebVRLayer;
pub use rust_webvr_api::VRMainThreadHeartbeat as WebVRMainThreadHeartbeat;
pub use rust_webvr_api::VRPose as WebVRPose;
pub use rust_webvr_api::VRStageParameters as WebVRStageParameters;

#[derive(Deserialize, Serialize)]
pub struct WebVRPoseInformation {
Copy link
Member

@asajeffrey asajeffrey May 6, 2019

Choose a reason for hiding this comment

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

bikeshed: not sure PoseInformation is the best name for data about the frame + gamepad state.

Copy link
Member Author

@Manishearth Manishearth May 6, 2019

Choose a reason for hiding this comment

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

Got a suggestion for a replacement?

Copy link
Member

@asajeffrey asajeffrey May 6, 2019

Choose a reason for hiding this comment

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

WebVRDeviceState?

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 6, 2019

Addressed. I haven't made the gamepad stuff async, perhaps that can be a followup?

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented May 6, 2019

OK, as long as we follow up on it. Tracking down deadlocks is not fun!

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 6, 2019

@bors-servo r=asajeffrey

@highfive highfive unassigned jdm May 6, 2019
bors-servo added a commit that referenced this issue May 7, 2019
Add support for XRInputSource and target ray spaces

Untested, but compiles.

r? @jdm or @asajeffrey

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23292)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented May 7, 2019

Testing commit f17f066 with merge a40445d...

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented May 7, 2019

💔 Test failed - linux-rel-wpt

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented May 7, 2019

  ▶ CRASH [expected OK] /offscreen-canvas/drawing-images-to-the-canvas/2d.drawImage.nonfinite.worker.html
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 18.3.0-devel
  │ 
  │ (servo:20290): GStreamer-WARNING **: External plugin loader failed. This most likely means that the plugin loader helper binary was not found or could not be run. You might need to set the GST_PLUGIN_SCANNER environment variable if your setup is unusual. This should normally not be required though.
  │ 0:00:00.015496348 20290 0x55df8801dc40 WARN      GST_PLUGIN_LOADING gstplugin.c:792:_priv_gst_plugin_load_file_for_registry: module_open failed: libmms.so.0: cannot open shared object file: No such file or directory
  │ 
  │ (servo:20290): GStreamer-WARNING **: Failed to load plugin '/home/servo/gst/lib/gstreamer-1.0/libgstmms.so': libmms.so.0: cannot open shared object file: No such file or directory
  │ 0:00:00.015670617 20290 0x55df8801dc40 WARN      GST_PLUGIN_LOADING gstplugin.c:792:_priv_gst_plugin_load_file_for_registry: module_open failed: libfaad.so.2: cannot open shared object file: No such file or directory
  │ 
  │ (servo:20290): GStreamer-WARNING **: Failed to load plugin '/home/servo/gst/lib/gstreamer-1.0/libgstfaad.so': libfaad.so.2: cannot open shared object file: No such file or directory
  │ CloneApi message response was dropped while Webrender was still alive: io error: All senders for this socket closed (thread main, at /home/servo/.cargo/git/checkouts/webrender-c3596abe1cf4f320/e53aae0/webrender_api/src/api.rs:981)
  │ stack backtrace:
  │    0:     0x55df8461e8bd - backtrace::backtrace::trace::h669a3feab6d0ae0a
  │    1:     0x55df8461d722 - <backtrace::capture::Backtrace as core::default::Default>::default::h071ee6441e7c008c
  │    2:     0x55df8136479f - servo::main::{{closure}}::he4675c5243648339
  │    3:     0x55df8480ae38 - rust_panic_with_hook
  │                         at src/libstd/panicking.rs:478
  │    4:     0x55df8480a8d1 - continue_panic_fmt
  │                         at src/libstd/panicking.rs:381
  │    5:     0x55df8480a81e - begin_panic_fmt
  │                         at src/libstd/panicking.rs:336
  │    6:     0x55df84672637 - webrender_api::api::RenderApiSender::create_api::h409b3a3ae3fb1c71
  │    7:     0x55df813b9dba - servo::create_constellation::h5457dca4f220b444
  │    8:     0x55df813467ab - servo::Servo<Window>::new::h44f71e5bb13ede20
  │    9:     0x55df8139dd6d - servo::app::App::run::hfb1bab59bf29cca0
  │   10:     0x55df813642fe - servo::main::h290b01630fe289fc
  │   11:     0x55df81351242 - std::rt::lang_start::{{closure}}::hbcb137af237c7b58
  │   12:     0x55df8480a752 - {{closure}}
  │                         at src/libstd/rt.rs:49
  │                          - do_call<closure,i32>
  │                         at src/libstd/panicking.rs:293
  │   13:     0x55df84815079 - __rust_maybe_catch_panic
  │                         at src/libpanic_unwind/lib.rs:85
  │   14:     0x55df8480b31c - try<i32,closure>
  │                         at src/libstd/panicking.rs:272
  │                          - catch_unwind<closure,i32>
  │                         at src/libstd/panic.rs:388
  │                          - lang_start_internal
  │                         at src/libstd/rt.rs:48
  │   15:     0x55df81364961 - main
  │   16:     0x7f1c8b60f82f - __libc_start_main
  │   17:     0x55df812f8ff8 - _start
  │   18:                0x0 - <unknown>
  └ called `Result::unwrap()` on an `Err` value: Io(Custom { kind: ConnectionReset, error: StringError("All senders for this socket closed") }) (thread StorageManager, at src/libcore/result.rs:999)

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented May 7, 2019

bors-servo added a commit that referenced this issue May 7, 2019
Add support for XRInputSource and target ray spaces

Untested, but compiles.

r? @jdm or @asajeffrey

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23292)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented May 7, 2019

Testing commit f17f066 with merge ea0bb8c...

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented May 7, 2019

💔 Test failed - mac-rel-wpt2

@jdm
Copy link
Member

@jdm jdm commented May 7, 2019

@bors-servo retry

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 7, 2019

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 7, 2019

Doesn't need p=1 anymore since the build results are long gone

@bors-servo p=0

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented May 7, 2019

Testing commit f17f066 with merge 670a32c...

bors-servo added a commit that referenced this issue May 7, 2019
Add support for XRInputSource and target ray spaces

Untested, but compiles.

r? @jdm or @asajeffrey

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23292)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented May 8, 2019

@bors-servo bors-servo merged commit f17f066 into servo:master May 8, 2019
3 of 4 checks passed
@bors-servo bors-servo mentioned this pull request May 8, 2019
@Manishearth Manishearth deleted the input branch May 8, 2019
@ferjm
Copy link
Member

@ferjm ferjm commented May 17, 2019

The link error is:

> = note: /home/manishearth/mozilla/voser/android-toolchains/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../arm-linux-androideabi/bin/ld.gold: error: /home/manishearth/mozilla/voser/target/android/armv7-linux-androideabi/debug/deps/libscript-baf7c7e550275eab.rlib: bad extended name index at 8

@Manishearth I am getting this link error locally. Did you manage to fix it?

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 17, 2019

Switching to normal ld may help. I think this occurs in debug but not release mode (or vice versa).

It's not our bug, it's caused by hitting a threshold of binary size on the linker, it seems. When I was bisecting this it bisected to a commit that added a blank interface, and the bisection kept changing as I rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants