[WIP/POC] Libscroll Integration

[szbergeron]

Creating to track feedback on integration approaches

Might be a little more complicated than I thought, since each Revery scrollview needs it's own corresponding tracking object from libscroll to avoid bugs akin to this.

Usage follows 2 phases (pipelined):

1. Open for events
2. Locked for render

Most sane way I can think to do this is to add a Revery event that marks end of frame (signals everything to move from phase 1 to phase 2), which each scrollview itself subscribes to. Pan events are not directly used to transform the viewport, but are simply passed to libscroll scrollview instances. When an end-of-frame event comes in, the scrollview is told to mark end-of-frame and then the resulting transform directive is used much as how scroll events are handled now.

I'm a bit shaky on how some areas of Revery fit together so if any part of this sounds dumb just let me know :)

[szbergeron]

This would also want wheelEvents to be a little bit smarter than they are now, or separate events between precise pan and normal wheel events (would probably make incremental integration a bit easier too)

[glennsl]

You're taking out skia and putting fontkit back?

[szbergeron]

@glennsl whoops, not intentionally. I think that file got left behind as I was jumping between branches

[glennsl]

You also need to run ./update-lockfiles.sh to fix the CI.

Is there anything else you want reviewed here?

[glennsl]

I guess the plan, but that sounds like @bryphe territory to me :)

[szbergeron]

Just wanted to make sure the plan for actually integrating it with scrollviews isn't blatantly stupid

[glennsl]

to avoid bugs akin to this.

That bug report was resolved with the conclusion that it only applies to "old-style" events, not "new-style" events. Is this integration using old-style events? If so, why? That model does seem pretty broken.

[szbergeron]

to avoid bugs akin to this.

That bug report was resolved with the conclusion that it only applies to "old-style" events, not "new-style" events. Is this integration using old-style events? If so, why? That model does seem pretty broken.

That bug was caused by the driver emitting scroll events for "fling", basically where if you start a fling then move the cursor over some other scrollview the first one will stop and the new one will spontaneously start moving. My original plan was to simply add libscroll at the top level and integrate with the render loop, but if it ignores what scrollview the cursor is currently over then a similar bug would occur

Would happen for either new or old style events, ends up just being an implication of doing fling or various other fancy scroll stuff without the context of the specific content it's interacting with

[glennsl]

Oh, I see, so that bug is just in reference to needing separate tracking objects? Then why is the two-phase approach necessary?

[szbergeron]

The two phase approach allows interpolating/extrapolating events properly (avoids jitter from misaligned or conflicting input/output refresh rates)

Good rundown on the topic: https://discourse.gnome.org/t/synchronizing-input-events-to-the-screen-refresh/1422

It would theoretically be possible to do simply with timers within libscroll, but that seemed a little janky/unecessary (makes usage a lot less predictable)

It basically just tells libscroll "OK, I'll be asking for the translation required for the next frame", then allows calling the functions to get that translation idempotently

[glennsl]

Could it not also use an external timing source, with the current time being passed in on each call?

[szbergeron]

Could it not also use an external timing source, with the current time being passed in on each call?

It definitely could, that seemed like it would expose more of the inner workings than felt convenient for consumers to use. Would that also include the prediction/overshoot period or should that be tacked on at the end of the timestamp given?

As an addendum, that might make it slightly more complicated to implement acceleration or correction, probably not much harder than handling VRR tho

[glennsl]

It definitely could, that seemed like it would expose more of the inner workings than felt convenient for consumers to use.

The two-phased approach seemed more like exposing implementation details to me, but then I'm still not sure how that would work. Time on the other hand is an inherent part of the model.

Would that also include the prediction/overshoot period or should that be tacked on at the end of the timestamp given?

I don't know what any of that means :) Why is prediction needed?

I'm sure you can guess by now that I haven't followed the earlier discussion all that closely. Apologies for sounding like I have no clue what you're talking about, but I mostly don't. So please don't feel like you have to explain everything in excruciating detail if you don't want to. I'm sure @bryphe will be able to give just as good feedback with much less effort needed on your end.

[szbergeron]

I'm sure you can guess by now that I haven't followed the earlier discussion all that closely. Apologies for sounding like I have no clue what you're talking about, but I mostly don't. So please don't feel like you have to explain everything in excruciating detail if you don't want to. I'm sure @bryphe will be able to give just as good feedback with much less effort needed on your end.

No worries lol, it's a surprisingly deep subject.

I don't know what any of that means :) Why is prediction needed?

Doing prediction is primarily for touchscreens, but should help make trackpads feel ever so slightly snappier. This basically tries to estimate where a touch would actually be mid way through the next frame's display period, to try to keep the delta from the onscreen content to the actual touch point as low as possible (theoretically zero if scroll velocity is constant.) A nice example of this in action (as far as I can tell) is microsoft surface tablets. From what I can see, they do some of this prediction to keep them as aligned as possible with the given touch point. Android doesn't, which leads to spacial lag (even if the actual temporal lag is imperceptible)

The two-phased approach seemed more like exposing implementation details to me, but then I'm still not sure how that would work. Time on the other hand is an inherent part of the model.

The two phase thing is ideally to allow users to do a minimum of their own math, just say how long frames usually last, how long until the next one will be displayed on screen, and then call mark_frame() every time a render loop happens so libscroll can do its magic. I could expose a way of asking to sample(timestamp) but that would force the consumer (in this case Revery) to do all the prediction math manually and understand the mechanics behind trying to do spacial prediction

[glennsl]

Ah, I see. Thanks for explaining! I think the iPad also does a great job with the Apple Pencil.

So the prediction period then is basically just the time between user action and display response? And the only variable there is render time/frame rate?

If so I still don't understand why phases are needed. If for every frame the client asks "give position at this time", can the server not assume this call happens exactly once per frame and estimate the frame rate based on that? And in the rare case that's not true, or the client can provide a better estimate, it can do so using an optional argument?

[szbergeron]

Ah, I see. Thanks for explaining! I think the iPad also does a great job with the Apple Pencil.

So the prediction period then is basically just the time between user action and display response? And the only variable there is render time/frame rate?

If so I still don't understand why phases are needed. If for every frame the client asks "give position at this time", can the server not assume this call happens exactly once per frame and estimate the frame rate based on that? And in the rare case that's not true, or the client can provide a better estimate, it can do so using an optional argument?

Good point, thinking about it I've come around to the idea of having a requested timestamp be provided by the consumer (especially around VRR cases and the like, so I can step animations by a precise delta instead of trying to speculate) for how to sample. It also probably allows for way easier testing/mocking on my part to have that be the case.

I can try getting that in place a bit more tonight probably.

Question: what would this look like inside of Revery? IE do elements in revery know when they're about to be rendered such that they could know when to sample?

[glennsl]

Yeah, I think the rendering would be driven by the element itself in this case, much like how we do animation:

revery/src/UI_Hooks/Revery_UI_Hooks.re

Lines 17 to 19 in 4ca3483

	let%hook (time, resetTimer) = timer(~active=active && !isCompleted, ());
	let (value, animationState) = Animation.apply(time, animation);

This will continuously trigger a re-render as long as ~active is true, and returns the time to be used in the rest of the render function. The animations in Revery are completely pure functions.

[szbergeron]

Yeah, I think the rendering would be driven by the element itself in this case, much like how we do animation:

revery/src/UI_Hooks/Revery_UI_Hooks.re

Lines 17 to 19 in 4ca3483

	let%hook (time, resetTimer) = timer(~active=active && !isCompleted, ());
	let (value, animationState) = Animation.apply(time, animation);

This will continuously trigger a re-render as long as ~active is true, and returns the time to be used in the rest of the render function. The animations in Revery are completely pure functions.

Oh very cool, I'll try to look into it a bit more tonight I guess, since that example almost perfectly fits how the library itself models things (save for events/correction and such, but those are silent here)

Those hooks only get called after all input events have been serviced right? Or are they called sometimes in the middle? It should still be possible to handle for libscroll, but it would improve latency/responsiveness slightly to make sure that all events come in before that hook

[glennsl]

Those hooks only get called after all input events have been serviced right? Or are they called sometimes in the middle?

I'm not entirely sure, but the application loop certainly looks like it does:

revery/src/Core/App.re

Lines 210 to 250 in 4ca3483

	let appLoop = () => {
	_flushEvents();
	Tick.Default.pump();
	if (appInstance.isFirstRender
	|| _anyWindowsDirty(appInstance)
	|| _anyPendingMainThreadJobs()
	|| !appInstance.canIdle^()) {
	if (appInstance.idleCount > 0) {
	Log.debug("Upshifting into active state.");
	};
	Performance.bench("_doPendingMainThreadJobs", () =>
	_doPendingMainThreadJobs()
	);
	Performance.bench("renderWindows", () => {
	List.iter(w => Window.render(w), getWindows(appInstance))
	});
	appInstance.idleCount = 0;
	appInstance.isFirstRender = false;
	} else {
	appInstance.idleCount = appInstance.idleCount + 1;
	if (appInstance.idleCount === framesToIdle) {
	Log.debug("Downshifting into idle state...");
	appInstance.onIdle();
	};
	let evt = Sdl2.Event.waitTimeout(250);
	switch (evt) {
	| None => ()
	| Some(evt) => _handleEvent(evt)
	};
	};
	Environment.yield();
	false;
	};

[glennsl]

Why is this an option? Wouldn't a delta of 0 be "neutral" and effectively do nothing anyway?

[szbergeron]

If it wasn't doing interpolation/prediction then yes, but with both enabled it would mean that acceleration goes to infinity and velocity drops to zero, which would cause weird stuttering if event ordering isn't consistent (and also make it so that fling sometimes wouldn't work)

[glennsl]

Can you not just weed out the 0s from the interpolation? Or does the None have some other significance?

Like, if it was not an option, would this not be equivalent?

if (wheelEvent.deltaX != 0) {
  Libscroll.push_pan(scrollview, Libscroll.Axis.Horizontal, wheelEvent.deltaX, wheelEvent.timestamp);
}

[szbergeron]

Probably, at least on Wayland I never seem to get a zero delta. I still need to figure out how to drag prediction toward zero if velocity drops but no zero event or fling event comes in.

It seems semantically odd to special case zero delta to me but it might be fine

[glennsl]

I find it odd that it needs to be special-cased too, but I would consider both using an option and weeding out 0s special-casing. A delta of 0 seems completely normal to me, so I'm wondering if the problem is somewhere else in the model. Unless you work with imperfect sensor data, which of course happens sometimes.

[glennsl]

If these were polymorphic variants, you wouldn't even need to do the mapping. It's a good use case for it.

[szbergeron]

Hm, how would that work? Am not super familiar with them

[glennsl]

Polymorphic variants are the structurally typed equivalents of ordinary variants. Their identity is derived from the type itself, not from any definition. In this case specifically that means SDL and libscroll can share a polymorphic variant type without either library knowing about the other.

By example, it looks like this:

let functionTakingPolyVariant = (poly: [`Number(int) | `Text(string)]) =>
  switch (poly) {
  | `Number(num) => "Number: " ++ string_of_int(num)
  | `Text(text) => "Text: " ++ text
  };

let str = functionTakingPolyVariant(`Number(42));

[szbergeron]

Something I'm also remembering here, is that wayland sends events for when the source axis changes for a pan, and that contains an undefined delta. I probably need a way of excluding the delta as well. Might make sense to have a more general axis enum indicating horizontal, vertical, or undefined (undefined meaning the delta is meaningless, and shouldn't be interpreted)

[szbergeron]

Looking at how this interacts with scroll stop events makes me want to bring back the guard, for single accesses.

Effectively what I want to encode is the variant/enum

Info(Source)
Fling(Axis, Source) |
Interrupt(Axis, Source) |
Pan(Axis, Source, delta)

in a way that is fairly ideomatic in c and translates well across to reasonml/rust code. What are your opinions on doing that nicely?

I might be overfitting for the linux way of handling input, but afaik it's pretty similar on OSX and can be fully described for the Windows model (just compose multiple events)

[glennsl]

Don't use pattern matching for boolean conditionals. That's what if expressions are for. They communicate intent better, are less noisy, and also shorter in cases like this where you can skip the else branch:

Suggested change

	switch(wheelEvent.isFling) {
	| true => Libscroll.push_fling(scrollview, wheelEvent.timestamp);
	| false => ()
	};
	if (wheelEvent.isFling) {
	Libscroll.push_fling(scrollview, wheelEvent.timestamp);
	};

[szbergeron]

Ok, will change