support ?shuffleListeners

[zepumph]

From phetsims/axon#215, when running with ?shuffleListeners we get an error:

Color.js?bust=1575941002694:714 Uncaught Error: distance must be between 0 and 1: 2.131111111111111
    at Function.Color.interpolateRGBA (Color.js?bust=1575941002694:714)
    at Solution.getColor (Solution.js?bust=1575941002694:107)
    at update (ConcentrationDisplay.js?bust=1575941002694:200)
    at ConcentrationDisplay.js?bust=1575941002694:211
    at TinyEmitter.emit (TinyEmitter.js?bust=1575941002694:68)
    at Property._notifyListeners (Property.js?bust=1575941002694:275)
    at Property.set (Property.js?bust=1575941002694:176)
    at Property.set value [as value] (Property.js?bust=1575941002694:345)
    at ComboBoxListBox.Action.parameters.phetioPrivate (ComboBoxListBox.js?bust=1575941002694:74)
    at Action.execute (Action.js?bust=1575941002694:230)

I'll take a look.

[zepumph]

@samreid, while talking with you over in phetsims/axon#215 about tackling this problem, you had mentioned that it wasn't too challenging. I would like to employ your help in problem solving this one. Basically the simplified problem I'm seeing is that there are two listeners on model.soluteProperty. The first is from the derived model Property concentrationProperty (which is normally the very first listener added). The second is a view listener to update the color of the SolutionNode. So basically we have two very different functions of listeners being treated as the same because they are in the same list.

The first are listeners that change core model DerivedProperties.

The second are view listeners that need the entire model to be up to date before making their changes.

For this particular error, reproduce with ?shuffleListeners&fuzz, then you will get the error
Uncaught Error: distance must be between 0 and 1: {X} coming from a listener attached to the soluteProperty. This listener will likely be the very early on in the emit loop while notifyingListeners. Such that after this listener is the listener updating the concentrationProperty.

@samreid do you have any thoughts?

[samreid]

I experimented with setDeferred in ComboBoxListBox and Multilink in SolutionNode, but they both suffered the same problem. A while ago @jonathanolson suggested this problem could be solved by making the property more stateful, so it can be set atomically. Something like Property.<MolarityState> where MolarityState has Solute, concentration, etc. I think that may work, but that's an atypical pattern for us and I wonder (a) if it would be worth it or (b) if there is a better way.

UPDATE: Likewise, we could use numbers and Emitters so we could control when notifications go out (and handle atomic state sets), but that seems like a step in the wrong direction.

[samreid]

Here's a patch that may be better than nothing:

Index: js/molarity/model/Solution.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/molarity/model/Solution.js	(revision 820954e694f41f7d4f5fbec4391088f9068e882a)
+++ js/molarity/model/Solution.js	(date 1575951303816)
@@ -115,7 +115,11 @@
       if ( this.concentrationProperty.value > 0 ) {
         const solute = this.soluteProperty.get();
         const colorScale = Util.linear( 0, solute.saturatedConcentration, 0, 1, this.concentrationProperty.value );
-        return Color.interpolateRGBA( solute.minColor, solute.maxColor, colorScale );
+
+        // When listeners are fired in an arbitrary order, sometimes there is a transient, intermediate value that
+        // is out of range.  However, once all listeners have fired, this value should already be between 0..1
+        const clampedColorScale = Util.clamp( colorScale, 0, 1 );
+        return Color.interpolateRGBA( solute.minColor, solute.maxColor, clampedColorScale );
       }
       else {
         return this.solvent.color;

This makes the listener order independent, but I wonder if it may inadvertently mask problems even after all values have updated. What do you think?

[zepumph]

I think that we have two bulls butting heads:

1. One listener array for model and view listeners. Another classification would be "internal state updating" listeners" and "outside" listeners
2. ?shuffleListeners assuming independent listener order.

The above band-aid would likely work for that particular view listener.

[zepumph]

I'd like to know how to better handle these issues. In my opinion (and I feel like we've discussed this before), it should be allowed to have some set of listeners (even if we don't call if that) which must be done before anything else. In the past this was explicit and obvious because the link call would be right below the Property declaration in the model file.

Question to developer team at large:

How much energy do we want to spend fixing these "problems" exposed by shuffleListeners, and at what priority?

[zepumph]

I don't think this should block publication of Molarity coming up imminently.

[jonathanolson]

How much energy do we want to spend fixing these "problems" exposed by shuffleListeners, and at what priority?

I haven't felt the need personally to ensure things work with all permutations of listeners, but if so I think it would be great to have more support for this.

[zepumph]

After a large discussion with developers in today's dev meeting, here are some notes:

• The design pattern for "describer" types used for Interactive Descriptions produce many chances for listener order dependencies. This is similar to previous discussions that we have had regarding sonification, in which we decided that listening directly to listeners on UI components was often safer than linking directly to model Properties. I'm pretty sure that this isn't always possible with interactive descriptions.
• In general the team feels like order dependencies in listeners is not ideal. There wasn't momentum behind having model listeners be treated differently than view listeners (and not allowed to be shuffled together). Instead there was more focus on potentially adding "transactional" qualities to certain sets of Properties. For example to DerivedProperty, where each dependency is set to defer its listeners until the DerivedProperty has been updated. This could work in a similar way for Multilink.
• We also discussed an option to Property that allows it to opt out of the ?shuffleListeners logic. We liked this because it was a way to codify an order dependency instead of just having documentation, but decided against adding it right now, because for Molarity, a code comment can't be much closer to the order dependency than where the Property is declared (at least I'm pretty sure we all agreed about that).

For Molarity specifically, we aren't going to hold up publication. First we will see if the order dependency can be fixed by alerting based on the slider changing instead of based on the model Property. If that doesn't work, then I will document that listener order dependency.

Then later on in February there will be a sub-team meeting to investigate adding transaction support to multi-dependency Types. I'll set a calendar reminder for February.

[samreid]

I think it could help a lot if we flag DerivedProperty listeners for identification, then update all DerivedProperties in a tree before sending any notifications. I don't think that strategy would work for multilink, which is more about side effects. @zepumph would getting all of the DerivedProperty values correct before notifications address the interactive description style problems mentioned in this issue?

[zepumph]

@zepumph would getting all of the DerivedProperty values correct before notifications address the interactive description style problems mentioned in this issue?

Yes that think that may be helpful, though in one particular case I decided to move the logic from a Property link over to the endDrag of a slider (#206). I'm excited to see how phetsims/axon#276 goes.

[zepumph]

This issue was quite helpful for understanding how PhET uses listener order in its notification code. To me, we fully depend on listener order in many many cases, most often in regards to how you wire up changes to model Properties in the model, and also listen to the Properties in the view, often within the same listener list. While ?shuffleListeners can be a useful tool in understanding this, as a team we are not going all in on ensuring the sim runs with it on. Closing