phetsims / forces-and-motion-basics

"Forces and Motion: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/forces-and-motion-basics
GNU General Public License v3.0
7 stars 10 forks source link

Pusher position not being reset in bottom sim #243

Open phet-steele opened 7 years ago

phet-steele commented 7 years ago
  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. Go to the Motion screen
  3. Get the pusher going a ways until it falls and goes off screen
  4. Reset the sim
  5. The pusher does not return in the bottom sim.

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

jessegreenberg commented 7 years ago

Seems possible this is related to #241.

jessegreenberg commented 7 years ago

Happening because this emitter is not instrumented:

this.resetAllEmitter = new Emitter();
jessegreenberg commented 7 years ago

Instrumenting this emitter doesn't help. That makes sense, Emitter's aren't meant to convey state.

The problem is that initializePusherNode() (called on construction and on reset) doesn't modify any sim Properties, it just gets the current Property values and sets image centerX accordingly. This function is never called in the state wrapper because it is triggered by resetAllEmitter, not by sim state.

Maybe the implementation in PusherNode could be changed so that its view position could be more directly linked to model.pusherPositionProperty and model.positionProperty but that would take some time. @samreid and @zepumph do you know of other ways to approach this problem first?

samreid commented 7 years ago

I tried a few things and didn't see any "quick fixes". I think the implementation and usage of getPusherNodeDeltaXand updateZeroForcePosition are confusing. The simulation should be rewritten so that the pusher can be positioned based on the pusherPositionProperty and the positionProperty instead of trying to track deltas.

samreid commented 7 years ago

By the way, the position of the pusher in the saved state may be classified as "aesthetic" and may not be a blocking feature for publication. A client wanting to use the save state for this could work around this by setting the state before using the pusher.

So, to clarify on my recommendation:

  1. This can be skipped for publication (unless @ariel-phet deems this as non-aesthetic)
  2. Should be fixed for long-term maintainability
jessegreenberg commented 7 years ago

Thanks @samreid, @ariel-phet could you please review this issue (particularly https://github.com/phetsims/forces-and-motion-basics/issues/243#issuecomment-328671834) and let us know if this issue is non-aesthetic?

ariel-phet commented 7 years ago

@jessegreenberg I would classify this issue as aesthetic, especially since reset is necessary to cause the misbehavior.

zepumph commented 7 years ago

In that case I will be unassigning myself. Let me know if I can be of further assistance.

jessegreenberg commented 7 years ago

Thanks @ariel-phet. Also removing assignment, and marking as deferred since we will address this at another time.