Merge pull request #7 from aws-lumberyard-dev/nvsickle/EditorViewportWidgetFixes

Fix several EditorViewportWidget bugs

Code/Sandbox/Editor/EditorViewportWidget.cpp

@@ -30,6 +30,7 @@
 #include <AzCore/Component/EntityId.h>
 #include <AzCore/Interface/Interface.h>
 #include <AzCore/Math/VectorConversions.h>
+#include <AzCore/Console/IConsole.h>
 
 // AzFramework
 #include <AzFramework/Components/CameraBus.h>
@@ -96,6 +97,10 @@
 
 #include <QtGui/private/qhighdpiscaling_p.h>
 
+AZ_CVAR(
+    bool, ed_visibility_logTiming, false, nullptr, AZ::ConsoleFunctorFlags::Null,
+    "Output the timing of the new IVisibilitySystem query");
+
 EditorViewportWidget* EditorViewportWidget::m_pPrimaryViewport = nullptr;
 
 #if AZ_TRAIT_OS_PLATFORM_APPLE
@@ -139,35 +144,6 @@ namespace AZ::ViewportHelpers
     };
 } // namespace AZ::ViewportHelpers
 
-struct EditorViewportWidget::SScopedCurrentContext
-{
-    const EditorViewportWidget* m_viewport;
-    EditorViewportWidget::SPreviousContext m_previousContext;
-
-    explicit SScopedCurrentContext(const EditorViewportWidget* viewport)
-        : m_viewport(viewport)
-    {
-        m_previousContext = viewport->SetCurrentContext();
-
-        // During normal updates of RenderViewport the value of m_cameraSetForWidgetRenderingCount is expected to be 0.
-        // This is to guarantee no loss in performance by tracking unnecessary calls to SetCurrentContext/RestorePreviousContext.
-        // If some code makes additional calls to Pre/PostWidgetRendering then the assert will be triggered because
-        // m_cameraSetForWidgetRenderingCount will be greater than 0.
-        // There is a legitimate case where the counter can be greater than 0. This is when QtViewport is processing mouse callbacks.
-        // QtViewport::MouseCallback() is surrounded by Pre/PostWidgetRendering and the m_processingMouseCallbacksCounter
-        // tracks this specific case. If an update of a RenderViewport happens while processing the mouse callback,
-        // for example when showing a QMessageBox, then both counters must match.
-        AZ_Assert(viewport->m_cameraSetForWidgetRenderingCount == viewport->m_processingMouseCallbacksCounter,
-            "SScopedCurrentContext constructor was called while viewport widget context is active "
-            "- this is unnecessary");
-    }
-
-    ~SScopedCurrentContext()
-    {
-        m_viewport->RestorePreviousContext(m_previousContext);
-    }
-};
-
 //////////////////////////////////////////////////////////////////////////
 // EditorViewportWidget
 //////////////////////////////////////////////////////////////////////////
@@ -224,7 +200,7 @@ EditorViewportWidget::EditorViewportWidget(const QString& name, QWidget* parent)
     m_manipulatorManager = GetIEditor()->GetViewManager()->GetManipulatorManager();
     if (!m_pPrimaryViewport)
     {
-        m_pPrimaryViewport = this;
+        SetAsActiveViewport();
     }
 }
 
@@ -489,7 +465,7 @@ void EditorViewportWidget::Update()
         {
             if (CheckRespondToInput()) // If this is the focused window, set primary viewport.
             {
-                m_pPrimaryViewport = this;
+                SetAsActiveViewport();
             }
             else if (!m_bUpdateViewport) // Skip this viewport.
             {
@@ -546,8 +522,6 @@ void EditorViewportWidget::Update()
 
     // Render
     {
-        SScopedCurrentContext context(this);
-
         // TODO: Move out this logic to a controller and refactor to work with Atom
         // m_renderer->SetClearColor(Vec3(0.4f, 0.4f, 0.4f));
         // 3D engine stats
@@ -581,6 +555,19 @@ void EditorViewportWidget::Update()
         gEnv->pSystem->SetViewCamera(CurCamera);
     }
 
+    {
+        auto start = std::chrono::steady_clock::now();
+
+        m_entityVisibilityQuery.UpdateVisibility(GetCameraState());
+
+        if (ed_visibility_logTiming)
+        {
+            auto stop = std::chrono::steady_clock::now();
+            std::chrono::duration<double> diff = stop - start;
+            AZ_Printf("Visibility", "FindVisibleEntities (new) - Duration: %f", diff);
+        }
+    }
+
     QtViewport::Update();
 
     PopDisableRendering();
@@ -680,17 +667,13 @@ void EditorViewportWidget::OnEditorNotifyEvent(EEditorNotifyEvent event)
 
                 if (deviceInfo)
                 {
+                    // Note: This may also need to adjust the viewport size
                     outputToHMD->Set(1);
-                    m_previousContext = SetCurrentContext(deviceInfo->renderWidth, deviceInfo->renderHeight);
                     SetActiveWindow();
                     SetFocus();
                     SetSelected(true);
                 }
             }
-            else
-            {
-                m_previousContext = SetCurrentContext();
-            }
             SetCurrentCursor(STD_CURSOR_GAME);
             AzFramework::InputSystemCursorConstraintRequestBus::Handler::BusConnect();
         }
@@ -706,7 +689,6 @@ void EditorViewportWidget::OnEditorNotifyEvent(EEditorNotifyEvent event)
             {
                 outputToHMD->Set(0);
             }
-            RestorePreviousContext(m_previousContext);
             m_bInRotateMode = false;
             m_bInMoveMode = false;
             m_bInOrbitMode = false;
@@ -1180,15 +1162,7 @@ void EditorViewportWidget::FindVisibleEntities(AZStd::vector<AZ::EntityId>& visi
 {
     FUNCTION_PROFILER(GetIEditor()->GetSystem(), PROFILE_EDITOR);
 
-    if (m_displayContext.GetView() == nullptr)
-    {
-        return;
-    }
-
-    const AZStd::vector<AZ::EntityId>& entityIdCache =
-        m_displayContext.GetView()->GetVisibleObjectsCache()->GetEntityIdCache();
-
-    visibleEntitiesOut.assign(entityIdCache.begin(), entityIdCache.end());
+    visibleEntitiesOut.assign(m_entityVisibilityQuery.Begin(), m_entityVisibilityQuery.End());
 }
 
 QPoint EditorViewportWidget::ViewportWorldToScreen(const AZ::Vector3& worldPosition)
@@ -1233,6 +1207,7 @@ void EditorViewportWidget::SetViewportId(int id)
 
     // Now that we have an ID, we can initialize our viewport.
     m_renderViewport = new AtomToolsFramework::RenderViewportWidget(id, this);
+    m_defaultViewportContextName = m_renderViewport->GetViewportContext()->GetName();
     QBoxLayout* layout = new QBoxLayout(QBoxLayout::Direction::TopToBottom, this);
     layout->setContentsMargins(QMargins());
     layout->addWidget(m_renderViewport);
@@ -1244,6 +1219,11 @@ void EditorViewportWidget::SetViewportId(int id)
     m_renderViewport->GetControllerList()->Add(AZStd::make_shared<SandboxEditor::ViewportManipulatorController>());
     m_renderViewport->GetControllerList()->Add(AZStd::make_shared<SandboxEditor::LegacyViewportCameraController>());
     UpdateScene();
+
+    if (m_pPrimaryViewport == this)
+    {
+        SetAsActiveViewport();
+    }
 }
 
 void EditorViewportWidget::ConnectViewportInteractionRequestBus()
@@ -1986,9 +1966,6 @@ void EditorViewportWidget::RenderSelectedRegion()
 
 Vec3 EditorViewportWidget::WorldToView3D(const Vec3& wp, [[maybe_unused]] int nFlags) const
 {
-    AZ_Assert(m_cameraSetForWidgetRenderingCount > 0,
-        "WorldToView3D was called but viewport widget rendering was not set. PreWidgetRendering must be called before.");
-
     Vec3 out(0, 0, 0);
     float x, y, z;
 
@@ -2007,10 +1984,6 @@ Vec3 EditorViewportWidget::WorldToView3D(const Vec3& wp, [[maybe_unused]] int nF
 //////////////////////////////////////////////////////////////////////////
 QPoint EditorViewportWidget::WorldToView(const Vec3& wp) const
 {
-    AZ_Assert(m_cameraSetForWidgetRenderingCount > 0,
-        "WorldToView was called but viewport widget rendering was not set. PreWidgetRendering must be called before.");
-
-
     return m_renderViewport->ViewportWorldToScreen(LYVec3ToAZVec3(wp));
 }
 //////////////////////////////////////////////////////////////////////////
@@ -2178,9 +2151,6 @@ void EditorViewportWidget::ProjectToScreen(float ptx, float pty, float ptz, floa
 //////////////////////////////////////////////////////////////////////////
 void EditorViewportWidget::ViewToWorldRay(const QPoint& vp, Vec3& raySrc, Vec3& rayDir) const
 {
-    AZ_Assert(m_cameraSetForWidgetRenderingCount > 0,
-        "ViewToWorldRay was called but SScopedCurrentContext was not set at a higher scope! This means the camera for this call is incorrect.");
-
     QRect rc = m_rcClient;
 
     Vec3 pos0, pos1;
@@ -2245,7 +2215,7 @@ bool EditorViewportWidget::CheckRespondToInput() const
         return false;
     }
 
-    if (!hasFocus())
+    if (!hasFocus() && !m_renderViewport->hasFocus())
     {
         return false;
     }
@@ -2650,55 +2620,6 @@ void EditorViewportWidget::OnStopPlayInEditor()
     }
 }
 
-//////////////////////////////////////////////////////////////////////////
-EditorViewportWidget::SPreviousContext EditorViewportWidget::SetCurrentContext(int /*newWidth*/, int /*newHeight*/) const
-{
-    SPreviousContext x;
-
-    return x;
-}
-
-//////////////////////////////////////////////////////////////////////////
-EditorViewportWidget::SPreviousContext EditorViewportWidget::SetCurrentContext() const
-{
-    const auto r = rect();
-    return SetCurrentContext(r.width(), r.height());
-}
-
-//////////////////////////////////////////////////////////////////////////
-void EditorViewportWidget::RestorePreviousContext(const SPreviousContext& /*x*/) const
-{
-}
-
-void EditorViewportWidget::PreWidgetRendering()
-{
-    // if we have not already set the render context for the viewport, do it now
-    // based on the current state of the renderer/viewport, record the previous
-    // context to restore afterwards
-    if (m_cameraSetForWidgetRenderingCount == 0)
-    {
-        m_preWidgetContext = SetCurrentContext();
-    }
-
-    // keep track of how many times we've attempted to update the context
-    m_cameraSetForWidgetRenderingCount++;
-}
-
-void EditorViewportWidget::PostWidgetRendering()
-{
-    if (m_cameraSetForWidgetRenderingCount > 0)
-    {
-        m_cameraSetForWidgetRenderingCount--;
-
-        // unwinding - when the viewport context is no longer required,
-        // restore the previous context when widget rendering first began
-        if (m_cameraSetForWidgetRenderingCount == 0)
-        {
-            RestorePreviousContext(m_preWidgetContext);
-        }
-    }
-}
-
 //////////////////////////////////////////////////////////////////////////
 void EditorViewportWidget::OnCameraFOVVariableChanged([[maybe_unused]] IVariable* var)
 {
@@ -2888,4 +2809,36 @@ void EditorViewportWidget::UpdateCameraFromViewportContext()
     m_Camera.SetZRange(cameraState.m_nearClip, cameraState.m_farClip);
 }
 
+void EditorViewportWidget::SetAsActiveViewport()
+{
+    auto viewportContextManager = AZ::Interface<AZ::RPI::ViewportContextRequestsInterface>::Get();
+
+    const AZ::Name defaultContextName = viewportContextManager->GetDefaultViewportContextName();
+
+    // If another viewport was active before, restore its name to its per-ID one.
+    if (m_pPrimaryViewport && m_pPrimaryViewport != this && m_pPrimaryViewport->m_renderViewport)
+    {
+        auto viewportContext = m_pPrimaryViewport->m_renderViewport->GetViewportContext();
+        if (viewportContext)
+        {
+            // Remove the old viewport's camera from the stack, as it's no longer the owning viewport
+            viewportContextManager->PopView(defaultContextName, viewportContext->GetDefaultView());
+            viewportContextManager->RenameViewportContext(viewportContext, m_pPrimaryViewport->m_defaultViewportContextName);
+        }
+    }
+
+    m_pPrimaryViewport = this;
+    if (m_renderViewport)
+    {
+        auto viewportContext = m_renderViewport->GetViewportContext();
+        if (viewportContext)
+        {
+            // Push our camera onto the default viewport's view stack to preserve camera state continuity
+            // Other views can still be pushed on top of our view for e.g. game mode
+            viewportContextManager->PushView(defaultContextName, viewportContext->GetDefaultView());
+            viewportContextManager->RenameViewportContext(viewportContext, defaultContextName);
+        }
+    }
+}
+
 #include <moc_EditorViewportWidget.cpp>

Code/Sandbox/Editor/EditorViewportWidget.h

@@ -354,26 +354,6 @@ protected:
 
     void RenderAll();
 
-    struct SPreviousContext
-    {
-        CCamera rendererCamera;
-        HWND window;
-        int width;
-        int height;
-        bool mainViewport;
-    };
-
-    SPreviousContext m_preWidgetContext;
-
-    // Create an auto-sized render context that is sized based on the Editor's current
-    // viewport.
-    SPreviousContext SetCurrentContext() const;
-
-    SPreviousContext SetCurrentContext(int newWidth, int newHeight) const;
-    void RestorePreviousContext(const SPreviousContext& x) const;
-
-    void PreWidgetRendering() override;
-    void PostWidgetRendering() override;
     void OnBeginPrepareRender() override;
 
     // Update the safe frame, safe action, safe title, and borders rectangles based on
@@ -579,6 +559,7 @@ protected:
     void BuildDragDropContext(AzQtComponents::ViewportDragContext& context, const QPoint& pt) override;
 
 private:
+    void SetAsActiveViewport();
     void PushDisableRendering();
     void PopDisableRendering();
     bool IsRenderingDisabled() const;
@@ -606,13 +587,10 @@ private:
 
     AzFramework::EntityVisibilityQuery m_entityVisibilityQuery;
 
-    SPreviousContext m_previousContext;
     QSet<int> m_keyDown;
 
     bool m_freezeViewportInput = false;
 
-    size_t m_cameraSetForWidgetRenderingCount = 0; ///< How many calls to PreWidgetRendering happened before
-                                                   ///< subsequent calls to PostWidetRendering.
     AZStd::shared_ptr<AzToolsFramework::ManipulatorManager> m_manipulatorManager;
 
     // Used to prevent circular set camera events
@@ -627,5 +605,7 @@ private:
     AZ::RPI::ViewportContext::MatrixChangedEvent::Handler m_cameraProjectionMatrixChangeHandler;
     AzFramework::DebugDisplayRequests* m_debugDisplay = nullptr;
 
+    AZ::Name m_defaultViewportContextName;
+
     AZ_POP_DISABLE_DLL_EXPORT_MEMBER_WARNING
 };

Code/Sandbox/Editor/RenderViewport.cpp

@@ -94,9 +94,6 @@
 AZ_CVAR(
     bool, ed_visibility_use, true, nullptr, AZ::ConsoleFunctorFlags::Null,
     "Enable/disable using the new IVisibilitySystem for Entity visibility determination");
-AZ_CVAR(
-    bool, ed_visibility_logTiming, false, nullptr, AZ::ConsoleFunctorFlags::Null,
-    "Output the timing of the new IVisibilitySystem query");
 
 CRenderViewport* CRenderViewport::m_pPrimaryViewport = nullptr;
 
@@ -1394,13 +1391,6 @@ void CRenderViewport::Update()
         auto start = std::chrono::steady_clock::now();
 
         m_entityVisibilityQuery.UpdateVisibility(GetCameraState());
-
-        if (ed_visibility_logTiming)
-        {
-            auto stop = std::chrono::steady_clock::now();
-            std::chrono::duration<double> diff = stop - start;
-            AZ_Printf("Visibility", "FindVisibleEntities (new) - Duration: %f", diff);
-        }
     }
 
     {

Code/Sandbox/Editor/Viewport.cpp

@@ -218,10 +218,11 @@ QtViewport::QtViewport(QWidget* parent)
     // Create drop target to handle Qt drop events.
     setAcceptDrops(true);
 
-    m_renderOverlay.setVisible(false);
+    m_renderOverlay.setVisible(true);
     m_renderOverlay.setUpdatesEnabled(false);
     m_renderOverlay.setMouseTracking(true);
     m_renderOverlay.setObjectName("renderOverlay");
+    m_renderOverlay.winId(); // Force the render overlay to create a backing native window
 
     m_viewportUi.InitializeViewportUi(this, &m_renderOverlay);
 

Code/Sandbox/Editor/Viewport.h

@@ -280,7 +280,7 @@ public:
     virtual CViewport *asCViewport() { return this; }
 
 protected:
-    CLayoutViewPane* m_viewPane;
+    CLayoutViewPane* m_viewPane = nullptr;
     CViewManager* m_viewManager;
     AZ_PUSH_DISABLE_DLL_EXPORT_MEMBER_WARNING
     // Viewport matrix.

Code/Sandbox/Editor/ViewportManipulatorController.cpp

@@ -20,8 +20,8 @@
 
 #include <QApplication>
 
-static const auto ManipulatorPriority = AzFramework::ViewportControllerPriority::High;
-static const auto InteractionPriority = AzFramework::ViewportControllerPriority::Low;
+static const auto ManipulatorPriority = AzFramework::ViewportControllerPriority::Highest;
+static const auto InteractionPriority = AzFramework::ViewportControllerPriority::High;
 
 namespace SandboxEditor
 {
@@ -127,12 +127,20 @@ bool ViewportManipulatorControllerInstance::HandleInputChannelEvent(const AzFram
             m_state.m_mouseButtons.m_mouseButtons |= static_cast<AZ::u32>(mouseButton);
             if (IsDoubleClick(mouseButton))
             {
-                m_pendingDoubleClicks.erase(mouseButton);
+                // Only remove the double click flag once we're done processing both Manipulator and Interaction events
+                if (event.m_priority == InteractionPriority)
+                {
+                    m_pendingDoubleClicks.erase(mouseButton);
+                }
                 eventType = MouseEvent::DoubleClick;
             }
             else
             {
-                m_pendingDoubleClicks[mouseButton] = m_curTime;
+                // Only insert the double click timing once we're done processing both Manipulator and Interaction events, to avoid a false IsDoubleClick positive
+                if (event.m_priority == InteractionPriority)
+                {
+                    m_pendingDoubleClicks[mouseButton] = m_curTime;
+                }
                 eventType = MouseEvent::Down;
             }
         }
@@ -161,7 +169,7 @@ bool ViewportManipulatorControllerInstance::HandleInputChannelEvent(const AzFram
         {
             mouseInteraction.m_mouseButtons.m_mouseButtons = static_cast<AZ::u32>(overrideButton.value());
         }
-        MouseInteractionEvent mouseEvent = MouseInteractionEvent(mouseInteraction, eventType.value());
+        mouseInteraction.m_interactionId.m_viewportId = GetViewportId();
 
         // Depending on priority, we dispatch to either the manipulator or viewport interaction event
         const auto& targetInteractionEvent =

Gems/Atom/RPI/Code/Source/RPI.Public/ViewportContext.cpp

@@ -192,6 +192,9 @@ namespace AZ
             {
                 m_defaultView = view;
                 UpdatePipelineView();
+
+                m_viewMatrixChangedEvent.Signal(view->GetWorldToViewMatrix());
+                m_projectionMatrixChangedEvent.Signal(view->GetViewToClipMatrix());
             }
         }
 

Gems/Atom/RPI/Code/Source/RPI.Public/ViewportContextManager.cpp

@@ -142,7 +142,7 @@ namespace AZ
                 params.renderScene
             );
             viewportContext->GetWindowContext()->RegisterAssociatedViewportContext(viewportContext);
-            RegisterViewportContext(contextName, viewportContext);
+            RegisterViewportContext(nameToUse, viewportContext);
             return viewportContext;
         }
 
@@ -170,7 +170,9 @@ namespace AZ
                 AZ_Assert(false, "Attempted to rename ViewportContext \"%s\" to \"%s\", but \"%s\" is already assigned to another ViewportContext", viewportContext->m_name.GetCStr(), newContextName.GetCStr(), newContextName.GetCStr());
                 return;
             }
-            RegisterViewportContext(newContextName, viewportContext);
+            GetOrCreateViewStackForContext(newContextName);
+            viewportContext->m_name = newContextName;
+            UpdateViewForContext(newContextName);
         }
 
         void ViewportContextManager::EnumerateViewportContexts(AZStd::function<void(ViewportContextPtr)> visitorFunction)

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

@@ -42,6 +42,7 @@ namespace AtomToolsFramework
         params.device = AZ::RHI::RHISystemInterface::Get()->GetDevice();
         params.windowHandle = reinterpret_cast<AzFramework::NativeWindowHandle>(winId());
         params.id = id;
+        AzFramework::WindowRequestBus::Handler::BusConnect(params.windowHandle);
         m_viewportContext = viewportContextManager->CreateViewportContext(AZ::Name(), params);
 
         SetControllerList(AZStd::make_shared<AzFramework::ViewportControllerList>());

Gems/AtomLyIntegration/AtomBridge/Code/CMakeLists.txt

@@ -21,7 +21,7 @@ ly_add_target(
             Include
     COMPILE_DEFINITIONS
         PRIVATE
-            ENABLE_ATOM_DEBUG_DISPLAY=0
+            ENABLE_ATOM_DEBUG_DISPLAY=1
     BUILD_DEPENDENCIES
         PUBLIC
             AZ::AtomCore
@@ -43,7 +43,7 @@ ly_add_target(
             Include
     COMPILE_DEFINITIONS
         PRIVATE
-            ENABLE_ATOM_DEBUG_DISPLAY=0
+            ENABLE_ATOM_DEBUG_DISPLAY=1
     BUILD_DEPENDENCIES
         PRIVATE
             Gem::Atom_AtomBridge.Static

Gems/AtomLyIntegration/CryRenderAtomShim/AtomShim_RendPipeline.cpp

@@ -165,7 +165,7 @@ void CAtomShimRenderer::EF_EndEf3D([[maybe_unused]] const int nFlags, [[maybe_un
 
     // Only render the UI Canvas and the Console on the main window
     // If we're not in the editor, don't bother to check viewport.
-    if (!gEnv->IsEditor() || m_currContext->m_isMainViewport)
+    if (!gEnv->IsEditor() || m_currContext == nullptr || m_currContext->m_isMainViewport)
     {
         EBUS_EVENT(AZ::RenderNotificationsBus, OnScene3DEnd);
     }

Gems/AtomLyIntegration/CryRenderAtomShim/AtomShim_Renderer.cpp

@@ -295,7 +295,12 @@ void CAtomShimRenderer::EndFrame()
     if (!m_viewportContext)
     {
         auto viewContextManager = AZ::Interface<AZ::RPI::ViewportContextRequestsInterface>::Get();
-        m_viewportContext = viewContextManager->GetViewportContextByName(viewContextManager->GetDefaultViewportContextName());
+        auto viewportContext = viewContextManager->GetViewportContextByName(viewContextManager->GetDefaultViewportContextName());
+        // If the viewportContext exists and is created with the default ID, we can safely assume control
+        if (viewportContext && viewportContext->GetId() == -10)
+        {
+            m_viewportContext = viewportContext;
+        }
     }
 
     if (m_viewportContext)