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.

Sign up for GitHub

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

Jump to bottom

Rename TimerType and timer #329

Closed
samreid opened this issue Sep 10, 2020 · 12 comments
Closed

Rename TimerType and timer #329

samreid opened this issue Sep 10, 2020 · 12 comments
Assignees
@samreid
Labels
status:blocks-publication status:ready-for-review type:bug

Comments

@samreid
Copy link
Member

samreid commented Sep 10, 2020

From https://github.com/phetsims/phet-io/issues/1703#issuecomment-690675434

If we rename timer=>stepTimer or simulationStepTimer then TimerType can be renamed Timer.js
@samreid samreid self-assigned this Sep 10, 2020
samreid added a commit to phetsims/circuit-construction-kit-black-box-study that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
982d193
samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
33a30fc
samreid added a commit to phetsims/balloons-and-static-electricity that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
50d6242
samreid added a commit to phetsims/gene-expression-essentials that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
3ce6cad
samreid added a commit to phetsims/expression-exchange that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
24cb20f
samreid added a commit to phetsims/natural-selection that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
47a30fa
samreid added a commit to phetsims/build-a-molecule that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
c55178d
samreid added a commit to phetsims/states-of-matter that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
565a2a7
samreid added a commit to phetsims/utterance-queue that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
1901646
samreid added a commit to phetsims/build-an-atom that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
030887c
samreid added a commit to phetsims/balancing-act that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
d0cf049
samreid added a commit to phetsims/scenery-phet that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
e81eb92
samreid added a commit to phetsims/faradays-law that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
7f51a92
samreid added a commit to phetsims/area-builder that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
db7efc6
samreid added a commit to phetsims/arithmetic that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
6bd5bf2
samreid added a commit to phetsims/friction that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
5a34fc7
samreid added a commit to phetsims/tangible that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
1d76007
samreid added a commit to phetsims/scenery that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
bd4c071
samreid added a commit to phetsims/twixt that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
cdc95db
samreid added a commit to phetsims/tappi that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
856f930
samreid added a commit to phetsims/joist that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
e956d8f
samreid added a commit to phetsims/vegas that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
ead34fa
samreid added a commit to phetsims/tambo that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
5e956ea
samreid added a commit that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see #329
eba63b6
samreid added a commit to phetsims/sun that referenced this issue Sep 11, 2020
@samreid
Rename timer => stepTimer, see phetsims/axon#329
bb85cb4
samreid added a commit that referenced this issue Sep 11, 2020
@samreid
Rename TimerType => Timer, see #329
10782c4
@samreid
Copy link
Member Author

samreid commented Sep 11, 2020

Fixed and ready for review.

@samreid samreid assigned zepumph and unassigned samreid Sep 11, 2020
@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 11, 2020 •
edited
Loading

Hmm... I didn't realize that timer.js was a step timer. That's now clear with the rename. But then I have a potential problem in NS PlayButtonGroup:

          stepTimer.runOnNextFrame( () => {
            addAMateButton.visible = ( bunnyCount === 1 );
            playButton.visible = ( bunnyCount > 1 );
          } );

This code assumes that it will be run on the next next frame, not on the next step. And I think that's a valid assumption based on the name. But now I see this (new?) documentation in stepTimer.js:

... Only runs when the sim is active, during the step. ...

So 2 questions:

(1) Can we do something about this misleading runOnNextFrame name?
(2) How do I run something on the next frame, regardless of whether this sim is playing/paused.

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 11, 2020 •
edited
Loading

(2) How do I run something on the next frame, regardless of whether this sim is playing/paused.

Looks like I should be using animationFrameTimer.js ? Tracking in phetsims/natural-selection#227.

EDIT: I was wondering if other sims might be making the same assumption as NS. But I see that NS is currently the only use of stepTimer.runOnNextFrame.

@pixelzoom pixelzoom mentioned this issue Sep 11, 2020
Closed
@pixelzoom
Copy link
Contributor

stepTimer.js doc says that it only runs "during the step". Does "run" mean "emits"? If that's the case, then I was expecting to see stepTimer.emit called only by Sim.js. But I also see stepTimer.emit called by KeyStateTracker, Display, and UtteranceTests. Can you clarify why those things are calling stepTimer.emit, and how that's "during the step"?

@zepumph
Copy link
Member

zepumph commented Sep 12, 2020

I may be seeing some CT errors from this issue in built scenery libraries:

Fatal error: Converting circular structure to JSON�

Webpack build errors: [ ModuleNotFoundError: Module not found: Error: Can't resolve './timer.js' in '/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js'
at factory.create (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/lib/Compilation.js:925:10)
at factory (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/lib/NormalModuleFactory.js:401:22)
at resolver (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/lib/NormalModuleFactory.js:130:21)
at asyncLib.parallel (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/lib/NormalModuleFactory.js:224:22)
at /data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/neo-async/async.js:2830:7
at /data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/neo-async/async.js:6877:13
at normalResolver.resolve (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/lib/NormalModuleFactory.js:214:25)
at doResolve (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:213:14)
at hook.callAsync (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:285:5)
at _fn0 (<anonymous>:15:1)
at resolver.doResolve (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:44:7)
at hook.callAsync (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:285:5)
at _fn0 (<anonymous>:15:1)
at hook.callAsync (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:285:5)
at _fn0 (<anonymous>:27:1)
at resolver.doResolve (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43)
at hook.callAsync (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:285:5)
at _fn42 (<anonymous>:16:1)
at hook.callAsync (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:285:5)
at _fn0 (<anonymous>:27:1)
at resolver.doResolve (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43)
at hook.callAsync (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:285:5)
at _fn1 (<anonymous>:16:1)
at hook.callAsync (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:285:5)
at _fn0 (<anonymous>:15:1)
at fs.stat (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/DirectoryExistsPlugin.js:27:15)
at process.nextTick (/data/share/phet/continuous-testing/ct-snapshots/1599867498367/chipper/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:85:15)
at process._tickCallback (internal/process/next_tick.js:61:11)

resolve './timer.js' in '/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js'
using description file: /data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/package.json (relative path: ./js)
Field 'browser' doesn't contain a valid alias configuration
using description file: /data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/package.json (relative path: ./js/timer.js)
no extension
Field 'browser' doesn't contain a valid alias configuration
/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js/timer.js doesn't exist
.wasm
Field 'browser' doesn't contain a valid alias configuration
/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js/timer.js.wasm doesn't exist
.mjs
Field 'browser' doesn't contain a valid alias configuration
/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js/timer.js.mjs doesn't exist
.js
Field 'browser' doesn't contain a valid alias configuration
/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js/timer.js.js doesn't exist
.json
Field 'browser' doesn't contain a valid alias configuration
/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js/timer.js.json doesn't exist
as directory
/data/share/phet/continuous-testing/ct-snapshots/1599867498367/axon/js/timer.js doesn't exist ]
Snapshot from 9/11/2020, 3:38:18 PM

@pixelzoom
Copy link
Contributor

Sounds like it's appropriate to replace "ready-for-review" with "blocks-publication".

@samreid
Copy link
Member Author

samreid commented Sep 12, 2020

Probably a casing problem, I'll take a look soon.

samreid added a commit that referenced this issue Sep 12, 2020
@samreid
Fix Timer casing, and add stepTimer and animationTimer to main, see #329
a4d2d98
samreid added a commit to phetsims/natural-selection that referenced this issue Sep 12, 2020
@samreid
Rename runOnNextFrame => runOnNextTick, see phetsims/axon#329
f931f8f
samreid added a commit to phetsims/joist that referenced this issue Sep 12, 2020
@samreid
Rename runOnNextFrame => runOnNextTick, see phetsims/axon#329
f781f5a
samreid added a commit that referenced this issue Sep 12, 2020
@samreid
Rename runOnNextFrame => runOnNextTick, see #329
e8c0f5c
samreid added a commit to phetsims/tandem that referenced this issue Sep 12, 2020
@samreid
Rename runOnNextFrame => runOnNextTick, see phetsims/axon#329
6ce6a6f
samreid added a commit that referenced this issue Sep 12, 2020
@samreid
Update JSDoc, see #329
44fc81d
samreid added a commit that referenced this issue Sep 12, 2020
@samreid
Update JSDoc, see #329
09f16b8
@samreid
Copy link
Member Author

samreid commented Sep 12, 2020

(1) Can we do something about this misleading runOnNextFrame name?

I renamed it to runOnNextTick.

Looks like I should be using animationFrameTimer.js ? Tracking in phetsims/natural-selection#227.

I commented in the issue.

stepTimer.js doc says that it only runs "during the step". Does "run" mean "emits"? If that's the case, then I was expecting to see stepTimer.emit called only by Sim.js. But I also see stepTimer.emit called by KeyStateTracker, Display, and UtteranceTests. Can you clarify why those things are calling stepTimer.emit, and how that's "during the step"?

I updated the JSDoc in 09f16b8

Probably a casing problem, I'll take a look soon.

I fixed the casing problem in the commits.

@pixelzoom
Copy link
Contributor

Nice improvements, and thanks for the clarification.

@pixelzoom pixelzoom removed their assignment Sep 12, 2020
@samreid samreid removed their assignment Sep 13, 2020
@pixelzoom
Copy link
Contributor

Is this issue blocking for the NS deliverable described in phetsims/natural-selection#208? Milestone for taking shas is this Thursday, 9/17. Ideally this should be wrapped up, because the work has been done and is awaiting review. But my opinion is not blocking, because it doesn't seem to causing any problems, and NS uses stepTimer in one place.

@zepumph
Copy link
Member

zepumph commented Sep 16, 2020

@samreid, the changes look good to me! I like the rename to runOnNextTick. Nothing else here for me.

@zepumph zepumph assigned samreid and unassigned zepumph Sep 16, 2020
@samreid
Copy link
Member Author

samreid commented Sep 16, 2020

Thanks, closing.

@samreid samreid closed this as completed Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@samreid samreid

Labels
status:blocks-publication status:ready-for-review type:bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@samreid @pixelzoom @zepumph