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

feat: adds customizable colors to CameraHelper #23829

Closed
wants to merge 1 commit into from
Closed

feat: adds customizable colors to CameraHelper #23829

wants to merge 1 commit into from

Conversation

gsimone
Copy link
Contributor

@gsimone gsimone commented Apr 1, 2022

Found myself needing to color-code camera helpers and wanting to use toned-down colors in other occasions.

This PR wouldn't let users change the colors after construction, but it could be a good middle ground - as helpers are cheap to rebuild anyway

Usage:

const helper = new CameraHelper( camera, 0xffffff ) // white frustum
...

image

TODO

  • Add docs

Comment on lines -33 to +37
const colorFrustum = new Color( 0xffaa00 );
const colorCone = new Color( 0xff0000 );
const colorUp = new Color( 0x00aaff );
const colorTarget = new Color( 0xffffff );
const colorCross = new Color( 0x333333 );
const _colorFrustum = new Color( colorFrustum );
const _colorCone = new Color( colorCone );
const _colorUp = new Color( colorUp );
const _colorTarget = new Color( colorTarget );
const _colorCross = new Color( colorCross );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming convention here is ugly, but I'd rather the arguments have the nice names - as they are user-facing

@@ -18,7 +18,7 @@ const _camera = /*@__PURE__*/ new Camera();

class CameraHelper extends LineSegments {

constructor( camera ) {
constructor( camera, colorFrustum = 0xffaa00, colorCone = 0xff0000, colorUp = 0x00aaff, colorTarget = 0xffffff, colorCross = 0x333333 ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative API would be passing an object with all colors as

{
   frustum: 0xff0000,
   cone: 0xff00ff,
  // ...etc
}

But I can't recall objects that also use this pattern, so this felt more appropriate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the object is a better approach. And it will also solve the naming problem.

@mrdoob
Copy link
Owner

mrdoob commented Apr 2, 2022

How about...

const helper = new THREE.CameraHelper( camera ).setColors( frustum, cone, up, target, cross );

@mrdoob mrdoob added this to the r140 milestone Apr 2, 2022
@gsimone
Copy link
Contributor Author

gsimone commented Apr 2, 2022

@mrdoob that API would be nice indeed, but it would require require rebuilding the vertices and colors attributes OR a possibly signifcant refactor of the class to just update the colors buffer - moving the two buffers creation to a method. Think it's worth pursuing? 🤔

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 4, 2022

In the past we have agreed to no introduce larger ctor signatures nor options parameters again. The idea is the usage of methods which tends to be a less error prone API. Hence, the PR in ist current form is problematic.

CameraHelper.setColors() would nicely map to AxesHelper.setColors(). As mentioned above, it's probably best to implement the color buffer setup in a separate function and reuse it in the ctor and setColors().

@gsimone
Copy link
Contributor Author

gsimone commented Apr 4, 2022

I'll give that a shot 👍
Think it's ok to also rebuild the position buffer? That would require the smallest possible code change - as I'd just take the current constructor to a method.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 4, 2022

I think it's more clear if setColors() only changes the color buffer. Even if that means more things in the class have to be changed.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 14, 2022

Closing in favor of #24235.

@Mugen87 Mugen87 closed this Jun 14, 2022
@Mugen87 Mugen87 removed this from the r142 milestone 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.

4 participants