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

r3f .connect method #338

Merged
merged 1 commit into from
Jan 22, 2023
Merged

r3f .connect method #338

merged 1 commit into from
Jan 22, 2023

Conversation

abernier
Copy link
Collaborator

Introducing a .connect method to allow binding events and side-effects independently from the instanciation:

const cameraControls = new CameraControls(camera)

cameraControls.connect(renderer.domElement)
cameraControls.dispose()
cameraControls.connect(renderer.domElement)
cameraControls.dispose()

Backward compatible:

const cameraControls = new CameraControls(camera, renderer.domElement) // auto-`connect`ed

This is useful in order <CameraControls> drei component to work properly with r3f.

See: #262 (comment)

@abernier
Copy link
Collaborator Author

abernier commented Jan 22, 2023

Git displays a big diff, but since the class was already really well organised, I basically just have made the _domElement optional:
image
image

and moved those lines out of the if ( domElement ) {} block:

image

and created this new this._addAllEventListeners method:

image

Finally I added a public .connect method near the dispose one:

image

@abernier
Copy link
Collaborator Author

abernier commented Jan 22, 2023

I also made a dedicated examples/r3f.html test with 2 buttons: connect and dispose and stress-tested them without any issue.

I also checked it does not break other examples (only tested 3 or 4 not all, but seemed ok)

@yomotsu
Copy link
Owner

yomotsu commented Jan 22, 2023

Thanks for your PR with detailed information as well as testing with examples.
I totally understand what you changed.
Merging the PR, and let me change it a little bit after that.

@yomotsu yomotsu merged commit 4ee490d into yomotsu:dev Jan 22, 2023
@abernier
Copy link
Collaborator Author

abernier commented Jan 22, 2023

Thank you very much 🙏🏻

I will PR an update of drei's CameraControls component when you release the next version of camera-controls, that takes advantage on that new .connect method ;)

@yomotsu
Copy link
Owner

yomotsu commented Jan 22, 2023

@abernier
Thank you too!
Okay, I will let you know when it's ready.

@yomotsu
Copy link
Owner

yomotsu commented Jan 22, 2023

@abernier
v1.38.0 has been released and published on npm

I changed some with #339 and added disconnect(). dispose() is now kind of a wrapper of disconnect().
(If you are interested, please check the PR for more change details)

  • disconnect() is literally disconnect from the domElement.
  • dispose() is for dispose or destroy, that includes disconnect() internally.

@abernier
Copy link
Collaborator Author

Excellent!

I'm created a drei's PR to take those changes into account: pmndrs/drei#1238

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.

2 participants