Fix sprite raycasting bugs

[WestLangley]

Fixes #14936 and #14624.

Adding a reference to camera in Raycaster.setFromCamera() seems the least hacky approach I can think of.

/ping @06wj
/ping @Mugen87

[06wj]

How about add sprite.camera in sprite.onBeforeRender

[gkjohnson]

I think the docs need to be really clear on this. If a user calculates a camera vector manually instead of using setFromCamera and tries to raycast with it the cast will yield different results which could be really confusing.

[WestLangley]

If a user calculates a camera vector manually instead of using setFromCamera

Yes, I am aware of that. This PR is a proof-of-concept.

[WestLangley]

@06wj @Mugen87 Can you please test this and provide feedback?

[Mugen87]

I've tested the change with webgl_raycast_sprite and with the mentioned fiddle from the forum (https://jsfiddle.net/9awj1tqm/). Raycasting works fine now.

[WestLangley]

I've tested the change with webgl_raycast_sprite and with the mentioned fiddle from the forum (https://jsfiddle.net/9awj1tqm/). Raycasting works fine now.

What about OrbitControls and #14936?

[06wj]

I've tested #14936, it works fine.

[WestLangley]

So, do we want to go with this approach? And if so, what instructions should we provide the user?

[Mugen87]

So, do we want to go with this approach?

I would say yes.

And if so, what instructions should we provide the user?

I would note in the documentation for Sprite that the usage of Raycaster.setFromCamera() is mandatory when raycasting against sprites.

[gkjohnson]

@WestLangley would it make sense to throw a more clear and explicit error when the camera isn't defined on the raycaster and sprites are being used? It looks like an exception would get thrown anyway.

And should it be supported to set the camera explicitly instead of with the setFromCamera function in the case the vector is calculated by some other means?

[06wj]

I think using camera in the renderer is better than using camera in the raycaster.
We can set the camera in sprite.onBeforeRender or in the WebGLRenderer.
sprite._currentRenderCamera = camera

[WestLangley]

@06wj I'll give that a try...

[WestLangley]

Added _camera to every sprite instead of just one raycaster.

@gkjohnson @06wj @Mugen87

[Mugen87]

I think this approach is better since it's not necessary anymore to modify Raycaster. As mentioned before, I did not like the tight coupling between Raycaster and Camera.

[WestLangley]

@gkjohnson Does this now satisfy your concerns?

[gkjohnson]

Sorry for the slow response -- I'm away this weekend. I personally prefer adding the camera to the raycaster. I feel relying on side effects from calling render seems a little confusing and not all that extensible. It means you can't raycast against a scene without rendering it first and you must raycast between renders if you're rendering from multiple camera angles because the last camera rendered with will be the one active on the sprites.

I think adding extra variables on to the raycaster for custom raycast scenarios is a fine pattern myself and have used it in my own projects when implementing a custom raycast function. Both ways work, though! Those are just a couple of the potential concerns that come to mind.

[WestLangley]

Well, I can always revert to the previous approach... /ping @06wj @Mugen87

[06wj]

In fact, raycast will fail if the scene is not rendered, no matter it is Sprite or other Mesh. Because the matrix of camera and objects are updated after the scene is rendered.

Whether or not the Sprite is raycasted depends on which camera it was rendered by, not which camera the ray was generated by.

[WestLangley]

The camera used to set the ray, and the camera used in the raycasting algorithm must be the same camera. I am inclined to revert to what I proposed originally.

[Mugen87]

I am inclined to revert to what I proposed originally.

What are you referring to exactly? Sry, many approaches were discussed and I just want to be sure to understand the right thing.

[WestLangley]

What are you referring to exactly?

Doing what I said in my original post.

[Mugen87]

Well, do whatever you think is best. Any of the proposed solutions is better than the current one.

[gkjohnson]

@WestLangley I do feel that Raycaster._camera should be considered a public variable (and therefore not be prefixed with an underscore). As it is the only way to set it is if you have a screen coordinate and call "setFromCamera" but there are cases where you might not want a camera-projected ray (or already have the pre-projected ray). In these cases you might want to set the camera to use and the ray separately.

One use case that comes to mind is using a controller with a pointer in VR to select sprites.

[WestLangley]

One use case that comes to mind is using a controller with a pointer in VR to select sprites.

Can webvr_dragging.html be hacked to add Sprites as a proof-of-concept?

/ping @gkjohnson

[gkjohnson]

Can webvr_dragging.html be hacked to add Sprites as a proof-of-concept?

It looks like it could be -- is your goal to have an example with which to test the behavior? Or show people how to use it?

I haven't done a whole lot of WebVR work myself so I'd have to get set up to run the VR examples. I can probably do that at some point, though. If you'd prefer to wait for the example then maybe the camera member can be made public in a separate PR.

[WestLangley]

One use case that comes to mind is using a controller with a pointer in VR to select sprites.

According to @mrdoob, sprites do not work well in VR due to the multiple cameras. For GUIs, non-billboarded planes are used instead.

[WestLangley]

I think this PR can be merged. It is the best we can do at the moment, and it is certainly better than what we have now.

[WestLangley]

OK, merging.