Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exception when trying to reference this inside callback (problem introduced by #5217) #5221

Merged
merged 1 commit into from
Jan 19, 2023

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. Fix exception when trying to reference this inside callback (problem introduced by #5217) 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants