Add a momentum event synthesizer
Created attachment 445452 [details] Patch
<rdar://problem/85571258>
Created attachment 445458 [details] Patch
Created attachment 445459 [details] Patch
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 }?
(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 :))
(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.
Created attachment 445480 [details] Patch
Created attachment 445492 [details] Patch
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].