Update `FusionPoseSensor` to use new Sensor APIs (instead of `devicemotion`)

cvan commented 6 years ago

The devicemotion API has been superseded by the Generic Sensor APIs.

It's shipped in Chrome for Android. I tested on a Samsung Galaxy S8. Load chrome://flags/#enable-generic-sensor, and toggle the Generic Sensor flag from Default to Enabled.

Here are some sample pages:

There is code in FusionPoseSensor that needs to use the new APIs when available (i.e., checking if ('Sensor' in window) …), and falling back to devicemotion only when necessary.

So instead of …

window.addEventListener('devicemotion', event => {
  console.log(event.accelerationIncludingGravity.x + ' m/s^2');
});

like this …

let sensor = new GravitySensor();
sensor.start();
sensor.addEventListener('reading', () => {
  console.log(sensor.x + ' m/s^2');
});

Examples:

jsantell commented 6 years ago

Thanks for this info! Any idea on how'd we handle if permissions aren't enabled for these if they exist, and to fallback to devicemotion?

cvan commented 6 years ago

In the Generic Sensor API spec, there's a section about Feature Detection:

Therefore, an effective strategy is to combine feature detection, which checks whether an API for the sought-after sensor actually exists, and defensive programming which includes:

checking for error thrown when instantiating a Sensor object,

listening to errors emitted by it,

handling all of the above graciously so that the user’s experience is enhanced by the possible usage of a sensor, not degraded by its absence.

Here are some variations of possible code paths that come to mind:

if ('permissions' in navigator) {
  Promise.all([
    navigator.permissions.query({
      name: 'accelerometer'
    }),
    navigator.permissions.query({
      name: 'gyroscope'
    })
  ]).then(results => {
    if (results[0].state === 'granted' && results[1].state === 'granted') {
      readFromSensors();
    } else {
      readFromDeviceMotion();  // NOTE: If the user explicitly revoked permissions for this sensor, we probably ought to not use `devicemotion` either (even if it's still available).
    }
  }).catch(err => {
    console.error('Encountered error:', err);
    readFromDeviceMotion();
  });
} else {
  readFromDeviceMotion();  // NOTE: If the user explicitly revoked permissions for this sensor, we probably ought to not use `devicemotion` either (even if it's still available).
}

Or just use the APIs directly and let the sensor throw a DOM SecurityError:

if (typeof window.Accelerometer === 'function' && typeof window.GravitySensor === 'function') {
  let sensorAcc = new GravitySensor();
  sensorAcc.start();
  sensorAcc.addEventListener('reading', () => {
    readFromSensors(sensorAcc);
  });
  sensorAcc.addEventListener('error', event => {
    if (event.error.name === 'SecurityError') {
      var msg = 'Permission was not granted to use the Accelerometer';
      console.error(msg);
      readFromDeviceMotion();  // NOTE: If the user explicitly revoked permissions for this sensor, we probably ought to not use `devicemotion` either (even if it's still available).
    }
  });
}

Or using a try/catch block:

var sensorAcc;
try {
  sensorAcc = new GravitySensor();
} catch (err) {
  readFromDeviceMotion();  // NOTE: If the user explicitly revoked permissions for this sensor, we probably ought to not use `devicemotion` either (even if it's still available).
}
if (sensorAcc) {
  sensorAcc.start();
  sensorAcc.addEventListener('reading', () => {
    readFromSensors(sensorAcc);
  });
  sensorAcc.addEventListener('error', event => {
    if (event.error.name === 'SecurityError') {
      var msg = 'Permission was not granted to use the Accelerometer';
      console.error(msg);
      readFromDeviceMotion();  // NOTE: If the user explicitly revoked permissions for this sensor, we probably ought to not use `devicemotion` either (even if it's still available).
    }
  });
} else {
  readFromDeviceMotion();
}

Hope that clears things up. I don't think it'll be too much additional code to support both while the APIs mature and ship. Let me know your thoughts.

jsantell commented 6 years ago

@cvan thanks for the background! Certainly enough information here for me to try this out

anssiko commented 6 years ago

Looping in Generic Sensor API spec editors @pozdnyakov @alexshalamov @rwaldron to check that the Feature Detection section is aligned with the implementation. Opened an upstream issue to track the spec update: https://github.com/w3c/sensors/issues/339

Also looping in @kenchris who created sensors-polyfill (Generic Sensor API polyfill) that has its own feature detection built in.

jsantell commented 6 years ago

Thank you @anssiko! Wasn't aware of the sensors-polyfill, that might be great to use if we can remove code from CardboardVRDisplay and shake out unused modules -- more to look into, thanks :smile:

kenchris commented 6 years ago

Please do @jsantell and file any issues you encounter and I will attempt to fix them

jsantell commented 6 years ago

After looking through the sensor polyfill, trying out some demos, and looking at results from browsers, some thoughts on uncertainties, questions, and notes:

When embedding a WebVR iframe, iOS requires postMessage from parent to iframe of message type 'devicemotion'. I think this will work if we just propagate this event and fire from the iframe on the window so that the polyfill picks it up.
It looks like this won't work on iframes at all, would this be circumvented by putting permissions on the iframe (via: <iframe src="https://third-party.com" allow="accelerometer; magnetometer; gyroscope"/>) when sensors are natively supported? Could this error also check the same permissions, even though it would still technically work if it did not throw (not too familiar with these APIs, but roughly window.parent != window.top && window.parent.getAttribute('allow').indexOf('gyroscope') == -1))? @kenchris if this sounds like something you would want the sensor polyfill to handle, let me know and I can work on a PR! Although checking for HTTP headers mentioned in the spec seems like it wouldn't even be possible to check.
Some browsers rotationRate values are in degrees (iOS, Firefox@Android, Edge@Windows Phone) and others radians, so that conversion will still be necessary.
I agree with what @cvan said, if permissions are denied for the Sensors, we should not fall back to devicemotion to respect the user agent's wishes. That being said, what would a valid experience be unless this is all done upfront?

alexshalamov commented 6 years ago

@cvan the snippet that you provided uses GravitySensor, I quickly checked FusionPoseSensor and it uses normal accelerometer data. Do you need GravitySensor somewhere? At the moment it is not implemented in Chromium, but it can be easily added if there is a need.

pozdnyakov commented 6 years ago

@cvan @jsantell please note, that the Generic Sensor API and the Device Orientation API implementation in Chrome soon will share the same feature policies (please see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/RX0GN4PyCF8).

The permissions for motion sensor classes (i.e. Accelerometer, Gyroscope, LinearAcceleration, AbsoluteOrientationSensor, RelativeOrientationSensor) in Chrome are always auto-granted, so that the user is not in the situation when a sensor class permission is denied however similar data still can be obtained from Device Motion API.

alexshalamov commented 6 years ago

@cvan @jsantell, for feature detection, I like try/catch version more, allows to catch cases when feature policy denies interface construction, or when interface is missing. What do you think?

@anssiko, once we agree on preferred solution, I will make PR for the spec and extend feature detection section.

let accelerometer = null;
try {
    accelerometer = new Accelerometer();
    accelerometer.addEventListener('error', event => {
        if (event.error.name === 'NotAllowedError') {
            console.log('Permission to access sensor was denied');
        } else if(event.error.name === 'NotReadableError' ) {
            console.log('Sensor not present');
        }
    });
} catch (error) {
    if (error.name === 'SecurityError') {
        console.log('Cannot construct sensor due to the feature policy');
    } else if (error.name === 'ReferenceError') {
        console.log('Interface is not present, falling back to DeviceMotion');
        readFromDeviceMotion();
    }
}

if (accelerometer) {
    accelerometer.addEventListener('reading', () => {
        readFromSensor(accelerometer);
    });
    accelerometer.start();
}

jsantell commented 6 years ago

@cvan the snippet that you provided uses GravitySensor, I quickly checked FusionPoseSensor and it uses normal accelerometer data. Do you need GravitySensor somewhere? At the moment it is not implemented in Chromium, but it can be easily added if there is a need. - @alexshalamov

This just needs Accelerometer and Gyroscope, I believe.

@cvan @jsantell please note, that the Generic Sensor API and the Device Orientation API implementation in Chrome soon will share the same feature policies (please see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/RX0GN4PyCF8). - @pozdnyakov

Good read, thanks for the insight!

@cvan @jsantell, for feature detection, I like try/catch version more, allows to catch cases when feature policy denies interface construction, or when interface is missing. What do you think? - @alexshalamov

Would this boilerplate code work with the sensor polyfill? It seems that the polyfill does not generate the same errors. I'm leaning towards using the sensor polyfill for here since we do need to rely on devicemotion fallback and could streamline some of the more complex code here, but looks like a solid snippet! Does listening to errors prevent the need for checking the navigator.permissions.query(...) queries?

kenchris commented 6 years ago

The polyfill might not do the error handling correct yet, but the intention is to do so if possible.

@alexshalamov @anssiko could you check this and file issues (or submit a PR with tests which should pass)

jsantell commented 6 years ago

@kenchris Great! Depending on timelines, I'll happily send PRs for any handling we implement around the polyfill here back upstream

cvan commented 6 years ago

@alexshalamov, @pozdnyakov: Thanks for the valuable insights and input.

@jsantell: Although it'd require less code, I'd recommend using the new Sensor APIs directly.

Chrome for Android M63+ has shipped an implementation (disabled by default, with an Origin Trial) of the Generic Sensor API, but TAG review (https://github.com/w3ctag/design-reviews/issues/207) feedback needs to be addressed.

And folks at Mozilla have expressed intent to unship the devicemotion (and deviceorientation) APIs in Firefox (Bugzilla bug). We don't have an implementation of the Generic Sensor APIs started yet, but I think it's fair to say that devicemotion will eventually be deprecated across all the browsers. I wrote a lengthy comment outlining a possible path forward for Firefox/Gecko.

To prepare for the eventual deprecation of devicemotion, what we can do today is rework the FusionPoseSensor code to support the new Accelerometer and Gyroscope when available, testing in Chrome for Android (M63+, with Origin Trial or chrome://flags/#enable-generic-sensor flag enabled).

Thanks again, everyone, for pitching in here.

jsantell commented 6 years ago

First attempt with outstanding issues: https://github.com/googlevr/cardboard-vr-display/pull/13

alexshalamov commented 6 years ago

@jsantell @cvan FYI, when we implemented Magnetometer sensor, we made a demo that uses cardboard's magnet button as an input. It is out of scope for this issue, yet, maybe you would need or consider using something like that in the future.

jsantell commented 6 years ago

Thanks for all the feedback everyone! @cvan @pozdnyakov @alexshalamov @kenchris

pozdnyakov commented 6 years ago

@cvan @jsantell FYI, screen coordinates synchronization for the Accelerometer, Gyroscope, LinearAcceleration, AbsoluteOrientationSensor and RelativeOrientationSensor classes is available now in Chrome canaries (starting from 66.0.3350.0).

In practice, it means that if you create a sensor object passing {referenceFrame:"screen"} to the constructor (e.g. new RelativeOrientationSensor({frequency: 60, referenceFrame:"screen"})) the screen coordinate system will be used to resolve the sensor readings. So, there will be no need to remap sensor readings on JS side when the screen orientation changes.

jsantell commented 6 years ago

@pozdnyakov great info, thanks! Looking through the spec and demos, is there anyway to distinguish the difference between Chrome m63-m65 implementations where referenceFrame is unsupported? Or to tell if referenceFrame will be respected?

pozdnyakov commented 6 years ago

@jsantell unfortunately, for m63-m65 UA sniffing needs to be done :( But we have just introduced a feature detection mechanism to the Generic Sensor specification, so that situations like this will be avoided in future. Now it is being implemented in Chrome.

jsantell commented 6 years ago

@pozdnyakov thanks for the info! Filed https://github.com/immersive-web/cardboard-vr-display/issues/17

immersive-web / cardboard-vr-display

Update `FusionPoseSensor` to use new Sensor APIs (instead of `devicemotion`) #10