phetsims / density-buoyancy-common

Common code for the Density and Buoyancy simulations
GNU General Public License v3.0
0 stars 2 forks source link

Changes in the Compare Screen (first screen for both: Buoyancy and B:B) #146

Closed DianaTavares closed 4 months ago

DianaTavares commented 4 months ago

image

samreid commented 4 months ago

This screen will be the first in buoyancy and Buoyancy: Basics, and in both, the name of the screen is "Compare"

I would like to clarify before starting. Does this mean the Compare screen will replace the the "intro" screen in Buoyancy?

Current screens in Buoyancy: Intro, Explore, Lab, Shapes, Applications

After this change: Compare, Explore, Lab, Shapes, Applications

And we will delete the code for Intro? Or will this be a 6th screen in Buoyancy? (I didn't think so but wanted to double check). Please clarify.

DianaTavares commented 4 months ago

Yes, this is the change for buoyancy:

Compare, Explore, Lab, Shapes, Applications

Intro will be replaced by Compare.

zepumph commented 4 months ago

https://github.com/phetsims/density-buoyancy-common/issues/156

zepumph commented 4 months ago

Working on this now

zepumph commented 4 months ago

Some progress but I'm not in love with this patch. I'll come back tomorrow

```diff Subject: [PATCH] remove unneeded opt outs, https://github.com/phetsims/density-buoyancy-common/issues/146 --- Index: js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts b/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts --- a/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts (revision ee103f64796fef46901480a2cf3985d8856bd9b4) +++ b/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts (date 1717454355068) @@ -20,6 +20,7 @@ import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js'; import PoolScaleHeightProperty from '../../common/model/PoolScaleHeightProperty.js'; import DensityBuoyancyCommonConstants from '../../common/DensityBuoyancyCommonConstants.js'; +import Material from '../../common/model/Material.js'; export type BuoyancyBasicsCompareModelOptions = StrictOmit; @@ -37,7 +38,9 @@ supportsDepthLines: true, usePoolScale: false, // create out own based on the PoolScaleHeightControl - + initialMaterials: [ Material.WOOD, Material.BRICK ], + sameMassValue: 4, + sameDensityValue: Material.WOOD.density, cubesData: [ { sameMassVolume: 0.002, 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 ee103f64796fef46901480a2cf3985d8856bd9b4) +++ b/js/common/model/CompareBlockSetModel.ts (date 1717457208237) @@ -28,6 +28,8 @@ import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import Vector2 from '../../../../dot/js/Vector2.js'; import WithRequired from '../../../../phet-core/js/types/WithRequired.js'; +import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; +import Multilink from '../../../../axon/js/Multilink.js'; // This hard coded range is a bit arbitrary, but it lends itself to better colors than the provided range in the options. const COLOR_DENSITY_RANGE = new Range( 10, 10000 ); @@ -63,6 +65,8 @@ sameVolumeRange?: Range; sameDensityValue?: number; sameDensityRange?: Range; + + initialMaterials?: Material[]; }; type ParentOptions = BlockSetModelOptions; @@ -89,6 +93,8 @@ sameDensityValue: 400, sameDensityRange: new Range( 100, 2000 ), + initialMaterials: [], + // BlockSetModel options initialMode: BlockSet.SAME_MASS, BlockSet: BlockSet.enumeration, @@ -121,6 +127,15 @@ units: 'kg/m^3' } ); + // TODO: reset this + // TODO: instrument this + // TODO: this logic shouldn't be in the density one (empty initialMaterials) + const anyObjectsChangedProperty = new BooleanProperty( false ); + + Multilink.lazyMultilink( [ massProperty, volumeProperty, densityProperty ], () => { + anyObjectsChangedProperty.value = true; + } ); + const commonCubeOptions = { minVolume: DensityBuoyancyCommonConstants.MIN_CUBE_VOLUME, maxVolume: DensityBuoyancyCommonConstants.MAX_CUBE_VOLUME @@ -135,11 +150,11 @@ return merge( { sameMassDensityProperty: sameMassDensityProperty, - sameMassMaterialProperty: CompareBlockSetModel.createMaterialProperty( cubeData.colorProperty, sameMassDensityProperty ), + sameMassMaterialProperty: CompareBlockSetModel.createMaterialProperty( cubeData.colorProperty, sameMassDensityProperty, anyObjectsChangedProperty, options.initialMaterials ), sameVolumeDensityProperty: sameVolumeDensityProperty, - sameVolumeMaterialProperty: CompareBlockSetModel.createMaterialProperty( cubeData.colorProperty, sameVolumeDensityProperty ), + sameVolumeMaterialProperty: CompareBlockSetModel.createMaterialProperty( cubeData.colorProperty, sameVolumeDensityProperty, anyObjectsChangedProperty, options.initialMaterials ), sameDensityDensityProperty: sameDensityDensityProperty, - sameDensityMaterialProperty: CompareBlockSetModel.createMaterialProperty( cubeData.colorProperty, sameDensityDensityProperty ) + sameDensityMaterialProperty: CompareBlockSetModel.createMaterialProperty( cubeData.colorProperty, sameDensityDensityProperty, anyObjectsChangedProperty, options.initialMaterials ) }, cubeData ); } ); @@ -246,23 +261,35 @@ super.reset(); } - private static createMaterialProperty( colorProperty: TProperty, myDensityProperty: TProperty ): TReadOnlyProperty { - return new DerivedProperty( [ colorProperty, myDensityProperty ], ( color, density ) => { - const lightness = Material.getNormalizedLightness( density, COLOR_DENSITY_RANGE ); // 0-1 + private static createMaterialProperty( colorProperty: TProperty, myDensityProperty: TProperty, + anyObjectValueChangedProperty: TProperty, initialMaterials: Material[] ): TReadOnlyProperty { + return new DerivedProperty( [ colorProperty, myDensityProperty, anyObjectValueChangedProperty ], + ( color, density, anyObjectValueChanged ) => { + + if ( !anyObjectValueChanged ) { + for ( let i = 0; i < initialMaterials.length; i++ ) { + const material = initialMaterials[ i ]; + if ( material.density === density ) { + return material; + } + } + } + + const lightness = Material.getNormalizedLightness( density, COLOR_DENSITY_RANGE ); // 0-1 - 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 ) ); + 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 ) ); - return Material.createCustomSolidMaterial( { - density: density, - customColor: new Property( modifiedColor, { tandem: Tandem.OPT_OUT } ) - } ); - }, { - strictAxonDependencies: false, // The DerivedProperty derivation triggers the creation of a DynamicProperty which calls .value on itself, which is safe - tandem: Tandem.OPT_OUT - } ); + return Material.createCustomSolidMaterial( { + density: density, + customColor: new Property( modifiedColor, { tandem: Tandem.OPT_OUT } ) + } ); + }, { + strictAxonDependencies: false, // The DerivedProperty derivation triggers the creation of a DynamicProperty which calls .value on itself, which is safe + tandem: Tandem.OPT_OUT + } ); } }
zepumph commented 4 months ago

I believe all of this is done besides the "Density Comparison" rename that will occur in a different issue. @DianaTavares please review.

zepumph commented 4 months ago

@samreid can you please spot check this commit:

https://github.com/phetsims/density-buoyancy-common/commit/07199490424177a9941f2ca9989e55ba62d91c6e

DianaTavares commented 4 months ago

From the view (design) side, it looks amazing! thanks.

I don't love how the Contact force is moving as crazy while I manipulate the slider. The other 2 forces are amazing, weight is constant, and the buoyancy force increases/decreases slowly as the block is submerged or moving out of the pool, but contact, goes up and down as crazy (I hope this is something see can do!) Untitled

I see that the same happens with the numbers in the scale, but that isn't as distracting as an arrow going up and down.

samreid commented 4 months ago

@AgustinVallejo are we using the blend algorithm on that displayed contact force value?

samreid commented 4 months ago

This patch adds more blending for the value and smooths out that displayed value. It isn't perfectly smooth though and it adds latency to reach the value

```diff Subject: [PATCH] Update blended property values during step, see https://github.com/phetsims/density-buoyancy-common/issues/158 --- Index: js/common/model/BlendedVector2Property.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/BlendedVector2Property.ts b/js/common/model/BlendedVector2Property.ts --- a/js/common/model/BlendedVector2Property.ts (revision 477800975c3e23e6cc98aa439f7cd22c0d488ed0) +++ b/js/common/model/BlendedVector2Property.ts (date 1717596829200) @@ -21,7 +21,7 @@ // choose the blendAmount based on the distance between values // This adds a hysteresis effect to the readout, which reduces flickering inherent to the model. const MIN_BLEND = 0.1; // When close, blend with the old value more - const MAX_BLEND = 1; // When far apart, take the new value completely + const MAX_BLEND = 0.2; // When far apart, take more of the new value const blendAmount = Utils.clamp( Utils.linear( 0, 1, MIN_BLEND, MAX_BLEND, newValue.minus( oldValue ).magnitude ), MIN_BLEND, MAX_BLEND ); ```

We should probably discuss and tune this during standup.

  1. Should we use this approach? (Adjusting the max blending cap)
  2. What should the number be?
  3. We should probably use the same number in BlendedNumberProperty so the contact force looks balanced with the weight.
AgustinVallejo commented 4 months ago

@samreid I applied the patch and very much like this solution. Vote for adding it!