should we change bidirectional from an option to a subtype?
@jonathanolson replied:
That sounds possible (they should be bidirectional or not on creation), however unless we use mixins (yuk), I don't see how that would fix the inheritance hierarchy.
@marlitas and I sketched out what BidirectionalDynamicProperty would look like, and it didn't exactly solve the problem from https://github.com/phetsims/density-buoyancy-common/issues/74 but we wanted to put a patch here in case we want to pursue BidirectionalDynamicProperty anyways. We saw around 38 cases of bidirectional: true that would be converted.
@jonathanolson do you like this direction?
```diff
Index: js/DynamicProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/DynamicProperty.ts b/js/DynamicProperty.ts
--- a/js/DynamicProperty.ts (revision a5961582e9fed7de7a0df002257961872cec4d7c)
+++ b/js/DynamicProperty.ts (date 1661357767234)
@@ -60,23 +60,6 @@
* } );
* So that if the currentSceneProperty's value is null, the value of our DynamicProperty will be Color.BLACK.
*
- *******************************
- * 'bidirectional' option
- *******************************
- * If you would like for direct changes to this Property to change the original source (bidirectional synchronization),
- * then pass bidirectional:true:
- * const firstProperty = new Property( 5 );
- * const secondProperty = new Property( 10 );
- * const numberPropertyProperty = new Property( firstProperty );
- * const dynamicProperty = new DynamicProperty( numberPropertyProperty, { bidirectional: true } );
- * dynamicProperty.value = 2; // allowed now that it is bidirectional, otherwise prohibited
- * firstProperty.value; // 2
- * numberPropertyProperty.value = secondProperty; // change which Property is active
- * dynamicProperty.value; // 10, from the new Property
- * dynamicProperty.value = 0;
- * secondProperty.value; // 0, set above.
- * firstProperty.value; // still 2 from above, since our dynamic Property switched to the other Property
- *
*******************************
* 'map' and 'inverseMap' options
*******************************
@@ -107,9 +90,6 @@
export type TNullableProperty = TReadOnlyProperty | TReadOnlyProperty;
type SelfOptions = {
- // If set to true then changes to this Property (if valuePropertyProperty.value is non-null at the time) will also be
- // made to derive( valuePropertyProperty.value ).
- bidirectional?: boolean;
// If valuePropertyProperty.value === null, this dynamicProperty will act instead like
// derive( valuePropertyProperty.value ) === new Property( defaultValue ). Note that if a custom map function is
@@ -143,14 +123,13 @@
export default class DynamicProperty> extends ReadOnlyProperty {
// Set to true when this Property's value is changing from an external source.
- private isExternallyChanging: boolean;
+ protected isExternallyChanging: boolean;
private defaultValue: InnerValueType;
protected derive: ( u: OuterValueType ) => TProperty;
protected map: ( v: InnerValueType ) => ThisValueType;
protected inverseMap: ( t: ThisValueType ) => InnerValueType;
- protected bidirectional: boolean;
- private valuePropertyProperty: TNullableProperty;
+ protected valuePropertyProperty: TNullableProperty;
private propertyPropertyListener: ( value: InnerValueType, oldValue: InnerValueType | null, innerProperty: TReadOnlyProperty | null ) => void;
private propertyListener: ( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ) => void;
@@ -161,7 +140,6 @@
public constructor( valuePropertyProperty: TNullableProperty | TReadOnlyProperty, providedOptions?: DynamicPropertyOptions ) {
const options = optionize, SelfOptions, PropertyOptions>()( {
- bidirectional: false,
defaultValue: null as unknown as InnerValueType,
derive: _.identity,
map: _.identity,
@@ -187,7 +165,6 @@
this.derive = derive;
this.map = map;
this.inverseMap = inverseMap;
- this.bidirectional = options.bidirectional;
this.valuePropertyProperty = valuePropertyProperty;
this.isExternallyChanging = false;
@@ -196,12 +173,6 @@
// Rehook our listener to whatever is the active Property.
valuePropertyProperty.link( this.propertyListener );
-
- // If we aren't bidirectional, we should never add this listener.
- if ( options.bidirectional ) {
- // No unlink needed, since our own disposal will remove this listener.
- this.lazyLink( this.onSelfChange.bind( this ) );
- }
}
/**
@@ -212,20 +183,7 @@
* @param oldValue - Ignored for our purposes, but is the 2nd parameter for Property listeners.
* @param innerProperty
*/
- private onPropertyPropertyChange( value: InnerValueType, oldValue: InnerValueType | null, innerProperty: TReadOnlyProperty | null ): void {
-
- // If the value of the inner Property is already the inverse of our value, we will never attempt to update our
- // own value in an attempt to limit "ping-ponging" cases mainly due to numerical error. Otherwise it would be
- // possible, given certain values and map/inverse, for both Properties to toggle back-and-forth.
- // See https://github.com/phetsims/axon/issues/197 for more details.
- if ( this.bidirectional && this.valuePropertyProperty.value !== null && innerProperty ) {
- const currentProperty = this.derive( this.valuePropertyProperty.value );
- // Notably, we only want to cancel interactions if the Property that sent the notification is still the Property
- // we are paying attention to.
- if ( currentProperty === innerProperty && innerProperty.areValuesEqual( this.inverseMap( this.value ), innerProperty.get() ) ) {
- return;
- }
- }
+ protected onPropertyPropertyChange( value: InnerValueType, oldValue: InnerValueType | null, innerProperty: TReadOnlyProperty | null ): void {
// Since we override the setter here, we need to call the version on the prototype
super.set( this.map( value ) );
@@ -252,25 +210,6 @@
}
}
- /**
- * Listener added to ourself when we are bidirectional
- */
- private onSelfChange( value: ThisValueType ): void {
- assert && assert( this.bidirectional );
-
- if ( this.valuePropertyProperty.value !== null ) {
- const innerProperty = this.derive( this.valuePropertyProperty.value );
-
- // If our new value is the result of map() from the inner Property's value, we don't want to propagate that
- // change back to the innerProperty in the case where the map/inverseMap are not exact matches (generally due
- // to floating-point issues).
- // See https://github.com/phetsims/axon/issues/197 for more details.
- if ( !this.areValuesEqual( value, this.map( innerProperty.value ) ) ) {
- innerProperty.value = this.inverseMap( value );
- }
- }
- }
-
/**
* Disposes this Property
*/
@@ -284,31 +223,6 @@
super.dispose();
}
- /**
- * Resets the current property (if it's a Property instead of a TinyProperty)
- */
- public reset(): void {
- assert && assert( this.bidirectional, 'Cannot reset a non-bidirectional DynamicProperty' );
-
- if ( this.valuePropertyProperty.value !== null ) {
- const property = this.derive( this.valuePropertyProperty.value );
- ( property as Property ).reset();
- }
- }
-
- /**
- * Prevent setting this Property manually if it is not marked as bidirectional.
- */
- public override set( value: ThisValueType ): void {
- assert && assert( this.bidirectional,
- `Cannot set values directly to a non-bidirectional DynamicProperty, tried to set: ${value}` );
-
- this.isExternallyChanging = true;
- super.set( value );
-
- this.isExternallyChanging = false;
- }
-
/**
* Overridden to make public
*/
@@ -324,13 +238,6 @@
public override set value( value: ThisValueType ) {
this.set( value );
}
-
- /**
- * Returns true if this Property value can be set externally, by set() or .value =
- */
- public override isSettable(): boolean {
- return super.isSettable() && this.bidirectional;
- }
}
axon.register( 'DynamicProperty', DynamicProperty );
Index: js/BidirectionalDynamicProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/BidirectionalDynamicProperty.ts b/js/BidirectionalDynamicProperty.ts
new file mode 100644
--- /dev/null (date 1661358105803)
+++ b/js/BidirectionalDynamicProperty.ts (date 1661358105803)
@@ -0,0 +1,153 @@
+// Copyright 2017-2022, University of Colorado Boulder
+
+/**
+ *******************************
+ * 'bidirectional' features
+ *******************************
+ * If you would like for direct changes to this Property to change the original source (bidirectional synchronization),
+ * then pass bidirectional:true:
+ * const firstProperty = new Property( 5 );
+ * const secondProperty = new Property( 10 );
+ * const numberPropertyProperty = new Property( firstProperty );
+ * const dynamicProperty = new BidirectionalDynamicProperty( numberPropertyProperty, { bidirectional: true } );
+ * dynamicProperty.value = 2; // allowed now that it is bidirectional, otherwise prohibited
+ * firstProperty.value; // 2
+ * numberPropertyProperty.value = secondProperty; // change which Property is active
+ * dynamicProperty.value; // 10, from the new Property
+ * dynamicProperty.value = 0;
+ * secondProperty.value; // 0, set above.
+ * firstProperty.value; // still 2 from above, since our dynamic Property switched to the other Property
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ * @author Marla Schulz (PhET Interactive Simulations)
+ */
+
+import axon from './axon.js';
+import Property from './Property.js';
+import TReadOnlyProperty from './TReadOnlyProperty.js';
+import DynamicProperty, { DynamicPropertyOptions, TNullableProperty } from './DynamicProperty.js';
+
+// @ts-ignore
+export default class BidirectionalDynamicProperty> extends DynamicProperty implements Property {
+
+ // @ts-ignore
+ protected _initialValue: ThisValueType;
+
+ // getInitialValue, initialValue, setInitialValue
+
+ public constructor( valuePropertyProperty: TNullableProperty | TReadOnlyProperty, providedOptions?: DynamicPropertyOptions ) {
+ super( valuePropertyProperty, providedOptions );
+ // If we aren't bidirectional, we should never add this listener.
+ // No unlink needed, since our own disposal will remove this listener.
+ this.lazyLink( this.onSelfChange.bind( this ) );
+ }
+
+ /**
+ * Listener added to the active inner Property.
+ *
+ * @param value - Should be either our defaultValue (if valuePropertyProperty.value is null), or
+ * derive( valuePropertyProperty.value ).value otherwise.
+ * @param oldValue - Ignored for our purposes, but is the 2nd parameter for Property listeners.
+ * @param innerProperty
+ */
+ protected override onPropertyPropertyChange( value: InnerValueType, oldValue: InnerValueType | null, innerProperty: TReadOnlyProperty | null ): void {
+
+ // If the value of the inner Property is already the inverse of our value, we will never attempt to update our
+ // own value in an attempt to limit "ping-ponging" cases mainly due to numerical error. Otherwise it would be
+ // possible, given certain values and map/inverse, for both Properties to toggle back-and-forth.
+ // See https://github.com/phetsims/axon/issues/197 for more details.
+ if ( this.valuePropertyProperty.value !== null && innerProperty ) {
+ const currentProperty = this.derive( this.valuePropertyProperty.value );
+ // Notably, we only want to cancel interactions if the Property that sent the notification is still the Property
+ // we are paying attention to.
+ if ( currentProperty === innerProperty && innerProperty.areValuesEqual( this.inverseMap( this.value ), innerProperty.get() ) ) {
+ return;
+ }
+ }
+
+ // Since we override the setter here, we need to call the version on the prototype
+ super.onPropertyPropertyChange( value, oldValue, innerProperty );
+ }
+
+ /**
+ * Listener added to ourself when we are bidirectional
+ */
+ private onSelfChange( value: ThisValueType ): void {
+
+ if ( this.valuePropertyProperty.value !== null ) {
+ const innerProperty = this.derive( this.valuePropertyProperty.value );
+
+ // If our new value is the result of map() from the inner Property's value, we don't want to propagate that
+ // change back to the innerProperty in the case where the map/inverseMap are not exact matches (generally due
+ // to floating-point issues).
+ // See https://github.com/phetsims/axon/issues/197 for more details.
+ if ( !this.areValuesEqual( value, this.map( innerProperty.value ) ) ) {
+ innerProperty.value = this.inverseMap( value );
+ }
+ }
+ }
+
+ /**
+ * Resets the current property (if it's a Property instead of a TinyProperty)
+ */
+ public reset(): void {
+ if ( this.valuePropertyProperty.value !== null ) {
+ const property = this.derive( this.valuePropertyProperty.value );
+ ( property as Property ).reset();
+ }
+ }
+
+ /**
+ * Prevent setting this Property manually if it is not marked as bidirectional.
+ */
+ public override set( value: ThisValueType ): void {
+ this.isExternallyChanging = true;
+ super.set( value );
+
+ this.isExternallyChanging = false;
+ }
+
+ /**
+ * Overridden to make public
+ */
+ public override get value(): ThisValueType {
+ return super.value;
+ }
+
+ /**
+ * Overridden to make public
+ * We ran performance tests on Chrome, and determined that calling super.value = newValue is statistically significantly
+ * slower at the p = 0.10 level( looping over 10,000 value calls). Therefore, we prefer this optimization.
+ */
+ public override set value( value: ThisValueType ) {
+ this.set( value );
+ }
+
+ /**
+ * Returns true if this Property value can be set externally, by set() or .value =
+ */
+ public override isSettable(): boolean {
+ return true;
+ }
+
+ /**
+ * Returns the initial value of this Property.
+ */
+ public getInitialValue(): ThisValueType {
+ return this._initialValue;
+ }
+
+ public get initialValue(): ThisValueType {
+ return this.getInitialValue();
+ }
+
+ /**
+ * Stores the specified value as the initial value, which will be taken on reset. Sims should use this sparingly,
+ * typically only in situations where the initial value is unknowable at instantiation.
+ */
+ public setInitialValue( initialValue: ThisValueType ): void {
+ this._initialValue = initialValue;
+ }
+}
+
+axon.register( 'BidirectionalDynamicProperty', BidirectionalDynamicProperty );
\ No newline at end of file
Index: js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/ReadOnlyProperty.ts b/js/ReadOnlyProperty.ts
--- a/js/ReadOnlyProperty.ts (revision a5961582e9fed7de7a0df002257961872cec4d7c)
+++ b/js/ReadOnlyProperty.ts (date 1661356616216)
@@ -199,7 +199,7 @@
* Returns true if the value can be set externally, using .value= or set()
*/
public isSettable(): boolean {
- return true;
+ return false;
}
/**
```
In https://github.com/phetsims/density-buoyancy-common/issues/74, I asked:
@jonathanolson replied:
@marlitas and I sketched out what BidirectionalDynamicProperty would look like, and it didn't exactly solve the problem from https://github.com/phetsims/density-buoyancy-common/issues/74 but we wanted to put a patch here in case we want to pursue BidirectionalDynamicProperty anyways. We saw around 38 cases of
bidirectional: truethat would be converted.@jonathanolson do you like this direction?
Seems like a good direction. Would it prevent TProperty with a non-bidirectional DynamicProperty?
This proposal seems reasonable but also like a lot of work. I don't think it is a priority at the moment. Unassigning for now.