playcanvas / engine

JavaScript game engine built on WebGL, WebGPU, WebXR and glTF
https://playcanvas.com
MIT License
9.59k stars 1.34k forks source link

WebXR Image detection method causing issues on image-tracking enabled browsers #5365

Closed benferns closed 1 year ago

benferns commented 1 year ago

Description

I’m developing a WebXR-compatible WebAR SDK ( https://launch.variant3d.com/ ) and have recently added image-tracking support in-line with Chrome Android’s implementation.

However I found that since PlayCanvas detects support via the existence of the XRImageTrackingResult class (regardless of whether that class is ever accessed in the scene's code), and also runs image-tracking setup regardless of whether its requested.

Android users who had my SDK (and hence XRImageTrackingResult class in code) on their page would get errors in this section of the engine engine/xr-image-tracking.js at cf28baf6f1ccd5568597946c62c1696a4ad9919e · playcanvas/engine · GitHub as it tried to start image-tracking despite it not being supported in that context (so no getTrackedImageScores).

Image tracking then runs every frame (calling getImageTrackingResults) , despite not being part of the project or requested in the requestSession() call.

Could I request a review of this approach, and suggest that the image-tracking setup code be gated based on the features enabled on the XRSession object, or wrapped in a try-catch?

Repro

I can't provide a reproduction for Variant Launch as the iOS app is not inspectable by end users, but if you visit this project on android or with the immersive simulator browser extension and hit "AR toggle" you will see the calls to image-tracking made & failing: https://playcanvas.com/editor/project/1082223 See BugRepro.js for the code where I'm stubbing out the image tracking methods.

On Launch the methods exist, but calling them is invalid as setup is not performed if the session doesn't request the feature, and has a performance impact due to how the native iOS/webview bridge has to work.

mvaligursky commented 1 year ago

@Maksims - would you have any comments in this?

benferns commented 1 year ago

btw this isn't critical for me as I'm patching around it in Launch (removing the classes on Android, returning early with hardcoded arrays on the getXXXX calls), but I thought it might not be intended behaviour or cause unexpected behaviour when image tracking becomes more widespread in other browsers.

Maksims commented 1 year ago

Do I understand correctly the issue, that cause of exception is that engine detects support of the feature by checking existence of a class. And in this case that class name is defined by Variant3D library, which is different from browser provided with real WebXR Image Tracking support?

benferns commented 1 year ago

Yes - thats the root cause with my SDK, but I thought other WebXR implementers might run into the same issue as usually you would expect to have the 'image-tracking' feature set on the requestSession() requiredFeatures if image tracking was going to be called, and isSessionSupported to be called for support checks (as on android with Launch that would return false, or possibly on other browsers where the support is hardware-configuration dependent - e.g. VR headsets with/without passthrough cameras).

Maksims commented 1 year ago

Most WebXR APIs designed in the way that when requesting feature, it has very little performance hit, unless the feature APIs are used. So to make it easier for developers to get working with APIs, early decision was to auto-request the feature. Also, we wanted to provide information if feature is supported before session request, hence the check for class existence. As there is no designed by WebXR way to do that.

If anyone would override the class, then I would expect it should mimic the functionality. Otherwise class should be called differently if it is not 1:1 implementation.

benferns commented 1 year ago

I wouldn't focus too much on my specific (mis)use of the class, its more that a single browser implementation (with the class) can exist on a collection of devices that do and don't have cameras (thus do and don't have support) and theres no implementation spec on the image-tracking code being called outside of a session where its requested. Also it may be a privacy complication if projects are requesting more permissions than necessary. Regardless, I'll close the issue as its solved for me now.