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

Use Sensor APIs when available instead of devicemotion. Fixes #10 #13

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

jsantell
Copy link
Contributor

Initial pass at using generic sensors API. Apologies for the extra noise in this diff -- the related areas are start(), onGyroscopeRead_(), onAccelerometerRead_() and onSensorError_().

Right now it works on Chrome M63 with flags enabled, but even though rendering at 60FPS, the actual pose orientation is very choppy, literally maybe 1-2 frames a second. Think it may have to do with the fusion and pose estimation, but not terribly familiar with that area.

Outstanding questions:

  • How will the iframe scenario work? Would it be up to developers to set the appropriate permissions on the iframes to allow sensor API to work? In this case, I think we are covered, as we would want it to fail and not fallback to device motion, I think, without appropriate permissions.
  • More of a generic webvr-polyfill question, but how can we alert a user that there's a failure and there's nothing we can do to support this device, after creating a VRDisplay? In some cases, it could be solved with permission changes.
  • A little stumped on the choppy fusion sensor, any ideas?

cc @cvan

@kenchris
Copy link

Wouldn't it make more sense to use the AbsoluteOrientationSensor (or Relative*) as then you don't need to do manual fusion as it happens on the sensor hub on most phones

@kenchris
Copy link

Also if you just want to do a complementary filter, that is quite simple with Generic Sensors, like

https://github.com/intel/websensor-compass/blob/master/scripts/compass.js#L301

@jsantell
Copy link
Contributor Author

Fixed the choppy issue so it works with the complimentary filter, as well as increased the frequency to 16.6 on both sensors; pushed changed.

@kenchris (cc @cvan) I haven't looked at OrientationSensors yet, doing so now! Would this make the complementary filter redundant? Ultimately we just want a quaternion on every frame, but not sure of the pros/cons of OrientationSensors vs complementary filters with Gyro/Accel. If a browser supports Gyro/Accel, will OrientationSensors also be supported, and vice versa, or will one be more ubiquitous?

@jsantell
Copy link
Contributor Author

From the RelativeOrientationSensor:

For the relative orientation sensor the value of latest reading["quaternion"] represents the rotation of a device hosting motion sensors in relation to a stationary reference coordinate system. The stationary reference coordinate system may drift due to the bias introduced by the gyroscope sensor, thus, the rotation value provided by the sensor, may drift over time.

That looks like it'd be great for our use case, but would using the complimentary filter snippet from #13 (comment) alleviate that drift?

@kenchris
Copy link

Most sensor hubs (as used by Orientation Sensors) use complementary filters or something similar as it is fast and easy to implement in hardware, and most likely attempt to remove drift, thought that can never fully be removed. Absolute Orientation Sensors can sometimes sync (ie. using the magnetic north then a quick movement is made).

I wrote an explainer about motion sensors here https://www.w3.org/TR/motion-sensors/

@kenchris
Copy link

The Magnetometer is running at low frequency (maybe @owencm remembers why) but it can still be used to measure drift over time (if calibrated properly)

@jsantell
Copy link
Contributor Author

@kenchris thanks for the info! I'll try a RelativeOrientationSensor patch and see how that performs

@kenchris
Copy link

OrientationSensors vs complementary filters with Gyro/Accel

Orientations sensors will use the sensor hub (hardware) if available or do the sensor fusion in code (Probably Java or C++ code on Android) which will presumable be a complementary with bios 98%. Sensor hubs most likely do the same, just in hardware or microcode

@kenchris
Copy link

You can play around with https://sensor-compass.appspot.com

The UI is not the best in the world though :-)

@jsantell
Copy link
Contributor Author

The RelativeOrientationSensor is working pretty well, some issues to resolve with syncing the coordinate system with WebVR's, but looking good 👍

@cvan cvan mentioned this pull request Jan 11, 2018
5 tasks
Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

First off, truly amazing work here. 💯 Thanks so much for digging into this and testing this out. This works far better than I originally expected.

NOTE: I had to proxy this through ngrok to get a secure context (i.e, the examples pages served over HTTPS) because of the Feature Policy restrictions.

(For local testing, I have a local branch that uses live-server + live-server-https, but I'll make a separate issue and PR for perhaps adding that, unless you have a better dev workflow.)

I tested this on my Samsung Galaxy S8, S6, and iPhone 7.

For Chrome for Android on Samsung S8/S6 phones:

  • WebVR ON, Generic Sensor ON — since using the native WebVR API, renders well
  • WebVR ON, Generic Sensor OFF — since using the native WebVR API, renders well
  • WebVR OFF, Generic Sensor ON — since using the native WebVR API, renders surprisingly very well (almost imperceptibly different from that of the Polyfill using devicemotion)
  • WebVR OFF, Generic Sensor OFF — renders well with devicemotion (not as well as with the native WebVR API enabled, but that's to be expected)

For iPhone 7:

  • WebVR OFF – as expected, the Polyfill behaviour seems unchanged, no regressions that I've noticed (I'm still in shock how buttery smooth this is)

For other phones:
I haven't tested on my Pixel, since I don't have that on my at the moment. I assume you tested this against a Pixel and/or Pixel 2. How's it feel?

this.resetQ.normalize();

// Take into account extra transformations in landscape mode.
if (Util.isLandscapeMode()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be triggered when/if orientationchange fires?

// origin IFrame. In this case, the polyfill can still work if the containing
// page sends synthetic devicemotion events. For an example of this, see
// the iframe example in the repo at `examples/iframe.html`
if (Util.isIOS() && Util.isInsideCrossOriginIFrame()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, this might change in Chrome for Android soon. I found this on the Chrome Platform Status.

There's also an issue filed in the spec about this: immersive-web/webxr#86

Now that we have both the Generic Sensor, Feature Policy APIs, and WebVR APIs in Chrome for Android, this helps a ton with the edge-casing and cross-browser support 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the iframe usage concerns me the most in these changes. But in the future, we can mostly offload the permissions onto developers using iframes since they'll be required to set feature policies and the APIs will either "just work" (or "not work"), correct?

//
// * Generic Sensor APIs do not exist, fallback to devicemotion
// * Underlying sensor does not exist, no fallback possible.
// * Feature policy failure (in an iframe); no fallback.
Copy link
Contributor

Choose a reason for hiding this comment

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

this.error = error;

if (error.name === 'SecurityError') {
console.error('Cannot construct sensors due to the feature policy');
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be thrown from the Permissions API too, btw

} catch (error) {
this.error = error;

if (error.name === 'SecurityError') {
Copy link
Contributor

Choose a reason for hiding this comment

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

there could be a NotAllowedError thrown too

Choose a reason for hiding this comment

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

If I remember correctly, NotAllowedError always reported asynchronously over 'error' event handler. Constructor only throws when sensor construction is denied due to feature policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like NotAllowedError only occurs in the error event when permission state is denied (or when it's subsequently denied)

this.sensors = { gyroscope, accelerometer };
this.onAccelerometerRead_ = this.onAccelerometerRead_.bind(this);
accelerometer.addEventListener('reading', this.onAccelerometerRead_);
accelerometer.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a try/catch block for NotReadableError and NotAllowedError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are also asynchronous, no? @alexshalamov

this.error = e.error;
if (e.error.name === 'NotAllowedError') {
console.error('Permission to access sensor was denied');
} else if (e.error.name === 'NotReadableError' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing space before closing paren

Copy link
Contributor

Choose a reason for hiding this comment

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

what about SecurityError? I don't think it could be a possible event based on reading the spec, but is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that it doesn't look possible in the spec, but maybe should display some message for other errors: else { console.error(e.error) } so we have some notification atleast

this.worldToScreenQ = new MathUtil.Quaternion();
this.originalPoseAdjustQ = new MathUtil.Quaternion();
this.originalPoseAdjustQ.setFromAxisAngle(new MathUtil.Vector3(0, 0, 1),
-window.orientation * Math.PI / 180);
Copy link
Contributor

Choose a reason for hiding this comment

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

add one more space to line up

@@ -85,7 +85,7 @@ CardboardVRDisplay.prototype = Object.create(VRDisplay.prototype);

Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to bump the dist files (npm run build) in this PR or separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that when bumping release!

// Frequency which the Sensors will attempt to fire their
// `reading` functions at. Use 16.6ms to achieve 60FPS, since
// we generally can't get higher without native VR hardware
const SENSOR_FREQUENCY = 16.6;
Copy link
Contributor

Choose a reason for hiding this comment

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

this frequency, in my testing, seems to be hit a real nice sweet spot on both my Samsung Galaxy S8 and S6. very impressive.

Copy link

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

@jsantell Great work!

Few thoughts about outstanding questions:

  • For iframes, soon, Chromium would deny cross-origin access to DevMotion / Orientation as well as for Generic Sensors (https://chromium-review.googlesource.com/c/chromium/src/+/850699). Web developers would need to explicitly grant feature access for cross origin iframes. Safari and Mozilla already forbids such usages.
  • I remember for DayDream, there was a special UI that is shown until user calibrates view and enables controller. Is there similar thing for cardboard-vr-display?
  • I don't know about historical reasons for doing manual fusion, but I would consider using/trying RelativeOrientationSensor (https://www.w3.org/TR/orientation-sensor/) that should perform better.

try {
accelerometer = new Accelerometer({ frequency: SENSOR_FREQUENCY });
gyroscope = new Gyroscope({ frequency: SENSOR_FREQUENCY });
accelerometer.addEventListener('error', this.onSensorError);

Choose a reason for hiding this comment

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

Is there a need to fuse data manually? Can RelativeOrientationSensor be used? Then, quaternion can be obtained without manual fusion. Only screen coordinate sync and other conversions would be needed in getOrientation.

Choose a reason for hiding this comment

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

That point was already made and he is looking into using that - check the PR comments

} catch (error) {
this.error = error;

if (error.name === 'SecurityError') {

Choose a reason for hiding this comment

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

If I remember correctly, NotAllowedError always reported asynchronously over 'error' event handler. Constructor only throws when sensor construction is denied due to feature policy.

@pozdnyakov
Copy link

some issues to resolve with syncing the coordinate system with WebVR's, but looking good 👍

@jsantell, we're currently working on solving syncing the coordinate system issue, pls let us know if you have any thoughts on ways to solve it (or maybe you already have a preferred API solution in your mind :) )

@jsantell
Copy link
Contributor Author

@cvan Thanks for the feedback and testing!

NOTE: I had to proxy this through ngrok to get a secure context (i.e, the examples pages served over HTTPS) because of the Feature Policy restrictions.

I was able to get around this using localhost (testing via adb on Pixel) FWIW.

@alexshalamov There is a fullscreen interstitial displayed when entering VR using the CardboardVRDisplay, prompting the user to rotate the screen, similar to Daydream. Regarding the sensor fusion, working on using the RelativeOrientationSensor as well with this! @pozdnyakov I'll ping if a good pattern surfaces on that transformation 👍

@kenchris
Copy link

I already do some transformation in the polyfill: https://github.com/kenchris/sensor-polyfills/blob/master/src/motion-sensors.js#L310

Basically what I believe makes the most sense to have by default because it makes things behave as most would expect.

// Quaternion to rotate from sensor coordinates to WebVR coordinates
const SENSOR_TO_VR = new Quaternion();
SENSOR_TO_VR.setFromAxisAngle(X_AXIS, -Math.PI / 2);
SENSOR_TO_VR.multiply(new Quaternion().setFromAxisAngle(Z_AXIS, Math.PI / 2));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to transform sensor coordinates to WebVR coordinates other than this?

}
}

_onSensorRead() {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to bind the reading event if we're just using WebVR's requestAnimationFrame (via getOrientation) for polling values?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but only if WebVR is not enabled (currently, if a valid Origin-Trial token is in the document response header or <meta> tag, or if the chrome://flags/#enable-webvr flag is enabled)

_onSensorRead() {}

_onOrientationChange() {
this._worldToScreenQ.setFromAxisAngle(new Vector3(0, 0, 1),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From @kenchris and sensor polyfill -- thanks!

@jsantell
Copy link
Contributor Author

Uploaded a new patch, using RelativeOrientationSensor -- seems more smooth than using fusion on Gyroscope/Accelerometer! The new PoseSensor class here uses the RelativeOrientationSensor, with falling back to the devicemotion FusionPoseSensor class giving us a nice separation of the two implementations. A bit rusty on some of these quaternion transforms, I'm sure there are some better ways here, any ideas? But works in all orientations with solid performance.

Feelin' good about this, ya'll -- thanks for all the feedback and help so far!

@jsantell
Copy link
Contributor Author

@cvan does AFrame have any explainers on feature policy changes that are incoming that'd affect developers using iframes? Would be a valuable collab (probably all of WebXR in general) to explain why iframe usage will break without using allow=. Could alert a more actionable message here when running into permission issues while being in an iframe that could help point devs to the right place

} else if (error.name === 'ReferenceError') {
// Fallback to devicemotion
this.api = 'devicemotion';
this.useDeviceMotion();

Choose a reason for hiding this comment

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

Will you attempt the polyfills for the fallback case? I would be interested in how well they work and whether things can be improved.

Copy link

@kenchris kenchris Jan 12, 2018

Choose a reason for hiding this comment

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

The polyfills are not doing manual fusion but also relying on the deviceorientation / deviceorientationabsolute (which on Android how shares the implementation with Generic Sensor API and most likely uses the sensor hub on iOS) events and should handle the differences between Android and iOS - at least @anssik and I tested that a lot manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit hesitant on changing the current sensor fusion code for devicemotion, there's a few edge cases in there (landscape-primary devices, browser-specific issues between Chrome vs Firefox) but would love to give it a go. Smoothing out the differences between Android & iOS is a huge win, but even on Android there are some differences between browsers. I'm also not 100% knowledgeable about the complexities in the current sensor fusion work here which also leads to some of my hesitation of changing existing behavior 😄

Choose a reason for hiding this comment

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

That is understandable, no rush. I would love to have those workarounds and issues fixed in the polyfill as well. I should spend some time on testing the polyfills with Firefox on Android. Do you know any other browser on Android which differs in behavior?

Well, if RelativeOrientationSensor works great, then in theory the polyfill should work the same way at least on Android and most likely on iOS (with the exception that is does the screen orientation adjustment at this moment, but I might turn that off depending on discussions).

For the rest of the browsers I cannot say anything at this point, until I get to test them, so will start with Firefox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's devicemotion's rotationRate on Chrome being different than Safari/iOS and Firefox/Android (radians vs degrees), I'll admit I'm less familiar with deviceorientation and not sure how it relates to devicemotion and if those differences across browsers affect both events. All the differences I'm aware of are just from me seeing them in the fusion-sensor-pose file and tracking down commit history of those little fixes.

The screen orientation adjustment in the sensor polyfill confused me a bit, because if I was using native RelativeOrientationSensor, I'd still have to take into account screen orientation (which the polyfill handles already) resulting in different results for quaternion in the native and polyfill scenarios, but maybe I'm misunderstanding!

Choose a reason for hiding this comment

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

I didn't run into the radians vs degrees issue with deviceorientation at least so either that is fixed or only applies to devicemotion. I tested on a 2017 model iPad and @anssik on an iPhone 8.

Yeah, sorry about confusing you. The polyfill is supposed to work just like the native implementation. So either we change the polyfill or we change the native implementation :-)

We still haven't decided on how to handle this, like do the remapping my default or add an option for that. I might turn that into an option in the polyfill for now that is polyfill specific until we reach agreement - Any input on this is of course welcome and I believe @pozdnyakov shared the link to the spec issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's great to hear deviceorientation doesn't suffer from some of the same issues as devicemotion when it comes to different platforms -- I need to do more research on these two events and how they relate (as well as browser support on older devices).

Ah I understand know with the polyfill vs native implementation on sensor orientations; I'll follow along in that issue on the spec.

Choose a reason for hiding this comment

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

I updated the polyfill now so that it now works exactly like the native implementation and moved the behavior to an option, ie

RelativeOrientationSensor({ frequency: 16.6, coordinateSystem: "screen" })

Where coordinateSystem defaults to "world"

Choose a reason for hiding this comment

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

frequency: 60 ? // it's in Hz in the native RelativeOrientationSensor implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pozdnyakov good catch, I was using seconds here (16.6) instead of hz -- will update

this.error = e.error;
if (e.error.name === 'NotAllowedError') {
console.error('Permission to access sensor was denied');
} else if (e.error.name === 'NotReadableError' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the trailing whitespace

}

_onSensorError(e) {
this.error = e.error;
Copy link
Contributor

Choose a reason for hiding this comment

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

since these are coming from events, IMO we ought to append to an array of errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking the same thing, but wondering how often multiple errors could occur; and this is mostly for future insight when debugging things in the wild. My only hesitation would be some sort of scenario where multiple errors can be triggered on every read resulting in a giant array, but even that's an already broken state. OK I'll update it 😸

} else if (screen.msOrientation) {
orientation = screen.msOrientation;
} else {
Object.defineProperty(orientation, "angle", {
Copy link
Contributor

Choose a reason for hiding this comment

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

use single-quotation marks

orientation = screen.msOrientation;
} else {
Object.defineProperty(orientation, "angle", {
get: () => { return (window.orientation || 0) }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a trailing ;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

// Fallback to devicemotion
this.api = 'devicemotion';
this.useDeviceMotion();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should still throw if the error name isn't handled by the above if statements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

if (sensor) {
this.sensor = sensor;
this.sensor.addEventListener('reading', this._onSensorRead);
this.sensor.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps you addressed this earlier and I missed it, but I think you need to try/catch this method call. it could throw NotReadableError or NotReadableError or NotAllowedError, per the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It runs notify error for those errors, which is event based, or am I misunderstanding?

Fire an event named "error" at sensor_instance using SensorErrorEvent with its error attribute initialized to error.

Either way, figuring out how to trigger real errors on this API would be a good test! (looking into it)

// Handle the yaw-only case.
if (this.config.YAW_ONLY) {
// Make a quaternion that only turns around the Y-axis.
out.x = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

to be consistent with the assignments above, feel free to change to out.x = out.z = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

return this._out;
}

_onSensorError(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: above, you have the this parameter named error. perhaps you could change this to event, for improved consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

if (e.error.name === 'NotAllowedError') {
console.error('Permission to access sensor was denied');
} else if (e.error.name === 'NotReadableError' ) {
console.error('Sensor not present');
Copy link
Contributor

Choose a reason for hiding this comment

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

Sensor not readable or Sensor could not be read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

console.error('Sensor not present');
} else {
console.error(e.error);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should return these errors

// First available in Chrome M63, this can fail for several reasons, and attempt
// to fallback to devicemotion. Failure scenarios include:
//
// * Generic Sensor APIs do not exist, fallback to devicemotion
Copy link
Contributor

Choose a reason for hiding this comment

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

end with a period for consistency

if (error.name === 'SecurityError') {
console.error('Cannot construct sensors due to the feature policy');
} else if (error.name === 'ReferenceError') {
// Fallback to devicemotion
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: end with a period for consistency.
nit: Fallback should be the verb Fall back.

this.error = error;

if (error.name === 'SecurityError') {
console.error('Cannot construct sensors due to the feature policy');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd capitalise Feature Policy; can't hurt IMO

// to fallback to devicemotion. Failure scenarios include:
//
// * Generic Sensor APIs do not exist, fallback to devicemotion
// * Underlying sensor does not exist, no fallback possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

use a ; instead of a ,

// First available in Chrome M63, this can fail for several reasons, and attempt
// to fallback to devicemotion. Failure scenarios include:
//
// * Generic Sensor APIs do not exist, fallback to devicemotion
Copy link
Contributor

Choose a reason for hiding this comment

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

use a ; instead of a ,

// * Generic Sensor APIs do not exist, fallback to devicemotion
// * Underlying sensor does not exist, no fallback possible.
// * Feature policy failure (in an iframe); no fallback.
// * Permission to sensor data denied; respect user agent, no fallback to devicemotion.
Copy link
Contributor

Choose a reason for hiding this comment

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

use a ; instead of a ,

// * Underlying sensor does not exist, no fallback possible.
// * Feature policy failure (in an iframe); no fallback.
// * Permission to sensor data denied; respect user agent, no fallback to devicemotion.
// Browsers are heading towards disabling devicemotion when sensors are denied as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's getting verbose (and fantastic work + patience, btw), but could we add relevant URLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the Chrome deprecating on-by-default permissions and Feature Policy in WebXR for iframes as more info, good call! As a follow up, I'll add more information to the README (linked from webvr-polyfill as well) as there's a bit more to handle in the iframe scenario on the developers end, and to indicate what underlying APIs are being used now

this.sensor = sensor;
this.sensor.addEventListener('reading', this._onSensorRead);
this.sensor.start();
this.api = 'sensor';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this to the top of this scope à la above and below

@jsantell
Copy link
Contributor Author

Repushed updates; thanks for review @cvan! Only outstanding thing is whether or not the errors from sensor.start() are try/catchable via notify error which:

Fire an event named "error" at sensor_instance using SensorErrorEvent with its error attribute initialized to error.

// integration is not available until Chrome M65, resulting in Sensors
// only being available in main frames.
// https://developers.google.com/web/updates/2017/09/sensors-for-the-web#feature_policy_integration
if (error.name === 'SecurityError') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvan just discovered that Sensors are only top level until Chrome M65 -- what do you think about enabling devicemotion in this case, at least for now? Otherwise this would break users on Chrome 63+, or Chrome 65+ without appropriate permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To try this out, I added permissions to the iframe example, which requires the devicemotion fallback on Chrome 63

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, good catch here

@@ -46,6 +46,7 @@
iframe.src = 'index.html';
iframe.width = '100%';
iframe.height = '100%';
iframe.setAttribute('allow', 'gyroscope,accelerometer');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the correct feature policy?

Choose a reason for hiding this comment

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

shouldn't it be "accelerometer; gyroscope" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's surprisingly difficult to find documentation on this :) having trouble remote debugging on Chrome m65 (where permissions should be enabled) so will use that to test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some testing, both , and ; seemed to work

@jsantell jsantell force-pushed the generic-sensors branch 2 times, most recently from d27fb16 to d510d07 Compare January 12, 2018 23:22
@jsantell
Copy link
Contributor Author

@cvan want to give this one last look? I think it's pretty solid, permissive fallback to devicemotion in iframes (due to Chrome m63 only supporting main frames) and the permission logic in general could use another safety check, I think

// https://developers.google.com/web/updates/2017/09/sensors-for-the-web#feature_policy_integration
if (error.name === 'SecurityError') {
console.error('Cannot construct sensors due to the Feature Policy');
console.warn('Attempting to fall back using "devicemotion", however this will ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

, -> ;

devicemotion-based FusionPoseSensor when not. Fixes #10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants