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.

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

all times in KeyboardDragListener should be in milliseconds #1011

Closed
zepumph opened this issue Nov 5, 2019 · 2 comments
Closed

all times in KeyboardDragListener should be in milliseconds #1011

zepumph opened this issue Nov 5, 2019 · 2 comments

Comments

@zepumph
Copy link
Member

zepumph commented Nov 5, 2019

Discovered while working on phetsims/gravity-force-lab#186. There are some values in step that are in seconds, and some in milliseconds. Today I talked to @jessegreenberg about it, and we thought about converting it to seconds because that is what the sim gives us in the step function. After poking around I don't think that is the direction we should go. All the following are public options, and seem nice as millisecond values:

  • hotkeyHoldInterval
    *moveOnHoldInterval
  • moveOnHoldDelay

And each of these have an internal counter that is stepped. I think that these should continue to be in milliseconds.

In which case, we should see if the whole file can run in ms.

@zepumph
Copy link
Member Author

zepumph commented Nov 13, 2019

This small change seems good for the most part. For the velocity, @jessegreenberg and I thought it best not to change the public facing apis from "pixels/second" (i.e. dragVelocity), so I ended up just converting within the step function and it is sorta a wash. I don't think it is too horrible though, and this way you have 5 sections in a row doing actions from ms instead of 4 from ms and one from dt. @jessegreenberg please review. Close if all looks good to you.

@jessegreenberg
Copy link
Contributor

Change looks good and has been working for a long time. Other scenery/PhET APIs like FireListener and axon timers are in ms so this was a good change. At the same time, the "velocity" options in pixels per second is also most intuitive.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants