From 795aa114e69f6846133442da9d74cae56416ca39 Mon Sep 17 00:00:00 2001 From: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> Date: Thu, 13 May 2021 16:19:53 +0100 Subject: [PATCH] Improve selection in the viewport (#720) * improve selection in the viewport * remove debug code * updates following review feedback - update API comments from /// to //! from - add [[nodiscard]] attribute to member function - move constructor implementations to .cpp files * use lambda instead of ternary operator * fix unit test failure caused by typo --- .../AzFramework/Viewport/CameraInput.cpp | 95 ++++++----- .../AzFramework/Viewport/CameraInput.h | 29 ++-- .../AzFramework/Viewport/ClickDetector.cpp | 68 ++++++++ .../AzFramework/Viewport/ClickDetector.h | 75 +++++++++ .../AzFramework/Viewport/CursorState.h | 56 +++++++ .../AzFramework/azframework_files.cmake | 3 + .../EditorTransformComponentSelection.cpp | 36 +++- .../EditorTransformComponentSelection.h | 156 +++++++++--------- Code/Framework/Tests/ClickDetectorTests.cpp | 142 ++++++++++++++++ Code/Framework/Tests/CursorStateTests.cpp | 54 ++++++ .../Tests/frameworktests_files.cmake | 2 + 11 files changed, 573 insertions(+), 143 deletions(-) create mode 100644 Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.cpp create mode 100644 Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.h create mode 100644 Code/Framework/AzFramework/AzFramework/Viewport/CursorState.h create mode 100644 Code/Framework/Tests/ClickDetectorTests.cpp create mode 100644 Code/Framework/Tests/CursorStateTests.cpp diff --git a/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.cpp b/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.cpp index 8669b58911..daf2c63921 100644 --- a/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.cpp +++ b/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -156,35 +157,25 @@ namespace AzFramework camera.m_lookAt = transform.GetTranslation() + (camera.Rotation().GetBasisY() * -camera.m_lookDist); } - static ScreenVector CursorDelta(const AZStd::optional& currentPosition, const AZStd::optional& lastPosition) - { - return currentPosition.has_value() && lastPosition.has_value() ? currentPosition.value() - lastPosition.value() - : ScreenVector(0, 0); - } - bool CameraSystem::HandleEvents(const InputEvent& event) { if (const auto& cursor = AZStd::get_if(&event)) { - m_currentCursorPosition = cursor->m_position; + m_cursorState.SetCurrentPosition(cursor->m_position); } else if (const auto& scroll = AZStd::get_if(&event)) { m_scrollDelta = scroll->m_delta; } - return m_cameras.HandleEvents(event, CursorDelta(m_currentCursorPosition, m_lastCursorPosition), m_scrollDelta); + return m_cameras.HandleEvents(event, m_cursorState.CursorDelta(), m_scrollDelta); } Camera CameraSystem::StepCamera(const Camera& targetCamera, const float deltaTime) { - const auto cursorDelta = CursorDelta(m_currentCursorPosition, m_lastCursorPosition); - if (m_currentCursorPosition.has_value()) - { - m_lastCursorPosition = m_currentCursorPosition; - } + const auto nextCamera = m_cameras.StepCamera(targetCamera, m_cursorState.CursorDelta(), m_scrollDelta, deltaTime); - const auto nextCamera = m_cameras.StepCamera(targetCamera, cursorDelta, m_scrollDelta, deltaTime); + m_cursorState.Update(); m_scrollDelta = 0.0f; @@ -236,12 +227,12 @@ namespace AzFramework } } - // accumulate - Camera nextCamera = targetCamera; - for (auto& cameraInput : m_activeCameraInputs) - { - nextCamera = cameraInput->StepCamera(nextCamera, cursorDelta, scrollDelta, deltaTime); - } + const Camera nextCamera = AZStd::accumulate( + AZStd::begin(m_activeCameraInputs), AZStd::end(m_activeCameraInputs), targetCamera, + [cursorDelta, scrollDelta, deltaTime](Camera acc, auto& camera) { + acc = camera->StepCamera(acc, cursorDelta, scrollDelta, deltaTime); + return acc; + }); for (int i = 0; i < m_activeCameraInputs.size();) { @@ -275,34 +266,42 @@ namespace AzFramework } } + RotateCameraInput::RotateCameraInput(const InputChannelId rotateChannelId) + : m_rotateChannelId(rotateChannelId) + { + } + void RotateCameraInput::HandleEvents(const InputEvent& event, const ScreenVector& cursorDelta, [[maybe_unused]] float scrollDelta) { - if (const auto& input = AZStd::get_if(&event)) - { - if (input->m_channelId == m_rotateChannelId) + const ClickDetector::ClickEvent clickEvent = [&event, this] { + if (const auto& input = AZStd::get_if(&event)) { - if (input->m_state == InputChannel::State::Began) + if (input->m_channelId == m_rotateChannelId) { - m_tryingToBegin = true; - m_moveAccumulator = 0.0f; - } - else if (input->m_state == InputChannel::State::Ended) - { - m_tryingToBegin = false; - EndActivation(); + if (input->m_state == InputChannel::State::Began) + { + return ClickDetector::ClickEvent::Down; + } + else if (input->m_state == InputChannel::State::Ended) + { + return ClickDetector::ClickEvent::Up; + } } } - } + return ClickDetector::ClickEvent::Nil; + }(); - if (m_tryingToBegin) + switch (const auto outcome = m_clickDetector.DetectClick(clickEvent, cursorDelta); outcome) { - // only allow the action to begin if the mouse has been moved a small amount - m_moveAccumulator += ScreenVectorLength(cursorDelta); - if (m_moveAccumulator > ed_cameraSystemLookDeadzone) - { - BeginActivation(); - m_tryingToBegin = false; - } + case ClickDetector::ClickOutcome::Move: + BeginActivation(); + break; + case ClickDetector::ClickOutcome::Release: + EndActivation(); + break; + default: + // noop + break; } } @@ -324,6 +323,12 @@ namespace AzFramework return nextCamera; } + PanCameraInput::PanCameraInput(const InputChannelId panChannelId, PanAxesFn panAxesFn) + : m_panAxesFn(AZStd::move(panAxesFn)) + , m_panChannelId(panChannelId) + { + } + void PanCameraInput::HandleEvents( const InputEvent& event, [[maybe_unused]] const ScreenVector& cursorDelta, [[maybe_unused]] float scrollDelta) { @@ -400,6 +405,11 @@ namespace AzFramework return TranslationType::Nil; } + TranslateCameraInput::TranslateCameraInput(TranslationAxesFn translationAxesFn) + : m_translationAxesFn(AZStd::move(translationAxesFn)) + { + } + void TranslateCameraInput::HandleEvents( const InputEvent& event, [[maybe_unused]] const ScreenVector& cursorDelta, [[maybe_unused]] float scrollDelta) { @@ -574,6 +584,11 @@ namespace AzFramework return nextCamera; } + OrbitDollyCursorMoveCameraInput::OrbitDollyCursorMoveCameraInput(const InputChannelId dollyChannelId) + : m_dollyChannelId(dollyChannelId) + { + } + void OrbitDollyCursorMoveCameraInput::HandleEvents( const InputEvent& event, [[maybe_unused]] const ScreenVector& cursorDelta, [[maybe_unused]] float scrollDelta) { diff --git a/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.h b/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.h index 6475753017..41d7f11385 100644 --- a/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.h +++ b/Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.h @@ -17,6 +17,8 @@ #include #include #include +#include +#include #include #include @@ -188,26 +190,21 @@ namespace AzFramework Cameras m_cameras; private: + CursorState m_cursorState; float m_scrollDelta = 0.0f; - AZStd::optional m_lastCursorPosition; - AZStd::optional m_currentCursorPosition; }; class RotateCameraInput : public CameraInput { public: - explicit RotateCameraInput(const InputChannelId rotateChannelId) - : m_rotateChannelId(rotateChannelId) - { - } + explicit RotateCameraInput(InputChannelId rotateChannelId); void HandleEvents(const InputEvent& event, const ScreenVector& cursorDelta, float scrollDelta) override; Camera StepCamera(const Camera& targetCamera, const ScreenVector& cursorDelta, float scrollDelta, float deltaTime) override; private: InputChannelId m_rotateChannelId; - float m_moveAccumulator = 0.0f; - bool m_tryingToBegin = false; + ClickDetector m_clickDetector; }; struct PanAxes @@ -240,11 +237,8 @@ namespace AzFramework class PanCameraInput : public CameraInput { public: - PanCameraInput(const InputChannelId panChannelId, PanAxesFn panAxesFn) - : m_panAxesFn(AZStd::move(panAxesFn)) - , m_panChannelId(panChannelId) - { - } + PanCameraInput(InputChannelId panChannelId, PanAxesFn panAxesFn); + void HandleEvents(const InputEvent& event, const ScreenVector& cursorDelta, float scrollDelta) override; Camera StepCamera(const Camera& targetCamera, const ScreenVector& cursorDelta, float scrollDelta, float deltaTime) override; @@ -283,10 +277,8 @@ namespace AzFramework class TranslateCameraInput : public CameraInput { public: - explicit TranslateCameraInput(TranslationAxesFn translationAxesFn) - : m_translationAxesFn(AZStd::move(translationAxesFn)) - { - } + explicit TranslateCameraInput(TranslationAxesFn translationAxesFn); + void HandleEvents(const InputEvent& event, const ScreenVector& cursorDelta, float scrollDelta) override; Camera StepCamera(const Camera& targetCamera, const ScreenVector& cursorDelta, float scrollDelta, float deltaTime) override; void ResetImpl() override; @@ -363,8 +355,7 @@ namespace AzFramework class OrbitDollyCursorMoveCameraInput : public CameraInput { public: - explicit OrbitDollyCursorMoveCameraInput(const InputChannelId dollyChannelId) - : m_dollyChannelId(dollyChannelId) {} + explicit OrbitDollyCursorMoveCameraInput(InputChannelId dollyChannelId); void HandleEvents(const InputEvent& event, const ScreenVector& cursorDelta, float scrollDelta) override; Camera StepCamera(const Camera& targetCamera, const ScreenVector& cursorDelta, float scrollDelta, float deltaTime) override; diff --git a/Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.cpp b/Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.cpp new file mode 100644 index 0000000000..5af44a81bc --- /dev/null +++ b/Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.cpp @@ -0,0 +1,68 @@ +/* + * All or portions of this file Copyright (c) Amazon.com, Inc. or its affiliates or + * its licensors. + * + * For complete copyright and license terms please see the LICENSE at the root of this + * distribution (the "License"). All use of this software is governed by the License, + * or, if provided, by the license below or the license accompanying this file. Do not + * remove or modify any license notices. This file is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * + */ + +#include +#include + +namespace AzFramework +{ + ClickDetector::ClickOutcome ClickDetector::DetectClick(const ClickEvent clickEvent, const ScreenVector& cursorDelta) + { + if (clickEvent == ClickEvent::Down) + { + const auto now = std::chrono::steady_clock::now(); + if (m_tryBeginTime) + { + const std::chrono::duration diff = now - m_tryBeginTime.value(); + if (diff.count() < m_doubleClickInterval) + { + return ClickOutcome::Nil; + } + } + + m_detectionState = DetectionState::WaitingForMove; + m_moveAccumulator = 0.0f; + + m_tryBeginTime = now; + } + else if (clickEvent == ClickEvent::Up) + { + const auto clickOutcome = [detectionState = m_detectionState] { + if (detectionState == DetectionState::WaitingForMove) + { + return ClickOutcome::Click; + } + if (detectionState == DetectionState::Moved) + { + return ClickOutcome::Release; + } + return ClickOutcome::Nil; + }(); + + m_detectionState = DetectionState::Nil; + return clickOutcome; + } + + if (m_detectionState == 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; + return ClickOutcome::Move; + } + } + + return ClickOutcome::Nil; + } +} // namespace AzFramework diff --git a/Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.h b/Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.h new file mode 100644 index 0000000000..997ccd07d9 --- /dev/null +++ b/Code/Framework/AzFramework/AzFramework/Viewport/ClickDetector.h @@ -0,0 +1,75 @@ +/* + * All or portions of this file Copyright (c) Amazon.com, Inc. or its affiliates or + * its licensors. + * + * For complete copyright and license terms please see the LICENSE at the root of this + * distribution (the "License"). All use of this software is governed by the License, + * or, if provided, by the license below or the license accompanying this file. Do not + * remove or modify any license notices. This file is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * + */ + +#pragma once + +#include + +#include + +namespace AzFramework +{ + struct ScreenVector; + + //! Utility class to help detect different types of mouse click (mouse down and up with + //! no movement), mouse move (down and initial move after some threshold) and mouse release + //! (mouse down with movement and then mouse up). + class ClickDetector + { + //! Alias for recording time of mouse down events + using Time = std::chrono::time_point; + + public: + //! Internal representation of click event (map from external event for this when + //! calling DetectClick). + enum class ClickEvent + { + Nil, + Down, + Up + }; + + //! The type of mouse click. + enum class ClickOutcome + { + Nil, //!< Not recognized. + Move, //!< Initial move after mouse down. + Click, //!< Mouse down and up with no intermediate movement. + Release //!< Mouse down with movement and then mouse up. + }; + + //! Called from any type of 'handle event' function. + ClickOutcome DetectClick(ClickEvent clickEvent, const ScreenVector& cursorDelta); + + void SetDoubleClickInterval(float doubleClickInterval); + + private: + //! Internal state of ClickDetector based on incoming events. + enum class DetectionState + { + Nil, //!< Initial state + WaitingForMove, //! Mouse down has happened but mouse hasn't yet moved. + Moved //! Mouse has moved, no longer will be counted as a click. + }; + + 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_doubleClickInterval = 0.4f; //!< Default double click interval, can be overridden. + DetectionState m_detectionState; //!< Internal state of ClickDetector. + AZStd::optional