Particles disappear if pumped above lid

[KatieWoe]

Test device
Dell
Operating System
Win 10
Browser
Chrome
Problem description
For ES6
When you pump particles, but the lid is bellow the intake, nothing should happen. Instead, the number of particles in the pump seems to go down. If you reset the sim, those particles stay gone. Middle of gif cut out to save time.

Visuals

Troubleshooting information:

!!!!! DO NOT EDIT !!!!!
Name: States of Matter: Basics
URL: http://phettest.colorado.edu/states-of-matter-basics/states-of-matter-basics_en.html?ea&brand=phet
Version: 1.1.0-dev.0 (require.js)
Features missing: touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.122 Safari/537.36
Language: en-US
Window: 1536x722
Pixel Ratio: 2.5/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 30 uniform: 4096
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

[jbphet]

This problem seems to be the result of the work done to generalize the bicycle pump node in #217. It looks like due to the fact that the pump directly sets the value of the property even when no injection should be allowed, the value of MultiParticleModel.numberOfMoleculesProperty is being increased when it shouldn't be, and that results in the behavior described.

Assigning to @chrisklus, since he did most of the work on the generalization. I'd suggest adding something like an injectionEnabledProperty that can be used to prevent injection when needed.

[chrisklus]

Thanks @KatieWoe and @jbphet. I looked into this a bit and think that the pump's existing enabledProperty option is the best thing to use.

I logged the value of MultiParticleModel.numberOfMoleculesProperty, and am confused by what I'm seeing, but I also don't know/remember much about the model of this sim. As @jbphet said, the value is being increased when it shouldn't be, but something else is preventing those particles from actually affecting the system when the lid is below the hose attachment point. I recreated the bug below but in a completely frozen state to show that both pressure and temperature don't change until the lid is raised and more particles are pumped in, and then they both change (as expected):

Also, MultiParticleModel.numberOfMoleculesProperty is not being reset when the atom/molecule type is not changed and the sim is reset. This is confusing because the visual number of neon atoms returns to 100. Is that because neon is not a molecule, so it uses a different Property? The only part of itself that the bike pump is responsible for resetting is its handle position since it never owns its model Property (which determines the pump's indicator value), so this explains why the the particle indicator in the pump tube is not refilling.

@jbphet here are my recommendations for how we proceed:

1. Figure out why MultiParticleModel.numberOfMoleculesProperty is not resetting for neon, and make it reset (assuming that it should be).

2. Right now the enabledProperty option to the bike pump is being used by model.isPlayingProperty. The behavior of the pump when the sim is paused is the same as what we want when the lid is below the hose attachment point. Let's find the code where the sim knows not to (visually) add particles when the lid is below that point, and then create a derived property that makes the pump disabled when sim is paused or the lid is too low.

I'm happy to pair on this if you're interested!

[jbphet]

For the record, this problem also occurs when the container is exploded.

I think I've implemented a reasonable solution for this. First off, I renamed numberOfMoleculesProperty to targetNumberOfMoleculesProperty to make it more clear what it does. I then added a derived property that tracks whether injection of new particles is allowed, and hooked it to the bike pump. This is a derived property, and if the lid of the container is below the injection point, it becomes false.

One thing that strikes me as a little odd is that the handle of the bike pump can still be moved when the pump is disabled. Most UI components become completely un-interactive when their enabled property goes false. @chrisklus - I'm going to assign this back to you to review the change and consider whether the pump should be un-pump-able when disabled. If you determine that the current behavior is actually what we want, please assign back to me and I think I'll set the pump to be non-pickable when injection is disallowed. Once that's all worked out, we can assign to @KatieWoe to test.

[chrisklus]

Nice! That's exactly the direction I was imagining. I agree with you about the enabled bit - seems like the pump shouldn't be pump-able when it's disabled. I can't remember if that was a conscious decision or not. @arouinfar what do you think? (should the bike pump appear disabled and be un-pickable when it's not enabled?)

[arouinfar]

@chrisklus I think we'll want to prevent interaction with the pump via its enabledProperty (non-pickable & reduced opacity). If we instead toggle its pickableProperty to false, the pump will still look interactive, which could feel a bit buggy.

[jbphet]

Unassigning myself, since I think it falls to @chrisklus to update the pump's behavior when disabled.

[chrisklus]

I got this working but must say it feels pretty strange for the whole pump to appear disabled when the sim is paused. I'm not sure of the best way to proceed. Thoughts?

[arouinfar]

Why are we disabling the pump when paused? I thought the issue was only if the lid was below the hose.

In latest, I can queue up one pump (3 particles) while paused. Is there a reason why we can't queue up more and then let them stream in when unpaused?

[jbphet]

Why are we disabling the pump when paused? I thought the issue was only if the lid was below the hose.

I think it makes sense to disable the pump when paused. It seems better than having users be able to pump the handle and have nothing happen. Perhaps for consistency we should disable the heater/cooler node as well.

I'm personally not too attached to disabling the pump when paused, since it hasn't been a problem before now, but I'd rather we didn't make substantial changes to the paused behavior, which leads me to...

In latest, I can queue up one pump (3 particles) while paused. Is there a reason why we can't queue up more and then let them stream in when unpaused?

This is how it is in the published version. There are three particles injected per full swipe, and they are queued up, and the queue is limited in length to three. That's a semi-arbitrary choice, but we want to avoid situations where the user queues up a ton of particles and then starts changing the size to, say, go below the injection point.

If the visual behavior of having the pump become disabled is too jarring or too much of a change from the previous versions, the code could be easily changed to allow no queuing of particles at all when paused.

[arouinfar]

Thanks for the clarification @jbphet. I don't think it's important to queue up particles in this particular sim, so disabling the BicyclePumpNode when paused seems most appropriate. I would also agree that we should disable the HeaterCoolerNode when paused (we do the same in Gas Props).

[chrisklus]

Thanks @arouinfar and @jbphet.

I would say for the short-term, we should go ahead and just set the enabledProperty of the entire BicyclePumpNode to false when the sim is paused.

That was already in place - linking the enabledProperty to make the pump unpickable and appear disabled is what I didn't commit.

Would it be possible to just disable the handle, since that is the only interactive portion of the BicyclePumpNode? For the HeaterCoolerNode, only the slider appears disabled.

Great suggestion - it's just as easy to do, so I went ahead with that and agree it looks much better. I included the pump shaft too since it looked funny to me when it just the handle appears disabled. What do you think @arouinfar?

[arouinfar]

Great suggestion - it's just as easy to do, so I went ahead with that and agree it looks much better. I included the pump shaft too since it looked funny to me when it just the handle appears disabled. What do you think @arouinfar?

This looks awesome @chrisklus, thanks!

[chrisklus]

Sweet, thanks @arouinfar. Reopening just so I can request @jbphet to please do a quick code review of my change above. Feel free to close if all looks good.

[jbphet]

@chrisklus - changes look good.

I made one more change to the behavior. The pump is now fully disabled when the container lid is pushed below the injection point.

@KatieWoe - please test this on master and, if all looks good, unassign but leave open. This would be a good one to test on the next release.

[KatieWoe]

Looks good in master

[KatieWoe]

Not sure if this is an issue or not, but #256 (comment) is not implemented in phetsims/qa#508

[KatieWoe]

Looks good in phetsims/qa#525. Will reopen if issues come up.