search

immersive-web / webxr-polyfill

Use the WebXR Device API today, providing fallbacks to native WebVR 1.1 and Cardboard
Apache License 2.0
378 stars 84 forks source link

make XRWebGLLayer spec compliant #168

Closed AAwouters closed 5 months ago

AAwouters commented 1 year ago

Fixes #167

Relevant spec change

matthargett commented 1 year ago

Can you add a unit test?

AAwouters commented 1 year ago

Can you add a unit test?

I'll look into adding a test. However currently the tests won't run and it seems lots of imports are no longer correct. I could fix the tests and then add a unit test but polluting this PR (which has about half a line of changes) with lots of unrelated changes in the tests seems less than ideal.

ceyhun-o commented 5 months ago

Can this PR be reviewed? What is needed to merge this?

MattiasBuelens commented 5 months ago

@AAwouters worked on this during his internship at @THEOplayer. I'm happy to take over this PR if needed.

Change looks fine. It would be good to add a unit tests as previously requested.

We looked at the unit tests, but they are completely broken. They haven't been touched since 2018 in https://github.com/immersive-web/webxr-polyfill/commit/39289985c085133fbf729c3a16637f70e5c213c3, and I suspect no-one has tried running them since then. I tried running npm test locally, but I didn't get very far:

I don't currently have the bandwidth to fix all of these tests.

toji commented 5 months ago

sigh, yeah. This repo is pretty neglected these days, and you certainly shouldn't be held accountable for picking up after bit-rotted tests.

Because the scope of this is pretty small, I'm just going to land it.