Added support for required and optional features

[toji]

Brings us closer to spec compliance and ensures that developers that start developing with the polyfill won't be blindsided by this requirement when they test on a natively supported device. (Previously the requirements would be accepted but naturally ignored, which had the effect of allowing already valid code to work but not properly catching invalid code.)

[takahirox]

Awesome! This is what I'm about to send the feature request.

One thing about the expansion of valid future descriptions.

Future iterations of this specification and additional modules may expand the list of accepted feature descriptors.

https://immersive-web.github.io/webxr/#feature-dependencies

I'm happy if we have a way to expand the valid feature descriptors.

For example in the current geo-alignment spec proposes user requests 'geo-alignment' via requiredFeatures (or optionalFeatures) of requestSession().

https://github.com/immersive-web/geo-alignment/blob/master/explainer.md

If I want to test it based on webxr-polyfill (perhaps by extending the polyfill to support geo-alignment) it ends up failing because of 'geo-alignment' is recognized as invalid descriptor.

Maybe exposing isValidFeatureDescriptor() or defining and exposing validDescriptors array would be good?

[toji]

If I want to test it based on webxr-polyfill (perhaps by extending the polyfill to support geo-alignment) it ends up failing because of 'geo-alignment' is recognized as invalid descriptor.

The spec was very carefully set up to allow exactly this sort of thing. If you pass geo-alignment into the optionalFeatures array and the implementation (like the Polyfill in this case) doesn't recognize it, it will simply ignore the value and move on. You'll still get a valid session, it just won't incorporate that feature. This is to allow for implementation differences and migration periods where a feature may not be globally recognized.

In contrast, if you pass an unknown value to the requiredFeatures array then the session creation will fail. After all, you told the implementation that you require it and the implementation doesn't know what it is, so obviously it's unable to meet your requirements. In that case requestSession will reject with a NotSupportedError before actually doing any mode switching or lighting up the headset or anything like that, so you can communicate to your users that they're missing a required feature.

[takahirox]

Thanks for the comment. Yeah I knew the behavior of required/optionalFeatures. Sorry I think I should've been more specific.

What I want to do is experimenting WIP AR-related APIs geo-alignment and others with the polyfill by defining custom device and extending the polyfill.

// custom device handling geo-alignment
class CustomDevice extends XRDevice {
  isFeatureSupported(featureDescriptor) {
    switch(featureDescriptor) {
      case 'geo-alignment':
        return true;
      default:
        return super.isFeatureSupported(featureDescriptor);
    }
  }
  requestSession(mode, enabledFeatures) {
    if (enabledFeatures.includes('geo-alignment')) {

      // geo-alignment specifig logics

    }
  }
  ...
}

// hack XR* for geo-alignment API
XRFrame.prototype.fooGeoAlignment = function () {
  // working with custom device
};

// user code
navigator.xr.requestSession('immersive-ar', {
  requiredFeatures: ['geo-alignment']
}).then(xrSession => {
  session = xrSession;
  ...
});

...

const animate = (timestamp, frame) => {
  session.requestAnimationFrame(animate);
  const alignment = frame.fooGeoAlignment();
  ...
}

The problem is geo-alignment is recognized as invalid desctiptor in XR.requestSession() and failing to create XRSession if geo-alignment is in requiredFeatures or geo-alignment descriptor isn't passed to CustomDevice.requestSession() even if it is in optionalFeatures.

One solution I have is overriding XR.requestSession() to accept geo-alignment descriptor. But it's easier if we have a way to expand the valid feature descriptors.

Or what I want to do is out of scope of the webxr-polyfill?

[toji]

Ah, I see. So in that case we make a distinction between the API implementation being aware of a feature and a specific device supporting it. If the implementation (in this case the polyfill) is updated to be aware of a feature it should be added to isValidFeatureDescriptor() for the whole thing. (It's really just a pre-filter to ensure that the XRDevice code doesn't get wildly invalid values.) Each individual device implementation (such as your hypothetical AR backend) would then have to explicitly indicate if it supports the new feature or not. That roughly parallels what we would expect a real browser implementation to do as well.

[takahirox]

Yeah, that sounds nice.

So, the problem I'm facing in this PR implementation is if I want let the device implementation support the new feature which the API (polyfill) isn't aware of, the feature descriptor is filtered by the API and then failing to create session or ignoring the descriptor.

for (let feature of requestedOptions.requiredFeatures) {
  if (!isValidFeatureDescriptor(feature)) {
    console.error(`The required feature '${feature}' is not a valid feature descriptor`);
    requirementsFailed = true;
  } else if (!this[PRIVATE].device.isFeatureSupported(feature)) {
    console.error(`The required feature '${feature}' is not supported`);
    requirementsFailed = true;
  } else {
    enabledFeatures.add(feature);
  }
}

I hope such new feature descriptors can pass the filter if the device implementation explicitly indicates supporting them. It may fit to

additional modules may expand the list of accepted feature descriptors.

https://immersive-web.github.io/webxr/#feature-dependencies

[jsantell]

IIUC, outside of inline (maybe that too), I think all of the other modes (immersive-ar, immersive-vr) need to be handled by the device (WebVRDevice in this case, or a custom WebARDevice, for example), and the strictness in the API (./src/api/XR.js) probably needs to be relaxed to accommodate custom devices for extensibility. We had to do similar for Google's Web AR platform, and lax the restrictions on the API side of things so that the device could experiment and create their own options.

[toji]

Lost track of this one for a bit, sorry!

Updated to only do the device-level feature check, removing the top-level "valid feature descriptor" check altogether. Thanks for the feedback!

[joshmarinacci]

Any progress? We are only a bit over a month from release date.

[toji]

I was waiting to hear from @jsantell, but given that it's been nearly a week and the primary concern that was brought up by reviewers has been addressed, I'm tempted to simply merge. I'll give it till EOD.

[toji]

Thanks Jordan! Updated to address your concerns, so I'm gonna merge now.