Move AnimFrame from lifecycle to event.

[jneem]

Fixes #1060.

I'm sure there's a good reason for the current placement of AnimFrame, so I definitely expect pushback on this; I'm mostly just exploring possible solutions for #1060, and this seemed like the easiest.

I think there is part of this patch that is worth salvaging in any case: it simplifies the handling of last_anim in window.rs, and it makes the behavior of AnimFrame match the documentation, even in the case that the animation is first requested in the event context (previously, that would lead to AnimFrame having a non-zero initial value).

[raphlinus]

I don't have very strong feelings whether animframe is an event or a lifecycle; I think I originally had it as the former. I believe @cmyr was the main driver behind moving it to lifecycle, and might have a good argument why it should be kept there.

[cmyr]

the animation event is definitely one of the awkward corners of druid, as it currently exists.

The reason I don't like AnimFrame as an event is pretty simple: it behaves differently from every other event.

In general, in druid, execution follows a very simple cycle:

• the runloop sits idle, waiting for an event to arrive from the OS
• an event arrives, and we call event on the root widget
  • various LifeCycle events possibly happen as a result of changes in the state of the framework triggered by the incoming event
• we finish handling the original event, and if Commands have been submitted we dispatch these (as events; Command could also very reasonably be a separate method, but it isn't clear that this is a win)
• when commands are finished, we send update to every window in the application; this is an opportunity for widgets that did not see the original event to see the changes in the data, and invalidate/update state as needed. Once update has been sent, widgets know that no more data changes are possible before the next layout & paint calls.
• we send layout, and figure out where widgets are in the window
• we send paint and draw the widgets.

The problem with AnimFrame is that it does not arrive at druid-shell like other events. This might be the problem we need to solve.

Druid keeps a flag indicating whether or not it is currently animating. When you request an animation frame, we set this flag, and then ask druid-shell to paint us. The paint call returns a bool, indicating whether we're animating; if we return true, druid-shell schedules another paint call.

The thing that is weird about the AnimFrame event is that it is generated while we are handling the paint call. This means that if we mutate data during the handling of this event, that mutation isn't visible to other widgets before they get a chance to paint; it isn't visible until the next event arrives that triggers the normal event/update cycle.

This is why AnimFrame doesn't quite fit as an Event; because of how it is delivered, it cannot provide the same guarantees.

A possible approach:

It would be nice if we could make AnimFrame an event. One possible approach to this would be to add a 'vsync' event; this might be a mode that you turn on and off, and when it is on it means that druid gets called back with an event each time the window can draw a frame, and we can handle this before the paint call arrives. I'm not sure how feasible this is, but if we could do it it would really help clear all this up; animation would behave just like other events.

[jneem]

Some of what you wrote I changed in #1057, which is what made this PR feasible. The current situation is that when the OS tells druid-shell that it's time to repaint, druid-shell calls into druid twice. The first time, it calls WinHandler::prepare_paint (which is allowed to cause invalidations and request animation frames). When prepare_paint returns, druid-shell calls paint.

This PR delivers the AnimFrame during prepare_paint, meaning that widgets can still request_paint or request_anim_frame. After the AnimFrame is delivered, there's an update call also. So unless I've misunderstood something, it appears to be doing pretty much what your 'vsync' proposal says.

[cmyr]

oh cool, yes, this does sound similar, and might be enough to make this change.

One other concern I would have is that often when the OS is telling us to paint, we're really not expected to be doing anything else, and we possibly risk missing frames if we're doing a bunch of other stuff while handling that event; but I'm happy to just write that down and put the onus on the programmer to not be too silly.

[cmyr]

And to clarify: really the only basis of my having moved AnimFrame out of Event was that it just didn't follow the same rules as other events. If that is changed, I have no complaints.

[jneem]

Ok, cool. I will do some cleanup and testing, and then remove the draft status.

One other concern I would have is that often when the OS is telling us to paint, we're really not expected to be doing anything else, and we possibly risk missing frames if we're doing a bunch of other stuff while handling that event; but I'm happy to just write that down and put the onus on the programmer to not be too silly.

This is a good point, and I've added a comment to AnimFrame. One related remark is that the X11 backend (which despite being half-finished in many respects is IIUC the only backend that explicitly knows about vsync timing) takes the most conservative approach to timing its paint: it starts painting as soon as the previous vsync is over. I think this means that it doesn't matter whether we do the expensive computation in the paint call or as a response to, for example, a different event.

[cmyr]

This looks entirely reasonable, and smooths over what was one of the uglier corners of the event loop, which makes me very happy. Thanks!

[jneem]

Great, thanks! @ForLoveOfCats are you close to landing #1107? If so, I can wait and rebase off that.

[cmyr]

I'll take another pass at #1107 soon.

[ForLoveOfCats]

This should no longer be blocked!