Bug 233653 – Add a momentum event synthesizer

• Component: Scrolling
• Status: RESOLVED FIXED

[Tim Horton]

Add a momentum event synthesizer

[Tim Horton]

Created attachment 445452 [details]
Patch

[Tim Horton]

<rdar://problem/85571258>

[Tim Horton]

Created attachment 445458 [details]
Patch

[Tim Horton]

Created attachment 445459 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 445459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445459&action=review

> Source/WebKit/Shared/ScrollingAccelerationCurve.cpp:76
> +        return (4 * std::pow(x, 3) * std::pow(m_gainQuartic, 4)) + (3 * std::pow(x, 2) * std::pow(m_gainCubic, 3)) + (2 * x * std::pow(m_gainParabolic, 2)) + m_gainLinear;

This calls std::pow(m_gainQuartic, 4) and std::pow(m_gainCubic, 3) on each call, but that probably doesn't matter.

> Source/WebKit/Shared/ScrollingAccelerationCurve.cpp:107
> +    constexpr float cursorScale = 96.f / frameRate;

Any idea where the 96 comes from? The word "cursor" here is confusing.

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:42
> +class ScrollingAccelerationCurve {

AccelerationCurve or DecelerationCurve?

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:50
> +    float accelerationFactor(float);

const

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:73
> +    float evaluateQuartic(float);

const

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:91
> +    // Input parameters.
> +    float m_gainLinear { 0 };
> +    float m_gainParabolic { 0 };
> +    float m_gainCubic { 0 };
> +    float m_gainQuartic { 0 };
> +    float m_tangentSpeedLinear { 0 };
> +    float m_tangentSpeedParabolicRoot { 0 };
> +    float m_resolution { 0 };
> +
> +    // Computed intermediate values.
> +    bool m_hasComputedIntermediateValues { false };
> +    float m_tangentStartX { 0 };
> +    float m_tangentSlope { 0 };
> +    float m_tangentIntercept { 0 };
> +    float m_falloffStartX { 0 };
> +    float m_falloffSlope { 0 };
> +    float m_falloffIntercept { 0 };

You could group these into structs to make the comments obsolete.

> Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:60
> +static ScrollingAccelerationCurve fromIOHIDCurveArrayWithAcceleration(NSArray *ioHIDCurves, float desiredAcceleration, float resolution)

Can the NSArray * be typed?

> Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:86
> +std::optional<ScrollingAccelerationCurve> ScrollingAccelerationCurve::fromNativeWheelEvent(const NativeWebWheelEvent& nativeWebWheelEvent)

It's a bit weird to say that you're getting a curve from an event. I guess you're really getting a curve from the device associated with the event, so maybe split this into two functions?

> Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:93
> +    auto hidEvent = adoptCF(CGEventCopyIOHIDEvent(event.CGEvent));

Is CGEventCopyIOHIDEvent OK with a null CGEvent?

> Source/WebKit/UIProcess/WebPageProxy.h:3167
> +#if ENABLE(MOMENTUM_EVENT_DISPATCHER)
> +    std::optional<ScrollingAccelerationCurve> m_scrollingAccelerationCurve;
> +    std::optional<ScrollingAccelerationCurve> m_lastSentScrollingAccelerationCurve;
> +#endif

Maybe we should bundle these up with:
    std::unique_ptr<WebWheelEventCoalescer> m_wheelEventCoalescer;
    PAL::HysteresisActivity m_wheelEventActivityHysteresis;

into a wheel-event-related helper class.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:50
> +    stopDisplayLink();

You're sure this gets hit (no retain cycles?)

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:108
> +    return isMomentumEventDuringSyntheticGesture;

I wish we used an enum value for "did this function handle the event" return values.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:138
> +    WebWheelEvent syntheticEvent(WebEvent::Wheel, m_currentGesture.initiatingEvent->position(), m_currentGesture.initiatingEvent->globalPosition(), appKitAcceleratedDelta, wheelTicks, WebWheelEvent::ScrollByPixelWheelEvent, m_currentGesture.initiatingEvent->directionInvertedFromDevice(), WebWheelEvent::PhaseNone, phase, true, m_currentGesture.initiatingEvent->scrollCount(), delta, m_lastIncomingEvent->modifiers(), time, time, { });

I would be inclined to put one arg on each line

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:172
> +    RELEASE_LOG(ScrollAnimations, "MomentumEventDispatcher saw momentum end phase with total offset %f %f, duration %f (event offset would have been %f %f)", m_currentGesture.currentOffset.width(), m_currentGesture.currentOffset.height(), (WallTime::now() - m_currentGesture.startTime).seconds(), m_currentGesture.accumulatedEventOffset.width(), m_currentGesture.accumulatedEventOffset.height());

Maybe %.1f since they are all integral right?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:174
> +    stopDisplayLink();

Is mismatched start/stop a problem, if that ever happens?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:184
> +    WTF::TextStream stream(WTF::TextStream::LineMode::SingleLine);
> +    stream << curve;

Should remove this at some point. Maybe a TEMPORARY_LOGGING ifdef or something?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:222
> +void MomentumEventDispatcher::windowScreenDidChange(WebCore::PageIdentifier pageID, WebCore::PlatformDisplayID displayID)

A bit odd that no argument references a window.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:415
> +        constexpr float velocityGainA = 2.f / 65536.0f;
> +        constexpr float velocityGainB = 955.f / 65536.0f;
> +        constexpr float velocityConstant = 98369.f / 65536.0f;
> +        constexpr float minimumVelocity = 1.f / 65536.0f;

Use fromFixedPoint()?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.h:84
> +    void didScrollWithInterval(FloatSize, Seconds);
> +    void didScroll(const WebWheelEvent&);

These aren't about scrolling (we have no idea if a scroll happened). They are about event processing I think?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.h:93
> +    typedef Deque<Delta, deltaHistoryQueueSize> HistoricalDeltas;
> +    HistoricalDeltas m_deltaHistoryX;
> +    HistoricalDeltas m_deltaHistoryY;

Can we use one deque of struct  { FloatSize, Seconds }?

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 445459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445459&action=review
> 
> > Source/WebKit/Shared/ScrollingAccelerationCurve.cpp:76
> > +        return (4 * std::pow(x, 3) * std::pow(m_gainQuartic, 4)) + (3 * std::pow(x, 2) * std::pow(m_gainCubic, 3)) + (2 * x * std::pow(m_gainParabolic, 2)) + m_gainLinear;
> 
> This calls std::pow(m_gainQuartic, 4) and std::pow(m_gainCubic, 3) on each
> call, but that probably doesn't matter.
> 
> > Source/WebKit/Shared/ScrollingAccelerationCurve.cpp:107
> > +    constexpr float cursorScale = 96.f / frameRate;
> 
> Any idea where the 96 comes from? The word "cursor" here is confusing.

Not yet.

> > Source/WebKit/Shared/ScrollingAccelerationCurve.h:42
> > +class ScrollingAccelerationCurve {
> 
> AccelerationCurve or DecelerationCurve?

Acceleration (this is "how scroll events are accelerated from physical motion", NOT "how to decelerate for momentum")

> > Source/WebKit/Shared/ScrollingAccelerationCurve.h:91
> > +    // Input parameters.
> > +    float m_gainLinear { 0 };
> > +    float m_gainParabolic { 0 };
> > +    float m_gainCubic { 0 };
> > +    float m_gainQuartic { 0 };
> > +    float m_tangentSpeedLinear { 0 };
> > +    float m_tangentSpeedParabolicRoot { 0 };
> > +    float m_resolution { 0 };
> > +
> > +    // Computed intermediate values.
> > +    bool m_hasComputedIntermediateValues { false };
> > +    float m_tangentStartX { 0 };
> > +    float m_tangentSlope { 0 };
> > +    float m_tangentIntercept { 0 };
> > +    float m_falloffStartX { 0 };
> > +    float m_falloffSlope { 0 };
> > +    float m_falloffIntercept { 0 };
> 
> You could group these into structs to make the comments obsolete.

True, I didn't pick up the inline struct thing when I wrote this, but did use it in MomentumEventDispatcher :)

> > Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:60
> > +static ScrollingAccelerationCurve fromIOHIDCurveArrayWithAcceleration(NSArray *ioHIDCurves, float desiredAcceleration, float resolution)
> 
> Can the NSArray * be typed?

NSArray<NSDictionary *> *, sure.

> > Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:86
> > +std::optional<ScrollingAccelerationCurve> ScrollingAccelerationCurve::fromNativeWheelEvent(const NativeWebWheelEvent& nativeWebWheelEvent)
> 
> It's a bit weird to say that you're getting a curve from an event. I guess
> you're really getting a curve from the device associated with the event, so
> maybe split this into two functions?

Heh, it used to be separate! I will put it back, I think it was better that way indeed.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:174
> > +    stopDisplayLink();
> 
> Is mismatched start/stop a problem, if that ever happens?

It seems to be OK, but I will double check.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:222
> > +void MomentumEventDispatcher::windowScreenDidChange(WebCore::PageIdentifier pageID, WebCore::PlatformDisplayID displayID)
> 
> A bit odd that no argument references a window.

Propagating existing odd naming, yes.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.h:93
> > +    typedef Deque<Delta, deltaHistoryQueueSize> HistoricalDeltas;
> > +    HistoricalDeltas m_deltaHistoryX;
> > +    HistoricalDeltas m_deltaHistoryY;
> 
> Can we use one deque of struct  { FloatSize, Seconds }?

We cannot, because we clear it if direction changes, which is a per-axis thing (I have made this mistake at least twice now :))

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 445459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445459&action=review
> 
> > Source/WebKit/Shared/ScrollingAccelerationCurve.cpp:76
> > +        return (4 * std::pow(x, 3) * std::pow(m_gainQuartic, 4)) + (3 * std::pow(x, 2) * std::pow(m_gainCubic, 3)) + (2 * x * std::pow(m_gainParabolic, 2)) + m_gainLinear;
> 
> This calls std::pow(m_gainQuartic, 4) and std::pow(m_gainCubic, 3) on each
> call, but that probably doesn't matter.

It's only called once :) just shared between two branches.

[Tim Horton]

Created attachment 445480 [details]
Patch

[Tim Horton]

Created attachment 445492 [details]
Patch

[EWS]

Committed r286346 (244705@main): <https://commits.webkit.org/244705@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445492 [details].