Smooth flights

[jaredkhan]

Fixes #910

Before:
https://github.com/user-attachments/assets/7e1d2d55-ac31-44c6-82b4-b9c2c312f9c6

After:
https://github.com/user-attachments/assets/63a520af-c209-4502-8d74-d105ffe137c2

Essentially replacing the use of setTimeout with the use of requestAnimationFrame in a way that checks the elapsed time and does the movement propoertionally based on that

[davidebbo]

Tests failing with:

ReferenceError: localStorage is not defined
    at progressWipe (/home/david/www-dev/web2py/applications/OZtree/OZprivate/rawJS/OZTreeModule/src/tour/handler/TsProgress.js:97:17)

Maybe something needs to be mocked?

[jaredkhan]

Yeah the localStorage thing is there in main too actually, the actual failure was due to requestAnimationFrame not being available. Fixed now.

[davidebbo]

Thanks, it passes now for me. But FWIW, it's not failing for me on main.

[jaredkhan]

No I'm not saying it fails on main , just that the error you quoted is there when running the tests:
(ReferenceError: localStorage is not defined...)

[davidebbo]

No I'm not saying it fails on main , just that the error you quoted is there when running the tests:
(ReferenceError: localStorage is not defined...)

Ah I see!

[davidebbo]

Tried it, definitely looks smoother!

[jaredkhan]

At the moment, we do this thing of going back to perform_actual_fly every frame when we're going a long way. This means that we'd ideally want to be more careful about capturing all non-trivial work between when we start the timer (fly_start_time) and when we measure the time proportion. So ideally we'd get all the re_calc/reanchor stuff inside that interval.

Buuut I wasn't totally sure why we do the re-anchoring this frequently anyway. Since we're limiting the r_mult range to 1M either way might it not make sense to just re-anchor at the end of this planned segment of the flight rather than every frame?

[lentinj]

I think the main thing to avoid is chunking up the flight into r_mult chunks, and the final deceleration phase being really short, because it's only just beyond the r_mult limit.

Beyond that, I think the constant re-anchoring is just what was easiest at the time. Removing it definitely makes sense.

[jaredkhan]

Yeah that makes sense. Perhaps a sensible scheme would be to chop the interval in half if it's greater than the interval

[lentinj]

I think this is all really good stuff, making the code both simpler and better.

My main criticism is that it makes the flights a bit robotic. Not from any bugs in this commit, but rather the increased framerate makes shortcomings in the flight code more obvious.

I think it's worth playing with the pan_proportion() / zoom_proportion() easing functions to make them less subtle, maybe using the equivalent of the bezier curve "ease-in-out" uses.

The pause at the common ancestor of a flight is also quite noticeable now, but as @jaredkhan already notes, we should plan more of the flight in advance, which would help here too.

[lentinj]

I think the main thing to avoid is chunking up the flight into r_mult chunks, and the final deceleration phase being really short, because it's only just beyond the r_mult limit.

Beyond that, I think the constant re-anchoring is just what was easiest at the time. Removing it definitely makes sense.

[lentinj]

Whilst it all looks correct to me, I think we have too many magic numbers in this expression at this point. global_anim_speed as a tweakable value is kinda pointless after this. The point was to fake smooth flights for screencasts, and now we just have smooth flights all the time :)

Does doing something like the following make sense?

fly_duration_s = Math.max(Math.abs(Math.log(r_mult)), 12 / global_anim_speed) * global_anim_speed
    / speed / original_flight_fps;

... and then ...

const original_flight_fps = (1000/60) / 10;  // 10 calibrates frame rate against animation speed values
fly_duration_s = Math.max(Math.abs(Math.log(r_mult)), 1.2)
    / (speed * original_flight_fps);

I also think that Math.max(x, 1.2) is too subtle to be worth it, and only really serves to accentuate the shuffling backwards to the common ancestor when you're basically already on it. My preference would be to bin it, but maybe @jrosindell should weigh in at this point.

[jaredkhan]

Yeah I think the units of global_anim_speed and speed aren't very obvious (some mystery constant factor of 1/scale-doubling time).

That said, I don't think the Math.max is that subtle, the current easing looks quite abrupt at low durations because what you end up with looks like high acceleration.

river_turtle_with_min.mov

I think I'll leave this as it is tbh, not seeing a clear benefit of reshuffling stuff, certainly if we call it original_flight_fps then it should be in units of frames per second and if we call it something else and stuff the factor of 10 in there then it's kind of just another bizzarely-united value floating around.

river_turtle_no_min.mov

[lentinj]

Worth noting I've only tested this at 60Hz, both on my desktop and my (ageing) phone. It'd be interesting to see if we can keep up with 120Hz, or close enough that it doesn't appear choppy.

[jaredkhan]

Yeah I've been running it on a 120Hz display sometimes. It more or less keeps up!
There's a couple of long-running frames which are, as usual, dominated by text rendering, but nothing particularly bothersome to my eye.

[hyanwong]

Is this ready to merge, or are there some of @lentinj 's suggestions to add / investigate?