-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Node Properties should not be allowed to have their listeners shuffled. #1131
Comments
Why is it OK to rely on listener order in scenery, but not in other code? |
I kinda feel like this is in part a deficiency in our listener library. I am getting flash backs to our lengthy discussion about separating Action from Emitter. The essence of that issue, this issue, and the molarity issue referenced above, is that there is code in listeners currently that is really foundational to the "value" of the element being listened to, and so it should occur before other items. My knowledge of design patterns is not good enough to be able to communicate what this "listener prime frankenstein pattern" is called. It isn't about prioritizing listeners as much as using AXON-style listening to accomplish something that is more foundational to the task than just listening. Making sure that visibility is updated in the Node from the state of the Property is much more foundational than any outside listener looking at that Node's visibility. To try to explore what I mean, I'll come up with a bad example. If you were creating a system to model a human body, you might have an important type called I understand having a problem with order dependencies in listeners, and I definitely feel like |
Marking high priority because this is getting some traction, and it would be good to get this off of dev meeting in a timely way. |
Some of these problems are the result of trying to combine too many responsibilities into a single Emitter (or Action, or Property, or other observable). To continue with @zepumph's "Brain" example... If it's important that "C-spine" listeners finish before the pinky toe listener is called, then the pinky toe should not be listening to the neuronXFiringEmitter - it should be listening to an Emitter whose firing says "a neuron fired and the C-spine has finished reacting". If there are ordering dependencies, then they can be codified as a chain of observables that notify in the required order. |
@zepumph said:
Here's how the "chain of observables" pattern could be applied to this problem in Node -- let's say for Those So something like this: // Node.js
this.visibleChangedEmitter = new Emitter( ... );
this.visibleProperty.lazyLink( visible => {
// Do whatever Node needs to do before clients are notified of the visibility change, then...
this.visibleChangedEmitter( visible );
} );
// client code
const myNode = new Node(...);
myNode.visibleChangedEmitter.addListener( visible => {
// Do whatever the client needs to do to react to the visibility change.
} ); Yeah, this is a big departure from using |
Some more comments....
By using a listener (and relying on ordering) to make the observable's state consistent, this fundamental rule is being voliated by
So... let's just say that I'm very skeptical that specifying (and relying on) an order of notification for Property/Emitter/etc. is a desirable direction. |
From 2/4/21 dev meeting: A subgroup of @pixelzoom, @zepumph, and @samreid will meet again to discuss further. |
More notes from 2/4/21 dev meeting... Consensus was that putting the subject in a consistent state should not be a responsibility of a listener. It should be something that is done before listeners are notified. So rather than relying on a listener order, we should investigate other patterns for separating these responsibilities (hence the subgroup mentioned in the previous comment). Off the top of my head, I'm not sure what those "other patterns" might be, and I'll need to give that some deeper thought. But since the GoF book is fresh in my mind, it recommends looking at the Mediator pattern "to ensure that observers are notified only after subjects have been modified". This section isn't totally clear to me, but here it is. Implementation subjection, item 8, p. 299-300, their emphasis:
|
Just a reminder that phetsims/ratio-and-proportion#300 is on hold until this issue progresses further. |
The old pattern is this._visibleProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.visible, true );
this._visibleProperty.lazyLink( this.onVisiblePropertyChange.bind( this ) ); Here is one idea for solving this problem: this._visibleProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.visible, true, {
beforeListeners: this.onVisiblePropertyChange.bind( this )
} ); @pixelzoom pointed out this paper trail regarding this approach: phetsims/axon#222 and phetsims/axon#217 |
Allowing a single listener to be called before might be more memory-efficient in general. |
Another possibility would be to wrap the |
I'm having deja vu. Didn't we go down the
It seems reasonable to have 1 optional callback that is responsible for ensuring that the Observable is in a consistent state before Observers are notified. I'd like to avoid referring to such a callback as a "listener" or "before listener", to keep it separate from the Observer pattern. I don't have a name to suggest yet, will think about it. And I definitely think it should be a single callback, to avoid ordering and coupling problems. |
The reason we didn't need |
Working patch: Index: main/scenery/js/nodes/Node.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery/js/nodes/Node.js b/main/scenery/js/nodes/Node.js
--- a/main/scenery/js/nodes/Node.js (revision 63fa15e487a2e4f6f31d4df25e74a3ce459f7dc2)
+++ b/main/scenery/js/nodes/Node.js (date 1612980025450)
@@ -335,8 +335,7 @@
// @private {TinyForwardingProperty.<boolean>} - Whether this Node (and its children) will be visible when the scene is updated.
// Visible Nodes by default will not be pickable either.
// NOTE: This is fired synchronously when the visibility of the Node is toggled
- this._visibleProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.visible, true );
- this._visibleProperty.lazyLink( this.onVisiblePropertyChange.bind( this ) );
+ this._visibleProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.visible, true, this.onVisiblePropertyChange.bind( this ) );
// @public {TinyProperty.<number>} - Opacity, in the range from 0 (fully transparent) to 1 (fully opaque).
// NOTE: This is fired synchronously when the opacity of the Node is toggled
@@ -345,12 +344,10 @@
// @private {TinyForwardingProperty.<boolean|null>} - See setPickable() and setPickableProperty()
// NOTE: This is fired synchronously when the pickability of the Node is toggled
- this._pickableProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.pickable, DEFAULT_OPTIONS.pickablePropertyPhetioInstrumented );
- this._pickableProperty.lazyLink( this.onPickablePropertyChange.bind( this ) );
+ this._pickableProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.pickable, DEFAULT_OPTIONS.pickablePropertyPhetioInstrumented, this.onPickablePropertyChange.bind( this ) );
// @public {TinyForwardingProperty.<boolean>} - See setEnabled() and setEnabledProperty()
- this._enabledProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.enabled, DEFAULT_OPTIONS.enabledPropertyPhetioInstrumented );
- this._enabledProperty.lazyLink( this.onEnabledPropertyChange.bind( this ) );
+ this._enabledProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.enabled, DEFAULT_OPTIONS.enabledPropertyPhetioInstrumented, this.onEnabledPropertyChange.bind( this ) );
// @public {TinyProperty.<boolean>} - Whether input event listeners on this Node or descendants on a trail will have
// input listeners. triggered. Note that this does NOT effect picking, and only prevents some listeners from being
@@ -3711,7 +3708,7 @@
// changing visibility can affect pickability pruning, which affects mouse/touch bounds
this._picker.onVisibilityChange();
- if ( assertSlow ) { this._picker.audit(); }
+ // if ( assertSlow ) { this._picker.audit(); }
// Defined in ParallelDOM.js
this._pdomDisplaysInfo.onVisibilityChange( visible );
Index: main/axon/js/TinyEmitter.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/TinyEmitter.js b/main/axon/js/TinyEmitter.js
--- a/main/axon/js/TinyEmitter.js (revision 324fdb40a5a93e93ec3c9800313e23e11c9a744b)
+++ b/main/axon/js/TinyEmitter.js (date 1612980169281)
@@ -13,7 +13,19 @@
const shuffleListeners = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.shuffleListeners;
class TinyEmitter {
- constructor() {
+
+ /**
+ * @param {function()} [onBeforeNotify]
+ */
+ constructor( onBeforeNotify ) {
+
+ if ( onBeforeNotify ) {
+
+ assert && assert(typeof onBeforeNotify==='function','onBeforeNotify should be a function');
+
+ // @private
+ this.onBeforeNotify = onBeforeNotify;
+ }
// @private {Set.<function>} - the listeners that will be called on emit
this.listeners = new Set();
@@ -52,6 +64,8 @@
emit() {
assert && assert( !this.isDisposed, 'should not be called if disposed' );
+ this.onBeforeNotify && this.onBeforeNotify();
+
// Support for a query parameter that shuffles listeners, but bury behind assert so it will be stripped out on build
// so it won't impact production performance.
if ( assert && shuffleListeners ) {
Index: main/axon/js/TinyProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/TinyProperty.js b/main/axon/js/TinyProperty.js
--- a/main/axon/js/TinyProperty.js (revision 324fdb40a5a93e93ec3c9800313e23e11c9a744b)
+++ b/main/axon/js/TinyProperty.js (date 1612980094450)
@@ -17,9 +17,10 @@
/**
* @param {*} value - The initial value of the property
+ * @param {function()} [onBeforeNotify]
*/
- constructor( value ) {
- super();
+ constructor( value, onBeforeNotify ) {
+ super( onBeforeNotify );
// @protected {*} - Store the internal value
this._value = value;
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 324fdb40a5a93e93ec3c9800313e23e11c9a744b)
+++ b/main/axon/js/Property.js (date 1612979751568)
@@ -51,7 +51,11 @@
// Use this to detect or prevent update cycles. Update cycles may be due to floating point error,
// faulty logic, etc. This may be of particular interest for PhET-iO instrumentation, where such
// cycles may pollute the data stream. See https://github.com/phetsims/axon/issues/179
- reentrant: false
+ reentrant: false,
+
+ // {function()|null} - if specified, runs before listeners are notified. Typically used to ensure a consistent state
+ // - or accomplish any work that must be done before any listeners are notified.
+ onBeforeNotify: null
// Property also supports validator options, see ValidatorDef.VALIDATOR_KEYS.
@@ -71,6 +75,9 @@
super( options );
+ // @private
+ this.onBeforeNotify = options.onBeforeNotify;
+
// @public {number} - Unique identifier for this Property.
this.id = globalId++;
@@ -272,6 +279,8 @@
*/
_notifyListeners( oldValue ) {
+ this.onBeforeNotify && this.onBeforeNotify();
+
this.phetioStartEvent( Property.CHANGED_EVENT_NAME, {
getData: () => {
const parameterType = this.phetioType.parameterTypes[ 0 ];
Index: main/axon/js/Emitter.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/Emitter.js b/main/axon/js/Emitter.js
--- a/main/axon/js/Emitter.js (revision 324fdb40a5a93e93ec3c9800313e23e11c9a744b)
+++ b/main/axon/js/Emitter.js (date 1612979554775)
@@ -25,7 +25,8 @@
constructor( options ) {
options = merge( {
- phetioOuterType: Emitter.EmitterIO
+ phetioOuterType: Emitter.EmitterIO,
+ onBeforeNotify: null
}, options );
super( function() {
@@ -38,7 +39,9 @@
const self = this;
// @private - provide Emitter functionality via composition
- this.tinyEmitter = new TinyEmitter();
+ this.tinyEmitter = new TinyEmitter( {
+ onBeforeNotify: options.onBeforeNotify
+ } );
}
/**
Index: main/axon/js/TinyForwardingProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/TinyForwardingProperty.js b/main/axon/js/TinyForwardingProperty.js
--- a/main/axon/js/TinyForwardingProperty.js (revision 324fdb40a5a93e93ec3c9800313e23e11c9a744b)
+++ b/main/axon/js/TinyForwardingProperty.js (date 1612980169306)
@@ -18,10 +18,11 @@
/**
* @param {*} value - The initial value of the property
- * @param {boolean} [targetPropertyInstrumented=false]
+ * @param {boolean} targetPropertyInstrumented
+ * @param {function()} [onBeforeNotify]
*/
- constructor( value, targetPropertyInstrumented = false ) {
- super( value );
+ constructor( value, targetPropertyInstrumented, onBeforeNotify ) {
+ super( value, onBeforeNotify );
/*******************************************************************************************************************
targetProperty - @public (read-only NodeTests) {Property.<*>|null|undefined} - Set in setTargetProperty()
|
2/10/21 Zoom meeting with @samreid @zepumph @jonathanolson @pixelzoom We decided to use the approach of adding an optional callback that is called before listeners are notified. This option will be called |
I committed the addition of onBeforeNotify above. @pixelzoom mentioned that he would review, thanks! Main summary of work:
I looked through the project at all instances of Let me know if you can think of better/more unit tests to make I noticed that unlike how we starting writing it out this morning (with Property implementing its own option and calling it), Property composes a TinyEmitter, so we could just pass that option through to that. @pixelzoom please make sure to take a look at that, especially since it is called Over to @pixelzoom. |
I've had a look around, and I don't see anything amiss. But I have to admit that I'm a little fuzzy on all of the classes involved and their relationships, as well as where For unit tests, it would be nice to have at least one test where |
Sounds good. I'm feeling pretty good about these changes. Closing. |
Over in phetsims/ratio-and-proportion#328, I encountered buggy behavior using
?shuffleListeners
because Node makes an assumption about the first listener that is added to its Property. For example for pickable and visible, those first listeners must go first to ensure functionality. Here is the code in Node:Immediately logic in Node responsible for the behavior of these features (visibility, pickability, etc) is added via a listener, when really it is more foundational than just a listener to a Property value change.
This is the same problem I encountered in phetsims/molarity#189 and also very related to the conversation about why we separated out Action from Emitter (a topic that is still causing discussion--see https://github.com/phetsims/phet-io/issues/1543.).
If we want ?shuffleListeners to be useful at all, then we have to at the very least ensure that these Node listeners go first always. I am sorry to say that I think another developer meeting agenda item is needed.
Possible ways to proceed:
This is related to our topic on Action vs Emitter because originally (pre-Action), each Action instance was an Emitter that immediately had a listener added to it, and that listener was expected to go first. I hope that whatever decision we come to in this issue makes sense and can be applied to the other.
I think I prefer 1 right now, but I'm not confident and don't feel strongly.
Perhaps this issue should move to axon to be more general, but I'll leave it here for now.
The text was updated successfully, but these errors were encountered: