[WIP/POC] Libscroll Integration #766

szbergeron · 2020-02-09T01:10:55Z

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):

Open for events
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 · 2020-02-09T01:13:38Z

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 · 2020-02-09T17:25:39Z

src/Core/dune

@@ -5,4 +5,4 @@
    (c_names file)
    (c_flags :standard -Wall -Wextra -Werror)
    (preprocess (pps ppx_deriving.show))
-    (libraries threads console.lib str lwt sdl2 skia flex Rench re Revery_Native revery-text-wrap timber))


You're taking out skia and putting fontkit back?

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

glennsl · 2020-02-09T17:28:00Z

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

Is there anything else you want reviewed here?

glennsl · 2020-02-09T17:28:52Z

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

szbergeron · 2020-02-09T17:29:20Z

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

glennsl · 2020-02-09T18:13:36Z

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 · 2020-02-09T18:23:19Z

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 · 2020-02-09T18:32:52Z

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

szbergeron · 2020-02-09T18:40:02Z

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 · 2020-02-09T18:47:48Z

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

szbergeron · 2020-02-09T18:50:06Z

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 · 2020-02-09T19:09:09Z

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 · 2020-02-09T19:18:05Z

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 · 2020-02-09T19:46:59Z

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 · 2020-02-09T19:56:12Z

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 · 2020-02-09T20:13:26Z

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 · 2020-02-09T20:18:02Z

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 · 2020-02-09T20:40:49Z

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; 
        
           };

…scroll_integration

glennsl · 2020-03-08T08:14:45Z

src/UI_Components/ScrollView.re

+            switch(wheelEvent.deltaX) {
+            | Some(delta) => {
+                Libscroll.push_pan(scrollview, Libscroll.Axis.Horizontal, delta, wheelEvent.timestamp);
+              }
+            | None => ()
+            };


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

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)

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); }

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

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.

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

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

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));

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)

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 · 2020-03-08T08:17:28Z

src/UI_Components/ScrollView.re

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


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);

};

Ok, will change

…scroll_integration

szbergeron requested review from glennsl and bryphe February 9, 2020 17:10

glennsl reviewed Feb 9, 2020

View reviewed changes

szbergeron added 3 commits February 21, 2020 19:43

Initial add libscroll

32d3a8c

Merge branch 'master' of https://github.com/revery-ui/revery

9b80ce6

Push temp changes for input

fef8273

szbergeron force-pushed the libscroll_integration branch from f30b1dc to fef8273 Compare February 26, 2020 21:36

szbergeron added 4 commits February 26, 2020 16:58

More changes to figure out why build fails

81223fe

Get building with new fields

d538a89

Precise scroll now (sort of) works

110bd8f

Change event makeup to be more ideomatic

d349008

glennsl mentioned this pull request Mar 4, 2020

Smooth scrolling instead of line scrolling? onivim/oni2#1408

Open

Add changes before pulling in latest master

52c120e

Merge branch 'master' of https://github.com/revery-ui/revery into lib…

9e15386

…scroll_integration

glennsl reviewed Mar 8, 2020

View reviewed changes

szbergeron mentioned this pull request Mar 10, 2020

Initial refactor for more sane pan events revery-ui/esy-sdl2#11

Open

szbergeron added 2 commits April 13, 2020 16:43

Merge branch 'master' of https://github.com/revery-ui/revery into lib…

f5451c1

…scroll_integration

Merge branch 'master' of https://github.com/revery-ui/revery into lib…

06c7a9d

…scroll_integration

szbergeron mentioned this pull request Apr 15, 2020

Support smooth scrolling #640

Closed

szbergeron added 5 commits April 21, 2020 19:35

Mostly working

a1bbd2e

Idle+animating+sampling+bounds/geometry

2a12b58

cleanup

7acd3f1

Cleanup

8863d53

Avoid libscroll panicking on non-pan-supporting platforms

b03b8a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP/POC] Libscroll Integration #766

[WIP/POC] Libscroll Integration #766

szbergeron commented Feb 9, 2020

szbergeron commented Feb 9, 2020

glennsl Feb 9, 2020

szbergeron Feb 9, 2020

glennsl commented Feb 9, 2020

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 •

edited

Loading

glennsl commented Feb 9, 2020 •

edited

Loading

szbergeron commented Feb 9, 2020 •

edited

Loading

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 •

edited

Loading

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 •

edited

Loading

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 •

edited

Loading

glennsl commented Feb 9, 2020

glennsl Mar 8, 2020

szbergeron Mar 8, 2020

glennsl Mar 8, 2020 •

edited

Loading

szbergeron Mar 8, 2020

glennsl Mar 8, 2020

glennsl Mar 8, 2020

szbergeron Mar 8, 2020

glennsl Mar 8, 2020

szbergeron Mar 9, 2020

szbergeron Mar 9, 2020

glennsl Mar 8, 2020 •

edited

Loading

szbergeron Mar 8, 2020

[WIP/POC] Libscroll Integration #766

Are you sure you want to change the base?

[WIP/POC] Libscroll Integration #766

Conversation

szbergeron commented Feb 9, 2020

szbergeron commented Feb 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl commented Feb 9, 2020

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 • edited Loading

glennsl commented Feb 9, 2020 • edited Loading

szbergeron commented Feb 9, 2020 • edited Loading

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 • edited Loading

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 • edited Loading

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020

glennsl commented Feb 9, 2020

szbergeron commented Feb 9, 2020 • edited Loading

glennsl commented Feb 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szbergeron commented Feb 9, 2020 •

edited

Loading

glennsl commented Feb 9, 2020 •

edited

Loading

szbergeron commented Feb 9, 2020 •

edited

Loading

szbergeron commented Feb 9, 2020 •

edited

Loading

szbergeron commented Feb 9, 2020 •

edited

Loading

szbergeron commented Feb 9, 2020 •

edited

Loading

glennsl Mar 8, 2020 •

edited

Loading

glennsl Mar 8, 2020 •

edited

Loading