Skip to content

Conversation

@diarmidmackenzie
Copy link
Contributor

@diarmidmackenzie diarmidmackenzie commented Jan 19, 2023

Description:

I recently proposed PR#5217 & it was merged. Unfortunately it triggers an exception on entry to WebXR, and also doesn't do what it was supposed to do.

This fixes that

Thanks to @mikemainguy for spotting this. He made a comment in #5217 yesterday, although I now can't find that comment, so maybe he since deleted it?

Changes proposed:

Originally submitted code was fatally flawed as it assumed that this was bound to the element within a callback.

I've corrected the code by using the self variable instead.

This is a complete screw-up by me. Sorry. Full disclosure on how this happened, so we (most of all me) can learn from this.

  • Prior to testing, I had built an external component that interacted with the WebXR API, and tested that extensively live, so I was confident I knew how the API worked.
  • Original code submitted for the PR was explicitly untested - intended for discussion about the feature
  • I then refactored the code, and did a full Unit Test of the code in the renderer component
  • I didn't UT the code in a-scene because the existing UTs in this area were limited, meaning it would have been a lot of work to extend them to cover this case. And since it was "only one line", I told myself the risk was low. If I had done a proper UT, I would have found the problem.
  • I also didn't test live at this point. If I had tested live and monitored the console output, I would also have found the problem.

Since master is broken right now (though the impact is just a console error, no other functional issues other than 5217 not actually working), I propose merging this fix now. I have tested this live on a Quest 2, including stepping through the code in the Chrome tools debugger.

Separately I will do a PR to extend coverage of the UTs for entering VR in WebXR mode (they currently focus on the legacy WebVR case).

@diarmidmackenzie diarmidmackenzie changed the title Fix exception when trying to reference this inside callback. Jan 19, 2023
@diarmidmackenzie diarmidmackenzie marked this pull request as ready for review January 19, 2023 09:54
@dmarcos
Copy link
Member

dmarcos commented Jan 19, 2023

Thanks, good catch! Sorry this bug slipped in.

@dmarcos dmarcos merged commit 9a651e8 into aframevr:master Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants