Add user-controlled camera and canvas to cursor component. #4983
Conversation
| @@ -196,14 +213,11 @@ module.exports.Component = registerComponent('cursor', { | |||
| }, | |||
|
|
|||
| onMouseMove: (function () { | |||
| var direction = new THREE.Vector3(); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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? |
|
Finally got the OK to publish some examples of how this can be used. In particular see: |
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:
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.
This widget
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.