diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/API/ViewportEditorModeTrackerNotificationBus.h b/Code/Framework/AzToolsFramework/AzToolsFramework/API/ViewportEditorModeTrackerNotificationBus.h index 42a1cb0113..1f05e869f1 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/API/ViewportEditorModeTrackerNotificationBus.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/API/ViewportEditorModeTrackerNotificationBus.h @@ -9,7 +9,7 @@ #pragma once #include -#include +#include #include namespace AzToolsFramework @@ -23,11 +23,11 @@ namespace AzToolsFramework Pick }; - //! Viewport identifier and other relevant viewport data. + //! Viewport editor mode tracker identifier and other relevant data. struct ViewportEditorModeInfo { - using IdType = AzFramework::ViewportId; - IdType m_id = ViewportUi::DefaultViewportId; //!< The unique identifier for a given viewport. + using IdType = AzFramework::EntityContextId; + IdType m_id = AzFramework::EntityContextId::CreateNull(); //!< The unique identifier for a given viewport editor mode tracker. }; //! Interface for the editor modes of a given viewport. diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ComponentMode/ComponentModeCollection.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/ComponentMode/ComponentModeCollection.cpp index 4d849466ee..df93e924b8 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ComponentMode/ComponentModeCollection.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/ComponentMode/ComponentModeCollection.cpp @@ -218,7 +218,7 @@ namespace AzToolsFramework // this call to activate the component mode editor state should eventually replace the bus call in // ComponentModeCollection::BeginComponentMode() to EditorComponentModeNotifications::EnteredComponentMode // such that all of the notifications for activating/deactivating the different editor modes are in a central location - m_viewportEditorModeTracker->ActivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Component); + m_viewportEditorModeTracker->ActivateMode({ GetEntityContextId() }, ViewportEditorMode::Component); // enable actions for the first/primary ComponentMode // note: if multiple ComponentModes are activated at the same time, actions @@ -296,7 +296,7 @@ namespace AzToolsFramework // this call to deactivate the component mode editor state should eventually replace the bus call in // ComponentModeCollection::EndComponentMode() to EditorComponentModeNotifications::LeftComponentMode // such that all of the notifications for activating/deactivating the different editor modes are in a central location - m_viewportEditorModeTracker->DeactivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Component); + m_viewportEditorModeTracker->DeactivateMode({ GetEntityContextId() }, ViewportEditorMode::Component); // clear stored modes and builders for this ComponentMode // TLDR: avoid 'use after free' error diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/FocusMode/FocusModeSystemComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/FocusMode/FocusModeSystemComponent.cpp index 3fafb4db81..2549792375 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/FocusMode/FocusModeSystemComponent.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/FocusMode/FocusModeSystemComponent.cpp @@ -10,7 +10,7 @@ #include #include -#include +#include namespace AzToolsFramework { @@ -72,11 +72,11 @@ namespace AzToolsFramework { if (!m_focusRoot.IsValid() && entityId.IsValid()) { - tracker->ActivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Focus); + tracker->ActivateMode({ GetEntityContextId() }, ViewportEditorMode::Focus); } else if (m_focusRoot.IsValid() && !entityId.IsValid()) { - tracker->DeactivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Focus); + tracker->DeactivateMode({ GetEntityContextId() }, ViewportEditorMode::Focus); } } } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorDefaultSelection.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorDefaultSelection.cpp index 30958cfbc1..1ad0ee8ff3 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorDefaultSelection.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorDefaultSelection.cpp @@ -32,14 +32,14 @@ namespace AzToolsFramework m_manipulatorManager = AZStd::make_shared(AzToolsFramework::g_mainManipulatorManagerId); m_transformComponentSelection = AZStd::make_unique(entityDataCache); - m_viewportEditorModeTracker->ActivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Default); + m_viewportEditorModeTracker->ActivateMode({ GetEntityContextId() }, ViewportEditorMode::Default); } EditorDefaultSelection::~EditorDefaultSelection() { ComponentModeFramework::ComponentModeSystemRequestBus::Handler::BusDisconnect(); ActionOverrideRequestBus::Handler::BusDisconnect(); - m_viewportEditorModeTracker->DeactivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Default); + m_viewportEditorModeTracker->DeactivateMode({ GetEntityContextId() }, ViewportEditorMode::Default); } void EditorDefaultSelection::SetOverridePhantomWidget(QWidget* phantomOverrideWidget) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorPickEntitySelection.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorPickEntitySelection.cpp index 18eda140a0..6e7777b31c 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorPickEntitySelection.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorPickEntitySelection.cpp @@ -21,7 +21,7 @@ namespace AzToolsFramework : m_editorHelpers(AZStd::make_unique(entityDataCache)) , m_viewportEditorModeTracker(viewportEditorModeTracker) { - m_viewportEditorModeTracker->ActivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Pick); + m_viewportEditorModeTracker->ActivateMode({ GetEntityContextId() }, ViewportEditorMode::Pick); } EditorPickEntitySelection::~EditorPickEntitySelection() @@ -31,7 +31,7 @@ namespace AzToolsFramework ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::SetEntityHighlighted, m_hoveredEntityId, false); } - m_viewportEditorModeTracker->DeactivateMode({ /* DefaultViewportId */ }, ViewportEditorMode::Pick); + m_viewportEditorModeTracker->DeactivateMode({ GetEntityContextId() }, ViewportEditorMode::Pick); } // note: entityIdUnderCursor is the authoritative entityId we get each frame by querying diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/ViewportEditorModeTracker.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/ViewportEditorModeTracker.cpp index 105712c789..05b645f52c 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/ViewportEditorModeTracker.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/ViewportEditorModeTracker.cpp @@ -52,7 +52,8 @@ namespace AzToolsFramework if (editorModes.IsModeActive(mode)) { return AZ::Failure(AZStd::string::format( - "Duplicate call to ActivateMode for mode '%u' on id '%i'", static_cast(mode), viewportEditorModeInfo.m_id)); + "Duplicate call to ActivateMode for mode '%u' on id '%s'", static_cast(mode), + viewportEditorModeInfo.m_id.ToString().c_str())); } if (const auto result = editorModes.ActivateMode(mode); @@ -78,7 +79,8 @@ namespace AzToolsFramework if (!editorModes->IsModeActive(mode)) { return AZ::Failure(AZStd::string::format( - "Duplicate call to DeactivateMode for mode '%u' on id '%i'", static_cast(mode), viewportEditorModeInfo.m_id)); + "Duplicate call to DeactivateMode for mode '%u' on id '%s'", static_cast(mode), + viewportEditorModeInfo.m_id.ToString().c_str())); } } else @@ -103,8 +105,8 @@ namespace AzToolsFramework else { return AZ::Failure(AZStd::string::format( - "Call to DeactivateMode for mode '%u' on id '%i' without precursor call to ActivateMode", static_cast(mode), - viewportEditorModeInfo.m_id)); + "Call to DeactivateMode for mode '%u' on id '%s' without precursor call to ActivateMode", static_cast(mode), + viewportEditorModeInfo.m_id.ToString().c_str())); } } diff --git a/Code/Framework/AzToolsFramework/Tests/Viewport/ViewportEditorModeTests.cpp b/Code/Framework/AzToolsFramework/Tests/Viewport/ViewportEditorModeTests.cpp index 866d88b7ba..393a1e9055 100644 --- a/Code/Framework/AzToolsFramework/Tests/Viewport/ViewportEditorModeTests.cpp +++ b/Code/Framework/AzToolsFramework/Tests/Viewport/ViewportEditorModeTests.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -18,7 +19,7 @@ namespace UnitTest using ViewportEditorModes = AzToolsFramework::ViewportEditorModes; using ViewportEditorModeTracker = AzToolsFramework::ViewportEditorModeTracker; using ViewportEditorModeInfo = AzToolsFramework::ViewportEditorModeInfo; - using ViewportId = ViewportEditorModeInfo::IdType; + using TrackerId = ViewportEditorModeInfo::IdType; using ViewportEditorModesInterface = AzToolsFramework::ViewportEditorModesInterface; using ViewportEditorModeTrackerInterface = AzToolsFramework::ViewportEditorModeTrackerInterface; @@ -113,10 +114,10 @@ namespace UnitTest using EditModeTracker = AZStd::unordered_map; - ViewportEditorModeNotificationsBusHandler(ViewportId viewportId) - : m_viewportSubscription(viewportId) + ViewportEditorModeNotificationsBusHandler(TrackerId id) + : m_trackerSubscription(id) { - AzToolsFramework::ViewportEditorModeNotificationsBus::Handler::BusConnect(m_viewportSubscription); + AzToolsFramework::ViewportEditorModeNotificationsBus::Handler::BusConnect(m_trackerSubscription); } ~ViewportEditorModeNotificationsBusHandler() @@ -124,11 +125,6 @@ namespace UnitTest AzToolsFramework::ViewportEditorModeNotificationsBus::Handler::BusDisconnect(); } - ViewportId GetViewportSubscription() const - { - return m_viewportSubscription; - } - const EditModeTracker& GetEditorModes() const { return m_editorModes; @@ -145,7 +141,7 @@ namespace UnitTest } private: - ViewportId m_viewportSubscription; + TrackerId m_trackerSubscription; EditModeTracker m_editorModes; }; @@ -158,10 +154,13 @@ namespace UnitTest void SetUpEditorFixtureImpl() override { + m_handlerIds.resize(ViewportEditorModes::NumEditorModes); for (auto mode = 0; mode < ViewportEditorModes::NumEditorModes; mode++) { - m_editorModeHandlers[mode] = AZStd::make_unique(mode); + m_handlerIds[mode] = TrackerId::CreateRandom(); + m_editorModeHandlers[mode] = AZStd::make_unique(m_handlerIds[mode]); } + } void TearDownEditorFixtureImpl() override @@ -173,6 +172,7 @@ namespace UnitTest } AZStd::array, ViewportEditorModes::NumEditorModes> m_editorModeHandlers; + AZStd::vector m_handlerIds; }; // Fixture for testing the integration of viewport editor mode state tracker @@ -184,7 +184,8 @@ namespace UnitTest { m_viewportEditorModeTracker = AZ::Interface::Get(); ASSERT_NE(m_viewportEditorModeTracker, nullptr); - m_viewportEditorModes = m_viewportEditorModeTracker->GetViewportEditorModes({}); + m_viewportEditorModes = m_viewportEditorModeTracker->GetViewportEditorModes({AzToolsFramework::GetEntityContextId()}); + ASSERT_NE(m_viewportEditorModes, nullptr); } ViewportEditorModeTrackerInterface* m_viewportEditorModeTracker = nullptr; @@ -316,17 +317,17 @@ namespace UnitTest TEST_F(ViewportEditorModeTrackerTestFixture, ActivatingViewportEditorModeForNonExistentIdCreatesViewportEditorModesForThatId) { // Given a viewport not currently being tracked - const ViewportId viewportid = 0; - EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); - EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }), nullptr); + const TrackerId id = 0; + EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); + EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ id }), nullptr); // When a mode is activated for that viewport const auto editorMode = ViewportEditorMode::Default; - m_viewportEditorModeTracker.ActivateMode({ viewportid }, editorMode); - const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }); + m_viewportEditorModeTracker.ActivateMode({ id }, editorMode); + const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ id }); // Expect that viewport to now be tracked - EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); + EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); EXPECT_NE(viewportEditorModeState, nullptr); // Expect the mode for that viewport to be active @@ -336,23 +337,24 @@ namespace UnitTest TEST_F(ViewportEditorModeTrackerTestFixture, DeactivatingViewportEditorModeForNonExistentIdCreatesViewportEditorModesForThatIdButReturnsError) { // Given a viewport not currently being tracked - const ViewportId viewportid = 0; - EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); - EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }), nullptr); + const TrackerId id = 0; + EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); + EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ id }), nullptr); // When a mode is deactivated for that viewport const auto editorMode = ViewportEditorMode::Default; const auto expectedErrorMsg = AZStd::string::format( - "Call to DeactivateMode for mode '%u' on id '%i' without precursor call to ActivateMode", static_cast(editorMode), viewportid); - const auto result = m_viewportEditorModeTracker.DeactivateMode({ viewportid }, editorMode); + "Call to DeactivateMode for mode '%u' on id '%s' without precursor call to ActivateMode", static_cast(editorMode), + id.ToString().c_str()); + const auto result = m_viewportEditorModeTracker.DeactivateMode({ id }, editorMode); // Expect an error due to no precursor activation of that mode EXPECT_FALSE(result.IsSuccess()); EXPECT_EQ(result.GetError(), expectedErrorMsg); // Expect that viewport to now be tracked - const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }); - EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); + const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ id }); + EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); // Expect the mode for that viewport to be inactive EXPECT_NE(viewportEditorModeState, nullptr); @@ -361,45 +363,46 @@ namespace UnitTest TEST_F(ViewportEditorModeTrackerTestFixture, GettingNonExistentViewportEditorModesForIdReturnsNull) { - const ViewportId viewportid = 0; - EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); - EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }), nullptr); + const TrackerId id = 0; + EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); + EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ id }), nullptr); } TEST_F(ViewportEditorModeTrackerTestFixture, ActivatingViewportEditorModesForExistingIdInThatStateReturnsError) { // Given a viewport not currently tracked - const ViewportId viewportid = 0; - EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); - EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }), nullptr); + const TrackerId id = 0; + EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); + EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ id }), nullptr); const auto editorMode = ViewportEditorMode::Default; { // When the mode is activated for the viewport - const auto result = m_viewportEditorModeTracker.ActivateMode({ viewportid }, editorMode); + const auto result = m_viewportEditorModeTracker.ActivateMode({ id }, editorMode); // Expect no error as there is no duplicate activation EXPECT_TRUE(result.IsSuccess()); // Expect the mode to be active for the viewport - const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }); - EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); + const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ id }); + EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); EXPECT_NE(viewportEditorModeState, nullptr); EXPECT_TRUE(viewportEditorModeState->IsModeActive(editorMode)); } { // When the mode is activated again for the viewport - const auto result = m_viewportEditorModeTracker.ActivateMode({ viewportid }, editorMode); + const auto result = m_viewportEditorModeTracker.ActivateMode({ id }, editorMode); // Expect an error for the duplicate activation const auto expectedErrorMsg = AZStd::string::format( - "Duplicate call to ActivateMode for mode '%u' on id '%i'", static_cast(editorMode), viewportid); + "Duplicate call to ActivateMode for mode '%u' on id '%s'", static_cast(editorMode), + id.ToString().c_str()); EXPECT_FALSE(result.IsSuccess()); EXPECT_EQ(result.GetError(), expectedErrorMsg); // Expect the mode to still be active for the viewport - const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }); - EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); + const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ id }); + EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); EXPECT_NE(viewportEditorModeState, nullptr); EXPECT_TRUE(viewportEditorModeState->IsModeActive(editorMode)); } @@ -408,38 +411,39 @@ namespace UnitTest TEST_F(ViewportEditorModeTrackerTestFixture, DeactivatingViewportEditorModesForExistingIdNotInThatStateReturnssError) { // Given a viewport not currently tracked - const ViewportId viewportid = 0; - EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); - EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }), nullptr); + const TrackerId id = 0; + EXPECT_FALSE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); + EXPECT_EQ(m_viewportEditorModeTracker.GetViewportEditorModes({ id }), nullptr); const auto editorMode = ViewportEditorMode::Default; { // When the mode is activated and then deactivated for the viewport - m_viewportEditorModeTracker.ActivateMode({ viewportid }, editorMode); - const auto result = m_viewportEditorModeTracker.DeactivateMode({ viewportid }, editorMode); + m_viewportEditorModeTracker.ActivateMode({ id }, editorMode); + const auto result = m_viewportEditorModeTracker.DeactivateMode({ id }, editorMode); // Expect no error as there is no duplicate deactivation EXPECT_TRUE(result.IsSuccess()); // Expect the mode to be inctive for the viewport - const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }); - EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); + const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ id }); + EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); EXPECT_NE(viewportEditorModeState, nullptr); EXPECT_FALSE(viewportEditorModeState->IsModeActive(editorMode)); } { // When the mode is deactivated again for the viewport - const auto result = m_viewportEditorModeTracker.DeactivateMode({ viewportid }, editorMode); + const auto result = m_viewportEditorModeTracker.DeactivateMode({ id }, editorMode); // Expect an error for the duplicate deactivation const auto expectedErrorMsg = AZStd::string::format( - "Duplicate call to DeactivateMode for mode '%u' on id '%i'", static_cast(editorMode), viewportid); + "Duplicate call to DeactivateMode for mode '%u' on id '%s'", static_cast(editorMode), + id.ToString().c_str()); EXPECT_FALSE(result.IsSuccess()); EXPECT_EQ(result.GetError(), expectedErrorMsg); // Expect the mode to still be inactive for the viewport - const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ viewportid }); - EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ viewportid })); + const auto* viewportEditorModeState = m_viewportEditorModeTracker.GetViewportEditorModes({ id }); + EXPECT_TRUE(m_viewportEditorModeTracker.IsViewportModeTracked({ id })); EXPECT_NE(viewportEditorModeState, nullptr); EXPECT_FALSE(viewportEditorModeState->IsModeActive(editorMode)); } @@ -459,9 +463,9 @@ namespace UnitTest // When each editor mode is activated by the state tracker for a specific viewport for (auto mode = 0; mode < ViewportEditorModes::NumEditorModes; mode++) { - const ViewportId viewportId = mode; + const TrackerId id = m_handlerIds[mode]; const ViewportEditorMode editorMode = static_cast(mode); - m_viewportEditorModeTracker.ActivateMode({ viewportId }, editorMode); + m_viewportEditorModeTracker.ActivateMode({ id }, editorMode); } for (auto mode = 0; mode < ViewportEditorModes::NumEditorModes; mode++) @@ -491,10 +495,10 @@ namespace UnitTest // When each editor mode is activated deactivated by the state tracker for a specific viewport for (auto mode = 0; mode < ViewportEditorModes::NumEditorModes; mode++) { - const ViewportId viewportId = mode; + const TrackerId id = m_handlerIds[mode]; const ViewportEditorMode editorMode = static_cast(mode); - m_viewportEditorModeTracker.ActivateMode({ viewportId }, editorMode); - m_viewportEditorModeTracker.DeactivateMode({ viewportId }, editorMode); + m_viewportEditorModeTracker.ActivateMode({ id }, editorMode); + m_viewportEditorModeTracker.DeactivateMode({ id }, editorMode); } for (auto mode = 0; mode < ViewportEditorModes::NumEditorModes; mode++)