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

make CameraHelper more customizable | choose colors | leave out sections if desired #24234

Closed
wants to merge 2 commits into from

Conversation

Heaust-ops
Copy link
Contributor

Description

Camera helper has too much going on,

If all that's needed is the info about the location and face direction of the camera,

the extra lines for the entire frustum of rendering is unneeded and gets in the way.

Same may be true vice-versa.

This PR makes the camera helper more customizable with extra arguments for the colors of the different parts of the helper.

Passing null instead of a color prevents that part of the helper from being rendered.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 14, 2022

Is this a duplicate of #23829?

@Heaust-ops
Copy link
Contributor Author

Is this a duplicate of #23829?

I can't say for sure, the main intent was the ability to leave out certain sections of the helper, #23829 doesn't seem to do that.

@Heaust-ops
Copy link
Contributor Author

for example if I don't wanna render the entire camera helper,
I can just do this,

new CameraHelper(camera, {colorFarFrustum: null, colorTarget: null});

and I'll only have the bits I want,
image

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 14, 2022

Adding customizable colors is okay since other helpers like AxesHelper support that, too. However, I think we should not start adding configuration options that influence what part of the helper is rendered. That feels like over-engineering and just introduces an unnecessary complexity.

@Heaust-ops
Copy link
Contributor Author

understandable, ty : )

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 14, 2022

Thanks for the PR but I think it's better if we focus on #23829, first.

@Mugen87 Mugen87 closed this Jun 14, 2022
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