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

Bottom screen in state wrapper picks up thermometers #388

Closed
KatieWoe opened this issue Oct 5, 2020 · 9 comments
Closed

Bottom screen in state wrapper picks up thermometers #388

KatieWoe opened this issue Oct 5, 2020 · 9 comments

Comments

@KatieWoe
Copy link
Contributor

KatieWoe commented Oct 5, 2020

For phetsims/qa#560. Occurs in previous RCs. (Sorry)
It is possible to drag an object over a thermometer on the first screen and have it behave properly in the top screen (thermometer left behind) and improperly on the bottom screen (thermometer picked up and carried with object). This can happen even if thermometers are in their box. Can be done with a mouse, does not require touch.
Seen on Mac 10.15 Chrome
snagthermometers
Edit: Reset all does not solve this. Dragging the thermometer in the top screen moves them in front of the object that picked them up, but does not lent you move the thermometers.

@kathy-phet
Copy link

This seems odd to find in the state wrapper. It's pretty obscure, but I'm also surprised that it happens at all.

@jbphet
Copy link
Contributor

jbphet commented Oct 6, 2020

I've examined the code and set some breakpoints, and I can see why this is happening. Code was added to StickyTemperatureAndColorSensor to make it stick to any object that it is over when state setting completes. That's because the actual state of ElementFollower instance, which is the thing that makes the temperature sensor 'stick' to an object, isn't actually being included in the state.

The code in question looks like this:

    // Check if any sensors should start following an element after being set by state
    Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => {
      followElements();
    } );

@jbphet
Copy link
Contributor

jbphet commented Oct 6, 2020

This code causes or is related to another problem, one in which once a thermometer is stuck to a model element in the downstream sim, it can't get un-stuck. Here's a screen capture, observe how the thermometer in the lower frame stays on the block once it's there, even though the color indicator associated with it changes.

efac-sticky-thermometerors

@jbphet
Copy link
Contributor

jbphet commented Oct 6, 2020

Here is another problematic sequence of events - notice how the thermometer doesn't stick in the top frame but does in the bottom:

efac-sticky-2

jbphet added a commit that referenced this issue Oct 6, 2020
@jbphet
Copy link
Contributor

jbphet commented Oct 6, 2020

The problem here, in a nutshell, is that the code that implements the "sticking" of the thermometers to the blocks and beakers has not been instrumented, so its state is not always being correctly set in the downstream sim, which leads to some incorrect "sticking" behavior. I've added code to turn off the element follower (i.e. the thing that makes the thermometer stick to - aka follow - a block or beaker) at the beginning of state setting process and then decide whether it needs to be turned back on at the end of state setting. This appears to work for all of the cases described above.

A better solution would be to convert ElementFollower to a form that works better for instrumenting and then instrument it so that its state is included in the phet-io state information. This is more complicated, which is why I didn't do it. I discussed this with @zepumph and he's up for trying that conversion on master. I'll create an issue for this and assign it to him.

@KatieWoe - for now, please test this on master an let me know if it's looking good. That will give us all confidence that it will be fixed in the next RC.

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Oct 7, 2020

This issue looks fixed, but energy chunks being added was buggy when they were being added to an object on master. I don't know if/how this may be related. Haven't seen in the RC so far.
energychunktwitch
Screen Shot 2020-10-07 at 9 12 40 AM

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Oct 7, 2020

Ok, it does look ok on master now. Not sure what caused earlier. I must not have had a good copy of master.

@jbphet
Copy link
Contributor

jbphet commented Oct 7, 2020

It may be that some of the work done in #391 improved things, since it was all about energy chunks that were on their way to something but not there yet.

It's good to hear that it's working now. I'll mark it as fixed and will have it tested in the next RC.

@phet-steele
Copy link

Looks fixed to me. 👍

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

4 participants