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

XR Cursor #5065

Merged
merged 2 commits into from
Jul 11, 2022
Merged

XR Cursor #5065

merged 2 commits into from
Jul 11, 2022

Conversation

AdaRoseCannon
Copy link
Contributor

Description:

Allows the cursor component to be used with generic XR inputs without needing to be specifically attached to anything.

In particular this lets it work screen based AR

Changes proposed:

  • Add an xrselect rayOrigin which sets the origin to the targetRaySpace where select events happen
  • Fix mouse constant fusing on mobile
  • built on top of degToRad fix in vendor #5064

Demo:
https://www.youtube.com/watch?v=j1SNRmsgfOo

src/components/cursor.js Outdated Show resolved Hide resolved

```html
<a-scene
cursor__mouse="rayOrigin: mouse"
Copy link
Member

@dmarcos dmarcos Jul 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple components for this might feel like an overkill. wondering if we can simplify somehow. maybe rayOrigin: mouse also considers selectstart? Not a fan either because makes rayOrigin: mouse a bit deceiving (not really only about mouse anymore)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of a few situations in which a developer may prefer to handle all of their own XR select behaviours but still take advantage of the mouse cursor.

My XR Input Handling library in particular makes it really simple to start building your x-platform own XR cursor using the entity method of the cursor component component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any other ideas to avoid the multiple component approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have rayOrigin be an array of strings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array would weird too I think. probably even weirder. Let's go with the multiple components for now but I don't feel super comfortable. Thanks for the patience

@dmarcos
Copy link
Member

dmarcos commented Jul 2, 2022

Thanks for the hard work and patience. Left a few comments questions. Let me know what you think

@AdaRoseCannon AdaRoseCannon force-pushed the ar-cursor branch 3 times, most recently from 4fd6d06 to 8c4fbd6 Compare July 7, 2022 14:37
@AdaRoseCannon
Copy link
Contributor Author

Here is a demo of the current behaviour https://a-cursor-test.glitch.me/

I haven't tested it in VR yet, it should work even if the controllers aren't visible

The user needs to have pressed the select button for mouseover to work

src/components/cursor.js Outdated Show resolved Hide resolved
// if something was tapped on don't do ar-hit-test things
if (
this.el.components.raycaster.intersectedEls.length &&
this.el.sceneEl.components['ar-hit-test'] !== undefined &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the component ar-hit-test be set on init if rayOrigin xrselect? For this to work it depends on the user to also set ar-hit-test separately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not so much a dependency just that things can get messy if both are being used so if the user is tapping on a 3d object then it shouldn't also interact with the real world behind it

src/components/cursor.js Outdated Show resolved Hide resolved
src/components/cursor.js Outdated Show resolved Hide resolved
@AdaRoseCannon
Copy link
Contributor Author

This now includes a fix for ar-hit-test to end any active hit testing if it's disabled mid hit test

if (this.data.enabled === false) {
this.hitTest = null;
this.bboxMesh.visible = false;
}
Copy link
Member

@dmarcos dmarcos Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to restore thee if the component is reenabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They get reassigned when an xr selectstart happens

the mesh is made visible
and the hit-test set to the one corresponding to that input

@AdaRoseCannon AdaRoseCannon changed the title AR Cursor XR Cursor Jul 11, 2022
@AdaRoseCannon
Copy link
Contributor Author

AdaRoseCannon commented Jul 11, 2022

I think this should only work when the XR Input's target ray mode NOT 'tracked-pointer' OR the Immersive Mode is AR

I think that if you need to add the extra elements to render controllers then you probably want to handle the cursoring yourself

@@ -250,6 +284,23 @@ module.exports.Component = registerComponent('cursor', {
evt.preventDefault();
}

if (this.data.rayOrigin === 'xrselect' && evt.type === 'selectstart') {
Copy link
Member

@dmarcos dmarcos Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this if be include in the if above to keep logic together

  if (evt.type === 'selectstart') {
     this.activeXRInput = evt.inputSource;
      this.onMouseMove(evt);
      this.el.components.raycaster.checkIntersections();
      ....
  } 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disregard. github was not displaying enough context. sorry

@dmarcos dmarcos merged commit fb9beae into aframevr:master Jul 11, 2022
@dmarcos
Copy link
Member

dmarcos commented Jul 11, 2022

Thanks for the patience. Awesome work!

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