Bug: Times passed to AnimationTimer should not fluctuate

[Michael Strauß]

Yes, that makes sense. In any case, we shouldn't be using a system
timer, but simply record the timestamp at v-sync, and then pass this
precise timestamp to all AnimationTimers. It shouldn't matter when
AnimationTimers are invoked between frames, as long as the timestamp
corresponds to the v-sync signal. (Well, unless the timer callback
measures its own time, which it shouldn't do.)


On Thu, Aug 29, 2024 at 8:20 PM John Hendrikx <john.hendrikx at gmail.com> wrote:
>
> I think they're a bit separate.  Even with VSync, the time it takes to kick the FX thread in action is still going to be between 0-30ms.  If it then passes `System.nanoTime()` to the AnimationRunnables, you're basically saying that they should render a frame at the precise time of VSync-Time + random time it took to schedule the FX thread... suffice to say that the extra accuracy of the more accurate VSync timer (just like my far more accurate timer) is made completely redundant by the jitter introduced by the scheduler.
>
> This brings me back to my original point: we should not be passing `System.nanoTime()` to AnimationRunnables.  Passing `System.nanoTime()` is basically asking to create a frame with a time index that will NEVER be rendered, so why are we asking Animations to use this value for calculating animation locations/offsets/sizes ?
>
> This problem is also present on Mac and Linux, just less noticeable because their schedulers generally react within 0-2 ms (vs 0-30 ms on Windows).  2 ms is "close enough" to the most commonly used frame rates (60 fps, at 16.667 ms per frame), but on Windows it can practically be a two frame difference.
>
> Even in the absence of V-sync, when JavaFX arbitrarily picks 60 Hz as its refresh frequency, the times passed to AnimationTimer should be multiples of 16.667 ms, not 16.667 ms + however long it took to wake up the FX thread.  In other words this code in AbstactPrimaryTimer:
>
> private long nextPulseTime = nanos();
>
> private long lastPulseDuration = Integer.MIN_VALUE;
>
> @Override
>
> public void run() {
>
> if (paused) {
>
> return;
>
> }
>
> final long now = nanos();
>
> recordStart((nextPulseTime - now) / 1000000);
>
> timePulseImpl(now);
>
> recordEnd();
>
> updateNextPulseTime(now);
>
> // reschedule animation runnable if needed
>
> updateAnimationRunnable();
>
> }
>
> ...would be far better if it passed "nextPulseTime" to `timePulseImpl` (which eventually calls the AnimationRunnables) instead of "now".
>
> Note: this is assuming the adaptive pulse flag is disabled.  If it is enabled, nextPulseTime won't be a nice multiple of the frame rate -- so when this is enabled we may want to round it up/down before passing it to the AnimationRunnables.
>
> Note 2: you can **already** achieve far smoother animation even on Windows by rounding the value you get passed in to a multiple of 1/frameRate. This only works when you have access to the this time. It won't solve Timeline calculations -- they will still calculate positions and values for frames that will never exist, subject to FX thread scheduling jitter...
>
> --John