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

ArcballControls: Expose raycaster. #22719

Merged
merged 4 commits into from
Oct 26, 2021
Merged

Conversation

Tirzono
Copy link
Contributor

@Tirzono Tirzono commented Oct 21, 2021

Description

This allows users to use their own raycaster (that has for example certain layers enabled/disabled). Since unprojectOnObj is used for the double click to focus functionality, passing in your own raycaster is useful if you want the double click to focus functionality also to work on objects that are in different layers.

@Mugen87 Mugen87 changed the title Arcballcontrols ArcballControls: Expose raycaster. Oct 21, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 21, 2021

@danielefornari Looking good?

@danielefornari
Copy link
Contributor

Yeah, sounds good!

@mrdoob
Copy link
Owner

mrdoob commented Oct 23, 2021

We should probably follow #22070 and add a getRaycaster() instead.

@mrdoob mrdoob added this to the r134 milestone Oct 23, 2021
@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 25, 2021

I am okay doing that change because I can achieve what I want with that as well by extending the ArcballControls and overwrite the getRaycaster method:

class ExtendedArcballControls extends ArcballControls {
    constructor(...args) {
        super(...args);

        this.raycaster = new Raycaster();
    }

    getRaycaster() {
        return this.raycaster;
    }
}

Another possible solution would be to do something like this in my code:

const controls = new ArcballControls();
const raycaster = controls.getRaycaster();

raycaster.layers.enable(4); // For example

However, I have multiple canvases on one page and if I would use multiple ArcballControls on one page as well they all share the same raycaster and this might not be what I want (because in one canvas I want layers 4 to be enabled and in the other one maybe not). Is this something we take into account?

Let me know what you think. It's more a question, I am happy to make the change because I can achieve what I want with the first example that I gave.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 25, 2021

It's just a matter of consistency. Instead of exposing raycaster as a property, it's better to expose it via a getter.

Sharing the same raycaster across multiple control instances is a different matter though. I would not adding support for this since I'm afraid there are unexpected side effects. Hence, the raycaster reference of the control class should not be writable by the application.

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 25, 2021

Sharing the same raycaster across multiple control instances is a different matter though. I would not adding support for this since I'm afraid there are unexpected side effects.

But isn't this not the case when we would use the getRaycaster method? At least, if I look at the code in TransformControsl, it is sharing the same raycaster across multiple control instances:

const first = new TransformControls(
  new OrthographicCamera(),
  document.createElement("canvas")
);
const second = new TransformControls(
  new OrthographicCamera(),
  document.createElement("canvas")
);

first.getRaycaster() === second.getRaycaster() // equals true

https://codesandbox.io/s/interesting-rubin-owq9j?file=/src/index.js

Should I not follow that example and make a this._raycaster so that we have a unique raycaster per ArcballControls instance and return that with getRaycaster?

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 25, 2021

Oh right, that's true. If forgot that TransformControls(and DragControls) define their raycaster in module scope.

I guess it would be good to do the same for ArcballControls if possible.

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 25, 2021

That's done now 😃

@Mugen87 Mugen87 merged commit b27e4c8 into mrdoob:dev Oct 26, 2021
@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2021

Thanks!

@joshuaellis joshuaellis mentioned this pull request Nov 2, 2021
12 tasks
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.

4 participants