Merge remote-tracking branch 'upstream/development' into camera_and_editor_viewport_widget_improvements

Signed-off-by: Yuriy Toporovskyy <toporovskyy.y@gmail.com>

Code/Framework/AzCore/AzCore/IO/Streamer/BlockCache.cpp

@@ -47,7 +47,7 @@ namespace AZ
             }
 
             auto stackEntry = AZStd::make_shared<BlockCache>(
-                cacheSize, blockSize, aznumeric_caster(hardware.m_maxPhysicalSectorSize), false);
+                cacheSize, aznumeric_cast<AZ::u32>(blockSize), aznumeric_cast<AZ::u32>(hardware.m_maxPhysicalSectorSize), false);
             stackEntry->SetNext(AZStd::move(parent));
             return stackEntry;
         }

Code/Framework/AzCore/AzCore/IO/Streamer/DedicatedCache.cpp

@@ -45,7 +45,7 @@ namespace AZ
             }
 
             auto stackEntry = AZStd::make_shared<DedicatedCache>(
-                cacheSize, blockSize, aznumeric_caster(hardware.m_maxPhysicalSectorSize), m_writeOnlyEpilog);
+                cacheSize, aznumeric_cast<AZ::u32>(blockSize), aznumeric_cast<AZ::u32>(hardware.m_maxPhysicalSectorSize), m_writeOnlyEpilog);
             stackEntry->SetNext(AZStd::move(parent));
             return stackEntry;
         }

Code/Framework/AzCore/AzCore/Math/Aabb.h

@@ -43,7 +43,7 @@ namespace AZ
         static Aabb CreateCenterRadius(const Vector3& center, float radius);
 
         //! Creates an AABB which contains the specified points.
-        static Aabb CreatePoints(const Vector3* pts, int numPts);
+        static Aabb CreatePoints(const Vector3* pts, size_t numPts);
 
         //! Creates an AABB which contains the specified OBB.
         static Aabb CreateFromObb(const Obb& obb);

Code/Framework/AzCore/AzCore/Math/Aabb.inl

@@ -60,10 +60,10 @@ namespace AZ
     }
 
 
-    AZ_MATH_INLINE Aabb Aabb::CreatePoints(const Vector3* pts, int numPts)
+    AZ_MATH_INLINE Aabb Aabb::CreatePoints(const Vector3* pts, size_t numPts)
     {
         Aabb aabb = Aabb::CreateFromPoint(pts[0]);
-        for (int i = 1; i < numPts; ++i)
+        for (size_t i = 1; i < numPts; ++i)
         {
             aabb.AddPoint(pts[i]);
         }

Code/Framework/AzCore/AzCore/Module/Environment.cpp

@@ -11,6 +11,7 @@
 #include <AzCore/Memory/OSAllocator.h>
 #include <AzCore/std/containers/unordered_map.h>
 #include <AzCore/Math/Crc.h>
+#include <AzCore/std/parallel/scoped_lock.h>
 
 namespace AZ
 {
@@ -75,6 +76,42 @@ namespace AZ
         bool operator==(const OSStdAllocator& a, const OSStdAllocator& b) { (void)a; (void)b; return true; }
         bool operator!=(const OSStdAllocator& a, const OSStdAllocator& b) { (void)a; (void)b; return false; }
 
+        void EnvironmentVariableHolderBase::UnregisterAndDestroy(DestructFunc destruct, bool moduleRelease)
+        {
+            const bool releaseByUseCount = (--m_useCount == 0);
+            // We take over the lock, and release it before potentially destroying/freeing ourselves
+            {
+                AZStd::scoped_lock envLockHolder(AZStd::adopt_lock, m_mutex);
+                const bool releaseByModule = (moduleRelease && !m_canTransferOwnership && m_moduleOwner == AZ::Environment::GetModuleId());
+
+                if (!releaseByModule && !releaseByUseCount)
+                {
+                    return;
+                }
+                // if the environment that created us is gone the owner can be null
+                // which means (assuming intermodule allocator) that the variable is still alive
+                // but can't be found as it's not part of any environment.
+                if (m_environmentOwner)
+                {
+                    m_environmentOwner->RemoveVariable(m_guid);
+                    m_environmentOwner = nullptr;
+                }
+                if (m_isConstructed)
+                {
+                    destruct(this, DestroyTarget::Member); // destruct the value
+                }
+            }
+            // m_mutex is no longer held here, envLockHolder has released it above.
+            if (releaseByUseCount)
+            {
+                // m_mutex is unlocked before this is deleted
+                Environment::AllocatorInterface* allocator = m_allocator;
+                // Call child class dtor and clear the memory
+                destruct(this, DestroyTarget::Self);
+                allocator->DeAllocate(this);
+            }
+        }
+
         // instance of the environment
         EnvironmentInterface* EnvironmentInterface::s_environment = nullptr;
 
@@ -110,7 +147,7 @@ namespace AZ
 #ifdef AZ_ENVIRONMENT_VALIDATE_ON_EXIT
                 AZ_Assert(m_numAttached == 0, "We should not delete an environment while there are %d modules attached! Unload all DLLs first!", m_numAttached);
 #endif
-                
+
                 for (auto variableIt : m_variableMap)
                 {
                     EnvironmentVariableHolderBase* holder = reinterpret_cast<EnvironmentVariableHolderBase*>(variableIt.second);

Code/Framework/AzCore/AzCore/Module/Environment.h

@@ -200,6 +200,11 @@ namespace AZ
         class EnvironmentVariableHolderBase
         {
             friend class EnvironmentImpl;
+        protected:
+            enum class DestroyTarget {
+                Member,
+                Self
+            };
         public:
             EnvironmentVariableHolderBase(u32 guid, AZ::Internal::EnvironmentInterface* environmentOwner, bool canOwnershipTransfer, Environment::AllocatorInterface* allocator)
                 : m_environmentOwner(environmentOwner)
@@ -217,12 +222,21 @@ namespace AZ
                 return m_isConstructed;
             }
 
+            bool IsOwner() const
+            {
+                return m_moduleOwner == Environment::GetModuleId();
+            }
+
             u32 GetId() const
             {
                 return m_guid;
             }
-
         protected:
+            using DestructFunc = void (*)(EnvironmentVariableHolderBase *, DestroyTarget);
+            // Assumes the m_mutex is already locked.
+            // On return m_mutex is in an unlocked state.
+            void UnregisterAndDestroy(DestructFunc destruct, bool moduleRelease);
+
             AZ::Internal::EnvironmentInterface* m_environmentOwner; ///< Used to know which environment we should use to free the variable if we can't transfer ownership
             void* m_moduleOwner; ///< Used when the variable can't transfered across module and we need to destruct the variable when the module is going away
             bool m_canTransferOwnership; ///< True if variable can be allocated in one module and freed in other. Usually true for POD types when they share allocator.
@@ -242,41 +256,29 @@ namespace AZ
                 memset(&m_value, 0, sizeof(T));
             }
 
-            template <class... Args>
+            template<class... Args>
             void ConstructImpl(const AZStd::false_type& /* AZStd::has_trivial_constructor<T> */, Args&&... args)
             {
                 // Construction of non-trivial types is left up to the type's constructor.
                 new(&m_value) T(AZStd::forward<Args>(args)...);
             }
-
-            void DestructImpl(const AZStd::true_type& /* AZStd::is_trivially_destructible<T> */)
+            static void DestructDispatchNoLock(EnvironmentVariableHolderBase *base, DestroyTarget selfDestruct)
             {
-                // do nothing
-            }
-
-            void DestructImpl(const AZStd::false_type& /* AZStd::is_trivially_destructible<T> */)
-            {
-                reinterpret_cast<T*>(&m_value)->~T();
-            }
-
-            // Assumes the lock is already held
-            void UnregisterAndDestruct()
-            {
-                // if the environment that created us is gone the owner can be null
-                // which means (assuming intermodule allocator) that the variable is still alive
-                // but can't be found as it's not part of any environment.
-                if (m_environmentOwner)
+                auto *self = reinterpret_cast<EnvironmentVariableHolder *>(base);
+                if (selfDestruct == DestroyTarget::Self)
                 {
-                    m_environmentOwner->RemoveVariable(m_guid);
-                    m_environmentOwner = nullptr;
+                    self->~EnvironmentVariableHolder();
+                    return;
                 }
 
-                if (m_isConstructed)
+                AZ_Assert(self->m_isConstructed, "Variable is not constructed. Please check your logic and guard if needed!");
+                self->m_isConstructed = false;
+                self->m_moduleOwner = nullptr;
+                if constexpr(!AZStd::is_trivially_destructible_v<T>)
                 {
-                    DestructNoLock();
+                    reinterpret_cast<T*>(&self->m_value)->~T();
                 }
             }
-
         public:
             EnvironmentVariableHolder(u32 guid, bool isOwnershipTransfer, Environment::AllocatorInterface* allocator)
                 : EnvironmentVariableHolderBase(guid, Environment::GetInstance(), isOwnershipTransfer, allocator)
@@ -287,12 +289,6 @@ namespace AZ
             {
                 AZ_Assert(!m_isConstructed, "To get the destructor we should have already destructed the variable!");
             }
-
-            bool IsOwner() const
-            {
-                return m_moduleOwner == Environment::GetModuleId();
-            }
-
             void AddRef()
             {
                 AZStd::lock_guard<AZStd::spin_mutex> lock(m_mutex);
@@ -303,30 +299,8 @@ namespace AZ
             void Release()
             {
                 m_mutex.lock();
-
-                if (--s_moduleUseCount == 0)
-                {
-                    if (!m_canTransferOwnership && m_moduleOwner == AZ::Environment::GetModuleId())
-                    {
-                        UnregisterAndDestruct();
-                    }
-                }
-
-                if (--m_useCount == 0)
-                {
-                    UnregisterAndDestruct();
-
-                    // unlock before this is deleted
-                    m_mutex.unlock();
-
-                    Environment::AllocatorInterface* allocator = m_allocator;
-                    // Call dtor and clear the memory
-                    this->~EnvironmentVariableHolder();
-                    allocator->DeAllocate(this);
-                    return;
-                }
-                
-                m_mutex.unlock();
+                const bool moduleRelease = (--s_moduleUseCount == 0);
+                UnregisterAndDestroy(DestructDispatchNoLock, moduleRelease);
             }
 
             void Construct()
@@ -352,18 +326,10 @@ namespace AZ
                 }
             }
 
-            void DestructNoLock()
-            {
-                AZ_Assert(m_isConstructed, "Variable is not constructed. Please check your logic and guard if needed!");
-                m_isConstructed = false;
-                m_moduleOwner = nullptr;
-                DestructImpl(typename AZStd::is_trivially_destructible<T>::type());
-            }
-
             void Destruct()
             {
                 AZStd::lock_guard<AZStd::spin_mutex> lock(m_mutex);
-                DestructNoLock();
+                DestructDispatchNoLock(this, DestroyTarget::Member);
             }
 
             // variable storage

Code/Framework/AzCore/AzCore/Serialization/Json/ByteStreamSerializer.cpp

@@ -71,7 +71,7 @@ namespace AZ
         if (context.ShouldKeepDefaults() || !defaultValue || (valAsByteStream != *static_cast<const JsonByteStream*>(defaultValue)))
         {
             const auto base64ByteStream = AZ::StringFunc::Base64::Encode(valAsByteStream.data(), valAsByteStream.size());
-            outputValue.SetString(base64ByteStream.c_str(), base64ByteStream.size(), context.GetJsonAllocator());
+            outputValue.SetString(base64ByteStream.c_str(), static_cast<rapidjson::SizeType>(base64ByteStream.size()), context.GetJsonAllocator());
             return context.Report(Tasks::WriteValue, Outcomes::Success, "ByteStream successfully stored.");
         }
 

Code/Framework/AzCore/AzCore/std/chrono/types.h

@@ -211,7 +211,7 @@ namespace AZStd
             // 20.9.3.2, observer:
             constexpr rep count() const { return m_rep; }
             // 20.9.3.3, arithmetic:
-            constexpr duration operator+() const { *this; }
+            constexpr duration operator+() const { return *this; }
             constexpr duration operator-() const { return duration(-m_rep); }
             constexpr duration& operator++() { ++m_rep; return *this; }
             constexpr duration operator++(int) { return duration(m_rep++); }

Code/Framework/AzCore/AzCore/std/containers/ring_buffer.h

@@ -1056,7 +1056,7 @@ namespace AZStd
         inline void insert(const iterator& pos, ForwardIterator first, ForwardIterator last, const AZStd::forward_iterator_tag&)
         {
             size_type size = AZStd::distance(first, last);
-            AZSTD_CONTAINER_ASSERT(size >= 0, "AZStd::ring_buffer::insert - there are no elements to insert!");
+            AZSTD_CONTAINER_ASSERT(first > last, "AZStd::ring_buffer::insert - there are no elements to insert!");
             if (size == 0)
             {
                 return;

Code/Framework/AzCore/AzCore/std/utils.h

@@ -294,7 +294,7 @@ namespace AZStd
             T& m_v;
             constexpr addr_impl_ref(T& v)
                 : m_v(v) {}
-            constexpr addr_impl_ref& operator=(const addr_impl_ref& v) { m_v = v; }
+            constexpr addr_impl_ref& operator=(const addr_impl_ref& v) { m_v = v; return *this; }
             constexpr operator T& () const { return m_v; }
         };