Skip to content
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

Pusher position not being reset in bottom sim #243

Open
phet-steele opened this issue Aug 24, 2017 · 9 comments
Open

Pusher position not being reset in bottom sim #243

phet-steele opened this issue Aug 24, 2017 · 9 comments

Comments

@phet-steele
Copy link

  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
Copy link
Contributor

Seems possible this is related to #241.

@jessegreenberg
Copy link
Contributor

Happening because this emitter is not instrumented:

this.resetAllEmitter = new Emitter();

@jessegreenberg
Copy link
Contributor

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
Copy link
Member

samreid commented Sep 11, 2017

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
Copy link
Member

samreid commented Sep 11, 2017

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

@zepumph zepumph changed the title [State] Pusher position not being reset in bottom sim Pusher position not being reset in bottom sim Sep 11, 2017
@jessegreenberg
Copy link
Contributor

Thanks @samreid, @ariel-phet could you please review this issue (particularly #243 (comment)) and let us know if this issue is non-aesthetic?

@samreid samreid removed their assignment Sep 12, 2017
@ariel-phet
Copy link

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

@ariel-phet ariel-phet removed their assignment Sep 12, 2017
@zepumph zepumph removed their assignment Sep 12, 2017
@zepumph
Copy link
Member

zepumph commented Sep 12, 2017

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

@jessegreenberg
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants