From ae735ad3381d0d51c3b11647c5828d8c5504ec7f Mon Sep 17 00:00:00 2001 From: Jonny Gallowy Date: Tue, 14 Sep 2021 14:26:50 -0500 Subject: [PATCH 1/6] new postfx layer categories and ordering values Signed-off-by: Jonny Gallowy --- .../PostProcess/default.postfxlayercategories | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories b/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories index 33adccfd33..d6bffb0e6a 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories +++ b/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories @@ -1,17 +1,30 @@ + - - + + - - + + + + + + + + + + - + + + + + From 52095e3e165ccc0d1cab0d11da74e882efa73919 Mon Sep 17 00:00:00 2001 From: hultonha <82228511+hultonha@users.noreply.github.com> Date: Wed, 15 Sep 2021 11:12:26 +0100 Subject: [PATCH 2/6] Ensure undo/redo operation for change entity selection is atomic (#4122) --- .../EditorTransformComponentSelection.cpp | 258 +++++++++--------- ...EditorTransformComponentSelectionTests.cpp | 28 +- 2 files changed, 158 insertions(+), 128 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp index 5a7b097e29..064fbb9da6 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp @@ -86,59 +86,60 @@ namespace AzToolsFramework "Sticky select implies a single click will not change selection with an entity already selected"); // strings related to new viewport interaction model (EditorTransformComponentSelection) - static const char* const s_togglePivotTitleRightClick = "Toggle pivot"; - static const char* const s_togglePivotTitleEditMenu = "Toggle Pivot Location"; - static const char* const s_togglePivotDesc = "Toggle pivot location"; - static const char* const s_manipulatorUndoRedoName = "Manipulator Adjustment"; - static const char* const s_lockSelectionTitle = "Lock Selection"; - static const char* const s_lockSelectionDesc = "Lock the selected entities so that they can't be selected in the viewport"; - static const char* const s_hideSelectionTitle = "Hide Selection"; - static const char* const s_hideSelectionDesc = "Hide the selected entities so that they don't appear in the viewport"; - static const char* const s_unlockAllTitle = "Unlock All Entities"; - static const char* const s_unlockAllDesc = "Unlock all entities the level"; - static const char* const s_showAllTitle = "Show All"; - static const char* const s_showAllDesc = "Show all entities so that they appear in the viewport"; - static const char* const s_selectAllTitle = "Select All"; - static const char* const s_selectAllDesc = "Select all entities"; - static const char* const s_invertSelectionTitle = "Invert Selection"; - static const char* const s_invertSelectionDesc = "Invert the current entity selection"; - static const char* const s_duplicateTitle = "Duplicate"; - static const char* const s_duplicateDesc = "Duplicate selected entities"; - static const char* const s_deleteTitle = "Delete"; - static const char* const s_deleteDesc = "Delete selected entities"; - static const char* const s_resetEntityTransformTitle = "Reset Entity Transform"; - static const char* const s_resetEntityTransformDesc = "Reset transform based on manipulator mode"; - static const char* const s_resetManipulatorTitle = "Reset Manipulator"; - static const char* const s_resetManipulatorDesc = "Reset the manipulator to recenter it on the selected entity"; - static const char* const s_resetTransformLocalTitle = "Reset Transform (Local)"; - static const char* const s_resetTransformLocalDesc = "Reset transform to local space"; - static const char* const s_resetTransformWorldTitle = "Reset Transform (World)"; - static const char* const s_resetTransformWorldDesc = "Reset transform to world space"; + static const char* const TogglePivotTitleRightClick = "Toggle pivot"; + static const char* const TogglePivotTitleEditMenu = "Toggle Pivot Location"; + static const char* const TogglePivotDesc = "Toggle pivot location"; + static const char* const ManipulatorUndoRedoName = "Manipulator Adjustment"; + static const char* const LockSelectionTitle = "Lock Selection"; + static const char* const LockSelectionDesc = "Lock the selected entities so that they can't be selected in the viewport"; + static const char* const HideSelectionTitle = "Hide Selection"; + static const char* const HideSelectionDesc = "Hide the selected entities so that they don't appear in the viewport"; + static const char* const UnlockAllTitle = "Unlock All Entities"; + static const char* const UnlockAllDesc = "Unlock all entities the level"; + static const char* const ShowAllTitle = "Show All"; + static const char* const ShowAllDesc = "Show all entities so that they appear in the viewport"; + static const char* const SelectAllTitle = "Select All"; + static const char* const SelectAllDesc = "Select all entities"; + static const char* const InvertSelectionTitle = "Invert Selection"; + static const char* const InvertSelectionDesc = "Invert the current entity selection"; + static const char* const DuplicateTitle = "Duplicate"; + static const char* const DuplicateDesc = "Duplicate selected entities"; + static const char* const DeleteTitle = "Delete"; + static const char* const DeleteDesc = "Delete selected entities"; + static const char* const ResetEntityTransformTitle = "Reset Entity Transform"; + static const char* const ResetEntityTransformDesc = "Reset transform based on manipulator mode"; + static const char* const ResetManipulatorTitle = "Reset Manipulator"; + static const char* const ResetManipulatorDesc = "Reset the manipulator to recenter it on the selected entity"; + static const char* const ResetTransformLocalTitle = "Reset Transform (Local)"; + static const char* const ResetTransformLocalDesc = "Reset transform to local space"; + static const char* const ResetTransformWorldTitle = "Reset Transform (World)"; + static const char* const ResetTransformWorldDesc = "Reset transform to world space"; - static const char* const s_entityBoxSelectUndoRedoDesc = "Box Select Entities"; - static const char* const s_entityDeselectUndoRedoDesc = "Deselect Entity"; - static const char* const s_entitiesDeselectUndoRedoDesc = "Deselect Entities"; - static const char* const s_entitySelectUndoRedoDesc = "Select Entity"; - static const char* const s_dittoManipulatorUndoRedoDesc = "Ditto Manipulator"; - static const char* const s_resetManipulatorTranslationUndoRedoDesc = "Reset Manipulator Translation"; - static const char* const s_resetManipulatorOrientationUndoRedoDesc = "Reset Manipulator Orientation"; - static const char* const s_dittoEntityOrientationIndividualUndoRedoDesc = "Ditto orientation individual"; - static const char* const s_dittoEntityOrientationGroupUndoRedoDesc = "Ditto orientation group"; - static const char* const s_resetTranslationToParentUndoRedoDesc = "Reset translation to parent"; - static const char* const s_resetOrientationToParentUndoRedoDesc = "Reset orientation to parent"; - static const char* const s_dittoTranslationGroupUndoRedoDesc = "Ditto translation group"; - static const char* const s_dittoTranslationIndividualUndoRedoDesc = "Ditto translation individual"; - static const char* const s_dittoScaleIndividualWorldUndoRedoDesc = "Ditto scale individual world"; - static const char* const s_dittoScaleIndividualLocalUndoRedoDesc = "Ditto scale individual local"; - static const char* const s_snapToWorldGridUndoRedoDesc = "Snap to world grid"; - static const char* const s_showAllEntitiesUndoRedoDesc = s_showAllTitle; - static const char* const s_lockSelectionUndoRedoDesc = s_lockSelectionTitle; - static const char* const s_hideSelectionUndoRedoDesc = s_hideSelectionTitle; - static const char* const s_unlockAllUndoRedoDesc = s_unlockAllTitle; - static const char* const s_selectAllEntitiesUndoRedoDesc = s_selectAllTitle; - static const char* const s_invertSelectionUndoRedoDesc = s_invertSelectionTitle; - static const char* const s_duplicateUndoRedoDesc = s_duplicateTitle; - static const char* const s_deleteUndoRedoDesc = s_deleteTitle; + static const char* const EntityBoxSelectUndoRedoDesc = "Box Select Entities"; + static const char* const EntityDeselectUndoRedoDesc = "Deselect Entity"; + static const char* const EntitiesDeselectUndoRedoDesc = "Deselect Entities"; + static const char* const ChangeEntitySelectionUndoRedoDesc = "Change Selected Entity"; + static const char* const EntitySelectUndoRedoDesc = "Select Entity"; + static const char* const DittoManipulatorUndoRedoDesc = "Ditto Manipulator"; + static const char* const ResetManipulatorTranslationUndoRedoDesc = "Reset Manipulator Translation"; + static const char* const ResetManipulatorOrientationUndoRedoDesc = "Reset Manipulator Orientation"; + static const char* const DittoEntityOrientationIndividualUndoRedoDesc = "Ditto orientation individual"; + static const char* const DittoEntityOrientationGroupUndoRedoDesc = "Ditto orientation group"; + static const char* const ResetTranslationToParentUndoRedoDesc = "Reset translation to parent"; + static const char* const ResetOrientationToParentUndoRedoDesc = "Reset orientation to parent"; + static const char* const DittoTranslationGroupUndoRedoDesc = "Ditto translation group"; + static const char* const DittoTranslationIndividualUndoRedoDesc = "Ditto translation individual"; + static const char* const DittoScaleIndividualWorldUndoRedoDesc = "Ditto scale individual world"; + static const char* const DittoScaleIndividualLocalUndoRedoDesc = "Ditto scale individual local"; + static const char* const SnapToWorldGridUndoRedoDesc = "Snap to world grid"; + static const char* const ShowAllEntitiesUndoRedoDesc = ShowAllTitle; + static const char* const LockSelectionUndoRedoDesc = LockSelectionTitle; + static const char* const HideSelectionUndoRedoDesc = HideSelectionTitle; + static const char* const UnlockAllUndoRedoDesc = UnlockAllTitle; + static const char* const SelectAllEntitiesUndoRedoDesc = SelectAllTitle; + static const char* const InvertSelectionUndoRedoDesc = InvertSelectionTitle; + static const char* const DuplicateUndoRedoDesc = DuplicateTitle; + static const char* const DeleteUndoRedoDesc = DeleteTitle; static const char* const TransformModeClusterTranslateTooltip = "Switch to translate mode"; static const char* const TransformModeClusterRotateTooltip = "Switch to rotate mode"; @@ -148,14 +149,14 @@ namespace AzToolsFramework static const char* const SpaceClusterLocalTooltip = "Toggle local space lock"; static const char* const SnappingClusterSnapToWorldTooltip = "Snap selected entities to the world space grid"; - static const AZ::Color s_fadedXAxisColor = AZ::Color(AZ::u8(200), AZ::u8(127), AZ::u8(127), AZ::u8(255)); - static const AZ::Color s_fadedYAxisColor = AZ::Color(AZ::u8(127), AZ::u8(190), AZ::u8(127), AZ::u8(255)); - static const AZ::Color s_fadedZAxisColor = AZ::Color(AZ::u8(120), AZ::u8(120), AZ::u8(180), AZ::u8(255)); + static const AZ::Color FadedXAxisColor = AZ::Color(AZ::u8(200), AZ::u8(127), AZ::u8(127), AZ::u8(255)); + static const AZ::Color FadedYAxisColor = AZ::Color(AZ::u8(127), AZ::u8(190), AZ::u8(127), AZ::u8(255)); + static const AZ::Color FadedZAxisColor = AZ::Color(AZ::u8(120), AZ::u8(120), AZ::u8(180), AZ::u8(255)); - static const AZ::Color s_pickedOrientationColor = AZ::Color(0.0f, 1.0f, 0.0f, 1.0f); - static const AZ::Color s_selectedEntityAabbColor = AZ::Color(0.6f, 0.6f, 0.6f, 0.4f); + static const AZ::Color PickedOrientationColor = AZ::Color(0.0f, 1.0f, 0.0f, 1.0f); + static const AZ::Color SelectedEntityAabbColor = AZ::Color(0.6f, 0.6f, 0.6f, 0.4f); - static const float s_pivotSize = 0.075f; // the size of the pivot (box) to render when selected + static const float PivotSize = 0.075f; // the size of the pivot (box) to render when selected // data passed to manipulators when processing mouse interactions // m_entityIds should be sorted based on the entity hierarchy @@ -1107,7 +1108,7 @@ namespace AzToolsFramework { // begin selection undo/redo command entityBoxSelectData->m_boxSelectSelectionCommand = - AZStd::make_unique(EntityIdList(), s_entityBoxSelectUndoRedoDesc); + AZStd::make_unique(EntityIdList(), EntityBoxSelectUndoRedoDesc); // grab currently selected entities entityBoxSelectData->m_selectedEntityIdsBeforeBoxSelect = m_selectedEntityIds; }); @@ -1131,7 +1132,7 @@ namespace AzToolsFramework if (!entityBoxSelectData->m_potentialDeselectedEntityIds.empty() || !entityBoxSelectData->m_potentialSelectedEntityIds.empty()) { - ScopedUndoBatch undoBatch(s_entityBoxSelectUndoRedoDesc); + ScopedUndoBatch undoBatch(EntityBoxSelectUndoRedoDesc); // restore manipulator overrides when undoing if (m_entityIdManipulators.m_manipulators && m_selectedEntityIds.empty()) @@ -1174,7 +1175,7 @@ namespace AzToolsFramework } debugDisplay.DepthTestOff(); - debugDisplay.SetColor(s_selectedEntityAabbColor); + debugDisplay.SetColor(SelectedEntityAabbColor); for (AZ::EntityId entityId : entityBoxSelectData->m_potentialSelectedEntityIds) { @@ -1222,7 +1223,7 @@ namespace AzToolsFramework { // check here if translation or orientation override are set m_manipulatorMoveCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); } } @@ -1696,7 +1697,7 @@ namespace AzToolsFramework if (!UndoRedoOperationInProgress()) { - ScopedUndoBatch undoBatch(s_entitiesDeselectUndoRedoDesc); + ScopedUndoBatch undoBatch(EntitiesDeselectUndoRedoDesc); // restore manipulator overrides when undoing if (m_entityIdManipulators.m_manipulators) @@ -1706,7 +1707,7 @@ namespace AzToolsFramework // select must happen after to ensure in the undo/redo step the selection command // happens before the manipulator command - auto selectionCommand = AZStd::make_unique(EntityIdList(), s_entitiesDeselectUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(EntityIdList(), EntitiesDeselectUndoRedoDesc); selectionCommand->SetParent(undoBatch.GetUndoBatch()); selectionCommand.release(); } @@ -1732,7 +1733,7 @@ namespace AzToolsFramework const auto nextEntityIds = EntityIdVectorFromContainer(m_selectedEntityIds); - ScopedUndoBatch undoBatch(s_entityDeselectUndoRedoDesc); + ScopedUndoBatch undoBatch(EntityDeselectUndoRedoDesc); // store manipulator state when removing last entity from selection if (m_entityIdManipulators.m_manipulators && nextEntityIds.empty()) @@ -1740,7 +1741,7 @@ namespace AzToolsFramework CreateEntityManipulatorDeselectCommand(undoBatch); } - auto selectionCommand = AZStd::make_unique(nextEntityIds, s_entityDeselectUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(nextEntityIds, EntityDeselectUndoRedoDesc); selectionCommand->SetParent(undoBatch.GetUndoBatch()); selectionCommand.release(); @@ -1755,8 +1756,8 @@ namespace AzToolsFramework const auto nextEntityIds = EntityIdVectorFromContainer(m_selectedEntityIds); - ScopedUndoBatch undoBatch(s_entitySelectUndoRedoDesc); - auto selectionCommand = AZStd::make_unique(nextEntityIds, s_entitySelectUndoRedoDesc); + ScopedUndoBatch undoBatch(EntitySelectUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(nextEntityIds, EntitySelectUndoRedoDesc); selectionCommand->SetParent(undoBatch.GetUndoBatch()); selectionCommand.release(); @@ -1772,6 +1773,13 @@ namespace AzToolsFramework void EditorTransformComponentSelection::ChangeSelectedEntity(const AZ::EntityId entityId) { + AZ_Assert( + !UndoRedoOperationInProgress(), + "ChangeSelectedEntity called from undo/redo operation - this is unexpected and not currently supported"); + + // ensure deselect/select is tracked as an atomic undo/redo operation + ScopedUndoBatch undoBatch(ChangeEntitySelectionUndoRedoDesc); + DeselectEntities(); SelectDeselect(entityId); } @@ -1799,7 +1807,7 @@ namespace AzToolsFramework const AZ::Transform& worldFromLocal = m_entityDataCache->GetVisibleEntityTransform(*entityIndex); const AZ::Vector3 boxPosition = worldFromLocal.TransformPoint(CalculateCenterOffset(entityId, m_pivotMode)); const AZ::Vector3 scaledSize = - AZ::Vector3(s_pivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState); + AZ::Vector3(PivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState); if (AabbIntersectMouseRay( mouseInteraction.m_mouseInteraction, @@ -2002,10 +2010,10 @@ namespace AzToolsFramework { if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch(s_dittoManipulatorUndoRedoDesc); + ScopedUndoBatch undoBatch(DittoManipulatorUndoRedoDesc); auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); if (entityId.IsValid()) { @@ -2131,7 +2139,7 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_lockSelectionUndoRedoDesc); + ScopedUndoBatch undoBatch(LockSelectionUndoRedoDesc); if (m_entityIdManipulators.m_manipulators) { @@ -2151,7 +2159,7 @@ namespace AzToolsFramework // lock selection AddAction( - m_actions, { QKeySequence(Qt::Key_L) }, LockSelection, s_lockSelectionTitle, s_lockSelectionDesc, + m_actions, { QKeySequence(Qt::Key_L) }, LockSelection, LockSelectionTitle, LockSelectionDesc, [lockUnlock]() { lockUnlock(true); @@ -2159,7 +2167,7 @@ namespace AzToolsFramework // unlock selection AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::Key_L) }, UnlockSelection, s_lockSelectionTitle, s_lockSelectionDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::Key_L) }, UnlockSelection, LockSelectionTitle, LockSelectionDesc, [lockUnlock]() { lockUnlock(false); @@ -2169,7 +2177,7 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_hideSelectionUndoRedoDesc); + ScopedUndoBatch undoBatch(HideSelectionUndoRedoDesc); if (m_entityIdManipulators.m_manipulators) { @@ -2189,7 +2197,7 @@ namespace AzToolsFramework // hide selection AddAction( - m_actions, { QKeySequence(Qt::Key_H) }, HideSelection, s_hideSelectionTitle, s_hideSelectionDesc, + m_actions, { QKeySequence(Qt::Key_H) }, HideSelection, HideSelectionTitle, HideSelectionDesc, [showHide]() { showHide(false); @@ -2197,7 +2205,7 @@ namespace AzToolsFramework // show selection AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::Key_H) }, ShowSelection, s_hideSelectionTitle, s_hideSelectionDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::Key_H) }, ShowSelection, HideSelectionTitle, HideSelectionDesc, [showHide]() { showHide(true); @@ -2205,12 +2213,12 @@ namespace AzToolsFramework // unlock all entities in the level/scene AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_L) }, UnlockAll, s_unlockAllTitle, s_unlockAllDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_L) }, UnlockAll, UnlockAllTitle, UnlockAllDesc, []() { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_unlockAllUndoRedoDesc); + ScopedUndoBatch undoBatch(UnlockAllUndoRedoDesc); EnumerateEditorEntities( [](AZ::EntityId entityId) @@ -2222,12 +2230,12 @@ namespace AzToolsFramework // show all entities in the level/scene AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_H) }, ShowAll, s_showAllTitle, s_showAllDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_H) }, ShowAll, ShowAllTitle, ShowAllDesc, []() { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_showAllEntitiesUndoRedoDesc); + ScopedUndoBatch undoBatch(ShowAllEntitiesUndoRedoDesc); EnumerateEditorEntities( [](AZ::EntityId entityId) @@ -2239,17 +2247,17 @@ namespace AzToolsFramework // select all entities in the level/scene AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::Key_A) }, SelectAll, s_selectAllTitle, s_selectAllDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::Key_A) }, SelectAll, SelectAllTitle, SelectAllDesc, [this]() { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_selectAllEntitiesUndoRedoDesc); + ScopedUndoBatch undoBatch(SelectAllEntitiesUndoRedoDesc); if (m_entityIdManipulators.m_manipulators) { auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); // note, nothing will change that the manipulatorCommand needs to keep track // for after so no need to call SetManipulatorAfter @@ -2269,7 +2277,7 @@ namespace AzToolsFramework auto nextEntityIds = EntityIdVectorFromContainer(m_selectedEntityIds); - auto selectionCommand = AZStd::make_unique(nextEntityIds, s_selectAllEntitiesUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(nextEntityIds, SelectAllEntitiesUndoRedoDesc); selectionCommand->SetParent(undoBatch.GetUndoBatch()); selectionCommand.release(); @@ -2279,17 +2287,17 @@ namespace AzToolsFramework // invert current selection AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_I) }, InvertSelect, s_invertSelectionTitle, s_invertSelectionDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_I) }, InvertSelect, InvertSelectionTitle, InvertSelectionDesc, [this]() { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_invertSelectionUndoRedoDesc); + ScopedUndoBatch undoBatch(InvertSelectionUndoRedoDesc); if (m_entityIdManipulators.m_manipulators) { auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); // note, nothing will change that the manipulatorCommand needs to keep track // for after so no need to call SetManipulatorAfter @@ -2316,7 +2324,7 @@ namespace AzToolsFramework auto nextEntityIds = EntityIdVectorFromContainer(entityIds); - auto selectionCommand = AZStd::make_unique(nextEntityIds, s_invertSelectionUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(nextEntityIds, InvertSelectionUndoRedoDesc); selectionCommand->SetParent(undoBatch.GetUndoBatch()); selectionCommand.release(); @@ -2326,7 +2334,7 @@ namespace AzToolsFramework // duplicate selection AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::Key_D) }, DuplicateSelect, s_duplicateTitle, s_duplicateDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::Key_D) }, DuplicateSelect, DuplicateTitle, DuplicateDesc, []() { AZ_PROFILE_FUNCTION(AzToolsFramework); @@ -2338,8 +2346,8 @@ namespace AzToolsFramework QApplication::focusWidget()->clearFocus(); } - ScopedUndoBatch undoBatch(s_duplicateUndoRedoDesc); - auto selectionCommand = AZStd::make_unique(EntityIdList(), s_duplicateUndoRedoDesc); + ScopedUndoBatch undoBatch(DuplicateUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(EntityIdList(), DuplicateUndoRedoDesc); selectionCommand->SetParent(undoBatch.GetUndoBatch()); selectionCommand.release(); @@ -2351,12 +2359,12 @@ namespace AzToolsFramework // delete selection AddAction( - m_actions, { QKeySequence(Qt::Key_Delete) }, DeleteSelect, s_deleteTitle, s_deleteDesc, + m_actions, { QKeySequence(Qt::Key_Delete) }, DeleteSelect, DeleteTitle, DeleteDesc, [this]() { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_deleteUndoRedoDesc); + ScopedUndoBatch undoBatch(DeleteUndoRedoDesc); CreateEntityManipulatorDeselectCommand(undoBatch); @@ -2375,14 +2383,14 @@ namespace AzToolsFramework }); AddAction( - m_actions, { QKeySequence(Qt::Key_P) }, EditPivot, s_togglePivotTitleEditMenu, s_togglePivotDesc, + m_actions, { QKeySequence(Qt::Key_P) }, EditPivot, TogglePivotTitleEditMenu, TogglePivotDesc, [this]() { ToggleCenterPivotSelection(); }); AddAction( - m_actions, { QKeySequence(Qt::Key_R) }, EditReset, s_resetEntityTransformTitle, s_resetEntityTransformDesc, + m_actions, { QKeySequence(Qt::Key_R) }, EditReset, ResetEntityTransformTitle, ResetEntityTransformDesc, [this]() { switch (m_mode) @@ -2400,11 +2408,11 @@ namespace AzToolsFramework }); AddAction( - m_actions, { QKeySequence(Qt::CTRL + Qt::Key_R) }, EditResetManipulator, s_resetManipulatorTitle, s_resetManipulatorDesc, + m_actions, { QKeySequence(Qt::CTRL + Qt::Key_R) }, EditResetManipulator, ResetManipulatorTitle, ResetManipulatorDesc, AZStd::bind(AZStd::mem_fn(&EditorTransformComponentSelection::DelegateClearManipulatorOverride), this)); AddAction( - m_actions, { QKeySequence(Qt::ALT + Qt::Key_R) }, EditResetLocal, s_resetTransformLocalTitle, s_resetTransformLocalDesc, + m_actions, { QKeySequence(Qt::ALT + Qt::Key_R) }, EditResetLocal, ResetTransformLocalTitle, ResetTransformLocalDesc, [this]() { switch (m_mode) @@ -2422,7 +2430,7 @@ namespace AzToolsFramework }); AddAction( - m_actions, { QKeySequence(Qt::SHIFT + Qt::Key_R) }, EditResetWorld, s_resetTransformWorldTitle, s_resetTransformWorldDesc, + m_actions, { QKeySequence(Qt::SHIFT + Qt::Key_R) }, EditResetWorld, ResetTransformWorldTitle, ResetTransformWorldDesc, [this]() { switch (m_mode) @@ -2431,7 +2439,7 @@ namespace AzToolsFramework { // begin an undo batch so operations inside CopyOrientation... and // DelegateClear... are grouped into a single undo/redo - ScopedUndoBatch undoBatch{ s_resetTransformWorldTitle }; + ScopedUndoBatch undoBatch{ ResetTransformWorldTitle }; CopyOrientationToSelectedEntitiesIndividual(AZ::Quaternion::CreateIdentity()); ClearManipulatorOrientationOverride(); } @@ -2685,7 +2693,7 @@ namespace AzToolsFramework const AZStd::array snapAxes = { AZ::Vector3::CreateAxisX(), AZ::Vector3::CreateAxisY(), AZ::Vector3::CreateAxisZ() }; - ScopedUndoBatch undoBatch(s_snapToWorldGridUndoRedoDesc); + ScopedUndoBatch undoBatch(SnapToWorldGridUndoRedoDesc); for (const AZ::EntityId& entityId : m_selectedEntityIds) { ScopedUndoBatch::MarkEntityDirty(entityId); @@ -2870,10 +2878,10 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch(s_resetManipulatorTranslationUndoRedoDesc); + ScopedUndoBatch undoBatch(ResetManipulatorTranslationUndoRedoDesc); auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); m_pivotOverrideFrame.ResetPickedTranslation(); m_pivotOverrideFrame.m_pickedEntityIdOverride.SetInvalid(); @@ -2896,10 +2904,10 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch{ s_resetManipulatorOrientationUndoRedoDesc }; + ScopedUndoBatch undoBatch{ ResetManipulatorOrientationUndoRedoDesc }; auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); m_pivotOverrideFrame.ResetPickedOrientation(); m_pivotOverrideFrame.m_pickedEntityIdOverride.SetInvalid(); @@ -2961,13 +2969,13 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch(s_dittoTranslationGroupUndoRedoDesc); + ScopedUndoBatch undoBatch(DittoTranslationGroupUndoRedoDesc); // store previous translation manipulator position const AZ::Vector3 previousPivotTranslation = m_entityIdManipulators.m_manipulators->GetLocalTransform().GetTranslation(); auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); // refresh the transform pivot override if it's set if (m_pivotOverrideFrame.m_translationOverride) @@ -3017,10 +3025,10 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch(s_dittoTranslationIndividualUndoRedoDesc); + ScopedUndoBatch undoBatch(DittoTranslationIndividualUndoRedoDesc); auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); // refresh the transform pivot override if it's set if (m_pivotOverrideFrame.m_translationOverride) @@ -3055,7 +3063,7 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_dittoScaleIndividualWorldUndoRedoDesc); + ScopedUndoBatch undoBatch(DittoScaleIndividualWorldUndoRedoDesc); ManipulatorEntityIds manipulatorEntityIds; BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds); @@ -3089,7 +3097,7 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_dittoScaleIndividualLocalUndoRedoDesc); + ScopedUndoBatch undoBatch(DittoScaleIndividualLocalUndoRedoDesc); ManipulatorEntityIds manipulatorEntityIds; BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds); @@ -3110,10 +3118,10 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch{ s_dittoEntityOrientationIndividualUndoRedoDesc }; + ScopedUndoBatch undoBatch{ DittoEntityOrientationIndividualUndoRedoDesc }; auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); ManipulatorEntityIds manipulatorEntityIds; BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds); @@ -3148,10 +3156,10 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch(s_dittoEntityOrientationGroupUndoRedoDesc); + ScopedUndoBatch undoBatch(DittoEntityOrientationGroupUndoRedoDesc); auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); ManipulatorEntityIds manipulatorEntityIds; BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds); @@ -3194,7 +3202,7 @@ namespace AzToolsFramework { AZ_PROFILE_FUNCTION(AzToolsFramework); - ScopedUndoBatch undoBatch(s_resetOrientationToParentUndoRedoDesc); + ScopedUndoBatch undoBatch(ResetOrientationToParentUndoRedoDesc); for (const auto& entityIdLookup : m_entityIdManipulators.m_lookups) { ScopedUndoBatch::MarkEntityDirty(entityIdLookup.first); @@ -3215,7 +3223,7 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { - ScopedUndoBatch undoBatch(s_resetTranslationToParentUndoRedoDesc); + ScopedUndoBatch undoBatch(ResetTranslationToParentUndoRedoDesc); ManipulatorEntityIds manipulatorEntityIds; BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds); @@ -3251,7 +3259,7 @@ namespace AzToolsFramework void EditorTransformComponentSelection::PopulateEditorGlobalContextMenu( QMenu* menu, [[maybe_unused]] const AZ::Vector2& point, [[maybe_unused]] int flags) { - QAction* action = menu->addAction(QObject::tr(s_togglePivotTitleRightClick)); + QAction* action = menu->addAction(QObject::tr(TogglePivotTitleRightClick)); QObject::connect( action, &QAction::triggered, action, [this]() @@ -3329,15 +3337,15 @@ namespace AzToolsFramework : 1.0f; }; - display.SetColor(s_fadedXAxisColor); + display.SetColor(FadedXAxisColor); display.DrawLine( transform.GetTranslation(), transform.GetTranslation() + transform.GetBasisX().GetNormalizedSafe() * axisLength * axisFlip(AZ::Vector3::CreateAxisX())); - display.SetColor(s_fadedYAxisColor); + display.SetColor(FadedYAxisColor); display.DrawLine( transform.GetTranslation(), transform.GetTranslation() + transform.GetBasisY().GetNormalizedSafe() * axisLength * axisFlip(AZ::Vector3::CreateAxisY())); - display.SetColor(s_fadedZAxisColor); + display.SetColor(FadedZAxisColor); display.DrawLine( transform.GetTranslation(), transform.GetTranslation() + transform.GetBasisZ().GetNormalizedSafe() * axisLength * axisFlip(AZ::Vector3::CreateAxisZ())); @@ -3416,11 +3424,11 @@ namespace AzToolsFramework CalculatePivotTranslation(m_pivotOverrideFrame.m_pickedEntityIdOverride, m_pivotMode)); const float scaledSize = - s_pivotSize * CalculateScreenToWorldMultiplier(pickedEntityWorldTransform.GetTranslation(), cameraState); + PivotSize * CalculateScreenToWorldMultiplier(pickedEntityWorldTransform.GetTranslation(), cameraState); debugDisplay.DepthWriteOff(); debugDisplay.DepthTestOff(); - debugDisplay.SetColor(s_pickedOrientationColor); + debugDisplay.SetColor(PickedOrientationColor); debugDisplay.DrawWireSphere(pickedEntityWorldTransform.GetTranslation(), scaledSize); @@ -3462,7 +3470,7 @@ namespace AzToolsFramework const AZ::Vector3 boxPosition = worldFromLocal.TransformPoint(CalculateCenterOffset(entityId, m_pivotMode)); const AZ::Vector3 scaledSize = - AZ::Vector3(s_pivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState); + AZ::Vector3(PivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState); const AZ::Color hiddenNormal[] = { AzFramework::ViewportColors::SelectedColor, AzFramework::ViewportColors::HiddenColor }; @@ -3708,7 +3716,7 @@ namespace AzToolsFramework if (m_entityIdManipulators.m_manipulators) { auto manipulatorCommand = - AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); manipulatorCommand->SetManipulatorAfter(EntityManipulatorCommand::State()); diff --git a/Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp b/Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp index 241ee3f576..72cafe5cd5 100644 --- a/Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp +++ b/Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp @@ -866,9 +866,7 @@ namespace UnitTest EXPECT_THAT(selectedEntitiesAfter, UnorderedElementsAre(m_entityId1)); } - TEST_F( - EditorTransformComponentSelectionViewportPickingManipulatorTestFixture, - BoxSelectWithNoInitialSelectionAddsEntitiesToSelection) + TEST_F(EditorTransformComponentSelectionViewportPickingManipulatorTestFixture, BoxSelectWithNoInitialSelectionAddsEntitiesToSelection) { AzToolsFramework::ed_viewportStickySelect = true; @@ -989,6 +987,30 @@ namespace UnitTest EXPECT_TRUE(selectedEntitiesAfter.empty()); } + TEST_F(EditorTransformComponentSelectionViewportPickingManipulatorTestFixture, UnstickyUndoOperationForChangeInSelectionIsAtomic) + { + AzToolsFramework::ed_viewportStickySelect = false; + + PositionEntities(); + PositionCamera(m_cameraState); + + AzToolsFramework::SelectEntity(m_entityId1); + + // calculate the position in screen space of the second entity + const auto entity2ScreenPosition = AzFramework::WorldToScreen(m_entity2WorldTranslation, m_cameraState); + + // single click select entity2 + m_actionDispatcher->CameraState(m_cameraState)->MousePosition(entity2ScreenPosition)->MouseLButtonDown()->MouseLButtonUp(); + + // undo action + AzToolsFramework::ToolsApplicationRequestBus::Broadcast(&AzToolsFramework::ToolsApplicationRequestBus::Events::UndoPressed); + + // entity1 is selected after undo + using ::testing::UnorderedElementsAre; + auto selectedEntitiesAfter = SelectedEntities(); + EXPECT_THAT(selectedEntitiesAfter, UnorderedElementsAre(m_entityId1)); + } + using EditorTransformComponentSelectionManipulatorTestFixture = IndirectCallManipulatorViewportInteractionFixtureMixin; From 169b8f36793f93d0dcab7902f736617b1952cd4c Mon Sep 17 00:00:00 2001 From: galibzon <66021303+galibzon@users.noreply.github.com> Date: Wed, 15 Sep 2021 07:50:14 -0500 Subject: [PATCH 3/6] [ATOM-5441] Shader Builders May Fail When Multiple New Files Are Added (#3862) * [ATOM-5441] Shader Builders May Fail When Multiple New Files Are Added ShaderAssetBuilder::CreateJobs now recursively parses *.azsl files looking for #include lines and builds the list of source dependencies using a depth-first algorithm. It was using MCPP before but not anymore (during CreateJobs). The new algorithm may over prescribe, but fixes the issues when multiple new shader related files are added, at once or out of order, to a game project or Gem. Overall the new ShaderAssetBuilder::CreateJobs() is around 40% faster and, of course, handles source dependencies in a robust way. * Added new test suite to AutomatedTesting project: Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py Bug fix to Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp discovered thanks to the automated test suite. The idea is that CreateJobs doesn't fail if the AZSL file doesn't exist. The failure is deferred during ProcessJob. This way if the AZSL file exists the .shader file is rebuilt automatically. * For testability purposes and avoid memory leakage errors during Unit Tests created the class ShaderBuilderUtility::IncludedFilesParser Now accepts "# include " with space between '#' and 'include'. Also now accepts the '-' character inside the file path. Added Unit Test to validate all cases of "#include " parsing. * Fixed linux runtime issues for Unit Tests in Atom_Asset_Shader.Tests Signed-off-by: garrieta --- .../PythonTests/atom_renderer/CMakeLists.txt | 11 + .../DependencyValidation.azsl.txt | 55 +++++ .../DependencyValidation.shader.txt | 26 +++ .../ShaderAssetBuilder/Test1Color.azsli.txt | 18 ++ .../ShaderAssetBuilder/Test2Color.azsli.txt | 16 ++ .../ShaderAssetBuilder/Test3Color.azsli.txt | 16 ++ ...pilesShaderAsChainOfDependenciesChanges.py | 188 ++++++++++++++++++ .../test_Atom_ShaderBuildPipelineSuite.py | 19 ++ Gems/Atom/Asset/Shader/Code/CMakeLists.txt | 1 + .../Code/Source/Editor/ShaderAssetBuilder.cpp | 150 +++++++++++--- .../Source/Editor/ShaderBuilderUtility.cpp | 46 +++++ .../Code/Source/Editor/ShaderBuilderUtility.h | 23 +++ .../Code/Tests/ShaderBuilderUtilityTests.cpp | 86 ++++++++ ...om_asset_shader_builders_tests_files.cmake | 1 + .../PostProcessing/FastDepthAwareBlurHor.azsl | 2 +- .../PostProcessing/FastDepthAwareBlurVer.azsl | 2 +- .../Assets/Shaders/PostProcessing/SMAA.azsli | 2 +- 17 files changed, 629 insertions(+), 33 deletions(-) create mode 100644 AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.azsl.txt create mode 100644 AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.shader.txt create mode 100644 AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test1Color.azsli.txt create mode 100644 AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test2Color.azsli.txt create mode 100644 AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test3Color.azsli.txt create mode 100644 AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges.py create mode 100644 AutomatedTesting/Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py create mode 100644 Gems/Atom/Asset/Shader/Code/Tests/ShaderBuilderUtilityTests.cpp diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt index f056623ecd..992420904c 100644 --- a/AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt @@ -46,4 +46,15 @@ if(PAL_TRAIT_BUILD_HOST_TOOLS AND PAL_TRAIT_BUILD_TESTS_SUPPORTED AND AutomatedT AutomatedTesting.Assets Editor ) + ly_add_pytest( + NAME AutomatedTesting::AtomRenderer_HydraTests_ShaderBuildPipeline + TEST_SUITE main + PATH ${CMAKE_CURRENT_LIST_DIR}/test_Atom_ShaderBuildPipelineSuite.py + TEST_SERIAL + TIMEOUT 600 + RUNTIME_DEPENDENCIES + AssetProcessor + AutomatedTesting.Assets + Editor + ) endif() diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.azsl.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.azsl.txt new file mode 100644 index 0000000000..c0b64f40b7 --- /dev/null +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.azsl.txt @@ -0,0 +1,55 @@ +/* + * 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 + * + */ + + /* + This is a dummy shader used to validate detection of "#included files" + */ + +#include + +#include "Test1Color.azsli" +#include + +ShaderResourceGroup DummySrg : SRG_PerDraw +{ + float4 m_color; +} + +struct VSInput +{ + float3 m_position : POSITION; + float4 m_color : COLOR0; +}; + +struct VSOutput +{ + float4 m_position : SV_Position; + float4 m_color : COLOR0; +}; + +VSOutput MainVS(VSInput vsInput) +{ + VSOutput OUT; + OUT.m_position = float4(vsInput.m_position, 1.0); + OUT.m_color = vsInput.m_color; + return OUT; +} + +struct PSOutput +{ + float4 m_color : SV_Target0; +}; + +PSOutput MainPS(VSOutput vsOutput) +{ + PSOutput OUT; + + OUT.m_color = GetTest1Color(DummySrg::m_color) + GetTest3Color(DummySrg::m_color); + + return OUT; +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.shader.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.shader.txt new file mode 100644 index 0000000000..b0eac1783e --- /dev/null +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.shader.txt @@ -0,0 +1,26 @@ +// This is a dummy shader used to validate detection of "#included files" +{ + "Source" : "DependencyValidation.azsl", + + "DepthStencilState" : { + "Depth" : { "Enable" : false, "CompareFunc" : "GreaterEqual" } + }, + + "DrawList" : "forward", + + "ProgramSettings": + { + "EntryPoints": + [ + { + "name": "MainVS", + "type": "Vertex" + }, + { + "name": "MainPS", + "type": "Fragment" + } + ] + } + +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test1Color.azsli.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test1Color.azsli.txt new file mode 100644 index 0000000000..7d097beafb --- /dev/null +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test1Color.azsli.txt @@ -0,0 +1,18 @@ +/* + * 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 + * + */ + + /* + This is a dummy shader used to validate detection of "#included files" + */ + +#include "Test2Color.azsli" + +float4 GetTest1Color(float4 color) +{ + return color + GetTest2Color(color); +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test2Color.azsli.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test2Color.azsli.txt new file mode 100644 index 0000000000..2ef946b947 --- /dev/null +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test2Color.azsli.txt @@ -0,0 +1,16 @@ +/* + * 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 + * + */ + + /* + This is a dummy shader used to validate detection of "#included files" + */ + +float4 GetTest2Color(float4 color) +{ + return color * 0.5; +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test3Color.azsli.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test3Color.azsli.txt new file mode 100644 index 0000000000..73b0cca434 --- /dev/null +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test3Color.azsli.txt @@ -0,0 +1,16 @@ +/* + * 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 + * + */ + + /* + This is a dummy shader used to validate detection of "#included files" + */ + +float4 GetTest3Color(float4 color) +{ + return color * 0.13; +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges.py b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges.py new file mode 100644 index 0000000000..a05420d960 --- /dev/null +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges.py @@ -0,0 +1,188 @@ +""" +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 + +""" + +import os +import shutil + +def _copy_file(src_file, src_path, target_file, target_path): + # type: (str, str, str, str) -> None + """ + Copies the [src_file] located in [src_path] to the [target_file] located at [target_path]. + Leaves the [target_file] unlocked for reading and writing privileges + :param src_file: The source file to copy (file name) + :param src_path: The source file's path + :param target_file: The target file to copy into (file name) + :param target_path: The target file's path + :return: None + """ + target_file_path = os.path.join(target_path, target_file) + src_file_path = os.path.join(src_path, src_file) + if os.path.exists(target_file_path): + fs.unlock_file(target_file_path) + shutil.copyfile(src_file_path, target_file_path) + +def _copy_tmp_files_in_order(src_directory, file_list, dst_directory, wait_time_in_between = 0.0): + # type: (str, list, str, float) -> None + """ + This function assumes that for each file name listed in @file_list + there's file named "@filename.txt" which the original source file + but they will be copied with just the @filename (.txt removed). + """ + for filename in file_list: + src_name = f"{filename}.txt" + _copy_file(src_name, src_directory, filename, dst_directory) + if wait_time_in_between > 0.0: + print(f"Created {filename} in {dst_directory}") + general.idle_wait(wait_time_in_between) + + +def _remove_file(src_file, src_path): + # type: (str, str) -> None + """ + Removes the [src_file] located in [src_path]. + :param src_file: The source file to copy (file name) + :param src_path: The source file's path + :return: None + """ + src_file_path = os.path.join(src_path, src_file) + if os.path.exists(src_file_path): + fs.unlock_file(src_file_path) + os.remove(src_file_path) + + +def _remove_files(directory, file_list): + for filename in file_list: + _remove_file(filename, directory) + + +def _asset_exists(cache_relative_path): + asset_id = azasset.AssetCatalogRequestBus(azbus.Broadcast, "GetAssetIdByPath", cache_relative_path, azmath.Uuid(), False) + return asset_id.is_valid() + +# List of results that we want to check, this is not 100% necessary but it's a good +# practice to make it easier to debug tests. +# Here we define a tuple of tests +class Results(): + azshader_was_removed = ("azshader was removed", "Failed to remove azshader") + azshader_was_compiled = ("azshader was compiled", "Failed to compile azshader") + + +def ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges(): + """ + This test validates [ATOM-5441] Shader Builders May Fail When Multiple New Files Are Added + It creates source assets to compile a particular shader. + 1- The first phase generates the source assets out of order and slowly. The AP should + wakeup each time one of the source dependencies appears but will fail each time. Only when the + last dependency appears then the shader should build successfully. + 2- The second phase is similar as above, except that all source assets will be created + at once and We also expect that in the end the shader is built successfully. + """ + # Required for automated tests + helper.init_idle() + + game_root_path = os.path.normpath(general.get_game_folder()) + game_asset_path = os.path.join(game_root_path, "Assets") + + base_dir = os.path.dirname(__file__) + src_assets_subdir = os.path.join(base_dir, "TestAssets", "ShaderAssetBuilder") + + with Tracer() as error_tracer: + # The script drives the execution of the test, to return the flow back to the editor, + # we will tick it one time + general.idle_wait_frames(1) + + # This is the order in which the source assets should be deployed + # to avoid source dependency issues with the old MCPP-based CreateJobs. + file_list = [ + "Test2Color.azsli", + "Test3Color.azsli", + "Test1Color.azsli", + "DependencyValidation.azsl", + "DependencyValidation.shader" + ] + + reverse_file_list = file_list[::-1] + + # Remove files in reverse order + _remove_files(game_asset_path, reverse_file_list) + + # Wait here until the azshader doesn't exist anymore. + azshader_name = "assets/dependencyvalidation.azshader" + helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0) + + Report.critical_result(Results.azshader_was_removed, not _asset_exists(azshader_name)) + + _copy_tmp_files_in_order(src_assets_subdir, file_list, game_asset_path, 1.0) + + # Give enough time to AP to compile the shader + helper.wait_for_condition(lambda: _asset_exists(azshader_name), 60.0) + + Report.critical_result(Results.azshader_was_compiled, _asset_exists(azshader_name)) + + # The first part was about compiling the shader under normal conditions. + # Let's remove the files from the previous phase and will proceed + # to make the source files visible to the AP in reverse order. The + # ShaderAssetBuilder will only succeed when the last file becomes visible. + _remove_files(game_asset_path, reverse_file_list) + helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0) + Report.critical_result(Results.azshader_was_removed, not _asset_exists(azshader_name)) + + # Remark, if you are running this test manually from the Editor with "pyRunFile", + # You'll notice how the AP issues notifications that it fails to compile the shader + # as the source files are being copied to the "Assets" subfolder. + # Those errors are OK and also expected because We need the AP to wake up as each + # reported source dependency exists. Once the last file is copied then all source + # dependencies are fully satisfied and the shader should compile successfully. + # And this summarizes the importance of this Test: The previous version + # of ShaderAssetBuilder::CreateJobs was incapable of compiling the shader under the conditions + # presented in this test, but with the new version of ShaderAssetBuilder::CreateJobs, which + # doesn't use MCPP for #include files discovery, it should eventually compile the shader + # once all the source files are in place. + _copy_tmp_files_in_order(src_assets_subdir, reverse_file_list, game_asset_path, 3.0) + + # Give enough time to AP to compile the shader + helper.wait_for_condition(lambda: _asset_exists(azshader_name), 60.0) + + Report.critical_result(Results.azshader_was_compiled, _asset_exists(azshader_name)) + + # The last phase of the test puts stress on potential race conditions + # when all required files appear as soon as possible. + + # First Clean up. + # Remove left over files. + _remove_files(game_asset_path, reverse_file_list) + helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0) + Report.critical_result(Results.azshader_was_removed, not _asset_exists(azshader_name)) + + # Now let's copy all the source files to the "Assets" folder as fast as possible. + _copy_tmp_files_in_order(src_assets_subdir, reverse_file_list, game_asset_path) + + # Give enough time to AP to compile the shader + helper.wait_for_condition(lambda: _asset_exists(azshader_name), 60.0) + + Report.critical_result(Results.azshader_was_compiled, _asset_exists(azshader_name)) + + # All good, let's cleanup leftover files before closing the test. + _remove_files(game_asset_path, reverse_file_list) + helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0) + + +if __name__ == "__main__": + # All exposed python bindings are in azlmbr + import azlmbr.legacy.general as general + import azlmbr.bus as azbus + import azlmbr.asset as azasset + import azlmbr.math as azmath + + # Import report and test helper utilities + from editor_python_test_tools.utils import Report + from editor_python_test_tools.utils import TestHelper as helper + from editor_python_test_tools.utils import Tracer + import ly_test_tools.environment.file_system as fs + + Report.start_test(ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges) \ No newline at end of file diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py b/AutomatedTesting/Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py new file mode 100644 index 0000000000..9ef93ea238 --- /dev/null +++ b/AutomatedTesting/Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py @@ -0,0 +1,19 @@ +""" +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 + +Main suite tests for the Shader Build Pipeline. +""" +import pytest +from ly_test_tools import LAUNCHERS +from ly_test_tools.o3de.editor_test import EditorTestSuite, EditorSingleTest + +@pytest.mark.parametrize("project", ["AutomatedTesting"]) +@pytest.mark.parametrize("launcher_platform", ['windows_editor']) +class TestShaderBuildPipelineMain(EditorTestSuite): + """Holds tests for Shader Build Pipeline validation""" + + class ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges(EditorSingleTest): + from .atom_hydra_scripts import hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges as test_module \ No newline at end of file diff --git a/Gems/Atom/Asset/Shader/Code/CMakeLists.txt b/Gems/Atom/Asset/Shader/Code/CMakeLists.txt index c0bdfd4a8b..efbedb0a38 100644 --- a/Gems/Atom/Asset/Shader/Code/CMakeLists.txt +++ b/Gems/Atom/Asset/Shader/Code/CMakeLists.txt @@ -65,6 +65,7 @@ ly_add_target( AZ::AzFramework AZ::AzToolsFramework Gem::Atom_RHI.Edit + Gem::Atom_RPI.Edit Gem::Atom_RPI.Public ) diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp index 6673da2be3..2babba1ecc 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp @@ -61,10 +61,99 @@ namespace AZ static constexpr char ShaderAssetBuilderName[] = "ShaderAssetBuilder"; static constexpr uint32_t ShaderAssetBuildTimestampParam = 0; + //! The search will start in @currentFolderPath. + //! if the file is not found then it searches in order of appearence in @includeDirectories. + //! If the search yields no existing file it returns an empty string. + static AZStd::string DiscoverFullPath(AZStd::string_view normalizedRelativePath, AZStd::string_view currentFolderPath, const AZStd::vector& includeDirectories) + { + AZStd::string fullPath; + AzFramework::StringFunc::Path::Join(currentFolderPath.data(), normalizedRelativePath.data(), fullPath); + if (AZ::IO::SystemFile::Exists(fullPath.c_str())) + { + return fullPath; + } + + for (const auto &includeDir : includeDirectories) + { + AzFramework::StringFunc::Path::Join(includeDir.c_str(), normalizedRelativePath.data(), fullPath); + if (AZ::IO::SystemFile::Exists(fullPath.c_str())) + { + return fullPath; + } + } + + return ""; + } + + // Appends to @includedFiles normalized paths of possible future locations of the file @normalizedRelativePath. + // The future locations are each directory listed in @includeDirectories joined with @normalizedRelativePath. + // This function is called when an included file doesn't exist but We need to declare source dependency so a .shader + // asset is rebuilt when the missing file appears in the future. + static void AppendListOfPossibleFutureLocations(AZStd::unordered_set& includedFiles, AZStd::string_view normalizedRelativePath, AZStd::string_view currentFolderPath, const AZStd::vector& includeDirectories) + { + AZStd::string fullPath; + AzFramework::StringFunc::Path::Join(currentFolderPath.data(), normalizedRelativePath.data(), fullPath); + includedFiles.insert(fullPath); + for (const auto &includeDir : includeDirectories) + { + AzFramework::StringFunc::Path::Join(includeDir.c_str(), normalizedRelativePath.data(), fullPath); + includedFiles.insert(fullPath); + } + } + + //! Parses, using depth-first recursive approach, azsl files. Looks for '#include ' or '#include "foo/bar/blah.h"' lines + //! and in turn parses the included files. + //! The included files are searched in the directories listed in @includeDirectories. Basically it's a similar approach + //! as how most C-preprocessors would find included files. + static void GetListOfIncludedFiles(AZStd::string_view sourceFilePath, const AZStd::vector& includeDirectories, + const ShaderBuilderUtility::IncludedFilesParser& includedFilesParser, AZStd::unordered_set& includedFiles) + { + auto outcome = includedFilesParser.ParseFileAndGetIncludedFiles(sourceFilePath); + if (!outcome.IsSuccess()) + { + AZ_Warning(ShaderAssetBuilderName, false, outcome.GetError().c_str()); + return; + } + + // Cache the path of the folder where @sourceFilePath is located. + AZStd::string sourceFileFolderPath; + { + AZStd::string drive; + AzFramework::StringFunc::Path::Split(sourceFilePath.data(), &drive, &sourceFileFolderPath); + if (!drive.empty()) + { + AzFramework::StringFunc::Path::Join(drive.c_str(), sourceFileFolderPath.c_str(), sourceFileFolderPath); + } + } + + auto listOfRelativePaths = outcome.TakeValue(); + for (auto relativePath : listOfRelativePaths) + { + auto fullPath = DiscoverFullPath(relativePath, sourceFileFolderPath, includeDirectories); + if (fullPath.empty()) + { + // The file doesn't exist in any of the includeDirectories. It doesn't exist in @sourceFileFolderPath either. + // The file may appear in the future in one of those directories, We must build an exhaustive list + // of full file paths where the file may appear in the future. + AppendListOfPossibleFutureLocations(includedFiles, relativePath, sourceFileFolderPath, includeDirectories); + continue; + } + + // Add the file to the list and keep parsing recursively. + if (includedFiles.count(fullPath)) + { + continue; + } + includedFiles.insert(fullPath); + GetListOfIncludedFiles(fullPath, includeDirectories, includedFilesParser, includedFiles); + } + } + void ShaderAssetBuilder::CreateJobs(const AssetBuilderSDK::CreateJobsRequest& request, AssetBuilderSDK::CreateJobsResponse& response) const { AZStd::string fullPath; AzFramework::StringFunc::Path::ConstructFull(request.m_watchFolder.data(), request.m_sourceFile.data(), fullPath, true); + ShaderBuilderUtility::IncludedFilesParser includedFilesParser; AZ_TracePrintf(ShaderAssetBuilderName, "CreateJobs for Shader \"%s\"\n", fullPath.data()); @@ -90,36 +179,6 @@ namespace AZ AZStd::string azslFullPath; ShaderBuilderUtility::GetAbsolutePathToAzslFile(fullPath, shaderSourceData.m_source, azslFullPath); - if (!IO::FileIOBase::GetInstance()->Exists(azslFullPath.c_str())) - { - AZ_Error( - ShaderAssetBuilderName, false, "Shader program listed as the source entry does not exist: %s.", azslFullPath.c_str()); - response.m_result = AssetBuilderSDK::CreateJobsResultCode::Failed; - return; - } - - - GlobalBuildOptions buildOptions = ReadBuildOptions(ShaderAssetBuilderName); - - // [GFX TODO] [ATOM-14966] In principle, based on macro definitions, included files can change per supervariant. - // So, the list of source asset dependencies must be collected by running MCPP on each supervariant. - // For now, we will run MCPP only once because CreateJobs() should be as light as possible. - // - // Regardless of the PlatformInfo and enabled ShaderPlatformInterfaces, the azsl file will be preprocessed - // with the sole purpose of extracting all included files. For each included file a SourceDependency will be declared. - PreprocessorData output; - buildOptions.m_compilerArguments.Merge(shaderSourceData.m_compiler); - PreprocessFile(azslFullPath, output, buildOptions.m_preprocessorSettings, true, true); - for (auto includePath : output.includedPaths) - { - // m_sourceFileDependencyList does not support paths with "." or ".." for relative lookup, but the preprocessor - // may produce path strings like "C:/a/b/c/../../d/file.azsli" so we have to normalize - AzFramework::StringFunc::Path::Normalize(includePath); - - AssetBuilderSDK::SourceFileDependency includeFileDependency; - includeFileDependency.m_sourceFileDependencyPath = includePath; - response.m_sourceFileDependencyList.emplace_back(includeFileDependency); - } { // Add the AZSL as source dependency @@ -128,6 +187,26 @@ namespace AZ response.m_sourceFileDependencyList.emplace_back(azslFileDependency); } + if (!IO::FileIOBase::GetInstance()->Exists(azslFullPath.c_str())) + { + AZ_Error( + ShaderAssetBuilderName, false, "Shader program listed as the source entry does not exist: %s.", azslFullPath.c_str()); + // Treat as success, so when the azsl file shows up the AP will try to recompile. + response.m_result = AssetBuilderSDK::CreateJobsResultCode::Success; + return; + } + + GlobalBuildOptions buildOptions = ReadBuildOptions(ShaderAssetBuilderName); + + AZStd::unordered_set includedFiles; + GetListOfIncludedFiles(azslFullPath, buildOptions.m_preprocessorSettings.m_projectIncludePaths, includedFilesParser, includedFiles); + for (auto includePath : includedFiles) + { + AssetBuilderSDK::SourceFileDependency includeFileDependency; + includeFileDependency.m_sourceFileDependencyPath = includePath; + response.m_sourceFileDependencyList.emplace_back(includeFileDependency); + } + for (const AssetBuilderSDK::PlatformInfo& platformInfo : request.m_enabledPlatforms) { AZ_TraceContext("For platform", platformInfo.m_identifier.data()); @@ -149,6 +228,10 @@ namespace AZ response.m_createJobOutputs.push_back(jobDescriptor); } // for all request.m_enabledPlatforms + const AZStd::sys_time_t createJobsEndStamp = AZStd::GetTimeNowMicroSecond(); + const u64 createJobDurationMicros = createJobsEndStamp - shaderAssetBuildTimestamp; + AZ_TracePrintf(ShaderAssetBuilderName, "CreateJobs for %s took %llu microseconds", fullPath.c_str(), createJobDurationMicros ); + response.m_result = AssetBuilderSDK::CreateJobsResultCode::Success; } @@ -286,6 +369,13 @@ namespace AZ return; } } + else + { + // CreateJobs was not successful if there's no timestamp property in m_jobParameters. + response.m_resultCode = AssetBuilderSDK::ProcessJobResult_Failed; + AZ_Assert(false, "Missing ShaderAssetBuildTimestampParam"); + return; + } auto supervariantList = ShaderBuilderUtility::GetSupervariantListFromShaderSourceData(shaderSourceData); diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp index e47dd55a97..eb91d9e866 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -814,6 +815,51 @@ namespace AZ return success; } + IncludedFilesParser::IncludedFilesParser() + { + AZStd::regex regex(R"(#\s*include\s+[<|"]([\w|/|\\|\.|-]+)[>|"])", AZStd::regex::ECMAScript); + m_includeRegex.swap(regex); + } + + AZStd::vector IncludedFilesParser::ParseStringAndGetIncludedFiles(AZStd::string_view haystack) const + { + AZStd::vector listOfFilePaths; + AZStd::smatch match; + AZStd::string::const_iterator searchStart(haystack.cbegin()); + while (AZStd::regex_search(searchStart, haystack.cend(), match, m_includeRegex)) + { + if (match.size() > 1) + { + AZStd::string relativeFilePath(match[1].str().c_str()); + AzFramework::StringFunc::Path::Normalize(relativeFilePath); + listOfFilePaths.push_back(relativeFilePath); + } + searchStart = match.suffix().first; + } + return listOfFilePaths; + } + + AZ::Outcome, AZStd::string> IncludedFilesParser::ParseFileAndGetIncludedFiles(AZStd::string_view sourceFilePath) const + { + AZ::IO::FileIOStream stream(sourceFilePath.data(), AZ::IO::OpenMode::ModeRead); + if (!stream.IsOpen()) + { + return AZ::Failure(AZStd::string::format("\"%s\" source file could not be opened.", sourceFilePath.data())); + } + + if (!stream.CanRead()) + { + return AZ::Failure(AZStd::string::format("\"%s\" source file could not be read.", sourceFilePath.data())); + } + + AZStd::string hayStack; + hayStack.resize_no_construct(stream.GetLength()); + stream.Read(stream.GetLength(), hayStack.data()); + + auto listOfFilePaths = ParseStringAndGetIncludedFiles(hayStack); + return AZ::Success(AZStd::move(listOfFilePaths)); + } + } // namespace ShaderBuilderUtility } // namespace ShaderBuilder } // AZ diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h index c000ba9df6..5d45ade9cb 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h @@ -141,6 +141,29 @@ namespace AZ const uint32_t rhiUniqueIndex, const AZStd::string& platformIdentifier, const AZStd::string& shaderJsonPath, const uint32_t supervariantIndex, RPI::ShaderAssetSubId shaderAssetSubId); + + class IncludedFilesParser + { + public: + IncludedFilesParser(); + ~IncludedFilesParser() = default; + + //! This static function was made public for testability purposes only. + //! Parses the string @haystack, looking for "#include file" lines with a regular expression. + //! Returns the list of relative paths as included by the file. + //! REMARK: The algorithm may over prescribe what files to include because it doesn't discern between comments, etc. + //! Also, a #include line may be protected by #ifdef macros but this algorithm doesn't care. + //! Over prescribing is not a real problem, albeit potential waste in processing. Under prescribing would be a real problem. + AZStd::vector ParseStringAndGetIncludedFiles(AZStd::string_view haystack) const; + + //! This static function was made public for testability purposes only. + //! Opens the file @sourceFilePath, loads the content into a string and returns ParseStringAndGetIncludedFiles(content) + AZ::Outcome, AZStd::string> ParseFileAndGetIncludedFiles(AZStd::string_view sourceFilePath) const; + + private: + AZStd::regex m_includeRegex; + }; + } // ShaderBuilderUtility namespace } // ShaderBuilder namespace } // AZ diff --git a/Gems/Atom/Asset/Shader/Code/Tests/ShaderBuilderUtilityTests.cpp b/Gems/Atom/Asset/Shader/Code/Tests/ShaderBuilderUtilityTests.cpp new file mode 100644 index 0000000000..d477060d04 --- /dev/null +++ b/Gems/Atom/Asset/Shader/Code/Tests/ShaderBuilderUtilityTests.cpp @@ -0,0 +1,86 @@ +/* + * 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 +#include + +#include "Common/ShaderBuilderTestFixture.h" + +#include + +namespace UnitTest +{ + using namespace AZ; + + // The main purpose of this class is to test ShaderBuilderUtility functions + class ShaderBuilderUtilityTests : public ShaderBuilderTestFixture + { + }; // class ShaderBuilderUtilityTests + + + TEST_F(ShaderBuilderUtilityTests, IncludedFilesParser_ParseStringAndGetIncludedFiles) + { + AZStd::string haystack( + "Some content to parse\n" + "#include \n" + "// #include \n" + "blah # include \"valid_file3.azsli\"\n" + "bar include \n" + "foo # include \"a/directory/valid-file5.azsli\"\n" + "# include \n" + "#includ \"a\\dire-ctory\\invalid-file7.azsli\"\n" + ); + + AZ::ShaderBuilder::ShaderBuilderUtility::IncludedFilesParser includedFilesParser; + auto fileList = includedFilesParser.ParseStringAndGetIncludedFiles(haystack); + EXPECT_EQ(fileList.size(), 5); + + auto it = AZStd::find(fileList.begin(), fileList.end(), "valid_file1.azsli"); + EXPECT_TRUE(it != fileList.end()); + + it = AZStd::find(fileList.begin(), fileList.end(), "valid_file2.azsli"); + EXPECT_TRUE(it != fileList.end()); + + it = AZStd::find(fileList.begin(), fileList.end(), "valid_file3.azsli"); + EXPECT_TRUE(it != fileList.end()); + + // Remark: From now on We must normalize because internally AZ::ShaderBuilder::ShaderBuilderUtility::IncludedFilesParser + // always returns normalized paths. + { + AZStd::string fileName("a\\dire-ctory\\invalid-file4.azsli"); + AzFramework::StringFunc::Path::Normalize(fileName); + it = AZStd::find(fileList.begin(), fileList.end(), fileName); + EXPECT_TRUE(it == fileList.end()); + } + + { + AZStd::string fileName("a\\directory\\valid-file5.azsli"); + AzFramework::StringFunc::Path::Normalize(fileName); + it = AZStd::find(fileList.begin(), fileList.end(), fileName); + EXPECT_TRUE(it != fileList.end()); + } + + { + AZStd::string fileName("a\\dire-ctory\\valid-file6.azsli"); + AzFramework::StringFunc::Path::Normalize(fileName); + it = AZStd::find(fileList.begin(), fileList.end(), fileName); + EXPECT_TRUE(it != fileList.end()); + } + + { + AZStd::string fileName("a\\dire-ctory\\invalid-file7.azsli"); + AzFramework::StringFunc::Path::Normalize(fileName); + it = AZStd::find(fileList.begin(), fileList.end(), fileName); + EXPECT_TRUE(it == fileList.end()); + } + } + +} //namespace UnitTest + +//AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV); + diff --git a/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake b/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake index 033b399478..ab8df70bdb 100644 --- a/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake +++ b/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake @@ -11,4 +11,5 @@ set(FILES Tests/Common/ShaderBuilderTestFixture.cpp Tests/SupervariantCmdArgumentTests.cpp Tests/McppBinderTests.cpp + Tests/ShaderBuilderUtilityTests.cpp ) diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl index 64f6b3a4b5..a7dfad1c24 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl @@ -55,7 +55,7 @@ int GetLdsIndex(int2 ldsPosition) // --- Common file start --- -// #include +// include ('#' symbol before 'include' was removed on purpose to avoid parsing this azsli file during ShaderAssetBuilder::CreateJobs) // This include fails with the asset processor when generating the .shader for this file // Everything below this is copy pasted from FastDepthAwareBlurCommon.azsli up until the // "Common file end" marker below diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl index cfb49f5911..b4f230c99c 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl @@ -55,7 +55,7 @@ int GetLdsIndex(int2 ldsPosition) // --- Common file start --- -// #include +// include ('#' symbol before 'include' was removed on purpose to avoid parsing this azsli file during ShaderAssetBuilder::CreateJobs) // This include fails with the asset processor when generating the .shader for this file // Everything below this is copy pasted from FastDepthAwareBlurCommon.azsli up until the // "Common file end" marker below diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli index 8c03816b26..a32b56857c 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli @@ -157,7 +157,7 @@ * #define SMAA_RT_METRICS float4(1.0 / 1280.0, 1.0 / 720.0, 1280.0, 720.0) * #define SMAA_HLSL_4 * #define SMAA_PRESET_HIGH - * #include "SMAA.h" + * include "SMAA.h" ('#' symbol before 'include' was removed on purpose to avoid parsing this azsli file during ShaderAssetBuilder::CreateJobs) * * Note that SMAA_RT_METRICS doesn't need to be a macro, it can be a * uniform variable. The code is designed to minimize the impact of not From dc930a5987af99e333e7f858a498faf682c045e7 Mon Sep 17 00:00:00 2001 From: Steve Pham <82231385+spham-amzn@users.noreply.github.com> Date: Wed, 15 Sep 2021 08:32:52 -0700 Subject: [PATCH 4/6] Fix LuaIDE crash at startup caused by missing System Allocator Initialization (#4130) Signed-off-by: Steve Pham --- Code/Tools/Standalone/Source/Editor/LuaEditor.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp b/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp index 89ab8207ff..f6d905515b 100644 --- a/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp +++ b/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp @@ -35,6 +35,10 @@ int main(int argc, char* argv[]) { AZ::AllocatorInstance::Create(); } + if (!AZ::AllocatorInstance::IsReady()) + { + AZ::AllocatorInstance::Create(); + } AZStd::unique_ptr fileIO = AZStd::unique_ptr(aznew AZ::IO::LocalFileIO()); AZ::IO::FileIOBase::SetInstance(fileIO.get()); @@ -70,6 +74,10 @@ int main(int argc, char* argv[]) // if its in GUI mode or not. } + if (AZ::AllocatorInstance::IsReady()) + { + AZ::AllocatorInstance::Destroy(); + } if (AZ::AllocatorInstance::IsReady()) { AZ::AllocatorInstance::Destroy(); From 089391f761ab3a1e1bda34273e8c7f3333ee1b9b Mon Sep 17 00:00:00 2001 From: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> Date: Wed, 15 Sep 2021 10:57:37 -0500 Subject: [PATCH 5/6] Terrain System cleanups and unit tests (#4119) * Remove the "TEST_SUPPORTED" traits. Terrain unit tests should be usable on all platforms, so they shouldn't need a platform-specific trait to enable/disable. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fix a few misc terrain bugs. * Change Activate/Deactivate to happen immediately instead of deferring. There were too many order-of-operation bugs caused by trying to defer this. * Added implementation for calculating normals. * Fixed bug where GetHeightSynchronous wasn't stopping at the highest-priority layer. * Added locks for SurfaceData bus to help ensure we lock our mutexes in the correct order and avoid deadlocks. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Add trivial TerrainSystem tests. Tests construction, Activate(), Deactivate(), and destruction. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Unified Terrain system calls on single bus. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Added mock for TerrainDataNotificationBus listener. Also added unit tests to verify the listener, and added in missing notification events. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Removed extra Sampler class. Fixed up APIs to correctly pass Sampler and terrainExistsPtr around. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switched MockTerrainSystem to be proper gmock. This makes it for flexible to use and easier to reuse from other test environments. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fix settings bug caused by bad order of operations that occurred when the methods moved to a different bus. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Eliminate extra EBus by simplifying area initialization. Previously, there was a back-and-forth ebus signal used for the terrain system to find any terrain spawners that were created prior to the terrain system activation. Now it uses the more simple technique of just grabbing all the spawners that are currently hooked up to the spawner ebus. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switch to NiceMock so that "uninteresting" mock calls get ignored. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Addressed PR feedback. Filled in terrainExistsPtr at the end, and added it to GetNormal as well. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fixed shader height calculation. It was off by half a pixel, and it was interpolating, both of which were wrong. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> --- Code/Editor/GameExporter.cpp | 2 +- .../Terrain/TerrainDataRequestBus.cpp | 3 +- .../Terrain/TerrainDataRequestBus.h | 5 +- Gems/PhysX/Code/Tests/PhysXTestUtil.h | 23 +- .../Shaders/Terrain/TerrainCommon.azsli | 14 +- Gems/Terrain/Code/CMakeLists.txt | 67 +++--- .../TerrainHeightGradientListComponent.cpp | 12 +- .../TerrainHeightGradientListComponent.h | 10 +- .../TerrainLayerSpawnerComponent.cpp | 7 - .../Components/TerrainLayerSpawnerComponent.h | 5 +- .../Components/TerrainWorldComponent.cpp | 14 +- .../TerrainWorldDebuggerComponent.cpp | 4 +- .../Source/TerrainSystem/TerrainSystem.cpp | 221 +++++++++++------- .../Code/Source/TerrainSystem/TerrainSystem.h | 17 +- .../Source/TerrainSystem/TerrainSystemBus.h | 47 +--- Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp | 59 +++-- Gems/Terrain/Code/Tests/TerrainMocks.h | 60 ++--- Gems/Terrain/Code/Tests/TerrainSystemTest.cpp | 92 ++++++++ Gems/Terrain/Code/Tests/TerrainTest.cpp | 20 -- Gems/Terrain/Code/terrain_tests_files.cmake | 1 + 20 files changed, 395 insertions(+), 288 deletions(-) create mode 100644 Gems/Terrain/Code/Tests/TerrainSystemTest.cpp diff --git a/Code/Editor/GameExporter.cpp b/Code/Editor/GameExporter.cpp index 1fc980fdac..00dc3d8de1 100644 --- a/Code/Editor/GameExporter.cpp +++ b/Code/Editor/GameExporter.cpp @@ -318,7 +318,7 @@ void CGameExporter::ExportLevelInfo(const QString& path) root->setAttr("Name", levelName.toUtf8().data()); auto terrain = AzFramework::Terrain::TerrainDataRequestBus::FindFirstHandler(); const AZ::Aabb terrainAabb = terrain ? terrain->GetTerrainAabb() : AZ::Aabb::CreateFromPoint(AZ::Vector3::CreateZero()); - const AZ::Vector2 terrainGridResolution = terrain ? terrain->GetTerrainGridResolution() : AZ::Vector2::CreateOne(); + const AZ::Vector2 terrainGridResolution = terrain ? terrain->GetTerrainHeightQueryResolution() : AZ::Vector2::CreateOne(); const int compiledHeightmapSize = static_cast(terrainAabb.GetXExtent() / terrainGridResolution.GetX()); root->setAttr("HeightmapSize", compiledHeightmapSize); diff --git a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp index 947d3821ab..1fb29cfa30 100644 --- a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp +++ b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp @@ -51,7 +51,8 @@ namespace AzFramework ->Event("GetNormal", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetNormal) ->Event("GetNormalFromFloats", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetNormalFromFloats) ->Event("GetTerrainAabb", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainAabb) - ->Event("GetTerrainGridResolution", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainGridResolution) + ->Event("GetTerrainHeightQueryResolution", + &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainHeightQueryResolution) ; } diff --git a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h index 08e238434e..92eb28a110 100644 --- a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h +++ b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h @@ -59,8 +59,11 @@ namespace AzFramework static AZ::Vector3 GetDefaultTerrainNormal() { return AZ::Vector3::CreateAxisZ(); } // System-level queries to understand world size and resolution - virtual AZ::Vector2 GetTerrainGridResolution() const = 0; + virtual AZ::Vector2 GetTerrainHeightQueryResolution() const = 0; + virtual void SetTerrainHeightQueryResolution(AZ::Vector2 queryResolution) = 0; + virtual AZ::Aabb GetTerrainAabb() const = 0; + virtual void SetTerrainAabb(const AZ::Aabb& worldBounds) = 0; //! Returns terrains height in meters at location x,y. //! @terrainExistsPtr: Can be nullptr. If != nullptr then, if there's no terrain at location x,y or location x,y is inside a terrain HOLE then *terrainExistsPtr will become false, diff --git a/Gems/PhysX/Code/Tests/PhysXTestUtil.h b/Gems/PhysX/Code/Tests/PhysXTestUtil.h index b1ba23600a..8e81b9cb89 100644 --- a/Gems/PhysX/Code/Tests/PhysXTestUtil.h +++ b/Gems/PhysX/Code/Tests/PhysXTestUtil.h @@ -93,9 +93,26 @@ namespace PhysX //////////////////////////////////////////////////////////////////////// // TerrainDataRequestBus interface dummy implementation - AZ::Vector2 GetTerrainGridResolution() const override { return {}; } - AZ::Aabb GetTerrainAabb() const override { return {}; } - float GetHeight(AZ::Vector3, Sampler, bool*) const override { return {}; } + AZ::Vector2 GetTerrainHeightQueryResolution() const override + { + return {}; + } + void SetTerrainHeightQueryResolution([[maybe_unused]] AZ::Vector2 queryResolution) override + { + } + + AZ::Aabb GetTerrainAabb() const override + { + return {}; + } + void SetTerrainAabb([[maybe_unused]] const AZ::Aabb& worldBounds) override + { + } + + float GetHeight(AZ::Vector3, Sampler, bool*) const override + { + return {}; + } float GetHeightFromFloats(float, float, Sampler, bool*) const override { return {}; } AzFramework::SurfaceData::SurfaceTagWeight GetMaxSurfaceWeight(AZ::Vector3, Sampler, bool*) const override { return {}; } AzFramework::SurfaceData::SurfaceTagWeight GetMaxSurfaceWeightFromFloats(float, float, Sampler, bool*) const override { return {}; } diff --git a/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli b/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli index 6e489796d7..18b85bbd43 100644 --- a/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli +++ b/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli @@ -11,16 +11,16 @@ ShaderResourceGroup ObjectSrg : SRG_PerObject { Texture2D m_heightmapImage; - Sampler LinearSampler + Sampler PointSampler { - MinFilter = Linear; - MagFilter = Linear; - MipFilter = Linear; + MinFilter = Point; + MagFilter = Point; + MipFilter = Point; AddressU = Clamp; AddressV = Clamp; AddressW = Clamp; }; - + row_major float3x4 m_modelToWorld; struct TerrainData @@ -57,8 +57,8 @@ float4x4 GetObject_WorldMatrix() float GetHeight(float2 origUv) { - float2 uv = clamp(origUv, 0.0f, 1.0f); - return ObjectSrg::m_terrainData.m_heightScale * (ObjectSrg::m_heightmapImage.SampleLevel(ObjectSrg::LinearSampler, uv, 0).r - 0.5f); + float2 uv = clamp(origUv + (ObjectSrg::m_terrainData.m_uvStep * 0.5f), 0.0f, 1.0f); + return ObjectSrg::m_terrainData.m_heightScale * (ObjectSrg::m_heightmapImage.SampleLevel(ObjectSrg::PointSampler, uv, 0).r - 0.5f); } float4 GetTerrainProjectedPosition(ObjectSrg::TerrainData terrainData, float2 vertexPosition, float2 uv) diff --git a/Gems/Terrain/Code/CMakeLists.txt b/Gems/Terrain/Code/CMakeLists.txt index b4a35edbbf..0feacdaf71 100644 --- a/Gems/Terrain/Code/CMakeLists.txt +++ b/Gems/Terrain/Code/CMakeLists.txt @@ -83,15 +83,36 @@ endif() ################################################################################ # See if globally, tests are supported if(PAL_TRAIT_BUILD_TESTS_SUPPORTED) - # We globally support tests, see if we support tests on this platform for Terrain.Static - if(PAL_TRAIT_TERRAIN_TEST_SUPPORTED) - # We support Terrain.Tests on this platform, add Terrain.Tests target which depends on Terrain.Static + ly_add_target( + NAME Terrain.Tests ${PAL_TRAIT_TEST_TARGET_TYPE} + NAMESPACE Gem + FILES_CMAKE + terrain_files.cmake + terrain_tests_files.cmake + INCLUDE_DIRECTORIES + PRIVATE + Tests + Source + BUILD_DEPENDENCIES + PRIVATE + AZ::AzTest + AZ::AzFramework + Gem::Terrain.Static + ) + + # Add Terrain.Tests to googletest + ly_add_googletest( + NAME Gem::Terrain.Tests + ) + + # If we are a host platform we want to add tools test like editor tests here + if(PAL_TRAIT_BUILD_HOST_TOOLS) + # We support Terrain.Editor.Tests on this platform, add Terrain.Editor.Tests target which depends on Terrain.Editor ly_add_target( - NAME Terrain.Tests ${PAL_TRAIT_TEST_TARGET_TYPE} + NAME Terrain.Editor.Tests ${PAL_TRAIT_TEST_TARGET_TYPE} NAMESPACE Gem FILES_CMAKE - terrain_files.cmake - terrain_tests_files.cmake + terrain_editor_tests_files.cmake INCLUDE_DIRECTORIES PRIVATE Tests @@ -99,40 +120,12 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED) BUILD_DEPENDENCIES PRIVATE AZ::AzTest - AZ::AzFramework - Gem::Terrain.Static + Gem::Terrain.Editor ) - # Add Terrain.Tests to googletest + # Add Terrain.Editor.Tests to googletest ly_add_googletest( - NAME Gem::Terrain.Tests + NAME Gem::Terrain.Editor.Tests ) endif() - - # If we are a host platform we want to add tools test like editor tests here - if(PAL_TRAIT_BUILD_HOST_TOOLS) - # We are a host platform, see if Editor tests are supported on this platform - if(PAL_TRAIT_TERRAIN_EDITOR_TEST_SUPPORTED) - # We support Terrain.Editor.Tests on this platform, add Terrain.Editor.Tests target which depends on Terrain.Editor - ly_add_target( - NAME Terrain.Editor.Tests ${PAL_TRAIT_TEST_TARGET_TYPE} - NAMESPACE Gem - FILES_CMAKE - terrain_editor_tests_files.cmake - INCLUDE_DIRECTORIES - PRIVATE - Tests - Source - BUILD_DEPENDENCIES - PRIVATE - AZ::AzTest - Gem::Terrain.Editor - ) - - # Add Terrain.Editor.Tests to googletest - ly_add_googletest( - NAME Gem::Terrain.Editor.Tests - ) - endif() - endif() endif() diff --git a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp index d64f660d90..4d0cb6576d 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp @@ -166,14 +166,20 @@ namespace Terrain } void TerrainHeightGradientListComponent::GetHeight( - const AZ::Vector3& inPosition, AZ::Vector3& outPosition, [[maybe_unused]] Sampler sampleFilter = Sampler::DEFAULT) + const AZ::Vector3& inPosition, + AZ::Vector3& outPosition, + [[maybe_unused]] AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter = + AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT) { const float height = GetHeight(inPosition.GetX(), inPosition.GetY()); outPosition.SetZ(height); } void TerrainHeightGradientListComponent::GetNormal( - const AZ::Vector3& inPosition, AZ::Vector3& outNormal, [[maybe_unused]] Sampler sampleFilter = Sampler::DEFAULT) + const AZ::Vector3& inPosition, + AZ::Vector3& outNormal, + [[maybe_unused]] AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter = + AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT) { const float x = inPosition.GetX(); const float y = inPosition.GetY(); @@ -206,7 +212,7 @@ namespace Terrain // Get the height range of the entire world m_cachedHeightQueryResolution = AZ::Vector2(1.0f); AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult( - m_cachedHeightQueryResolution, &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainGridResolution); + m_cachedHeightQueryResolution, &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainHeightQueryResolution); AZ::Aabb worldBounds = AZ::Aabb::CreateNull(); AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult( diff --git a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.h b/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.h index 7635680815..509f003afa 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.h +++ b/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.h @@ -64,8 +64,14 @@ namespace Terrain TerrainHeightGradientListComponent() = default; ~TerrainHeightGradientListComponent() = default; - void GetHeight(const AZ::Vector3& inPosition, AZ::Vector3& outPosition, Sampler sampleFilter) override; - void GetNormal(const AZ::Vector3& inPosition, AZ::Vector3& outNormal, Sampler sampleFilter) override; + void GetHeight( + const AZ::Vector3& inPosition, + AZ::Vector3& outPosition, + AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter) override; + void GetNormal( + const AZ::Vector3& inPosition, + AZ::Vector3& outNormal, + AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter) override; ////////////////////////////////////////////////////////////////////////// // AZ::Component interface implementation diff --git a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp index e17dbd0e93..245c98ccac 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp @@ -104,7 +104,6 @@ namespace Terrain { AZ::TransformNotificationBus::Handler::BusConnect(GetEntityId()); LmbrCentral::ShapeComponentNotificationsBus::Handler::BusConnect(GetEntityId()); - TerrainAreaRequestBus::Handler::BusConnect(GetEntityId()); TerrainSpawnerRequestBus::Handler::BusConnect(GetEntityId()); TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RegisterArea, GetEntityId()); @@ -114,7 +113,6 @@ namespace Terrain { TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::UnregisterArea, GetEntityId()); TerrainSpawnerRequestBus::Handler::BusDisconnect(); - TerrainAreaRequestBus::Handler::BusDisconnect(); LmbrCentral::ShapeComponentNotificationsBus::Handler::BusDisconnect(); AZ::TransformNotificationBus::Handler::BusDisconnect(); @@ -161,11 +159,6 @@ namespace Terrain return m_configuration.m_useGroundPlane; } - void TerrainLayerSpawnerComponent::RegisterArea() - { - TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RegisterArea, GetEntityId()); - } - void TerrainLayerSpawnerComponent::RefreshArea() { TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId()); diff --git a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.h b/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.h index 3f8e72e1b8..c7398bf93e 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.h +++ b/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.h @@ -58,7 +58,6 @@ namespace Terrain : public AZ::Component , private AZ::TransformNotificationBus::Handler , private LmbrCentral::ShapeComponentNotificationsBus::Handler - , private Terrain::TerrainAreaRequestBus::Handler , private Terrain::TerrainSpawnerRequestBus::Handler { public: @@ -81,6 +80,7 @@ namespace Terrain bool ReadInConfig(const AZ::ComponentConfig* baseConfig) override; bool WriteOutConfig(AZ::ComponentConfig* outBaseConfig) const override; + protected: ////////////////////////////////////////////////////////////////////////// // AZ::TransformNotificationBus::Handler void OnTransformChanged(const AZ::Transform& local, const AZ::Transform& world) override; @@ -92,8 +92,7 @@ namespace Terrain void GetPriority(AZ::u32& outLayer, AZ::u32& outPriority) override; bool GetUseGroundPlane() override; - void RegisterArea() override; - void RefreshArea() override; + void RefreshArea(); private: TerrainLayerSpawnerConfig m_configuration; diff --git a/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp index 669a8f4b02..d8b7308ef7 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp @@ -13,6 +13,7 @@ #include #include #include +#include namespace Terrain { @@ -85,17 +86,16 @@ namespace Terrain void TerrainWorldComponent::Activate() { - TerrainSystemServiceRequestBus::Broadcast( - &TerrainSystemServiceRequestBus::Events::SetWorldBounds, - AZ::Aabb::CreateFromMinMax(m_configuration.m_worldMin, m_configuration.m_worldMax) - ); - TerrainSystemServiceRequestBus::Broadcast( - &TerrainSystemServiceRequestBus::Events::SetHeightQueryResolution, m_configuration.m_heightQueryResolution); - // Currently, the Terrain System Component owns the Terrain System instance because the Terrain World component gets recreated // every time an entity is added or removed to a level. If this ever changes, the Terrain System ownership could move into // the level component. TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::Activate); + + AzFramework::Terrain::TerrainDataRequestBus::Broadcast( + &AzFramework::Terrain::TerrainDataRequestBus::Events::SetTerrainAabb, + AZ::Aabb::CreateFromMinMax(m_configuration.m_worldMin, m_configuration.m_worldMax)); + AzFramework::Terrain::TerrainDataRequestBus::Broadcast( + &AzFramework::Terrain::TerrainDataRequestBus::Events::SetTerrainHeightQueryResolution, m_configuration.m_heightQueryResolution); } void TerrainWorldComponent::Deactivate() diff --git a/Gems/Terrain/Code/Source/Components/TerrainWorldDebuggerComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainWorldDebuggerComponent.cpp index d7504295c2..89ffa6364f 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainWorldDebuggerComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainWorldDebuggerComponent.cpp @@ -171,7 +171,7 @@ namespace Terrain // Determine how far to draw in each direction in world space based on our MaxSectorsToDraw AZ::Vector2 queryResolution = AZ::Vector2(1.0f); AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult( - queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainGridResolution); + queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainHeightQueryResolution); AZ::Vector3 viewDistance( queryResolution.GetX() * SectorSizeInGridPoints * sqrtf(MaxSectorsToDraw), queryResolution.GetY() * SectorSizeInGridPoints * sqrtf(MaxSectorsToDraw), @@ -214,7 +214,7 @@ namespace Terrain AZ::Vector2 queryResolution = AZ::Vector2(1.0f); AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult( - queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainGridResolution); + queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainHeightQueryResolution); // Calculate the world size of each sector. Note that this size actually ends at the last point, not the last square. // So for example, the sector size for 3 points will go from (*--*--*) even though it will be used to draw (*--*--*--). diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp index a271d624f5..674af376e4 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp +++ b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -53,7 +54,7 @@ TerrainSystem::TerrainSystem() m_currentSettings.m_worldBounds = AZ::Aabb::CreateNull(); m_requestedSettings = m_currentSettings; - m_requestedSettings.m_worldBounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(0.0f, 0.0f, 0.0f), AZ::Vector3(4096.0f, 4096.0f, 2048.0f)); + m_requestedSettings.m_worldBounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(-512.0f), AZ::Vector3(512.0f)); } TerrainSystem::~TerrainSystem() @@ -66,23 +67,76 @@ TerrainSystem::~TerrainSystem() void TerrainSystem::Activate() { - m_requestedSettings.m_systemActive = true; + AzFramework::Terrain::TerrainDataNotificationBus::Broadcast( + &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataCreateBegin); + + m_dirtyRegion = AZ::Aabb::CreateNull(); + m_terrainHeightDirty = true; m_terrainSettingsDirty = true; + m_requestedSettings.m_systemActive = true; + + { + AZStd::shared_lock lock(m_areaMutex); + m_registeredAreas.clear(); + } + + AzFramework::Terrain::TerrainDataRequestBus::Handler::BusConnect(); + + // Register any terrain spawners that were already active before the terrain system activated. + auto enumerationCallback = [&]([[maybe_unused]] Terrain::TerrainSpawnerRequests* terrainSpawner) -> bool + { + AZ::EntityId areaId = *(Terrain::TerrainSpawnerRequestBus::GetCurrentBusId()); + RegisterArea(areaId); + + // Keep Enumerating + return true; + }; + Terrain::TerrainSpawnerRequestBus::EnumerateHandlers(enumerationCallback); + + AzFramework::Terrain::TerrainDataNotificationBus::Broadcast( + &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataCreateEnd); } void TerrainSystem::Deactivate() { - m_requestedSettings.m_systemActive = false; + AzFramework::Terrain::TerrainDataNotificationBus::Broadcast( + &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataDestroyBegin); + + AzFramework::Terrain::TerrainDataRequestBus::Handler::BusDisconnect(); + + { + AZStd::shared_lock lock(m_areaMutex); + m_registeredAreas.clear(); + } + + m_dirtyRegion = AZ::Aabb::CreateNull(); + m_terrainHeightDirty = true; m_terrainSettingsDirty = true; + m_requestedSettings.m_systemActive = false; + + if (auto rpi = AZ::RPI::RPISystemInterface::Get(); rpi) + { + if (auto defaultScene = rpi->GetDefaultScene(); defaultScene) + { + const AZ::RPI::Scene* scene = defaultScene.get(); + if (auto terrainFeatureProcessor = scene->GetFeatureProcessor(); terrainFeatureProcessor) + { + terrainFeatureProcessor->RemoveTerrainData(); + } + } + } + + AzFramework::Terrain::TerrainDataNotificationBus::Broadcast( + &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataDestroyEnd); } -void TerrainSystem::SetWorldBounds(const AZ::Aabb& worldBounds) +void TerrainSystem::SetTerrainAabb(const AZ::Aabb& worldBounds) { m_requestedSettings.m_worldBounds = worldBounds; m_terrainSettingsDirty = true; } -void TerrainSystem::SetHeightQueryResolution(AZ::Vector2 queryResolution) +void TerrainSystem::SetTerrainHeightQueryResolution(AZ::Vector2 queryResolution) { m_requestedSettings.m_heightQueryResolution = queryResolution; m_terrainSettingsDirty = true; @@ -93,13 +147,15 @@ AZ::Aabb TerrainSystem::GetTerrainAabb() const return m_currentSettings.m_worldBounds; } -AZ::Vector2 TerrainSystem::GetTerrainGridResolution() const +AZ::Vector2 TerrainSystem::GetTerrainHeightQueryResolution() const { return m_currentSettings.m_heightQueryResolution; } -float TerrainSystem::GetHeightSynchronous(float x, float y) const +float TerrainSystem::GetHeightSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const { + bool terrainExists = false; + AZ::Vector3 inPosition((float)x, (float)y, m_currentSettings.m_worldBounds.GetMin().GetZ()); AZ::Vector3 outPosition((float)x, (float)y, m_currentSettings.m_worldBounds.GetMin().GetZ()); @@ -111,67 +167,77 @@ float TerrainSystem::GetHeightSynchronous(float x, float y) const if (areaBounds.Contains(inPosition)) { Terrain::TerrainAreaHeightRequestBus::Event( - areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetHeight, inPosition, outPosition, - Terrain::TerrainAreaHeightRequestBus::Events::Sampler::DEFAULT); + areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetHeight, inPosition, outPosition, sampler); + + terrainExists = true; + + break; } } + if (terrainExistsPtr) + { + *terrainExistsPtr = terrainExists; + } + return AZ::GetClamp( outPosition.GetZ(), m_currentSettings.m_worldBounds.GetMin().GetZ(), m_currentSettings.m_worldBounds.GetMax().GetZ()); } -float TerrainSystem::GetHeight(AZ::Vector3 position, [[maybe_unused]] Sampler sampler, [[maybe_unused]] bool* terrainExistsPtr) const +float TerrainSystem::GetHeight(AZ::Vector3 position, Sampler sampler, bool* terrainExistsPtr) const { - if (terrainExistsPtr) + return GetHeightSynchronous(position.GetX(), position.GetY(), sampler, terrainExistsPtr); +} + +float TerrainSystem::GetHeightFromFloats(float x, float y, Sampler sampler, bool* terrainExistsPtr) const +{ + return GetHeightSynchronous(x, y, sampler, terrainExistsPtr); +} + +bool TerrainSystem::GetIsHoleFromFloats(float x, float y, Sampler sampler) const +{ + bool terrainExists = false; + GetHeightSynchronous(x, y, sampler, &terrainExists); + return !terrainExists; +} + +AZ::Vector3 TerrainSystem::GetNormalSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const +{ + bool terrainExists = false; + + AZ::Vector3 inPosition((float)x, (float)y, m_currentSettings.m_worldBounds.GetMin().GetZ()); + AZ::Vector3 outNormal = AZ::Vector3::CreateAxisZ(); + + AZStd::shared_lock lock(m_areaMutex); + + for (auto& [areaId, areaBounds] : m_registeredAreas) { - *terrainExistsPtr = true; + inPosition.SetZ(areaBounds.GetMin().GetZ()); + if (areaBounds.Contains(inPosition)) + { + Terrain::TerrainAreaHeightRequestBus::Event( + areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetNormal, inPosition, outNormal, sampler); + terrainExists = true; + break; + } } - return GetHeightSynchronous(position.GetX(), position.GetY()); -} - -float TerrainSystem::GetHeightFromFloats( - float x, float y, [[maybe_unused]] Sampler sampler, [[maybe_unused]] bool* terrainExistsPtr) const -{ if (terrainExistsPtr) { - *terrainExistsPtr = true; + *terrainExistsPtr = terrainExists; } - return GetHeightSynchronous(x, y); + return outNormal; } -bool TerrainSystem::GetIsHoleFromFloats( - [[maybe_unused]] float x, [[maybe_unused]] float y, [[maybe_unused]] Sampler sampleFilter) const +AZ::Vector3 TerrainSystem::GetNormal(AZ::Vector3 position, Sampler sampler, bool* terrainExistsPtr) const { - return false; + return GetNormalSynchronous(position.GetX(), position.GetY(), sampler, terrainExistsPtr); } -AZ::Vector3 TerrainSystem::GetNormalSynchronous([[maybe_unused]] float x, [[maybe_unused]] float y) const +AZ::Vector3 TerrainSystem::GetNormalFromFloats(float x, float y, Sampler sampler, bool* terrainExistsPtr) const { - return AZ::Vector3::CreateAxisZ(); -} - -AZ::Vector3 TerrainSystem::GetNormal( - AZ::Vector3 position, [[maybe_unused]] Sampler sampleFilter, [[maybe_unused]] bool* terrainExistsPtr) const -{ - if (terrainExistsPtr) - { - *terrainExistsPtr = true; - } - - return GetNormalSynchronous(position.GetX(), position.GetY()); -} - -AZ::Vector3 TerrainSystem::GetNormalFromFloats( - float x, float y, [[maybe_unused]] Sampler sampleFilter, [[maybe_unused]] bool* terrainExistsPtr) const -{ - if (terrainExistsPtr) - { - *terrainExistsPtr = true; - } - - return GetNormalSynchronous(x, y); + return GetNormalSynchronous(x, y, sampler, terrainExistsPtr); } @@ -298,35 +364,6 @@ void TerrainSystem::ProcessSurfacePointsFromRegion(const AZ::Aabb& inRegion, con } */ -void TerrainSystem::SystemActivate() -{ - { - AZStd::shared_lock lock(m_areaMutex); - m_registeredAreas.clear(); - } - - AzFramework::Terrain::TerrainDataRequestBus::Handler::BusConnect(); - - TerrainAreaRequestBus::Broadcast(&TerrainAreaRequestBus::Events::RegisterArea); -} - -void TerrainSystem::SystemDeactivate() -{ - AzFramework::Terrain::TerrainDataRequestBus::Handler::BusDisconnect(); - - { - AZStd::shared_lock lock(m_areaMutex); - m_registeredAreas.clear(); - } - - const AZ::RPI::Scene* scene = AZ::RPI::RPISystemInterface::Get()->GetDefaultScene().get(); - auto terrainFeatureProcessor = scene->GetFeatureProcessor(); - if (terrainFeatureProcessor) - { - terrainFeatureProcessor->RemoveTerrainData(); - } -} - void TerrainSystem::RegisterArea(AZ::EntityId areaId) { AZStd::unique_lock lock(m_areaMutex); @@ -383,6 +420,7 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/) if (m_terrainSettingsDirty) { + terrainSettingsChanged = true; m_terrainSettingsDirty = false; // This needs to happen before the "system active" check below, because activating the system will cause the various @@ -393,24 +431,12 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/) m_dirtyRegion.AddAabb(m_requestedSettings.m_worldBounds); m_terrainHeightDirty = true; m_currentSettings.m_worldBounds = m_requestedSettings.m_worldBounds; - terrainSettingsChanged = true; } if (m_requestedSettings.m_heightQueryResolution != m_currentSettings.m_heightQueryResolution) { m_dirtyRegion = AZ::Aabb::CreateNull(); m_terrainHeightDirty = true; - terrainSettingsChanged = true; - } - - if (m_requestedSettings.m_systemActive != m_currentSettings.m_systemActive) - { - m_requestedSettings.m_systemActive ? SystemActivate() : SystemDeactivate(); - - // Null dirty region will be interpreted as updating everything - m_dirtyRegion = AZ::Aabb::CreateNull(); - m_terrainHeightDirty = true; - terrainSettingsChanged = true; } m_currentSettings = m_requestedSettings; @@ -420,6 +446,14 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/) { AZStd::shared_lock lock(m_areaMutex); + // Block other threads from accessing the surface data bus while we are in GetValue (which may call into the SurfaceData bus). + // We lock our surface data mutex *before* checking / setting "isRequestInProgress" so that we prevent race conditions + // that create false detection of cyclic dependencies when multiple requests occur on different threads simultaneously. + // (One case where this was previously able to occur was in rapid updating of the Preview widget on the + // GradientSurfaceDataComponent in the Editor when moving the threshold sliders back and forth rapidly) + auto& surfaceDataContext = SurfaceData::SurfaceDataSystemRequestBus::GetOrCreateContext(false); + typename SurfaceData::SurfaceDataSystemRequestBus::Context::DispatchLockGuard scopeLock(surfaceDataContext.m_contextMutex); + AZ::Transform transform = AZ::Transform::CreateTranslation(m_currentSettings.m_worldBounds.GetCenter()); uint32_t width = aznumeric_cast( @@ -449,7 +483,8 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/) } AZ::Vector3 outPosition; - const Terrain::TerrainAreaHeightRequests::Sampler sampleFilter = Terrain::TerrainAreaHeightRequests::Sampler::DEFAULT; + const AzFramework::Terrain::TerrainDataRequestBus::Events::Sampler sampleFilter = + AzFramework::Terrain::TerrainDataRequestBus::Events::Sampler::DEFAULT; Terrain::TerrainAreaHeightRequestBus::Event( areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetHeight, inPosition, outPosition, sampleFilter); @@ -476,6 +511,14 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/) if (terrainSettingsChanged || m_terrainHeightDirty) { + // Block other threads from accessing the surface data bus while we are in GetValue (which may call into the SurfaceData bus). + // We lock our surface data mutex *before* checking / setting "isRequestInProgress" so that we prevent race conditions + // that create false detection of cyclic dependencies when multiple requests occur on different threads simultaneously. + // (One case where this was previously able to occur was in rapid updating of the Preview widget on the + // GradientSurfaceDataComponent in the Editor when moving the threshold sliders back and forth rapidly) + auto& surfaceDataContext = SurfaceData::SurfaceDataSystemRequestBus::GetOrCreateContext(false); + typename SurfaceData::SurfaceDataSystemRequestBus::Context::DispatchLockGuard scopeLock(surfaceDataContext.m_contextMutex); + AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask changeMask = AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask::None; diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h index a75685fb12..c52165da6b 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h +++ b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h @@ -42,10 +42,6 @@ namespace Terrain /////////////////////////////////////////// // TerrainSystemServiceRequestBus::Handler Impl - - void SetWorldBounds(const AZ::Aabb& worldBounds) override; - void SetHeightQueryResolution(AZ::Vector2 queryResolution) override; - void Activate() override; void Deactivate() override; @@ -55,8 +51,12 @@ namespace Terrain /////////////////////////////////////////// // TerrainDataRequestBus::Handler Impl - AZ::Vector2 GetTerrainGridResolution() const override; + AZ::Vector2 GetTerrainHeightQueryResolution() const override; + void SetTerrainHeightQueryResolution(AZ::Vector2 queryResolution) override; + AZ::Aabb GetTerrainAabb() const override; + void SetTerrainAabb(const AZ::Aabb& worldBounds) override; + //! Returns terrains height in meters at location x,y. //! @terrainExistsPtr: Can be nullptr. If != nullptr then, if there's no terrain at location x,y or location x,y is inside a terrain @@ -94,15 +94,12 @@ namespace Terrain float x, float y, Sampler sampleFilter = Sampler::BILINEAR, bool* terrainExistsPtr = nullptr) const override; private: - float GetHeightSynchronous(float x, float y) const; - AZ::Vector3 GetNormalSynchronous(float x, float y) const; + float GetHeightSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const; + AZ::Vector3 GetNormalSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const; // AZ::TickBus::Handler overrides ... void OnTick(float deltaTime, AZ::ScriptTimePoint time) override; - void SystemActivate(); - void SystemDeactivate(); - struct TerrainSystemSettings { AZ::Aabb m_worldBounds; diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h index cb41ba9957..1ba63a8f84 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h +++ b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h @@ -16,6 +16,8 @@ #include #include +#include + namespace Terrain { /** @@ -39,9 +41,6 @@ namespace Terrain virtual void Activate() = 0; virtual void Deactivate() = 0; - virtual void SetWorldBounds(const AZ::Aabb& worldBounds) = 0; - virtual void SetHeightQueryResolution(AZ::Vector2 queryResolution) = 0; - // register an area to override terrain virtual void RegisterArea(AZ::EntityId areaId) = 0; virtual void UnregisterArea(AZ::EntityId areaId) = 0; @@ -50,27 +49,6 @@ namespace Terrain using TerrainSystemServiceRequestBus = AZ::EBus; - /** - * A bus to signal the life times of terrain areas - * Note: all the API are meant to be queued events - */ - class TerrainAreaRequests - : public AZ::ComponentBus - { - public: - //////////////////////////////////////////////////////////////////////// - // EBusTraits - using MutexType = AZStd::recursive_mutex; - //////////////////////////////////////////////////////////////////////// - - virtual ~TerrainAreaRequests() = default; - - virtual void RegisterArea() = 0; - virtual void RefreshArea() = 0; - - }; - - using TerrainAreaRequestBus = AZ::EBus; /** * A bus to signal the life times of terrain areas @@ -87,15 +65,6 @@ namespace Terrain virtual ~TerrainAreaHeightRequests() = default; - enum class Sampler - { - BILINEAR, // Get the value at the requested location, using terrain sample grid to bilinear filter between sample grid points - CLAMP, // Clamp the input point to the terrain sample grid, then get the exact value - EXACT, // Directly get the value at the location, regardless of terrain sample grid density - - DEFAULT = BILINEAR - }; - enum SurfacePointDataMask { POSITION = 0x01, @@ -107,8 +76,16 @@ namespace Terrain // Synchronous single input location. The Vector3 input position versions are defined to ignore the input Z value. - virtual void GetHeight(const AZ::Vector3& inPosition, AZ::Vector3& outPosition, Sampler sampleFilter = Sampler::DEFAULT) = 0; - virtual void GetNormal(const AZ::Vector3& inPosition, AZ::Vector3& outNormal, Sampler sampleFilter = Sampler::DEFAULT) = 0; + virtual void GetHeight( + const AZ::Vector3& inPosition, + AZ::Vector3& outPosition, + AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter = + AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT) = 0; + virtual void GetNormal( + const AZ::Vector3& inPosition, + AZ::Vector3& outNormal, + AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter = + AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT) = 0; }; using TerrainAreaHeightRequestBus = AZ::EBus; diff --git a/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp b/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp index 91d6a26f75..f0533322c8 100644 --- a/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp +++ b/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp @@ -17,6 +17,10 @@ #include +using ::testing::NiceMock; +using ::testing::AtLeast; +using ::testing::_; + class LayerSpawnerComponentTest : public ::testing::Test { @@ -26,7 +30,7 @@ protected: AZStd::unique_ptr m_entity; Terrain::TerrainLayerSpawnerComponent* m_layerSpawnerComponent; UnitTest::MockBoxShapeComponent* m_shapeComponent; - AZStd::unique_ptr m_terrainSystem; + AZStd::unique_ptr> m_terrainSystem; void SetUp() override { @@ -40,10 +44,8 @@ protected: void TearDown() override { - if (m_terrainSystem) - { - m_terrainSystem->Deactivate(); - } + m_entity.reset(); + m_terrainSystem.reset(); m_app.Destroy(); } @@ -72,16 +74,9 @@ protected: ASSERT_TRUE(m_shapeComponent); } - void ResetEntity() - { - m_entity->Deactivate(); - m_entity->Reset(); - } - void CreateMockTerrainSystem() { - m_terrainSystem = AZStd::make_unique(); - m_terrainSystem->Activate(); + m_terrainSystem = AZStd::make_unique>(); } }; @@ -93,7 +88,7 @@ TEST_F(LayerSpawnerComponentTest, ActivatEntityActivateSuccess) m_entity->Activate(); EXPECT_EQ(m_entity->GetState(), AZ::Entity::State::Active); - ResetEntity(); + m_entity->Deactivate(); } TEST_F(LayerSpawnerComponentTest, LayerSpawnerDefaultValuesCorrect) @@ -115,7 +110,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerDefaultValuesCorrect) EXPECT_TRUE(useGroundPlane); - ResetEntity(); + m_entity->Deactivate(); } TEST_F(LayerSpawnerComponentTest, LayerSpawnerConfigValuesCorrect) @@ -147,7 +142,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerConfigValuesCorrect) EXPECT_FALSE(useGroundPlane); - ResetEntity(); + m_entity->Deactivate(); } TEST_F(LayerSpawnerComponentTest, LayerSpawnerRegisterAreaUpdatesTerrainSystem) @@ -156,14 +151,14 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerRegisterAreaUpdatesTerrainSystem) CreateMockTerrainSystem(); + // The Activate call should register the area. + EXPECT_CALL(*m_terrainSystem, RegisterArea(_)).Times(1); + AddLayerSpawnerAndShapeComponentToEntity(); m_entity->Activate(); - // The Activate call should have registered the area. - EXPECT_EQ(1, m_terrainSystem->m_registerAreaCalledCount); - - ResetEntity(); + m_entity->Deactivate(); } TEST_F(LayerSpawnerComponentTest, LayerSpawnerUnregisterAreaUpdatesTerrainSystem) @@ -172,16 +167,14 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerUnregisterAreaUpdatesTerrainSystem CreateMockTerrainSystem(); + // The Deactivate call should unregister the area. + EXPECT_CALL(*m_terrainSystem, UnregisterArea(_)).Times(1); + AddLayerSpawnerAndShapeComponentToEntity(); m_entity->Activate(); - m_layerSpawnerComponent->Deactivate(); - - // The Deactivate call should have unregistered the area. - EXPECT_EQ(1, m_terrainSystem->m_unregisterAreaCalledCount); - - ResetEntity(); + m_entity->Deactivate(); } TEST_F(LayerSpawnerComponentTest, LayerSpawnerTransformChangedUpdatesTerrainSystem) @@ -190,6 +183,9 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerTransformChangedUpdatesTerrainSyst CreateMockTerrainSystem(); + // The TransformChanged call should refresh the area. + EXPECT_CALL(*m_terrainSystem, RefreshArea(_)).Times(1); + AddLayerSpawnerAndShapeComponentToEntity(); m_entity->Activate(); @@ -197,9 +193,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerTransformChangedUpdatesTerrainSyst AZ::TransformNotificationBus::Event( m_entity->GetId(), &AZ::TransformNotificationBus::Events::OnTransformChanged, AZ::Transform(), AZ::Transform()); - EXPECT_EQ(1, m_terrainSystem->m_refreshAreaCalledCount); - - ResetEntity(); + m_entity->Deactivate(); } TEST_F(LayerSpawnerComponentTest, LayerSpawnerShapeChangedUpdatesTerrainSystem) @@ -208,6 +202,9 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerShapeChangedUpdatesTerrainSystem) CreateMockTerrainSystem(); + // The ShapeChanged call should refresh the area. + EXPECT_CALL(*m_terrainSystem, RefreshArea(_)).Times(1); + AddLayerSpawnerAndShapeComponentToEntity(); m_entity->Activate(); @@ -216,7 +213,5 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerShapeChangedUpdatesTerrainSystem) m_entity->GetId(), &LmbrCentral::ShapeComponentNotificationsBus::Events::OnShapeChanged, LmbrCentral::ShapeComponentNotifications::ShapeChangeReasons::ShapeChanged); - EXPECT_EQ(1, m_terrainSystem->m_refreshAreaCalledCount); - - ResetEntity(); + m_entity->Deactivate(); } diff --git a/Gems/Terrain/Code/Tests/TerrainMocks.h b/Gems/Terrain/Code/Tests/TerrainMocks.h index 674104e26b..03a2eccf1d 100644 --- a/Gems/Terrain/Code/Tests/TerrainMocks.h +++ b/Gems/Terrain/Code/Tests/TerrainMocks.h @@ -7,7 +7,10 @@ */ #pragma once +#include + #include +#include #include namespace UnitTest @@ -62,44 +65,45 @@ namespace UnitTest } }; - class MockTerrainSystem : private Terrain::TerrainSystemServiceRequestBus::Handler + class MockTerrainSystemService : private Terrain::TerrainSystemServiceRequestBus::Handler { public: - void Activate() override + MockTerrainSystemService() { Terrain::TerrainSystemServiceRequestBus::Handler::BusConnect(); } - void Deactivate() override + ~MockTerrainSystemService() { Terrain::TerrainSystemServiceRequestBus::Handler::BusDisconnect(); } - void SetWorldBounds([[maybe_unused]] const AZ::Aabb& worldBounds) override - { - } + MOCK_METHOD0(Activate, void()); + MOCK_METHOD0(Deactivate, void()); - void SetHeightQueryResolution([[maybe_unused]] AZ::Vector2 queryResolution) override - { - } - - void RegisterArea([[maybe_unused]] AZ::EntityId areaId) override - { - m_registerAreaCalledCount++; - } - - void UnregisterArea([[maybe_unused]] AZ::EntityId areaId) override - { - m_unregisterAreaCalledCount++; - } - - void RefreshArea([[maybe_unused]] AZ::EntityId areaId) override - { - m_refreshAreaCalledCount++; - } - - int m_registerAreaCalledCount = 0; - int m_refreshAreaCalledCount = 0; - int m_unregisterAreaCalledCount = 0; + MOCK_METHOD1(RegisterArea, void(AZ::EntityId areaId)); + MOCK_METHOD1(UnregisterArea, void(AZ::EntityId areaId)); + MOCK_METHOD1(RefreshArea, void(AZ::EntityId areaId)); }; + + class MockTerrainDataNotificationListener : public AzFramework::Terrain::TerrainDataNotificationBus::Handler + { + public: + MockTerrainDataNotificationListener() + { + AzFramework::Terrain::TerrainDataNotificationBus::Handler::BusConnect(); + } + + ~MockTerrainDataNotificationListener() + { + AzFramework::Terrain::TerrainDataNotificationBus::Handler::BusDisconnect(); + } + + MOCK_METHOD0(OnTerrainDataCreateBegin, void()); + MOCK_METHOD0(OnTerrainDataCreateEnd, void()); + MOCK_METHOD0(OnTerrainDataDestroyBegin, void()); + MOCK_METHOD0(OnTerrainDataDestroyEnd, void()); + MOCK_METHOD2(OnTerrainDataChanged, void(const AZ::Aabb& dirtyRegion, TerrainDataChangedMask dataChangedMask)); + }; + } diff --git a/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp b/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp new file mode 100644 index 0000000000..3e557c66a6 --- /dev/null +++ b/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp @@ -0,0 +1,92 @@ +/* + * 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 +#include + +#include + +#include +#include + +using ::testing::AtLeast; +using ::testing::NiceMock; + +class TerrainSystemTest : public ::testing::Test +{ +protected: + AZ::ComponentApplication m_app; + + AZStd::unique_ptr m_entity; + AZStd::unique_ptr m_terrainSystem; + + void SetUp() override + { + AZ::ComponentApplication::Descriptor appDesc; + appDesc.m_memoryBlocksByteSize = 20 * 1024 * 1024; + appDesc.m_recordingMode = AZ::Debug::AllocationRecords::RECORD_NO_RECORDS; + appDesc.m_stackRecordLevels = 20; + + m_app.Create(appDesc); + } + + void TearDown() override + { + m_terrainSystem.reset(); + m_app.Destroy(); + } + + void CreateEntity() + { + m_entity = AZStd::make_unique(); + m_entity->Init(); + + ASSERT_TRUE(m_entity); + } + + void ResetEntity() + { + m_entity->Deactivate(); + m_entity->Reset(); + } +}; + +TEST_F(TerrainSystemTest, TrivialCreateDestroy) +{ + m_terrainSystem = AZStd::make_unique(); +} + +TEST_F(TerrainSystemTest, TrivialActivateDeactivate) +{ + m_terrainSystem = AZStd::make_unique(); + m_terrainSystem->Activate(); + m_terrainSystem->Deactivate(); +} + +TEST_F(TerrainSystemTest, CreateEventsCalledOnActivation) +{ + NiceMock mockTerrainListener; + EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateBegin()).Times(AtLeast(1)); + EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateEnd()).Times(AtLeast(1)); + + m_terrainSystem = AZStd::make_unique(); + m_terrainSystem->Activate(); +} + +TEST_F(TerrainSystemTest, DestroyEventsCalledOnDeactivation) +{ + NiceMock mockTerrainListener; + EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyBegin()).Times(AtLeast(1)); + EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyEnd()).Times(AtLeast(1)); + + m_terrainSystem = AZStd::make_unique(); + m_terrainSystem->Activate(); + m_terrainSystem->Deactivate(); +} + + diff --git a/Gems/Terrain/Code/Tests/TerrainTest.cpp b/Gems/Terrain/Code/Tests/TerrainTest.cpp index 9b47c91a31..40217ff9bc 100644 --- a/Gems/Terrain/Code/Tests/TerrainTest.cpp +++ b/Gems/Terrain/Code/Tests/TerrainTest.cpp @@ -8,24 +8,4 @@ #include -class TerrainTest - : public ::testing::Test -{ -protected: - void SetUp() override - { - - } - - void TearDown() override - { - - } -}; - -TEST_F(TerrainTest, SanityTest) -{ - ASSERT_TRUE(true); -} - AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV); diff --git a/Gems/Terrain/Code/terrain_tests_files.cmake b/Gems/Terrain/Code/terrain_tests_files.cmake index b44f143f3b..6d1cf97fd9 100644 --- a/Gems/Terrain/Code/terrain_tests_files.cmake +++ b/Gems/Terrain/Code/terrain_tests_files.cmake @@ -9,5 +9,6 @@ set(FILES Tests/TerrainMocks.h Tests/TerrainTest.cpp + Tests/TerrainSystemTest.cpp Tests/LayerSpawnerTests.cpp ) From b53bf52e0dc0a2af6886b35478a9c65eb64e03ba Mon Sep 17 00:00:00 2001 From: Chris Burel Date: Wed, 15 Sep 2021 09:48:08 -0700 Subject: [PATCH 6/6] Perform global deinitialization when exiting the game launcher (#4131) * Fix code that deregisters the Atom Scene subsystem from the AzFramework Scene The AzFramework Scene subsystem API is a generic container based on the type of argument that is passed to it. It maintains a vector of typeids, and only one object of any type is stored at a time. The Bootstrap system component registers the Atom scene as a `ScenePtr` (aka `AZStd::shared_ptr`) with the AzFramework Scene's generic subsystem. However, the component was previously deregistering the type by value, `RPI::Scene`. Since no subsystem for the type `RPI::Scene` was set, unsetting this type did nothing. The result was that the `RPI::Scene` object would still be around by the time that all the Atom `InstanceDatabse`s were being destroyed, resulting in a large number of errors reported about leaked instances during global shutdown. This fixes the above issue by passing the `m_defaultScene` as a parameter to `AzFramework::Scene::UnsetSubsystem`, the same value that is passed to `SetSubsystem`. This is better, because instead of providing explicit template arguments (which were specifying the incorrect type), this now allows the compiler to deduce the correct type, and the syntax is symmetric with the call to `SetSubsystem`. Signed-off-by: Chris Burel * Correctly release the AWS API from the `HttpRequestManager` module This code was incorrectly assuming that `AWSNativeSDKInit::InitializationManager::Shutdown()` would be called automatically by the `InitializationManager` itself. However, all that `InitAwsApi()` does is create an `AZ::EnvironmentVariable`, which is a ref-counted type, and stores it in a global static. That global static is defined in a static library (namely `AWSNativeSDKInit`), which is linked in to the `HttpRequestManager` dynamic lib. Because it is a global static, it has to be explicitly cleared with the call to `Shutdown()`. Otherwise the destructor of the EnvironmentVariable doesn't happen until global destruction, by which time the allocator that is supplied to the AWS SDK has already been destroyed, and the shutdown of the AWS SDK attempts to use the already-destroyed allocator. Signed-off-by: Chris Burel * Avoid blocking the remote console server thread if there are no connections The Remote console server runs in a separate thread. Previously, it would directly call `AzSock::Accept()` and block the server thread until some client connected to it. However, if no client connected, the thread would continue to be blocked, even if the game launcher tried to exit. This adds a check to see if there's a client on the socket before calling `Accept()`, to avoid the deadlock on launcher exit. Signed-off-by: Chris Burel * Fix a log message to print one message per line Signed-off-by: Chris Burel * Allow pumping the event loop to close the launcher window Events from the OS are handled in the game's main loop. The general loop looks like this: * Read events from the OS * Tick the game application One of the events that can come from the OS is that the window hosting the game is closed. When this event happens, many resources provided by the renderer are freed, and the game application's `shouldExit` bit is set. However, when the game's `Tick()` is called, there is lots of code that assumes the renderer is still there. To avoid crashing in the `Tick()` call, check if the game should exit after pumping the system events. Signed-off-by: Chris Burel * Unload the level when exiting the launcher This ensures that any resources held onto by the level are freed before the launcher exits. Signed-off-by: Chris Burel * Add an explicit bus `Disconnect()` call to `AZCoreLogSink` This is necessary because this bus has virtual functions and can be called from multiple threads. Signed-off-by: Chris Burel * Allow normal cleanup to take place when exiting the game launcher Previously, global cleanup was side-stepped by calling `TerminateProcess` or `exit`, when quitting the game launcher. This is in contrast to the call to `_exit` on Linux and Mac when exiting the Editor. That leading `_` makes a big difference: the former runs object destruction, the latter does not. Instead of making the launcher exit with `_exit` on Linux, instead, remove that call and actually run all the atexit code. This does not modify the Editor's behavior however. It still uses `_exit` and `TerminateProcess`. Signed-off-by: Chris Burel --- .../AzFramework/Archive/Archive.cpp | 2 +- Code/LauncherUnified/Launcher.cpp | 11 +++++---- Code/Legacy/CrySystem/AZCoreLogSink.h | 5 ++++ .../CrySystem/LevelSystem/LevelSystem.cpp | 1 + Code/Legacy/CrySystem/System.cpp | 23 ------------------- .../RemoteConsole/Core/RemoteConsoleCore.cpp | 5 ++++ .../Code/Source/BootstrapSystemComponent.cpp | 2 +- .../Code/Source/HttpRequestManager.cpp | 3 +-- 8 files changed, 20 insertions(+), 32 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp b/Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp index 012d713cf5..1891c50d10 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp +++ b/Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp @@ -2185,7 +2185,7 @@ namespace AZ::IO AZStd::unique_lock lock(m_archiveMutex); if (pArchive) { - AZ_TracePrintf("Archive", "Closing Archive file: %s", pArchive->GetFullPath()); + AZ_TracePrintf("Archive", "Closing Archive file: %s\n", pArchive->GetFullPath()); } ArchiveArray::iterator it; if (m_arrArchives.size() < 16) diff --git a/Code/LauncherUnified/Launcher.cpp b/Code/LauncherUnified/Launcher.cpp index de4c496b2a..27aab074ef 100644 --- a/Code/LauncherUnified/Launcher.cpp +++ b/Code/LauncherUnified/Launcher.cpp @@ -232,13 +232,17 @@ namespace // our frame time to be managed by AzGameFramework::GameApplication // instead, which probably isn't going to happen anytime soon given // how many things depend on the ITimer interface). - bool continueRunning = true; ISystem* system = gEnv ? gEnv->pSystem : nullptr; - while (continueRunning) + while (!gameApplication.WasExitMainLoopRequested()) { // Pump the system event loop gameApplication.PumpSystemEventLoopUntilEmpty(); + if (gameApplication.WasExitMainLoopRequested()) + { + break; + } + // Update the AzFramework system tick bus gameApplication.TickSystem(); @@ -256,9 +260,6 @@ namespace { system->UpdatePostTickBus(); } - - // Check for quit requests - continueRunning = !gameApplication.WasExitMainLoopRequested() && continueRunning; } } } diff --git a/Code/Legacy/CrySystem/AZCoreLogSink.h b/Code/Legacy/CrySystem/AZCoreLogSink.h index e824c4c172..a54a78157c 100644 --- a/Code/Legacy/CrySystem/AZCoreLogSink.h +++ b/Code/Legacy/CrySystem/AZCoreLogSink.h @@ -31,6 +31,11 @@ class AZCoreLogSink : public AZ::Debug::TraceMessageBus::Handler { public: + ~AZCoreLogSink() + { + Disconnect(); + } + inline static void Connect() { GetInstance().m_ignoredAsserts = new IgnoredAssertMap(); diff --git a/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp b/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp index a0338941b5..c5d364a025 100644 --- a/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp +++ b/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp @@ -233,6 +233,7 @@ CLevelSystem::CLevelSystem(ISystem* pSystem, const char* levelsFolder) //------------------------------------------------------------------------ CLevelSystem::~CLevelSystem() { + UnloadLevel(); } //------------------------------------------------------------------------ diff --git a/Code/Legacy/CrySystem/System.cpp b/Code/Legacy/CrySystem/System.cpp index 4a9d9ece5f..2e18e13841 100644 --- a/Code/Legacy/CrySystem/System.cpp +++ b/Code/Legacy/CrySystem/System.cpp @@ -548,29 +548,6 @@ void CSystem::Quit() logger->Flush(); } - /* - * TODO: This call to _exit, _Exit, TerminateProcess etc. needs to - * eventually be removed. This causes an extremely early exit before we - * actually perform cleanup. When this gets called most managers are - * simply never deleted and we leave it to the OS to clean up our mess - * which is just really bad practice. However there are LOTS of issues - * with shutdown at the moment. Removing this will simply cause - * a crash when either the Editor or Launcher initiate shutdown. Both - * applications crash differently too. Bugs will be logged about those - * issues. - */ -#if defined(AZ_RESTRICTED_PLATFORM) -#define AZ_RESTRICTED_SECTION SYSTEM_CPP_SECTION_4 -#include AZ_RESTRICTED_FILE(System_cpp) -#endif -#if defined(AZ_RESTRICTED_SECTION_IMPLEMENTED) -#undef AZ_RESTRICTED_SECTION_IMPLEMENTED -#elif defined(WIN32) || defined(WIN64) - TerminateProcess(GetCurrentProcess(), m_env.retCode); -#else - exit(m_env.retCode); -#endif - #ifdef WIN32 //Post a WM_QUIT message to the Win32 api which causes the message loop to END //This is not the same as handling a WM_DESTROY event which destroys a window diff --git a/Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp b/Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp index d0e9e6a760..0a8d9d8eb7 100644 --- a/Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp +++ b/Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp @@ -239,6 +239,11 @@ void SRemoteServer::Run() while (m_bAcceptClients) { + AZTIMEVAL timeout { 1, 0 }; + if (!AZ::AzSock::IsRecvPending(m_socket, &timeout)) + { + continue; + } AZ::AzSock::AzSocketAddress clientAddress; sClient = AZ::AzSock::Accept(m_socket, clientAddress); if (!m_bAcceptClients || !AZ::AzSock::IsAzSocketValid(sClient)) diff --git a/Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp b/Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp index 97faa7380f..ddd88a1f52 100644 --- a/Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp +++ b/Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp @@ -386,7 +386,7 @@ namespace AZ // Unbind m_defaultScene to the GameEntityContext's AzFramework::Scene if (m_defaultFrameworkScene) { - m_defaultFrameworkScene->UnsetSubsystem(); + m_defaultFrameworkScene->UnsetSubsystem(m_defaultScene); } m_defaultScene = nullptr; diff --git a/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp b/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp index 8ed40c1599..523d84f0f5 100644 --- a/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp +++ b/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp @@ -35,7 +35,6 @@ namespace HttpRequestor desc.m_name = s_loggingName; desc.m_cpuId = AFFINITY_MASK_USERTHREADS; m_runThread = true; - // Shutdown will be handled by the InitializationManager - no need to call in the destructor AWSNativeSDKInit::InitializationManager::InitAwsApi(); auto function = AZStd::bind(&Manager::ThreadFunction, this); m_thread = AZStd::thread(function, &desc); @@ -43,7 +42,7 @@ namespace HttpRequestor Manager::~Manager() { - // NativeSDK Shutdown does not need to be called here - will be taken care of by the InitializationManager + AWSNativeSDKInit::InitializationManager::Shutdown(); m_runThread = false; m_requestConditionVar.notify_all(); if (m_thread.joinable())