diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp index 862e3599a3..71e880fdef 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp @@ -95,6 +95,7 @@ namespace SurfaceData m_refresh = false; // Update the cached mesh data and bounds, then register the surface data provider + AssignSurfaceTagWeights(m_configuration.m_tags, 1.0f, m_newPointWeights); UpdateMeshData(); } @@ -115,7 +116,7 @@ namespace SurfaceData // Clear the cached mesh data { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::unique_lock lock(m_cacheMutex); m_meshAssetData = {}; m_meshBounds = AZ::Aabb::CreateNull(); m_meshWorldTM = AZ::Transform::CreateIdentity(); @@ -145,7 +146,7 @@ namespace SurfaceData bool SurfaceDataMeshComponent::DoRayTrace(const AZ::Vector3& inPosition, AZ::Vector3& outPosition, AZ::Vector3& outNormal) const { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::shared_lock lock(m_cacheMutex); // test AABB as first pass to claim the point const AZ::Vector3 testPosition = AZ::Vector3( @@ -181,8 +182,8 @@ namespace SurfaceData point.m_entityId = GetEntityId(); point.m_position = hitPosition; point.m_normal = hitNormal; - AddMaxValueForMasks(point.m_masks, m_configuration.m_tags, 1.0f); - surfacePointList.push_back(point); + point.m_masks = m_newPointWeights; + surfacePointList.push_back(AZStd::move(point)); } } @@ -235,7 +236,7 @@ namespace SurfaceData bool meshValidAfterUpdate = false; { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::unique_lock lock(m_cacheMutex); meshValidBeforeUpdate = (m_meshAssetData.GetAs() != nullptr) && (m_meshBounds.IsValid()); diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h index 92536f9523..758bb1ee99 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -96,11 +97,12 @@ namespace SurfaceData // cached data AZStd::atomic_bool m_refresh{ false }; - mutable AZStd::recursive_mutex m_cacheMutex; + mutable AZStd::shared_mutex m_cacheMutex; AZ::Data::Asset m_meshAssetData; AZ::Transform m_meshWorldTM = AZ::Transform::CreateIdentity(); AZ::Transform m_meshWorldTMInverse = AZ::Transform::CreateIdentity(); AZ::Vector3 m_meshNonUniformScale = AZ::Vector3::CreateOne(); AZ::Aabb m_meshBounds = AZ::Aabb::CreateNull(); + SurfaceTagWeightMap m_newPointWeights; }; } diff --git a/Gems/SurfaceData/Code/Include/SurfaceData/Utility/SurfaceDataUtility.h b/Gems/SurfaceData/Code/Include/SurfaceData/Utility/SurfaceDataUtility.h index 29b3fac8c7..2a4a39ee3b 100644 --- a/Gems/SurfaceData/Code/Include/SurfaceData/Utility/SurfaceDataUtility.h +++ b/Gems/SurfaceData/Code/Include/SurfaceData/Utility/SurfaceDataUtility.h @@ -88,6 +88,16 @@ namespace SurfaceData const AZ::Vector3& rayStart, const AZ::Vector3& rayEnd, AZ::Vector3& outPosition, AZ::Vector3& outNormal); + AZ_INLINE void AssignSurfaceTagWeights(const SurfaceTagVector& tags, float weight, SurfaceTagWeightMap& weights) + { + weights.clear(); + weights.reserve(tags.size()); + for (auto& tag : tags) + { + weights[tag] = weight; + } + } + AZ_INLINE void AddMaxValueForMasks(SurfaceTagWeightMap& masks, const AZ::Crc32 tag, const float value) { const auto maskItr = masks.find(tag); @@ -166,7 +176,8 @@ namespace SurfaceData } template - AZ_INLINE bool HasMatchingTags(const SurfaceTagWeightMap& sourceTags, const SampleContainer& sampleTags, float valueMin, float valueMax) + AZ_INLINE bool HasMatchingTags( + const SurfaceTagWeightMap& sourceTags, const SampleContainer& sampleTags, float valueMin, float valueMax) { for (const auto& sampleTag : sampleTags) { diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp index d7de299829..dc4e5e6e74 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp @@ -132,6 +132,7 @@ namespace SurfaceData Physics::ColliderComponentEventBus::Handler::BusConnect(GetEntityId()); // Update the cached collider data and bounds, then register the surface data provider / modifier + AssignSurfaceTagWeights(m_configuration.m_providerTags, 1.0f, m_newPointWeights); UpdateColliderData(); } @@ -157,7 +158,7 @@ namespace SurfaceData // Clear the cached mesh data { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::unique_lock lock(m_cacheMutex); m_colliderBounds = AZ::Aabb::CreateNull(); } } @@ -184,7 +185,7 @@ namespace SurfaceData bool SurfaceDataColliderComponent::DoRayTrace(const AZ::Vector3& inPosition, bool queryPointOnly, AZ::Vector3& outPosition, AZ::Vector3& outNormal) const { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::shared_lock lock(m_cacheMutex); // test AABB as first pass to claim the point const AZ::Vector3 testPosition = AZ::Vector3( @@ -240,17 +241,14 @@ namespace SurfaceData point.m_entityId = GetEntityId(); point.m_position = hitPosition; point.m_normal = hitNormal; - for (auto& tag : m_configuration.m_providerTags) - { - point.m_masks[tag] = 1.0f; - } - surfacePointList.push_back(point); + point.m_masks = m_newPointWeights; + surfacePointList.push_back(AZStd::move(point)); } } void SurfaceDataColliderComponent::ModifySurfacePoints(SurfacePointList& surfacePointList) const { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::shared_lock lock(m_cacheMutex); if (m_colliderBounds.IsValid() && !m_configuration.m_modifierTags.empty()) { @@ -308,7 +306,7 @@ namespace SurfaceData bool colliderValidAfterUpdate = false; { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::unique_lock lock(m_cacheMutex); colliderValidBeforeUpdate = m_colliderBounds.IsValid(); diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.h b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.h index 43528ecae3..50c84f9519 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.h +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -98,7 +99,8 @@ namespace SurfaceData // cached data AZStd::atomic_bool m_refresh{ false }; - mutable AZStd::recursive_mutex m_cacheMutex; + mutable AZStd::shared_mutex m_cacheMutex; AZ::Aabb m_colliderBounds = AZ::Aabb::CreateNull(); + SurfaceTagWeightMap m_newPointWeights; }; } diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp index a45903498e..682bac150a 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp @@ -89,6 +89,7 @@ namespace SurfaceData LmbrCentral::ShapeComponentNotificationsBus::Handler::BusConnect(GetEntityId()); // Update the cached shape data and bounds, then register the surface data provider / modifier + AssignSurfaceTagWeights(m_configuration.m_providerTags, 1.0f, m_newPointWeights); UpdateShapeData(); } @@ -115,7 +116,7 @@ namespace SurfaceData // Clear the cached shape data { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::unique_lock lock(m_cacheMutex); m_shapeBounds = AZ::Aabb::CreateNull(); m_shapeBoundsIsValid = false; } @@ -143,7 +144,7 @@ namespace SurfaceData void SurfaceDataShapeComponent::GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::shared_lock lock(m_cacheMutex); if (m_shapeBoundsIsValid) { @@ -158,36 +159,35 @@ namespace SurfaceData point.m_entityId = GetEntityId(); point.m_position = rayOrigin + intersectionDistance * rayDirection; point.m_normal = AZ::Vector3::CreateAxisZ(); - for (auto& tag : m_configuration.m_providerTags) - { - point.m_masks[tag] = 1.0f; - } - surfacePointList.push_back(point); + point.m_masks = m_newPointWeights; + surfacePointList.push_back(AZStd::move(point)); } } } void SurfaceDataShapeComponent::ModifySurfacePoints(SurfacePointList& surfacePointList) const { - AZ_PROFILE_FUNCTION(Entity); - - AZStd::lock_guard lock(m_cacheMutex); + AZStd::shared_lock lock(m_cacheMutex); if (m_shapeBoundsIsValid && !m_configuration.m_modifierTags.empty()) { const AZ::EntityId entityId = GetEntityId(); - for (auto& point : surfacePointList) - { - if (point.m_entityId != entityId && m_shapeBounds.Contains(point.m_position)) + LmbrCentral::ShapeComponentRequestsBus::Event( + GetEntityId(), + [entityId, this, &surfacePointList](LmbrCentral::ShapeComponentRequestsBus::Events* shape) { - bool inside = false; - LmbrCentral::ShapeComponentRequestsBus::EventResult(inside, GetEntityId(), &LmbrCentral::ShapeComponentRequestsBus::Events::IsPointInside, point.m_position); - if (inside) + for (auto& point : surfacePointList) { - AddMaxValueForMasks(point.m_masks, m_configuration.m_modifierTags, 1.0f); + if (point.m_entityId != entityId && m_shapeBounds.Contains(point.m_position)) + { + bool inside = shape->IsPointInside(point.m_position); + if (inside) + { + AddMaxValueForMasks(point.m_masks, m_configuration.m_modifierTags, 1.0f); + } + } } - } - } + }); } } @@ -228,7 +228,7 @@ namespace SurfaceData bool shapeValidAfterUpdate = false; { - AZStd::lock_guard lock(m_cacheMutex); + AZStd::unique_lock lock(m_cacheMutex); shapeValidBeforeUpdate = m_shapeBoundsIsValid; diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.h b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.h index f2c478ed27..59b7950412 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.h +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -92,9 +93,10 @@ namespace SurfaceData // cached data AZStd::atomic_bool m_refresh{ false }; - mutable AZStd::recursive_mutex m_cacheMutex; + mutable AZStd::shared_mutex m_cacheMutex; AZ::Aabb m_shapeBounds = AZ::Aabb::CreateNull(); bool m_shapeBoundsIsValid = false; static const float s_rayAABBHeightPadding; + SurfaceTagWeightMap m_newPointWeights; }; } diff --git a/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.cpp b/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.cpp index a2a3bc352e..66f282d12b 100644 --- a/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.cpp +++ b/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.cpp @@ -181,10 +181,10 @@ namespace SurfaceData void SurfaceDataSystemComponent::GetSurfacePoints(const AZ::Vector3& inPosition, const SurfaceTagVector& desiredTags, SurfacePointList& surfacePointList) const { - const bool hasDesiredTags = HasValidTags(desiredTags); - const bool hasModifierTags = hasDesiredTags && HasMatchingTags(desiredTags, m_registeredModifierTags); + const bool useTagFilters = HasValidTags(desiredTags); + const bool hasModifierTags = useTagFilters && HasMatchingTags(desiredTags, m_registeredModifierTags); - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::shared_lock registrationLock(m_registrationMutex); surfacePointList.clear(); @@ -195,7 +195,7 @@ namespace SurfaceData const SurfaceDataRegistryEntry& entry = entryPair.second; if (!entry.m_bounds.IsValid() || AabbContains2D(entry.m_bounds, inPosition)) { - if (!hasDesiredTags || hasModifierTags || HasMatchingTags(desiredTags, entry.m_tags)) + if (!useTagFilters || hasModifierTags || HasMatchingTags(desiredTags, entry.m_tags)) { SurfaceDataProviderRequestBus::Event(entryAddress, &SurfaceDataProviderRequestBus::Events::GetSurfacePoints, inPosition, surfacePointList); } @@ -219,7 +219,12 @@ namespace SurfaceData // same XY coordinates and extremely similar Z values. This produces results that are sorted in decreasing Z order. // Also, this filters out any remaining points that don't match the desired tag list. This can happen when a surface provider // doesn't add a desired tag, and a surface modifier has the *potential* to add it, but then doesn't. - CombineSortAndFilterNeighboringPoints(surfacePointList, hasDesiredTags, desiredTags); + if (useTagFilters) + { + FilterPoints(surfacePointList, desiredTags); + } + + CombineAndSortNeighboringPoints(surfacePointList); } } @@ -248,34 +253,33 @@ namespace SurfaceData void SurfaceDataSystemComponent::GetSurfacePointsFromList( AZStd::span inPositions, const SurfaceTagVector& desiredTags, SurfacePointLists& surfacePointLists) const { - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::shared_lock registrationLock(m_registrationMutex); const size_t totalQueryPositions = inPositions.size(); surfacePointLists.clear(); surfacePointLists.resize(totalQueryPositions); - const bool hasDesiredTags = HasValidTags(desiredTags); - const bool hasModifierTags = hasDesiredTags && HasMatchingTags(desiredTags, m_registeredModifierTags); + const bool useTagFilters = HasValidTags(desiredTags); + const bool hasModifierTags = useTagFilters && HasMatchingTags(desiredTags, m_registeredModifierTags); // Loop through each data provider, and query all the points for each one. This allows us to check the tags and the overall // AABB bounds just once per provider, instead of once per point. It also allows for an eventual optimization in which we could // send the list of points directly into each SurfaceDataProvider. - for (const auto& entryPair : m_registeredSurfaceDataProviders) + for (const auto& [providerHandle, provider] : m_registeredSurfaceDataProviders) { - const SurfaceDataRegistryEntry& entry = entryPair.second; - bool alwaysApplies = !entry.m_bounds.IsValid(); + bool hasInfiniteBounds = !provider.m_bounds.IsValid(); - if (!hasDesiredTags || hasModifierTags || HasMatchingTags(desiredTags, entry.m_tags)) + if (!useTagFilters || hasModifierTags || HasMatchingTags(desiredTags, provider.m_tags)) { for (size_t index = 0; index < totalQueryPositions; index++) { - const auto& inPosition = inPositions[index]; - SurfacePointList& surfacePointList = surfacePointLists[index]; - if (alwaysApplies || AabbContains2D(entry.m_bounds, inPosition)) + bool inBounds = hasInfiniteBounds || AabbContains2D(provider.m_bounds, inPositions[index]); + if (inBounds) { SurfaceDataProviderRequestBus::Event( - entryPair.first, &SurfaceDataProviderRequestBus::Events::GetSurfacePoints, inPosition, surfacePointList); + providerHandle, &SurfaceDataProviderRequestBus::Events::GetSurfacePoints, + inPositions[index], surfacePointLists[index]); } } } @@ -289,7 +293,7 @@ namespace SurfaceData for (const auto& entryPair : m_registeredSurfaceDataModifiers) { const SurfaceDataRegistryEntry& entry = entryPair.second; - bool alwaysApplies = !entry.m_bounds.IsValid(); + bool hasInfiniteBounds = !entry.m_bounds.IsValid(); for (size_t index = 0; index < totalQueryPositions; index++) { @@ -297,10 +301,11 @@ namespace SurfaceData SurfacePointList& surfacePointList = surfacePointLists[index]; if (!surfacePointList.empty()) { - if (alwaysApplies || AabbContains2D(entry.m_bounds, inPosition)) + if (hasInfiniteBounds || AabbContains2D(entry.m_bounds, inPosition)) { SurfaceDataModifierRequestBus::Event( - entryPair.first, &SurfaceDataModifierRequestBus::Events::ModifySurfacePoints, surfacePointList); + entryPair.first, &SurfaceDataModifierRequestBus::Events::ModifySurfacePoints, + surfacePointList); } } } @@ -312,88 +317,85 @@ namespace SurfaceData // doesn't add a desired tag, and a surface modifier has the *potential* to add it, but then doesn't. for (auto& surfacePointList : surfacePointLists) { - if (!surfacePointList.empty()) + if (useTagFilters) { - CombineSortAndFilterNeighboringPoints(surfacePointList, hasDesiredTags, desiredTags); + FilterPoints(surfacePointList, desiredTags); } + CombineAndSortNeighboringPoints(surfacePointList); } - } - + } - void SurfaceDataSystemComponent::CombineSortAndFilterNeighboringPoints(SurfacePointList& sourcePointList, bool hasDesiredTags, const SurfaceTagVector& desiredTags) const + void SurfaceDataSystemComponent::FilterPoints(SurfacePointList& sourcePointList, const SurfaceTagVector& desiredTags) const { - AZ_PROFILE_FUNCTION(Entity); + // Before sorting and combining, filter out any points that don't match our search tags. + sourcePointList.erase( + AZStd::remove_if( + sourcePointList.begin(), sourcePointList.end(), + [desiredTags](SurfacePoint& point) -> bool + { + return !HasMatchingTags(point.m_masks, desiredTags); + }), + sourcePointList.end()); + } - if (sourcePointList.empty()) + void SurfaceDataSystemComponent::CombineAndSortNeighboringPoints(SurfacePointList& sourcePointList) const + { + // If there's only 0 or 1 point, there is no sorting or combining that needs to happen, so just return. + if (sourcePointList.size() <= 1) { return; } - // Sorting only makes sense if we have two or more points - if (sourcePointList.size() > 1) + // Efficient point consolidation requires the points to be pre-sorted so we are only comparing/combining neighbors. + // Sort XY points together, with decreasing Z. + AZStd::sort(sourcePointList.begin(), sourcePointList.end(), [](const SurfacePoint& a, const SurfacePoint& b) { - //sort by depth/distance before combining points - AZStd::sort(sourcePointList.begin(), sourcePointList.end(), [](const SurfacePoint& a, const SurfacePoint& b) + // Our goal is to have identical XY values sorted adjacent to each other with decreasing Z. + // We sort increasing Y, then increasing X, then decreasing Z, because we need to compare all 3 values for a + // stable sort. The choice of increasing Y first is because we'll often generate the points as ranges of X values within + // ranges of Y values, so this will produce the most usable and expected output sort. + if (a.m_position.GetY() != b.m_position.GetY()) { - return a.m_position.GetZ() > b.m_position.GetZ(); - }); - } - - - //efficient point consolidation requires the points to be pre-sorted so we are only comparing/combining neighbors - const size_t sourcePointCount = sourcePointList.size(); - size_t targetPointIndex = 0; - size_t sourcePointIndex = 0; - - m_targetPointList.clear(); - m_targetPointList.reserve(sourcePointCount); - - // Locate the first point that matches our desired tags, if one exists. - for (sourcePointIndex = 0; sourcePointIndex < sourcePointCount; sourcePointIndex++) - { - if (!hasDesiredTags || (HasMatchingTags(sourcePointList[sourcePointIndex].m_masks, desiredTags))) + return a.m_position.GetY() < b.m_position.GetY(); + } + if (a.m_position.GetX() != b.m_position.GetX()) { - break; + return a.m_position.GetX() < b.m_position.GetX(); + } + if (a.m_position.GetZ() != b.m_position.GetZ()) + { + return a.m_position.GetZ() > b.m_position.GetZ(); } - } - if (sourcePointIndex < sourcePointCount) + // If we somehow ended up with two points with identical positions getting generated, use the entity ID as the tiebreaker + // to guarantee a stable sort. We should never have two identical positions generated from the same entity. + return a.m_entityId < b.m_entityId; + }); + + // iterate over subsequent source points for comparison and consolidation with the last added target/unique point + for (auto pointItr = sourcePointList.begin() + 1; pointItr < sourcePointList.end();) { - // We found a point that matches our tags, so add it to our target list as the first point. - m_targetPointList.push_back(sourcePointList[sourcePointIndex++]); + auto prevPointItr = pointItr - 1; - //iterate over subsequent source points for comparison and consolidation with the last added target/unique point - for (; sourcePointIndex < sourcePointCount; ++sourcePointIndex) + // (Someday we should add a configurable tolerance for comparison) + if (pointItr->m_position.IsClose(prevPointItr->m_position) && pointItr->m_normal.IsClose(prevPointItr->m_normal)) { - const auto& sourcePoint = sourcePointList[sourcePointIndex]; - - if (!hasDesiredTags || (HasMatchingTags(sourcePoint.m_masks, desiredTags))) - { - auto& targetPoint = m_targetPointList[targetPointIndex]; - - // [LY-90907] need to add a configurable tolerance for comparison - if (targetPoint.m_position.IsClose(sourcePoint.m_position) && - targetPoint.m_normal.IsClose(sourcePoint.m_normal)) - { - //consolidate points with similar attributes by adding masks to the target point and ignoring the source - AddMaxValueForMasks(targetPoint.m_masks, sourcePoint.m_masks); - continue; - } + // consolidate points with similar attributes by adding masks/weights to the previous point and deleting this point. + AddMaxValueForMasks(prevPointItr->m_masks, pointItr->m_masks); - //if the points were too different, we have to add a new target point to compare against - m_targetPointList.push_back(sourcePoint); - ++targetPointIndex; - } + pointItr = sourcePointList.erase(pointItr); + } + else + { + pointItr++; } - - AZStd::swap(sourcePointList, m_targetPointList); } } SurfaceDataRegistryHandle SurfaceDataSystemComponent::RegisterSurfaceDataProviderInternal(const SurfaceDataRegistryEntry& entry) { - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::unique_lock registrationLock(m_registrationMutex); SurfaceDataRegistryHandle handle = ++m_registeredSurfaceDataProviderHandleCounter; m_registeredSurfaceDataProviders[handle] = entry; return handle; @@ -401,7 +403,7 @@ namespace SurfaceData SurfaceDataRegistryEntry SurfaceDataSystemComponent::UnregisterSurfaceDataProviderInternal(const SurfaceDataRegistryHandle& handle) { - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::unique_lock registrationLock(m_registrationMutex); SurfaceDataRegistryEntry entry; auto entryItr = m_registeredSurfaceDataProviders.find(handle); if (entryItr != m_registeredSurfaceDataProviders.end()) @@ -414,7 +416,7 @@ namespace SurfaceData bool SurfaceDataSystemComponent::UpdateSurfaceDataProviderInternal(const SurfaceDataRegistryHandle& handle, const SurfaceDataRegistryEntry& entry, AZ::Aabb& oldBounds) { - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::unique_lock registrationLock(m_registrationMutex); auto entryItr = m_registeredSurfaceDataProviders.find(handle); if (entryItr != m_registeredSurfaceDataProviders.end()) { @@ -427,7 +429,7 @@ namespace SurfaceData SurfaceDataRegistryHandle SurfaceDataSystemComponent::RegisterSurfaceDataModifierInternal(const SurfaceDataRegistryEntry& entry) { - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::unique_lock registrationLock(m_registrationMutex); SurfaceDataRegistryHandle handle = ++m_registeredSurfaceDataModifierHandleCounter; m_registeredSurfaceDataModifiers[handle] = entry; m_registeredModifierTags.insert(entry.m_tags.begin(), entry.m_tags.end()); @@ -436,7 +438,7 @@ namespace SurfaceData SurfaceDataRegistryEntry SurfaceDataSystemComponent::UnregisterSurfaceDataModifierInternal(const SurfaceDataRegistryHandle& handle) { - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::unique_lock registrationLock(m_registrationMutex); SurfaceDataRegistryEntry entry; auto entryItr = m_registeredSurfaceDataModifiers.find(handle); if (entryItr != m_registeredSurfaceDataModifiers.end()) @@ -449,7 +451,7 @@ namespace SurfaceData bool SurfaceDataSystemComponent::UpdateSurfaceDataModifierInternal(const SurfaceDataRegistryHandle& handle, const SurfaceDataRegistryEntry& entry, AZ::Aabb& oldBounds) { - AZStd::lock_guard registrationLock(m_registrationMutex); + AZStd::unique_lock registrationLock(m_registrationMutex); auto entryItr = m_registeredSurfaceDataModifiers.find(handle); if (entryItr != m_registeredSurfaceDataModifiers.end()) { diff --git a/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.h b/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.h index 8914ee7972..6ec2cab4eb 100644 --- a/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.h +++ b/Gems/SurfaceData/Code/Source/SurfaceDataSystemComponent.h @@ -10,6 +10,7 @@ #include #include +#include #include namespace SurfaceData @@ -57,7 +58,8 @@ namespace SurfaceData void RefreshSurfaceData(const AZ::Aabb& dirtyArea) override; private: - void CombineSortAndFilterNeighboringPoints(SurfacePointList& sourcePointList, bool hasDesiredTags, const SurfaceTagVector& desiredTags) const; + void FilterPoints(SurfacePointList& sourcePointList, const SurfaceTagVector& desiredTags) const; + void CombineAndSortNeighboringPoints(SurfacePointList& sourcePointList) const; SurfaceDataRegistryHandle RegisterSurfaceDataProviderInternal(const SurfaceDataRegistryEntry& entry); SurfaceDataRegistryEntry UnregisterSurfaceDataProviderInternal(const SurfaceDataRegistryHandle& handle); @@ -67,7 +69,7 @@ namespace SurfaceData SurfaceDataRegistryEntry UnregisterSurfaceDataModifierInternal(const SurfaceDataRegistryHandle& handle); bool UpdateSurfaceDataModifierInternal(const SurfaceDataRegistryHandle& handle, const SurfaceDataRegistryEntry& entry, AZ::Aabb& oldBounds); - mutable AZStd::recursive_mutex m_registrationMutex; + mutable AZStd::shared_mutex m_registrationMutex; AZStd::unordered_map m_registeredSurfaceDataProviders; AZStd::unordered_map m_registeredSurfaceDataModifiers; SurfaceDataRegistryHandle m_registeredSurfaceDataProviderHandleCounter = InvalidSurfaceDataRegistryHandle; diff --git a/Gems/SurfaceData/Code/Tests/SurfaceDataColliderComponentTest.cpp b/Gems/SurfaceData/Code/Tests/SurfaceDataColliderComponentTest.cpp index b0aa5dd38f..c51ea49f14 100644 --- a/Gems/SurfaceData/Code/Tests/SurfaceDataColliderComponentTest.cpp +++ b/Gems/SurfaceData/Code/Tests/SurfaceDataColliderComponentTest.cpp @@ -93,10 +93,28 @@ namespace UnitTest // Compare two surface points. bool SurfacePointsAreEqual(const SurfaceData::SurfacePoint& lhs, const SurfaceData::SurfacePoint& rhs) { - return (lhs.m_entityId == rhs.m_entityId) - && (lhs.m_position == rhs.m_position) - && (lhs.m_normal == rhs.m_normal) - && (lhs.m_masks == rhs.m_masks); + if ((lhs.m_entityId != rhs.m_entityId) + || (lhs.m_position != rhs.m_position) + || (lhs.m_normal != rhs.m_normal) + || (lhs.m_masks.size() != rhs.m_masks.size())) + { + return false; + } + + for (auto& mask : lhs.m_masks) + { + auto maskEntry = rhs.m_masks.find(mask.first); + if (maskEntry == rhs.m_masks.end()) + { + return false; + } + if (maskEntry->second != mask.second) + { + return false; + } + } + + return true; } // Common test function for testing the "Provider" functionality of the component. diff --git a/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp b/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp index 1d339edc71..2d6d64f65d 100644 --- a/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp +++ b/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp @@ -204,30 +204,31 @@ public: m_surfaceDataSystemEntity.reset(); } - bool ValidateRegionListSize(AZ::Aabb bounds, AZ::Vector2 stepSize, const SurfaceData::SurfacePointLists& outputLists) - { - // We expect the output list to contain width * height output entries. - // The right edge of the AABB should be treated as exclusive, so a 4x4 box with 1 step size will produce 16 entries (0, 1, 2, 3 on each dimension), - // but a 4.1 x 4.1 box with 1 step size will produce 25 entries (0, 1, 2, 3, 4 on each dimension). - return (outputLists.size() == aznumeric_cast(ceil(bounds.GetXExtent() * stepSize.GetX()) * ceil(bounds.GetYExtent() * stepSize.GetY()))); - } - void CompareSurfacePointListWithGetSurfacePoints( - SurfaceData::SurfacePointLists surfacePointLists, const SurfaceData::SurfaceTagVector& testTags) + const AZStd::vector& queryPositions, SurfaceData::SurfacePointLists surfacePointLists, + const SurfaceData::SurfaceTagVector& testTags) { - for (auto& pointList : surfacePointLists) - { - AZ::Vector3 queryPosition(pointList[0].m_position.GetX(), pointList[0].m_position.GetY(), 16.0f); - SurfaceData::SurfacePointList singleQueryPointList; + SurfaceData::SurfacePointLists singleQueryPointLists; + for (auto& queryPosition : queryPositions) + { + SurfaceData::SurfacePointList tempSingleQueryPointList; SurfaceData::SurfaceDataSystemRequestBus::Broadcast( - &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePoints, queryPosition, testTags, singleQueryPointList); + &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePoints, queryPosition, testTags, tempSingleQueryPointList); + singleQueryPointLists.push_back(tempSingleQueryPointList); + } - // Verify the two point lists are the same size, then verify that each point in each list is equal. - ASSERT_EQ(pointList.size(), singleQueryPointList.size()); - for (size_t index = 0; index < pointList.size(); index++) + // Verify the two point lists are the same size, then verify that each point in each list is equal. + ASSERT_EQ(singleQueryPointLists.size(), surfacePointLists.size()); + for (size_t listIndex = 0; listIndex < surfacePointLists.size(); listIndex++) + { + auto& surfacePointList = surfacePointLists[listIndex]; + auto& singleQueryPointList = singleQueryPointLists[listIndex]; + + ASSERT_EQ(singleQueryPointList.size(), surfacePointList.size()); + for (size_t index = 0; index < surfacePointList.size(); index++) { - SurfaceData::SurfacePoint& point1 = pointList[index]; + SurfaceData::SurfacePoint& point1 = surfacePointList[index]; SurfaceData::SurfacePoint& point2 = singleQueryPointList[index]; EXPECT_EQ(point1.m_entityId, point2.m_entityId); @@ -399,8 +400,8 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestAabbOverlaps2D) // Make sure the test produces the correct result. // Also make sure it's correct regardless of which order the boxes are passed in. - EXPECT_TRUE(SurfaceData::AabbOverlaps2D(box1, box2) == testCase.m_overlaps); - EXPECT_TRUE(SurfaceData::AabbOverlaps2D(box2, box1) == testCase.m_overlaps); + EXPECT_EQ(SurfaceData::AabbOverlaps2D(box1, box2), testCase.m_overlaps); + EXPECT_EQ(SurfaceData::AabbOverlaps2D(box2, box1), testCase.m_overlaps); } } @@ -448,9 +449,9 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestAabbContains2D) AZ::Vector3& point = testCase.m_testData[TestCase::POINT]; // Make sure the test produces the correct result. - EXPECT_TRUE(SurfaceData::AabbContains2D(box, point) == testCase.m_contains); + EXPECT_EQ(SurfaceData::AabbContains2D(box, point), testCase.m_contains); // Test the Vector2 version as well. - EXPECT_TRUE(SurfaceData::AabbContains2D(box, AZ::Vector2(point.GetX(), point.GetY())) == testCase.m_contains); + EXPECT_EQ(SurfaceData::AabbContains2D(box, AZ::Vector2(point.GetX(), point.GetY())), testCase.m_contains); } } @@ -482,19 +483,17 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestSurfacePointsFromRegion) &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromRegion, regionBounds, stepSize, testTags, availablePointsPerPosition); - EXPECT_TRUE(ValidateRegionListSize(regionBounds, stepSize, availablePointsPerPosition)); - // We expect every entry in the output list to have two surface points, at heights 0 and 4, sorted in // decreasing height order. The masks list should be the same size as the set of masks the provider owns. // We *could* check every mask as well for completeness, but that seems like overkill. for (auto& pointList : availablePointsPerPosition) { - EXPECT_TRUE(pointList.size() == 2); - EXPECT_TRUE(pointList[0].m_position.GetZ() == 4.0f); - EXPECT_TRUE(pointList[1].m_position.GetZ() == 0.0f); + EXPECT_EQ(pointList.size(), 2); + EXPECT_EQ(pointList[0].m_position.GetZ(), 4.0f); + EXPECT_EQ(pointList[1].m_position.GetZ(), 0.0f); for (auto& point : pointList) { - EXPECT_TRUE(point.m_masks.size() == providerTags.size()); + EXPECT_EQ(point.m_masks.size(), providerTags.size()); } } } @@ -520,13 +519,11 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestSurfacePointsFromRegion_NoMatchingMas &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromRegion, regionBounds, stepSize, testTags, availablePointsPerPosition); - EXPECT_TRUE(ValidateRegionListSize(regionBounds, stepSize, availablePointsPerPosition)); - // We expect every entry in the output list to have no surface points, since the requested mask doesn't match // any of the masks from our mock surface provider. for (auto& queryPosition : availablePointsPerPosition) { - EXPECT_TRUE(queryPosition.size() == 0); + EXPECT_TRUE(queryPosition.empty()); } } @@ -550,13 +547,11 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestSurfacePointsFromRegion_NoMatchingReg &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromRegion, regionBounds, stepSize, testTags, availablePointsPerPosition); - EXPECT_TRUE(ValidateRegionListSize(regionBounds, stepSize, availablePointsPerPosition)); - // We expect every entry in the output list to have no surface points, since the input points don't overlap with // our surface provider. for (auto& pointList : availablePointsPerPosition) { - EXPECT_TRUE(pointList.size() == 0); + EXPECT_TRUE(pointList.empty()); } } @@ -602,16 +597,17 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestSurfacePointsFromRegion_ProviderModif &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromRegion, regionBounds, stepSize, testTags, availablePointsPerPosition); - EXPECT_TRUE(ValidateRegionListSize(regionBounds, stepSize, availablePointsPerPosition)); - // We expect every entry in the output list to have two surface points (with heights 0 and 4), // and each point should have both the "test_surface1" and "test_surface2" tag. for (auto& pointList : availablePointsPerPosition) { - EXPECT_TRUE(pointList.size() == 2); + EXPECT_EQ(pointList.size(), 2); + float expectedZ = 4.0f; for (auto& point : pointList) { - EXPECT_TRUE(point.m_masks.size() == 2); + EXPECT_EQ(point.m_position.GetZ(), expectedZ); + EXPECT_EQ(point.m_masks.size(), 2); + expectedZ = (expectedZ == 4.0f) ? 0.0f : 4.0f; } } } @@ -624,7 +620,7 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestSurfacePointsFromRegion_SimilarPoints // sets of tags. // Create two mock Surface Providers that covers from (0, 0) - (8, 8) in space, with points spaced 0.25 apart. - // The first has heights 0 and 4, with the tag "surfaceTag1". The second has heights 0.005 and 4.005, with the tag "surfaceTag2". + // The first has heights 0 and 4, with the tag "surfaceTag1". The second has heights 0.0005 and 4.0005, with the tag "surfaceTag2". SurfaceData::SurfaceTagVector provider1Tags = { SurfaceData::SurfaceTag(m_testSurface1Crc) }; MockSurfaceProvider mockProvider1(MockSurfaceProvider::ProviderType::SURFACE_PROVIDER, provider1Tags, AZ::Vector3(0.0f), AZ::Vector3(8.0f), AZ::Vector3(0.25f, 0.25f, 4.0f), @@ -648,16 +644,17 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestSurfacePointsFromRegion_SimilarPoints &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromRegion, regionBounds, stepSize, testTags, availablePointsPerPosition); - EXPECT_TRUE(ValidateRegionListSize(regionBounds, stepSize, availablePointsPerPosition)); - // We expect every entry in the output list to have two surface points, not four. The two points // should have both surface tags on them. for (auto& pointList : availablePointsPerPosition) { - EXPECT_TRUE(pointList.size() == 2); + EXPECT_EQ(pointList.size(), 2); + float expectedZ = 4.0005f; for (auto& point : pointList) { - EXPECT_TRUE(point.m_masks.size() == 2); + EXPECT_EQ(point.m_position.GetZ(), expectedZ); + EXPECT_EQ(point.m_masks.size(), 2); + expectedZ = (expectedZ == 4.0005f) ? 0.0005f : 4.0005f; } } } @@ -692,16 +689,14 @@ TEST_F(SurfaceDataTestApp, SurfaceData_TestSurfacePointsFromRegion_DissimilarPoi &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromRegion, regionBounds, stepSize, testTags, availablePointsPerPosition); - EXPECT_TRUE(ValidateRegionListSize(regionBounds, stepSize, availablePointsPerPosition)); - // We expect every entry in the output list to have four surface points with one tag each, // because the points are far enough apart that they won't merge. for (auto& pointList : availablePointsPerPosition) { - EXPECT_TRUE(pointList.size() == 4); + EXPECT_EQ(pointList.size(), 4); for (auto& point : pointList) { - EXPECT_TRUE(point.m_masks.size() == 1); + EXPECT_EQ(point.m_masks.size(), 1); } } } @@ -727,10 +722,17 @@ TEST_F(SurfaceDataTestApp, SurfaceData_VerifyGetSurfacePointsFromRegionAndGetSur &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromRegion, regionBounds, stepSize, providerTags, availablePointsPerPosition); - EXPECT_TRUE(ValidateRegionListSize(regionBounds, stepSize, availablePointsPerPosition)); - // For each point entry returned from GetSurfacePointsFromRegion, call GetSurfacePoints and verify the results match. - CompareSurfacePointListWithGetSurfacePoints(availablePointsPerPosition, providerTags); + AZStd::vector queryPositions; + for (float y = 0.0f; y < 4.0f; y += 1.0f) + { + for (float x = 0.0f; x < 4.0f; x += 1.0f) + { + queryPositions.push_back(AZ::Vector3(x, y, 16.0f)); + } + } + + CompareSurfacePointListWithGetSurfacePoints(queryPositions, availablePointsPerPosition, providerTags); } TEST_F(SurfaceDataTestApp, SurfaceData_VerifyGetSurfacePointsFromListAndGetSurfacePointsMatch) @@ -763,7 +765,7 @@ TEST_F(SurfaceDataTestApp, SurfaceData_VerifyGetSurfacePointsFromListAndGetSurfa EXPECT_EQ(availablePointsPerPosition.size(), 16); // For each point entry returned from GetSurfacePointsFromList, call GetSurfacePoints and verify the results match. - CompareSurfacePointListWithGetSurfacePoints(availablePointsPerPosition, providerTags); + CompareSurfacePointListWithGetSurfacePoints(queryPositions, availablePointsPerPosition, providerTags); } // This uses custom test / benchmark hooks so that we can load LmbrCentral and use Shape components in our unit tests and benchmarks. AZ_UNIT_TEST_HOOK(new UnitTest::SurfaceDataTestEnvironment, UnitTest::SurfaceDataBenchmarkEnvironment); diff --git a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp index 03011db2f3..9a9cb24e4c 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp @@ -177,7 +177,7 @@ namespace Terrain const AZ::Crc32 terrainTag = isHole ? Constants::s_terrainHoleTagCrc : Constants::s_terrainTagCrc; point.m_masks[terrainTag] = 1.0f; - surfacePointList.push_back(point); + surfacePointList.push_back(AZStd::move(point)); } AZ::Aabb TerrainSurfaceDataSystemComponent::GetSurfaceAabb() const