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

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

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

Conversation

diarmidmackenzie
Copy link
Contributor

Description:

For an application I've been building, I have had to make some changes to the cursor component. I believe these increase flexibility of the component and would be generally useful.

Where there are multiple cameras & multiple canvases used in an A-Frame scene, these changes allow cursor components to be used on both canvases.

See below for more details on the use case that drove this.

This PR is to yet ready to merge. It needs further testing, and examples to illustrate how this can be used. I'm raising it now to share the concept, and confirm that it's worth progressing with this testing and examples.

Feedback on whether this PR could be considered for A-Frame, once supporting examples are available, would be appreciated.

Changes proposed:

Changes are all to cursor component & accompanying docs. In summary:

  • new configuration options on the cursor component to determine whether or not user has control of camera & canvas.
  • if not (auto), function is completely unchanged.
  • if yes (user), the cursor component exposes the camera and canvas, the user can set them to whichever camera and canvas is desired, and the cursor component will use this camera & canvas in interpreting mouse movements.

Use case that motivates this

We are using A-Frame for a collaborative model viewer that supports both VR and desktop users.

For the desktop users, we want to offer a view selector widget in the corner, like this one in AutodeskMeshMixer.
image

This widget

  • is 3D, so it's desirable to implement it using A-Frame
  • has to be rendered using a different camera and canvas
  • requires a cursor component, so that different sub-elements of the widget can be selected by mouse.

A-Frame's primary purpose is as a VR library, so we don't expect native support for multiple cameras, multiple canvases etc. (as per this discussion: #916), and and we are building out our own code to handle these functions.

However, we want to use A-Frame as much as possible.

This PR doesn't add any new native functionality to cursor, so it is not extending the scope of A-Frame. It simply refactors the code in a way that makes it much easier for an application to customize the function of cursor as needed (and does so with minimum risk to pre-existing function, by keeping the default behaviour identical).

I hope this wil be considered to be well-aligned with the A-Frame philosophy to provide a small common core of code focused on VR use cases, but provide users with the flexibility to extend functionality using custom THREE.js code as required.

** Examples**

I would like to provide an example to show the use of this in an application. I think this be helpful both to appreciate the value of the PR, and to help future users to understand how they can take advantage of it.

I am working on getting permission to open-source the view-controller widget as an example. However if I don't get agreement to do that, I'll build a simple demo app that illustrates the concept.

I don't think this PR should be merged until the example is in place.

However I'm raising the PR now, as I don't want to put more time into building such an example if there is actually zero chance of the PR being accepted.

@@ -196,14 +213,11 @@ module.exports.Component = registerComponent('cursor', {
},

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

@dmarcos dmarcos Jan 5, 2022

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

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

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

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).

if (this.data.camera === 'user') {
this.camera = this.el.sceneEl.camera;
}
if (this.data.canvas === 'user') {
Copy link
Member

@dmarcos dmarcos Jan 5, 2022

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

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.

@diarmidmackenzie
Copy link
Contributor Author

Stepping back, the fact you have reviewed the code suggests to me that if we can solve the issues you have flagged, you might be willing to take this into A-Frame.

Hence I'm thinking it would be worth pushing ahead with an example that illustrates how this can be used, to complete the PR.

Any comment on that?

@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Jan 20, 2022

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.

None yet

2 participants