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

Preallocate all Material instances and make the density mutable #256

Closed
samreid opened this issue Jul 10, 2024 · 16 comments
Closed

Preallocate all Material instances and make the density mutable #256

samreid opened this issue Jul 10, 2024 · 16 comments
Assignees
Labels
dev:help-wanted Extra attention is needed priority:2-high

Comments

@samreid
Copy link
Member

samreid commented Jul 10, 2024

The simulation currently has multiple ways to manage the material of a solid or fluid. It is managed by a named enumeration plus a Material instance. In some, but not all cases, there is a 2-way binding. Custom materials have a separate outer Property to manage creating new Materials when the density changes. This has led to problems such as #176, as well as #250 as well as generally making the simulation very difficult to navigate, understand and maintain. We should attempt to preallocate all Material instances (including the custom ones) and make the density a densityProperty on the Materials in order to solve this family of problems and make the simulation much less complex in general.

@samreid
Copy link
Member Author

samreid commented Jul 10, 2024

From collaboration with @zepumph

Subject: [PATCH] Require providedOptions in optionize, see https://github.com/phetsims/phet-core/issues/134
---
Index: js/common/view/MaterialControlNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MaterialControlNode.ts b/js/common/view/MaterialControlNode.ts
--- a/js/common/view/MaterialControlNode.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/view/MaterialControlNode.ts	(date 1720637112824)
@@ -8,13 +8,10 @@
  * @author Jonathan Olson (PhET Interactive Simulations)
  */
 
-import DynamicProperty from '../../../../axon/js/DynamicProperty.js';
 import Property from '../../../../axon/js/Property.js';
-import Utils from '../../../../dot/js/Utils.js';
 import optionize from '../../../../phet-core/js/optionize.js';
-import { HBox, Node, Text, VBox, VBoxOptions } from '../../../../scenery/js/imports.js';
+import { Color, ColorProperty, HBox, Node, Text, VBox, VBoxOptions } from '../../../../scenery/js/imports.js';
 import ComboBox from '../../../../sun/js/ComboBox.js';
-import StringIO from '../../../../tandem/js/types/StringIO.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import DensityBuoyancyCommonStrings from '../../DensityBuoyancyCommonStrings.js';
 import DensityBuoyancyCommonConstants from '../DensityBuoyancyCommonConstants.js';
@@ -82,33 +79,6 @@
     const materialNames: MaterialName[] = [ ...materials.map( material => material.identifier ) ];
     options.supportCustomMaterial && materialNames.push( 'CUSTOM' );
 
-    // TODO: https://github.com/phetsims/density-buoyancy-common/issues/163 re-evaluate eliminating or moving this code
-    //       after addressing #163
-    const comboBoxMaterialProperty = new DynamicProperty( new Property( materialProperty ), {
-      bidirectional: true,
-      map: ( material: Material ) => material.identifier,
-      inverseMap: ( materialName: MaterialName ): Material => {
-        if ( materialName === 'CUSTOM' ) {
-          // Handle our minimum volume if we're switched to custom (if needed)
-          const volume = Math.max( volumeProperty.value, options.minCustomVolumeLiters / DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER );
-          return Material.createCustomSolidMaterial( {
-            density: Utils.clamp( materialProperty.value.density, options.minCustomMass / volume, options.maxCustomMass / volume ),
-            densityRange: this.customDensityRange
-          } );
-        }
-        else {
-          return Material.getMaterial( materialName );
-        }
-      },
-      reentrant: true,
-      phetioState: false,
-      tandem: options.tandem.createTandem( 'comboBoxMaterialProperty' ),
-      phetioFeatured: true,
-      phetioDocumentation: 'Current material of the block. Changing the material will result in changes to the mass, but the volume will remain the same.',
-      validValues: materialNames,
-      phetioValueType: StringIO
-    } );
-
     const comboMaxWidth = 110;
 
     const regularMaterials = materials.filter( material => !material.hidden );
@@ -116,7 +86,7 @@
 
     const materialToItem = ( material: Material ) => {
       return {
-        value: material.identifier,
+        value: material,
         createNode: () => new Text( material.nameProperty, {
           font: DensityBuoyancyCommonConstants.COMBO_BOX_ITEM_FONT,
           maxWidth: comboMaxWidth
@@ -125,12 +95,14 @@
         a11yName: material.nameProperty
       };
     };
+    const customMaterial = Material.createCustomMaterial( {
+      customColor: new ColorProperty( new Color( 'green' ) )
+    } );
 
-    // TODO: parametrize to MaterialName, https://github.com/phetsims/density-buoyancy-common/issues/176
-    const comboBox = new ComboBox( comboBoxMaterialProperty, [
+    const comboBox = new ComboBox( materialProperty, [
       ...regularMaterials.map( materialToItem ),
       ...( options.supportCustomMaterial ? [ {
-        value: 'CUSTOM',
+        value: customMaterial,
         createNode: () => new Text( DensityBuoyancyCommonStrings.material.customStringProperty, {
           font: DensityBuoyancyCommonConstants.COMBO_BOX_ITEM_FONT,
           maxWidth: comboMaxWidth
Index: js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts b/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts
--- a/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts	(date 1720649883713)
@@ -105,6 +105,11 @@
     this.transformEmitter.addListener( this.positionResetSceneButton );
     this.positionResetSceneButton();
 
+    const customMaterialInside = Material.createCustomSolidMaterial( {
+      density: 1000,
+      densityRange: new Range( 0, 1000 )
+    } );
+
     const bottleControlsTandem = tandem.createTandem( 'bottleControls' );
     const materialInsideControlsTandem = bottleControlsTandem.createTandem( 'materialInsideControls' );
     const materialInsideControls = new MaterialMassVolumeControlNode( model.bottle.materialInsideProperty, model.bottle.interiorMassProperty, model.bottle.materialInsideVolumeProperty, [
@@ -138,23 +143,20 @@
 
     let materialChangeLocked = false;
     Multilink.lazyMultilink( [
-      model.bottle.customDensityProperty,
-      model.bottle.customDensityProperty.rangeProperty,
+      // model.bottle.customDensityProperty,
+      // model.bottle.customDensityProperty.rangeProperty,
       model.bottle.interiorMassProperty,
       customDensityControlVisibleProperty
     ], ( density, densityRange ) => {
       if ( !materialChangeLocked && model.bottle.materialInsideProperty.value.custom ) {
         materialChangeLocked = true;
-        model.bottle.materialInsideProperty.value = Material.createCustomSolidMaterial( {
-          density: density * DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER,
-          densityRange: densityRange.times( DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER )
-        } );
+        model.bottle.materialInsideProperty.value.densityProperty.value = density * DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER;
         materialChangeLocked = false;
       }
     } );
 
     const customBottleDensityControlTandem = materialInsideControlsTandem.createTandem( 'customBottleDensityNumberControl' );
-    const customBottleDensityControl = new NumberControl( DensityBuoyancyCommonStrings.densityStringProperty, model.bottle.customDensityProperty, model.bottle.customDensityProperty.range, combineOptions<NumberControlOptions>( {
+    const customBottleDensityControl = new NumberControl( DensityBuoyancyCommonStrings.densityStringProperty, customMaterialInside.densityProperty, customMaterialInside.densityProperty.range, combineOptions<NumberControlOptions>( {
       visibleProperty: new GatedVisibleProperty( customDensityControlVisibleProperty, customBottleDensityControlTandem ),
       sliderOptions: {
         accessibleName: DensityBuoyancyCommonStrings.densityStringProperty,
Index: js/common/view/MassLabelNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MassLabelNode.ts b/js/common/view/MassLabelNode.ts
--- a/js/common/view/MassLabelNode.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/view/MassLabelNode.ts	(date 1720650286242)
@@ -35,12 +35,14 @@
     this.readoutStringProperty = new DerivedProperty(
       [
         mass.materialProperty,
+        mass.densityProperty,
         mass.volumeProperty,
         DensityBuoyancyCommonStrings.kilogramsPatternStringProperty,
         DensityBuoyancyCommonStrings.questionMarkStringProperty
       ],
       (
         material,
+        density,
         volume,
         patternStringProperty,
         questionMarkString
@@ -49,7 +51,7 @@
                questionMarkString :
                StringUtils.fillIn( patternStringProperty, {
                  // Deriving the mass instead of using massProperty to avoid including the contained mass, for the case of the boat
-                 kilograms: Utils.toFixed( volume * material.density, 2 ),
+                 kilograms: Utils.toFixed( volume * density, 2 ),
                  decimalPlaces: 2
                } );
       } );
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 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/model/Mass.ts	(date 1720650148617)
@@ -27,7 +27,7 @@
 import IOType from '../../../../tandem/js/types/IOType.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import InterpolatedProperty from './InterpolatedProperty.js';
-import Material, { CreateCustomMaterialOptions, MaterialName } from './Material.js';
+import Material from './Material.js';
 import PhysicsEngine, { PhysicsEngineBody } from './PhysicsEngine.js';
 import Basin from './Basin.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
@@ -41,7 +41,7 @@
 import BlendedVector2Property from './BlendedVector2Property.js';
 import { GuardedNumberProperty, GuardedNumberPropertyOptions } from './GuardedNumberProperty.js';
 import DensityBuoyancyCommonConstants from '../DensityBuoyancyCommonConstants.js';
-import StringUnionProperty from '../../../../axon/js/StringUnionProperty.js';
+import DynamicProperty from '../../../../axon/js/DynamicProperty.js';
 
 // For the Buoyancy Shapes screen, but needed here because setRatios is included in each core type
 // See https://github.com/phetsims/buoyancy/issues/29
@@ -77,7 +77,7 @@
   massPropertyOptions?: NumberPropertyOptions;
 
   // Accepted values for the Material Combo Box
-  materialEnumPropertyValidValues?: MaterialName[];
+  materialValidValues?: Material[];
 
   minVolume?: number;
   maxVolume?: number;
@@ -119,10 +119,10 @@
   public readonly materialProperty: Property<Material>;
 
   // for phet-io support (to control the materialProperty)
-  public readonly materialEnumProperty?: StringUnionProperty<MaterialName>;
+  // public readonly materialEnumProperty?: StringUnionProperty<MaterialName>;
 
   // for phet-io support (to control the materialProperty)
-  public readonly customDensityProperty?: NumberProperty;
+  // public readonly customDensityProperty?: NumberProperty;
 
   // for phet-io support (to control the materialProperty)
   private readonly customColorProperty?: Property<Color>;
@@ -195,6 +195,7 @@
   public stepTop: number; // maximum y value of the mass
 
   public readonly adjustableMaterial: boolean;
+  public readonly densityProperty: DynamicProperty<number, number, Material>;
 
   protected constructor( engine: PhysicsEngine, providedOptions: MassOptions ) {
 
@@ -213,16 +214,22 @@
       volumePropertyOptions: {},
       massPropertyOptions: {},
 
-      materialEnumPropertyValidValues: [
-        'ALUMINUM',
-        'BRICK',
-        'COPPER',
-        'ICE',
-        'PLATINUM',
-        'STEEL',
-        'STYROFOAM',
-        'WOOD',
-        'CUSTOM'
+      // TODO: Make the default an empty list and move this list to density's mystery screen
+      // TODO: Should all masses be created with their valid materials immediately?
+      // TODO: Move the materialValidValues over to the view, and let each materialProperty be able to take on any material.
+      materialValidValues: [
+        Material.ALUMINUM,
+        Material.BRICK,
+        Material.COPPER,
+        Material.ICE,
+        Material.PLATINUM,
+        Material.STEEL,
+        Material.STYROFOAM,
+        Material.WOOD,
+        Material.createCustomSolidMaterial( {
+          density: 1000,
+          customColor: new ColorProperty( Color.white )
+        } )
       ],
 
       minVolume: 0,
@@ -276,86 +283,91 @@
       phetioFeatured: true
     }, options.materialPropertyOptions ) );
 
+    this.densityProperty = new DynamicProperty<number, number, Material>( this.materialProperty, {
+      bidirectional: false,
+      derive: material => material.densityProperty
+    } );
+
     this.adjustableMaterial = options.adjustableMaterial;
 
     if ( options.adjustableMaterial ) {
-      this.materialEnumProperty = new StringUnionProperty<MaterialName>( options.material.identifier, {
-        tandem: tandem?.createTandem( 'materialEnumProperty' ),
-        validValues: options.materialEnumPropertyValidValues,
-        phetioFeatured: true,
-        phetioDocumentation: 'Current material of the object. Changing the material will result in changes to the mass, but the volume will remain the same.'
-      } );
-      this.customDensityProperty = new NumberProperty( options.material.density, {
-        tandem: tandem?.createTandem( 'customDensityProperty' ),
-        phetioFeatured: true,
-        phetioDocumentation: 'Density of the object when the material is set to “CUSTOM”.',
-        range: new Range( 150, 23000 ),
-        units: 'kg/m^3'
-      } );
-      this.customColorProperty = new ColorProperty( options.material.customColor ? options.material.customColor.value : Color.WHITE, {
-        tandem: options.adjustableColor ? tandem?.createTandem( 'customColorProperty' ) : Tandem.OPT_OUT,
-        phetioFeatured: true
-      } );
-
-      this.materialProperty.addPhetioStateDependencies( [ this.materialEnumProperty, this.customDensityProperty, this.customColorProperty ] );
-
-      // Hook up phet-io Properties for interoperation with the normal ones
-      let enumLock = false;
-      let densityLock = false;
-      let colorLock = false;
-      const colorListener = ( color: Color ) => {
-        if ( !colorLock ) {
-          colorLock = true;
-          this.customColorProperty!.value = color;
-          colorLock = false;
-        }
-      };
-      this.materialProperty.link( ( material, oldMaterial ) => {
-        if ( !enumLock ) {
-          enumLock = true;
-          this.materialEnumProperty!.value = material.identifier;
-          enumLock = false;
-        }
-        if ( !densityLock ) {
-          densityLock = true;
-          this.customDensityProperty!.value = material.density;
-          densityLock = false;
-        }
-        if ( oldMaterial && oldMaterial.customColor ) {
-          oldMaterial.customColor.unlink( colorListener );
-        }
-        if ( material && material.customColor ) {
-          material.customColor.link( colorListener );
-        }
-      } );
-
-      Multilink.lazyMultilink( [ this.materialEnumProperty, this.customDensityProperty, this.customColorProperty ], materialEnum => {
-        // See if it's an external change
-        if ( !enumLock && !densityLock && !colorLock ) {
-          enumLock = true;
-          densityLock = true;
-          colorLock = true;
-          if ( materialEnum === 'CUSTOM' ) {
-            const createMaterialOptions: CreateCustomMaterialOptions = {
-              density: this.customDensityProperty!.value
-            };
-            if ( options.adjustableColor ) {
-              createMaterialOptions.customColor = this.customColorProperty;
-            }
-            else {
-              createMaterialOptions.densityRange = this.customDensityProperty!.range;
-            }
-
-            this.materialProperty.value = Material.createCustomSolidMaterial( createMaterialOptions );
-          }
-          else {
-            this.materialProperty.value = Material.getMaterial( materialEnum );
-          }
-          enumLock = false;
-          densityLock = false;
-          colorLock = false;
-        }
-      } );
+      // this.materialEnumProperty = new StringUnionProperty<MaterialName>( options.material.identifier, {
+      //   tandem: tandem?.createTandem( 'materialEnumProperty' ),
+      //   validValues: options.materialEnumPropertyValidValues,
+      //   phetioFeatured: true,
+      //   phetioDocumentation: 'Current material of the object. Changing the material will result in changes to the mass, but the volume will remain the same.'
+      // } );
+      // this.customDensityProperty = new NumberProperty( options.material.density, {
+      //   tandem: tandem?.createTandem( 'customDensityProperty' ),
+      //   phetioFeatured: true,
+      //   phetioDocumentation: 'Density of the object when the material is set to “CUSTOM”.',
+      //   range: new Range( 150, 23000 ),
+      //   units: 'kg/m^3'
+      // } );
+      // this.customColorProperty = new ColorProperty( options.material.customColor ? options.material.customColor.value : Color.WHITE, {
+      //   tandem: options.adjustableColor ? tandem?.createTandem( 'customColorProperty' ) : Tandem.OPT_OUT,
+      //   phetioFeatured: true
+      // } );
+      //
+      // this.materialProperty.addPhetioStateDependencies( [ this.materialEnumProperty, this.customDensityProperty, this.customColorProperty ] );
+      //
+      // // Hook up phet-io Properties for interoperation with the normal ones
+      // let enumLock = false;
+      // let densityLock = false;
+      // let colorLock = false;
+      // const colorListener = ( color: Color ) => {
+      //   if ( !colorLock ) {
+      //     colorLock = true;
+      //     this.customColorProperty!.value = color;
+      //     colorLock = false;
+      //   }
+      // };
+      // this.materialProperty.link( ( material, oldMaterial ) => {
+      //   if ( !enumLock ) {
+      //     enumLock = true;
+      //     this.materialEnumProperty!.value = material.identifier;
+      //     enumLock = false;
+      //   }
+      //   if ( !densityLock ) {
+      //     densityLock = true;
+      //     this.customDensityProperty!.value = material.density;
+      //     densityLock = false;
+      //   }
+      //   if ( oldMaterial && oldMaterial.customColor ) {
+      //     oldMaterial.customColor.unlink( colorListener );
+      //   }
+      //   if ( material && material.customColor ) {
+      //     material.customColor.link( colorListener );
+      //   }
+      // } );
+      //
+      // Multilink.lazyMultilink( [ this.materialEnumProperty, this.customDensityProperty, this.customColorProperty ], materialEnum => {
+      //   // See if it's an external change
+      //   if ( !enumLock && !densityLock && !colorLock ) {
+      //     enumLock = true;
+      //     densityLock = true;
+      //     colorLock = true;
+      //     if ( materialEnum === 'CUSTOM' ) {
+      //       const createMaterialOptions: CreateCustomMaterialOptions = {
+      //         density: this.customDensityProperty!.value
+      //       };
+      //       if ( options.adjustableColor ) {
+      //         createMaterialOptions.customColor = this.customColorProperty;
+      //       }
+      //       else {
+      //         createMaterialOptions.densityRange = this.customDensityProperty!.range;
+      //       }
+      //
+      //       this.materialProperty.value = Material.createCustomSolidMaterial( createMaterialOptions );
+      //     }
+      //     else {
+      //       this.materialProperty.value = Material.getMaterial( materialEnum );
+      //     }
+      //     enumLock = false;
+      //     densityLock = false;
+      //     colorLock = false;
+      //   }
+      // } );
     }
 
     this.volumeProperty = new NumberProperty( options.volume, combineOptions<NumberPropertyOptions>( {
@@ -404,11 +416,10 @@
       }
     }, options.massPropertyOptions ) );
 
-    Multilink.multilink( [ this.materialProperty, this.volumeProperty, this.containedMassProperty ], ( material, volume, containedMass ) => {
+    Multilink.multilink( [ this.materialProperty, this.volumeProperty, this.containedMassProperty, this.densityProperty ], ( material, volume, containedMass, density ) => {
 
-      const selfMass = Utils.roundToInterval( material.density * volume, DensityBuoyancyCommonConstants.TOLERANCE );
+      const selfMass = Utils.roundToInterval( density * volume, DensityBuoyancyCommonConstants.TOLERANCE );
       this.massProperty.value = selfMass + containedMass;
-
     } );
 
     this.bodyOffsetProperty = new Vector2Property( Vector2.ZERO, {
Index: js/common/model/CompareBlockSetModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CompareBlockSetModel.ts b/js/common/model/CompareBlockSetModel.ts
--- a/js/common/model/CompareBlockSetModel.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/model/CompareBlockSetModel.ts	(date 1720649706872)
@@ -20,7 +20,6 @@
 import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
 import Material from './Material.js';
 import Property from '../../../../axon/js/Property.js';
-import Tandem from '../../../../tandem/js/Tandem.js';
 import Cube, { CubeOptions } from './Cube.js';
 import merge from '../../../../phet-core/js/merge.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
@@ -159,8 +158,7 @@
       // don't need to be removed.
       return blockSet === BlockSet.SAME_MASS ?
              cubesData.map( cubeData => {
-               const cube = Cube.createWithMass( model.engine, cubeData.sameMassMaterialProperty.value, Vector2.ZERO,
-                 massProperty.value, cubeData.sameMassCubeOptions );
+               const cube = Cube.createWithMass( model.engine, cubeData.sameMassMaterialProperty.value, Vector2.ZERO, massProperty.value, cubeData.sameMassCubeOptions );
 
                cubeData.sameMassMaterialProperty.link( material => cube.materialProperty.set( material ) );
                massProperty.lazyLink( massValue => cubeData.sameMassDensityProperty.set( massValue / cube.volumeProperty.value ) );
@@ -243,36 +241,49 @@
    */
   private static createMaterialProperty( colorProperty: TReadOnlyProperty<Color>, densityProperty: TReadOnlyProperty<number>,
                                          blockSetValueChangedProperty: TReadOnlyProperty<boolean>, initialMaterials: Material[] ): TReadOnlyProperty<Material> {
-    return new DerivedProperty( [ colorProperty, densityProperty, blockSetValueChangedProperty ],
-      ( color, density, blockSetValueChanged ) => {
+
+    // Create and return a custom material with the modified color and density.
+    const myColorProperty = new DerivedProperty( [ colorProperty, densityProperty ], ( color, density ) => {
+      // myCustomMaterial.densityProperty.value = density;
+
+      // Calculate the lightness of the material based on its density.
+      const lightness = Material.getNormalizedLightness( densityProperty.value, COLOR_DENSITY_RANGE ); // 0-1
+
+      // Modify the color brightness based on the lightness.
+      const modifier = 0.1;
+      const rawValue = ( lightness * 2 - 1 ) * ( 1 - modifier ) + modifier;
+      const power = 0.7;
+
+      return colorProperty.value.colorUtilsBrightness( Math.sign( rawValue ) * Math.pow( Math.abs( rawValue ), power ) );
+    } );
+    const myCustomMaterial = Material.createCustomSolidMaterial( {
+      density: densityProperty.value,
+      customColor: myColorProperty
+    } );
+
+    // TODO: Pass the densityProperty directly into the custom one
+    densityProperty.link( density => {
+      myCustomMaterial.densityProperty.value = density;
+    } );
+    //
+    // // TODO: https://github.com/phetsims/density-buoyancy-common/issues/256 allow passing in the density property directly
+    // Multilink.multilink( [ colorProperty, densityProperty ], ( color, density ) => {
+    //
+    // } );
+
+    return new DerivedProperty( [ densityProperty, blockSetValueChangedProperty ], ( density, blockSetValueChanged ) => {
 
-        // If the block set value has not changed, attempt to use an initial material with the same density.
-        if ( !blockSetValueChanged ) {
-          for ( let i = 0; i < initialMaterials.length; i++ ) {
-            const material = initialMaterials[ i ];
-            if ( material.density === density ) {
-              return material;
-            }
-          }
-        }
-
-        // Calculate the lightness of the material based on its density.
-        const lightness = Material.getNormalizedLightness( density, COLOR_DENSITY_RANGE ); // 0-1
-
-        // Modify the color brightness based on the lightness.
-        const modifier = 0.1;
-        const rawValue = ( lightness * 2 - 1 ) * ( 1 - modifier ) + modifier;
-        const power = 0.7;
-        const modifiedColor = color.colorUtilsBrightness( Math.sign( rawValue ) * Math.pow( Math.abs( rawValue ), power ) );
-
-        // Create and return a custom material with the modified color and density.
-        return Material.createCustomSolidMaterial( {
-          density: density,
-          customColor: new Property( modifiedColor, { tandem: Tandem.OPT_OUT } )
-        } );
-      }, {
-        tandem: Tandem.OPT_OUT
-      } );
+      // If the block set value has not changed, attempt to use an initial material with the same density.
+      if ( !blockSetValueChanged ) {
+        for ( let i = 0; i < initialMaterials.length; i++ ) {
+          const material = initialMaterials[ i ];
+          if ( material.density === density ) {
+            return material;
+          }
+        }
+      }
+      return myCustomMaterial;
+    } );
   }
 }
 
Index: js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts b/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts
--- a/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts	(date 1720637112855)
@@ -12,11 +12,12 @@
 import Vector2 from '../../../../dot/js/Vector2.js';
 import Cube from '../../common/model/Cube.js';
 import DensityBuoyancyModel, { DensityBuoyancyModelOptions } from '../../common/model/DensityBuoyancyModel.js';
-import Material, { MaterialName } from '../../common/model/Material.js';
+import Material from '../../common/model/Material.js';
 import Scale, { DisplayType } from '../../common/model/Scale.js';
 import TwoBlockMode from '../../common/model/TwoBlockMode.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import MassTag from '../../common/model/MassTag.js';
+import { Color, ColorProperty } from '../../../../scenery/js/imports.js';
 
 type BuoyancyBasicsExploreModelOptions = DensityBuoyancyModelOptions;
 
@@ -38,12 +39,15 @@
     } );
 
     // B:B Explore has a more limited set of available materials.
-    const simpleMaterialsIdentifiers: MaterialName[] = Material.SIMPLE_MASS_MATERIALS.map( x => x.identifier ).concat( [ 'CUSTOM' ] );
+    // const simpleMaterialsIdentifiers: MaterialName[] = Material.SIMPLE_MASS_MATERIALS.map( x => x.identifier ).concat( [ 'CUSTOM' ] );
 
     this.massA = Cube.createWithMass( this.engine, Material.WOOD, new Vector2( -0.2, 0.2 ), 2, {
       tag: MassTag.OBJECT_A,
       adjustableMaterial: true,
-      materialEnumPropertyValidValues: simpleMaterialsIdentifiers,
+      materialValidValues: Material.SIMPLE_MASS_MATERIALS.concat( [ Material.createCustomSolidMaterial( {
+        density: 1000,
+        customColor: new ColorProperty( Color.RED )
+      } ) ] ),
       adjustableColor: false,
       tandem: blocksTandem.createTandem( 'blockA' )
     } );
@@ -51,7 +55,10 @@
     this.massB = Cube.createWithMass( this.engine, Material.ALUMINUM, new Vector2( 0.05, 0.35 ), 13.5, {
       tag: MassTag.OBJECT_B,
       adjustableMaterial: true,
-      materialEnumPropertyValidValues: simpleMaterialsIdentifiers,
+      materialValidValues: Material.SIMPLE_MASS_MATERIALS.concat( [ Material.createCustomSolidMaterial( {
+        density: 1000,
+        customColor: new ColorProperty( Color.RED )
+      } ) ] ),
       adjustableColor: false,
       tandem: blocksTandem.createTandem( 'blockB' ),
       visible: false
Index: js/buoyancy/view/DensityAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/DensityAccordionBox.ts b/js/buoyancy/view/DensityAccordionBox.ts
--- a/js/buoyancy/view/DensityAccordionBox.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/buoyancy/view/DensityAccordionBox.ts	(date 1720649237948)
@@ -18,13 +18,11 @@
 import DerivedStringProperty from '../../../../axon/js/DerivedStringProperty.js';
 import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
 
-type DensityReadoutType = TReadOnlyProperty<Material>;
-
-type ParentOptions = ReadoutListAccordionBoxOptions<DensityReadoutType>;
+type ParentOptions = ReadoutListAccordionBoxOptions<TReadOnlyProperty<Material>>;
 type SelfOptions = EmptySelfOptions;
 type DensityAccordionBoxOptions = SelfOptions & ParentOptions;
 
-export default class DensityAccordionBox extends ReadoutListAccordionBox<DensityReadoutType> {
+export default class DensityAccordionBox extends ReadoutListAccordionBox<TReadOnlyProperty<Material>> {
 
   public constructor( titleStringProperty: TReadOnlyProperty<string>, providedOptions?: DensityAccordionBoxOptions ) {
 
@@ -36,7 +34,7 @@
     super( titleStringProperty, options );
   }
 
-  public override generateReadoutData( materialProperty: DensityReadoutType ): ReadoutData {
+  public override generateReadoutData( materialProperty: TReadOnlyProperty<Material> ): ReadoutData {
 
     // Use DynamicProperty so that this name is updated based on the material AND material's name changing.
     const nameProperty = new DynamicProperty<string, string, Material>( materialProperty, {
@@ -48,7 +46,8 @@
       [
         materialProperty,
         DensityBuoyancyCommonConstants.KILOGRAMS_PER_VOLUME_PATTERN_STRING_PROPERTY,
-        DensityBuoyancyCommonStrings.questionMarkStringProperty
+        DensityBuoyancyCommonStrings.questionMarkStringProperty,
+        // myCustomProperty.densityProperty
       ],
       ( material, patternStringProperty, questionMarkString ) => {
         return material.hidden ?
@@ -56,6 +55,9 @@
                StringUtils.fillIn( patternStringProperty, {
 
                  // convert from kg/m^3 to kg/L
+                 // TODO: This needs to update when custom material density changes
+                 // Can do with a DynamicProperty that takes the value of density of the selected material
+                 // Or with DerivedProperty.deriveAny across all the validValues which are Materials and then take their densityProperties
                  value: Utils.toFixed( material.density / DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER, 2 ),
                  decimalPlaces: 2
                } );
Index: js/common/model/Material.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Material.ts b/js/common/model/Material.ts
--- a/js/common/model/Material.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/model/Material.ts	(date 1720649992807)
@@ -29,6 +29,7 @@
 import Range from '../../../../dot/js/Range.js';
 import WithRequired from '../../../../phet-core/js/types/WithRequired.js';
 import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
+import NumberProperty from '../../../../axon/js/NumberProperty.js';
 
 const NullableColorPropertyReferenceType = NullableIO( ReferenceIO( Property.PropertyIO( Color.ColorIO ) ) );
 
@@ -117,10 +118,10 @@
   hidden?: boolean;
 
   // Uses the color for a solid material's color
-  customColor?: Property<Color> | null;
+  customColor?: ReadOnlyProperty<Color> | null;
 
   // Uses the alpha channel for opacity
-  liquidColor?: Property<Color> | null;
+  liquidColor?: ReadOnlyProperty<Color> | null;
 
   // Used for the color of depth lines added on top of the Material
   depthLinesColorProperty?: TReadOnlyProperty<Color>;
@@ -133,15 +134,15 @@
   public readonly nameProperty: TReadOnlyProperty<string>;
   public readonly identifier: MaterialName;
   public readonly tandemName: string | null;
-  public readonly density: number; // in SI (kg/m^3)
   public readonly viscosity: number;
 
   // TODO: Eliminate custom as an orthogonal attribute, it can be determined from identifier. https://github.com/phetsims/density-buoyancy-common/issues/176
   public readonly custom: boolean;
   public readonly hidden: boolean;
-  public readonly customColor: Property<Color> | null;
-  public readonly liquidColor: Property<Color> | null;
+  public readonly customColor: ReadOnlyProperty<Color> | null;
+  public readonly liquidColor: ReadOnlyProperty<Color> | null;
   public readonly depthLinesColorProperty: TReadOnlyProperty<Color>;
+  public readonly densityProperty: NumberProperty;
 
   public constructor( providedOptions: MaterialOptions ) {
 
@@ -162,13 +163,24 @@
     this.nameProperty = options.nameProperty;
     this.identifier = options.identifier;
     this.tandemName = options.tandemName;
-    this.density = options.density;
+    this.densityProperty = new NumberProperty( options.density, {
+        // phetioFeatured: true,
+        // phetioDocumentation: 'Density of the object when the material is set to “CUSTOM”.',
+        range: new Range( 0, 23000 ),
+        units: 'kg/m^3'
+    } );
     this.viscosity = options.viscosity;
     this.custom = options.custom;
     this.hidden = options.hidden;
     this.customColor = options.customColor;
     this.liquidColor = options.liquidColor;
     this.depthLinesColorProperty = options.depthLinesColorProperty;
+
+    console.log( 'hello new material' );
+  }
+
+  public get density(): number {
+    return this.densityProperty.value;
   }
 
   /**
@@ -188,6 +200,9 @@
    */
   public static createCustomLiquidMaterial( options: WithRequired<CreateCustomMaterialOptions, 'densityRange'> ): Material {
     return Material.createCustomMaterial( combineOptions<MaterialOptions>( {
+
+      // TODO: Make sure to change the liquidColor when the density changes
+      // This can be done by moving more things into the Material constructor, so they can use the densityProperty
       liquidColor: Material.getCustomLiquidColor( options.density, options.densityRange )
     }, options ) );
   }
@@ -199,7 +214,10 @@
 
     assert && assert( options.hasOwnProperty( 'customColor' ) || options.hasOwnProperty( 'densityRange' ), 'we need a way to have a material color' );
 
+    // TODO: Make sure to change the solidColorProperty when the density changes
     const solidColorProperty = options.customColor || Material.getCustomSolidColor( options.density, options.densityRange! );
+
+    // TODO: Make sure to change the depthLinesColorPropertyProperty when the density changes
     const depthLinesColorPropertyProperty = new MappedProperty( solidColorProperty, {
       map: solidColor => {
 
Index: js/common/view/BlockControlNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/BlockControlNode.ts b/js/common/view/BlockControlNode.ts
--- a/js/common/view/BlockControlNode.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/view/BlockControlNode.ts	(date 1720637112852)
@@ -19,6 +19,7 @@
 import PrecisionSliderThumb from './PrecisionSliderThumb.js';
 import UnitConversionProperty from '../../../../axon/js/UnitConversionProperty.js';
 import Range from '../../../../dot/js/Range.js';
+import NumberProperty from '../../../../axon/js/NumberProperty.js';
 
 type SelfOptions = {
   mysteryMaterials: Material[]; // Provide empty list to opt out.
@@ -41,19 +42,23 @@
       assert && assert( cuboid.adjustableMaterial, 'useDensityControlInsteadOfMassControl should only be used with adjustable materials' );
 
       const densityNumberControlTandem = options.tandem.createTandem( 'densityNumberControl' );
-      const customDensityProperty = cuboid.customDensityProperty!;
+      // const customDensityProperty = cuboid.customDensityProperty!;
 
       // TODO: See https://github.com/phetsims/density-buoyancy-common/issues/176
       // customDensityProperty.lazyLink( () => {
       //   cuboid.materialEnumProperty!.value = 'CUSTOM';
       // } );
 
+      const customDensityProperty = new NumberProperty( 1000, {} );
+
       const densityAsLitersProperty = new UnitConversionProperty( customDensityProperty, {
         factor: 1 / DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER
       } );
 
-      const densityNumberControl = new NumberControl( DensityBuoyancyCommonStrings.densityStringProperty,
-        densityAsLitersProperty, new Range( customDensityProperty.range.min / DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER, 10 ),
+      const densityNumberControl = new NumberControl(
+        DensityBuoyancyCommonStrings.densityStringProperty,
+        densityAsLitersProperty,
+        new Range( customDensityProperty.range.min / DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER, 10 ),
         combineOptions<NumberControlOptions>( {
           sliderOptions: {
             accessibleName: DensityBuoyancyCommonStrings.densityStringProperty,
Index: js/common/view/MaterialView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MaterialView.ts b/js/common/view/MaterialView.ts
--- a/js/common/view/MaterialView.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/view/MaterialView.ts	(date 1720646764416)
@@ -52,6 +52,7 @@
 import Wood26_rgh_jpg from '../../../images/Wood26_rgh_jpg.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import Material from '../model/Material.js';
+import ReadOnlyProperty from '../../../../axon/js/ReadOnlyProperty.js';
 
 // MaterialView definition
 
@@ -339,10 +340,10 @@
 
 export class ColoredMaterialView extends MaterialView<THREE.MeshLambertMaterial> {
 
-  private readonly colorProperty: Property<Color>;
+  private readonly colorProperty: ReadOnlyProperty<Color>;
   private readonly listener: ( color: Color ) => void;
 
-  public constructor( colorProperty: Property<Color> ) {
+  public constructor( colorProperty: ReadOnlyProperty<Color> ) {
 
     assert && assert( colorProperty !== null, 'colorProperty should not be null' );
 
Index: js/common/view/MaterialMassVolumeControlNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MaterialMassVolumeControlNode.ts b/js/common/view/MaterialMassVolumeControlNode.ts
--- a/js/common/view/MaterialMassVolumeControlNode.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/view/MaterialMassVolumeControlNode.ts	(date 1720637112850)
@@ -182,10 +182,7 @@
 
         // If we're custom, adjust the density
         if ( materialProperty.value.custom && !options.customKeepsConstantDensity ) {
-          materialProperty.value = Material.createCustomSolidMaterial( {
-            density: massProperty.value / cubicMeters,
-            densityRange: this.customDensityRange
-          } );
+          materialProperty.value.densityProperty.value = massProperty.value / cubicMeters;
         }
         setVolume( cubicMeters );
 
@@ -221,10 +218,11 @@
         // It is possible for the volumeProperty to be 0, so avoid the infinite density case, see https://github.com/phetsims/density-buoyancy-common/issues/78
         if ( materialProperty.value.custom && volumeProperty.value > 0 ) {
           if ( !options.customKeepsConstantDensity ) { // Separate if statement, so we don't setVolume() below
-            materialProperty.value = Material.createCustomSolidMaterial( {
-              density: mass / volumeProperty.value,
-              densityRange: this.customDensityRange
-            } );
+            materialProperty.value.densityProperty.value = mass / volumeProperty.value;
+            // materialProperty.value = Material.createCustomSolidMaterial( {
+            //   density: mass / volumeProperty.value,
+            //   densityRange: this.customDensityRange
+            // } );
           }
         }
         else {
Index: js/common/view/CuboidView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CuboidView.ts b/js/common/view/CuboidView.ts
--- a/js/common/view/CuboidView.ts	(revision 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/common/view/CuboidView.ts	(date 1720648238174)
@@ -95,6 +95,8 @@
     const materialListener = ( material: Material ) => {
       this.depthLinesNode.stroke = material.depthLinesColorProperty;
     };
+
+    // TODO: Change the depth lines stroke when the
     cuboid.materialProperty.link( materialListener );
 
     this.disposeEmitter.addListener( () => {
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 90eda68564375b5ea129e761083ca1ed1d888f92)
+++ b/js/buoyancy/model/applications/Bottle.ts	(date 1720637112834)
@@ -191,7 +191,7 @@
   // In kg (kilograms)
   public interiorMassProperty: ReadOnlyProperty<number>;
 
-  public override readonly customDensityProperty: NumberProperty;
+  // public override readonly customDensityProperty: NumberProperty;
 
   public constructor( engine: PhysicsEngine, providedOptions: BottleOptions ) {
 
@@ -231,16 +231,15 @@
       units: 'm^3'
     } );
 
-    // @ts-expect-error
-    assert && assert( !this.customDensityProperty, 'There should not be a customDensityProperty on bottle before we create our internal one below' );
-
-    this.customDensityProperty = new NumberProperty( 1, {
-      range: new Range( 0.05, 20 ),
-      tandem: materialInsideTandem.createTandem( 'customDensityProperty' ),
-      phetioDocumentation: 'Density of the material inside the bottle when ‘CUSTOM’ is chosen.',
-      phetioFeatured: true,
-      units: 'kg/L'
-    } );
+    // assert && assert( !this.customDensityProperty, 'There should not be a customDensityProperty on bottle before we create our internal one below' );
+    //
+    // this.customDensityProperty = new NumberProperty( 1, {
+    //   range: new Range( 0.05, 20 ),
+    //   tandem: materialInsideTandem.createTandem( 'customDensityProperty' ),
+    //   phetioDocumentation: 'Density of the material inside the bottle when ‘CUSTOM’ is chosen.',
+    //   phetioFeatured: true,
+    //   units: 'kg/L'
+    // } );
 
     this.interiorMassProperty = new DerivedProperty( [ this.materialInsideProperty, this.materialInsideVolumeProperty ], ( material, volume ) => {
       return material.density * volume;

@zepumph
Copy link
Member

zepumph commented Jul 11, 2024

We made it to a commit point! But we are on a branch. Lots of TODOs, but in general the sim works. We mainly need to add support for Material to have its own density range, and colorProperties for Materials when they are custom.

zepumph added a commit that referenced this issue Jul 11, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit that referenced this issue Jul 11, 2024
@zepumph
Copy link
Member

zepumph commented Jul 11, 2024

A couple more changes coming in. Material is nicer with subtypes to handle the colors for it, but it would be good to next try to combine customColor and liquidColor into the same field. Also noting that I don't like that the subtypes have to mutate the fields to define them, but that is best so that they can be derived from the densityProperty created in the supertype.

@zepumph
Copy link
Member

zepumph commented Jul 11, 2024

  • Bug:
  1. application screen, bottle scene,
  2. Custom material inside the bottle.
  3. Move the volume slider

@samreid
Copy link
Member Author

samreid commented Jul 12, 2024

Subject: [PATCH] Require providedOptions in optionize, see https://github.com/phetsims/phet-core/issues/134
---
Index: js/common/model/Material.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Material.ts b/js/common/model/Material.ts
--- a/js/common/model/Material.ts	(revision 8863803231c554d842cbef1d948e30f6bedcc53f)
+++ b/js/common/model/Material.ts	(date 1720803133539)
@@ -12,13 +12,7 @@
 import Utils from '../../../../dot/js/Utils.js';
 import ThreeUtils from '../../../../mobius/js/ThreeUtils.js';
 import optionize, { combineOptions, EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
-import { Color, ColorProperty, ColorState } from '../../../../scenery/js/imports.js';
-import BooleanIO from '../../../../tandem/js/types/BooleanIO.js';
-import IOType from '../../../../tandem/js/types/IOType.js';
-import NullableIO from '../../../../tandem/js/types/NullableIO.js';
-import NumberIO from '../../../../tandem/js/types/NumberIO.js';
-import ReferenceIO, { ReferenceIOState } from '../../../../tandem/js/types/ReferenceIO.js';
-import StringIO from '../../../../tandem/js/types/StringIO.js';
+import { Color } from '../../../../scenery/js/imports.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import DensityBuoyancyCommonStrings from '../../DensityBuoyancyCommonStrings.js';
 import DensityBuoyancyCommonColors from '../view/DensityBuoyancyCommonColors.js';
@@ -29,24 +23,6 @@
 import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
 import NumberProperty from '../../../../axon/js/NumberProperty.js';
 import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
-import MappedProperty from '../../../../axon/js/MappedProperty.js';
-
-const NullableColorPropertyReferenceType = NullableIO( ReferenceIO( Property.PropertyIO( Color.ColorIO ) ) );
-
-type MaterialState = {
-  identifier: MaterialName;
-  name: ReferenceIOState;
-  tandemName: string | null;
-  density: number;
-  viscosity: number;
-  custom: boolean;
-  hidden: boolean;
-  staticCustomColor: null | ColorState;
-  customColor: null | ColorState;
-  staticLiquidColor: null | ColorState;
-  liquidColor: null | ColorState;
-  depthLinesColor: ColorState;
-};
 
 const nonCustomMaterialNames = [
   'ALUMINUM',
@@ -123,10 +99,7 @@
   // TODO: Rename to customColorProperty, https://github.com/phetsims/density-buoyancy-common/issues/256
   // TODO: Can we combine custom/liquid colors? https://github.com/phetsims/density-buoyancy-common/issues/256
   // Uses the color for a solid material's color
-  customColor?: ReadOnlyProperty<Color> | null;
-
-  // Uses the alpha channel for opacity
-  liquidColor?: ReadOnlyProperty<Color> | null;
+  colorProperty?: ReadOnlyProperty<Color> | null;
 
   // Used for the color of depth lines added on top of the Material
   depthLinesColorProperty?: TReadOnlyProperty<Color>;
@@ -156,8 +129,7 @@
   // TODO: Eliminate custom as an orthogonal attribute, it can be determined from identifier. https://github.com/phetsims/density-buoyancy-common/issues/256
   public readonly custom: boolean;
   public readonly hidden: boolean;
-  public customColor: ReadOnlyProperty<Color> | null;
-  public liquidColor: ReadOnlyProperty<Color> | null;
+  public colorProperty: ReadOnlyProperty<Color> | null;
   public depthLinesColorProperty: TReadOnlyProperty<Color>;
   public readonly densityProperty: NumberProperty;
 
@@ -171,8 +143,7 @@
       viscosity: 1e-3,
       custom: false,
       hidden: false,
-      customColor: null,
-      liquidColor: null,
+      colorProperty: null,
       depthLinesColorProperty: DensityBuoyancyCommonColors.depthLinesDarkColorProperty
     }, providedOptions );
 
@@ -190,9 +161,10 @@
     this.viscosity = options.viscosity;
     this.custom = options.custom;
     this.hidden = options.hidden;
+    this.colorProperty = options.colorProperty;
 
-    this.customColor = options.customColor;
-    this.liquidColor = options.liquidColor;
+    // this.customColor = options.customColor;
+    // this.liquidColor = options.liquidColor;
     this.depthLinesColorProperty = options.depthLinesColorProperty;
 
     // TODO: make this happen less https://github.com/phetsims/density-buoyancy-common/issues/256
@@ -285,9 +257,9 @@
   public static linkLiquidColor( property: TProperty<Material>, threeMaterial: THREE.MeshPhongMaterial | THREE.MeshLambertMaterial | THREE.MeshBasicMaterial ): void {
     new DynamicProperty<Color, Color, Material>( property, {
       derive: material => {
-        assert && assert( material.liquidColor );
+        assert && assert( material.colorProperty );
 
-        return material.liquidColor!;
+        return material.colorProperty!;
       }
     } ).link( ( color: Color ) => {
       threeMaterial.color = ThreeUtils.colorToThree( color );
@@ -334,7 +306,7 @@
     tandemName: 'concrete',
     identifier: 'CONCRETE',
     density: 3150,
-    liquidColor: DensityBuoyancyCommonColors.materialConcreteColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialConcreteColorProperty
   } );
 
   public static readonly COPPER = new Material( {
@@ -342,7 +314,7 @@
     tandemName: 'copper',
     identifier: 'COPPER',
     density: 8960,
-    liquidColor: DensityBuoyancyCommonColors.materialCopperColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialCopperColorProperty
   } );
 
   public static readonly DIAMOND = new Material( {
@@ -385,7 +357,7 @@
     tandemName: 'lead',
     identifier: 'LEAD',
     density: 11342,
-    liquidColor: DensityBuoyancyCommonColors.materialLeadColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialLeadColorProperty
   } );
 
   public static readonly PLATINUM = new Material( {
@@ -462,7 +434,7 @@
     identifier: 'AIR',
     density: 1.2,
     viscosity: 0,
-    liquidColor: DensityBuoyancyCommonColors.materialAirColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialAirColorProperty
   } );
 
   public static readonly FLUID_A = new Material( {
@@ -470,7 +442,7 @@
     tandemName: 'fluidA',
     identifier: 'FLUID_A',
     density: 3100,
-    liquidColor: DensityBuoyancyCommonColors.materialDensityAColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialDensityAColorProperty,
     hidden: true
   } );
 
@@ -479,7 +451,7 @@
     tandemName: 'fluidB',
     identifier: 'FLUID_B',
     density: 790,
-    liquidColor: DensityBuoyancyCommonColors.materialDensityBColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialDensityBColorProperty,
     hidden: true
   } );
 
@@ -488,7 +460,7 @@
     tandemName: 'fluidC',
     identifier: 'FLUID_C',
     density: 490,
-    liquidColor: DensityBuoyancyCommonColors.materialDensityCColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialDensityCColorProperty,
     hidden: true
   } );
 
@@ -497,7 +469,7 @@
     tandemName: 'fluidD',
     identifier: 'FLUID_D',
     density: 2890,
-    liquidColor: DensityBuoyancyCommonColors.materialDensityDColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialDensityDColorProperty,
     hidden: true
   } );
 
@@ -506,7 +478,7 @@
     tandemName: 'fluidE',
     identifier: 'FLUID_E',
     density: 1260,
-    liquidColor: DensityBuoyancyCommonColors.materialDensityEColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialDensityEColorProperty,
     hidden: true
   } );
 
@@ -515,7 +487,7 @@
     tandemName: 'fluidF',
     identifier: 'FLUID_F',
     density: 6440,
-    liquidColor: DensityBuoyancyCommonColors.materialDensityFColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialDensityFColorProperty,
     hidden: true
   } );
 
@@ -525,7 +497,7 @@
     identifier: 'GASOLINE',
     density: 680,
     viscosity: 6e-4,
-    liquidColor: DensityBuoyancyCommonColors.materialGasolineColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialGasolineColorProperty
   } );
 
   public static readonly HONEY = new Material( {
@@ -534,7 +506,7 @@
     identifier: 'HONEY',
     density: 1440,
     viscosity: 0.03, // NOTE: actual value around 2.5, but we can get away with this for animation
-    liquidColor: DensityBuoyancyCommonColors.materialHoneyColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialHoneyColorProperty
   } );
 
   public static readonly MERCURY = new Material( {
@@ -543,7 +515,7 @@
     identifier: 'MERCURY',
     density: 13593,
     viscosity: 1.53e-3,
-    liquidColor: DensityBuoyancyCommonColors.materialMercuryColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialMercuryColorProperty
   } );
 
   public static readonly OIL = new Material( {
@@ -552,7 +524,7 @@
     identifier: 'OIL',
     density: 920,
     viscosity: 0.02, // Too much bigger and it won't work, not particularly physical
-    liquidColor: DensityBuoyancyCommonColors.materialOilColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialOilColorProperty
   } );
 
   public static readonly SAND = new Material( {
@@ -561,7 +533,7 @@
     identifier: 'SAND',
     density: 1442,
     viscosity: 0.03, // Too much bigger and it won't work, not particularly physical
-    liquidColor: DensityBuoyancyCommonColors.materialSandColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialSandColorProperty
   } );
 
   public static readonly SEAWATER = new Material( {
@@ -570,7 +542,7 @@
     identifier: 'SEAWATER',
     density: 1029,
     viscosity: 1.88e-3,
-    liquidColor: DensityBuoyancyCommonColors.materialSeawaterColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialSeawaterColorProperty
   } );
 
   public static readonly WATER = new Material( {
@@ -579,7 +551,7 @@
     identifier: 'WATER',
     density: 1000,
     viscosity: 8.9e-4,
-    liquidColor: DensityBuoyancyCommonColors.materialWaterColorProperty
+    colorProperty: DensityBuoyancyCommonColors.materialWaterColorProperty
   } );
 
   ////////////////// MYSTERY MATERIALS //////////////////
@@ -589,7 +561,7 @@
     tandemName: 'materialO',
     identifier: 'MATERIAL_O',
     hidden: true,
-    customColor: new Property( new Color( '#f00' ) ),
+    colorProperty: new Property( new Color( '#f00' ) ),
     density: 950 // Same as the Human's average density
   } );
 
@@ -598,7 +570,7 @@
     tandemName: 'materialP',
     identifier: 'MATERIAL_P',
     hidden: true,
-    customColor: new Property( new Color( '#0f0' ) ),
+    colorProperty: new Property( new Color( '#0f0' ) ),
     density: Material.DIAMOND.density
   } );
 
@@ -607,7 +579,7 @@
     tandemName: 'materialR',
     identifier: 'MATERIAL_R',
     hidden: true,
-    liquidColor: DensityBuoyancyCommonColors.materialRColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialRColorProperty,
     density: Material.ICE.density
   } );
 
@@ -616,7 +588,7 @@
     tandemName: 'materialS',
     identifier: 'MATERIAL_S',
     hidden: true,
-    liquidColor: DensityBuoyancyCommonColors.materialSColorProperty,
+    colorProperty: DensityBuoyancyCommonColors.materialSColorProperty,
     density: Material.LEAD.density
   } );
 
@@ -625,7 +597,7 @@
     tandemName: 'materialV',
     identifier: 'MATERIAL_V',
     hidden: true,
-    customColor: new Property( new Color( '#ff0' ) ),
+    colorProperty: new Property( new Color( '#ff0' ) ),
     density: Material.TITANIUM.density
   } );
 
@@ -634,7 +606,7 @@
     tandemName: 'materialW',
     identifier: 'MATERIAL_W',
     hidden: true,
-    customColor: new Property( new Color( '#0af' ) ),
+    colorProperty: new Property( new Color( '#0af' ) ),
     density: Material.MERCURY.density
   } );
 
@@ -750,67 +722,6 @@
     Material.FLUID_E,
     Material.FLUID_F
   ];
-
-  public static readonly MaterialIO = new IOType<Material, MaterialState>( 'MaterialIO', {
-    valueType: Material,
-    documentation: 'Represents different materials that solids/liquids in the simulations can take, including density (kg/m^3), viscosity (Pa * s), and color.',
-    stateSchema: {
-      name: ReferenceIO( ReadOnlyProperty.PropertyIO( StringIO ) ),
-      identifier: StringIO,
-      tandemName: NullableIO( StringIO ),
-      density: NumberIO,
-      viscosity: NumberIO,
-      custom: BooleanIO,
-      hidden: BooleanIO,
-      staticCustomColor: NullableIO( Color.ColorIO ),
-      customColor: NullableColorPropertyReferenceType,
-      staticLiquidColor: NullableIO( Color.ColorIO ),
-      liquidColor: NullableColorPropertyReferenceType,
-      depthLinesColor: Color.ColorIO
-    },
-    toStateObject( material ) {
-
-      const isCustomColorUninstrumented = material.customColor && !material.customColor.isPhetioInstrumented();
-      const isLiquidColorUninstrumented = material.liquidColor && !material.liquidColor.isPhetioInstrumented();
-
-      return {
-        name: ReferenceIO( ReadOnlyProperty.PropertyIO( StringIO ) ).toStateObject( material.nameProperty ),
-        identifier: material.identifier,
-        tandemName: NullableIO( StringIO ).toStateObject( material.tandemName ),
-        density: material.density,
-        viscosity: material.viscosity,
-        custom: material.custom,
-        hidden: material.hidden,
-        staticCustomColor: NullableIO( Color.ColorIO ).toStateObject( isCustomColorUninstrumented ? material.customColor!.value : null ),
-        customColor: NullableColorPropertyReferenceType.toStateObject( isCustomColorUninstrumented ? null : material.customColor ),
-        staticLiquidColor: NullableIO( Color.ColorIO ).toStateObject( isLiquidColorUninstrumented ? material.liquidColor!.value : null ),
-        liquidColor: NullableColorPropertyReferenceType.toStateObject( isLiquidColorUninstrumented ? null : material.liquidColor ),
-        depthLinesColor: Color.ColorIO.toStateObject( material.depthLinesColorProperty.value )
-      };
-    },
-    // TODO: even custom materials will want to grab the right reference based on screen/scene/mass, uh oh https://github.com/phetsims/density-buoyancy-common/issues/256
-    fromStateObject( obj ): Material {
-      if ( obj.identifier !== 'CUSTOM' ) {
-        return Material.getMaterial( obj.identifier );
-      }
-      else {
-        const staticCustomColor = NullableIO( Color.ColorIO ).fromStateObject( obj.staticCustomColor );
-        const staticLiquidColor = NullableIO( Color.ColorIO ).fromStateObject( obj.staticLiquidColor );
-        return new Material( {
-          nameProperty: ReferenceIO( ReadOnlyProperty.PropertyIO( StringIO ) ).fromStateObject( obj.name ),
-          identifier: NullableIO( StringIO ).fromStateObject( obj.identifier ),
-          tandemName: NullableIO( StringIO ).fromStateObject( obj.tandemName ),
-          density: obj.density,
-          viscosity: obj.viscosity,
-          custom: obj.custom,
-          hidden: obj.hidden,
-          customColor: staticCustomColor ? new ColorProperty( staticCustomColor ) : NullableColorPropertyReferenceType.fromStateObject( obj.customColor ),
-          liquidColor: staticLiquidColor ? new ColorProperty( staticLiquidColor ) : NullableColorPropertyReferenceType.fromStateObject( obj.liquidColor ),
-          depthLinesColorProperty: new ColorProperty( Color.ColorIO.fromStateObject( obj.depthLinesColor ) )
-        } );
-      }
-    }
-  } );
 }
 
 class SolidMaterial extends Material {
@@ -820,23 +731,26 @@
 
     super( options );
 
-    if ( !this.customColor ) {
+    assert && assert( this.custom, 'SolidMaterial should only be used for custom materials' );
+
+    if ( !this.colorProperty ) {
+
       // TODO: can we make this field readonly again? https://github.com/phetsims/density-buoyancy-common/issues/256
-      this.customColor = new DerivedProperty( [ this.densityProperty, this.densityProperty.rangeProperty ], ( density, densityRange ) => {
+      this.colorProperty = new DerivedProperty( [ this.densityProperty, this.densityProperty.rangeProperty ], ( density, densityRange ) => {
         const lightness = Material.getCustomLightness( density, densityRange );
         return new Color( lightness, lightness, lightness );
       } );
-      this.liquidColor = this.customColor;
     }
 
-    this.depthLinesColorProperty = new MappedProperty( this.customColor, {
-      map: solidColor => {
+    this.depthLinesColorProperty = new DerivedProperty( [
+      this.colorProperty,
+      DensityBuoyancyCommonColors.depthLinesLightColorProperty,
+      DensityBuoyancyCommonColors.depthLinesDarkColorProperty
+    ], ( color, depthLinesLightColor, depthLinesDarkColor ) => {
 
-        // The lighter depth line color has better contrast, so use that for more than half
-        const isDark = ( solidColor.r + solidColor.g + solidColor.b ) / 3 < 255 * 0.6;
-
-        return isDark ? DensityBuoyancyCommonColors.depthLinesLightColorProperty.value : DensityBuoyancyCommonColors.depthLinesDarkColorProperty.value;
-      }
+      // The lighter depth line color has better contrast, so use that for more than half
+      const isDark = ( color.r + color.g + color.b ) / 3 < 255 * 0.6;
+      return isDark ? depthLinesLightColor : depthLinesDarkColor;
     } );
   }
 }
@@ -848,9 +762,9 @@
 
     super( options );
     // TODO: This could be custom color given a "liquid" flag/subtype, https://github.com/phetsims/density-buoyancy-common/issues/256
-    if ( !this.liquidColor && this.custom ) {
+    if ( !this.colorProperty && this.custom ) {
       // TODO: can we make this field readonly again? https://github.com/phetsims/density-buoyancy-common/issues/256
-      this.liquidColor = new DerivedProperty( [ this.densityProperty, this.densityProperty.rangeProperty ], ( density, densityRange ) => {
+      this.colorProperty = new DerivedProperty( [ this.densityProperty, this.densityProperty.rangeProperty ], ( density, densityRange ) => {
         return Material.getCustomLiquidColor( density, densityRange );
       } );
     }

@zepumph
Copy link
Member

zepumph commented Jul 12, 2024

  • Let's make a dev version of main right before merging this branch into it.

zepumph added a commit that referenced this issue Jul 12, 2024
zepumph added a commit that referenced this issue Jul 12, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit that referenced this issue Jul 12, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit that referenced this issue Jul 12, 2024
zepumph added a commit that referenced this issue Jul 12, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
samreid added a commit that referenced this issue Jul 12, 2024
zepumph added a commit that referenced this issue Jul 12, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit that referenced this issue Jul 12, 2024
@zepumph
Copy link
Member

zepumph commented Jul 12, 2024

Great progress today! All three sims fuzzed for 30 seconds in PhET brand. Next steps for next week. Most of this is already in the 50 TODOs we have linked to this issue, but I'm going to write some ordered priorities to help focus us for next time.

  1. Fluid selection is still very much unsupported, let's try to apply what we did to the block controls to the FluidDensityControlNode.
  2. Let's have a discussion and investigation about moving all material selections into the model. Perhaps using validValues for materials.

zepumph added a commit that referenced this issue Jul 12, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
@zepumph
Copy link
Member

zepumph commented Jul 18, 2024

We are getting quite close here. Today we merged this work onto main, and created even more side issues.

Still TODO:

  • 11 TODOs pointing to this issue.
  • I believe QA should get some time with these changes.

@samreid
Copy link
Member Author

samreid commented Jul 19, 2024

Most of this issue is complete or moved to side issues. All remaining TODOs in the code are labeled for AV in the TODO. Would also be good for @zepumph to review. Self-unassigning for now.

@zepumph
Copy link
Member

zepumph commented Jul 19, 2024

Everything above looks good thanks. I also appreciate all the side issues. Over to @AgustinVallejo for final TODOs. (I count 6).

@AgustinVallejo
Copy link
Contributor

AgustinVallejo commented Jul 20, 2024

Adding some answers to TODO:

TODO AV: BUG: see #256 there was a bug when changing the custom density of a block in screen 1. The block didn't seem to change its floating level.

Changing the custom density is not affecting the actual used material, which is wood. In fact
any density change to a block which has a selected material, won't update it until it's changed to custom. Should we auto switch to custom when the density is changed via studio? Because otherwise we'd have super dense wood, would that be desirable?

@samreid
Copy link
Member Author

samreid commented Jul 20, 2024

Changing the custom density is not affecting the actual used material, which is wood. In fact
any density change to a block which has a selected material, won't update it until it's changed to custom. Should we auto switch to custom when the density is changed via studio?

Good discovery! Maybe this is working as desired? And we need to document that the client has to also change the material to the custom one?

AgustinVallejo added a commit that referenced this issue Jul 20, 2024
@AgustinVallejo
Copy link
Contributor

Good discovery! Maybe this is working as desired? And we need to document that the client has to also change the material to the custom one?

That will be determined here: #273

@AgustinVallejo
Copy link
Contributor

No more todo's for this issue. Spoke with @samreid and it seems this behemoth of an issue is ready to be closed! 🥳🥳🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:help-wanted Extra attention is needed priority:2-high
Projects
None yet
Development

No branches or pull requests

3 participants