phetsims / buoyancy

"Buoyancy" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 2 forks source link

`Material.getCustomLightness()` has some problems #81

Closed zepumph closed 1 month ago

zepumph commented 4 months ago

A couple problems here:

  1. It should not have a hard coded range of density. This was causing trouble in https://github.com/phetsims/buoyancy/issues/28.
  2. It should not have the same range from solids and liquids, and so splitting this up will likely change the look and feel of usages of createCustomSolidMaterial. This is likely best solved with some pixel comparisons.
AgustinVallejo commented 1 month ago

Patch of today's work:

```diff Subject: [PATCH] Colorin colorado --- 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 df718247a74f69b25814b377c6ba965d5894ae65) +++ b/js/common/view/MaterialMassVolumeControlNode.ts (date 1713815818384) @@ -132,6 +132,9 @@ valueComparisonStrategy: 'equalsFunction' } ); + const customDensityRange = new Range( options.minCustomMass / options.maxVolumeLiters, + options.maxCustomMass / options.minCustomVolumeLiters ); + // passed to the NumberControl const numberControlMassProperty = new NumberProperty( massProperty.value, { tandem: massNumberControlContainerTandem.createTandem( 'numberControlMassProperty' ), @@ -159,7 +162,8 @@ // If we're custom, adjust the density if ( materialProperty.value.custom ) { materialProperty.value = Material.createCustomSolidMaterial( { - density: massProperty.value / cubicMeters + density: massProperty.value / cubicMeters, + densityRange: customDensityRange } ); } setVolume( cubicMeters ); @@ -196,7 +200,8 @@ // It is possible for the volumeProperty to be 0, so avoid the infinte density case, see https://github.com/phetsims/density-buoyancy-common/issues/78 if ( materialProperty.value.custom && volumeProperty.value > 0 ) { materialProperty.value = Material.createCustomSolidMaterial( { - density: mass / volumeProperty.value + density: mass / volumeProperty.value, + densityRange: customDensityRange } ); } else { Index: js/density/model/DensityCompareModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/density/model/DensityCompareModel.ts b/js/density/model/DensityCompareModel.ts --- a/js/density/model/DensityCompareModel.ts (revision df718247a74f69b25814b377c6ba965d5894ae65) +++ b/js/density/model/DensityCompareModel.ts (date 1713813843715) @@ -74,10 +74,10 @@ const createMaterialProperty = ( colorProperty: TProperty, densityProperty: TProperty ) => { return new DerivedProperty( [ colorProperty, densityProperty ], ( color, density ) => { - const lightness = Material.getCustomLightness( density ); // 0-255 + const lightness = Material.getNormalizedLightness( density ); // 0-1 const modifier = 0.1; - const rawValue = ( lightness / 128 - 1 ) * ( 1 - modifier ) + modifier; + 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 ) ); Index: js/common/view/DensityMaterials.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/DensityMaterials.ts b/js/common/view/DensityMaterials.ts --- a/js/common/view/DensityMaterials.ts (revision df718247a74f69b25814b377c6ba965d5894ae65) +++ b/js/common/view/DensityMaterials.ts (date 1713814258491) @@ -319,10 +319,10 @@ class CustomMaterialView extends MaterialView { public constructor( density: number ) { - const lightness = Material.getCustomLightness( density ); + const lightness = Material.getNormalizedLightness( density ); super( new THREE.MeshLambertMaterial( { - color: new THREE.Color( `hsl(0, 0%, ${lightness}%)` ) + color: new THREE.Color( lightness, lightness, lightness ) } ) ); } } 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 df718247a74f69b25814b377c6ba965d5894ae65) +++ b/js/common/model/Material.ts (date 1713815085244) @@ -26,6 +26,7 @@ import TinyProperty from '../../../../axon/js/TinyProperty.js'; import ReadOnlyProperty from '../../../../axon/js/ReadOnlyProperty.js'; import MappedProperty from '../../../../axon/js/MappedProperty.js'; +import Range from '../../../../dot/js/Range.js'; const NullableColorPropertyReferenceType = NullableIO( ReferenceIO( Property.PropertyIO( Color.ColorIO ) ) ); @@ -143,8 +144,8 @@ /** * Returns a custom material that can be modified at will, but with a solid color specified */ - public static createCustomSolidMaterial( config: MaterialOptions & Required> ): Material { - const solidColorProperty = Material.getCustomSolidColor( config.density ); + public static createCustomSolidMaterial( config: MaterialOptions & Required> & { densityRange?: Range } ): Material { + const solidColorProperty = Material.getCustomSolidColor( config.density, config.densityRange ); const depthLinesColorProperty = new MappedProperty( solidColorProperty, { map: solidColor => { @@ -163,17 +164,25 @@ /** * Returns a value suitable for use in colors (0-255 value) that should be used as a grayscale value for - * a material of a given density. + * a material of a given density. The mappíng is inverted, i.e. larger densities yield darker colors. */ - public static getCustomLightness( density: number ): number { - return Utils.roundSymmetric( Utils.clamp( Utils.linear( 1, -2, 0, 255, Utils.log10( density / 1000 ) ), 0, 255 ) ); + public static getCustomLightness( density: number, densityRange?: Range ): number { + return Utils.roundSymmetric( this.getNormalizedLightness( density, densityRange ) * 255 ); + } + + public static getNormalizedLightness( density: number, densityRange: Range = new Range( 10, 27000 ) ): number { + const scaleFactor = 1000; + const scaleMax = Utils.log10( densityRange.max / scaleFactor ); // 1 for the default + const scaleMin = Utils.log10( densityRange.min / scaleFactor ); // -2 for the default + const scaleValue = Utils.log10( density / scaleFactor ); + return Utils.clamp( Utils.linear( scaleMax, scaleMin, 0, 1, scaleValue ), 0, 1 ); } /** * Similar to getCustomLightness, but returns the generated color, with an included alpha effect. */ public static getCustomLiquidColor( density: number ): ColorProperty { - const lightness = Material.getCustomLightness( density * 0.25 ); + const lightness = Material.getNormalizedLightness( density, new Range( 40, 40000 ) ); return new ColorProperty( new Color( lightness, lightness, lightness, 0.8 * ( 1 - lightness / 255 ) ) ); } @@ -181,8 +190,8 @@ /** * Similar to getCustomLightness, but returns the generated color */ - public static getCustomSolidColor( density: number ): ColorProperty { - const lightness = Material.getCustomLightness( density ); + public static getCustomSolidColor( density: number, densityRange?: Range ): ColorProperty { + const lightness = Material.getCustomLightness( density, densityRange ); return new ColorProperty( new Color( lightness, lightness, lightness ) ); } @@ -527,7 +536,7 @@ tandemName: 'materialW', identifier: 'MATERIAL_W', hidden: true, - customColor: new Property( new Color( '#0af' ) ), + customColor: new Property( new Color( '#101010' ) ), density: Material.MERCURY.density } );