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 OrbitControls connect method #23832

Closed

Conversation

alexandernanberg
Copy link

@alexandernanberg alexandernanberg commented Apr 2, 2022

Related issue: #20575

Description

Add a connect method which will perform the side-effects (attach event listeners). Backwards compatibility is kept but a warning message is logged.

connect with dispose sounds a bit weird. connect + disconnect feels more natural but dispose is what you commonly use. Happy to bike-shed more about naming

I'll fix the remaining todo's once I get an OK on the change

TODO

  • Update docs
  • Update examples

@LeviPesin
Copy link
Contributor

connect with dispose sounds a bit weird. connect + disconnect feels more natural but dispose is what you commonly use. Happy to bike-shed more about naming

Maybe make a function disconnect and just do this.dispose = this.disconnect? Then in other controls a similar pattern could be used - making functions connect and disconnect and calling disconnect (with some other code, possibly) from dispose.

@LeviPesin
Copy link
Contributor

So please remove the warning from the dispose function.

@alexandernanberg
Copy link
Author

Done! However I think dispose should always just be an alias of disconnect, I don't think there should be 2 clean up fns for controllers, but it's up to you I guess.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 5, 2022

I don't think it's necessary to update the examples. However adding the new methods to the documentation would be great!

@alexandernanberg
Copy link
Author

I've updated the docs now, let me know what you think!

@LeviPesin
Copy link
Contributor

@Mugen87 Will this PR be a part of the r140?

@drcmda
Copy link
Contributor

drcmda commented Jan 22, 2023

this would appreciated. though it affects all controls not just orbit it would be a start.

to reiterate, constructor side effects prevent a class from being created and discarded without knowing implementation details (like dispose). any system or automatism that expects classes to be pure (which they should be) will fail.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 10, 2024

Fixed via #29142.

@Mugen87 Mugen87 closed this Sep 10, 2024
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