phetsims / axon Goto Github PK

PropertySet.multilink provides this functionality, but if your Properties aren't part of the same PropertySet (or any PropertySet), you're out of luck.

It was suggested that I look at using (@static) DerivedProperty.multilink. A couple of problems with that. I don't need or want a DerivedProperty, and the observer function is not going to return a derived value. So this feels like an abuse of DerivedProperty. And I'm wondering why DerivedProperty.multilink exists - since it is identical to the constructor, it looks to me like it exists solely to perform this abuse. So I'm not going to use it, and I recommend that it be deprecated.

So we need something that does this. Here's what was discussed on Skype, to be added to Property:

  /**
   * Registers an observer with multiple properties, then notifies the observer immediately.
   * @param {Array<Property>} properties
   * @param {function} observer no params, returns nothing
   * @static
  */
  axon.Property.multilink = function( properties, observer ) {
    var numProperties = properties.length;
    for ( var i = 0; i < numProperties; i++ ) {
      properties[i].lazyLink( observer );
    }
    observer();
  };

Sims are crashing on IE10, just showing the splash screen, and showing this error message:

SCRIPT1051: Setter functions must have one argument 
forces-and-motion-basics_en.html, line 87 character 6221

It is referring to this line of code:

set value() { throw new Error( 'Cannot es5-set values directly to a derived property'); },

However, the source line is:

set value( newValue ) { throw new Error( 'Cannot es5-set values directly to a derived property'); },

Perhaps the argument was trimmed out during compilation?

To match naming conventions in axon.Events, we decided to rename notifyObserversUnsafe to notifyObserversStatic in Property.

The client currently has to keep track of the initial state of the array, perform a clear, then reinitialize the array. This should all be handled by ObservableArray, similar to Property.

DerivedProperty is loosely based on Property.js, but could extend it to get all its functionality and stay in sync. We may have to rename the set() method but I think it could be made to work.

DerivedProperty.detach does this:

detach: function() {
  for ( var i = 0; i < this.dependencies.length; i++ ) {
      var dependency = this.dependencies[i];
      dependency.unlink( this.dependencyListeners[i] );
  }
}

But it neglects to clear the arrays this.dependencies and this.dependencyListeners. Both of these contain references to the derived properties (through a function closure in dependencyListeners). So while the observers are unlinked from the derived properties, the derived property still holds references to the properties that it's derived from.

Here's an example:

var Property = require( 'AXON/Property' );
var DerivedProperty = require( 'AXON/DerivedProperty' );
var widthProperty = new Property( 2 );
var heightProperty = new Property( 3 );
var areaProperty = new DerivedProperty( [ widthProperty, heightProperty ], 
  function( width, height ) { return width * height; } );
areaProperty.link( function( area ) { console.log( 'area = ' + area );  } );
debugger;

In a browser console:

> widthProperty._observers.length
1 // ok
> heightProperty._observers.length
1 // ok
> areaProperty.dependencies.length
2 // ok
> areaProperty.dependencyListeners.length
2 // ok
> areaProperty.detach()
undefined
> widthProperty._observers.length
0 // ok
> heightProperty._observers.length
0 // ok
> areaProperty.dependencies.length
2 // should be 0
> areaProperty.dependencyListeners.length
2 // should be 0

I'd like one other developer to confirm that I'm interpreting this correctly. Assigning to @samreid since he was the primary author of DerivedProperty.

I would add one, but I'm unclear what's going on with licenses. Looks like common-code repos have a different license than sims.

Proposed API:

// add this.visible (ES5 getter/setter) and this.visibleProperty, with a new Property( true )
Property.addProperty( this, 'visible', true );

UCB is requesting support for a study where they collect data on student usage. This ticket is primarily for tracking commits.

Backbone and jquery have nice event channel patterns that we may wish to integrate into PropertySet. Let's discuss.

In the future we are planning to do sim event data collection through input recording, not through model recording. So that should be removed, and this ticket can be a paper trail for if we ever need to find it again easily.

After more experience with ObservableArray, I'm really not digging the listener interface. A listener has the form function({Array}added, {Array}removed, array ). But one of added or removed is always null, and there is never more than 1 item in the arrays. But I can rely on this in client code, so I'm having to write boilerplate in every callback that iterates over 2 arrays.

Proposed new interface:

addItemAddedListener( callback )
addItemRemovedListener( callback )
addListener( itemAddedCallback, itemRemovedCallback ) // as a convenience

/** 
 * Callbacks have this form.
 * @param {*} item the item that was added to or removed from the array
 * @param {ObservableArray} observableArray the observable array, post modification
 */
function( item, observableArray )

This came up in acid-base-solutions.AcidBaseSolutionsAbstractModel, where observers are rewired when the solution changes. We'd like to use linkAttribute to link to strength and concentration, but there is no unlinkAttribute. So the implementation resorts to using functions.

Currently when a PropertySet is created with a Property called, say, 'mass', the mass property can be accessed as propertySet.massProperty. This shows up in IDEA as an unknown variable, and cannot be navigated to. However, if we made the getter work like propertySet.property('mass') then it won't be marked as an undefined var and you can use navigation to navigate to the source. Should we do something like this?

Property.js has this comment:

// TODO: JO: avoid slice() by storing observers array correctly

@jonathanolson can you please elaborate on how to store observers "correctly"?

The PropertySet object treats all its members as plain Property items, because of this we couldnt use NumberProperty methods such as lessThanNumber and greaterThanNumber. This limitation is forcing us to explicitly define many such NumberProperty members on a Object that already extends PropertySet. It would simplify the make the code consistent If lessThanNumber and greaterThanNumber are moved to Property.

In addition to this, it would also be ideal to have "plus" method similar to "times" in Property Object

@jonathanolson reported:

FAMB is throwing an assertion for:

    var showGoPauseButtonProperty = model.toDerivedProperty( ['running', 'state', 'numberPullersAttached'], function( running, state, numberPullersAttached ) {
      goPauseButton.visible = state !== 'completed' && (numberPullersAttached > 0 || running);
    } );
    showGoPauseButtonProperty.linkAttribute( this, 'visible' );

in GoPauseButton.The last line causes setVisible( undefined ) to be called, because the derived property is undefined (hasn't been initialized yet).

As of this writing, observable arrays only support monitoring of all changes, and not of individual events such as item added, item removed, array cleared. Finer grained resolution would simplify code and potentially improve performance.

I think the more general map function that would be useful in axon.Property take a function as an argument rather than a lookup table. I recommend to rename map to mapValues and adding a function map as the new map. Another possibility would be to overload map to take a function or options hash (based on input type), but it is usually our convention to avoid that, thinking that may save us some refactoring trouble in the future.

Please note this is a breaking change to axon.Property.map. But as far as I know, this function is only used in a few places so far.

In phetsims/balancing-act#7 @jonathanolson pointed out code like this is being used:

      gameModel.movableMasses.addItemRemovedListener( function() {
        thisScreen.challengeLayer.removeChild( massNode );
        gameModel.movableMasses.removeItemRemovedListener( this );
      } );

Which causes a problem on the last line since the this is not the function. The current solution is to use a named anonymous function:

      gameModel.movableMasses.addItemRemovedListener( function massAddListener() {
        thisScreen.challengeLayer.removeChild( massNode );
        gameModel.movableMasses.removeItemRemovedListener( massAddListener );
      } );

But we could also consider adding support to axon and other listener patterns to make this be the listener function.

At today's meeting, @jonathanolson suggested a varargs derived property constructor like:

//proposed JS implementation
anySolutes = new DerivedProperty( salt.moles, sugar.moles ,function(saltMoles,sugarMoles){

   return saltMoles > 0 || sugarMoles > 0;

});

instead of what we have now:

//proposed JS implementation
anySolutes = new DerivedProperty([salt.moles,sugar.moles],function(saltMoles,sugarMoles){

   return saltMoles > 0 || sugarMoles > 0;

});

This would affect several of the DerivedProperty creation methods as well. We would have to decide whether we want to support both array and varargs construction, or to update all old references to use varargs.

No specific deadline on this, just something on our wish list 🌠

I've been following the scenery/scenery-phet/sun convention of using dispose as the name of the function that does cleanup at the end of an object's lifetime. Should we rename DerivedProperty.detach to dispose, so that it's consistent with this naming convention? (And rename similarly for other AXON APIs.)

Assigning to @samreid as the author of DerivedProperty.detach, but other developers please chime in.

Multilink and DerivedProperty contain a lot of duplicate code, looks like a copy-paste job. This should be factored out.

Additional support for the copy-paste theory is this erroneous comment in Multilink.detach:

Detaches this derived property from its dependencies.

DerivedProperty has these methods:

      //Override the mutators to provide an error message.  These should not be called directly, the value should only be modified when the dependencies change
      set: function( value ) { throw new Error( 'Cannot set values directly to a derived property, tried to set: ' + value ); },

      //Override the mutators to provide an error message.  These should not be called directly, the value should only be modified when the dependencies change
      //Keep the newValue output in the string so the argument won't be stripped by minifier (which would cause crashes like https://github.com/phetsims/axon/issues/15)
      set value( newValue ) { throw new Error( 'Cannot es5-set values directly to a derived property, tried to set: ' + newValue ); },

      //Override get value as well to satisfy the linter which wants get/set pairs (even though it just uses the same code as the superclass).
      get value() {return Property.prototype.get.call( this );},

      //Override the mutators to provide an error message.  These should not be called directly, the value should only be modified when the dependencies change
      reset: function() { throw new Error( 'Cannot reset a derived property directly' ); }

Maybe it is better to remove these methods from the instance's interface instead? This could be done in the constructor, I suppose. @jonathanolson @pixelzoom @jbphet any thoughts?

As discussed in a recent developer meeting, we could skip allocation of the listener array when sending Property.js notifications. It seems the fastest way would be to maintain two lists and sync after the set method if one changed.
Flag 1. Are you in an iteration over observers flag (true in slice and false after the loop).
Flag 2. Was the observer list modified during the loop traversal
Centralize push/remove listeners, so it can maintain two lists.

Flag must be a counter, if it needs to be re-entrant.

Another way to avoid the allocation (but slower) would be to use listenerCopy.length=0 and push all.

At a recent developer meeting we discussed modifying Property.js so that it can be mixed in or used in core classes like Vector2, to support allocation-free messaging as discussed in #6 . @jonathanolson said he had some ideas about how to proceed with this.

The main reason for this idea is so that the listener pattern can be used easily and uniformly without requiring new allocations each time a Vector2 changes (as is the current axon idiom).

See phetsims/molecule-shapes#99

I recommend to rename as PropetySet.propertySetID.

Will try to get to it today.

In phetsims/forces-and-motion-basics#55 we observed that sometimes calling object.someProperty.get() returned different values than just object.some. It is not clear what is causing the problem (could be something specific to Forces and Motion: Basics or more general) and where else the problem may manifest.

See #29.

Make it possible to use Events independently of PropertySet

Some inconsistencies/holes in the AXON API related to 'multilinks'...

(1) Different objects created for similar functions.
Property.mulitlink creates a Multilink.
PropertySet.multilink creates a DerivedProperty.
Should we changePropertySet.multilink to create a Multilink? (looks pretty trivial, but should be tested)

(2) Missing functionality.
PropertySet has unmultilink.
Property has no such function.
Should we add Property. unmultilink? (looks pretty trivial)

Property and PropertySet interchange 'observer' and 'listener', in both documentation and variable names. Observer should be used throughout.

PropertySet.unmultilink refers to a derivedProperty, when in fact it's a Multilink.

@dkras said:

Our developer has a question regarding an issue we have in the Friction app.
He believes that the bug is caused by an issue in the underlying framework Axon.
The issue is due to the way the listeners to the model changes are being removed from the active list.
The particular line of the code is in the file https://github.com/phetsims/axon/blob/master/js/Property.js
120) this._observers.splice( index, index + 1 )

Our developer believes this should be changed to
this._observers.splice( index, 1 )

since the second parameter is the number of elements to remove and not the index of the removed element.

Could you please check on this and confirm or get back to me with explanation why we can be wrong.

This came up in the context of phetsims/ph-scale#40.

Would it be generally useful if derived properties were updated before notifying general observers? That would ensure that derived properties had been updated and weren't 'stale' if consulted by the observers.

Review the 22 TODOs in source code, including 9 in PropertySet. Address them, or make GitHub issues.

Performance tests do not seem like the arch changes will slow things down (when phetEvents.active = false). The performance test results #33 were high variance, but there is a guard on every phetEvents message and that may be sufficient.

So I think this is a good time to perform a larger review of the arch implementation and see if it is suitable (a) for master and (b) if the design seems scalable enough for future studies or (c) if we should make API changes before proliferating usage.

@pixelzoom can you please take a look at the arch branch of axon and discuss it with me? You will probably need to look at it in conjunction with the arch branches in sun and joist as well.

An issue that comes up in code reviews is that passing an entire model to a dependency breaks encapsulation, and only the necessary parts should be passed. We can provide support for this in axon.Events by creating a method that passes a reduced Events interface, so that the dependent class won't have access to the entire Events. Something else may be possible for PropertySet.

In order to match the naming of other higher order Property derivations such as and, or, I recommend to rename derivedNot to not, and rename what is currently not to invertibleNot or something, so all of the non-invertible properties will have the same naming conventions.

Currently, property values send notifications if newValue!==oldValue. But this could send a false positive when the value didn't truly change. For example if setting

p.position = new Vector2(10,10);
p.position = new Vector2(10,10);

that will send two notifications, because the 2nd Vector2 !== the 1st Vector2. We may wish to implement .equals() functions for comparing instances. However, this would increase the complexity of implementation for values used by Property, and equality checks can take longer at runtime.

In phetsims/reactants-products-and-leftovers#18, @pixelzoom said:

It sure would be nice if AXON had some support for summarizing Properties and how many observers they have.

Suggestions for an API?

We'd like to merge the arch branch to master, but are concerned that it could decrease performance. We need to test the performance of the arch branch and compare it to master branch.

Our group's recent convention has been to use:

this.locationProperty

instead of

this.property('location');

Let's reopen discussion briefly on this. Can/should we remote the latter? Having two ways of doing the same thing may create maintenance issues here.

The stub for arch is created by axon.Property.initArch, and it's called from joist.Sim. Let's find a better place for this.

A bunch of misc cleanup needed in PropertySet:

• @param name and propertyName used interchangeably
• @param propertyNames and dependencies used interchangeably
• @param dependencies used in cases where there's no DerivedProperty involved
• no documentation for many function
• no documentation for propertyID (apparently related to data communication)
• creation of array index duplicated repeatedly: propertyName + 'Property'

The Property.one function doesn't unlink the observer until after it fires it, which means that if the observer causes some action which triggers another change to the property, the observer can get fired again. It would be better to first unlink the observer and then invoke it so the observer doesn't end up getting called multiple times.

For reference, this issue was discovered while working on the interaction between orphan release and animation observers in the Area Builder simulation.

In axon, if you want to have a 2d-position property you would traditionally use a Property instance wrapped around a Vector2 (possibly created with PropertySet). Normally we would create a new Vector2 and set it to the PropertySet to fire notifications, but this requires allocating a new Vector2 (unless pooling was used). An alternative that @jonathanolson suggested would be to mutate the existing Vector2 in-place and send out a notification that it had changed, if we need to avoid the re-allocation. Not sure this warrants working on now, but I wanted to write it down in case we need to investigate in the future for reducing garbage collection.

For scenery, it is important to receive callbacks when events listeners are added or removed. For now, this feature is implemented in scenery using a custom solution, but we wanted to create this issue in case it makes sense to add this feature into axon in the future.

PropertySet has a set function to set multiple values at once, and it would be good to have a get function to get all state in a JS object (e.g., for serialization).

In LineFormsGraphNode.js I saw this usage:

    // Visibility of lines
    Property.multilink( [ viewProperties.linesVisibleProperty, viewProperties.interactiveLineVisibleProperty, viewProperties.slopeVisibleProperty ],
      thisNode.updateLinesVisibility.bind( thisNode ) );

We could potentially add support to axon for the binding argument. The above case could be made to read like so:

    // Visibility of lines
    Property.multilink( [ viewProperties.linesVisibleProperty, viewProperties.interactiveLineVisibleProperty, viewProperties.slopeVisibleProperty ],
      updateLinesVisibility,thisNode);

However, I don't know how prevalent this binding pattern is, or if it would need to be applied to other kinds of link/multilink/lazy/etc.