Arm positions not updating after quick object drags

[phet-steele]

@jessegreenberg, very quick object movements have a chance to not update the state of a human's arms on the stack. I think it is easier to show than explain:

1. Go to http://www.colorado.edu/physics/phet/dev/html/forces-and-motion-basics/2.3.0-phetiorc.3/wrappers/state/state.html?sim=forces-and-motion-basics&relativeSimPath
2. On the Motion screen, take a human and place it onto the stack.
3. Now very quickly take another object and "flick" it up onto the stack. You can release the object pretty early within the play area and it will fly to the stack on its own, this is fastest.
4. The human in the lower sim likely does not have their arms lifted to hold the object above them. This works in reverse too, where quickly removing an object doesn't have the human lower their arms:

This video shows the first case and the reverse case (with a few seconds where I "reset" the arm positions to match each other again).

This feels reminiscent of an old build-an-atom issue (...that I can't find, ask JB). Since the state wrapper only updates ~once a second, if there are events like dragging or position changes that fully occur in <1 second, then the State wrapper is completely unaware those events happened (I might be embellishing). It might be that whatever drag event that causes the arms to raise/lower can happen fast enough that the lower sim was never aware of a drag and never updated any arms.

Seen on macOS 10.12.6 Chrome. For phetsims/QA#42.

[jessegreenberg]

Since the state wrapper only updates ~once a second, if there are events like dragging or position changes that fully occur in <1 second, then the State wrapper is completely unaware those events happened (I might be embellishing)

That sounds very possible.

But I also noticed that this ins't happening in the "launched" sim from saved state.

As such, I am wondering if this is worth fixing?

[jessegreenberg]

@samreid @zepumph do you have any inclination about whether this is a problem with the sim or with the state wrapper?

[jessegreenberg]

If I remove the throttle from the state wrapper the problem goes away.

[jessegreenberg]

Because of the refresh rate of the state wrapper, you can see the number of times listeners associated with draggingProperty are called between the two sims.

16 times in the top sim, 4 times in the bottom sim. Since the images update with draggingProperty it is easy to miss an update in the state wrapper.

[samreid]

I remember in some other sim we needed to add a hook that would run after values were set, perhaps we need to figure that out and call ItemNode.updateImage? I need to investigate further.

[samreid]

@jessegreenberg please try this code after the declaration of updateImage in ItemNode.js

    // Make sure the arms are updated (even if nothing else changed)
    phet.phetIo && phet.phetIo.phetio.setStateEmitter && phet.phetIo.phetio.setStateEmitter.addListener( updateImage );

[jessegreenberg]

That seems to fix the problem @samreid! Should this be committed as a final solution? Does that listener have purpose outside the state wrapper or is it used more generally for phetio? Will we start seeing that line in many sims?

[samreid]

Should this be committed as a final solution?

I wish I understood the problem better so that I knew why this workaround is necessary, but listening to the setStateEmitter is a production-ready technique.

Does that listener have purpose outside the state wrapper or is it used more generally for phetio?

It provides an opportunity for the sim to update itself after the state has been set (whether in the state wrapper or another wrapper that customizes the sim).

Will we start seeing that line in many sims?

It seems like a workaround that it would be nice to eliminate, but may be necessary given certain sims.

[jessegreenberg]

My understanding of setState is that when invoked, it iterates over all Properties in the JSON object and sets them. The state wrapper does this every second or so. It seems like the problem is that if a Property is set to a new value and then to its previous value before setState is called, it will look the Property value never changed in the state sim so associated listeners will never be called.

[jessegreenberg]

Thanks for clarifying @samreid, Ill commit the that line.

Would it be possible to notifyListenersStatic at the end of setState so that all listeners are still called when the issue in https://github.com/phetsims/forces-and-motion-basics/issues/240#issuecomment-325409572 happens?

[samreid]

My understanding of setState is that when invoked, it iterates over all Properties in the JSON object and sets them. The state wrapper does this every second or so. It seems like the problem is that if a Property is set to a new value and then to its previous value before setState is called, it will look the Property value never changed in the state sim so associated listeners will never be called.

Let's say the value changed from 3 to 4 to 3 in the source sim. And the destination sim only gets the memo about going from 3 to 3, so it doesn't callback any listeners. Why would it need to call back listeners, since the value has not changed?

Would it be possible to notifyListenersStatic at the end of setState so that all listeners are still called when the issue in https://github.com/phetsims/forces-and-motion-basics/issues/240#issuecomment-325409572 happens?

Are you recommending to notifyListenersStatic for every Property after state is set? It's still unclear to me why this may be necessary.

[jessegreenberg]

Wait a minute, this line should also be updating the image whenever the stack size changes, which should not be dependent on the refresh rate of the state wrapper.

    model.stack.lengthProperty.link( updateImage );

There is no lengthProperty in the state data, maybe it isn't instrumented.

[jessegreenberg]

Instrumented the Property in ObservableArray, but it doesn't help. It looks like item.armsUp() is still returning false in the state wrapper.

[jessegreenberg]

Let's say the value changed from 3 to 4 to 3 in the source sim. And the destination sim only gets the memo about going from 3 to 3, so it doesn't callback any listeners. Why would it need to call back listeners, since the value has not changed?

My thought was that if a Property's listener changes some aspect of the simulation that is not a Property, the state wrapper could get out of sync. For instance:

var dragCount = 0;
draggedProperty.link( function( dragged ) {
  if ( dragged ) {
    dragCount++;
  }
} );

otherProperty.link( function( other ) {
  // do something with drag count, but there is no guarantee that the value
  // of dragCount will be the same in both sim and downstream sim.
  myFunction( dragCount );
} );

This can be fixed by making dragCount a Property so state is always up to date or by the recommendation in https://github.com/phetsims/forces-and-motion-basics/issues/240#issuecomment-325041914, but I was wondering if triggering listeners statically was generally necessary if this happens often.

[jessegreenberg]

But it looks like the problem in this case is more likely related to https://github.com/phetsims/axon/issues/135 and the fact that in the state sim the observable array lengthProperty is not always equal to its _array.length.

[jessegreenberg]

In MotionModel's isItemStackedAbove, changing

this.stack.indexOf( item ) < this.stack.length - 1;

to

this.stack.indexOf( item ) < this.stack.lengthProperty.get() - 1;

also fixes the problem, see phetsims/axon#135.

[jessegreenberg]

I commited the recommendation in https://github.com/phetsims/forces-and-motion-basics/issues/240#issuecomment-325041914, but further work for this issue is likely on hold until https://github.com/phetsims/axon/issues/135 is resolved.

[ariel-phet]

@phet-steele @jessegreenberg @samreid I get that you want to understand this issue in that it may have other consequences for iO in general....

However, in terms of whether the arms are up or not, this is just not a very important detail pedagogically, so I would suggest no more time for fixing this specific issue.

[jessegreenberg]

https://github.com/phetsims/axon/issues/135 was resolved, and the fix also fixed this issue. When I remove the line

    phet.phetIo && phet.phetIo.phetio.setStateEmitter && phet.phetIo.phetio.setStateEmitter.addListener( updateImage );

the problem is still fixed.

[jessegreenberg]

2.3 and 2.3-phetio are at axon sha 62935a11739c6b82860668019e37fa3e3d5f77ce. To incorporate this fix, they need axon changes through 020d59a7cf4587040f2ca181f652466d0234022c.

[jessegreenberg]

I think there are too many changes in between these SHAs to just update axon SHA, we will need to create a maintenance branch in sun for this. forces-and-motion-basics-2.3 and forces-and-motion-basics-2.3-phetio.

[jessegreenberg]

Branches created, patches applied and dependencies.json updated for both 2.3 and 2.3-phetio branches.

[phet-steele]

Good work, fixed.