Investigate re-entrant Properties

[samreid]

In https://github.com/phetsims/axon/issues/179 we identified Property instances that are re-entrant. In this context, re-entrant means a change in value of the Property causes (via listeners) another change in the value of the same Property instance.

Re-entry can occur for at least 3 different reasons, which are document here: https://github.com/phetsims/axon/issues/179#issuecomment-414717687

value=0.9999999999999998, oldValue=1 // epsilon problem
value=-0.4277580409572783, oldValue=-0.5 // large delta problem
value=[Object], oldValue=[Object] // object changed to object problem.  May be same object?

This issue is to search through the Properties with reentrant: true and:

(a) confirm that the Property really requires reentrant: true (b) identify the reason for the reentry (may be one of the 3 classes above) (c-i) see if the code can be rewritten so it no longer requires a reentrant Property, or document why the code uses a re-entrant Property or (c-ii) document why the code uses a re-entrant Property

Initially assigning to the responsible-dev for this repo, though it is unclear what the priority should be for this issue.

[samreid]

I'd like to defer work on this until resuming work on the sim.

[samreid]

So far, I haven't found a way around this problem. You can trigger one case by dragging the top medium slider all the way to the right. It seems like a roundoff error. But I tried adding deep equals for medium and substance, and looked for a way to combine the medium property with mediumIndexProperty. Neither seems to help, there is still numerical difference. Here's my patch so far:

```diff Index: main/bending-light/js/common/model/Medium.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/bending-light/js/common/model/Medium.js b/main/bending-light/js/common/model/Medium.js --- a/main/bending-light/js/common/model/Medium.js (revision 885eed779c46e8dc4b366f2ec83989e32693e11a) +++ b/main/bending-light/js/common/model/Medium.js (date 1612212023294) @@ -27,6 +27,13 @@ this.color = color; // @public (read-only), color is based on the index of refraction at red wavelength } + // @public + equals( medium ) { + return this.shape === medium.shape && + this.substance.equals( medium.substance ) && + this.color.equals( medium.color ); + } + /** * Determines the index of refraction of medium * @public Index: main/axon/js/Property.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/Property.js b/main/axon/js/Property.js --- a/main/axon/js/Property.js (revision 0197c33333f9e859d5ea450532126e4a9bc11475) +++ b/main/axon/js/Property.js (date 1612281817730) @@ -183,6 +183,7 @@ } else if ( !this.equalsValue( value ) ) { const oldValue = this.get(); + console.log(oldValue); this.setPropertyValue( value ); this._notifyListeners( oldValue ); } Index: main/bending-light/js/intro/model/IntroModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/bending-light/js/intro/model/IntroModel.js b/main/bending-light/js/intro/model/IntroModel.js --- a/main/bending-light/js/intro/model/IntroModel.js (revision 885eed779c46e8dc4b366f2ec83989e32693e11a) +++ b/main/bending-light/js/intro/model/IntroModel.js (date 1612212108137) @@ -48,7 +48,8 @@ const topMedium = new Medium( Shape.rect( -0.1, 0, 0.2, 0.1 ), Substance.AIR, this.mediumColorFactory.getColor( Substance.AIR.indexOfRefractionForRedLight ) ); this.topMediumProperty = new Property( topMedium, { - reentrant: true, + // reentrant: true, + useDeepEquality:true, tandem: tandem.createTandem( 'topMediumProperty' ), phetioType: Property.PropertyIO( Medium.MediumIO ) } ); @@ -57,7 +58,8 @@ const bottomMedium = new Medium( Shape.rect( -0.1, -0.1, 0.2, 0.1 ), bottomSubstance, this.mediumColorFactory.getColor( bottomSubstance.indexOfRefractionForRedLight ) ); this.bottomMediumProperty = new Property( bottomMedium, { - reentrant: true, + // reentrant: true, + useDeepEquality:true, tandem: tandem.createTandem( 'bottomMediumProperty' ), phetioType: Property.PropertyIO( Medium.MediumIO ) } ); Index: main/bending-light/js/common/view/MediumControlPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/bending-light/js/common/view/MediumControlPanel.js b/main/bending-light/js/common/view/MediumControlPanel.js --- a/main/bending-light/js/common/view/MediumControlPanel.js (revision 885eed779c46e8dc4b366f2ec83989e32693e11a) +++ b/main/bending-light/js/common/view/MediumControlPanel.js (date 1612282368102) @@ -46,7 +46,7 @@ class MediumControlPanel extends Node { /** - * @param {BendingLightView} view - view of the simulation + * @param {BendingLightScreenView} view - view of the simulation * @param {MediumColorFactory} mediumColorFactory - for turning index of refraction into color * @param {Property.} mediumProperty - specifies medium * @param {string} name - name of the medium material Index: main/bending-light/js/common/model/Substance.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/bending-light/js/common/model/Substance.js b/main/bending-light/js/common/model/Substance.js --- a/main/bending-light/js/common/model/Substance.js (revision 885eed779c46e8dc4b366f2ec83989e32693e11a) +++ b/main/bending-light/js/common/model/Substance.js (date 1612212244517) @@ -38,6 +38,14 @@ this.indexOfRefractionForRedLight = this.dispersionFunction.getIndexOfRefraction( BendingLightConstants.WAVELENGTH_RED ); this.indexForRed = indexForRed; // @public (read-only) } + + // @public + equals( substance ) { + return this.name === substance.name && + this.mystery === substance.mystery && + this.indexOfRefractionForRedLight === substance.indexOfRefractionForRedLight && + this.custom === substance.custom; + } } bendingLight.register( 'Substance', Substance ); ```

[samreid]

Our project is OK with re-entrant properties, I've added references from the sites back to here. Closing.