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.

Sign up for GitHub

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

Jump to bottom

Add user-controlled camera and canvas to cursor component. #4983

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add user-controlled camera and canvas to cursor component. #4983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
167e8c6
Add user-controlled camera and canvas to cursor component.
diarmidmackenzie Jan 3, 2022
9b66031
Bug fix (this needs to be bound).
diarmidmackenzie Jan 9, 2022
5f0b723
Updated builds with latest bug fix
diarmidmackenzie Jan 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3,554 changes: 1,118 additions & 2,436 deletions dist/aframe-master.js
View file
Open in desktop

Large diffs are not rendered by default.

88 changes: 22 additions & 66 deletions dist/aframe-master.js.map
View file
Open in desktop

Large diffs are not rendered by default.

3,160 changes: 1,619 additions & 1,541 deletions dist/aframe-master.min.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/aframe-master.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion docs/components/cursor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,10 @@ AFRAME.registerComponent('cursor-listener', {
| 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 |
| mouseCursorStylesEnabled | Whether to show pointer cursor in `rayOrigin: mouse` mode when hovering over entity. | true |
| rayOrigin | Where the intersection ray is cast from (i.e.,entity or mouse). `rayOrigin: mouse` is extremely useful for VR development on a mouse and keyboard. | entity
| rayOrigin | Where the intersection ray is cast from (i.e.,entity or mouse). `rayOrigin: mouse` is extremely useful for VR development on a mouse and keyboard. | entity|
| upEvents | Array of additional events on the entity to *listen* to for triggering `mouseup` (e.g., `trackpadup` for daydream-controls). | [] |
| canvas | Provides more control is scenarios where a scene is being rendered to multiple canvases. When set to "auto" the cursor component always uses the main scene canvas. When set to "user" the cursor component exposes a variable at `this.canvas` which the user can set to an alternative canvas. | auto |
| camera | Provides more control is scenarios where a scene is being rendered by multiple cameras. When set to "auto" the cursor component always uses the main scene camera. When set to "user" the cursor component exposes a variable at `this.camera` which the user can set to an alternative camera. | auto |

To further customize the cursor component, we configure the cursor's dependency
component, [the raycaster component][raycaster].
Expand Down
68 changes: 49 additions & 19 deletions src/components/cursor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ module.exports.Component = registerComponent('cursor', {
fuseTimeout: {default: 1500, min: 0},
mouseCursorStylesEnabled: {default: true},
upEvents: {default: []},
rayOrigin: {default: 'entity', oneOf: ['mouse', 'entity']}
rayOrigin: {default: 'entity', oneOf: ['mouse', 'entity']},
canvas: {default: 'auto', oneOf: ['auto', 'user']},
camera: {default: 'auto', oneOf: ['auto', 'user']}
},

init: function () {
Expand All @@ -64,9 +66,18 @@ module.exports.Component = registerComponent('cursor', {
this.canvasBounds = document.body.getBoundingClientRect();
this.isCursorDown = false;

// expose camera and cursor to user if required.
if (this.data.camera === 'user') {
this.camera = this.el.sceneEl.camera;
}
if (this.data.canvas === 'user') {
Copy link
Member

@dmarcos dmarcos Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This API is strange. Public variables are set or not depending on the value of a component property. Purpose is not clear either reading the schema or the docs. Probably feels too ad-hoc to your use case.

Copy link
Contributor Author

@diarmidmackenzie diarmidmackenzie Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it is a bit ad hoc.

The reason for adding this to the interface was a desire to maintain complete back compatibility.

An example of a problem that could be caused by not putting this control on the interface is the following:

  • An A-Frame app that switches between 2 different cameras, e.g. orthographic & perspective.
  • With the existing cursor, cursor will auto-adapt to the current camera (as of this fix).
  • After my change, it would become necessary to manually update the cursor's view of the camera when switching between ortho & perspective.
  • I didn't want to risk breaking existing applications, hence the default setting of "auto" and the new mode needing to be explicitly activated in config.
  • Not strictly necessary for 2 different parameters (camera and canvas), but they are logically different functions so it seemed better.

In terms of the documentation, I think it would help a lot if I had an example to link to. As per notes above, I'm hoping to re-use the component I have built as an example, but still TBD whether the owners of that code will be willing to open source it.

In short, I think a config option is necessary if we are to avoid breaking existing apps. OTOH, the fix to make cursor work with an orthographic camera isn't even released yet (in master but not 1.2.0), so another option might be to simply document that fact that if you want to switch which camera is used for a cursor, you need to explicitly update this.camera in the cursor component.

this.canvas = this.el.sceneEl.canvas;
this.canvasBounds = this.canvas.getBoundingClientRect();
}

// Debounce.
this.updateCanvasBounds = utils.debounce(function updateCanvasBounds () {
self.canvasBounds = self.el.sceneEl.canvas.getBoundingClientRect();
self.canvasBounds = self.getCanvas().getBoundingClientRect();
}, 500);

this.eventDetail = {};
Expand All @@ -79,6 +90,12 @@ module.exports.Component = registerComponent('cursor', {
this.onIntersectionCleared = bind(this.onIntersectionCleared, this);
this.onMouseMove = bind(this.onMouseMove, this);
this.onEnterVR = bind(this.onEnterVR, this);

// Variables used in raycasting. One set of these needed per-cursor
// instance.
this.direction = new THREE.Vector3();
this.origin = new THREE.Vector3();
this.rayCasterConfig = {origin: this.origin, direction: this.direction};
},

update: function (oldData) {
Expand Down Expand Up @@ -109,18 +126,18 @@ module.exports.Component = registerComponent('cursor', {
var el = this.el;
var self = this;

function addCanvasListeners () {
canvas = el.sceneEl.canvas;
const addCanvasListeners = () => {
canvas = this.getCanvas();
if (data.downEvents.length || data.upEvents.length) { return; }
CANVAS_EVENTS.DOWN.forEach(function (downEvent) {
canvas.addEventListener(downEvent, self.onCursorDown);
});
CANVAS_EVENTS.UP.forEach(function (upEvent) {
canvas.addEventListener(upEvent, self.onCursorUp);
});
}
};

canvas = el.sceneEl.canvas;
canvas = this.getCanvas();
if (canvas) {
addCanvasListeners();
} else {
Expand Down Expand Up @@ -152,7 +169,7 @@ module.exports.Component = registerComponent('cursor', {
var el = this.el;
var self = this;

canvas = el.sceneEl.canvas;
canvas = this.getCanvas();
if (canvas && !data.downEvents.length && !data.upEvents.length) {
CANVAS_EVENTS.DOWN.forEach(function (downEvent) {
canvas.removeEventListener(downEvent, self.onCursorDown);
Expand Down Expand Up @@ -184,7 +201,7 @@ module.exports.Component = registerComponent('cursor', {
var canvas;
var el = this.el;

canvas = el.sceneEl.canvas;
canvas = this.getCanvas();
canvas.removeEventListener('mousemove', this.onMouseMove);
canvas.removeEventListener('touchmove', this.onMouseMove);
el.setAttribute('raycaster', 'useWorldCoordinates', false);
Expand All @@ -196,14 +213,11 @@ module.exports.Component = registerComponent('cursor', {
},

onMouseMove: (function () {
var direction = new THREE.Vector3();
Copy link
Member

@dmarcos dmarcos Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

These variables not meant to store state but used for local calculations and cached to prevent unnecessary instantiations. Why do they need to be public now?

They should also work for multiple cursors if the variables purpose hasn't changed

Copy link
Member

@dmarcos dmarcos Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The more variables exposed, the larger the API surface area, the greater maintenance burden (can't remove / rename variables without thinking about 3rd parties) and increased possibility of misuse.

Copy link
Contributor Author

@diarmidmackenzie diarmidmackenzie Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

They don't need to be public. The issue is that they are not only used transiently, but actually passed by reference into the raycaster and used there.

this.el.setAttribute('raycaster', rayCasterConfig);

Result is that if you have two separate cursor components on the same scene, they share these variables, and end up scribblng over each others' raycaster parameters.

It was a bit of a surprise to me that this happened, but it definitely did (took me quite a long time to figure out what was going on, as it was so unexpected - can create a repro if you want to take a look as exactly what happens here).

This was a simple solution that definitely works, because it gives you a unique instance of origin & direction per cursor instance.

I don't know of another way to solve this that doesn't have perf implications (e.g. copying the value of the origin & direction vectors to pass them into the raycaster). Can you suggest a better way to solve this?

Copy link
Contributor Author

@diarmidmackenzie diarmidmackenzie Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Would giving them names that indicate they are internal, e.g. this._orgin and this._direction help?

Or can we make them private? this.#origin, this.#direction? (apparently this is brand new in ES2022, I only just found out about this option)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields
https://tc39.es/ecma262/multipage/ecmascript-language-lexical-grammar.html#prod-PrivateIdentifier

(edited - initial reading suggested this was introduced in ECMA2019, but in fact it appears to be new in ECMA2022).

var mouse = new THREE.Vector2();
var origin = new THREE.Vector3();
var rayCasterConfig = {origin: origin, direction: direction};

return function (evt) {
var bounds = this.canvasBounds;
var camera = this.el.sceneEl.camera;
var camera = this.getCamera();
var left;
var point;
var top;
Expand All @@ -224,16 +238,16 @@ module.exports.Component = registerComponent('cursor', {
mouse.y = -(top / bounds.height) * 2 + 1;

if (camera && camera.isPerspectiveCamera) {
origin.setFromMatrixPosition(camera.matrixWorld);
direction.set(mouse.x, mouse.y, 0.5).unproject(camera).sub(origin).normalize();
this.origin.setFromMatrixPosition(camera.matrixWorld);
this.direction.set(mouse.x, mouse.y, 0.5).unproject(camera).sub(this.origin).normalize();
} else if (camera && camera.isOrthographicCamera) {
origin.set(mouse.x, mouse.y, (camera.near + camera.far) / (camera.near - camera.far)).unproject(camera); // set origin in plane of camera
direction.set(0, 0, -1).transformDirection(camera.matrixWorld);
this.origin.set(mouse.x, mouse.y, (camera.near + camera.far) / (camera.near - camera.far)).unproject(camera); // set origin in plane of camera
this.direction.set(0, 0, -1).transformDirection(camera.matrixWorld);
} else {
console.error('AFRAME.Raycaster: Unsupported camera type: ' + camera.type);
}

this.el.setAttribute('raycaster', rayCasterConfig);
this.el.setAttribute('raycaster', this.rayCasterConfig);
if (evt.type === 'touchmove') { evt.preventDefault(); }
};
})(),
Expand Down Expand Up @@ -359,7 +373,7 @@ module.exports.Component = registerComponent('cursor', {
this.twoWayEmit(EVENTS.MOUSEENTER);

if (this.data.mouseCursorStylesEnabled && this.data.rayOrigin === 'mouse') {
this.el.sceneEl.canvas.classList.add(CANVAS_HOVER_CLASS);
this.getCanvas().classList.add(CANVAS_HOVER_CLASS);
}

// Begin fuse if necessary.
Expand Down Expand Up @@ -388,7 +402,7 @@ module.exports.Component = registerComponent('cursor', {
this.twoWayEmit(EVENTS.MOUSELEAVE);

if (this.data.mouseCursorStylesEnabled && this.data.rayOrigin === 'mouse') {
this.el.sceneEl.canvas.classList.remove(CANVAS_HOVER_CLASS);
this.getCanvas().classList.remove(CANVAS_HOVER_CLASS);
}

// Unset intersected entity (after emitting the event).
Expand Down Expand Up @@ -425,5 +439,21 @@ module.exports.Component = registerComponent('cursor', {

this.intersectedEventDetail.intersection = intersection;
intersectedEl.emit(evtName, this.intersectedEventDetail);
},

getCanvas: function () {
if (this.data.canvas === 'user') {
return this.canvas;
} else {
return this.el.sceneEl.canvas;
}
},

getCamera: function () {
if (this.data.camera === 'user') {
return this.camera;
} else {
return this.el.sceneEl.camera;
}
}
});
2 changes: 1 addition & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ require('./core/a-mixin');
require('./extras/components/');
require('./extras/primitives/');

console.log('A-Frame Version: 1.2.0 (Date 2021-12-29, Commit #ee2e3198)');
console.log('A-Frame Version: 1.2.0 (Date 2022-01-09, Commit #9b66031c)');
console.log('THREE Version (https://github.com/supermedium/three.js):',
pkg.dependencies['super-three']);
console.log('WebVR Polyfill Version:', pkg.dependencies['webvr-polyfill']);
Expand Down