phetsims / states-of-matter

"States of Matter" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
7 stars 8 forks source link

migrate BicyclePumpNode to scenery-phet #217

Closed pixelzoom closed 4 years ago

pixelzoom commented 6 years ago

The Gas Properties design document description of the bicycle pump UI component (see screenshot below) points to "similar in States of Matter". I see STATES_OF_MATTER/BicyclePumpNode, but it's coupled to that sim. So we'll need to allocate a chunk of time to generalize it and move it to scenery-phet, so that I can use it in Gas Properties.

Assigning to me, @jbphet and @ariel-phet to discuss.

screenshot_702

jbphet commented 5 years ago

Does SOM require adding particles one at a time?

The main reasons that the particles are not added all at once are:

Based on the above, I feel strongly that it should continue to work the way it has in the past, i.e. particles should be injected sequentially, not in bursts.

pixelzoom commented 5 years ago

Somes like all needs are met with option addParticlesOneAtATime.

pixelzoom commented 5 years ago

@chrisklus The stuck particle appears to have been a Gas Properties problem. Details in https://github.com/phetsims/gas-properties/issues/62#issuecomment-491059437. But in summary, it was possible to create a particle with zero initial speed, because the initial temperature was incorrect.

ariel-phet commented 5 years ago

In regards to https://github.com/phetsims/states-of-matter/issues/217#issuecomment-488186493 if there is an option for height, it should allow us to scale the height, not just the overall size of the pump. In future sims I could see such an option being useful.

However, I am guessing it will be a nontrivial amount of rework this option. I will leave it to @chrisklus and @pixelzoom to decide if they want to add this option now, or (since I don't think either current sim needs such an option), perhaps make a separate issue for this work to be done at some point in the future, or punt until it is desired for a particular sim.

chrisklus commented 5 years ago

@pixelzoom after a bit of confusion and thought, your changes from https://github.com/phetsims/scenery-phet/commit/872860a38ac811394eeae94a8ea107dd4adb896e make sense and look good. I guess I didn't understand that literally the only issue you were trying to change was the uniform-looking wave of particles that appeared when BPN was quickly pumped. My understanding was that you wanted the particles to be batched per pumping stroke, not per drag event. Batching per drag event is a very subtle change from the original implementation. My misinterpretation of numberAddedOnThisDrag in https://github.com/phetsims/states-of-matter/issues/217#issuecomment-489338309 must have been how I got so far off track :)

Regarding https://github.com/phetsims/states-of-matter/issues/217#issuecomment-492445608 - I agree that it will likely need to be done one day, but I'm thinking it could be a while before another bike pump is needed in a sim and it needs to be scaled differently. So I think it's pretty low priority.

My one reservation for pushing it off is that there is some scaling functionality already built into the code - it's just bad (the whole pump stretches instead of just the body tube changing size), it's only half working (the handle doesn't stretch with the rest of the pump and the hose attachment becomes misaligned), and therefore it's not at all useful. I would feel better if there wasn't a way to stretch the width and height at all instead of the current state that it's in.

What do you think @pixelzoom?

pixelzoom commented 5 years ago

@chrisklus asked:

... What do you think @pixelzoom?

I agree that the current implementation is not a good way the way to handle sizing of the pump, and it's only a matter of time before it will need to be redone. That said, I don't have time to work on this, and it's not needed for Gas Properties. So I will defer the decision to you and @ariel-phet.

chrisklus commented 5 years ago

Okay, I'm just going to do it then. Can you assign a priority please @ariel-phet.

pixelzoom commented 5 years ago

BicyclePumpNode is failing in Gas Properties. It's allowing me to pump in more particles than rangeProperty. Still no demo in scenery-phet, so I'm going to add one.

chrisklus commented 5 years ago

Ah, looks like you skipped over the range check in https://github.com/phetsims/scenery-phet/commit/872860a38ac811394eeae94a8ea107dd4adb896e. Same for enabled property.

Edit: Yeah I originally screwed that up in https://github.com/phetsims/scenery-phet/commit/0bf6b4488dc808c2f32b08d86fa6bf1c4ca680a1, my apologies.

pixelzoom commented 5 years ago

Where are those checks skipped? I'm not seeing it. The relevant code is at line 635:

if ( rangeProperty.value.max - numberProperty.value > 0 && enabledProperty.get() ) {
  if ( options.addParticlesOneAtATime ) {
    numberProperty.value++;
  }
  else {
    numberOfBatchParticles++;
  }
}
pixelzoom commented 5 years ago

Ah... The check is using numberProperty.value, which is not updated when !options.addParticlesOneAtATime.

pixelzoom commented 5 years ago

Range check fixed in the above commit. Seems to be working fine in Gas Properties and scenery-phet demo. I'm less familiar with SOM; I pumped until the pump was empty and didn't experience any problem. @chrisklus could you check my work?

pixelzoom commented 5 years ago

@chrisklus you might also want to review demoBicyclePumpNode in scenery-phet demo.

chrisklus commented 5 years ago

Range check fixed in the above commit. Seems to be working fine in Gas Properties and scenery-phet demo. I'm less familiar with SOM; I pumped until the pump was empty and didn't experience any problem. @chrisklus could you check my work?

Looks perfect. The range check for !options.addParticlesOneAtATime will just work the same as before since numberOfBatchParticles is initialized to 0.

chrisklus commented 5 years ago

@chrisklus you might also want to review demoBicyclePumpNode in scenery-phet demo.

Looks great, thanks for doing that.

pixelzoom commented 4 years ago

I believe work on this has been done for awhile, so I'm going to close this issue. If that's not the case, @chrisklus please reopen.