Fixes for viewport selection issues (#5494)

* improvements to editor selection in the viewport Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * fix issue with being able to select icons that are not showing for entities inside entity containers Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * update comment after review feedback Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * updates to viewport picking code to simplify the api Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * add test to replicate near clip intersection issue Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * small tidy-up changes Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * updates to how we perform world to screen and screen to world calculations, added test coverage and some tidy-up Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * add some more tests for ViewportInteractionImpl Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * minor tweaks before PR feedback Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * fix typo in fix Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * fix for manipulator test framework tests Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * updates to RPI::View and RenderPipeline after review feedback from VickyAtAZ Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * add constexpr to ScreenPoint, ScreenVector and ScreenSize initializing constructors Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * add PrintTo functions for Screen* types Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * downgrade error to warning temporarily Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * check incoming view is null Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com> * remove pragma optimize off Signed-off-by: Tom Hulton-Harrop <82228511+hultonha@users.noreply.github.com>
2021-11-17 17:12:21 +00:00
parent ef16207ac5
commit e34ea3bcaa
45 changed files with 680 additions and 309 deletions
@@ -8,18 +8,18 @@

 #include "CameraState.h"

-#include <AzCore/Serialization/SerializeContext.h>
 #include <AzCore/Math/Matrix3x4.h>
 #include <AzCore/Math/Transform.h>
+#include <AzCore/Serialization/SerializeContext.h>

 namespace AzFramework
 {
    void SetCameraClippingVolume(
-        AzFramework::CameraState& cameraState, const float nearPlane, const float farPlane, const float fovRad)
+        AzFramework::CameraState& cameraState, const float nearPlane, const float farPlane, const float verticalFovRad)
    {
        cameraState.m_nearClip = nearPlane;
        cameraState.m_farClip = farPlane;
-        cameraState.m_fovOrZoom = fovRad;
+        cameraState.m_fovOrZoom = verticalFovRad;
    }

    void SetCameraTransform(CameraState& cameraState, const AZ::Transform& transform)
@@ -35,20 +35,34 @@ namespace AzFramework
        SetCameraClippingVolume(cameraState, 0.1f, 1000.0f, AZ::DegToRad(60.0f));
    }

-    AzFramework::CameraState CreateDefaultCamera(
-         const AZ::Transform& transform, const AZ::Vector2& viewportSize)
+    CameraState CreateCamera(
+        const AZ::Transform& transform,
+        const float nearPlane,
+        const float farPlane,
+        const float verticalFovRad,
+        const AZ::Vector2& viewportSize)
    {
        AzFramework::CameraState cameraState;

-        SetDefaultCameraClippingVolume(cameraState);
        SetCameraTransform(cameraState, transform);
+        SetCameraClippingVolume(cameraState, nearPlane, farPlane, verticalFovRad);
        cameraState.m_viewportSize = viewportSize;

        return cameraState;
    }

-    AzFramework::CameraState CreateIdentityDefaultCamera(
-        const AZ::Vector3& position, const AZ::Vector2& viewportSize)
+    AzFramework::CameraState CreateDefaultCamera(const AZ::Transform& transform, const AZ::Vector2& viewportSize)
+    {
+        AzFramework::CameraState cameraState;
+
+        SetCameraTransform(cameraState, transform);
+        SetDefaultCameraClippingVolume(cameraState);
+        cameraState.m_viewportSize = viewportSize;
+
+        return cameraState;
+    }
+
+    AzFramework::CameraState CreateIdentityDefaultCamera(const AZ::Vector3& position, const AZ::Vector2& viewportSize)
    {
        return CreateDefaultCamera(AZ::Transform::CreateTranslation(position), viewportSize);
    }
@@ -89,15 +103,15 @@ namespace AzFramework

    void CameraState::Reflect(AZ::SerializeContext& serializeContext)
    {
-        serializeContext.Class<CameraState>()->
-            Field("Position", &CameraState::m_position)->
-            Field("Forward", &CameraState::m_forward)->
-            Field("Side", &CameraState::m_side)->
-            Field("Up", &CameraState::m_up)->
-            Field("ViewportSize", &CameraState::m_viewportSize)->
-            Field("NearClip", &CameraState::m_nearClip)->
-            Field("FarClip", &CameraState::m_farClip)->
-            Field("FovZoom", &CameraState::m_fovOrZoom)->
-            Field("Ortho", &CameraState::m_orthographic);
+        serializeContext.Class<CameraState>()
+            ->Field("Position", &CameraState::m_position)
+            ->Field("Forward", &CameraState::m_forward)
+            ->Field("Side", &CameraState::m_side)
+            ->Field("Up", &CameraState::m_up)
+            ->Field("ViewportSize", &CameraState::m_viewportSize)
+            ->Field("NearClip", &CameraState::m_nearClip)
+            ->Field("FarClip", &CameraState::m_farClip)
+            ->Field("FovZoom", &CameraState::m_fovOrZoom)
+            ->Field("Ortho", &CameraState::m_orthographic);
    }
 } // namespace AzFramework
@@ -40,10 +40,14 @@ namespace AzFramework
        AZ::Vector2 m_viewportSize = AZ::Vector2::CreateZero(); //!< Dimensions of the viewport.
        float m_nearClip = 0.01f; //!< Near clip plane of the camera.
        float m_farClip = 100.0f; //!< Far clip plane of the camera.
-        float m_fovOrZoom = 0.0f; //!< Fov or zoom of camera depending on if it is using orthographic projection or not.
+        float m_fovOrZoom = 0.0f; //!< Vertical fov or zoom of camera depending on if it is using orthographic projection or not.
        bool m_orthographic = false; //!< Is the camera using orthographic projection or not.
    };

+    //! Create a camera at the given transform, specifying the near and far clip planes as well as the fov with a specific viewport size.
+    CameraState CreateCamera(
+        const AZ::Transform& transform, float nearPlane, float farPlane, float verticalFovRad, const AZ::Vector2& viewportSize);
+
    //! Create a camera at the given transform with a specific viewport size.
    //! @note The near/far clip planes and fov are sensible default values - please
    //! use SetCameraClippingVolume to override them.
@@ -60,7 +64,7 @@ namespace AzFramework
    CameraState CreateCameraFromWorldFromViewMatrix(const AZ::Matrix4x4& worldFromView, const AZ::Vector2& viewportSize);

    //! Override the default near/far clipping planes and fov of the camera.
-    void SetCameraClippingVolume(CameraState& cameraState, float nearPlane, float farPlane, float fovRad);
+    void SetCameraClippingVolume(CameraState& cameraState, float nearPlane, float farPlane, float verticalFovRad);

    //! Override the default near/far clipping planes and fov of the camera by inferring them the specified right handed transform into clip space.
    void SetCameraClippingVolumeFromPerspectiveFovMatrixRH(CameraState& cameraState, const AZ::Matrix4x4& clipFromView);
@@ -24,6 +24,10 @@ namespace AzFramework
            serializeContext->Class<ScreenVector>()->
                Field("X", &ScreenVector::m_x)->
                Field("Y", &ScreenVector::m_y);
+
+            serializeContext->Class<ScreenSize>()->
+                Field("Width", &ScreenSize::m_width)->
+                Field("Height", &ScreenSize::m_height);
        }
    }
 } // namespace AzFramework
@@ -26,7 +26,7 @@ namespace AzFramework
        AZ_TYPE_INFO(ScreenPoint, "{8472B6C2-527F-44FC-87F8-C226B1A57A97}");
        ScreenPoint() = default;

-        ScreenPoint(int x, int y)
+        constexpr ScreenPoint(int x, int y)
            : m_x(x)
            , m_y(y)
        {
@@ -45,7 +45,7 @@ namespace AzFramework
        AZ_TYPE_INFO(ScreenVector, "{1EAA2C62-8FDB-4A28-9FE3-1FA4F1418894}");
        ScreenVector() = default;

-        ScreenVector(int x, int y)
+        constexpr ScreenVector(int x, int y)
            : m_x(x)
            , m_y(y)
        {
@@ -55,6 +55,22 @@ namespace AzFramework
        int m_y; //!< Y screen delta.
    };

+    //! A wrapper around a screen width and height.
+    struct ScreenSize
+    {
+        AZ_TYPE_INFO(ScreenSize, "{26D28916-6E8E-44B8-83F9-C44BCDA370E2}");
+        ScreenSize() = default;
+
+        constexpr ScreenSize(int width, int height)
+            : m_width(width)
+            , m_height(height)
+        {
+        }
+
+        int m_width; //!< Screen size width.
+        int m_height; //!< Screen size height.
+    };
+
    void ScreenGeometryReflect(AZ::ReflectContext* context);

    inline const ScreenVector operator-(const ScreenPoint& lhs, const ScreenPoint& rhs)
@@ -138,6 +154,16 @@ namespace AzFramework
        return !operator==(lhs, rhs);
    }

+    inline const bool operator==(const ScreenSize& lhs, const ScreenSize& rhs)
+    {
+        return lhs.m_width == rhs.m_width && lhs.m_height == rhs.m_height;
+    }
+
+    inline const bool operator!=(const ScreenSize& lhs, const ScreenSize& rhs)
+    {
+        return !operator==(lhs, rhs);
+    }
+
    inline ScreenVector& operator*=(ScreenVector& lhs, const float rhs)
    {
        lhs.m_x = aznumeric_cast<int>(AZStd::lround(aznumeric_cast<float>(lhs.m_x) * rhs));
@@ -152,6 +178,20 @@ namespace AzFramework
        return result;
    }

+    inline ScreenSize& operator*=(ScreenSize& lhs, const float rhs)
+    {
+        lhs.m_width = aznumeric_cast<int>(AZStd::lround(aznumeric_cast<float>(lhs.m_width) * rhs));
+        lhs.m_height = aznumeric_cast<int>(AZStd::lround(aznumeric_cast<float>(lhs.m_height) * rhs));
+        return lhs;
+    }
+
+    inline const ScreenSize operator*(const ScreenSize& lhs, const float rhs)
+    {
+        ScreenSize result{ lhs };
+        result *= rhs;
+        return result;
+    }
+
    inline float ScreenVectorLength(const ScreenVector& screenVector)
    {
        return aznumeric_cast<float>(AZStd::sqrt(screenVector.m_x * screenVector.m_x + screenVector.m_y * screenVector.m_y));
@@ -168,4 +208,28 @@ namespace AzFramework
    {
        return AZ::Vector2(aznumeric_cast<float>(screenVector.m_x), aznumeric_cast<float>(screenVector.m_y));
    }
+
+    //! Return an AZ::Vector2 from a ScreenSize.
+    inline AZ::Vector2 Vector2FromScreenSize(const ScreenSize& screenSize)
+    {
+        return AZ::Vector2(aznumeric_cast<float>(screenSize.m_width), aznumeric_cast<float>(screenSize.m_height));
+    }
+
+    //! Return a ScreenPoint from an AZ::Vector2.
+    inline ScreenPoint ScreenPointFromVector2(const AZ::Vector2& vector2)
+    {
+        return ScreenPoint(aznumeric_cast<int>(AZStd::lround(vector2.GetX())), aznumeric_cast<int>(AZStd::lround(vector2.GetY())));
+    }
+
+    //! Return a ScreenVector from an AZ::Vector2.
+    inline ScreenVector ScreenVectorFromVector2(const AZ::Vector2& vector2)
+    {
+        return ScreenVector(aznumeric_cast<int>(AZStd::lround(vector2.GetX())), aznumeric_cast<int>(AZStd::lround(vector2.GetY())));
+    }
+
+    //! Return a ScreenSize from an AZ::Vector2.
+    inline ScreenSize ScreenSizeFromVector2(const AZ::Vector2& vector2)
+    {
+        return ScreenSize(aznumeric_cast<int>(AZStd::lround(vector2.GetX())), aznumeric_cast<int>(AZStd::lround(vector2.GetY())));
+    }
 } // namespace AzFramework
@@ -10,6 +10,7 @@

 #include <AzCore/Math/Frustum.h>
 #include <AzCore/Math/Matrix4x4.h>
+#include <AzCore/Math/MatrixUtils.h>
 #include <AzCore/Math/Vector4.h>
 #include <AzCore/Math/VectorConversions.h>
 #include <AzFramework/Entity/EntityDebugDisplayBus.h>
@@ -112,9 +113,8 @@ namespace AzFramework
        const AZ::Matrix4x4& cameraProjection,
        const AZ::Vector2& viewportSize)
    {
-        const auto ndcNormalizedPosition = WorldToScreenNdc(worldPosition, cameraView, cameraProjection);
        // scale ndc position by screen dimensions to return screen position
-        return ScreenPointFromNdc(AZ::Vector3ToVector2(ndcNormalizedPosition), viewportSize);
+        return ScreenPointFromNdc(AZ::Vector3ToVector2(WorldToScreenNdc(worldPosition, cameraView, cameraProjection)), viewportSize);
    }

    ScreenPoint WorldToScreen(const AZ::Vector3& worldPosition, const CameraState& cameraState)
@@ -144,9 +144,7 @@ namespace AzFramework
        const AZ::Matrix4x4& inverseCameraProjection,
        const AZ::Vector2& viewportSize)
    {
-        const auto normalizedScreenPosition = NdcFromScreenPoint(screenPosition, viewportSize);
-
-        return ScreenNdcToWorld(normalizedScreenPosition, inverseCameraView, inverseCameraProjection);
+        return ScreenNdcToWorld(NdcFromScreenPoint(screenPosition, viewportSize), inverseCameraView, inverseCameraProjection);
    }

    AZ::Vector3 ScreenToWorld(const ScreenPoint& screenPosition, const CameraState& cameraState)
@@ -11,6 +11,7 @@
 #include <AzFramework/Viewport/CameraState.h>
 #include <AZTestShared/Math/MathTestHelpers.h>
 #include <AzCore/Math/SimdMath.h>
+#include <AzCore/Math/MatrixUtils.h>
 #include <AzCore/Math/Matrix4x4.h>

 namespace UnitTest
@@ -51,22 +52,6 @@ namespace UnitTest
    {
    };

-    // Taken from Atom::MatrixUtils for testing purposes, this can be removed if MakePerspectiveFovMatrixRH makes it into AZ
-    static AZ::Matrix4x4 MakePerspectiveMatrixRH(float fovY, float aspectRatio, float nearClip, float farClip)
-    {
-        float sinFov, cosFov;
-        AZ::SinCos(0.5f * fovY, sinFov, cosFov);
-        float yScale = cosFov / sinFov; //cot(fovY/2)
-        float xScale = yScale / aspectRatio;
-
-        AZ::Matrix4x4 out;
-        out.SetRow(0,   xScale, 0.f,    0.f,                                0.f                                     );
-        out.SetRow(1,   0.f,    yScale, 0.f,                                0.f                                     );
-        out.SetRow(2,   0.f,    0.f,    farClip / (nearClip - farClip),     nearClip*farClip / (nearClip - farClip) );
-        out.SetRow(3,   0.f,    0.f,    -1.f,                               0.f                                     );
-        return out;
-    }
-
    TEST_P(Translation, Permutation)
    {
        // Given a position
@@ -176,7 +161,8 @@ namespace UnitTest
    {
        auto [fovY, aspectRatio, nearClip, farClip] = GetParam();

-        AZ::Matrix4x4 clipFromView = MakePerspectiveMatrixRH(fovY, aspectRatio, nearClip, farClip);
+        AZ::Matrix4x4 clipFromView;
+        MakePerspectiveFovMatrixRH(clipFromView, fovY, aspectRatio, nearClip, farClip);

        AzFramework::SetCameraClippingVolumeFromPerspectiveFovMatrixRH(m_cameraState, clipFromView);

@@ -8,12 +8,13 @@

 #include <AzCore/UnitTest/TestTypes.h>
 #include <AzFramework/Viewport/CursorState.h>
+#include <Tests/Utils/Printers.h>

 namespace UnitTest
 {
    using AzFramework::CursorState;
-    using AzFramework::ScreenVector;
    using AzFramework::ScreenPoint;
+    using AzFramework::ScreenVector;

    class CursorStateFixture : public ::testing::Test
    {
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include "Printers.h"
+
+#include <AzFramework/Viewport/ScreenGeometry.h>
+
+#include <ostream>
+#include <string>
+
+namespace AzFramework
+{
+    void PrintTo(const ScreenPoint& screenPoint, std::ostream* os)
+    {
+        *os << "(x: " << screenPoint.m_x << ", y: " << screenPoint.m_y << ")";
+    }
+
+    void PrintTo(const ScreenVector& screenVector, std::ostream* os)
+    {
+        *os << "(x: " << screenVector.m_x << ", y: " << screenVector.m_y << ")";
+    }
+
+    void PrintTo(const ScreenSize& screenSize, std::ostream* os)
+    {
+        *os << "(width: " << screenSize.m_width << ", height: " << screenSize.m_height << ")";
+    }
+} // namespace AzFramework
@@ -0,0 +1,20 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include <iosfwd>
+
+namespace AzFramework
+{
+    struct ScreenPoint;
+    struct ScreenVector;
+    struct ScreenSize;
+
+    void PrintTo(const ScreenPoint& screenPoint, std::ostream* os);
+    void PrintTo(const ScreenVector& screenVector, std::ostream* os);
+    void PrintTo(const ScreenSize& screenSize, std::ostream* os);
+} // namespace AzFramework
@@ -11,5 +11,7 @@ set(FILES
    Mocks/MockWindowRequests.h
    Utils/Utils.h
    Utils/Utils.cpp
+    Utils/Printers.h
+    Utils/Printers.cpp
    FrameworkApplicationFixture.h
 )
@@ -41,8 +41,8 @@ namespace AzManipulatorTestFramework
        // ViewportInteractionRequestBus overrides ...
        AzFramework::CameraState GetCameraState() override;
        AzFramework::ScreenPoint ViewportWorldToScreen(const AZ::Vector3& worldPosition) override;
-        AZStd::optional<AZ::Vector3> ViewportScreenToWorld(const AzFramework::ScreenPoint& screenPosition, float depth) override;
-        AZStd::optional<AzToolsFramework::ViewportInteraction::ProjectedViewportRay> ViewportScreenToWorldRay(
+        AZ::Vector3 ViewportScreenToWorld(const AzFramework::ScreenPoint& screenPosition) override;
+        AzToolsFramework::ViewportInteraction::ProjectedViewportRay ViewportScreenToWorldRay(
            const AzFramework::ScreenPoint& screenPosition) override;
        float DeviceScalingFactor() override;

@@ -95,7 +95,7 @@ namespace AzManipulatorTestFramework

        AzToolsFramework::ViewportInteraction::MousePick mousePick;
        mousePick.m_screenCoordinates = screenPoint;
-        mousePick.m_rayOrigin = cameraState.m_position;
+        mousePick.m_rayOrigin = nearPlaneWorldPosition;
        mousePick.m_rayDirection = (nearPlaneWorldPosition - cameraState.m_position).GetNormalized();

        return mousePick;
@@ -69,8 +69,6 @@ namespace AzManipulatorTestFramework
    void ImmediateModeActionDispatcher::CameraStateImpl(const AzFramework::CameraState& cameraState)
    {
        m_viewportManipulatorInteraction.GetViewportInteraction().SetCameraState(cameraState);
-        GetMouseInteractionEvent()->m_mouseInteraction.m_mousePick.m_rayOrigin = cameraState.m_position;
-        GetMouseInteractionEvent()->m_mouseInteraction.m_mousePick.m_rayDirection = cameraState.m_forward;
    }

    void ImmediateModeActionDispatcher::MouseLButtonDownImpl()
@@ -20,7 +20,8 @@ namespace AzManipulatorTestFramework
    {
    public:
        IndirectCallManipulatorManager(ViewportInteractionInterface& viewportInteraction);
-        // ManipulatorManagerInterface ...
+
+        // ManipulatorManagerInterface overrides ...
        void ConsumeMouseInteractionEvent(const MouseInteractionEvent& event) override;
        AzToolsFramework::ManipulatorManagerId GetId() const override;
        bool ManipulatorBeingInteracted() const override;
@@ -140,13 +140,12 @@ namespace AzManipulatorTestFramework
        return m_viewportId;
    }

-    AZStd::optional<AZ::Vector3> ViewportInteraction::ViewportScreenToWorld(
-        [[maybe_unused]] const AzFramework::ScreenPoint& screenPosition, [[maybe_unused]] float depth)
+    AZ::Vector3 ViewportInteraction::ViewportScreenToWorld([[maybe_unused]] const AzFramework::ScreenPoint& screenPosition)
    {
-        return {};
+        return AZ::Vector3::CreateZero();
    }

-    AZStd::optional<AzToolsFramework::ViewportInteraction::ProjectedViewportRay> ViewportInteraction::ViewportScreenToWorldRay(
+    AzToolsFramework::ViewportInteraction::ProjectedViewportRay ViewportInteraction::ViewportScreenToWorldRay(
        [[maybe_unused]] const AzFramework::ScreenPoint& screenPosition)
    {
        return {};
@@ -140,8 +140,8 @@ namespace UnitTest
        // given a left mouse down ray in world space
        // consume the mouse move event
        state.m_actionDispatcher->CameraState(m_cameraState)
-            ->MouseLButtonDown()
            ->MousePosition(AzManipulatorTestFramework::GetCameraStateViewportCenter(m_cameraState))
+            ->MouseLButtonDown()
            ->ExpectTrue(state.m_linearManipulator->PerformingAction())
            ->ExpectManipulatorBeingInteracted()
            ->MouseLButtonUp()
@@ -158,7 +158,10 @@ namespace UnitTest
            {
                // Create & Start a new ToolsApplication if there's no existing one
                m_app = CreateTestApplication();
-                m_app->Start(AzFramework::Application::Descriptor());
+                AZ::ComponentApplication::StartupParameters startupParameters;
+                startupParameters.m_loadAssetCatalog = false;
+
+                m_app->Start(AzFramework::Application::Descriptor(), startupParameters);
            }

            // without this, the user settings component would attempt to save on finalize/shutdown. Since the file is
@@ -162,12 +162,11 @@ namespace AzToolsFramework
            //! Multiply by DeviceScalingFactor to get the position in viewport pixel space.
            virtual AzFramework::ScreenPoint ViewportWorldToScreen(const AZ::Vector3& worldPosition) = 0;
            //! Transforms a point from Qt widget screen space to world space based on the given clip space depth.
-            //! Depth specifies a relative camera depth to project in the range of [0.f, 1.f].
            //! Returns the world space position if successful.
-            virtual AZStd::optional<AZ::Vector3> ViewportScreenToWorld(const AzFramework::ScreenPoint& screenPosition, float depth) = 0;
+            virtual AZ::Vector3 ViewportScreenToWorld(const AzFramework::ScreenPoint& screenPosition) = 0;
            //! Casts a point in screen space to a ray in world space originating from the viewport camera frustum's near plane.
            //! Returns a ray containing the ray's origin and a direction normal, if successful.
-            virtual AZStd::optional<ProjectedViewportRay> ViewportScreenToWorldRay(const AzFramework::ScreenPoint& screenPosition) = 0;
+            virtual ProjectedViewportRay ViewportScreenToWorldRay(const AzFramework::ScreenPoint& screenPosition) = 0;
            //! Gets the DPI scaling factor that translates Qt widget space into viewport pixel space.
            virtual float DeviceScalingFactor() = 0;

@@ -229,9 +228,6 @@ namespace AzToolsFramework
        class MainEditorViewportInteractionRequests
        {
        public:
-            //! Given a point in screen space, return the picked entity (if any).
-            //! Picked EntityId will be returned, InvalidEntityId will be returned on failure.
-            virtual AZ::EntityId PickEntity(const AzFramework::ScreenPoint& point) = 0;
            //! Given a point in screen space, return the terrain position in world space.
            virtual AZ::Vector3 PickTerrain(const AzFramework::ScreenPoint& point) = 0;
            //! Return the terrain height given a world position in 2d (xy plane).
@@ -266,7 +262,6 @@ namespace AzToolsFramework
        {
        public:
            static const AZ::EBusAddressPolicy AddressPolicy = AZ::EBusAddressPolicy::Single;
-            static const AZ::EBusHandlerPolicy HandlerPolicy = AZ::EBusHandlerPolicy::Single;

            //! Returns the current state of the keyboard modifier keys.
            virtual KeyboardModifiers QueryKeyboardModifiers() = 0;
@@ -290,7 +285,6 @@ namespace AzToolsFramework
        {
        public:
            static const AZ::EBusAddressPolicy AddressPolicy = AZ::EBusAddressPolicy::Single;
-            static const AZ::EBusHandlerPolicy HandlerPolicy = AZ::EBusHandlerPolicy::Single;

            //! Returns the current time in seconds.
            //! This interface can be overridden for the purposes of testing to simplify viewport input requests.
@@ -148,7 +148,6 @@ namespace AzToolsFramework
        return false;
    }

-
    EditorHelpers::EditorHelpers(const EditorVisibleEntityDataCache* entityDataCache)
        : m_entityDataCache(entityDataCache)
    {
@@ -190,7 +189,10 @@ namespace AzToolsFramework
            if (helpersVisible)
            {
                // some components choose to hide their icons (e.g. meshes)
-                if (!m_entityDataCache->IsVisibleEntityIconHidden(entityCacheIndex))
+                // we also do not want to test against icons that may not be showing as they're inside a 'closed' entity container
+                // (these icons only become visible when it is opened for editing)
+                if (!m_entityDataCache->IsVisibleEntityIconHidden(entityCacheIndex) &&
+                    m_entityDataCache->IsVisibleEntityIndividuallySelectableInViewport(entityCacheIndex))
                {
                    const AZ::Vector3& entityPosition = m_entityDataCache->GetVisibleEntityPosition(entityCacheIndex);

@@ -235,7 +237,7 @@ namespace AzToolsFramework
                    viewportId, &ViewportInteraction::ViewportMouseCursorRequestBus::Events::SetOverrideCursor,
                    ViewportInteraction::CursorStyleOverride::Forbidden);
            }
-                
+
            if (mouseInteraction.m_mouseInteraction.m_mouseButtons.Left() &&
                    mouseInteraction.m_mouseEvent == ViewportInteraction::MouseEvent::Down ||
                mouseInteraction.m_mouseEvent == ViewportInteraction::MouseEvent::DoubleClick)
@@ -409,7 +409,7 @@ namespace AzToolsFramework
            const AzFramework::CameraState cameraState = GetCameraState(viewportId);
            for (size_t entityCacheIndex = 0; entityCacheIndex < entityDataCache.VisibleEntityDataCount(); ++entityCacheIndex)
            {
-                if (!entityDataCache.IsVisibleEntitySelectableInViewport(entityCacheIndex))
+                if (!entityDataCache.IsVisibleEntityIndividuallySelectableInViewport(entityCacheIndex))
                {
                    continue;
                }
@@ -983,7 +983,7 @@ namespace AzToolsFramework
    {
        if (auto entityIndex = entityDataCache.GetVisibleEntityIndexFromId(entityId))
        {
-            if (entityDataCache.IsVisibleEntitySelectableInViewport(*entityIndex))
+            if (entityDataCache.IsVisibleEntityIndividuallySelectableInViewport(*entityIndex))
            {
                return *entityIndex;
            }
@@ -293,12 +293,10 @@ namespace AzToolsFramework
        return m_impl->m_visibleEntityDatas[index].m_iconHidden;
    }

-    bool EditorVisibleEntityDataCache::IsVisibleEntitySelectableInViewport(size_t index) const
+    bool EditorVisibleEntityDataCache::IsVisibleEntityIndividuallySelectableInViewport(const size_t index) const
    {
-        return m_impl->m_visibleEntityDatas[index].m_visible
-            && !m_impl->m_visibleEntityDatas[index].m_locked
-            && m_impl->m_visibleEntityDatas[index].m_inFocus
-            && !m_impl->m_visibleEntityDatas[index].m_descendantOfClosedContainer;
+        return m_impl->m_visibleEntityDatas[index].m_visible && !m_impl->m_visibleEntityDatas[index].m_locked &&
+            m_impl->m_visibleEntityDatas[index].m_inFocus && !m_impl->m_visibleEntityDatas[index].m_descendantOfClosedContainer;
    }

    AZStd::optional<size_t> EditorVisibleEntityDataCache::GetVisibleEntityIndexFromId(const AZ::EntityId entityId) const
@@ -55,7 +55,10 @@ namespace AzToolsFramework
        bool IsVisibleEntityVisible(size_t index) const;
        bool IsVisibleEntitySelected(size_t index) const;
        bool IsVisibleEntityIconHidden(size_t index) const;
-        bool IsVisibleEntitySelectableInViewport(size_t index) const;
+        //! Returns true if the entity is individually selectable (none of its ancestors are a closed container entity).
+        //! @note It may still be desirable to be able to 'click' an entity that is a descendant of a closed container
+        //! to select the container itself, not the individual entity.
+        bool IsVisibleEntityIndividuallySelectableInViewport(size_t index) const;

        AZStd::optional<size_t> GetVisibleEntityIndexFromId(AZ::EntityId entityId) const;

@@ -40,6 +40,9 @@ namespace UnitTest
    {
        AzFramework::BoundsRequestBus::Handler::BusConnect(GetEntityId());
        AzToolsFramework::EditorComponentSelectionRequestsBus::Handler::BusConnect(GetEntityId());
+
+        // default local bounds to unit cube
+        m_localBounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(-0.5f), AZ::Vector3(0.5f));
    }

    void BoundsTestComponent::Deactivate()
@@ -57,7 +60,6 @@ namespace UnitTest

    AZ::Aabb BoundsTestComponent::GetLocalBounds()
    {
-        return AZ::Aabb::CreateFromMinMax(AZ::Vector3(-0.5f), AZ::Vector3(0.5f));
+        return m_localBounds;
    }
-
 } // namespace UnitTest
@@ -41,5 +41,7 @@ namespace UnitTest
        // BoundsRequestBus overrides ...
        AZ::Aabb GetWorldBounds() override;
        AZ::Aabb GetLocalBounds() override;
+
+        AZ::Aabb m_localBounds; //!< Local bounds that can be modified for certain tests (defaults to unit cube).
    };
 } // namespace UnitTest
@@ -38,7 +38,7 @@
 #include <AzToolsFramework/ViewportSelection/EditorVisibleEntityDataCache.h>
 #include <AzToolsFramework/ViewportUi/ViewportUiManager.h>

-#include<Tests/BoundsTestComponent.h>
+#include <Tests/BoundsTestComponent.h>

 namespace AZ
 {
@@ -493,12 +493,8 @@ namespace UnitTest

        ///////////////////////////////////////////////////////////////////////////////////////////////////////////////
        // Then
-        AzToolsFramework::EntityIdList selectedEntities;
-        AzToolsFramework::ToolsApplicationRequestBus::BroadcastResult(
-            selectedEntities, &AzToolsFramework::ToolsApplicationRequestBus::Events::GetSelectedEntities);
-
-        AzToolsFramework::EntityIdList expectedSelectedEntities = { entity4, entity5, entity6 };
-
+        const AzToolsFramework::EntityIdList selectedEntities = SelectedEntities();
+        const AzToolsFramework::EntityIdList expectedSelectedEntities = { entity4, entity5, entity6 };
        EXPECT_THAT(selectedEntities, UnorderedElementsAreArray(expectedSelectedEntities));
        ///////////////////////////////////////////////////////////////////////////////////////////////////////////////
    }
@@ -527,12 +523,8 @@ namespace UnitTest

        ///////////////////////////////////////////////////////////////////////////////////////////////////////////////
        // Then
-        AzToolsFramework::EntityIdList selectedEntities;
-        AzToolsFramework::ToolsApplicationRequestBus::BroadcastResult(
-            selectedEntities, &AzToolsFramework::ToolsApplicationRequestBus::Events::GetSelectedEntities);
-
-        AzToolsFramework::EntityIdList expectedSelectedEntities = { m_entityId1, entity2, entity3, entity4 };
-
+        const AzToolsFramework::EntityIdList selectedEntities = SelectedEntities();
+        const AzToolsFramework::EntityIdList expectedSelectedEntities = { m_entityId1, entity2, entity3, entity4 };
        EXPECT_THAT(selectedEntities, UnorderedElementsAreArray(expectedSelectedEntities));
        ///////////////////////////////////////////////////////////////////////////////////////////////////////////////
    }
@@ -946,6 +938,42 @@ namespace UnitTest
        EXPECT_THAT(selectedEntitiesAfter, UnorderedElementsAre(m_entityId1));
    }

+    TEST_F(
+        EditorTransformComponentSelectionViewportPickingManipulatorTestFixture, BoundsBetweenCameraAndNearClipPlaneDoesNotIntersectMouseRay)
+    {
+        // move camera to 10 units along the y-axis
+        AzFramework::SetCameraTransform(m_cameraState, AZ::Transform::CreateTranslation(AZ::Vector3::CreateAxisY(10.0f)));
+
+        // send a very narrow bounds for entity1
+        AZ::Entity* entity1 = AzToolsFramework::GetEntityById(m_entityId1);
+        auto* boundTestComponent = entity1->FindComponent<BoundsTestComponent>();
+        boundTestComponent->m_localBounds =
+            AZ::Aabb::CreateFromMinMax(AZ::Vector3(-0.5f, -0.0025f, -0.5f), AZ::Vector3(0.5f, 0.0025f, 0.5f));
+
+        // move entity1 in front of the camera between it and the near clip plane
+        AZ::TransformBus::Event(
+            m_entityId1, &AZ::TransformBus::Events::SetWorldTM, AZ::Transform::CreateTranslation(AZ::Vector3::CreateAxisY(10.05f)));
+        // move entity2 behind entity1
+        AZ::TransformBus::Event(
+            m_entityId2, &AZ::TransformBus::Events::SetWorldTM, AZ::Transform::CreateTranslation(AZ::Vector3::CreateAxisY(15.0f)));
+
+        const auto entity2ScreenPosition = AzFramework::WorldToScreen(AzToolsFramework::GetWorldTranslation(m_entityId2), m_cameraState);
+
+        // click the entity in the viewport
+        m_actionDispatcher->SetStickySelect(true)
+            ->CameraState(m_cameraState)
+            ->MousePosition(entity2ScreenPosition)
+            ->CameraState(m_cameraState)
+            ->MouseLButtonDown()
+            ->MouseLButtonUp();
+
+        // ensure entity1 is not selected as it is before the near clip plane
+        using ::testing::UnorderedElementsAreArray;
+        const AzToolsFramework::EntityIdList selectedEntities = SelectedEntities();
+        const AzToolsFramework::EntityIdList expectedSelectedEntities = { m_entityId2 };
+        EXPECT_THAT(selectedEntities, UnorderedElementsAreArray(expectedSelectedEntities));
+    }
+
    class EditorTransformComponentSelectionViewportPickingManipulatorTestFixtureParam
        : public EditorTransformComponentSelectionViewportPickingManipulatorTestFixture
        , public ::testing::WithParamInterface<bool>
@@ -23,6 +23,7 @@
 #include <AzManipulatorTestFramework/AzManipulatorTestFrameworkTestHelpers.h>
 #include <AzManipulatorTestFramework/IndirectManipulatorViewportInteraction.h>
 #include <AzManipulatorTestFramework/ImmediateModeActionDispatcher.h>
+#include <Tests/Utils/Printers.h>

 using namespace AzToolsFramework;

@@ -17,6 +17,7 @@
 #include <AzTest/AzTest.h>
 #include <AzToolsFramework/UnitTest/AzToolsFrameworkTestHelpers.h>
 #include <AzToolsFramework/Viewport/ViewportTypes.h>
+#include <Tests/Utils/Printers.h>

 namespace UnitTest
 {
@@ -35,6 +36,7 @@ namespace UnitTest
        const auto worldResult = AzFramework::ScreenToWorld(screenPoint, cameraState);
        return AzFramework::WorldToScreen(worldResult, cameraState);
    }
+
    ////////////////////////////////////////////////////////////////////////////////////////////////////////
    // ScreenPoint tests
    TEST(ViewportScreen, WorldToScreenAndScreenToWorldReturnsTheSameValueIdentityCameraOffsetFromOrigin)
@@ -102,8 +104,8 @@ namespace UnitTest
    }

    ////////////////////////////////////////////////////////////////////////////////////////////////////////
-    // NDC tests
-    TEST(ViewportScreen, WorldToScreenNDCAndScreenNDCToWorldReturnsTheSameValueIdentityCameraOffsetFromOrigin)
+    // Ndc tests
+    TEST(ViewportScreen, WorldToScreenNdcAndScreenNdcToWorldReturnsTheSameValueIdentityCameraOffsetFromOrigin)
    {
        using NdcPoint = AZ::Vector2;

@@ -136,7 +138,7 @@ namespace UnitTest
        }
    }

-    TEST(ViewportScreen, WorldToScreenNDCAndScreenNDCToWorldReturnsTheSameValueOrientatedCamera)
+    TEST(ViewportScreen, WorldToScreenNdcAndScreenNdcToWorldReturnsTheSameValueOrientatedCamera)
    {
        using NdcPoint = AZ::Vector2;

@@ -153,7 +155,7 @@ namespace UnitTest

    // note: nearClip is 0.1 - the world space value returned will be aligned to the near clip
    // plane of the camera so use that to confirm the mapping to/from is correct
-    TEST(ViewportScreen, ScreenNDCToWorldReturnsPositionOnNearClipPlaneInWorldSpace)
+    TEST(ViewportScreen, ScreenNdcToWorldReturnsPositionOnNearClipPlaneInWorldSpace)
    {
        using NdcPoint = AZ::Vector2;