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

Cursor component: remove addWebXREventListeners/removeWebXREventListeners in update #5623

Closed
wants to merge 1 commit into from

Conversation

vincentfretin
Copy link
Contributor

Description:

Those calls were introduced in 655ab16
If you set cursor="rayOrigin: xrselect", it calls addWebXREventListeners in update and then in onEnterVR, so adding the listeners twice.
Later if you update a property that is not rayOrigin like fuse, it will call addWebXREventListeners and right away removeWebXREventListeners, so registering and unregistering listeners, it doesn't make sense.

Changes proposed:

  • Remove addWebXREventListeners/removeWebXREventListeners from update, only keep addWebXREventListeners in onEnterVR, and the removeWebXREventListeners in removeEventListeners that is called in pause/remove.

I tested that on Android examples/showcase/ui/index.html with <a-scene xr-mode-ui="XRMode: xr" and cursor="rayOrigin: xrselect; fuse: false". When I click on AR button, touching the image opens the modal correctly.
Am I missing something @mrxz?

@mrxz
Copy link
Contributor

mrxz commented Jan 8, 2025

The first guard should already prevent adding and removing the event listeners in the same update. Effectively there should only be two possible scenarios:

  • No active XRsession when cursor is created, update does not add WebXR event listeners (addWebXREventListeners is a no-op without xrSession), upon onEnterVR they are added
  • Entering VR session before creating cursor so not added in onEnterVR, upon creation they are added in update

The general idea behind the logic is to handle the cases a cursor is created or has its rayOrigin changed during an XR session. Instead of removing the logic, it should consider both xrselect and entity instead of just xrselect, similar to your fix in #5622.

@vincentfretin
Copy link
Contributor Author

Okay, it makes sense now. Thanks for the explanation. I'm closing it and I'll change the condition as part of #5622

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