-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add a mouse based cursor / raycaster and applies to the link traversal example #2861
Conversation
a8edde4
to
cb821a5
Compare
9e348bb
to
18c14da
Compare
docs/components/cursor.md
Outdated
@@ -80,6 +80,7 @@ AFRAME.registerComponent('cursor-listener', { | |||
| downEvents | Array of additional events on the entity to *listen* to for triggering `mousedown` (e.g., `triggerdown` for vive-controls). | [] | | |||
| fuse | Whether cursor is fuse-based. | false on desktop, true on mobile | | |||
| fuseTimeout | How long to wait (in milliseconds) before triggering a fuse-based click event. | 1500 | | |||
| mode | Camera (gaze) or mouse controlled. | gaze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just make it a boolean based for mouse. mouseMode: true|false
. The cursor can also be used for controllers as well, not just for gaze.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but you also might want mouse and not gaze. Also the raycaster operates differently for each mode: raycaster from the entity or from the mouse cursor on the screen. If you want gaze + mouse you can add two cursors to the same or different entities. In the link traversal examples the cursor is added to the a-scene
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know if my comment was understood. mode: gaze
doesn't really do anything (and the non-mouse cursor is not just for gaze). What we want to represent is either the cursor has mouse capabilities or it doesn't, represented as a boolean mouseMode: true
or mouseMode: false
. This would be consistent to how it's represented in the code as well. Also better not to use strings when possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that gaze and mouse are mutually exclusive because the raycaster configuration is different (hence the mode property). A cursor as it is, cannot operate in both gaze and mouse modes. A mouseMode
flag is confusing because it conveys an additional functionality on top of the normal behavior that can be toggled. We could make the raycaster
a multiple component and the cursor component could add two of them: One for gaze and the other for mouse. This might introduce confusion since configuring the raycaster by the user would not be as simple as:
el.setAttribute('raycaster', 'objects', '.clickable')
Two different raycasters would have to be configured instead:
el.setAttribute('raycaster__gaze', 'objects', '.clickable');
el.setAttribute('raycaster__mouse', 'objects', '.clickable');
An alternative would be to add an objects
property to the cursor
component so the user does not configure the raycaster anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A boolean mouseMode
property properly represents the mutual exclusiveness, that the entire component is flipped to a mouse mode or it's not. "Modes" usually mean an entire configuration, not "additional functionality".
"Gaze mode" does not make sense as the cursor can be used with controllers as well, and in the component code, "gaze mode" doesn't trigger any logic. It's only when setting to mouse mode does any different code run.
How I could see this component being used (although I'm not too amped on including a mouse mode in the first place):
<a-scene>
<a-mixin id="raycaster" raycaster="objects: .clickable"></a-mixin>
<a-entity mixin="raycaster" cursor="mouseMode: true"></a-entity>
<a-camera>
<a-entity mixin="raycaster" cursor></a-entity>
</a-camera>
<a-entity mixin="raycaster" laser-controls</a-entity>
</a-scene>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the mode
sort of makes sense, but I don't really like having to provide strings, that gaze
isn't the only other way to use the cursor, and that gaze
is not represented in the code as doing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we have different expectations on what a flag means. When seeing mouseMode: true
enabled many people will interpret it as I can use the mouse cursor as well to select things
in addition to the normal behavior. A mode
word indicates... modality 😄 We can replace the mode names mouse
\ gaze
with screen
\ entity
to indicate where the cursor performs the raycasting from. A cleaner solution is probably adding two raycasters, an objects property and a mouseMode
flag (false by default) so the modes are not mutually exclusive. The API would be then:
<a-entity cursor="mouseMode: true; objects: .clickable">
The cursor would operate in both modes simultaneously and use two raycasters: raycaster__mouse
and raycaster__entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, yeah that makes sense as long as it's not named gaze
. I need to think about operating in both simultaneously, it would make it less work for developers and possibly more performant (one cursor vs. two cursors), although it's more magic and complex.
I replaced |
So not going to try the idea of having a cursor work with both simultaneously? |
It needs more thought and probably not a priority right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc
src/components/raycaster.js
Outdated
@@ -27,7 +27,8 @@ module.exports.Component = registerComponent('raycaster', { | |||
objects: {default: ''}, | |||
origin: {type: 'vec3'}, | |||
recursive: {default: true}, | |||
showLine: {default: false} | |||
showLine: {default: false}, | |||
worldCoordinates: {default: false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd name this isWorldSpace
(or areWorldCoordinates
) to not sound like it takes coordinates.
src/components/cursor.js
Outdated
var origin = new THREE.Vector3(); | ||
var direction = new THREE.Vector3(); | ||
return function (evt) { | ||
var camera = this.el.sceneEl.camera; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a test for this block
src/components/cursor.js
Outdated
mouse.y = -(evt.clientY / window.innerHeight) * 2 + 1; | ||
origin.setFromMatrixPosition(camera.matrixWorld); | ||
direction.set(mouse.x, mouse.y, 0.5).unproject(camera).sub(origin).normalize(); | ||
this.el.setAttribute('raycaster', {origin: origin, direction: direction}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reuse an object for this
src/components/cursor.js
Outdated
}, | ||
|
||
update: function () { | ||
this.updateMouseEventListeners(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd only call this if rayOrigin
changes
docs/components/raycaster.md
Outdated
@@ -58,6 +58,7 @@ AFRAME.registerComponent('collider-check', { | |||
| origin | Vector3 coordinate of where the ray should originate from relative to the entity's origin. | 0, 0, 0 | | |||
| recursive | Checks all children of objects if set. Else only checks intersections with root objects. | true | | |||
| showLine | Whether or not to display the raycaster visually with the [line component][line]. | false | | |||
| worldCoordinates | Determine if origin and direction are given in local or world coordinates. | false | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether the raycaster origin and direction properties are specified in world space.
docs/components/cursor.md
Outdated
@@ -80,6 +80,7 @@ AFRAME.registerComponent('cursor-listener', { | |||
| downEvents | Array of additional events on the entity to *listen* to for triggering `mousedown` (e.g., `triggerdown` for vive-controls). | [] | | |||
| fuse | Whether cursor is fuse-based. | false on desktop, true on mobile | | |||
| fuseTimeout | How long to wait (in milliseconds) before triggering a fuse-based click event. | 1500 | | |||
| rayOrigin | if the intersection ray is cast from the entity or the mouse | entity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where the intersection ray is cast from (i.e.,
entityor
mouse)
tests/components/cursor.test.js
Outdated
@@ -76,6 +76,16 @@ suite('cursor', function () { | |||
done(); | |||
}); | |||
}); | |||
|
|||
suite('update', function () { | |||
test('attach mousemove event listener on mouse mode', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also test removing mousemove event listeners
261be15
to
6f0f7c8
Compare
I can change the rayOrigin from |
@stephanedemotte can you open an issue? |
done ;) #2929 |
cursor
component similar to https://github.com/mayognaise/aframe-mouse-cursor-componentraycaster
component to accommodate the mouse cursor use caseDemo