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

density.global.view.colorProfile should only contain colors relevant to Density #89

Closed
arouinfar opened this issue Aug 30, 2021 · 10 comments

Comments

@arouinfar
Copy link
Contributor

arouinfar commented Aug 30, 2021

For #76

Several of the colors in the color profile only apply to the Buoyancy simulation. Can they be removed from Density @jonathanolson?

@jonathanolson
Copy link
Contributor

@samreid or @zepumph, could we conditionally remove the display of color properties? Or would it be best to conditionally provide opt-out tandems?

@jonathanolson jonathanolson assigned samreid and zepumph and unassigned jonathanolson Sep 1, 2021
@samreid
Copy link
Member

samreid commented Sep 1, 2021

How about DensityColors.js and BuoyancyColors.js?

@jonathanolson
Copy link
Contributor

We still instantiate constant Materials on startup, and have a list of all Materials. Would I need to make color/material loading lazy? This could totally mess up state restoring.

@jonathanolson jonathanolson removed their assignment Sep 1, 2021
@samreid
Copy link
Member

samreid commented Sep 2, 2021

Would it be easy to have a list of DensityMaterials and BuoyancyMaterials, or is it better to have them all defined together?

@samreid samreid assigned jonathanolson and unassigned samreid Sep 2, 2021
@jonathanolson
Copy link
Contributor

It would be better to have them defined together.

@zepumph
Copy link
Member

zepumph commented Sep 7, 2021

What about metadata on them that can be used to conditionally instrument? Or conditionally phetioFeature them depending on what sim is being run?

@zepumph
Copy link
Member

zepumph commented Sep 7, 2021

What about using something like the namespace solution from phetsims/scenery#1259? Or will this not work because they are all defined in the common repo?

@zepumph
Copy link
Member

zepumph commented Sep 7, 2021

Probably it will be best to just have a ternary on how you provide the Tandem, like for

https://github.com/phetsims/density-buoyancy-common/blob/6703af36f2c598358f2bdbbaa3def94d5198bfbd/js/common/view/DensityBuoyancyCommonColors.js#L110

. . . .

    tandem: DENSITY_IS_THE_SIM ? tandem.createTandem( 'mysteryOrangeColorProperty' ) : Tandem.OPT_OUT

jonathanolson added a commit to phetsims/density-buoyancy-common that referenced this issue Sep 7, 2021
@jonathanolson
Copy link
Contributor

It looks like the ternary is working best. Committed above, @arouinfar can you verify?

@arouinfar
Copy link
Contributor Author

Thanks @jonathanolson! The tree looks much better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants