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

KeyboardDragListener accumulates remainder errors in the long run #1531

Closed
samreid opened this issue Feb 1, 2023 · 3 comments
Closed

KeyboardDragListener accumulates remainder errors in the long run #1531

samreid opened this issue Feb 1, 2023 · 3 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Feb 1, 2023

Discovered by me and @zepumph while working on #1530 and phetsims/friction#322, we observed that the "press a key and hold" quantized behavior may be inaccurately ignoring remainders as it looks. These errors could accumulate so that the long-term behavior is not as expected. @zepumph and I experimented with tracking the remainders and subtracting them from the next cycle, but it was complicated because there were 2 places calling updatePosition.

We also observed that Animation.ts in twixt already has support for this behavior: passing in dts, having a start-up delay, and then firing events at a given rate. I'm also recalling maybe there is something in phet-core for this? Perhaps EventTimer.ts would be very appropriate here and easy to use.

Anyways, we thought it may be good to bring to @jessegreenberg attention. When we are developing more sims with the quantized drag listener, this may be good to investigate.

@zepumph
Copy link
Member

zepumph commented Feb 1, 2023

@jessegreenberg please note the above stop gap for the bug fix. What do you think?

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Feb 1, 2023

The main issue makes sense to me, thanks. The next step from #1520 will be to re-implement KeyboardDragListener with KeyboardListener, which uses CallbackTimer and might address this for us.

While reviewing b7829bd I noticed that the overflowTime value was much larger than moveOnHoldInterval so things were getting moved every frame. It was because the interval was never getting set back to zero after the moveOnHoldDelay. Fixed above.

Adding this to the "alt input" project board for when KeyboardListener/KeyboardDragListener gets work again.

EDIT: Just confirming that CallbackTimer (uses Timer) would fix this
https://github.com/phetsims/axon/blob/33b2c7ffbbad32ffb5b6b9e97a913f54387ee8ae/js/Timer.ts#L66-L67

EDIT2: As shown in that github reference, it would be fixed by using Timer.setInterval (not just by using Timer).

@jessegreenberg
Copy link
Contributor

This was fixed as part of #1570 because KeyboardDragListener now uses CallbackTimer. 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

4 participants