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

Add Migration Processors for material and gravity mutability changes #274

Closed
samreid opened this issue Jul 19, 2024 · 5 comments
Closed
Assignees

Comments

@samreid
Copy link
Member

samreid commented Jul 19, 2024

From #256

@zepumph
Copy link
Member

zepumph commented Jul 19, 2024

We want to unblock migration wrapper testing for new problems.

@zepumph
Copy link
Member

zepumph commented Jul 19, 2024

Subject: [PATCH] one phetioID please, https://github.com/phetsims/density-buoyancy-common/issues/274
---
Index: repos/density/js/density-migration-processors.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/repos/density/js/density-migration-processors.ts b/repos/density/js/density-migration-processors.ts
--- a/repos/density/js/density-migration-processors.ts	(revision f96c906241b303dc75a598727122ad54cd6e3688)
+++ b/repos/density/js/density-migration-processors.ts	(date 1721418365033)
@@ -529,7 +529,23 @@
 
     // Uninstrumenting internal number control properties, see https://github.com/phetsims/density-buoyancy-common/issues/259
     new DeleteUninstrumentedPhetioElement( 'density.introScreen.view.blockAPanel.volumeNumberControl.numberControlVolumeProperty.rangeProperty' ),
-    new DeleteUninstrumentedPhetioElement( 'density.introScreen.view.blockBPanel.volumeNumberControl.numberControlVolumeProperty.rangeProperty' )
+    new DeleteUninstrumentedPhetioElement( 'density.introScreen.view.blockBPanel.volumeNumberControl.numberControlVolumeProperty.rangeProperty' ),
+
+    new ChangeIOType(
+      'PropertyIO<MaterialIO>', 'PropertyIO<ReferenceIO<ObjectIO>>', ( state, currentPhetioID ) => {
+        if ( !( !state.value?.identifier || state.value?.identifier === 'CUSTOM' ) ) {
+
+          const subSection = state.value?.identifier === 'WATER' ? 'fluids' : 'solids';
+          state.value = {
+            phetioID: `density.global.model.materials.${subSection}.${_.camelCase( state.value.identifier )}`
+          };
+        }
+        else {
+          state.value = {
+            phetioID: `${currentPhetioID}.customMaterial`
+          };
+        }
+      } )
   ]
 };
 

@samreid
Copy link
Member Author

samreid commented Jul 19, 2024

Current patch:

Subject: [PATCH] Remove {}, see https://github.com/phetsims/density-buoyancy-common/issues/274
---
Index: js/common/model/Cube.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Cube.ts b/js/common/model/Cube.ts
--- a/js/common/model/Cube.ts	(revision 9b381d1345323fa8fa2ed3d7c051ca0ad882e528)
+++ b/js/common/model/Cube.ts	(date 1721423110780)
@@ -77,7 +77,8 @@
   /**
    * Creates a Cube with a defined volume
    */
-  public static createWithVolume( engine: PhysicsEngine, material: Material, position: Vector2, volume: number, options?: StrictOmit<CubeOptions, 'matrix' | 'material'> ): Cube {
+  // TODO: MK wants a type for this
+  public static createWithVolume( engine: PhysicsEngine, material: Material | 'CUSTOM', position: Vector2, volume: number, options?: StrictOmit<CubeOptions, 'matrix' | 'material'> ): Cube {
     return new Cube( engine, volume, combineOptions<CubeOptions>( {
       matrix: Matrix3.translation( position.x, position.y ),
       minVolume: Cuboid.MIN_VOLUME,
Index: js/buoyancy/model/applications/Bottle.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/model/applications/Bottle.ts b/js/buoyancy/model/applications/Bottle.ts
--- a/js/buoyancy/model/applications/Bottle.ts	(revision 9b381d1345323fa8fa2ed3d7c051ca0ad882e528)
+++ b/js/buoyancy/model/applications/Bottle.ts	(date 1721422815664)
@@ -225,13 +225,18 @@
 
     const materialInsideTandem = options.tandem.createTandem( 'materialInside' );
 
-    this.materialInsideProperty = new MaterialProperty( BOTTLE_INITIAL_INTERIOR_MATERIAL, tandem => new CustomSolidMaterial( tandem, {
+
+    const materialInsidePropertyTandem = materialInsideTandem.createTandem( 'materialProperty' );
+
+    const customInsideMaterial = new CustomSolidMaterial( materialInsidePropertyTandem.createTandem( 'customMaterial' ), {
       density: BOTTLE_INITIAL_INTERIOR_MATERIAL.density,
       densityRange: new Range( 50, 20000 )
-    } ), {
+    } );
+
+    this.materialInsideProperty = new MaterialProperty( BOTTLE_INITIAL_INTERIOR_MATERIAL, customInsideMaterial, {
       valueType: Material,
       reentrant: true,
-      tandem: materialInsideTandem.createTandem( 'materialProperty' ),
+      tandem: materialInsidePropertyTandem,
       phetioValueType: ReferenceIO( IOType.ObjectIO )
     } );
 
Index: js/common/model/MaterialProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/MaterialProperty.ts b/js/common/model/MaterialProperty.ts
--- a/js/common/model/MaterialProperty.ts	(revision 9b381d1345323fa8fa2ed3d7c051ca0ad882e528)
+++ b/js/common/model/MaterialProperty.ts	(date 1721422745252)
@@ -13,7 +13,6 @@
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
 import MappedWrappedProperty from './MappedWrappedProperty.js';
 import { combineOptions, EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
-import Tandem from '../../../../tandem/js/Tandem.js';
 import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
 import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js';
 
@@ -25,8 +24,8 @@
   public readonly densityProperty: TReadOnlyProperty<number>;
   public readonly customMaterial: Material;
 
-  public constructor( material: Material, createCustomMaterial: ( tandem: Tandem ) => Material, providedOptions: MaterialPropertyOptions ) {
-    const customMaterial = createCustomMaterial( providedOptions.tandem.createTandem( 'customMaterial' ) );
+  // Note the material could be the customMaterial
+  public constructor( material: Material, customMaterial: Material, providedOptions: MaterialPropertyOptions ) {
     super( material, customMaterial, combineOptions<MaterialPropertyOptions>( {
       phetioFeatured: true
     }, providedOptions ) );
Index: js/common/model/Mass.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Mass.ts b/js/common/model/Mass.ts
--- a/js/common/model/Mass.ts	(revision 9b381d1345323fa8fa2ed3d7c051ca0ad882e528)
+++ b/js/common/model/Mass.ts	(date 1721423331094)
@@ -27,7 +27,7 @@
 import IOType from '../../../../tandem/js/types/IOType.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import InterpolatedProperty from './InterpolatedProperty.js';
-import Material, { CustomSolidMaterial } from './Material.js';
+import Material, { CustomSolidMaterial, MaterialOptions } from './Material.js';
 import PhysicsEngine, { PhysicsEngineBody } from './PhysicsEngine.js';
 import Basin from './Basin.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
@@ -54,7 +54,7 @@
   // Required
   body: PhysicsEngineBody;
   shape: Shape;
-  material: Material;
+  material: Material | 'CUSTOM';
   volume: number;
   massShape: MassShape;
 
@@ -76,6 +76,7 @@
   materialPropertyOptions?: PropertyOptions<Material>;
   volumePropertyOptions?: NumberPropertyOptions;
   massPropertyOptions?: NumberPropertyOptions;
+  customMaterialOptions?: MaterialOptions;
 
   minVolume?: number;
   maxVolume?: number;
@@ -203,7 +204,8 @@
       volumePropertyOptions: {},
       massPropertyOptions: {},
       minVolume: 0,
-      maxVolume: Number.POSITIVE_INFINITY
+      maxVolume: Number.POSITIVE_INFINITY,
+      customMaterialOptions: {}
     }, providedOptions );
 
     assert && assert( options.body, 'options.body required' );
@@ -245,16 +247,20 @@
 
     this.visibleProperty = new GatedVisibleProperty( this.internalVisibleProperty, tandem );
 
-    this.materialProperty = new MaterialProperty( options.material, tandem => new CustomSolidMaterial( tandem, {
-      density: options.material.density,
+    const materialPropertyTandem = tandem?.createTandem( 'materialProperty' );
+
+    const customSolidMaterial = new CustomSolidMaterial( materialPropertyTandem?.createTandem( 'customMaterial' ), combineOptions<MaterialOptions>( {
+      density: options.material === 'CUSTOM' ? undefined : options.material.density
 
       // TODO: It is incorrect to take the range of the default value, this affects the color, see https://github.com/phetsims/density-buoyancy-common/issues/268
-      densityRange: options.material.densityProperty.range
-    } ), combineOptions<PropertyOptions<Material> & PickRequired<PhetioObjectOptions, 'tandem'>>( {
+      // densityRange: options.material.densityProperty.range
+    }, options.customMaterialOptions ) );
+
+    this.materialProperty = new MaterialProperty( options.material === 'CUSTOM' ? customSolidMaterial : options.material, customSolidMaterial, combineOptions<PropertyOptions<Material> & PickRequired<PhetioObjectOptions, 'tandem'>>( {
       valueType: Material,
       reentrant: true,
 
-      tandem: tandem?.createTandem( 'materialProperty' ),
+      tandem: materialPropertyTandem,
       phetioValueType: ReferenceIO( IOType.ObjectIO ),
       phetioFeatured: true
     }, options.materialPropertyOptions ) );
Index: js/common/model/Pool.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Pool.ts b/js/common/model/Pool.ts
--- a/js/common/model/Pool.ts	(revision 9b381d1345323fa8fa2ed3d7c051ca0ad882e528)
+++ b/js/common/model/Pool.ts	(date 1721422890471)
@@ -57,13 +57,17 @@
     this.stepBottom = bounds.minY;
     this.stepTop = bounds.maxY;
 
-    this.fluidMaterialProperty = new MaterialProperty( Material.WATER, tandem => new CustomLiquidMaterial( tandem, {
+    const fluidMaterialPropertyTandem = this.fluidTandem.createTandem( 'materialProperty' );
+
+    const customFluidMaterial = new CustomLiquidMaterial( fluidMaterialPropertyTandem.createTandem( 'customMaterial' ), {
       density: Material.WATER.density,
       densityRange: DensityBuoyancyCommonConstants.FLUID_DENSITY_RANGE_PER_M3
-    } ), {
+    } );
+
+    this.fluidMaterialProperty = new MaterialProperty( Material.WATER, customFluidMaterial, {
       valueType: Material,
       phetioValueType: ReferenceIO( IOType.ObjectIO ),
-      tandem: this.fluidTandem.createTandem( 'materialProperty' ),
+      tandem: fluidMaterialPropertyTandem,
       phetioDocumentation: 'The material of the fluid in the pool'
     } );
 
Index: js/buoyancy/model/shapes/BuoyancyShapesModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/model/shapes/BuoyancyShapesModel.ts b/js/buoyancy/model/shapes/BuoyancyShapesModel.ts
--- a/js/buoyancy/model/shapes/BuoyancyShapesModel.ts	(revision 9b381d1345323fa8fa2ed3d7c051ca0ad882e528)
+++ b/js/buoyancy/model/shapes/BuoyancyShapesModel.ts	(date 1721422841775)
@@ -57,7 +57,7 @@
     this.materialProperty = new MaterialProperty( Material.WOOD,
 
       // This hack is a way of saying, we do not create or support a custom material in this case.
-      () => Material.WOOD, {
+      Material.WOOD, {
         tandem: objectsTandem.createTandem( 'materialProperty' ),
         phetioValueType: ReferenceIO( IOType.ObjectIO )
       } );
Index: js/density/model/DensityMysteryModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/density/model/DensityMysteryModel.ts b/js/density/model/DensityMysteryModel.ts
--- a/js/density/model/DensityMysteryModel.ts	(revision 9b381d1345323fa8fa2ed3d7c051ca0ad882e528)
+++ b/js/density/model/DensityMysteryModel.ts	(date 1721423560644)
@@ -27,9 +27,9 @@
 import DensityBuoyancyCommonStrings from '../../DensityBuoyancyCommonStrings.js';
 import LocalizedStringProperty from '../../../../chipper/js/LocalizedStringProperty.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
+import { ColorProperty } from '../../../../scenery/js/imports.js';
 
 // constants
-const randomMaterials = Material.DENSITY_MYSTERY_MATERIALS;
 const randomColors = [
   DensityBuoyancyCommonColors.compareYellowColorProperty,
   DensityBuoyancyCommonColors.compareBlueColorProperty,
@@ -78,17 +78,32 @@
       adjustableMaterial: true
     };
 
-    // TODO AV: this should just set density/color on 5 references that live forever. https://github.com/phetsims/density-buoyancy-common/issues/256
-    const createMysteryMaterials = () => {
-      const densities = dotRandom.shuffle( randomMaterials ).slice( 0, 5 ).map( material => material.density );
-      const colors = dotRandom.shuffle( randomColors ).slice( 0, 5 );
+    const densities = dotRandom.shuffle( Material.DENSITY_MYSTERY_MATERIALS ).slice( 0, 5 ).map( material => material.density );
+
 
-      // TODO AV: Specify the correct tandems throughout this file? See https://github.com/phetsims/density-buoyancy-common/issues/123
-      return _.range( 0, 5 ).map( i => Material.createCustomMaterial( Tandem.OPT_OUT, {
-        density: densities[ i ],
-        colorProperty: colors[ i ]
-      } ) );
-    };
+    // const mysteryMaterialsAndColors = _.range( 0, 5 ).map( i => {
+    //
+    //   const mysteryMaterialTandem = tandem.createTandem( `mysteryMaterial${i + 1}` );
+    //   const colorProperty = new ColorProperty( colors[ i ].value, {
+    //     tandem: mysteryMaterialTandem.createTandem( 'colorProperty' )
+    //   } );
+    //
+    //   const material = Material.createCustomMaterial( mysteryMaterialTandem, {
+    //     density: densities[ i ],
+    //     colorProperty: colorProperty
+    //   } );
+    //   return { material: material, colorProperty: colorProperty };
+    // } );
+
+    // const randomizeMysteryMaterials = () => {
+    //   const densities = dotRandom.shuffle( Material.DENSITY_MYSTERY_MATERIALS ).slice( 0, 5 ).map( material => material.density );
+    //   const colors = dotRandom.shuffle( randomColors ).slice( 0, 5 );
+    //
+    //   // mysteryMaterialsAndColors.forEach( ( material, i ) => {
+    //   //   material.material.densityProperty.value = densities[ i ];
+    //   //   material.colorProperty.value = colors[ i ].value;
+    //   // } );
+    // };
     const createMysteryVolumes = () => {
       return [
         // we will want 3 smaller masses on the right, then 2 larger masses on the left
@@ -285,17 +300,48 @@
             MassTag.B
           ];
 
-          const mysteryMaterials = createMysteryMaterials();
           const mysteryVolumes = createMysteryVolumes();
 
-          return _.range( 0, 5 ).map( i => {
-            return Cube.createWithVolume( model.engine, mysteryMaterials[ i ], Vector2.ZERO, mysteryVolumes[ i ], {
+          const cubes = _.range( 0, 5 ).map( i => {
+
+
+            // const mysteryMaterialTandem = tandem.createTandem( `mysteryMaterial${i + 1}` );
+            // const colorProperty = new ColorProperty( colors[ i ].value, {
+            //   tandem: mysteryMaterialTandem.createTandem( 'colorProperty' )
+            // } );
+
+            // const material = Material.createCustomMaterial( mysteryMaterialTandem, {
+            //   density: densities[ i ],
+            //   colorProperty: colorProperty
+            // } );
+            // return { material: material, colorProperty: colorProperty };
+
+            const cubeTandem = randomTandem.createTandem( `block${tags[ i ].tandemName}` );
+
+            const colors = dotRandom.shuffle( randomColors ).slice( 0, 5 );
+            const colorProperty = new ColorProperty( colors[ i ].value, {
+              tandem: cubeTandem.createTandem( 'colorProperty' )
+            } );
+
+            // TODO: What about the initial material? What about resetting? help.
+
+            const cube = Cube.createWithVolume( model.engine, 'CUSTOM', Vector2.ZERO, mysteryVolumes[ i ], {
               adjustVolumeOnMassChanged: true,
               adjustableMaterial: true,
               tag: tags[ i ],
-              tandem: randomTandem.createTandem( `block${tags[ i ].tandemName}` )
+              tandem: cubeTandem,
+              customMaterialOptions: {
+                colorProperty: colorProperty,
+                density: densities[ i ]
+              }
             } );
+
+            return cube;
           } );
+
+          // TODO: recycle colors etc.
+
+          return cubes;
         }
         default:
           throw new Error( `unknown blockSet: ${blockSet}` );
@@ -303,15 +349,15 @@
     };
 
     const regenerateMasses = ( blockSet: MysteryBlockSet, masses: Cuboid[] ) => {
-      if ( blockSet === MysteryBlockSet.RANDOM ) {
-        const mysteryMaterials = createMysteryMaterials();
-        const mysteryVolumes = createMysteryVolumes();
+
+      assert && assert( blockSet === MysteryBlockSet.RANDOM, 'Unexpected blockSet', blockSet );
+
+      // randomizeMysteryMaterials();
+      const mysteryVolumes = createMysteryVolumes();
 
-        masses.forEach( ( mass, i ) => {
-          mass.materialProperty.value = mysteryMaterials[ i ];
-          mass.updateSize( Cube.boundsFromVolume( mysteryVolumes[ i ] ) );
-        } );
-      }
+      masses.forEach( ( mass, i ) => {
+        mass.updateSize( Cube.boundsFromVolume( mysteryVolumes[ i ] ) );
+      } );
     };
 
     const positionMasses = ( model: DensityBuoyancyModel, blockSet: MysteryBlockSet, masses: Cuboid[] ) => {

@zepumph
Copy link
Member

zepumph commented Jul 22, 2024

  • custom density ranges on the mystery screen are getting constrained to their old value, 150-23000, but we are now .8-27000. Problem?

zepumph added a commit to phetsims/phetmarks that referenced this issue Jul 22, 2024
@samreid
Copy link
Member Author

samreid commented Jul 22, 2024

@zepumph and I finished this up today, nice work. 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

2 participants