Merges the changes made to stabilization into development

Several fixes were applied to stabilization in the last few weeks
which have not yet been merged to development.

Includes the following changes:
 * Allows O3DE to compile with cmake v4.xx ([#18879](https://github.com/o3de/o3de/issues/18879))
 * added IsEqual() to the render attachment descriptor to optionally ignore ([#18860](https://github.com/o3de/o3de/issues/18860))
 * Clamp output in cosine integration of edge in a hemishpere ([#18896](https://github.com/o3de/o3de/issues/18896))
 * Moves ImGuiPass into the public API section ([#18912](https://github.com/o3de/o3de/issues/18912))
 * Restricts the creation of projects that have invalid names ([#18911](https://github.com/o3de/o3de/issues/18911))

The following commits are part of the merge chain, but originally came from development, or have already been merged.
I verified that these commits do not modify the source files in the merge node.

 * Fixes issues with undo and redo and continuous edits ([#18872](https://github.com/o3de/o3de/issues/18872))
 * Fix SceneNotification/ShaderReloadNotificationBus instantiation compile error when building a monolithic-nounity-configuration ([#18825](https://github.com/o3de/o3de/issues/18825))
 * Added PhysX locking to ArticulationLink component. ([#18828](https://github.com/o3de/o3de/issues/18828))
 * Fixes spam when a TCP socket fails to bind in Remote tools. ([#18838](https://github.com/o3de/o3de/issues/18838))
 * DPE: Don't request a size update for every Dom change ([#18821](https://github.com/o3de/o3de/issues/18821))
 * ScriptCanvas: Fix default file path when saving a new graph ([#18853](https://github.com/o3de/o3de/issues/18853))
 * Update slot names if the referenced variable is renamed ([#18852](https://github.com/o3de/o3de/issues/18852))
 * Cherry-pick to stabilization: Enable testing using SDK ([#18883](https://github.com/o3de/o3de/issues/18883))
 * Fixing DrawLinear issue in DynamicDrawContext caused by GeometryView changes ([#18901](https://github.com/o3de/o3de/issues/18901))

Signed-off-by: Nicholas Lawson <[email protected]>

Gems/Atom/Feature/Common/Assets/ShaderLib/Atom/Features/PBR/Lights/Ltc.azsli

@@ -102,7 +102,8 @@ float IntegrateEdge(float3 v1, float3 v2)
         theta_sinTheta = PI * rsqrt(clamp(1.0 - cosTheta * cosTheta, EPSILON, 1.0)) - theta_sinTheta;
     }
 
-    return theta_sinTheta * cross(v1, v2).z;
+    float ans = theta_sinTheta * cross(v1, v2).z;
+    return clamp(ans, -PI, PI);
 }
 
 // Cheaper version of above which is good enough for diffuse
@@ -118,7 +119,8 @@ float IntegrateEdgeDiffuse(float3 v1, float3 v2)
         theta_sinTheta = PI * rsqrt(clamp(1.0 - cosTheta * cosTheta, EPSILON, 1.0)) - theta_sinTheta;
     }
 
-    return theta_sinTheta * cross(v1, v2).z;
+    float ans = theta_sinTheta * cross(v1, v2).z;
+    return clamp(ans, -PI, PI);
 }
 
 // Returns the unnormalized z plane intersection point between pointAboveHorizon and pointBelowHorizon.
@@ -387,7 +389,7 @@ void LtcQuadEvaluate(
     out float3 specularOut)
 {
     diffuseOut = 0.0f;
-    specularOut = 0.0f;
+    specularOut = float3(0.0f, 0.0f, 0.0f);
 
     // Initialize quad with dummy point at end in case one corner is clipped (resulting in 5 sided polygon)
     float3 polygon[5];
@@ -396,8 +398,6 @@ void LtcQuadEvaluate(
     int vertexCount = LtcQuadTransformAndClip(surface.GetDefaultNormal(), lightingData.dirToCamera, p, polygon);
     if (vertexCount == 0)
     {
-        diffuseOut = 0.0f;
-        specularOut = float3(0.0f, 0.0f, 0.0f);
         // Entire light is below the horizon.
         return;
     }
@@ -464,7 +464,7 @@ void LtcQuadEvaluate(
 //    - Save the intersection point for later
 // 3. The first point is below the horizon, but the second point is above.
 //    - Find the point along the edge that intersects the horizon.
-//    - Integate from the previous saved insection (see option 2 above) to this new insection
+//    - Integrate from the previous saved insection (see option 2 above) to this new insection
 //    - Integrate from the new insection to the second point.
 // 4. Both points are below the horizon
 //    - Do nothing.
@@ -526,7 +526,7 @@ void EvaluatePolyEdgeSpecularOnly(in float3 p0, in float3 p1, in float3x3 ltcMat
     }
 }
 
-// Evaluates the intial points to start looping through a polygon light. The first point in polygon may be below the surface
+// Evaluates the initial points to start looping through a polygon light. The first point in polygon may be below the surface
 // so care must be taking to figure out which point to start with and what point to use to close the polygon.
 void LtcPolygonEvaluateInitialPoints(
     in float3 surfacePosition,

Gems/Atom/Feature/Common/Code/Source/CommonSystemComponent.cpp

@@ -97,7 +97,7 @@
 #include <Shadows/FullscreenShadowPass.h>
 #include <CoreLights/LightCullingRemap.h>
 #include <Decals/DecalTextureArrayFeatureProcessor.h>
-#include <ImGui/ImGuiPass.h>
+#include <Atom/Feature/ImGui/ImGuiPass.h>
 
 #include <RayTracing/RayTracingAccelerationStructurePass.h>
 #include <ReflectionScreenSpace/ReflectionScreenSpacePass.h>

Gems/Atom/Feature/Common/Code/Source/CoreLights/ShadowmapPass.cpp

@@ -161,7 +161,6 @@ namespace AZ
         void ShadowmapPass::SetClearShadowDrawPacket(AZ::RHI::ConstPtr<RHI::DrawPacket> clearShadowDrawPacket)
         {
             m_clearShadowDrawPacket = clearShadowDrawPacket;
-            m_clearShadowDrawItemProperties = clearShadowDrawPacket->GetDrawItemProperties(0);
         }
 
         void ShadowmapPass::UpdatePipelineViewTag(const RPI::PipelineViewTag& viewTag)
@@ -173,6 +172,9 @@ namespace AZ
         {
             Base::SetupFrameGraphDependencies(frameGraph);
 
+            // Report + 1 to make room for the clear draw packet.
+            uint32_t estimatedItemCount = static_cast<uint32_t>(m_drawListView.size() + 1);
+
             // Override the estimated item count set by the base class. Draw item count is compared against
             // the last frame to detect cases where a moving object leaves the shadow frustum. It wouldn't
             // set m_casterMovedBit since its outside the view, but needs to trigger a re-render anyway.
@@ -185,15 +187,11 @@ namespace AZ
                     if (view && (view->GetOrFlags() & m_casterMovedBit.GetIndex()) == 0)
                     {
                         // Shadow is static and no casters moved since last frame.
-                        frameGraph.SetEstimatedItemCount(0);
+                        estimatedItemCount = 0;
                     }
                 }
             }
-            else
-            {
-                // Report + 1 to make room for the clear draw packet.
-                frameGraph.SetEstimatedItemCount(static_cast<uint32_t>(m_drawListView.size() + 1));
-            }
+            frameGraph.SetEstimatedItemCount(estimatedItemCount);
         }
 
         void ShadowmapPass::SubmitDrawItems(const RHI::FrameGraphExecuteContext& context, uint32_t startIndex, uint32_t endIndex, uint32_t offset) const

Gems/Atom/Feature/Common/Code/Source/CoreLights/ShadowmapPass.h

@@ -84,7 +84,6 @@ namespace AZ
             uint32_t GetNumDraws() const;
 
             RHI::ConstPtr<RHI::DrawPacket> m_clearShadowDrawPacket;
-            RHI::DrawItemProperties m_clearShadowDrawItemProperties;
             RHI::Handle<uint32_t> m_casterMovedBit;
             uint16_t m_arraySlice = 0;
             bool m_clearEnabled = true;

Gems/Atom/Feature/Common/Code/Source/ImGui/ImGuiPass.cpp

@@ -6,7 +6,7 @@
  *
  */
 
-#include <ImGui/ImGuiPass.h>
+#include <Atom/Feature/ImGui/ImGuiPass.h>
 
 #include <AzFramework/Input/Devices/Gamepad/InputDeviceGamepad.h>
 #include <AzFramework/Input/Devices/Keyboard/InputDeviceKeyboard.h>

Gems/Atom/Feature/Common/Code/Source/ImGui/ImGuiSystemComponent.cpp

@@ -7,8 +7,7 @@
  */
 
 #include <ImGui/ImGuiSystemComponent.h>
-#include <ImGui/ImGuiPass.h>
-
+#include <Atom/Feature/ImGui/ImGuiPass.h>
 #include <AzCore/Serialization/SerializeContext.h>
 #include <Atom/RPI.Public/Pass/PassSystemInterface.h>
 #include <Atom/RPI.Public/Pass/PassFilter.h>

Gems/Atom/Feature/Common/Code/atom_feature_common_files.cmake

@@ -108,8 +108,6 @@ set(FILES
     Source/DisplayMapper/DisplayMapperFullScreenPass.cpp
     Source/DisplayMapper/BakeAcesOutputTransformLutPass.cpp
     Source/DisplayMapper/OutputTransformPass.cpp
-    Source/ImGui/ImGuiPass.cpp
-    Source/ImGui/ImGuiPass.h
     Source/ImGui/ImGuiSystemComponent.cpp
     Source/ImGui/ImGuiSystemComponent.h
     Source/ImageBasedLights/ImageBasedLightFeatureProcessor.cpp

Gems/Atom/Feature/Common/Code/atom_feature_common_public_files.cmake

@@ -39,6 +39,7 @@ set(FILES
     Include/Atom/Feature/DisplayMapper/DisplayMapperFeatureProcessorInterface.h
     Include/Atom/Feature/ImGui/ImGuiUtils.h
     Include/Atom/Feature/ImGui/SystemBus.h
+    Include/Atom/Feature/ImGui/ImGuiPass.h
     Include/Atom/Feature/ImageBasedLights/ImageBasedLightFeatureProcessorInterface.h
     Include/Atom/Feature/LightingChannel/LightingChannelConfiguration.h
     Include/Atom/Feature/Material/ConvertEmissiveUnitFunctor.h
@@ -158,4 +159,5 @@ set(FILES
     Source/Utils/GpuBufferHandler.cpp
     Source/Utils/LightingPreset.cpp
     Source/Utils/ModelPreset.cpp
+    Source/ImGui/ImGuiPass.cpp
 )

Gems/Atom/RHI/Code/Include/Atom/RHI.Reflect/RenderAttachmentLayout.h

@@ -40,6 +40,7 @@ namespace AZ::RHI
 
         bool operator==(const RenderAttachmentDescriptor& other) const;
         bool operator!=(const RenderAttachmentDescriptor& other) const;
+        bool IsEqual(const RenderAttachmentDescriptor& other, const bool compareLoadStoreAction) const;
 
         //! Position of the attachment in the layout.
         uint32_t m_attachmentIndex = InvalidRenderAttachmentIndex;
@@ -95,6 +96,7 @@ namespace AZ::RHI
 
         bool operator==(const SubpassRenderAttachmentLayout& other) const;
         bool operator!=(const SubpassRenderAttachmentLayout& other) const;
+        bool IsEqual(const SubpassRenderAttachmentLayout& other, const bool compareLoadStoreAction) const;
 
         //! Number of render targets in the subpass.
         uint32_t m_rendertargetCount = 0;
@@ -126,6 +128,7 @@ namespace AZ::RHI
         HashValue64 GetHash() const;
 
         bool operator==(const RenderAttachmentLayout& other) const;
+        bool IsEqual(const RenderAttachmentLayout& other, const bool compareLoadStoreAction) const;
 
         //! Number of total attachments in the list.
         uint32_t m_attachmentCount = 0;
@@ -148,6 +151,7 @@ namespace AZ::RHI
         HashValue64 GetHash() const;
 
         bool operator==(const RenderAttachmentConfiguration& other) const;
+        bool IsEqual(const RenderAttachmentConfiguration& other, const bool compareLoadStoreAction) const;
 
         //! Returns the format of a render target in the subpass being used.
         Format GetRenderTargetFormat(uint32_t index) const;

Gems/Atom/RHI/Code/Source/RHI.Reflect/RenderAttachmentLayout.cpp

@@ -33,12 +33,18 @@ namespace AZ::RHI
 
     bool RenderAttachmentDescriptor::operator==(const RenderAttachmentDescriptor& other) const
     {
-        return (m_attachmentIndex == other.m_attachmentIndex)
-            && (m_resolveAttachmentIndex == other.m_resolveAttachmentIndex)
-            && (m_loadStoreAction == other.m_loadStoreAction)
-            && (m_scopeAttachmentAccess == other.m_scopeAttachmentAccess)
-            && (m_scopeAttachmentStage == other.m_scopeAttachmentStage)
-            ;
+        return IsEqual(other, true);
+    }
+
+    bool RenderAttachmentDescriptor::IsEqual(const RenderAttachmentDescriptor& other, const bool compareLoadStoreAction) const
+    {
+        // clang-format off
+        return (m_attachmentIndex == other.m_attachmentIndex) && 
+               (m_resolveAttachmentIndex == other.m_resolveAttachmentIndex) &&
+               (!compareLoadStoreAction || (m_loadStoreAction == other.m_loadStoreAction)) && 
+               (m_scopeAttachmentAccess == other.m_scopeAttachmentAccess) &&
+               (m_scopeAttachmentStage == other.m_scopeAttachmentStage);
+        // clang-format on
     }
 
     bool RenderAttachmentDescriptor::operator!=(const RenderAttachmentDescriptor& other) const
@@ -65,16 +71,20 @@ namespace AZ::RHI
 
     bool SubpassRenderAttachmentLayout::operator==(const SubpassRenderAttachmentLayout& other) const
     {
-        if ((m_rendertargetCount != other.m_rendertargetCount)
-            || (m_subpassInputCount != other.m_subpassInputCount)
-            || (m_depthStencilDescriptor != other.m_depthStencilDescriptor))
+        return IsEqual(other, true);
+    }
+
+    bool SubpassRenderAttachmentLayout::IsEqual(const SubpassRenderAttachmentLayout& other, const bool compareLoadStoreAction) const
+    {
+        if ((m_rendertargetCount != other.m_rendertargetCount) || (m_subpassInputCount != other.m_subpassInputCount) ||
+            (!m_depthStencilDescriptor.IsEqual(other.m_depthStencilDescriptor, compareLoadStoreAction)))
         {
             return false;
         }
 
         for (uint32_t i = 0; i < m_rendertargetCount; ++i)
         {
-            if (m_rendertargetDescriptors[i] != other.m_rendertargetDescriptors[i])
+            if (!m_rendertargetDescriptors[i].IsEqual(other.m_rendertargetDescriptors[i], compareLoadStoreAction))
             {
                 return false;
             }
@@ -118,8 +128,12 @@ namespace AZ::RHI
 
     bool RenderAttachmentLayout::operator==(const RenderAttachmentLayout& other) const
     {
-        if ((m_attachmentCount != other.m_attachmentCount)
-            || (m_subpassCount != other.m_subpassCount))
+        return IsEqual(other, true);
+    }
+
+    bool RenderAttachmentLayout::IsEqual(const RenderAttachmentLayout& other, const bool compareLoadStoreAction) const
+    {
+        if ((m_attachmentCount != other.m_attachmentCount) || (m_subpassCount != other.m_subpassCount))
         {
             return false;
         }
@@ -134,7 +148,7 @@ namespace AZ::RHI
 
         for (uint32_t i = 0; i < m_subpassCount; ++i)
         {
-            if(m_subpassLayouts[i] != other.m_subpassLayouts[i])
+            if (!m_subpassLayouts[i].IsEqual(other.m_subpassLayouts[i], compareLoadStoreAction))
             {
                 return false;
             }
@@ -209,7 +223,13 @@ namespace AZ::RHI
 
     bool RenderAttachmentConfiguration::operator==(const RenderAttachmentConfiguration& other) const
     {
-        return (m_renderAttachmentLayout == other.m_renderAttachmentLayout) && (m_subpassIndex == other.m_subpassIndex);
+        return IsEqual(other, true);
+    }
+
+    bool RenderAttachmentConfiguration::IsEqual(const RenderAttachmentConfiguration& other, const bool compareLoadStoreAction) const
+    {
+        return m_renderAttachmentLayout.IsEqual(other.m_renderAttachmentLayout, compareLoadStoreAction) &&
+            (m_subpassIndex == other.m_subpassIndex);
     }
 
     void SubpassInputDescriptor::Reflect(ReflectContext* context)

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

@@ -1116,8 +1116,18 @@ namespace AZ
                                 for (; index < size; ++index)
                                 {
                                     PipelineStateData stateData = pipelineStateList[index];
-                                    if (stateData.m_multisampleState == multisampleState
-                                        && stateData.m_renderAttachmentConfiguration == renderAttachmentConfg)
+
+                                    // don't compare the loadStoreAction of the renderAttachmentConfiguration here for two reasons:
+                                    //
+                                    // - The FrameGraph takes the actual LoadStoreAction from the Pass (or more exactly from the scope), and
+                                    //   not from the Renderattachment - config here
+                                    //
+                                    // - The ShadowMap - Passes are used for both CSM and projected Shadowmaps with the same drawListTag,
+                                    //   but they use different LoadStoreActions, which would cause two separate entries for the same
+                                    //   drawListTag here.
+                                    if (stateData.m_multisampleState == multisampleState &&
+                                        stateData.m_renderAttachmentConfiguration.IsEqual(
+                                            renderAttachmentConfg, false /* compareLoadStoreAction */))
                                     {
                                         break;
                                     }

Gems/RecastNavigation/Code/CMakeLists.txt

@@ -19,14 +19,26 @@ o3de_pal_dir(pal_dir ${CMAKE_CURRENT_LIST_DIR}/Platform/${PAL_PLATFORM_NAME} "${
 # is supported by this platform.
 include(${pal_dir}/PAL_${PAL_PLATFORM_NAME_LOWERCASE}.cmake)
 
+# If you change these, please change the copy of them below in the installer 
+# section.  That installer section is the ONLY part of the file that is shipped in the installer
+# (SDK) version of the package, the rest of this file is not, so it must be duplicated there.
+set(RECAST_GIT_REPO "https://github.com/recastnavigation/recastnavigation.git")
+set(RECAST_GIT_TAG 5a870d4)
+set(RECAST_GIT_PATCH "${CMAKE_CURRENT_LIST_DIR}/recast-o3de.patch")
+
 # Add RecastNavigation dependency using FetchContent
 # Note: RecastNavigation::Detour requires lowering of the warning level to compile.
 include(FetchContent)
 FetchContent_Declare(
         RecastNavigation
-        GIT_REPOSITORY "https://github.com/recastnavigation/recastnavigation"
-        GIT_TAG "5a870d427e47abd4a8e4ce58a95582ec049434d5" # main branch
+        GIT_REPOSITORY ${RECAST_GIT_REPO}
+        GIT_TAG ${RECAST_GIT_TAG} # main branch as of March 17, 2022
+        PATCH_COMMAND cmake -P "${LY_ROOT_FOLDER}/cmake/PatchIfNotAlreadyPatched.cmake" ${RECAST_GIT_PATCH}
 )
+
+# please always be really clear about what third parties your gem uses.
+message(STATUS "RecastNavigation Gem uses ${RECAST_GIT_REPO} commit 5a870d4 (License: Zlib)")
+
 set(CMAKE_POLICY_DEFAULT_CMP0077 NEW)
 set(RECASTNAVIGATION_DEMO OFF)
 set(RECASTNAVIGATION_TESTS OFF)
@@ -71,7 +83,14 @@ foreach(recastLibrary ${recastLibraries})
 endforeach()
 
 # Export external recast navigation targets for installers.
+# Note that the following property in the square brackets is the only part of this entire file
+# that gets included in installers.  This is why we need to duplicate the github repo from above
+# and cannot reuse the variables above.  The rest of this file won't be there to refer to.
 set_property(DIRECTORY APPEND PROPERTY O3DE_SUBDIRECTORY_INSTALL_CODE [[
+    set(RECAST_GIT_REPO "https://github.com/recastnavigation/recastnavigation.git")
+    set(RECAST_GIT_TAG 5a870d4)
+    message(STATUS "RecastNavigation Gem uses ${RECAST_GIT_REPO} commit 5a870d4 (License: Zlib)")
+    
     set(recastLibraries DebugUtils;Detour;DetourCrowd;DetourTileCache;Recast)
     foreach(recastLibrary ${recastLibraries})
         add_library(RecastNavigation::${recastLibrary} STATIC IMPORTED GLOBAL)

Gems/RecastNavigation/Code/recast-o3de.patch

@@ -0,0 +1,10 @@
+diff --git a/CMakeLists.txt b/CMakeLists.txt
+index c3121c6..f4b8e73 100644
+--- a/CMakeLists.txt
++++ b/CMakeLists.txt
+@@ -1,4 +1,4 @@
+-cmake_minimum_required(VERSION 3.0)
++cmake_minimum_required(VERSION 3.22)
+ 
+ project(RecastNavigation)
+ 

scripts/o3de/o3de/engine_template.py

@@ -95,6 +95,14 @@ O3DE_LICENSE_TEXT = \
 # {END_LICENSE}
 """
 
+# This is a list of reserved words that should not be used for project names, gem names, etc.
+TEMPLATE_RESERVED_WORDS = [ "project", "project_name", "and", "or", "not", "in", "is", "if", "else", "elif", "while",
+                            "for", "return", "break", "continue", "def", "class", "import", "from", "name", "version",
+                            "as", "with", "try", "except", "finally", "raise", "assert" , "projectid", "o3de", "sdk",
+                            "nameupper", "namelower", "sanitizedcppname", "projectpath", "enginepath", "moduleclassid",
+# and then all the reserved words that you don't want to see in cmake files that mean something to cmake:
+                            "editorsyscompclassid", "module", "shared", "static", "namespace", "target"]
+
 this_script_parent = pathlib.Path(os.path.dirname(os.path.realpath(__file__)))
 
 
@@ -577,6 +585,11 @@ def create_template(source_path: pathlib.Path,
     if template_name in restricted_platforms:
         logger.error(f'Template path cannot be a restricted name. {template_name}')
         return 1
+    
+    # there are some reserved words that it is bad idea to use as a template name
+    if template_name.lower() in TEMPLATE_RESERVED_WORDS:
+        logger.error(f'Template name cannot be "{template_name}" as this might cause issues with compilation.  Please try another name.')
+        return 1
 
     # if the source restricted name was given and no source restricted path, look up the restricted name to fill
     # in the path
@@ -1724,18 +1737,17 @@ def create_project(project_path: pathlib.Path,
         logger.error(
             f'Project name must be fewer than 64 characters, contain only alphanumeric, "_" or "-" characters, and start with a letter.  {project_name}')
         return 1
+    
+    # there are some reserved words that it is bad idea to use as a project name
+    if project_name.lower() in TEMPLATE_RESERVED_WORDS:
+        logger.error(f'Project name cannot be "{project_name}" as this might cause issues with compilation.  Please try another name.')
+        return 1
 
     # project name cannot be the same as a restricted platform name
     if project_name in restricted_platforms:
         logger.error(f'Project name cannot be a restricted name. {project_name}')
         return 1
 
-    # the generic launcher (and the engine, often) are referred to as o3de, so prevent the user from 
-    # accidentally creating a confusing error situation.
-    if project_name.lower() == 'o3de':
-        logger.error(f"Project name cannot be 'o3de' as this is reserved for the generic launcher.")
-        return 1
-
     # project restricted name
     if project_restricted_name and not project_restricted_path:
         gem_restricted_path = manifest.get_registered(restricted_name=project_restricted_name)
@@ -2144,6 +2156,11 @@ def create_gem(gem_path: pathlib.Path,
     if gem_name in restricted_platforms:
         logger.error(f'Gem path cannot be a restricted name. {gem_name}')
         return 1
+    
+    # there are some reserved words that it is bad idea to use as a gem name
+    if gem_name.lower() in TEMPLATE_RESERVED_WORDS:
+        logger.error(f'Project name cannot be "{gem_name}" as this might cause issues with compilation.  Please try another name.')
+        return 1
 
     # gem restricted name
     if gem_restricted_name and not gem_restricted_path: