Stack changes when copy sim is launched in Instance Proxies and State

[powderh0und]

This occurs in the Motion, Friction, and Acceleration screens. The object can either be stopped, pushed or freely moving. Works on all objects except default box.

For Instance Proxies:

1. Go to Motion, Friction, or Acceleration screens
2. Add any object to stack or swap with default box
3. Click Launch to start a copied sim
4. Object is now floating in air and stack has force properties of default box

For State: Follow same steps as Instance Proxies

Additionally, clicking the floating object adds it and an invisible box to the stack. Clicking again adds default box to the stack.

Original sim:

Newly opened copy:

Clicking object once:

Clicking object twice:

New objects can be added to stack:

Tested this on Windows 10 with Chrome, Firefox, and Edge and on Mac OS 10.12 with Safari For phetsims/QA/issues/12

[zepumph]

This touches phet-io, I'll take it over.

[jessegreenberg]

Thanks @zepumph!

[zepumph]

This seems to be a very old problem, not from a recent change, I was able to recreate it on FAMB 2.2.6-phetio. https://phet-io.colorado.edu/sims/forces-and-motion-basics/2.2.6-phetio/wrappers/instance-proxies/instance-proxies.html?sim=forces-and-motion-basics

I found these pictures very similar to the ones above, but with less clutter and a bit simpler for me to convey and understand the problem.

A nice way to demonstrate this problem from what I found takes a few steps:

1. On the acceleration screen, remove the default box, add the other box, and then stack the man on top of it.

2. Press launch and witness the problem on startup:

3. Drag the central box just a little, and it will snap to a space that is above the ground.

4. Click the box again and see that the man has been left behind (literally, ha hahaha) the box, and the first starting box has jumped up to the base of the stack.

[zepumph]

My best guess to what is happening is that there is a property or setting that is not instrumented, and so it is not being conveyed to the downstream sim. Something like a "dropped" or "selected into the stack" property. @jessegreenberg, FAMB is pretty large, could you comment on a good place for me to start to look into this?

[jessegreenberg]

@zepumph there is a dragging Property in Item.js, but it looks instrumented to me. However, in the same type there is an 'animating' property in the list of properties that looks suspicious. Any chance it could be related to this?

      //TODO: to instrument `animating` it would needs its own type, TObject does not know how to serialize it.
      animating: {
        value: {
          enabled: false,
          x: 0,
          y: 0,
          end: null,
          destination: 'home'
        }
      },

[zepumph]

I'm looking into that one currently! I wonder if successful instrumentation of that won't just solve our problem.

[zepumph]

I'm glad we agree to a starting point.

[zepumph]

Instrumenting the 'animated' property helped to fix the look of the sim when it first launches, but it still isn't correct, the first crate will still bounce back up to the base of the stack, like some sort of edge case based on the initial configuration.

[zepumph]

I think I found the culprit. In MotionModel see the viewInitialized method:

    viewInitialized: function( view ) {
      var item = this.items[ 1 ];
      // only move item to the top of the stack if it is not being dragged
      if ( !item.dragging ) {
        this.view = view;
        item.onBoard = true;
        var itemNode = view.itemNodes[ 1 ];
        item.animating = { enabled: false, x: 0, y: 0, end: null };
        item.interactionScale = 1.3;
        var scaledWidth = this.view.getSize( item ).width;
        item.position = new Vector2( view.layoutBounds.width / 2 - scaledWidth / 2, view.topOfStack - itemNode.height );
        this.stack.add( item );
      }
    },

It is manipulating the starting block pretty directly, and part of me thinks that this is overriding the state coming from the upstream simulation (in the state wrapper) or the launch customizations (in instance proxies).

Then I decided to get the state out of the sim twice, once from the upstream sim (in instance proxies) just before launching, and the other, after fully loading the launched sim with customizations, and the states are almost identical. There seems to be nothing that stands out to make me think just yet, that I am on the right track. Here is the complete diff, nothing too exciting:

Michael /zepumph/Programming/PHET/temp1
$ diff upstream.json  downstream.json
63c63,66
<   "forcesAndMotionBasics.netForceScreen.model.smallBluePuller1.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.smallBluePuller1.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
72c75,78
<   "forcesAndMotionBasics.netForceScreen.model.smallBluePuller2.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.smallBluePuller2.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
81c87,90
<   "forcesAndMotionBasics.netForceScreen.model.mediumBluePuller.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.mediumBluePuller.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
90c99,102
<   "forcesAndMotionBasics.netForceScreen.model.largeBluePuller.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.largeBluePuller.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
99c111,114
<   "forcesAndMotionBasics.netForceScreen.model.smallRedPuller1.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.smallRedPuller1.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
108c123,126
<   "forcesAndMotionBasics.netForceScreen.model.smallRedPuller2.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.smallRedPuller2.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
117c135,138
<   "forcesAndMotionBasics.netForceScreen.model.mediumRedPuller.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.mediumRedPuller.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
126c147,150
<   "forcesAndMotionBasics.netForceScreen.model.largeRedPuller.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.largeRedPuller.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },
499c523

{{NOTE}} These tandems below are only instrumented on my local machine right now.
<   "forcesAndMotionBasics.accelerationScreen.model.timeSinceFallenProperty": 42.67699999999984,
---
>   "forcesAndMotionBasics.accelerationScreen.model.timeSinceFallenProperty": 84.625000000001,
502c526
<   "forcesAndMotionBasics.accelerationScreen.model.timeProperty": 32.67699999999924,
---
>   "forcesAndMotionBasics.accelerationScreen.model.timeProperty": 74.62500000000098,

I would have expected to see something like crate1.onBoardProperty: true in the downstream sim, like it was being overridden. I'll keep poking. @jessegreenberg it could be nice to have you and I pair on this sometime next week.

[zepumph]

In the above commits I instrumented a few things that I'm unsure about. I know that instrumenting the 'animating' property helped, but I'm not so sure I did it in the best way. All I wanted was for the object to be passed across the frame, which I would have assumed was TObject's job, to pass an object verbatim across the frame. Instead it's toStateObject looks like this:

    toStateObject: function( o ) {
      return o === null ? 'null' :
             o === undefined ? 'undefined' :
             o.phetioID;
    },

This seems strange to me. wouldn't we want to whole object to be passed along. So I had to create a new TType that just looked like so:

  phetioInherit( TObject, 'TAnimatingData', TAnimatingData, {}, {
    documentation: 'Data that is stored in the "Item.animating" Property. Type to serialize the data object across the iframe',

    toStateObject: function( instance ) {
      return instance;
    },

    fromStateObject: function( stateObject ) {
      return stateObject;
    }
  } );

ps, I don't like the name, but I didn't know what else to do.

I'm also unsure about instrumenting the observable array. I don't think it is adding any value in terms of client side customization, but it may be helpful for states. I want to talk through these problems with @samreid.

[jessegreenberg]

@zepumph any time. I know that there are some state properties in this sim that are not observable axon Properties, maybe they aren't getting updated correctly in the wrapper case.

[zepumph]

@jessegreenberg and I looked into this, it turns out the problem was that TObservableArray was not instrumented for state's. I found a solution to this problem. It involved saving references to the elements so that they could be reassigned in the downstream sim. This solution isn't perfect, and probably doesn't work on all cases, but I applied a workaround in phetio.js until we figure it all out. I added a new property just to TObservableArray and use it like so in setStateForInstance to short circuit setting the state for other observable arrays:

      // Workaround specific to ObservableArray to work out setState bugs in FAMB, see https://github.com/phetsims/forces-and-motion-basics/issues/232
      if ( type.typeName === 'TObservableArray' && !wrapper.instance.phetioIncludeInState ) {
        return true;
      }

This workaround works successfully in other state wrappers with ObservableArrays like charges and fields and build and atom.

I'm going to close this issue, and carry on the observable array discussion in phetsims/phet-io#1054. I would love @samreid's advice on how to proceed from here in that issue. I'll make a comment over there. The bug is fixed though! Thanks for the help @jessegreenberg.

[zepumph]

on second thought. It may want to be confirmed fixed on the next round of testing. Unassigning.

[zepumph]

There is a problem here because of https://github.com/phetsims/phet-io/issues/1027. The TAnimatingData needs it's own type. I want to talk to @samreid about trying to make this just be TObject.

[zepumph]

Bumping priority because it is blocking the publication of the new version of FAMB. @samreid I would love to talk through this one with you soon.

[zepumph]

When I look at the state taken from the upstream sim, and compare it to a 'getState' call from the downstream "launched" sim after it has been set. They are basically the same thing. The only different looks like this in 'lastLocationProperty`

"forcesAndMotionBasics.netForceScreen.model.largeBluePuller.lastLocationProperty": {},
---
>   "forcesAndMotionBasics.netForceScreen.model.largeBluePuller.lastLocationProperty": {
>     "x": 0,
>     "y": 0
>   },

[zepumph]

The main problem I am trying to solve is that after a state has been set where there is no crate1 in the stack, the crate1 still seems to be in the stack, sortof (see https://github.com/phetsims/forces-and-motion-basics/issues/232#issuecomment-318772171 bullet 3 and 4). I think that we still aren't fully setting the state of observable arrays correctly.

[zepumph]

I found the bug. I wasn't adding the phetioIncludeInState property to ObservableArray, so it was just in the options.

The bug is fixed now, but I'm not sure we have the best solution. @samreid could you please look at the above commits, and mainly see the static stateObject methods in ObservableArray and comment on a few things.

1. Do you think I have fully solved this error for this bug?
2. Do you think that we are good to cherry pick these commits into the FAMB rc that brought this bug to our attention?
3. Please let me know what you think is to be the best long term solution for states and Observable array (this may belong in https://github.com/phetsims/phet-io/issues/1054)
4. Comment on my use of TAnimatingData, and notice the todo at the top of the type (paraphrase: why can't this be a TObject?)

(@samreid I swear I was going to assign you right now, but you beat me to it)

[samreid]

Summarizing what I gleaned from looking at ObservableArray and TObservable, to see if I have it right:

a. ObservableArray has a new option that it can use to opt-in to saving data on state save b. If that flag is set, then TObservableArray uses phetioIDs to save and restore state--this requires fixed (not dynamic) instances.

If this is correct, I think it will work for some sims but not others and is not fully general. The fully general case may need to deal with the dynamic state save/load or some custom code for save/load. But this may work ok for some sims (I recall TObservableArray has gone back and forth about what support it provides for save/load--often fixing it for one sim broke it for another sim, perhaps having this be "opt in" will help).

Do you think I have fully solved this error for this bug?

I tried adding options.phetioIncludeInState=false; to ObservableArray after the extend call to try to reproduce the bug, but after stacking a person on the box and pressing launch, I'm not seeing the bug. Is that flag respected?

Do you think that we are good to cherry pick these commits into the FAMB rc that brought this bug to our attention?

It depends how old the SHAs are. If they cherry pick without conflicts, it may be OK.

Please let me know what you think is to be the best long term solution for states and Observable array (this may belong in phetsims/phet-io#1054)

I don't know, my best offer is from my paragraph above about perhaps using dynamic state save/load or custom code.

Comment on my use of TAnimatingData, and notice the todo at the top of the type (paraphrase: why can't this be a TObject?)

Here are the usages of animating:

Perhaps we should create a type called AnimatingData?

[samreid]

Also, the property this.animation is poorly named. Perhaps it should be this.animationState and have type AnimationState?

[samreid]

Aha, I found a hack that covers up my hack!

      // Workaround specific to ObservableArray to work out setState bugs in FAMB, see https://github.com/phetsims/forces-and-motion-basics/issues/232
      if ( type.typeName === 'TObservableArray' && !wrapper.instance.phetioIncludeInState ) {
        return true;
      }

UPDATE: Even if I comment that out, things still seem to be working. I must be doing something wrong.

[samreid]

Also, regarding whether we can use cherry-picks for this, please be aware in https://github.com/phetsims/forces-and-motion-basics/issues/235 @kathy-phet requested this sim be ready for Legends of Learning. I do not know when the SHAs are from or if this sim suffers from the multitouch problems we found in build an atom. It would be good to get that worked out before next publication.

[zepumph]

I tried adding options.phetioIncludeInState=false; to ObservableArray after the extend call to try to reproduce the bug, but after stacking a person on the box and pressing launch, I'm not seeing the bug. Is that flag respected?

Instead of adding the above line of code, just change the call site in MotionModel. I am definitely seeing the bug, here are the steps to reproduce.

1. swap crates so crate2 is on the stack alone
2. add a person on top
3. launch
4. click on the crate that is at the base of the stack. (It should weirdly float up to lay in front of (z layer) the man)

See https://github.com/phetsims/forces-and-motion-basics/issues/232#issuecomment-318772171 as reference to recreate the bug.

I'm happy to communicate live sometime about this as well. It's a weird bug.

---

I agree that the animating property is named very poorly, but I still feel like the TProperty should have a phetio value type of TObject, because that's all that the property is. I don't really want to have to make a TAnimatingState for this individual case, and then TSomeOtherObject for the next case.

---

I'm unsure about how to go about deciding to cherry-pick or grab master. I defer to someone that know more about the situation. (priority of "any release" of FAMB vs priority of LOL integration). When we get everything sorted out we can ask a higher up.

[zepumph]

In the above commits. . .

I renamed animating Property to animationState.

I also got rid of TAnimatingData in exchange for TObject. I had to make a small change to TObject so that it can support wrapping json objects out of the box.

This is a change that seems to effect many places, but I looks and saw that in all of our phet-io sims, TObject.to/fromStateObject() is never called because it is always overridden by sub types. animationStateProperty is now the only type that is wrapped by TObject. I tested this with our iframe api tests. There were no TObjects that ever had toStateObject called on them.

@samreid will you take a look?

[samreid]

I'm unsure about how to go about deciding to cherry-pick or grab master. I defer to someone that know more about the situation.

In https://github.com/phetsims/forces-and-motion-basics/issues/235#issuecomment-322817011 @ariel-phet recommended to go to master.

[zepumph]

After speaking with @samreid today, we decided to clean this up for ObservableArray to support references, and then proceed with the new FAMB release. As for the 'general case', we want to pursue supporting references and values across the frame, but that is a bit down the road.

This issue is ready to be closed. @jessegreenberg please let me know if there is more I can do with the new FAMB releases before I leave.