Closed pixelzoom closed 1 year ago
The elements being instrumented are instances of ProfileColorProperty, so "global.view.colorProfile" would be OK. But I have a slight preference for "global.view.colors", since they are (by convention) grouped in a file named *Colors.ts.
Tandem.COLORS
is the way of the future. Let's update the others and add migration rules eagerly so that there isn't a dangling wrong pattern in our code to potentially proliferate.
In https://github.com/phetsims/acid-base-solutions/issues/215, I used Tandem.COLORS, so colors live under acidBaseSolutions.global.view.colors
.
I prefer global.view.colors
, glad to hear it's the way of the future. The colorProfile
terminology is a little less clear to me.
I'll convert sims to Tandem.COLORS
.
I started with calculus-grapher and ran into problem with joist changes in https://github.com/phetsims/joist/issues/934 that have broken migration. So I'm going to defer until that issue is addressed.
I put a patch in https://github.com/phetsims/joist/issues/934#issuecomment-1688146369 that seems to help calculus grapher past the joist issues.
Thanks for the patch @samreid. But I suspect that I'll hit the same problem with cck-* and density. I don't want to spend the time to discover that, or (worse) leave this issue in a half-finished state that breaks migration for sims. So this issue will continue to be deferred until https://github.com/phetsims/joist/issues/934 is closed.
I'm bailing on this for 2 reasons: (1) it does not block progress on Acid-Base Solutions, which is using Tandem.COLORS, and (2) there doesn't seem to be a consensus on when migration rules should be addressed, or the severity of breaking migration in master. So self unassigning, and we can revisit when other regressions in main have been addressed.
there doesn't seem to be a consensus on when migration rules should be addressed
We have come together around eagerly solving migration problems as they arise. CT is now reporting these problems for many repos and we should be able to continue on this work now. @pixelzoom is this something you would like to pick up now that CT will back you up on migration rules, and we have a process of pulling out common migration rules over in https://github.com/phetsims/phet-io-wrappers/issues/467, like joistRules.uninstrumentAboutDialogDetails()
. @samreid and I are happy to meet to discuss more if you'd like.
From 9/7/23 phet-io meeting.... I'll resume work on this.
Changing calculus-grapher went smoothly, see the above commits.
Changing density did not go smoothly. I discussed this with @samreid. He said this is a case that's not currently supported by migration, and I should assign back to him and @zepumph.
Here's the background... With this patch:
The Migration wrapper reports several errors of this form, where customColor.phetioID
, and possibly liquidColor.phetioID
, are still using the old "colorProfile" name:
"density.mysteryScreen.model.blockSets.set1.block1B.materialProperty": {
"value": {
"name": {
"phetioID": "density.general.model.strings.densityBuoyancyCommon.material.customStringProperty"
},
"identifier": null,
"tandemName": "custom",
"density": 400,
"viscosity": 0.001,
"custom": true,
"hidden": false,
"staticCustomColor": null,
"customColor": {
"phetioID": "density.global.view.colorProfile.compareBlueColorProperty"
},
"staticLiquidColor": null,
"liquidColor": null
},
"validValues": null,
"units": null
},
The relevant lines in Material.ts are:
const NullableColorPropertyReferenceType = NullableIO( ReferenceIO( Property.PropertyIO( Color.ColorIO ) ) );
...
public static readonly MaterialIO = new IOType<Material, MaterialState>( 'MaterialIO', {
...
stateSchema: {
...
customColor: NullableColorPropertyReferenceType,
...
liquidColor: NullableColorPropertyReferenceType
},
cck-dc and cck-dc-virtual-lab went smoothly, see above commits.
density will need to be completed by @samreid and @zepumph, so I'll unassign myself.
@kathy-phet Reporting back on how this work went... This should have been a straightforward rename ("global.view.colorProfile" => "global.view.colors"), and I anticipated that it would take ~30 minutes. Instead it required a solid 1 hour. I needed to involve another developer. I was only able to accomplish 3/4 of the work. And we identifed a case that is not supported by migration.
@pixelzoom - Thanks for taking it for a test drive and reporting back on where further work is to streamline these kinds of things.
@samreid and I worked on this this afternoon. We knew we'd get here eventually because we were never renaming any state values that were associated with a phetioID (like ReferenceIO). Here is a patch to get us started. Our favorite part about it is an assertion after migrating a rename that checks to see if the "old" phetioID is anywhere in the state. This double check will help us make sure we cover all cases. For now we will focus on the ReferenceIO state case and see if others present themselves.
I worked on the recursive API visiting part and got this far. Here is a patch which includes the above patch (parts commented out). The main idea is that when putting a value into the top-level state, we call processStateValue
like so:
// Migrate the old value into the new state, always do this because we fill in newState from a clean, empty object.
newState[ newPhetioID ] = this.processStateValue( phetioID, oldState, migrationEngine );
Then there are a pair of functions getStateSchema
and processStateValue
to recurse over a state. This is untested and half-baked but I wanted to share my progress before I have to step away.
@samreid and I got to a commit point today. There are still a bunch of TODOs, but we got Density's color rename committed and passing in the migration engine.
The most important next step is to use a bit less hard coded logic about checking phetioID when its a string. Perhaps by using the API files' stateSchema.
I moved the StringUnion problem to a new issue https://github.com/phetsims/tandem/issues/306. As far as I can tell this issue is close to closing. @zepumph what do you think?
@zepumph and I finished this up today, nice work team!
We have an inconsistency with where colors (instrumented ProfileColorProperty elements) appear in the Studio tree.
cck-dc, cck-dc-virtual-lab, density, and calculus-grapher have them under
global.view.colorProfile
, because they have this duplicated in their *Colors.ts file:GAO and solar-system-common have them under
global.view.colors
because they are usingTandem.COLORS
. I suspect that the solar-system-common occurrences are not acutally used in any PhET-iO sims.Tandem.COLORS
was added by @samreid in https://github.com/phetsims/tandem/commit/f93fc8e1db9c3757dd34372c0b1ae156bf96c17b.Questions:
This blocks Acid-Base Solutions 1.3, where we need to instrument pH colors. It should presumably block publication of any other PhET-iO sims that have instrumented colors.