Camera fixes follow-up (#5703)

* allow unconstrained camera when tracking transform and fix some camera interpolation issues

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

* tests for interpolation fixes

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

* add test for camera constraints change

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

* updates following review feeedback

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

Code/Editor/EditorModularViewportCameraComposer.cpp

@@ -145,6 +145,15 @@ namespace SandboxEditor
             }
         };
 
+        const auto trackingTransform = [viewportId = m_viewportId]
+        {
+            bool tracking = false;
+            AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+                tracking, viewportId, &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::IsTrackingTransform);
+
+            return tracking;
+        };
+
         m_firstPersonRotateCamera = AZStd::make_shared<AzFramework::RotateCameraInput>(SandboxEditor::CameraFreeLookChannelId());
 
         m_firstPersonRotateCamera->m_rotateSpeedFn = []
@@ -152,6 +161,11 @@ namespace SandboxEditor
             return SandboxEditor::CameraRotateSpeed();
         };
 
+        m_firstPersonRotateCamera->m_constrainPitch = [trackingTransform]
+        {
+            return !trackingTransform();
+        };
+
         // default behavior is to hide the cursor but this can be disabled (useful for remote desktop)
         // note: See CaptureCursorLook in the Settings Registry
         m_firstPersonRotateCamera->SetActivationBeganFn(hideCursor);
@@ -255,6 +269,11 @@ namespace SandboxEditor
             return SandboxEditor::CameraOrbitYawRotationInverted();
         };
 
+        m_orbitRotateCamera->m_constrainPitch = [trackingTransform]
+        {
+            return !trackingTransform();
+        };
+
         m_orbitTranslateCamera = AZStd::make_shared<AzFramework::TranslateCameraInput>(
             translateCameraInputChannelIds, AzFramework::LookTranslation, AzFramework::TranslateOffsetOrbit);
 

Code/Editor/Lib/Tests/Camera/test_EditorCamera.cpp

@@ -38,7 +38,9 @@ namespace UnitTest
         AzFramework::ViewportControllerListPtr m_controllerList;
         AZStd::unique_ptr<AZ::Entity> m_entity;
 
-        static const AzFramework::ViewportId TestViewportId;
+        static inline constexpr AzFramework::ViewportId TestViewportId = 2345;
+        static inline constexpr float HalfInterpolateToTransformDuration =
+            AtomToolsFramework::ModularViewportCameraControllerRequests::InterpolateToTransformDuration * 0.5f;
 
         void SetUp() override
         {
@@ -77,8 +79,6 @@ namespace UnitTest
         }
     };
 
-    const AzFramework::ViewportId EditorCameraFixture::TestViewportId = AzFramework::ViewportId(1337);
-
     TEST_F(EditorCameraFixture, ModularViewportCameraControllerReferenceFrameUpdatedWhenViewportEntityisChanged)
     {
         // Given
@@ -149,8 +149,10 @@ namespace UnitTest
             transformToInterpolateTo);
 
         // simulate interpolation
-        m_controllerList->UpdateViewport({ TestViewportId, AzFramework::FloatSeconds(0.5f), AZ::ScriptTimePoint() });
-        m_controllerList->UpdateViewport({ TestViewportId, AzFramework::FloatSeconds(0.5f), AZ::ScriptTimePoint() });
+        m_controllerList->UpdateViewport(
+            { TestViewportId, AzFramework::FloatSeconds(HalfInterpolateToTransformDuration), AZ::ScriptTimePoint() });
+        m_controllerList->UpdateViewport(
+            { TestViewportId, AzFramework::FloatSeconds(HalfInterpolateToTransformDuration), AZ::ScriptTimePoint() });
 
         const auto finalTransform = m_cameraViewportContextView->GetCameraTransform();
 
@@ -175,14 +177,87 @@ namespace UnitTest
             transformToInterpolateTo);
 
         // simulate interpolation
-        m_controllerList->UpdateViewport({ TestViewportId, AzFramework::FloatSeconds(0.5f), AZ::ScriptTimePoint() });
-        m_controllerList->UpdateViewport({ TestViewportId, AzFramework::FloatSeconds(0.5f), AZ::ScriptTimePoint() });
+        m_controllerList->UpdateViewport(
+            { TestViewportId, AzFramework::FloatSeconds(HalfInterpolateToTransformDuration), AZ::ScriptTimePoint() });
+        m_controllerList->UpdateViewport(
+            { TestViewportId, AzFramework::FloatSeconds(HalfInterpolateToTransformDuration), AZ::ScriptTimePoint() });
 
         const auto finalTransform = m_cameraViewportContextView->GetCameraTransform();
 
         // Then
         EXPECT_THAT(finalTransform, IsClose(transformToInterpolateTo));
     }
+
+    TEST_F(EditorCameraFixture, BeginningCameraInterpolationReturnsTrue)
+    {
+        // Given/When
+        bool interpolationBegan = false;
+        AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+            interpolationBegan, TestViewportId,
+            &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::InterpolateToTransform,
+            AZ::Transform::CreateTranslation(AZ::Vector3(10.0f, 10.0f, 10.0f)));
+
+        // Then
+        EXPECT_THAT(interpolationBegan, ::testing::IsTrue());
+    }
+
+    TEST_F(EditorCameraFixture, CameraInterpolationDoesNotBeginDuringAnExistingInterpolation)
+    {
+        // Given/When
+        bool initialInterpolationBegan = false;
+        AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+            initialInterpolationBegan, TestViewportId,
+            &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::InterpolateToTransform,
+            AZ::Transform::CreateTranslation(AZ::Vector3(10.0f, 10.0f, 10.0f)));
+
+        m_controllerList->UpdateViewport(
+            { TestViewportId, AzFramework::FloatSeconds(HalfInterpolateToTransformDuration), AZ::ScriptTimePoint() });
+
+        bool nextInterpolationBegan = true;
+        AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+            nextInterpolationBegan, TestViewportId,
+            &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::InterpolateToTransform,
+            AZ::Transform::CreateTranslation(AZ::Vector3(10.0f, 10.0f, 10.0f)));
+
+        bool interpolating = false;
+        AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+            interpolating, TestViewportId, &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::IsInterpolating);
+
+        // Then
+        EXPECT_THAT(initialInterpolationBegan, ::testing::IsTrue());
+        EXPECT_THAT(nextInterpolationBegan, ::testing::IsFalse());
+        EXPECT_THAT(interpolating, ::testing::IsTrue());
+    }
+
+    TEST_F(EditorCameraFixture, CameraInterpolationCanBeginAfterAnInterpolationCompletes)
+    {
+        // Given/When
+        bool initialInterpolationBegan = false;
+        AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+            initialInterpolationBegan, TestViewportId,
+            &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::InterpolateToTransform,
+            AZ::Transform::CreateTranslation(AZ::Vector3(10.0f, 10.0f, 10.0f)));
+
+        m_controllerList->UpdateViewport(
+            { TestViewportId,
+              AzFramework::FloatSeconds(AtomToolsFramework::ModularViewportCameraControllerRequests::InterpolateToTransformDuration + 0.5f),
+              AZ::ScriptTimePoint() });
+
+        bool interpolating = true;
+        AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+            interpolating, TestViewportId, &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::IsInterpolating);
+
+        bool nextInterpolationBegan = false;
+        AtomToolsFramework::ModularViewportCameraControllerRequestBus::EventResult(
+            nextInterpolationBegan, TestViewportId,
+            &AtomToolsFramework::ModularViewportCameraControllerRequestBus::Events::InterpolateToTransform,
+            AZ::Transform::CreateTranslation(AZ::Vector3(10.0f, 10.0f, 10.0f)));
+
+        // Then
+        EXPECT_THAT(initialInterpolationBegan, ::testing::IsTrue());
+        EXPECT_THAT(interpolating, ::testing::IsFalse());
+        EXPECT_THAT(nextInterpolationBegan, ::testing::IsTrue());
+    }
 } // namespace UnitTest
 
 // required to support running integration tests with the Camera Gem

Code/Editor/Lib/Tests/test_ModularViewportCameraController.cpp

@@ -74,7 +74,7 @@ namespace UnitTest
     class ModularViewportCameraControllerFixture : public AllocatorsTestFixture
     {
     public:
-        static const AzFramework::ViewportId TestViewportId;
+        static inline constexpr AzFramework::ViewportId TestViewportId = 1234;
 
         void SetUp() override
         {
@@ -220,8 +220,6 @@ namespace UnitTest
         AZStd::unique_ptr<SandboxEditor::EditorModularViewportCameraComposer> m_editorModularViewportCameraComposer;
     };
 
-    const AzFramework::ViewportId ModularViewportCameraControllerFixture::TestViewportId = AzFramework::ViewportId(0);
-
     TEST_F(ModularViewportCameraControllerFixture, MouseMovementDoesNotAccumulateExcessiveDriftInModularViewportCameraWithVaryingDeltaTime)
     {
         SandboxEditor::SetCameraCaptureCursorForLook(false);

Code/Editor/Plugins/ComponentEntityEditorPlugin/SandboxIntegration.cpp

@@ -1675,6 +1675,12 @@ void SandboxIntegrationManager::GoToEntitiesInViewports(const AzToolsFramework::
         if (auto viewportContext = viewportContextManager->GetViewportContextById(viewIndex))
         {
             const AZ::Transform cameraTransform = viewportContext->GetCameraTransform();
+            // do not attempt to interpolate to where we currently are
+            if (cameraTransform.GetTranslation().IsClose(center))
+            {
+                continue;
+            }
+
             const AZ::Vector3 forward = (center - cameraTransform.GetTranslation()).GetNormalized();
 
             // move camera 25% further back than required

Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.cpp

@@ -313,6 +313,11 @@ namespace AzFramework
         {
             return false;
         };
+
+        m_constrainPitch = []() constexpr
+        {
+            return true;
+        };
     }
 
     bool RotateCameraInput::HandleEvents(const InputEvent& event, const ScreenVector& cursorDelta, [[maybe_unused]] const float scrollDelta)
@@ -334,7 +339,10 @@ namespace AzFramework
         nextCamera.m_yaw -= float(cursorDelta.m_x) * rotateSpeed * Invert(m_invertYawFn());
 
         nextCamera.m_yaw = WrapYawRotation(nextCamera.m_yaw);
+        if (m_constrainPitch())
+        {
             nextCamera.m_pitch = ClampPitchRotation(nextCamera.m_pitch);
+        }
 
         return nextCamera;
     }
@@ -748,14 +756,14 @@ namespace AzFramework
 
     Camera SmoothCamera(const Camera& currentCamera, const Camera& targetCamera, const CameraProps& cameraProps, const float deltaTime)
     {
-        const auto clamp_rotation = [](const float angle)
+        const auto clampRotation = [](const float angle)
         {
             return AZStd::fmod(angle + AZ::Constants::TwoPi, AZ::Constants::TwoPi);
         };
 
         // keep yaw in 0 - 360 range
-        float targetYaw = clamp_rotation(targetCamera.m_yaw);
-        const float currentYaw = clamp_rotation(currentCamera.m_yaw);
+        float targetYaw = clampRotation(targetCamera.m_yaw);
+        const float currentYaw = clampRotation(currentCamera.m_yaw);
 
         // return the sign of the float input (-1, 0, 1)
         const auto sign = [](const float value)
@@ -764,8 +772,7 @@ namespace AzFramework
         };
 
         // ensure smooth transition when moving across 0 - 360 boundary
-        const float yawDelta = targetYaw - currentYaw;
-        if (AZStd::abs(yawDelta) >= AZ::Constants::Pi)
+        if (const float yawDelta = targetYaw - currentYaw; AZStd::abs(yawDelta) >= AZ::Constants::Pi)
         {
             targetYaw -= AZ::Constants::TwoPi * sign(yawDelta);
         }

Code/Framework/AzFramework/AzFramework/Viewport/CameraInput.h

@@ -347,6 +347,7 @@ namespace AzFramework
         AZStd::function<float()> m_rotateSpeedFn;
         AZStd::function<bool()> m_invertPitchFn;
         AZStd::function<bool()> m_invertYawFn;
+        AZStd::function<bool()> m_constrainPitch;
 
     private:
         InputChannelId m_rotateChannelId; //!< Input channel to begin the rotate camera input.

Code/Framework/AzFramework/Tests/CameraInputTests.cpp

@@ -104,9 +104,10 @@ namespace UnitTest
         AZStd::shared_ptr<AzFramework::OrbitCameraInput> m_orbitCamera;
         AZ::Vector3 m_pivot = AZ::Vector3::CreateZero();
 
-        //! This is approximately Pi/2 * 1000 - this can be used to rotate the camera 90 degrees (pitch or yaw based
-        //! on vertical or horizontal motion) as the rotate speed function is set to be 1/1000.
-        inline static const int PixelMotionDelta = 1570;
+        // this is approximately Pi/2 * 1000 - this can be used to rotate the camera 90 degrees (pitch or yaw based
+        // on vertical or horizontal motion) as the rotate speed function is set to be 1/1000.
+        inline static const int PixelMotionDelta90Degrees = 1570;
+        inline static const int PixelMotionDelta135Degrees = 2356;
     };
 
     TEST_F(CameraInputFixture, BeginAndEndOrbitCameraInputConsumesCorrectEvents)
@@ -292,7 +293,7 @@ namespace UnitTest
 
         HandleEventAndUpdate(
             AzFramework::DiscreteInputEvent{ AzFramework::InputDeviceMouse::Button::Right, AzFramework::InputChannel::State::Began });
-        HandleEventAndUpdate(AzFramework::HorizontalMotionEvent{ PixelMotionDelta });
+        HandleEventAndUpdate(AzFramework::HorizontalMotionEvent{ PixelMotionDelta90Degrees });
 
         const float expectedYaw = AzFramework::WrapYawRotation(-AZ::Constants::HalfPi);
 
@@ -310,7 +311,7 @@ namespace UnitTest
 
         HandleEventAndUpdate(
             AzFramework::DiscreteInputEvent{ AzFramework::InputDeviceMouse::Button::Right, AzFramework::InputChannel::State::Began });
-        HandleEventAndUpdate(AzFramework::VerticalMotionEvent{ PixelMotionDelta });
+        HandleEventAndUpdate(AzFramework::VerticalMotionEvent{ PixelMotionDelta90Degrees });
 
         const float expectedPitch = AzFramework::ClampPitchRotation(-AZ::Constants::HalfPi);
 
@@ -331,7 +332,7 @@ namespace UnitTest
         HandleEventAndUpdate(AzFramework::DiscreteInputEvent{ m_orbitChannelId, AzFramework::InputChannel::State::Began });
         HandleEventAndUpdate(
             AzFramework::DiscreteInputEvent{ AzFramework::InputDeviceMouse::Button::Left, AzFramework::InputChannel::State::Began });
-        HandleEventAndUpdate(AzFramework::VerticalMotionEvent{ PixelMotionDelta });
+        HandleEventAndUpdate(AzFramework::VerticalMotionEvent{ PixelMotionDelta90Degrees });
 
         const auto expectedCameraEndingPosition = AZ::Vector3(0.0f, -10.0f, 10.0f);
         const float expectedPitch = AzFramework::ClampPitchRotation(-AZ::Constants::HalfPi);
@@ -354,7 +355,7 @@ namespace UnitTest
         HandleEventAndUpdate(AzFramework::DiscreteInputEvent{ m_orbitChannelId, AzFramework::InputChannel::State::Began });
         HandleEventAndUpdate(
             AzFramework::DiscreteInputEvent{ AzFramework::InputDeviceMouse::Button::Left, AzFramework::InputChannel::State::Began });
-        HandleEventAndUpdate(AzFramework::HorizontalMotionEvent{ -PixelMotionDelta });
+        HandleEventAndUpdate(AzFramework::HorizontalMotionEvent{ -PixelMotionDelta90Degrees });
 
         const auto expectedCameraEndingPosition = AZ::Vector3(20.0f, -5.0f, 0.0f);
         const float expectedYaw = AzFramework::WrapYawRotation(AZ::Constants::HalfPi);
@@ -366,4 +367,42 @@ namespace UnitTest
         EXPECT_THAT(m_camera.m_offset, IsClose(AZ::Vector3(5.0f, -10.0f, 0.0f)));
         EXPECT_THAT(m_camera.Translation(), IsCloseTolerance(expectedCameraEndingPosition, 0.01f));
     }
+
+    TEST_F(CameraInputFixture, CameraPitchCanNotBeMovedPastNinetyDegreesWhenConstrained)
+    {
+        const auto cameraStartingPosition = AZ::Vector3(15.0f, -20.0f, 0.0f);
+        m_targetCamera.m_pivot = cameraStartingPosition;
+
+        HandleEventAndUpdate(
+            AzFramework::DiscreteInputEvent{ AzFramework::InputDeviceMouse::Button::Right, AzFramework::InputChannel::State::Began });
+        // pitch by 135.0 degrees
+        HandleEventAndUpdate(AzFramework::VerticalMotionEvent{ -PixelMotionDelta135Degrees });
+
+        // clamped to 90.0 degrees
+        const float expectedPitch = AZ::DegToRad(90.0f);
+
+        using ::testing::FloatNear;
+        EXPECT_THAT(m_camera.m_pitch, FloatNear(expectedPitch, 0.001f));
+    }
+
+    TEST_F(CameraInputFixture, CameraPitchCanBeMovedPastNinetyDegreesWhenUnconstrained)
+    {
+        m_firstPersonRotateCamera->m_constrainPitch = []
+        {
+            return false;
+        };
+
+        const auto cameraStartingPosition = AZ::Vector3(15.0f, -20.0f, 0.0f);
+        m_targetCamera.m_pivot = cameraStartingPosition;
+
+        HandleEventAndUpdate(
+            AzFramework::DiscreteInputEvent{ AzFramework::InputDeviceMouse::Button::Right, AzFramework::InputChannel::State::Began });
+        // pitch by 135.0 degrees
+        HandleEventAndUpdate(AzFramework::VerticalMotionEvent{ -PixelMotionDelta135Degrees });
+
+        const float expectedPitch = AZ::DegToRad(135.0f);
+
+        using ::testing::FloatNear;
+        EXPECT_THAT(m_camera.m_pitch, FloatNear(expectedPitch, 0.001f));
+    }
 } // namespace UnitTest

Gems/Atom/Tools/AtomToolsFramework/Code/Include/AtomToolsFramework/Viewport/ModularViewportCameraController.h

@@ -112,7 +112,8 @@ namespace AtomToolsFramework
         void UpdateViewport(const AzFramework::ViewportControllerUpdateEvent& event) override;
 
         // ModularViewportCameraControllerRequestBus overrides ...
-        void InterpolateToTransform(const AZ::Transform& worldFromLocal) override;
+        bool InterpolateToTransform(const AZ::Transform& worldFromLocal) override;
+        bool IsInterpolating() const override;
         void StartTrackingTransform(const AZ::Transform& worldFromLocal) override;
         void StopTrackingTransform() override;
         bool IsTrackingTransform() const override;

Gems/Atom/Tools/AtomToolsFramework/Code/Include/AtomToolsFramework/Viewport/ModularViewportCameraControllerRequestBus.h

@@ -23,13 +23,20 @@ namespace AtomToolsFramework
     class ModularViewportCameraControllerRequests : public AZ::EBusTraits
     {
     public:
+        static inline constexpr float InterpolateToTransformDuration = 1.0f;
+
         using BusIdType = AzFramework::ViewportId;
         static const AZ::EBusAddressPolicy AddressPolicy = AZ::EBusAddressPolicy::ById;
         static const AZ::EBusHandlerPolicy HandlerPolicy = AZ::EBusHandlerPolicy::Single;
 
         //! Begin a smooth transition of the camera to the requested transform.
         //! @param worldFromLocal The transform of where the camera should end up.
-        virtual void InterpolateToTransform(const AZ::Transform& worldFromLocal) = 0;
+        //! @return Returns true if the call began an interpolation and false otherwise. Calls to InterpolateToTransform
+        //! will have no effect if an interpolation is currently in progress.
+        virtual bool InterpolateToTransform(const AZ::Transform& worldFromLocal) = 0;
+
+        //! Returns if the camera is currently interpolating to a new transform.
+        virtual bool IsInterpolating() const = 0;
 
         //! Start tracking a transform.
         //! Store the current camera transform and move to the next camera transform.

Gems/Atom/Tools/AtomToolsFramework/Code/Source/Viewport/ModularViewportCameraController.cpp

@@ -235,7 +235,10 @@ namespace AtomToolsFramework
                 return t * t * t * (t * (t * 6.0f - 15.0f) + 10.0f);
             };
 
-            m_cameraAnimation.m_time = AZ::GetClamp(m_cameraAnimation.m_time + event.m_deltaTime.count(), 0.0f, 1.0f);
+            m_cameraAnimation.m_time = AZ::GetClamp(
+                m_cameraAnimation.m_time +
+                    (event.m_deltaTime.count() / ModularViewportCameraControllerRequests::InterpolateToTransformDuration),
+                0.0f, 1.0f);
 
             const auto& [transformStart, transformEnd, animationTime] = m_cameraAnimation;
 
@@ -263,10 +266,22 @@ namespace AtomToolsFramework
         m_updatingTransformInternally = false;
     }
 
-    void ModularViewportCameraControllerInstance::InterpolateToTransform(const AZ::Transform& worldFromLocal)
+    bool ModularViewportCameraControllerInstance::InterpolateToTransform(const AZ::Transform& worldFromLocal)
+    {
+        if (!IsInterpolating())
         {
             m_cameraMode = CameraMode::Animation;
             m_cameraAnimation = CameraAnimation{ CombinedCameraTransform(), worldFromLocal, 0.0f };
+
+            return true;
+        }
+
+        return false;
+    }
+
+    bool ModularViewportCameraControllerInstance::IsInterpolating() const
+    {
+        return m_cameraMode == CameraMode::Animation;
     }
 
     void ModularViewportCameraControllerInstance::StartTrackingTransform(const AZ::Transform& worldFromLocal)