phetsims / masses-and-springs-basics

"Masses and Springs: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 3 forks source link

Period Trace stops after it is complete when stepping through animation #58

Closed KatieWoe closed 5 years ago

KatieWoe commented 5 years ago

Test device: Dell Operating System: Win 10 Browser: firefox Problem description: For https://github.com/phetsims/QA/issues/281 When stepping through the animation of the mass bouncing on the lab screen, the trace stops drawing after a round is complete. If there is no trace on the screen, or one is in the middle of being drawn, pausing and stepping through the animation appears normal. However, normally the completed trace fades and the process starts over. When stepping through, the trace remains once it is completed, or is paused while fading. Occurs in published MAS and is slightly more serious there. Since dampening changes the period trace, the fact that it does not redraw means you can be left with a large period trace but a small period occurring. It also reveals that the period trace becomes very thin when the mass is about to stop. Steps to reproduce:

  1. Set period trace and start a mass bouncing
  2. Pause the sim.
  3. Hold down the step through button and observe behavior.

Screenshots: periodtracestops

Troubleshooting information (do not edit):

Name: ‪Masses and Springs: Basics‬ URL: https://phet-dev.colorado.edu/html/masses-and-springs-basics/1.0.0-rc.2/phet/masses-and-springs-basics_all_phet.html Version: 1.0.0-rc.2 2019-02-01 20:48:23 UTC Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0 Language: en-US Window: 1536x760 Pixel Ratio: 2.5/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 Vendor: Mozilla (Mozilla) 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: {}
phet-steele commented 5 years ago

This sounds like https://github.com/phetsims/masses-and-springs/issues/269#issuecomment-391831669 (and the later comments) of which that issue was labeled "wont-fix".

KatieWoe commented 5 years ago

Good catch. I'd done a brief search for it, but must have used the wrong keywords.

KatieWoe commented 5 years ago

As a reference. These are the thin lines I mentioned in the issue. First gif shows when using step through. Second shows effect when moving at normal speed.

thinperiodtrace thinlinemoving

arouinfar commented 5 years ago

Nice catch @KatieWoe, and thanks for linking the older issue @phet-steele.

I generally agree with @ariel-phet's decision in https://github.com/phetsims/masses-and-springs/issues/269#issuecomment-392220672. However, I also think it can look a bit buggy when damping is thrown into the mix.

@Denz1994 would it be possible to instead simply clear the period trace after it had been stepped through? We might also consider clearing a few dt's after the trace to give users some leeway. If this would require significant refactoring, feel free to veto @Denz1994.

Denz1994 commented 5 years ago

Would it be possible to instead simply clear the period trace after it had been stepped through?

Pros: Resolves the buggy looking behavior. Once manually stepped, via step forward button, the period trace state is reset and disappears.

Cons: Lose the ability to follow a period trace via stepping manually. This may not be an intention based on this comment

We might also consider clearing a few dt's after the trace to give users some leeway. If this would require significant refactoring, feel free to veto.

Vetoing. This would require more refactoring to the step functions in the model and also monitoring the state of the period during said step functions. I spent about 20 minutes going down this rabbit hole and the code was looking more complicated.

I would prefer the first suggestion to clear after it had been stepped through. The above commit implements this in the master branch. Could you review in MAS master @arouinfar and reassign to me if this looks okay? This requires more testing through QA.

arouinfar commented 5 years ago

@Denz1994 I reviewed master, and the behavior isn't quite what I was envisioning. When stepping through, the period trace should continue to draw until completed, and once completed it would clear.

Here are the test cases I used:

Case 1

  1. Pause while period trace is partially drawn.
  2. Step forward, and the period trace clears immediately instead of continuing to draw.

Case 2

  1. Pause before the period trace starts to draw.
  2. Step forward until the mass crosses the center of oscillation. At this point, the period trace should start drawing, but it doesn't.
  3. Keep stepping forward just to be sure, no period trace is drawn.
Denz1994 commented 5 years ago

I see. The behavior has been modified. Now, stepping manually clears the period trace after a cycle has been completed. If the user continues stepping manually the period trace will redraw itself normally and clear after a cycle.

Note that the period trace clears immediately and doesn't fade because the sim must be paused when stepping manually. It would require a workaround to add this feature.

Could you check this behavior in master @arouinfar?

Note to self: This required a refactor to add Spring.periodTrace across several commits in this issue 7da4769, e7d6823, 8f25714, and 312c4fd

arouinfar commented 5 years ago

Looks great in master, thanks @Denz1994!

Denz1994 commented 5 years ago

RC branch is ready for verification in the next RC test.

KatieWoe commented 5 years ago

This looks ok 1.0.0-rc.3