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

Review instrumentation of colorProfile #261

Closed
arouinfar opened this issue Jul 13, 2024 · 8 comments
Closed

Review instrumentation of colorProfile #261

arouinfar opened this issue Jul 13, 2024 · 8 comments

Comments

@arouinfar
Copy link

In phetsims/buoyancy#51 (comment) the devs asked:

All the colors listed in Buoyancy and Density are phetioReadOnly:true. Why is this valuable to have? Should they be uninstrumented?

The relevant paper trail is in #65 and it appears that the instrumented ColorProperties may be required for State.

I'd like to review global.view.colors for all sims in the suite and decide:

  • Should any additional colors be instrumented?
  • Are there any ColorProperties that would benefit from being phetioReadOnly: false?
@arouinfar arouinfar self-assigned this Jul 13, 2024
@arouinfar
Copy link
Author

arouinfar commented Jul 13, 2024

The mystery fluid colors still use the old "Density A", etc. naming scheme and should likely be updated to the new "Fluid A" language introduced in #248

image

Edit: I reported this in #265.

@arouinfar
Copy link
Author

Here is the full list of the instrumented colors. All 3 density-buoyancy-common sims are the same, even though these colors don't necessarily appear in all sims. In general, these colors correspond to the fluids in the pool and the materials inside the bottle, which seem like useful things to let clients control.

image

I'd like to wait for #265 to wrap up before working on this issue.

@arouinfar
Copy link
Author

The design requests in #265 have wrapped up, but I think it makes the most sense to review this along with the materials over in #250.

@arouinfar
Copy link
Author

arouinfar commented Jul 26, 2024

We discussed this in today's design meeting over in #268 (comment).

I'll leave this on hold until #268 is completed.

@AgustinVallejo
Copy link
Contributor

#268 Is almost done, and no longer holding this one.

@arouinfar
Copy link
Author

arouinfar commented Aug 6, 2024

Density does not require any instrumentation of the color profile, as the Mystery blocks all have a colorProperty as a part of the customMaterial.

Buoyancy and Buoyancy: Basics both have the same set of instrumented colors:
image

None of these colors need to be customizable, so I recommend uninstrumenting them. In #268 (comment), we noted that customizing fluid colors, in particular, could be problematic.

It would be nice to customize the colors of the mystery materials (T-Y) that don't have a textured appearance. Can we instrument those colors?

To summarize:

  • No changes needed for Density
  • For Buoyancy & Buoyancy: Basics: Uninstrument all currently instrumented colors.
  • For Buoyancy only: Instrument the colors of mystery materials T-Y

@arouinfar arouinfar assigned samreid and AgustinVallejo and unassigned arouinfar Aug 6, 2024
samreid added a commit that referenced this issue Aug 9, 2024
@samreid
Copy link
Member

samreid commented Aug 9, 2024

In the commits, I uninstrumented all ColorProperties, then added read/write instrumentation for the mysteries T-Y. In studio I was able to customize the color, and I saw it was preserved in Test. Please review.

@arouinfar
Copy link
Author

Looks good on main, thanks @samreid! Closing.

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

3 participants