Add fetch_gamepads() support on VRDisplay #70

Manishearth · 2019-04-20T05:18:42Z

WebXR considers gamepads to be attached to the display. I could pass around the VR service on the Servo side, however the safety invariants seem super tricky and I'd rather not complicate that.

This also cleans up the code on backends which don't support multiple displays or gamepads.

Eventually VR displays should have a way to subscribe to individual gamepads so that sync_poses calls can get all the updates they need at once.

r? @MortimerGoro

…mepads on GoogleVRDisplay

…mepads on OculusVRDisplay

MortimerGoro · 2019-04-23T09:22:43Z

rust-webvr/src/api/googlevr/display.rs

@@ -26,6 +27,7 @@ const PREDICTION_OFFSET_NANOS: i64 = 50000000; // 50ms

 pub struct GoogleVRDisplay {
    service: *const GoogleVRService,
+    gamepad: Option<GoogleVRGamepadPtr>,


Lenovo Mireage uses googlevr API and has 2 controllers

Yeah but that's not how we use it right now -- I'd rather update the code to handle multiple controllers in a different issue.

asajeffrey

Mostly looks good, just some nits. You can r=me when you're happy with it.

asajeffrey · 2019-04-23T13:45:25Z

.travis.yml

+
+script:
+  - cargo check --target x86_64-pc-windows-gnu
+  - cd rust-webvr && cargo check --features "mock oculusvr googlevr" --no-default-features --target=armv7-linux-androideabi


Not obvious why these targets?

We cfg(Android) for these, they can't be tested normally

asajeffrey · 2019-04-23T13:46:52Z

rust-webvr-api/src/vr_display.rs

@@ -13,6 +13,9 @@ pub trait VRDisplay: Send + Sync {
    /// Returns the current display data.
    fn data(&self) -> VRDisplayData;

+    /// Returns gamepads attached to this display
+    fn fetch_gamepads(&mut self) -> Result<Vec<VRGamepadPtr>, String>;


Any chance of a spec link? (Probably not, but you never know.)

This isn't a specced method, I need it here to be able to safely fetch gamepads on the servo side without having to further complicate the unsafe code dealing with display pointers.

asajeffrey · 2019-04-23T13:49:00Z

rust-webvr/src/api/googlevr/display.rs

@@ -328,6 +335,9 @@ impl GoogleVRDisplay {
        let right_eye_vp = gvr::gvr_buffer_viewport_create(ctx);
        gvr::gvr_buffer_viewport_list_get_item(list, gvr::gvr_eye::GVR_RIGHT_EYE as usize, right_eye_vp);

+        let display_id = utils::new_id();
+        let gamepad = GoogleVRGamepad::new(ctx, controller_ctx, display_id).ok();


Log the error if this fails?

asajeffrey · 2019-04-23T13:50:19Z

rust-webvr/src/api/googlevr/display.rs

        }))
    }

+    pub fn gamepad(&self) -> Option<&GoogleVRGamepadPtr> {


Should this return a Option<GoogleVRGamepadPtr>?

I don't want to clone when it's not necessary, you don't need the new arc for poll_events()

asajeffrey · 2019-04-23T13:52:22Z

rust-webvr/src/api/googlevr/service.rs

@@ -17,8 +16,7 @@ const SERVICE_CLASS_NAME:&'static str = "com/rust/webvr/GVRService";
 pub struct GoogleVRService {
    ctx: *mut gvr::gvr_context,
    controller_ctx: *mut gvr::gvr_controller_context,
-    displays: Vec<GoogleVRDisplayPtr>,
-    gamepads: Vec<GoogleVRGamepadPtr>,
+    display: Option<GoogleVRDisplayPtr>,


This seems like an odd change, does Google VR really guarantee only one display?

From the API it seems so?

Either way, our API usage guarantees one display, so the question is moot for the purposes of this PR.

asajeffrey · 2019-04-23T13:54:49Z

rust-webvr/src/api/googlevr/service.rs

+        } else {
+            self.display = unsafe { Some(GoogleVRDisplay::new(self, self.ctx, self.controller_ctx)) };
+            Ok(self.display.as_ref().unwrap())
+        }


Can we use Option::get_or_insert_with?

Tried that, we borrow self here so we can't

asajeffrey · 2019-04-23T13:55:45Z

rust-webvr/src/api/oculusvr/service.rs

@@ -14,7 +14,7 @@ const SERVICE_CLASS_NAME:&'static str = "com/rust/webvr/OVRService";

 pub struct OculusVRService {
    initialized: bool,
-    displays: Vec<OculusVRDisplayPtr>,
+    display: Option<OculusVRDisplayPtr>,


Only one display?

API seems to think so, and we use it that way

asajeffrey · 2019-04-23T13:56:31Z

rust-webvr/src/api/oculusvr/service.rs

+            self.display = Some(OculusVRDisplay::new(self.service_java.clone(), self.ovr_java.handle()));
+            Ok(self.display.as_ref().unwrap())
+        }
+    }


get_or_insert_with?

Manishearth · 2019-04-23T14:47:18Z

Overall except for openvr all of our API usage assumes that there is a single display and a single gamepad, so I've fit the code to that assumption. If it is violated we already had problems, this PR isn't making them worse -- we can update our API usage to deal with this separately. I don't want this PR to be more than a refactoring and a change in the external API.

I don't think anything supports multiple displays, and WebXR seems to do away with the ability to do this anyway (webvr is being deprecated in favor of webxr). In webxr you can request one immersive session at a time, and infinite inline sessions (to support inline we need direct gyroscope data, not more displays).

So we may actually want to eventually make the external API reflect one-display-per-session since webxr can't make use of more displays anyway.

Manishearth · 2019-04-23T15:12:09Z

@bors-servo r=asajeffrey,mortimergoro

bors-servo · 2019-04-23T15:12:10Z

📌 Commit 7964f14 has been approved by asajeffrey,mortimergoro

bors-servo · 2019-04-23T15:12:16Z

⌛ Testing commit 7964f14 with merge 5fe1257...

@MortimerGoro

Add fetch_gamepads() support on VRDisplay WebXR considers gamepads to be attached to the display. I could pass around the VR service on the Servo side, however the safety invariants seem super tricky and I'd rather not complicate that. This also cleans up the code on backends which don't support multiple displays or gamepads. Eventually VR displays should have a way to subscribe to individual gamepads so that sync_poses calls can get all the updates they need at once. r? @MortimerGoro

Manishearth · 2019-04-23T16:28:16Z

hmm the build has succeeded but github hasn't gotten the notification yet

Manishearth · 2019-04-23T16:29:21Z

@bors-servo retry

@MortimerGoro

Add fetch_gamepads() support on VRDisplay WebXR considers gamepads to be attached to the display. I could pass around the VR service on the Servo side, however the safety invariants seem super tricky and I'd rather not complicate that. This also cleans up the code on backends which don't support multiple displays or gamepads. Eventually VR displays should have a way to subscribe to individual gamepads so that sync_poses calls can get all the updates they need at once. r? @MortimerGoro

bors-servo · 2019-04-23T16:29:26Z

⌛ Testing commit 7964f14 with merge bda7cd9...

bors-servo · 2019-04-23T16:33:29Z

☀️ Test successful - checks-travis
Approved by: asajeffrey,mortimergoro
Pushing bda7cd9 to master...

bors-servo · 2019-04-24T01:12:16Z

💥 Test timed out

Manishearth added 5 commits April 19, 2019 20:32

Add fetch_gamepads to VRDisplay

1549d9e

Clean up GoogleVR code to only support a single display, add fetch_ga…

9429332

…mepads on GoogleVRDisplay

Clean up OculusVR code to only support a single display, add fetch_ga…

bae9587

…mepads on OculusVRDisplay

Add fetch_gamepads to OpenVRDisplay

e84b4d2

Add fetch_gamepads to VRExternalDisplay

42c0ab2

Manishearth force-pushed the gamepads branch 2 times, most recently from a422a21 to d0ac41d Compare April 20, 2019 05:26

Manishearth added 2 commits April 19, 2019 22:33

Update travis to test more targets

c4c37cb

Don't build libloading when not on windows

c9ce954

Manishearth force-pushed the gamepads branch from d0ac41d to c9ce954 Compare April 20, 2019 05:35

Add fetch_gamepads to GlVrWindowDisplay

8f331b9

MortimerGoro approved these changes Apr 23, 2019

View reviewed changes

asajeffrey approved these changes Apr 23, 2019

View reviewed changes

log missing gamepad

7964f14

Manishearth closed this Apr 23, 2019

Manishearth reopened this Apr 23, 2019

bors-servo merged commit 7964f14 into master Apr 23, 2019

Manishearth deleted the gamepads branch April 23, 2019 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fetch_gamepads() support on VRDisplay #70

Add fetch_gamepads() support on VRDisplay #70

Manishearth commented Apr 20, 2019

MortimerGoro Apr 23, 2019

Manishearth Apr 23, 2019

asajeffrey left a comment

asajeffrey Apr 23, 2019

Manishearth Apr 23, 2019

asajeffrey Apr 23, 2019

Manishearth Apr 23, 2019

asajeffrey Apr 23, 2019

asajeffrey Apr 23, 2019

Manishearth Apr 23, 2019 •

edited

asajeffrey Apr 23, 2019

Manishearth Apr 23, 2019

asajeffrey Apr 23, 2019

Manishearth Apr 23, 2019

asajeffrey Apr 23, 2019

Manishearth Apr 23, 2019

asajeffrey Apr 23, 2019

Manishearth Apr 23, 2019

Manishearth commented Apr 23, 2019 •

edited

Manishearth commented Apr 23, 2019

bors-servo commented Apr 23, 2019

bors-servo commented Apr 23, 2019

Manishearth commented Apr 23, 2019

Manishearth commented Apr 23, 2019

bors-servo commented Apr 23, 2019

bors-servo commented Apr 23, 2019

bors-servo commented Apr 24, 2019

Add fetch_gamepads() support on VRDisplay #70

Add fetch_gamepads() support on VRDisplay #70

Conversation

Manishearth commented Apr 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Apr 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Apr 23, 2019 • edited

Manishearth commented Apr 23, 2019

bors-servo commented Apr 23, 2019

bors-servo commented Apr 23, 2019

Manishearth commented Apr 23, 2019

Manishearth commented Apr 23, 2019

bors-servo commented Apr 23, 2019

bors-servo commented Apr 23, 2019

bors-servo commented Apr 24, 2019

Manishearth Apr 23, 2019 •

edited

Manishearth commented Apr 23, 2019 •

edited