Assertion failed: cannot start event while event is in progress

[pixelzoom]

Related to #38.

In console: colorized phet-io wrapper, there's a cycle happening when the robotic arm is dragged. This results in "Assertion failed: cannot start event while event is in progress".

Here's the relevant code in SingleSpringSystem, and there's a similar problem in SeriesSystem and ParallelSystem.

    // Connect arm to spring.
    this.spring.rightProperty.link( function( right ) {
       self.roboticArm.leftProperty.set( right );
    } );

    // Robotic arm sets displacement of spring.
    this.roboticArm.leftProperty.link( function( left ) {
      self.spring.displacementProperty.set( left - self.spring.equilibriumXProperty.get() );
    } );

[pixelzoom]

I don't understand this problem. And I don't understand how a "cannot start event while event is in progress" policy can work with multi-touch.

[samreid]

Here is the simplified stack trace:

RoboticArmNode.drag(...);
roboticArm.leftProperty.set( left );
self.spring.displacementProperty.set( left - self.spring.equilibriumXProperty.get() );
self.appliedForceProperty.set( self.appliedForceRange.constrainValue( appliedForce ) );
self.displacementProperty.set( appliedForce / self.springConstantProperty.get() ); // x = F/k

Note the loop in displacementProperty. I think that a Property cascading changes to itself should generally be avoided. I'm not familiar enough with the sim to know how to address this.

[samreid]

It looks like it could be caused by rounding in the constrainValue step.

setting spring displacement from robotic arm: 0.17399999999999993
setting displacement to f/k = 0.175

[pixelzoom]

I'm not familiar enough with the sim to know how to address this.

I'll investigate first thing on Tuesday 6/12.

[samreid]

I'd also like to investigate allowing the same Property instance loops in PhetioObject. I'll create a tandem issue for this.

[samreid]

I added support for Property link cycles in phetsims/tandem#57, so this is no longer a blocking issue. However, it would still be good to identify what is causing the Property link loop and if it should be eliminated.

[pixelzoom]

After going round and round, and using this patch in Property to identify the loops, I discovered that there's an inherent problem in NumberControl's arrow buttons. Here's the code and an example with values when numberProperty.get() is 0.008 and delta is 0.001:

186 var value = numberProperty.get() + delta; // -> 0.009000000000000001
187 value = Util.roundSymmetric( value / delta ) * delta; // -> 0.009000000000000001

So this does nothing to remove the floating point error. And it's the same approach that I've used in Hooke's Law (and other sims?) to remove floating point error.

To reproduce:

1. Go to the Energy screen
2. Press the right arrow on the Displacement control to get to 0.009.
3. Boom

[pixelzoom]

I believe that I've eliminated Property loops in the Energy screen. See the above commit and the fix to NumberControl in phetsims/scenery-phet#384. This literally took me all day to work out, and there are still similar loops (related to Applied Force, I believe) in the other 2 screens.

@samreid FYI.

[pixelzoom]

The other 2 screens (Intro, Systems) will need to be addressed in order to complete the API requirements in #58.

We'll also need to sort out the confusion about "number of decimals" in #57.

[pixelzoom]

I tried 2 things in the above commits, and published in 1.1.0-dev.13:

(1) Update displacement and applied force only if their new value exceeds the old value by 1E-10. This should prevent update cycles due to floating point error. ThresholdProperty (EDIT: renamed EpsilonProperty) generalizes the implementation, and is used only for the Properties that are the cause of cycles. I'd like to discuss this strategy with another developer, perhaps @samreid or @jonathanolson. Are there potential problems? Might this be generally useful?

(2) Move the definition of DerivedProperties after registering any listeners that may update their dependencies. For example, potential energy is derived from k and x. But (in the first 2 screens) changing k results in a change to x. If potential energy is updated before x is recomputed, then it will be updated twice, with the first update being an intermediate value that use the new value of k and the stale value of x. This works only because Property listeners are notified in the order that they are added. I don't like having these type of ordering dependencies in code, but I don't see another way to address this.

[pixelzoom]

@arouinfar Could you please try 1.0.0-dev.13 as see if you notices anything wrong? I'm trying a new approach to mitigate the "update cycles" caused by floating point error in the model.

[samreid]

I don't like having these type of ordering dependencies in code, but I don't see another way to address this.

One way to avoid ordering dependencies and eliminate loops is to use lower case properties and Emitters, like so:

this.springConstant = ...;
this.force = ...;
this.displacement = ...;

setDisplacementWithoutChangingSpringConstant:function(displacement){
   this.displacement=displacement;
   this.force = displacement * this.springConstant;
   this.displacementChangedEmitter.emit();
   this.forceChangedEmitter.emit();
}

setForceWithoutChangingDisplacement:function(force){
   this.force = force;
   this.springConstant = force/this.displacement;
   this.springConstantChangedEmitter.emit();
   this.forceChangedEmitter.emit();
}

Many of our user interface components (like HSlider) require a Property interface, so you would need to build adapters for those.

[samreid]

We have seen this problem in other circumstances so I'm brainstorming ways to address it. What if Property had a "withholdNotifications" state so you could set the values of multiple Properties at once, then resume notifications after all values are set?

this.springConstantProperty.setWithholdNotifications(true);
this.forceProperty.setWithholdNotifications(true);
this.springConstantProperty = ...;
this.forceProperty = ...;
this.springConstantProperty.setWithholdNotifications(false);
this.forceProperty.setWithholdNotifications(false);

Or what if there was a function that set the values of multiple Properties at the same time, only sending out notifications after all internal values were set? The implementation would (a) check which values are different then (b) set private internal values for the new values then (c) send notifications for the changed Properties.

Property.setValues(
  springConstantProperty, k,
  forceProperty, f
)

[arouinfar]

@pixelzoom things are looking good to me. I also tested with ?log and the messages seemed appropriate (and non-repetitive), though I'm not a PhET-iO expert.

[pixelzoom]

@samreid wrote:

One way to avoid ordering dependencies and eliminate loops is to use lower case properties and Emitters, like so: ...

More complicated client code, and no leveraging of Property.

What if Property had a "withholdNotifications" state so you could set the values of multiple Properties at once, then resume notifications after all values are set? ...

Better than the "properties and Emitters" idea. More complicated client code, but it could be used to address the "intermediate state" problem encountered with DerivedProperty.

[pixelzoom]

Here's where I'm at with this...

I've been able to reduce, but not eliminate update cycles that are due to floating-point error.

I've used EpsilonProperty to prevent some Properties (appliedForceProperty and distanceProperty) from being set unless the difference between their current value and new value is greater than some small epsilon (1E-10). This terminated many of the update cycles. That said, I'm a tad uneasy about using EpsilonProperty since I think it could result in other more subtle problems, and I don't think it's a good general way to address this problem.

In some cases, floating-point error results in a quantity's range being slightly exceeded. In these cases, the value is clamped to the range and an update cycle is unpreventable.

In Slack discussions, it was suggested that it might be preferable to "do nothing" in the sim code, and address this issue by filtering messages. I don’t have a vision of how that filtering would be accomplished, either by us or the client. And I’m not sure how we can identify which messages are undesirable/spurious by looking at the message stream.

I'm going to defer further work on this issue until there's a consensus and plan for how to address this problem in the general case.

[pixelzoom]

Above I said:

I’m not sure how we can identify which messages are undesirable/spurious by looking at the message stream.

Perhaps this could be accomplished by using the nested nature of the message stream. If a message related to some Property contains additional messages related to that Property, then ignore them (and other related messages?) This would take some significant investigation to characterize the various situations where messages should be filtered out.

[pixelzoom]

Moving discussion of the general issue to https://github.com/phetsims/phet-io/issues/1349. This issue is blocked until that issue is resolved.

[pixelzoom]

Note to self: Before closing this issue, I should:

• Include something in implementation-notes.md about this issue, why/when reentrant: true is used, why/when EpsilonProperty is used, concerns, etc.

[pixelzoom]

I'm leaning toward removing EpsilonProperty. It's a questionable way of addressing the issue, puts the sim in a (technically) inconsistent state, and could cause other problems. In the meantime, I've added an epsilon query parameter, see above commit.

[samreid]

Removing EpsilonProperty sounds reasonable to me. If we need to alert our clients that the data stream sometimes cycles or has numerical roundoff issues, that would be acceptable.

[pixelzoom]

EpsilonProperty and epsilon query parameter were removed in the above commit.

[pixelzoom]

So.... "reentrant" was added as a Property option, and PhET-iO no longer fails when reentry occurs. And Hooke's Law is basically back to where we started -- it still has 3 Properties that require reentrant: true.

Reentry has an impact on the PhET-iO data stream, and in https://github.com/phetsims/phet-io/issues/1349 we're discussing what to do about that. If there's a need to do anything for Hooke's Law, we'll create a new issue.

Closing.