Improvement for click detection when clicking quickly between mouse moves (#5786)

* improvement for click detection when clicking quickly between mouse moves

Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com>

* remove magic number and introduce constant

Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com>

* add missing update from camera test

Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com>

Code/Editor/Lib/Tests/test_ModularViewportCameraController.cpp

@@ -389,6 +389,7 @@ namespace UnitTest
         m_controllerList->UpdateViewport({ TestViewportId, AzFramework::FloatSeconds(deltaTime), AZ::ScriptTimePoint() });
 
         QTest::mouseRelease(m_rootWidget.get(), Qt::MouseButton::RightButton, Qt::NoModifier, start + mouseDelta);
+        m_controllerList->UpdateViewport({ TestViewportId, AzFramework::FloatSeconds(deltaTime), AZ::ScriptTimePoint() });
 
         // update the position of the widget
         const auto offset = QPoint(500, 500);

Code/Editor/Lib/Tests/test_ViewportManipulatorController.cpp

@@ -12,6 +12,7 @@
 #include <AzToolsFramework/UnitTest/AzToolsFrameworkTestHelpers.h>
 #include <AzToolsFramework/ViewportSelection/EditorInteractionSystemViewportSelectionRequestBus.h>
 #include <Editor/ViewportManipulatorController.h>
+#include <Mocks/MockWindowRequests.h>
 
 namespace UnitTest
 {
@@ -77,14 +78,15 @@ namespace UnitTest
     class ViewportManipulatorControllerFixture : public AllocatorsTestFixture
     {
     public:
-        static const AzFramework::ViewportId TestViewportId;
+        static inline constexpr AzFramework::ViewportId TestViewportId = 1234;
+        static inline const QSize WidgetSize = QSize(1920, 1080);
 
         void SetUp() override
         {
             AllocatorsTestFixture::SetUp();
 
             m_rootWidget = AZStd::make_unique<QWidget>();
-            m_rootWidget->setFixedSize(QSize(100, 100));
+            m_rootWidget->setFixedSize(WidgetSize);
             QApplication::setActiveWindow(m_rootWidget.get());
 
             m_controllerList = AZStd::make_shared<AzFramework::ViewportControllerList>();
@@ -111,8 +113,6 @@ namespace UnitTest
         AZStd::unique_ptr<AzToolsFramework::QtEventToAzInputMapper> m_inputChannelMapper;
     };
 
-    const AzFramework::ViewportId ViewportManipulatorControllerFixture::TestViewportId = AzFramework::ViewportId(0);
-
     TEST_F(ViewportManipulatorControllerFixture, AnEventIsNotPropagatedToTheViewportWhenAManipulatorHandlesItFirst)
     {
         // forward input events to our controller list
@@ -227,4 +227,74 @@ namespace UnitTest
         // the key was released (cleared)
         EXPECT_TRUE(endedEvent);
     }
+
+    TEST_F(ViewportManipulatorControllerFixture, DoubleClickIsNotRegisteredIfMouseDeltaHasMovedMoreThanDeadzoneInClickInterval)
+    {
+        AzFramework::NativeWindowHandle nativeWindowHandle = nullptr;
+
+        // forward input events to our controller list
+        QObject::connect(
+            m_inputChannelMapper.get(), &AzToolsFramework::QtEventToAzInputMapper::InputChannelUpdated, m_rootWidget.get(),
+            [this, nativeWindowHandle](const AzFramework::InputChannel* inputChannel, [[maybe_unused]] QEvent* event)
+            {
+                m_controllerList->HandleInputChannelEvent(
+                    AzFramework::ViewportControllerInputEvent{ TestViewportId, nativeWindowHandle, *inputChannel });
+            });
+
+        ::testing::NiceMock<MockWindowRequests> mockWindowRequests;
+        mockWindowRequests.Connect(nativeWindowHandle);
+
+        using ::testing::Return;
+        // note: WindowRequests is used internally by ViewportManipulatorController
+        ON_CALL(mockWindowRequests, GetClientAreaSize())
+            .WillByDefault(Return(AzFramework::WindowSize(WidgetSize.width(), WidgetSize.height())));
+
+        EditorInteractionViewportSelectionFake editorInteractionViewportFake;
+        editorInteractionViewportFake.m_internalHandleMouseManipulatorInteraction = [](const MouseInteractionEvent&)
+        {
+            // report the event was not handled (manipulator was not interacted with)
+            return false;
+        };
+
+        bool doubleClickDetected = false;
+        editorInteractionViewportFake.m_internalHandleMouseViewportInteraction =
+            [&doubleClickDetected](const MouseInteractionEvent& mouseInteractionEvent)
+        {
+            // ensure no double click event is detected with the given inputs below
+            if (mouseInteractionEvent.m_mouseEvent == AzToolsFramework::ViewportInteraction::MouseEvent::DoubleClick)
+            {
+                doubleClickDetected = true;
+            }
+
+            return true;
+        };
+
+        editorInteractionViewportFake.Connect();
+
+        m_controllerList->Add(AZStd::make_shared<SandboxEditor::ViewportManipulatorController>());
+
+        // simulate a click, move, click
+        MouseMove(m_rootWidget.get(), QPoint(0, 0), QPoint(10, 10));
+        MousePressAndMove(m_rootWidget.get(), QPoint(10, 10), QPoint(0, 0), Qt::MouseButton::LeftButton);
+        QTest::mouseRelease(m_rootWidget.get(), Qt::MouseButton::LeftButton, Qt::KeyboardModifier::NoModifier, QPoint(10, 10));
+        MouseMove(m_rootWidget.get(), QPoint(10, 10), QPoint(20, 20));
+        MousePressAndMove(m_rootWidget.get(), QPoint(20, 20), QPoint(0, 0), Qt::MouseButton::LeftButton);
+        QTest::mouseRelease(m_rootWidget.get(), Qt::MouseButton::LeftButton, Qt::KeyboardModifier::NoModifier, QPoint(20, 20));
+
+        // ensure no double click was detected
+        EXPECT_FALSE(doubleClickDetected);
+
+        // simulate double click (sanity check it still is detected correctly with no movement)
+        MouseMove(m_rootWidget.get(), QPoint(0, 0), QPoint(10, 10));
+        MousePressAndMove(m_rootWidget.get(), QPoint(10, 10), QPoint(0, 0), Qt::MouseButton::LeftButton);
+        QTest::mouseRelease(m_rootWidget.get(), Qt::MouseButton::LeftButton, Qt::KeyboardModifier::NoModifier, QPoint(10, 10));
+        MousePressAndMove(m_rootWidget.get(), QPoint(10, 10), QPoint(0, 0), Qt::MouseButton::LeftButton);
+        QTest::mouseRelease(m_rootWidget.get(), Qt::MouseButton::LeftButton, Qt::KeyboardModifier::NoModifier, QPoint(10, 10));
+
+        // ensure a double click was detected
+        EXPECT_TRUE(doubleClickDetected);
+
+        mockWindowRequests.Disconnect();
+        editorInteractionViewportFake.Disconnect();
+    }
 } // namespace UnitTest

Code/Editor/ViewportManipulatorController.cpp

@@ -157,7 +157,7 @@ namespace SandboxEditor
                     // Only insert the double click timing once we're done processing events, to avoid a false IsDoubleClick positive
                     if (finishedProcessingEvents)
                     {
-                        m_pendingDoubleClicks[mouseButton] = m_curTime;
+                        m_pendingDoubleClicks[mouseButton] = { m_currentTime, m_mouseInteraction.m_mousePick.m_screenCoordinates };
                     }
                     eventType = MouseEvent::Down;
                 }
@@ -251,17 +251,22 @@ namespace SandboxEditor
 
     void ViewportManipulatorControllerInstance::UpdateViewport(const AzFramework::ViewportControllerUpdateEvent& event)
     {
-        m_curTime = event.m_time;
+        m_currentTime = event.m_time;
     }
 
     bool ViewportManipulatorControllerInstance::IsDoubleClick(AzToolsFramework::ViewportInteraction::MouseButton button) const
     {
-        auto clickIt = m_pendingDoubleClicks.find(button);
-        if (clickIt == m_pendingDoubleClicks.end())
+        if (auto clickIt = m_pendingDoubleClicks.find(button); clickIt != m_pendingDoubleClicks.end())
         {
-            return false;
+            const double doubleClickThresholdMilliseconds = qApp->doubleClickInterval();
+            const bool insideTimeThreshold =
+                (m_currentTime.GetMilliseconds() - clickIt->second.m_time.GetMilliseconds()) < doubleClickThresholdMilliseconds;
+            const bool insideDistanceThreshold =
+                AzFramework::ScreenVectorLength(clickIt->second.m_position - m_mouseInteraction.m_mousePick.m_screenCoordinates) <
+                AzFramework::DefaultMouseMoveDeadZone;
+            return insideTimeThreshold && insideDistanceThreshold;
         }
-        const double doubleClickThresholdMilliseconds = qApp->doubleClickInterval();
-        return (m_curTime.GetMilliseconds() - clickIt->second.GetMilliseconds()) < doubleClickThresholdMilliseconds;
+
+        return false;
     }
 } // namespace SandboxEditor

Code/Editor/ViewportManipulatorController.h

@@ -39,8 +39,16 @@ namespace SandboxEditor
         static bool IsMouseMove(const AzFramework::InputChannel& inputChannel);
         static AzToolsFramework::ViewportInteraction::KeyboardModifier GetKeyboardModifier(const AzFramework::InputChannel& inputChannel);
 
+        //! Represents the time and location of a click.
+        struct ClickEvent
+        {
+            AZ::ScriptTimePoint m_time;
+            AzFramework::ScreenPoint m_position;
+        };
+
         AzToolsFramework::ViewportInteraction::MouseInteraction m_mouseInteraction;
-        AZStd::unordered_map<AzToolsFramework::ViewportInteraction::MouseButton, AZ::ScriptTimePoint> m_pendingDoubleClicks;
-        AZ::ScriptTimePoint m_curTime;
+        AZStd::unordered_map<AzToolsFramework::ViewportInteraction::MouseButton, ClickEvent> m_pendingDoubleClicks;
+
+        AZ::ScriptTimePoint m_currentTime;
     };
 } // namespace SandboxEditor

Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.cpp

@@ -24,11 +24,12 @@ namespace AzFramework
 
     ClickDetector::ClickOutcome ClickDetector::DetectClick(const ClickEvent clickEvent, const ScreenVector& cursorDelta)
     {
+        m_moveAccumulator += ScreenVectorLength(cursorDelta);
+
         const auto previousDetectionState = m_detectionState;
         if (previousDetectionState == DetectionState::WaitingForMove)
         {
             // only allow the action to begin if the mouse has been moved a small amount
-            m_moveAccumulator += ScreenVectorLength(cursorDelta);
             if (m_moveAccumulator > m_deadZone)
             {
                 m_detectionState = DetectionState::Moved;
@@ -43,7 +44,7 @@ namespace AzFramework
                 using FloatingPointSeconds = AZStd::chrono::duration<float, AZStd::chrono::seconds::period>;
 
                 const auto diff = now - m_tryBeginTime.value();
-                if (FloatingPointSeconds(diff).count() < m_doubleClickInterval)
+                if (FloatingPointSeconds(diff).count() < m_doubleClickInterval && m_moveAccumulator < m_deadZone)
                 {
                     return ClickOutcome::Nil;
                 }

Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.h

@@ -15,6 +15,10 @@
 
 namespace AzFramework
 {
+    //! Default value to use for detecting if the mouse has moved far enough after a mouse down to no longer
+    //! register a click when a mouse up occurs.
+    inline constexpr float DefaultMouseMoveDeadZone = 2.0f;
+
     struct ScreenVector;
 
     //! Utility class to help detect different types of mouse click (mouse down and up with
@@ -66,7 +70,7 @@ namespace AzFramework
         };
 
         float m_moveAccumulator = 0.0f; //!< How far the mouse has moved after mouse down.
-        float m_deadZone = 2.0f; //!< How far to move before a click is cancelled (when Move will fire).
+        float m_deadZone = DefaultMouseMoveDeadZone; //!< How far to move before a click is cancelled (when Move will fire).
         float m_doubleClickInterval = 0.4f; //!< Default double click interval, can be overridden.
         DetectionState m_detectionState; //!< Internal state of ClickDetector.
         //! Mouse down time (happens each mouse down, helps with double click handling).

Code/Framework/AzFramework/Tests/ClickDetectorTests.cpp

@@ -144,12 +144,45 @@ namespace UnitTest
     {
         using ::testing::Eq;
 
-        const ClickDetector::ClickOutcome downOutcome =
-            m_clickDetector.DetectClick(ClickDetector::ClickEvent::Down, ScreenVector(0, 0));
-        const ClickDetector::ClickOutcome upOutcome =
-            m_clickDetector.DetectClick(ClickDetector::ClickEvent::Up, ScreenVector(50, 50));
+        const ClickDetector::ClickOutcome downOutcome = m_clickDetector.DetectClick(ClickDetector::ClickEvent::Down, ScreenVector(0, 0));
+        const ClickDetector::ClickOutcome upOutcome = m_clickDetector.DetectClick(ClickDetector::ClickEvent::Up, ScreenVector(50, 50));
 
         EXPECT_THAT(downOutcome, Eq(ClickDetector::ClickOutcome::Nil));
         EXPECT_THAT(upOutcome, Eq(ClickDetector::ClickOutcome::Release));
     }
+
+    //! note: ClickDetector does not explicitly return double clicks but if one occurs the ClickOutcome will be Nil
+    TEST_F(ClickDetectorFixture, DoubleClickIsRegisteredIfMouseDeltaHasMovedLessThanDeadzoneInClickInterval)
+    {
+        using ::testing::Eq;
+
+        const ClickDetector::ClickOutcome firstDownOutcome =
+            m_clickDetector.DetectClick(ClickDetector::ClickEvent::Down, ScreenVector(0, 0));
+        const ClickDetector::ClickOutcome firstUpOutcome = m_clickDetector.DetectClick(ClickDetector::ClickEvent::Up, ScreenVector(0, 0));
+        const ClickDetector::ClickOutcome secondDownOutcome =
+            m_clickDetector.DetectClick(ClickDetector::ClickEvent::Down, ScreenVector(0, 0));
+        const ClickDetector::ClickOutcome secondUpOutcome = m_clickDetector.DetectClick(ClickDetector::ClickEvent::Up, ScreenVector(0, 0));
+
+        EXPECT_THAT(firstDownOutcome, Eq(ClickDetector::ClickOutcome::Nil));
+        EXPECT_THAT(firstUpOutcome, Eq(ClickDetector::ClickOutcome::Click));
+        EXPECT_THAT(secondDownOutcome, Eq(ClickDetector::ClickOutcome::Nil));
+        EXPECT_THAT(secondUpOutcome, Eq(ClickDetector::ClickOutcome::Nil));
+    }
+
+    TEST_F(ClickDetectorFixture, DoubleClickIsNotRegisteredIfMouseDeltaHasMovedMoreThanDeadzoneInClickInterval)
+    {
+        using ::testing::Eq;
+
+        const ClickDetector::ClickOutcome firstDownOutcome =
+            m_clickDetector.DetectClick(ClickDetector::ClickEvent::Down, ScreenVector(0, 0));
+        const ClickDetector::ClickOutcome firstUpOutcome = m_clickDetector.DetectClick(ClickDetector::ClickEvent::Up, ScreenVector(0, 0));
+        const ClickDetector::ClickOutcome secondDownOutcome =
+            m_clickDetector.DetectClick(ClickDetector::ClickEvent::Down, ScreenVector(10, 10));
+        const ClickDetector::ClickOutcome secondUpOutcome = m_clickDetector.DetectClick(ClickDetector::ClickEvent::Up, ScreenVector(0, 0));
+
+        EXPECT_THAT(firstDownOutcome, Eq(ClickDetector::ClickOutcome::Nil));
+        EXPECT_THAT(firstUpOutcome, Eq(ClickDetector::ClickOutcome::Click));
+        EXPECT_THAT(secondDownOutcome, Eq(ClickDetector::ClickOutcome::Nil));
+        EXPECT_THAT(secondUpOutcome, Eq(ClickDetector::ClickOutcome::Click));
+    }
 } // namespace UnitTest