search

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

Timer node stops early #36

Closed KatieWoe closed 5 years ago

KatieWoe commented 5 years ago

Test device: Dell Laptop Operating System: Win 10 Browser: chrome Problem description: For https://github.com/phetsims/QA/issues/220 The timer node stops at 01:39.99 and it should go to 99:99.99 Steps to reproduce:

  1. Go to the Bounce screen
  2. Start the timer and let it run until it stops

Screenshots: bug time

Troubleshooting information (do not edit):

Name: ‪Masses and Springs: Basics‬ URL: https://phet-dev.colorado.edu/html/masses-and-springs-basics/1.0.0-dev.11/phet/masses-and-springs-basics_en_phet.html Version: 1.0.0-dev.11 2018-11-12 16:16:41 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36 Language: en-US Window: 1536x732 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: 16384x16384 OES_texture_float: true Dependencies JSON: {"assert":{"sha":"928741cf","branch":"master"},"axon":{"sha":"036865d6","branch":"master"},"brand":{"sha":"1fd6682e","branch":"master"},"chipper":{"sha":"9e1987e6","branch":"master"},"dot":{"sha":"bbbd8526","branch":"master"},"griddle":{"sha":"f64a7cf0","branch":"master"},"joist":{"sha":"11f7cf47","branch":"master"},"kite":{"sha":"b0fc6c9c","branch":"master"},"masses-and-springs":{"sha":"0ab4798a","branch":"master"},"masses-and-springs-basics":{"sha":"90423db0","branch":"master"},"phet-core":{"sha":"714b088a","branch":"master"},"phet-io":{"sha":"3acf45ca","branch":"master"},"phet-io-wrapper-classroom-activity":{"sha":"5dea7f54","branch":"master"},"phet-io-wrapper-hookes-law-energy":{"sha":"b42eaef7","branch":"master"},"phet-io-wrapper-lab-book":{"sha":"b884e9c1","branch":"master"},"phet-io-wrappers":{"sha":"e90822f4","branch":"master"},"phetcommon":{"sha":"869b2561","branch":"master"},"query-string-machine":{"sha":"06ed6276","branch":"master"},"scenery":{"sha":"3fbe7bbb","branch":"master"},"scenery-phet":{"sha":"76baf536","branch":"master"},"sherpa":{"sha":"2cd50500","branch":"master"},"sun":{"sha":"bc8424bb","branch":"master"},"tambo":{"sha":"ad355580","branch":"master"},"tandem":{"sha":"93822898","branch":"master"},"twixt":{"sha":"11a0c804","branch":"master"}}
Denz1994 commented 5 years ago

I was able to reproduce this same bug with the TimerNode in WOAS, MAS, Pendulum Lab, and CLB. This may be a potential issue in TimerNode.js or TimerNodeReadout.js.

Investigating.

arouinfar commented 5 years ago

@Denz1994 I think in working on wave-interference @samreid introduced a maxValue option to TimerNode, which might be relevant here.

Denz1994 commented 5 years ago

maxValue was introduced https://github.com/phetsims/scenery-phet/commit/d6f4660e7488d1ee6754d56e01878d0605bf233a and the default value has always been maxValue: 99.99. The max value is measured in seconds, so the timer stops at 1:39.99. I don't know why this hasn't been caught before, but the issue seems to be the default value is too small.

I have increased the default to maxValue: 5940 or 99 minutes. Fix pushed to master awaiting next dev release for verification.

KatieWoe commented 5 years ago

Sorry this wasn't caught before. Would this need a maintenance release for any previous sims?

arouinfar commented 5 years ago

@KatieWoe it's unlikely that this issue exists in other published sims, since maxValue wasn't introduced until after the 1.0 publication dates. I think @Denz1994 only found the issue in master, but it would be good to double check.

KatieWoe commented 5 years ago

Just tested Masses and Springs, which is the latest published with timer node that I can recall at the moment, and I got to 2+ just fine.

Denz1994 commented 5 years ago

@arouinfar is correct about this not afffecting other sims. maxValue is a fairly recent addition.

Can you test if this change is acceptable in this dev version @KatieWoe? If all looks good please reassign to me.

KatieWoe commented 5 years ago

Has gone past previous time it stopped at. Will let run on another screen to see if any other problems are encountered.

KatieWoe commented 5 years ago

I have noticed the numbers twitching every few seconds. twitchtime

arouinfar commented 5 years ago

@KatieWoe I'm pretty sure the "twitching" is just a natural consequence of 1's having a smaller width than other numbers. That's not something we can/should address.

KatieWoe commented 5 years ago

Timer node now seems to start over at some point, rather than stopping at a max value. This image is from the same session as above. It was at about 50 min the last time I had checked. I can't be sure of the exact time when the change occurred unfortunately, as I had it running in the background at the time. wrap

Denz1994 commented 5 years ago

Hey, great catch @KatieWoe. I'll be doing some memory test tomorrow, which will involve letting the sim run for a while. I'll try to catch this bug during those tests.

Denz1994 commented 5 years ago

The minute's section of the timer node readout goes from 00-59, then it resets back to 00. The timer only measures one hour before resetting. However, the readout is the only thing that resets and the affiliated property or button state is left unchanged. I changed default time to stop at 59:59.99.

@arouinfar do you think this stopping point is okay? For the timer to reach a stopping point of 99:99.99 (which is what I thought it was doing originally), we would need to adjust some logic. Thoughts @arouinfar?

image

arouinfar commented 5 years ago

@Denz1994 59:59.99 would be the appropriate stopping point! I didn't account for the differences in the mm:ss notation used here, so 99:99.99 was an error on my part. :)

Is 59:59.99 the default maxValue for this flavor of TimerNode? Or is it MAS(B)-specific? Seems like a reasonable default maxValue for all instances of TimerNode (using the 00:00.00 display).

Denz1994 commented 5 years ago

The default value maxValue for TimerNode has been changed to read 59:59.99 as in the comment above. It looks like we can close this issue.

pixelzoom commented 5 years ago

Reopening. I encountered this while working on gas-properties, and it needs some more documentation. The current value and doc is:

      // The maximum value, that can be shown by the TimerNode, so it can set up the size to accommodate the largest
      // string. Default value is set to 59 minutes and 59 seconds.
      maxValue: 3599.99,

This value is not "59 minutes and 59 seconds". It's "59 minutes, 59 seconds, and 99/100 seconds", expressed in seconds. And it was apparently chosen because the timer is not capable of showing any quantity larger than minutes. I'll update the doc.

pixelzoom commented 5 years ago

@Denz1994 please review the above commit to verify that the doc is correct.

Denz1994 commented 5 years ago

The added doc is more comprehensive and describes why this value was chosen. Thanks for the update. Closing.