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: Allow setting the gizmos radius factor #22721

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

Tirzono
Copy link
Contributor

@Tirzono Tirzono commented Oct 22, 2021

Description

I would like to be able to set the gizmos radius factor. Currently the only way to do this is to extend the ArcballControls and overwrite the calculateTbRadius function, but I think it would be cleaner to have a way of setting it as a property.

@danielefornari let me know if you are happy with this (thanks for your great work on ArcballControls!) :)

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 22, 2021

@Tirzono It's not necessary to do it in this PR but it would be great if you add both new properties to the documentation page of ArcBallControls: https://threejs.org/docs/index.html#examples/en/controls/ArcballControls

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 22, 2021

That's not a problem, let me know what you think. I'll also add it to the other PR.

@danielefornari
Copy link
Contributor

Indeed is a nice feature to have! Beside radiusFactor, i would also add a setTbRadius(float) method which updates the radius property, redraw gizmos and send a changeEvent to visualize changes in real time.

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 22, 2021

Sounds like a good idea. I am not sure if I have time for that to add before the weekend, but if not then I'll add that after the weekend.

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

Tirzono commented Oct 25, 2021

I've added the setTbRadius function. Let me know what you think or if it needs any more changes.

@mrdoob mrdoob merged commit f83120f into mrdoob:dev Oct 28, 2021
@mrdoob
Copy link
Owner

mrdoob commented Oct 28, 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