buggy behavior of bidirectional DynamicProperty

[pixelzoom]

@samreid @zepumph @chrisklus FYI.

This issue is related to understanding why reentrant: true is needed for a bidirectional DynamicProperty in GRAPHING_QUADRATICS/CoefficientSlider, see phetsims/graphing-quadratics#52 and phetsims/sun#417.

Here's an example, a bidirectional mapping where viewProperty is the square of modelProperty.

      const DynamicProperty = require( 'AXON/DynamicProperty' );
      const NumberProperty = require( 'AXON/NumberProperty' );

      const modelProperty = new NumberProperty( 0, {
        isValidValue: value => ( value >= 0 ),
        reentrant: true
      } );

      const viewProperty = new DynamicProperty( new Property( modelProperty ), {
        bidirectional: true,
        reentrant: true,
        map: value => {
          const newValue = Math.pow( value, 2 );
          console.log( 'map: ' + value + ' -> ' + newValue );
          return newValue;
        },
        inverseMap: value => {
          const newValue = Math.sqrt( value );
          console.log( 'inverseMap: ' + value + ' -> ' + newValue );
          return newValue;
        }
      } );

     debugger;

In the console, here's what I observe:

When I set viewProperty to a value that does not involve floating point error, both inverseMap and map are called. Why is map called in this case?

> viewProperty.value = 4
inverseMap: 4 -> 2
map: 2 -> 4
< 4

When I set viewProperty to a value that does involve floating point error, both inverseMap and map are called, and then inverseMap is called again. Why is map called? Why is inverseMap called a second time?

> viewProperty.value = 4.5
inverseMap: 4.5 -> 2.1213203435596424
map: 2.1213203435596424 -> 4.499999999999999
inverseMap: 4.499999999999999 -> 2.1213203435596424
> 4.5

When I set modelProperty, I see map and inverseMap called. Why is inverseMap called? Why do I never see a second call to map, as I did for inverseMap with viewProperty?

> modelProperty.value = 2
CoefficientSlider.js?bust=1542329934673:158 map: 2 -> 4
CoefficientSlider.js?bust=1542329934673:163 inverseMap: 4 -> 2
< 2

> modelProperty.value = 2.1213203435596424
map: 2.1213203435596424 -> 4.499999999999999
inverseMap: 4.499999999999999 -> 2.1213203435596424
< 2.1213203435596424

[pixelzoom]

DynamicProperty appears to be missing guards to prevent a call to map from resulting in a call to inverseMap, and visa versa.

E.g. see onSelfChange:

    onSelfChange: function( value ) {
      if ( this.valuePropertyProperty.value !== null ) {
        this.derive( this.valuePropertyProperty.value ).value = this.inverseMap( value );
      }
    },

When the DynamicProperty is changed (viewProperty in my example), onSelfChange is called, and that changes the "outer" Property (modelProperty in my example). And any change to the outer Property will result in a call to this.map. That call to this.map needs to be short-circuited when the outer Property was changed as the result of changing the DynamicProperty.

[pixelzoom]

I tried adding (but did not commit) a couple of flags to short-circuit "ping ponging" of map and inverseMap. I don't know if this is complete (there are other calls to this.map) or correct (I'm not familiar with all the features of DynamicProperty), but it seems to solve the problem.

    this.callingMap = false; // true when we're in the middle of a call to this.map
    this.callingInverseMap = false; // true when we're in the middle of a call to this.inverseMap

...

    onPropertyPropertyChange: function( value ) {
      if ( !this.callingInverseMap ) {
        // Since we override the setter here, we need to call the version on the prototype
        this.callingMap = true;
        Property.prototype.set.call( this, this.map( value ) );
        this.callingMap = false;
      }
    },

...

   onSelfChange: function( value ) {
      if ( this.valuePropertyProperty.value !== null ) {
        if ( !this.callingMap ) {
          this.callingInverseMap = true;
          this.derive( this.valuePropertyProperty.value ).value = this.inverseMap( value );
          this.callingInverseMap = false;
        }
      }
    },

Previous console tests now behave as expected. That is, there is no "ping ponging" of map and inverseMap. And more importantly, reentrant: true can be removed from both modelProperty and viewProperty.

> viewProperty.value = 4
inverseMap: 4 -> 2
< 4

> viewProperty.value = 4.5
inverseMap: 4.5 -> 2.1213203435596424
< 4.5

> modelProperty.value = 2
map: 2 -> 4
< 2

> modelProperty.value = 2.1213203435596424
map: 2.1213203435596424 -> 4.499999999999999
< 2.1213203435596424

[pixelzoom]

I also verified that the guard flags added to DynamicProperty in #197 (comment) allow me to remove reentrant: true from the DynamicProperty in GRAPHING_QUADRATIC/CoefficientSlider. And it resolves the "reentry detected" assertion failure in Studio, allowing us to remove phetioStudioControl: false from the associated NumberProperties (see phetsims/graphing-quadratics#52).

[pixelzoom]

Top priority, since this fixes phetsims/graphing-quadratics#52 which is also top priority.

[samreid]

DynamicProperty unit tests pass with the changes proposed in #197 (comment)

[jonathanolson]

Hmm, it seems like ignoring onSelfChange in those cases will break DynamicProperty in other cases (where reentrance is legitimately used). I'll try to come up with an example.

I'd generally prefer that any time one of the properties changes, the other wouldn't change if either a===map(b) or inverseMap(a)===b, since this will not bounce at all, and seems a bit safer.

[pixelzoom]

Yeah, that's why I said:

I don't know if this is complete (there are other calls to this.map) or correct (I'm not familiar with all the features of DynamicProperty), but it seems to solve the problem.

So however you want to solve this is fine with me. The purpose of my "solution" was to illustrate the "bouncing", and to verify that eliminating the bouncing solved the other problems that we were seeing in Graphing Quadratics and Studio.

Whatever the solution, it also seems like DynamicPropertyTests should be augmented to verify behavior related to this issue.

[jonathanolson]

I'm running fairly comprehensive testing on a solution right now, I'll expect to commit soon.

It cuts off the Property changes in either direction when a===map(b) or inverseMap(a)===b (making sure that it is the same Property that the changes result from, etc.)

For testing, it's possible to use a map/inverseMap pair that are obviously not an "exact" inverse:

var sourceProperty = new axon.Property( 0 );
var wrapperProperty = new axon.Property( sourceProperty );
var dynamicProperty = new axon.DynamicProperty( wrapperProperty, {
  bidirectional: true,
  // NOT a true inverse
  map: n => n + 2,
  inverseMap: n => n - 1
} );
// source: 0, dynamic: 2

dynamicProperty.value = 3;
// source: 2, dynamic: 3

sourceProperty.value = 5;
// source: 5, dynamic: 7

[jonathanolson]

Pushed the main behavior, will be adding full tests.

[jonathanolson]

@ariel-phet should this change be reviewed?

[pixelzoom]

The example in #197 (comment) is not behaving as expected. See sample console output below. In all of the examples, the first call (to map or inverseMap) is unexpected and looks like it is somehow related to the previous example. And then the second call is duplicated.

// on startup
map: 0 -> 0
inverseMap: 0 -> 0

> viewProperty.value = 4
map: 0 -> 0
inverseMap: 4 -> 2
inverseMap: 4 -> 2
< 4

> viewProperty.value = 2
map: 2 -> 4
inverseMap: 2 -> 1.4142135623730951
inverseMap: 2 -> 1.4142135623730951
< 2

> viewProperty.value = 4.5
map: 1.4142135623730951 -> 2.0000000000000004
inverseMap: 4.5 -> 2.1213203435596424
inverseMap: 4.5 -> 2.1213203435596424
< 4.5

> modelProperty.value = 2
inverseMap: 4.5 -> 2.1213203435596424
map: 2 -> 4
map: 2 -> 4
< 2

> modelProperty.value = 2.1213203435596424
inverseMap: 4 -> 2
map: 2.1213203435596424 -> 4.499999999999999
map: 2.1213203435596424 -> 4.499999999999999
< 2.1213203435596424

[pixelzoom]

Discussed with @jonathanolson via Zoom. The first and third calls in each example are actually done in if expressions internal to DynamicProperty, to prevent ping-ponging. The second call is the only one that results in Property value assignment. So this is working as expected, here and in Graphing Quadratics. I feel like I've reviewed the code sufficiently, so closing.