From d00436e26a19085ec3ff2a4b73b67f9536fb609c Mon Sep 17 00:00:00 2001 From: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> Date: Thu, 10 Feb 2022 10:57:16 -0800 Subject: [PATCH 01/12] Improved the way the Settings Registry can handle stacks/arrays. The ConfigurableStack makes configuring stacks and arrays through the Settings Registry easier than using direct serialization to a container like AZStd::vector. It does this by using JSON Objects rather than JSON arrays, although arrays are supported for backwards compatibility. Two key words were added: - $stack_before : Insert the new entry before the referenced entry. Referencing is done by name. - $stack_after : Insert the new entry after the referenced entry. Referencing is done by name. to allow inserting new entries at specific locations. An example of a .setreg file at updates existing settings would be: // Original settings { "Settings in a stack": { "AnOriginalEntry": { "MyValue": "hello", "ExampleValue": 84 }, "TheSecondEntry": { "MyValue": "world" } } } // Customized settings. { "Settings in a stack": { // Add a new entry before "AnOriginalEntry" in the original document. "NewEntry": { "$stack_before": "AnOriginalEntry", "MyValue": 42 }, // Add a second entry after "AnOriginalEntry" in the original document. "SecondNewEntry": { "$stack_after": "AnOriginalEntry", "MyValue": "FortyTwo". }, // Update a value in "AnOriginalEntry". "AnOriginalEntry": { "ExampleValue": 42 }, // Delete the "TheSecondEntry" from the settings. "TheSecondEntry" : null, } } The ConfigurableStack uses an AZStd::shared_ptr to store the values. This supports settings up a base class and specifying derived classes in the settings, but requires that the base and derived classes all have a memory allocator associated with them (i.e. by using the "AZ_CLASS_ALLOCATOR" macro) and that the relation of the classes is reflected. Loading a ConfigurableStack can be done using the GetObject call on the SettingsRegistryInterface. Signed-off-by: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> --- .../Json/JsonSystemComponent.cpp | 3 + .../Serialization/Json/RegistrationContext.h | 4 +- .../AzCore/Settings/ConfigurableStack.cpp | 174 +++++++++++ .../AzCore/Settings/ConfigurableStack.h | 189 ++++++++++++ .../AzCore/Settings/ConfigurableStack.inl | 161 ++++++++++ .../AzCore/AzCore/azcore_files.cmake | 3 + .../Tests/Settings/ConfigurableStackTests.cpp | 283 ++++++++++++++++++ .../AzCore/Tests/azcoretests_files.cmake | 1 + 8 files changed, 817 insertions(+), 1 deletion(-) create mode 100644 Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp create mode 100644 Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h create mode 100644 Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl create mode 100644 Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp diff --git a/Code/Framework/AzCore/AzCore/Serialization/Json/JsonSystemComponent.cpp b/Code/Framework/AzCore/AzCore/Serialization/Json/JsonSystemComponent.cpp index 0ba44e4e61..c08f038c8b 100644 --- a/Code/Framework/AzCore/AzCore/Serialization/Json/JsonSystemComponent.cpp +++ b/Code/Framework/AzCore/AzCore/Serialization/Json/JsonSystemComponent.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -50,6 +51,8 @@ namespace AZ void JsonSystemComponent::Reflect(ReflectContext* reflectContext) { + JsonConfigurableStackSerializer::Reflect(reflectContext); + if (JsonRegistrationContext* jsonContext = azrtti_cast(reflectContext)) { jsonContext->Serializer()->HandlesType(); diff --git a/Code/Framework/AzCore/AzCore/Serialization/Json/RegistrationContext.h b/Code/Framework/AzCore/AzCore/Serialization/Json/RegistrationContext.h index 25a3bfedba..f62dd54e71 100644 --- a/Code/Framework/AzCore/AzCore/Serialization/Json/RegistrationContext.h +++ b/Code/Framework/AzCore/AzCore/Serialization/Json/RegistrationContext.h @@ -8,11 +8,12 @@ #pragma once +#include +#include #include #include #include #include -#include #include namespace AZ @@ -22,6 +23,7 @@ namespace AZ { public: AZ_RTTI(JsonRegistrationContext, "{5A763774-CA8B-4245-A897-A03C503DCD60}", ReflectContext); + AZ_CLASS_ALLOCATOR(JsonRegistrationContext, SystemAllocator, 0); class SerializerBuilder; using SerializerMap = AZStd::unordered_map, AZStd::hash>; diff --git a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp new file mode 100644 index 0000000000..75748e0f11 --- /dev/null +++ b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp @@ -0,0 +1,174 @@ +/* + * 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 + * + */ + +#pragma once + +#include +#include +#include +#include +#include + +namespace AZ +{ + AZ_CLASS_ALLOCATOR_IMPL(JsonConfigurableStackSerializer, AZ::SystemAllocator, 0); + + JsonSerializationResult::Result JsonConfigurableStackSerializer::Load( + void* outputValue, + [[maybe_unused]] const Uuid& outputValueTypeId, + const rapidjson::Value& inputValue, + JsonDeserializerContext& context) + { + namespace JSR = JsonSerializationResult; // Used remove name conflicts in AzCore in uber builds. + + switch (inputValue.GetType()) + { + case rapidjson::kArrayType: + return LoadFromArray(outputValue, inputValue, context); + case rapidjson::kObjectType: + return LoadFromObject(outputValue, inputValue, context); + + case rapidjson::kNullType: + [[fallthrough]]; + case rapidjson::kFalseType: + [[fallthrough]]; + case rapidjson::kTrueType: + [[fallthrough]]; + case rapidjson::kStringType: + [[fallthrough]]; + case rapidjson::kNumberType: + return context.Report( + JSR::Tasks::ReadField, JSR::Outcomes::Unsupported, + "Unsupported type. Configurable stack values can only be read from arrays or objects."); + default: + return context.Report(JSR::Tasks::ReadField, JSR::Outcomes::Unknown, "Unknown json type encountered for string value."); + } + } + + JsonSerializationResult::Result JsonConfigurableStackSerializer::Store( + [[maybe_unused]] rapidjson::Value& outputValue, + [[maybe_unused]] const void* inputValue, + [[maybe_unused]] const void* defaultValue, + [[maybe_unused]] const Uuid& valueTypeId, + JsonSerializerContext& context) + { + return context.Report( + JsonSerializationResult::Tasks::WriteValue, JsonSerializationResult::Outcomes::Unsupported, + "Configuration stacks can not be written out."); + } + + void JsonConfigurableStackSerializer::Reflect(ReflectContext* context) + { + if (JsonRegistrationContext* jsonContext = azrtti_cast(context)) + { + jsonContext->Serializer()->HandlesType(); + } + } + + JsonSerializationResult::Result JsonConfigurableStackSerializer::LoadFromArray( + void* outputValue, const rapidjson::Value& inputValue, JsonDeserializerContext& context) + { + namespace JSR = JsonSerializationResult; // Used remove name conflicts in AzCore in uber builds. + + auto stack = reinterpret_cast(outputValue); + const Uuid& nodeValueType = stack->GetNodeType(); + + JSR::ResultCode result(JSR::Tasks::ReadField); + uint32_t counter = 0; + for (auto& it : inputValue.GetArray()) + { + ScopedContextPath subPath(context, counter); + void* value = stack->AddNode(AZStd::to_string(counter)); + result.Combine(ContinueLoading(value, nodeValueType, it, context)); + counter++; + } + + return context.Report( + result, + result.GetProcessing() != JSR::Processing::Halted ? "Loaded configurable stack from array." + : "Failed to load configurable stack from array."); + } + + JsonSerializationResult::Result JsonConfigurableStackSerializer::LoadFromObject( + void* outputValue, const rapidjson::Value& inputValue, JsonDeserializerContext& context) + { + namespace JSR = JsonSerializationResult; // Used remove name conflicts in AzCore in uber builds. + + auto stack = reinterpret_cast(outputValue); + const Uuid& nodeValueType = stack->GetNodeType(); + + AZStd::queue> + delayedEntries; + JSR::ResultCode result(JSR::Tasks::ReadField); + + auto add = [&](ConfigurableStackInterface::InsertPosition position, rapidjson::Value::ConstMemberIterator target, + rapidjson::Value::ConstMemberIterator it) + { + if (target->value.IsString()) + { + delayedEntries.emplace(position, AZStd::string_view(target->value.GetString(), target->value.GetStringLength()), it); + } + else + { + result.Combine(context.Report( + JSR::Tasks::ReadField, JSR::Outcomes::Skipped, + "Skipped value for the Configurable Stack because the target wasn't a string.")); + } + }; + + // Load all the regular entries into the stack and store any with a before or after binding for + // later inserting. + for (auto it = inputValue.MemberBegin(); it != inputValue.MemberEnd(); ++it) + { + AZStd::string_view name(it->name.GetString(), it->name.GetStringLength()); + ScopedContextPath subPath(context, name); + if (it->value.IsObject()) + { + if (auto target = it->value.FindMember(StackBefore); target != it->value.MemberEnd()) + { + add(ConfigurableStackInterface::InsertPosition::Before, target, it); + continue; + } + if (auto target = it->value.FindMember(StackAfter); target != it->value.MemberEnd()) + { + add(ConfigurableStackInterface::InsertPosition::After, target, it); + continue; + } + } + + void* value = stack->AddNode(name); + result.Combine(ContinueLoading(value, nodeValueType, it->value, context)); + } + + // Insert the entries that have been delayed. + while (!delayedEntries.empty()) + { + auto&& [insertPosition, target, valueStore] = delayedEntries.front(); + AZStd::string_view name(valueStore->name.GetString(), valueStore->name.GetStringLength()); + ScopedContextPath subPath(context, name); + void* value = stack->AddNode(name, target, insertPosition); + if (value != nullptr) + { + result.Combine(ContinueLoading(value, nodeValueType, valueStore->value, context)); + } + else + { + result.Combine(context.Report( + JSR::Tasks::ReadField, JSR::Outcomes::Skipped, + AZStd::string::format( + "Skipped value for the Configurable Stack because the target '%.*s' couldn't be found.", AZ_STRING_ARG(name)))); + } + delayedEntries.pop(); + } + + return context.Report( + result, + result.GetProcessing() != JSR::Processing::Halted ? "Loaded configurable stack from array." + : "Failed to load configurable stack from array."); + } +} // namespace AZ diff --git a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h new file mode 100644 index 0000000000..bafb6f47a3 --- /dev/null +++ b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h @@ -0,0 +1,189 @@ +/* + * 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 + * + */ + +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +namespace AZ +{ + //! The ConfigurableStack makes configuring stacks and arrays through the Settings Registry easier than using direct serialization + //! to a container like AZStd::vector. It does this by using JSON Objects rather than JSON arrays, although arrays are supported + //! for backwards compatibility. Two key words were added: + //! - $stack_before : Insert the new entry before the referenced entry. Referencing is done by name. + //! - $stack_after : Insert the new entry after the referenced entry. Referencing is done by name. + //! to allow inserting new entries at specific locations. An example of a .setreg file at updates existing settings would be: + //! // Original settings + //! { + //! "Settings in a stack": + //! { + //! "AnOriginalEntry": + //! { + //! "MyValue": "hello", + //! "ExampleValue": 84 + //! }, + //! "TheSecondEntry": + //! { + //! "MyValue": "world" + //! } + //! } + //! } + //! + //! // Customized settings. + //! { + //! "Settings in a stack": + //! { + //! // Add a new entry before "AnOriginalEntry" in the original document. + //! "NewEntry": + //! { + //! "$stack_before": "AnOriginalEntry", + //! "MyValue": 42 + //! }, + //! // Add a second entry after "AnOriginalEntry" in the original document. + //! "SecondNewEntry": + //! { + //! "$stack_after": "AnOriginalEntry", + //! "MyValue": "FortyTwo". + //! }, + //! // Update a value in "AnOriginalEntry". + //! "AnOriginalEntry": + //! { + //! "ExampleValue": 42 + //! }, + //! // Delete the "TheSecondEntry" from the settings. + //! "TheSecondEntry" : null, + //! } + //! } + //! + //! The ConfigurableStack uses an AZStd::shared_ptr to store the values. This supports settings up a base class and specifying + //! derived classes in the settings, but requires that the base and derived classes all have a memory allocator associated with + //! them (i.e. by using the "AZ_CLASS_ALLOCATOR" macro) and that the relation of the classes is reflected. Loading a + //! ConfigurableStack can be done using the GetObject call on the SettingsRegistryInterface. + + class ReflectContext; + + class ConfigurableStackInterface + { + public: + friend class JsonConfigurableStackSerializer; + + virtual ~ConfigurableStackInterface() = default; + + virtual const TypeId& GetNodeType() const = 0; + + protected: + enum class InsertPosition + { + Before, + After + }; + virtual void* AddNode(AZStd::string name) = 0; + virtual void* AddNode(AZStd::string name, AZStd::string_view target, InsertPosition position) = 0; + }; + + template + class ConfigurableStack final : public ConfigurableStackInterface + { + public: + using NodeValue = AZStd::shared_ptr; + using Node = AZStd::pair; + using NodeContainer = AZStd::vector; + using Iterator = typename NodeContainer::iterator; + using ConstIterator = typename NodeContainer::const_iterator; + + ~ConfigurableStack() override = default; + + const TypeId& GetNodeType() const override; + + Iterator begin(); + Iterator end(); + ConstIterator begin() const; + ConstIterator end() const; + ConstIterator cbegin() const; + ConstIterator cend() const; + + size_t size() const; + + protected: + void* AddNode(AZStd::string name) override; + void* AddNode(AZStd::string name, AZStd::string_view target, InsertPosition position) override; + + private: + NodeContainer m_nodes; + }; + + AZ_TYPE_INFO_TEMPLATE(ConfigurableStack, "{0A3D2038-6E6A-4EFD-A1B4-F30D947E21B1}", AZ_TYPE_INFO_TYPENAME); + + template + struct SerializeGenericTypeInfo> + { + using ConfigurableStackType = ConfigurableStack; + + class GenericConfigurableStackInfo : public GenericClassInfo + { + public: + AZ_TYPE_INFO(GenericConfigurableStackInfo, "{FC5A9353-D0DE-48F6-81B5-1CB2985C5F65}"); + + GenericConfigurableStackInfo(); + + SerializeContext::ClassData* GetClassData() override; + size_t GetNumTemplatedArguments() override; + const Uuid& GetTemplatedTypeId([[maybe_unused]] size_t element) override; + const Uuid& GetSpecializedTypeId() const override; + const Uuid& GetGenericTypeId() const override; + + void Reflect(SerializeContext* serializeContext) override; + + SerializeContext::ClassData m_classData; + }; + + using ClassInfoType = GenericConfigurableStackInfo; + + static ClassInfoType* GetGenericInfo(); + static const Uuid& GetClassTypeId(); + }; + + class JsonConfigurableStackSerializer : public BaseJsonSerializer + { + public: + AZ_RTTI(JsonConfigurableStackSerializer, "{45A31805-9058-41A9-B1A3-71E2CB4D9237}", BaseJsonSerializer); + AZ_CLASS_ALLOCATOR_DECL; + + JsonSerializationResult::Result Load( + void* outputValue, + const Uuid& outputValueTypeId, + const rapidjson::Value& inputValue, + JsonDeserializerContext& context) override; + + JsonSerializationResult::Result Store( + rapidjson::Value& outputValue, + const void* inputValue, + const void* defaultValue, + const Uuid& valueTypeId, + JsonSerializerContext& context) override; + + static void Reflect(ReflectContext* context); + + private: + JsonSerializationResult::Result LoadFromArray( + void* outputValue, const rapidjson::Value& inputValue, JsonDeserializerContext& context); + JsonSerializationResult::Result LoadFromObject( + void* outputValue, const rapidjson::Value& inputValue, JsonDeserializerContext& context); + + static constexpr const char* StackBefore = "$stack_before"; + static constexpr const char* StackAfter = "$stack_after"; + }; +} // namespace AZ + +#include diff --git a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl new file mode 100644 index 0000000000..ba34ac1d03 --- /dev/null +++ b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl @@ -0,0 +1,161 @@ +/* + * 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 + * + */ + +#pragma once + +namespace AZ +{ + // + // ConfigurableStack + // + + template + const TypeId& ConfigurableStack::GetNodeType() const + { + return azrtti_typeid(); + } + + template + auto ConfigurableStack::begin() -> Iterator + { + return m_nodes.begin(); + } + + template + auto ConfigurableStack::end() -> Iterator + { + return m_nodes.end(); + } + + template + auto ConfigurableStack::begin() const -> ConstIterator + { + return m_nodes.begin(); + } + + template + auto ConfigurableStack::end() const -> ConstIterator + { + return m_nodes.end(); + } + + template + auto ConfigurableStack::cbegin() const -> ConstIterator + { + return m_nodes.cbegin(); + } + + template + auto ConfigurableStack::cend() const -> ConstIterator + { + return m_nodes.cend(); + } + + template + size_t ConfigurableStack::size() const + { + return m_nodes.size(); + } + + template + void* ConfigurableStack::AddNode(AZStd::string name) + { + Node& result = m_nodes.emplace_back(AZStd::move(name)); + return &result.second; + } + + template + void* ConfigurableStack::AddNode(AZStd::string name, AZStd::string_view target, InsertPosition position) + { + auto end = m_nodes.end(); + for (auto it = m_nodes.begin(); it != end; ++it) + { + if (it->first == target) + { + if (position == InsertPosition::After) + { + ++it; + } + auto result = m_nodes.insert(it, Node(AZStd::move(name), {})); + return &result->second; + } + } + return nullptr; + } + + + + // + // SerializeGenericTypeInfo + // + + + template + SerializeGenericTypeInfo>::GenericConfigurableStackInfo::GenericConfigurableStackInfo() + : m_classData{ SerializeContext::ClassData::Create( + "AZ::ConfigurableStack", GetSpecializedTypeId(), Internal::NullFactory::GetInstance(), nullptr, nullptr) } + { + } + + template + SerializeContext::ClassData* SerializeGenericTypeInfo>::GenericConfigurableStackInfo::GetClassData() + { + return &m_classData; + } + + template + size_t SerializeGenericTypeInfo>::GenericConfigurableStackInfo::GetNumTemplatedArguments() + { + return 1; + } + + template + const Uuid& SerializeGenericTypeInfo>::GenericConfigurableStackInfo::GetTemplatedTypeId( + [[maybe_unused]] size_t element) + { + return SerializeGenericTypeInfo::GetClassTypeId(); + } + + template + const Uuid& SerializeGenericTypeInfo>::GenericConfigurableStackInfo::GetSpecializedTypeId() const + { + return azrtti_typeid(); + } + + template + const Uuid& SerializeGenericTypeInfo>::GenericConfigurableStackInfo::GetGenericTypeId() const + { + return TYPEINFO_Uuid(); + } + + template + void SerializeGenericTypeInfo>::GenericConfigurableStackInfo::Reflect(SerializeContext* serializeContext) + { + if (serializeContext) + { + serializeContext->RegisterGenericClassInfo(GetSpecializedTypeId(), this, &AnyTypeInfoConcept::CreateAny); + if (serializeContext->FindClassData(azrtti_typeid>()) == nullptr) + { + serializeContext->RegisterGenericType>(); + } + } + } + + template + auto SerializeGenericTypeInfo>::GetGenericInfo() -> ClassInfoType* + { + return GetCurrentSerializeContextModule().CreateGenericClassInfo(); + } + + template + const Uuid& SerializeGenericTypeInfo>::GetClassTypeId() + { + return GetGenericInfo()->GetClassData()->m_typeId; + } +} // namespace AZ + diff --git a/Code/Framework/AzCore/AzCore/azcore_files.cmake b/Code/Framework/AzCore/AzCore/azcore_files.cmake index b7821f1d65..dc2481b4c5 100644 --- a/Code/Framework/AzCore/AzCore/azcore_files.cmake +++ b/Code/Framework/AzCore/AzCore/azcore_files.cmake @@ -560,6 +560,9 @@ set(FILES Serialization/std/VariantReflection.inl Settings/CommandLine.cpp Settings/CommandLine.h + Settings/ConfigurableStack.cpp + Settings/ConfigurableStack.inl + Settings/ConfigurableStack.h Settings/SettingsRegistry.cpp Settings/SettingsRegistry.h Settings/SettingsRegistryConsoleUtils.cpp diff --git a/Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp b/Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp new file mode 100644 index 0000000000..df535976bf --- /dev/null +++ b/Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp @@ -0,0 +1,283 @@ +/* + * 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 +#include +#include + +namespace UnitTest +{ + struct ConfigInt + { + AZ_TYPE_INFO(UnitTest::ConfigInt, "{1FAF6E55-7FA4-4FFA-8C41-34F422B8E8AB}"); + AZ_CLASS_ALLOCATOR(ConfigInt, AZ::SystemAllocator, 0); + + int m_value; + + static void Reflect(AZ::ReflectContext* context) + { + if (auto sc = azrtti_cast(context)) + { + sc->Class()->Field("Value", &ConfigInt::m_value); + } + } + }; + + struct ConfigurableStackTests : public AllocatorsFixture + { + void Reflect(AZ::ReflectContext* context) + { + if (auto sc = azrtti_cast(context)) + { + AZ::JsonSystemComponent::Reflect(sc); + ConfigInt::Reflect(sc); + sc->RegisterGenericType>(); + } + else if (auto jrc = azrtti_cast(context)) + { + AZ::JsonSystemComponent::Reflect(jrc); + } + } + + void SetUp() override + { + AllocatorsFixture::SetUp(); + + m_serializeContext = aznew AZ::SerializeContext(); + m_jsonRegistrationContext = aznew AZ::JsonRegistrationContext(); + + Reflect(m_serializeContext); + Reflect(m_jsonRegistrationContext); + + m_deserializationSettings.m_registrationContext = m_jsonRegistrationContext; + m_deserializationSettings.m_serializeContext = m_serializeContext; + } + + void TearDown() + { + m_jsonRegistrationContext->EnableRemoveReflection(); + Reflect(m_jsonRegistrationContext); + m_jsonRegistrationContext->DisableRemoveReflection(); + + m_serializeContext->EnableRemoveReflection(); + Reflect(m_serializeContext); + m_serializeContext->DisableRemoveReflection(); + + delete m_jsonRegistrationContext; + delete m_serializeContext; + + AllocatorsFixture::TearDown(); + } + + void ObjectTest(AZStd::string_view jsonText) + { + AZ::ConfigurableStack stack; + + rapidjson::Document document; + document.Parse(jsonText.data(), jsonText.length()); + ASSERT_FALSE(document.HasParseError()); + AZ::JsonSerializationResult::ResultCode result = AZ::JsonSerialization::Load(stack, document, m_deserializationSettings); + ASSERT_EQ(AZ::JsonSerializationResult::Processing::Completed, result.GetProcessing()); + ASSERT_EQ(4, stack.size()); + + int numberCounter = 0; + int valueCounter = 42; + for (auto& [name, value] : stack) + { + EXPECT_STREQ(AZStd::string::format("Value%i", numberCounter).c_str(), name.c_str()); + EXPECT_EQ(valueCounter, value->m_value); + numberCounter++; + valueCounter++; + } + } + + AZ::SerializeContext* m_serializeContext; + AZ::JsonRegistrationContext* m_jsonRegistrationContext; + AZ::JsonDeserializerSettings m_deserializationSettings; + }; + + TEST_F(ConfigurableStackTests, DeserializeArray) + { + AZ::ConfigurableStack stack; + + rapidjson::Document document; + document.Parse( + R"([ + { "Value": 42 }, + { "Value": 43 }, + { "Value": 44 }, + { "Value": 45 } + ])"); + ASSERT_FALSE(document.HasParseError()); + AZ::JsonSerializationResult::ResultCode result = AZ::JsonSerialization::Load(stack, document, m_deserializationSettings); + ASSERT_EQ(AZ::JsonSerializationResult::Processing::Completed, result.GetProcessing()); + ASSERT_EQ(4, stack.size()); + + int numberCounter = 0; + int valueCounter = 42; + for (auto& [name, value] : stack) + { + EXPECT_STREQ(AZStd::to_string(numberCounter).c_str(), name.c_str()); + EXPECT_EQ(valueCounter, value->m_value); + numberCounter++; + valueCounter++; + } + } + + TEST_F(ConfigurableStackTests, DeserializeObject) + { + ObjectTest( + R"({ + "Value0": { "Value": 42 }, + "Value1": { "Value": 43 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 } + })"); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithLateBefore) + { + ObjectTest( + R"({ + "Value0": { "Value": 42 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 }, + "Value1": + { + "$stack_before": "Value2", + "Value": 43 + } + })"); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithEarlyBefore) + { + ObjectTest( + R"({ + "Value1": + { + "$stack_before": "Value2", + "Value": 43 + }, + "Value0": { "Value": 42 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 } + })"); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithLateAfter) + { + ObjectTest( + R"({ + "Value0": { "Value": 42 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 }, + "Value1": + { + "$stack_after": "Value0", + "Value": 43 + } + })"); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithEarlyAfter) + { + ObjectTest( + R"({ + "Value1": + { + "$stack_after": "Value0", + "Value": 43 + }, + "Value0": { "Value": 42 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 } + })"); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithBeforeFirst) + { + ObjectTest( + R"({ + "Value1": { "Value": 43 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 }, + "Value0": + { + "$stack_before": "Value1", + "Value": 42 + } + + })"); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithInsertAfterLast) + { + ObjectTest( + R"({ + "Value3": + { + "$stack_after": "Value2", + "Value": 45 + }, + "Value0": { "Value": 42 }, + "Value1": { "Value": 43 }, + "Value2": { "Value": 44 } + })"); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithInvalidTarget) + { + AZ::ConfigurableStack stack; + + rapidjson::Document document; + document.Parse( + R"({ + "Value1": + { + "$stack_after": "airplane", + "Value": 43 + }, + "Value0": { "Value": 42 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 } + })"); + ASSERT_FALSE(document.HasParseError()); + AZ::JsonSerializationResult::ResultCode result = AZ::JsonSerialization::Load(stack, document, m_deserializationSettings); + EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, result.GetProcessing()); + EXPECT_EQ(AZ::JsonSerializationResult::Outcomes::PartialSkip, result.GetOutcome()); + EXPECT_EQ(3, stack.size()); + } + + TEST_F(ConfigurableStackTests, DeserializeObjectWithInvalidTargetType) + { + AZ::ConfigurableStack stack; + + rapidjson::Document document; + document.Parse( + R"({ + "Value1": + { + "$stack_after": 42, + "Value": 43 + }, + "Value0": { "Value": 42 }, + "Value2": { "Value": 44 }, + "Value3": { "Value": 45 } + })"); + ASSERT_FALSE(document.HasParseError()); + AZ::JsonSerializationResult::ResultCode result = AZ::JsonSerialization::Load(stack, document, m_deserializationSettings); + EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, result.GetProcessing()); + EXPECT_EQ(AZ::JsonSerializationResult::Outcomes::PartialSkip, result.GetOutcome()); + EXPECT_EQ(3, stack.size()); + } +} // namespace UnitTest diff --git a/Code/Framework/AzCore/Tests/azcoretests_files.cmake b/Code/Framework/AzCore/Tests/azcoretests_files.cmake index b07cd32ec1..1f67bd6db8 100644 --- a/Code/Framework/AzCore/Tests/azcoretests_files.cmake +++ b/Code/Framework/AzCore/Tests/azcoretests_files.cmake @@ -76,6 +76,7 @@ set(FILES Name/NameTests.cpp RTTI/TypeSafeIntegralTests.cpp Settings/CommandLineTests.cpp + Settings/ConfigurableStackTests.cpp Settings/SettingsRegistryTests.cpp Settings/SettingsRegistryConsoleUtilsTests.cpp Settings/SettingsRegistryMergeUtilsTests.cpp From b340877f7c69eab1e9ff55a839b325d5bd6bd7f7 Mon Sep 17 00:00:00 2001 From: Scott Murray Date: Fri, 11 Feb 2022 13:20:05 -0800 Subject: [PATCH 02/12] updating tests for screenshot using editor_test.py Signed-off-by: Scott Murray --- .../PythonTests/Atom/TestSuite_Main_GPU.py | 204 ++++++++++++------ .../Atom/atom_utils/atom_component_helper.py | 14 +- .../tests/hydra_AtomGPU_BasicLevelSetup.py | 22 +- 3 files changed, 157 insertions(+), 83 deletions(-) diff --git a/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py b/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py index 14d850245c..8aec70e3fd 100644 --- a/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py +++ b/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py @@ -12,7 +12,7 @@ import pytest import editor_python_test_tools.hydra_test_utils as hydra import ly_test_tools.environment.file_system as file_system from ly_test_tools.benchmark.data_aggregator import BenchmarkDataAggregator -from ly_test_tools.o3de.editor_test import EditorSharedTest, EditorTestSuite +from ly_test_tools.o3de.editor_test import EditorSingleTest, EditorTestSuite from Atom.atom_utils.atom_component_helper import compare_screenshot_to_golden_image, golden_images_directory DEFAULT_SUBFOLDER_PATH = 'user/PythonTests/Automated/Screenshots' @@ -23,89 +23,165 @@ logger = logging.getLogger(__name__) @pytest.mark.parametrize("launcher_platform", ['windows_editor']) class TestAutomation(EditorTestSuite): # Remove -autotest_mode from global_extra_cmdline_args since we need rendering for these tests. - global_extra_cmdline_args = ["-BatchMode"] # Default is ["-BatchMode", "-autotest_mode"] - + global_extra_cmdline_args = ["-autotest_mode"] # Default is ["-BatchMode", "-autotest_mode"] + use_null_renderer = False # Default is True enable_prefab_system = False - @pytest.mark.test_case_id("C34603773") - class AtomGPU_BasicLevelSetup_SetsUpLevel(EditorSharedTest): - use_null_renderer = False # Default is True - screenshot_name = "AtomBasicLevelSetup.ppm" - test_screenshots = [] # Gets set by setup() - screenshot_directory = "" # Gets set by setup() - - # Clear existing test screenshots before starting test. - def setup(self, workspace): - screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) - test_screenshots = [os.path.join(screenshot_directory, self.screenshot_name)] - file_system.delete(test_screenshots, True, True) + @staticmethod + def screenshot_setup(screenshot_directory, screenshot_names): + """ + :param screenshot_names: list of screenshot file names with extensions + :return: tuple test_screenshots, golden_images each a list of full file paths + """ + test_screenshots = [] + golden_images = [] + for screenshot in screenshot_names: + screenshot_path = os.path.join(screenshot_directory, screenshot) + test_screenshots.append(screenshot_path) + file_system.delete(test_screenshots, True, True) + for golden_image in screenshot_names: + golden_image_path = os.path.join(golden_images_directory(), golden_image) + golden_images.append(golden_image_path) + return test_screenshots, golden_images - golden_images = [os.path.join(golden_images_directory(), screenshot_name)] + @pytest.mark.test_case_id("C34603773") + @pytest.mark.REQUIRES_gpu + class AtomGPU_BasicLevelSetup_SetsUpLevel_DX12(EditorSingleTest): from Atom.tests import hydra_AtomGPU_BasicLevelSetup as test_module - assert compare_screenshot_to_golden_image(screenshot_directory, test_screenshots, golden_images, 0.99) is True + extra_cmdline_args = ["-rhi=dx12"] + + # Custom setup/teardown to remove old screenshots and establish paths to golden images + def setup(self, request, workspace, editor, editor_test_results, launcher_platform): + self.screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) + self.screenshot_names = ["AtomBasicLevelSetup.ppm"] + self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( + screenshot_directory=self.screenshot_directory, + screenshot_names=self.screenshot_names) + + def wrap_run(self, request, workspace, editor, editor_test_results, launcher_platform): + yield + assert compare_screenshot_to_golden_image(self.screenshot_directory, + self.test_screenshots, + self.golden_images, + similarity_threshold=0.96) is True @pytest.mark.test_case_id("C34525095") - class AtomGPU_LightComponent_AreaLightScreenshotsMatchGoldenImages(EditorSharedTest): - use_null_renderer = False # Default is True - screenshot_names = [ - "AreaLight_1.ppm", - "AreaLight_2.ppm", - "AreaLight_3.ppm", - "AreaLight_4.ppm", - "AreaLight_5.ppm", - ] - test_screenshots = [] # Gets set by setup() - screenshot_directory = "" # Gets set by setup() + @pytest.mark.REQUIRES_gpu + class AtomGPU_LightComponent_AreaLightScreenshotsMatchGoldenImages_DX12(EditorSingleTest): + from Atom.tests import hydra_AtomGPU_AreaLightScreenshotTest as test_module - # Clear existing test screenshots before starting test. - def setup(self, workspace): - screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) - for screenshot in self.screenshot_names: - screenshot_path = os.path.join(screenshot_directory, screenshot) - self.test_screenshots.append(screenshot_path) - file_system.delete(self.test_screenshots, True, True) + extra_cmdline_args = ["-rhi=dx12"] - golden_images = [] - for golden_image in screenshot_names: - golden_image_path = os.path.join(golden_images_directory(), golden_image) - golden_images.append(golden_image_path) + # Custom setup/teardown to remove old screenshots and establish paths to golden images + def setup(self, request, workspace, editor, editor_test_results, launcher_platform): + self.screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) + self.screenshot_names = [ + "AreaLight_1.ppm", + "AreaLight_2.ppm", + "AreaLight_3.ppm", + "AreaLight_4.ppm", + "AreaLight_5.ppm", + ] + self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( + screenshot_directory=self.screenshot_directory, + screenshot_names=self.screenshot_names) + def wrap_run(self, request, workspace, editor, editor_test_results, launcher_platform): + yield + assert compare_screenshot_to_golden_image(self.screenshot_directory, + self.test_screenshots, + self.golden_images, + similarity_threshold=0.96) is True + + @pytest.mark.test_case_id("C34525095") + @pytest.mark.REQUIRES_gpu + class AtomGPU_LightComponent_AreaLightScreenshotsMatchGoldenImages_Vulkan(EditorSingleTest): from Atom.tests import hydra_AtomGPU_AreaLightScreenshotTest as test_module - assert compare_screenshot_to_golden_image(screenshot_directory, test_screenshots, golden_images, 0.99) is True + extra_cmdline_args = ["-rhi=vulkan"] + + # Custom setup/teardown to remove old screenshots and establish paths to golden images + def setup(self, request, workspace, editor, editor_test_results, launcher_platform): + self.screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) + self.screenshot_names = [ + "AreaLight_1.ppm", + "AreaLight_2.ppm", + "AreaLight_3.ppm", + "AreaLight_4.ppm", + "AreaLight_5.ppm", + ] + self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( + screenshot_directory=self.screenshot_directory, + screenshot_names=self.screenshot_names) + + def wrap_run(self, request, workspace, editor, editor_test_results, launcher_platform): + yield + assert compare_screenshot_to_golden_image(self.screenshot_directory, + self.test_screenshots, + self.golden_images, + similarity_threshold=0.96) is True @pytest.mark.test_case_id("C34525110") - class AtomGPU_LightComponent_SpotLightScreenshotsMatchGoldenImages(EditorSharedTest): - use_null_renderer = False # Default is True - screenshot_names = [ - "SpotLight_1.ppm", - "SpotLight_2.ppm", - "SpotLight_3.ppm", - "SpotLight_4.ppm", - "SpotLight_5.ppm", - "SpotLight_6.ppm", - ] - test_screenshots = [] # Gets set by setup() - screenshot_directory = "" # Gets set by setup() + @pytest.mark.REQUIRES_gpu + class AtomGPU_LightComponent_SpotLightScreenshotsMatchGoldenImages_DX12(EditorSingleTest): + from Atom.tests import hydra_AtomGPU_SpotLightScreenshotTest as test_module - # Clear existing test screenshots before starting test. - def setup(self, workspace): - screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) - for screenshot in self.screenshot_names: - screenshot_path = os.path.join(screenshot_directory, screenshot) - self.test_screenshots.append(screenshot_path) - file_system.delete(self.test_screenshots, True, True) + extra_cmdline_args = ["-rhi=dx12"] - golden_images = [] - for golden_image in screenshot_names: - golden_image_path = os.path.join(golden_images_directory(), golden_image) - golden_images.append(golden_image_path) + # Custom setup/teardown to remove old screenshots and establish paths to golden images + def setup(self, request, workspace, editor, editor_test_results, launcher_platform): + self.screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) + self.screenshot_names = [ + "SpotLight_1.ppm", + "SpotLight_2.ppm", + "SpotLight_3.ppm", + "SpotLight_4.ppm", + "SpotLight_5.ppm", + "SpotLight_6.ppm", + ] + print(self.screenshot_directory) + self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( + screenshot_directory=self.screenshot_directory, + screenshot_names=self.screenshot_names) + def wrap_run(self, request, workspace, editor, editor_test_results, launcher_platform): + yield + assert compare_screenshot_to_golden_image(self.screenshot_directory, + self.test_screenshots, + self.golden_images, + similarity_threshold=0.96) is True + + @pytest.mark.test_case_id("C34525110") + @pytest.mark.REQUIRES_gpu + class AtomGPU_LightComponent_SpotLightScreenshotsMatchGoldenImages_Vulkan(EditorSingleTest): from Atom.tests import hydra_AtomGPU_SpotLightScreenshotTest as test_module - assert compare_screenshot_to_golden_image(screenshot_directory, test_screenshots, golden_images, 0.99) is True + extra_cmdline_args = ["-rhi=vulkan"] + + # Custom setup/teardown to remove old screenshots and establish paths to golden images + def setup(self, request, workspace, editor, editor_test_results, launcher_platform): + self.screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) + self.screenshot_names = [ + "SpotLight_1.ppm", + "SpotLight_2.ppm", + "SpotLight_3.ppm", + "SpotLight_4.ppm", + "SpotLight_5.ppm", + "SpotLight_6.ppm", + ] + print(self.screenshot_directory) + self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( + screenshot_directory=self.screenshot_directory, + screenshot_names=self.screenshot_names) + + def wrap_run(self, request, workspace, editor, editor_test_results, launcher_platform): + yield + assert compare_screenshot_to_golden_image(self.screenshot_directory, + self.test_screenshots, + self.golden_images, + similarity_threshold=0.96) is True @pytest.mark.parametrize('rhi', ['dx12', 'vulkan']) diff --git a/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py b/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py index 5fdd4c810d..e76e009de9 100644 --- a/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py +++ b/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py @@ -82,12 +82,13 @@ def compare_screenshot_similarity( result = 'You must specify a screenshot_directory in order to create a zip archive.\n' mean_similarity = compare_screenshots(test_screenshot, golden_image) + print(f"MEAN_SIMILARITY = {mean_similarity}") if not mean_similarity > similarity_threshold: if create_zip_archive: create_screenshots_archive(screenshot_directory) result = ( f"When comparing the test_screenshot: '{test_screenshot}' " - f"to golden_image: '{golden_image}' the mean similarity of '{mean_similarity}' " + f"to golden_image: '{golden_image}'.\nThe mean similarity of '{mean_similarity}' " f"was lower than the similarity threshold of '{similarity_threshold}'. ") return result @@ -123,7 +124,9 @@ def initial_viewport_setup(screen_width=1280, screen_height=720): import azlmbr.legacy.general as general general.set_viewport_size(screen_width, screen_height) + general.idle_wait_frames(1) general.update_viewport() + general.idle_wait_frames(1) def enter_exit_game_mode_take_screenshot(screenshot_name, enter_game_tuple, exit_game_tuple, timeout_in_seconds=4): @@ -137,13 +140,18 @@ def enter_exit_game_mode_take_screenshot(screenshot_name, enter_game_tuple, exit """ import azlmbr.legacy.general as general - from editor_python_test_tools.utils import TestHelper + from editor_python_test_tools.utils import TestHelper, Report from Atom.atom_utils.screenshot_utils import ScreenshotHelper + screenshot_helper = ScreenshotHelper(general.idle_wait_frames) TestHelper.enter_game_mode(enter_game_tuple) TestHelper.wait_for_condition(function=lambda: general.is_in_game_mode(), timeout_in_seconds=timeout_in_seconds) - ScreenshotHelper(general.idle_wait_frames).capture_screenshot_blocking(screenshot_name) + screenshot_helper.prepare_viewport_for_screenshot(1920, 1080) + success_screenshot = TestHelper.wait_for_condition( + function=lambda: screenshot_helper.capture_screenshot_blocking(screenshot_name), + timeout_in_seconds=timeout_in_seconds) + Report.result(("Screenshot taken", "Screenshot failed to be taken"), success_screenshot) TestHelper.exit_game_mode(exit_game_tuple) TestHelper.wait_for_condition(function=lambda: not general.is_in_game_mode(), timeout_in_seconds=timeout_in_seconds) diff --git a/AutomatedTesting/Gem/PythonTests/Atom/tests/hydra_AtomGPU_BasicLevelSetup.py b/AutomatedTesting/Gem/PythonTests/Atom/tests/hydra_AtomGPU_BasicLevelSetup.py index b15e5b2131..c910662222 100644 --- a/AutomatedTesting/Gem/PythonTests/Atom/tests/hydra_AtomGPU_BasicLevelSetup.py +++ b/AutomatedTesting/Gem/PythonTests/Atom/tests/hydra_AtomGPU_BasicLevelSetup.py @@ -107,10 +107,8 @@ def AtomGPU_BasicLevelSetup_SetsUpLevel(): 18. Add Mesh component to Sphere Entity and set the Mesh Asset property for the Mesh component. 19. Create a Camera Entity as a child entity of the Default Level Entity then add a Camera component. 20. Set the Camera Entity rotation value and set the Camera component Field of View value. - 21. Enter game mode. - 22. Take screenshot. - 23. Exit game mode. - 24. Look for errors. + 21. Enter/Exit game mode taking screenshot. + 22. Look for errors. :return: None """ @@ -127,7 +125,7 @@ def AtomGPU_BasicLevelSetup_SetsUpLevel(): from Atom.atom_utils.atom_constants import AtomComponentProperties from Atom.atom_utils.atom_component_helper import initial_viewport_setup - from Atom.atom_utils.screenshot_utils import ScreenshotHelper + from Atom.atom_utils.atom_component_helper import enter_exit_game_mode_take_screenshot DEGREE_RADIAN_FACTOR = 0.0174533 SCREENSHOT_NAME = "AtomBasicLevelSetup" @@ -300,18 +298,10 @@ def AtomGPU_BasicLevelSetup_SetsUpLevel(): Report.result(Tests.camera_fov_set, camera_component.get_component_property_value( AtomComponentProperties.camera('Field of view')) == camera_fov_value) - # 21. Enter game mode. - TestHelper.enter_game_mode(Tests.enter_game_mode) - TestHelper.wait_for_condition(function=lambda: general.is_in_game_mode(), timeout_in_seconds=4.0) + # 21. Enter/Exit game mode taking screenshot. + enter_exit_game_mode_take_screenshot(f"{SCREENSHOT_NAME}.ppm", Tests.enter_game_mode, Tests.exit_game_mode) - # 22. Take screenshot. - ScreenshotHelper(general.idle_wait_frames).capture_screenshot_blocking(f"{SCREENSHOT_NAME}.ppm") - - # 23. Exit game mode. - TestHelper.exit_game_mode(Tests.exit_game_mode) - TestHelper.wait_for_condition(function=lambda: not general.is_in_game_mode(), timeout_in_seconds=4.0) - - # 24. Look for errors. + # 22. Look for errors. TestHelper.wait_for_condition(lambda: error_tracer.has_errors or error_tracer.has_asserts, 1.0) for error_info in error_tracer.errors: Report.info(f"Error: {error_info.filename} {error_info.function} | {error_info.message}") From 361a7385d5d03edd8cdde1a9038ff0ac8856b157 Mon Sep 17 00:00:00 2001 From: Scott Murray Date: Fri, 11 Feb 2022 13:28:09 -0800 Subject: [PATCH 03/12] removing debugging print statement Signed-off-by: Scott Murray --- .../Gem/PythonTests/Atom/atom_utils/atom_component_helper.py | 1 - 1 file changed, 1 deletion(-) diff --git a/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py b/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py index e76e009de9..83ef9ec3d7 100644 --- a/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py +++ b/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py @@ -82,7 +82,6 @@ def compare_screenshot_similarity( result = 'You must specify a screenshot_directory in order to create a zip archive.\n' mean_similarity = compare_screenshots(test_screenshot, golden_image) - print(f"MEAN_SIMILARITY = {mean_similarity}") if not mean_similarity > similarity_threshold: if create_zip_archive: create_screenshots_archive(screenshot_directory) From 8cf29ebc25e983e13eb8261204615849a674b56d Mon Sep 17 00:00:00 2001 From: Chris Galvan Date: Mon, 14 Feb 2022 11:10:58 -0600 Subject: [PATCH 04/12] Added support for several UNORM_SRGB formats Signed-off-by: Chris Galvan --- .../ReflectedPropertyCtrl.cpp | 12 ----- .../ReflectedPropertyItem.cpp | 3 -- .../ReflectedVarWrapper.cpp | 44 ------------------ .../ReflectedVarWrapper.h | 15 ------ Code/Editor/Util/ColorUtils.cpp | 46 ------------------- Code/Editor/Util/EditorUtils.cpp | 13 +++--- Code/Editor/editor_core_files.cmake | 1 - Code/Framework/AzCore/AzCore/Math/Color.h | 6 +++ Code/Framework/AzCore/AzCore/Math/Color.inl | 21 ++++++--- .../Code/Source/Converters/Gamma.cpp | 16 ++----- .../RPI/Code/Source/RPI.Public/RPIUtils.cpp | 17 +++++++ .../Editor/Animation/Util/UiEditorUtils.cpp | 14 +++--- 12 files changed, 56 insertions(+), 152 deletions(-) delete mode 100644 Code/Editor/Util/ColorUtils.cpp diff --git a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyCtrl.cpp b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyCtrl.cpp index c5bf6a4d81..dd32b96b2c 100644 --- a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyCtrl.cpp +++ b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyCtrl.cpp @@ -360,18 +360,6 @@ void ReflectedPropertyControl::CreateItems(XmlNodeRef node, CVarBlockPtr& outBlo textureVar->Set(textureName); } } - else if (!azstricmp(type, "color")) - { - CSmartVariable colorVar; - AddVariable(group, colorVar, child->getTag(), humanReadableName.toUtf8().data(), strDescription.toUtf8().data(), func, pUserData, IVariable::DT_COLOR); - ColorB color; - if (child->getAttr("value", color)) - { - ColorF colorLinear = ColorGammaToLinear(QColor(color.r, color.g, color.b)); - Vec3 colorVec3(colorLinear.r, colorLinear.g, colorLinear.b); - colorVar->Set(colorVec3); - } - } } } diff --git a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyItem.cpp b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyItem.cpp index 0fcbac3204..b56a3e411d 100644 --- a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyItem.cpp +++ b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedPropertyItem.cpp @@ -243,9 +243,6 @@ void ReflectedPropertyItem::SetVariable(IVariable *var) case ePropertySelection: m_reflectedVarAdapter = new ReflectedVarEnumAdapter; break; - case ePropertyColor: - m_reflectedVarAdapter = new ReflectedVarColorAdapter; - break; case ePropertyUser: m_reflectedVarAdapter = new ReflectedVarUserAdapter; break; diff --git a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.cpp b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.cpp index 8eefbbc8eb..1e8d42acc0 100644 --- a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.cpp +++ b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.cpp @@ -312,50 +312,6 @@ void ReflectedVarVector4Adapter::SyncIVarToReflectedVar(IVariable *pVariable) pVariable->Set(Vec4(m_reflectedVar->m_value.GetX(), m_reflectedVar->m_value.GetY(), m_reflectedVar->m_value.GetZ(), m_reflectedVar->m_value.GetW())); } - -void ReflectedVarColorAdapter::SetVariable(IVariable *pVariable) -{ - m_reflectedVar.reset(new CReflectedVarColor(pVariable->GetHumanName().toUtf8().data())); - m_reflectedVar->m_description = pVariable->GetDescription().toUtf8().data(); -} - -void ReflectedVarColorAdapter::SyncReflectedVarToIVar(IVariable *pVariable) -{ - if (pVariable->GetType() == IVariable::VECTOR) - { - Vec3 v(0, 0, 0); - pVariable->Get(v); - const QColor col = ColorLinearToGamma(ColorF(v.x, v.y, v.z)); - m_reflectedVar->m_color.Set(static_cast(col.redF()), static_cast(col.greenF()), static_cast(col.blueF())); - } - else - { - int col(0); - pVariable->Get(col); - const QColor qcolor = ColorToQColor((uint32)col); - m_reflectedVar->m_color.Set(static_cast(qcolor.redF()), static_cast(qcolor.greenF()), static_cast(qcolor.blueF())); - } -} - -void ReflectedVarColorAdapter::SyncIVarToReflectedVar(IVariable *pVariable) -{ - if (pVariable->GetType() == IVariable::VECTOR) - { - ColorF colLin = ColorGammaToLinear(QColor::fromRgbF(m_reflectedVar->m_color.GetX(), m_reflectedVar->m_color.GetY(), m_reflectedVar->m_color.GetZ())); - pVariable->Set(Vec3(colLin.r, colLin.g, colLin.b)); - } - else - { - int ir = static_cast(m_reflectedVar->m_color.GetX() * 255.0f); - int ig = static_cast(m_reflectedVar->m_color.GetY() * 255.0f); - int ib = static_cast(m_reflectedVar->m_color.GetZ() * 255.0f); - - pVariable->Set(static_cast(RGB(ir, ig, ib))); - } -} - - - void ReflectedVarResourceAdapter::SetVariable(IVariable *pVariable) { m_reflectedVar.reset(new CReflectedVarResource(pVariable->GetHumanName().toUtf8().data())); diff --git a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.h b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.h index 807413226c..4efbab0bab 100644 --- a/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.h +++ b/Code/Editor/Controls/ReflectedPropertyControl/ReflectedVarWrapper.h @@ -186,21 +186,6 @@ AZ_PUSH_DISABLE_DLL_EXPORT_MEMBER_WARNING AZ_POP_DISABLE_DLL_EXPORT_MEMBER_WARNING }; - -class EDITOR_CORE_API ReflectedVarColorAdapter - : public ReflectedVarAdapter -{ -public: - void SetVariable(IVariable* pVariable) override; - void SyncReflectedVarToIVar(IVariable* pVariable) override; - void SyncIVarToReflectedVar(IVariable* pVariable) override; - CReflectedVar* GetReflectedVar() override { return m_reflectedVar.data(); } -private: -AZ_PUSH_DISABLE_DLL_EXPORT_MEMBER_WARNING - QScopedPointer m_reflectedVar; -AZ_POP_DISABLE_DLL_EXPORT_MEMBER_WARNING -}; - class EDITOR_CORE_API ReflectedVarResourceAdapter : public ReflectedVarAdapter { diff --git a/Code/Editor/Util/ColorUtils.cpp b/Code/Editor/Util/ColorUtils.cpp deleted file mode 100644 index eaaea8a2ef..0000000000 --- a/Code/Editor/Util/ColorUtils.cpp +++ /dev/null @@ -1,46 +0,0 @@ -/* - * 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 - * - */ - -// Qt -#include -#include - -////////////////////////////////////////////////////////////////////////// -QColor ColorLinearToGamma(ColorF col) -{ - float r = clamp_tpl(col.r, 0.0f, 1.0f); - float g = clamp_tpl(col.g, 0.0f, 1.0f); - float b = clamp_tpl(col.b, 0.0f, 1.0f); - float a = clamp_tpl(col.a, 0.0f, 1.0f); - - r = (float)(r <= 0.0031308 ? (12.92 * r) : (1.055 * pow((double)r, 1.0 / 2.4) - 0.055)); - g = (float)(g <= 0.0031308 ? (12.92 * g) : (1.055 * pow((double)g, 1.0 / 2.4) - 0.055)); - b = (float)(b <= 0.0031308 ? (12.92 * b) : (1.055 * pow((double)b, 1.0 / 2.4) - 0.055)); - - return QColor(int(r * 255.0f), int(g * 255.0f), int(b * 255.0f), int(a * 255.0f)); -} - -////////////////////////////////////////////////////////////////////////// -ColorF ColorGammaToLinear(const QColor& col) -{ - float r = (float)col.red() / 255.0f; - float g = (float)col.green() / 255.0f; - float b = (float)col.blue() / 255.0f; - float a = (float)col.alpha() / 255.0f; - - return ColorF((float)(r <= 0.04045 ? (r / 12.92) : pow(((double)r + 0.055) / 1.055, 2.4)), - (float)(g <= 0.04045 ? (g / 12.92) : pow(((double)g + 0.055) / 1.055, 2.4)), - (float)(b <= 0.04045 ? (b / 12.92) : pow(((double)b + 0.055) / 1.055, 2.4)), a); -} - -QColor ColorToQColor(uint32 color) -{ - return QColor::fromRgbF((float)GetRValue(color) / 255.0f, - (float)GetGValue(color) / 255.0f, - (float)GetBValue(color) / 255.0f); -} diff --git a/Code/Editor/Util/EditorUtils.cpp b/Code/Editor/Util/EditorUtils.cpp index 47264a51ab..440941e469 100644 --- a/Code/Editor/Util/EditorUtils.cpp +++ b/Code/Editor/Util/EditorUtils.cpp @@ -6,6 +6,7 @@ * */ +#include #include "EditorDefs.h" @@ -184,9 +185,9 @@ QColor ColorLinearToGamma(ColorF col) float b = clamp_tpl(col.b, 0.0f, 1.0f); float a = clamp_tpl(col.a, 0.0f, 1.0f); - r = (float)(r <= 0.0031308 ? (12.92 * r) : (1.055 * pow((double)r, 1.0 / 2.4) - 0.055)); - g = (float)(g <= 0.0031308 ? (12.92 * g) : (1.055 * pow((double)g, 1.0 / 2.4) - 0.055)); - b = (float)(b <= 0.0031308 ? (12.92 * b) : (1.055 * pow((double)b, 1.0 / 2.4) - 0.055)); + r = AZ::Color::ConvertSrgbLinearToGamma(r); + g = AZ::Color::ConvertSrgbLinearToGamma(g); + b = AZ::Color::ConvertSrgbLinearToGamma(b); return QColor(int(r * 255.0f), int(g * 255.0f), int(b * 255.0f), int(a * 255.0f)); } @@ -199,9 +200,9 @@ ColorF ColorGammaToLinear(const QColor& col) float b = (float)col.blue() / 255.0f; float a = (float)col.alpha() / 255.0f; - return ColorF((float)(r <= 0.04045 ? (r / 12.92) : pow(((double)r + 0.055) / 1.055, 2.4)), - (float)(g <= 0.04045 ? (g / 12.92) : pow(((double)g + 0.055) / 1.055, 2.4)), - (float)(b <= 0.04045 ? (b / 12.92) : pow(((double)b + 0.055) / 1.055, 2.4)), a); + return ColorF(AZ::Color::ConvertSrgbGammaToLinear(r), + AZ::Color::ConvertSrgbGammaToLinear(g), + AZ::Color::ConvertSrgbGammaToLinear(b), a); } QColor ColorToQColor(uint32 color) diff --git a/Code/Editor/editor_core_files.cmake b/Code/Editor/editor_core_files.cmake index 447d763a63..15ad0f13a1 100644 --- a/Code/Editor/editor_core_files.cmake +++ b/Code/Editor/editor_core_files.cmake @@ -48,7 +48,6 @@ set(FILES Util/Image.cpp Util/ImageHistogram.h Util/Image.h - Util/ColorUtils.cpp Undo/Undo.cpp Undo/IUndoManagerListener.h Undo/IUndoObject.h diff --git a/Code/Framework/AzCore/AzCore/Math/Color.h b/Code/Framework/AzCore/AzCore/Math/Color.h index 4f506e219d..c0c798fbb9 100644 --- a/Code/Framework/AzCore/AzCore/Math/Color.h +++ b/Code/Framework/AzCore/AzCore/Math/Color.h @@ -134,6 +134,12 @@ namespace AZ //! Color from u32 => 0xAABBGGRR, RGB convert from Gamma corrected to Linear values. void FromU32GammaToLinear(u32 c); + //! Convert SRGB gamma space to linear space + static float ConvertSrgbGammaToLinear(float x); + + //! Convert SRGB linear space to gamma space + static float ConvertSrgbLinearToGamma(float x); + //! Convert color from linear to gamma corrected space. Color LinearToGamma() const; diff --git a/Code/Framework/AzCore/AzCore/Math/Color.inl b/Code/Framework/AzCore/AzCore/Math/Color.inl index c07d0f2d88..f3b691e163 100644 --- a/Code/Framework/AzCore/AzCore/Math/Color.inl +++ b/Code/Framework/AzCore/AzCore/Math/Color.inl @@ -370,6 +370,15 @@ namespace AZ *this = GammaToLinear(); } + AZ_MATH_INLINE float Color::ConvertSrgbGammaToLinear(float x) + { + return x <= 0.04045 ? (x / 12.92f) : static_cast(pow((static_cast(x) + 0.055) / 1.055, 2.4)); + } + + AZ_MATH_INLINE float Color::ConvertSrgbLinearToGamma(float x) + { + return x <= 0.0031308 ? 12.92f * x : static_cast(1.055 * pow(static_cast(x), 1.0 / 2.4) - 0.055); + } AZ_MATH_INLINE Color Color::LinearToGamma() const { @@ -377,9 +386,9 @@ namespace AZ float g = GetG(); float b = GetB(); - r = (r <= 0.0031308 ? 12.92f * r : static_cast(1.055 * pow(static_cast(r), 1.0 / 2.4) - 0.055)); - g = (g <= 0.0031308 ? 12.92f * g : static_cast(1.055 * pow(static_cast(g), 1.0 / 2.4) - 0.055)); - b = (b <= 0.0031308 ? 12.92f * b : static_cast(1.055 * pow(static_cast(b), 1.0 / 2.4) - 0.055)); + r = ConvertSrgbLinearToGamma(r); + g = ConvertSrgbLinearToGamma(g); + b = ConvertSrgbLinearToGamma(b); return Color(r,g,b,GetA()); } @@ -391,9 +400,9 @@ namespace AZ float g = GetG(); float b = GetB(); - return Color(r <= 0.04045 ? (r / 12.92f) : static_cast(pow((static_cast(r) + 0.055) / 1.055, 2.4)), - g <= 0.04045 ? (g / 12.92f) : static_cast(pow((static_cast(g) + 0.055) / 1.055, 2.4)), - b <= 0.04045 ? (b / 12.92f) : static_cast(pow((static_cast(b) + 0.055) / 1.055, 2.4)), GetA()); + return Color(ConvertSrgbGammaToLinear(r), + ConvertSrgbGammaToLinear(g), + ConvertSrgbGammaToLinear(b), GetA()); } diff --git a/Gems/Atom/Asset/ImageProcessingAtom/Code/Source/Converters/Gamma.cpp b/Gems/Atom/Asset/ImageProcessingAtom/Code/Source/Converters/Gamma.cpp index e00af4ab88..0e44a22af3 100644 --- a/Gems/Atom/Asset/ImageProcessingAtom/Code/Source/Converters/Gamma.cpp +++ b/Gems/Atom/Asset/ImageProcessingAtom/Code/Source/Converters/Gamma.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -118,19 +119,8 @@ namespace ImageProcessingAtom float m_fMaxDiff = 0.0f; }; - - static float GammaToLinear(float x) - { - return (x <= 0.04045f) ? x / 12.92f : powf((x + 0.055f) / 1.055f, 2.4f); - } - - static float LinearToGamma(float x) - { - return (x <= 0.0031308f) ? x * 12.92f : 1.055f * powf(x, 1.0f / 2.4f) - 0.055f; - } - - static FunctionLookupTable<1024> s_lutGammaToLinear(GammaToLinear, 0.04045f, 0.00001f); - static FunctionLookupTable<1024> s_lutLinearToGamma(LinearToGamma, 0.05f, 0.00001f); + static FunctionLookupTable<1024> s_lutGammaToLinear(AZ::Color::ConvertSrgbGammaToLinear, 0.04045f, 0.00001f); + static FunctionLookupTable<1024> s_lutLinearToGamma(AZ::Color::ConvertSrgbLinearToGamma, 0.05f, 0.00001f); /////////////////////////////////////////////////////////////////////////////////// diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp index a92d0e5ee8..5683a3effa 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp @@ -14,6 +14,7 @@ #include #include +#include #include namespace AZ @@ -113,12 +114,22 @@ namespace AZ case AZ::RHI::Format::A8_UNORM: case AZ::RHI::Format::R8G8_UNORM: case AZ::RHI::Format::R8G8B8A8_UNORM: + case AZ::RHI::Format::A8B8G8R8_UNORM: { return mem[index] / static_cast(std::numeric_limits::max()); } + case AZ::RHI::Format::R8_UNORM_SRGB: + case AZ::RHI::Format::R8G8_UNORM_SRGB: + case AZ::RHI::Format::R8G8B8A8_UNORM_SRGB: + case AZ::RHI::Format::A8B8G8R8_UNORM_SRGB: + { + float srgbValue = mem[index] / static_cast(std::numeric_limits::max()); + return AZ::Color::ConvertSrgbGammaToLinear(srgbValue); + } case AZ::RHI::Format::R8_SNORM: case AZ::RHI::Format::R8G8_SNORM: case AZ::RHI::Format::R8G8B8A8_SNORM: + case AZ::RHI::Format::A8B8G8R8_SNORM: { // Scale the value from AZ::s8 min/max to -1 to 1 // We need to treat -128 and -127 the same, so that we get a symmetric @@ -452,9 +463,15 @@ namespace AZ case AZ::RHI::Format::A8_UNORM: case AZ::RHI::Format::R8G8_UNORM: case AZ::RHI::Format::R8G8B8A8_UNORM: + case AZ::RHI::Format::A8B8G8R8_UNORM: + case AZ::RHI::Format::R8_UNORM_SRGB: + case AZ::RHI::Format::R8G8_UNORM_SRGB: + case AZ::RHI::Format::R8G8B8A8_UNORM_SRGB: + case AZ::RHI::Format::A8B8G8R8_UNORM_SRGB: case AZ::RHI::Format::R8_SNORM: case AZ::RHI::Format::R8G8_SNORM: case AZ::RHI::Format::R8G8B8A8_SNORM: + case AZ::RHI::Format::A8B8G8R8_SNORM: case AZ::RHI::Format::D16_UNORM: case AZ::RHI::Format::R16_UNORM: case AZ::RHI::Format::R16G16_UNORM: diff --git a/Gems/LyShine/Code/Editor/Animation/Util/UiEditorUtils.cpp b/Gems/LyShine/Code/Editor/Animation/Util/UiEditorUtils.cpp index 85270df355..fb43e00780 100644 --- a/Gems/LyShine/Code/Editor/Animation/Util/UiEditorUtils.cpp +++ b/Gems/LyShine/Code/Editor/Animation/Util/UiEditorUtils.cpp @@ -9,6 +9,8 @@ //#include "CustomizeKeyboardPage.h" +#include + #include #include @@ -99,9 +101,9 @@ QColor ColorLinearToGamma(ColorF col) float g = clamp_tpl(col.g, 0.0f, 1.0f); float b = clamp_tpl(col.b, 0.0f, 1.0f); - r = (float)(r <= 0.0031308 ? (12.92 * r) : (1.055 * pow((double)r, 1.0 / 2.4) - 0.055)); - g = (float)(g <= 0.0031308 ? (12.92 * g) : (1.055 * pow((double)g, 1.0 / 2.4) - 0.055)); - b = (float)(b <= 0.0031308 ? (12.92 * b) : (1.055 * pow((double)b, 1.0 / 2.4) - 0.055)); + r = AZ::Color::ConvertSrgbLinearToGamma(r); + g = AZ::Color::ConvertSrgbLinearToGamma(g); + b = AZ::Color::ConvertSrgbLinearToGamma(b); return QColor(int(r * 255.0f), int(g * 255.0f), int(b * 255.0f)); } @@ -113,7 +115,7 @@ ColorF ColorGammaToLinear(const QColor& col) float g = (float)col.green() / 255.0f; float b = (float)col.blue() / 255.0f; - return ColorF((float)(r <= 0.04045 ? (r / 12.92) : pow(((double)r + 0.055) / 1.055, 2.4)), - (float)(g <= 0.04045 ? (g / 12.92) : pow(((double)g + 0.055) / 1.055, 2.4)), - (float)(b <= 0.04045 ? (b / 12.92) : pow(((double)b + 0.055) / 1.055, 2.4))); + return ColorF(AZ::Color::ConvertSrgbGammaToLinear(r), + AZ::Color::ConvertSrgbGammaToLinear(g), + AZ::Color::ConvertSrgbGammaToLinear(b)); } From 9335ce7ba935e61c3759d330e0475f46777a780f Mon Sep 17 00:00:00 2001 From: Scott Murray Date: Mon, 14 Feb 2022 12:21:51 -0800 Subject: [PATCH 05/12] removing pytest marks and debugging prints Signed-off-by: Scott Murray --- .../PythonTests/Atom/TestSuite_Main_GPU.py | 28 ------------------- .../Atom/atom_utils/atom_component_helper.py | 5 ++-- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py b/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py index 8aec70e3fd..ad2662113b 100644 --- a/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py +++ b/AutomatedTesting/Gem/PythonTests/Atom/TestSuite_Main_GPU.py @@ -45,30 +45,7 @@ class TestAutomation(EditorTestSuite): return test_screenshots, golden_images - @pytest.mark.test_case_id("C34603773") - @pytest.mark.REQUIRES_gpu - class AtomGPU_BasicLevelSetup_SetsUpLevel_DX12(EditorSingleTest): - from Atom.tests import hydra_AtomGPU_BasicLevelSetup as test_module - - extra_cmdline_args = ["-rhi=dx12"] - - # Custom setup/teardown to remove old screenshots and establish paths to golden images - def setup(self, request, workspace, editor, editor_test_results, launcher_platform): - self.screenshot_directory = os.path.join(workspace.paths.project(), DEFAULT_SUBFOLDER_PATH) - self.screenshot_names = ["AtomBasicLevelSetup.ppm"] - self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( - screenshot_directory=self.screenshot_directory, - screenshot_names=self.screenshot_names) - - def wrap_run(self, request, workspace, editor, editor_test_results, launcher_platform): - yield - assert compare_screenshot_to_golden_image(self.screenshot_directory, - self.test_screenshots, - self.golden_images, - similarity_threshold=0.96) is True - @pytest.mark.test_case_id("C34525095") - @pytest.mark.REQUIRES_gpu class AtomGPU_LightComponent_AreaLightScreenshotsMatchGoldenImages_DX12(EditorSingleTest): from Atom.tests import hydra_AtomGPU_AreaLightScreenshotTest as test_module @@ -96,7 +73,6 @@ class TestAutomation(EditorTestSuite): similarity_threshold=0.96) is True @pytest.mark.test_case_id("C34525095") - @pytest.mark.REQUIRES_gpu class AtomGPU_LightComponent_AreaLightScreenshotsMatchGoldenImages_Vulkan(EditorSingleTest): from Atom.tests import hydra_AtomGPU_AreaLightScreenshotTest as test_module @@ -124,7 +100,6 @@ class TestAutomation(EditorTestSuite): similarity_threshold=0.96) is True @pytest.mark.test_case_id("C34525110") - @pytest.mark.REQUIRES_gpu class AtomGPU_LightComponent_SpotLightScreenshotsMatchGoldenImages_DX12(EditorSingleTest): from Atom.tests import hydra_AtomGPU_SpotLightScreenshotTest as test_module @@ -141,7 +116,6 @@ class TestAutomation(EditorTestSuite): "SpotLight_5.ppm", "SpotLight_6.ppm", ] - print(self.screenshot_directory) self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( screenshot_directory=self.screenshot_directory, screenshot_names=self.screenshot_names) @@ -154,7 +128,6 @@ class TestAutomation(EditorTestSuite): similarity_threshold=0.96) is True @pytest.mark.test_case_id("C34525110") - @pytest.mark.REQUIRES_gpu class AtomGPU_LightComponent_SpotLightScreenshotsMatchGoldenImages_Vulkan(EditorSingleTest): from Atom.tests import hydra_AtomGPU_SpotLightScreenshotTest as test_module @@ -171,7 +144,6 @@ class TestAutomation(EditorTestSuite): "SpotLight_5.ppm", "SpotLight_6.ppm", ] - print(self.screenshot_directory) self.test_screenshots, self.golden_images = TestAutomation.screenshot_setup( screenshot_directory=self.screenshot_directory, screenshot_names=self.screenshot_names) diff --git a/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py b/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py index 83ef9ec3d7..42310ee983 100644 --- a/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py +++ b/AutomatedTesting/Gem/PythonTests/Atom/atom_utils/atom_component_helper.py @@ -86,9 +86,8 @@ def compare_screenshot_similarity( if create_zip_archive: create_screenshots_archive(screenshot_directory) result = ( - f"When comparing the test_screenshot: '{test_screenshot}' " - f"to golden_image: '{golden_image}'.\nThe mean similarity of '{mean_similarity}' " - f"was lower than the similarity threshold of '{similarity_threshold}'. ") + f"When comparing the test_screenshot: '{test_screenshot}' to golden_image: '{golden_image}'.\n" + f"The mean similarity ({mean_similarity}) was lower than the similarity threshold ({similarity_threshold})") return result From 9bd487e3bda9463ea9ad75bb9eba54eb44b4036f Mon Sep 17 00:00:00 2001 From: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> Date: Tue, 15 Feb 2022 10:11:41 -0800 Subject: [PATCH 06/12] Minor fixes to the configurable stack based on provided feedback. Signed-off-by: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> --- .../AzCore/Settings/ConfigurableStack.h | 4 +-- .../AzCore/Settings/ConfigurableStack.inl | 12 -------- .../Tests/Settings/ConfigurableStackTests.cpp | 30 +++++++++---------- 3 files changed, 15 insertions(+), 31 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h index bafb6f47a3..afb0eca6e0 100644 --- a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h +++ b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.h @@ -110,9 +110,7 @@ namespace AZ Iterator end(); ConstIterator begin() const; ConstIterator end() const; - ConstIterator cbegin() const; - ConstIterator cend() const; - + size_t size() const; protected: diff --git a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl index ba34ac1d03..7cfba21497 100644 --- a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl +++ b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.inl @@ -44,18 +44,6 @@ namespace AZ return m_nodes.end(); } - template - auto ConfigurableStack::cbegin() const -> ConstIterator - { - return m_nodes.cbegin(); - } - - template - auto ConfigurableStack::cend() const -> ConstIterator - { - return m_nodes.cend(); - } - template size_t ConfigurableStack::size() const { diff --git a/Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp b/Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp index df535976bf..0a69ac0053 100644 --- a/Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp +++ b/Code/Framework/AzCore/Tests/Settings/ConfigurableStackTests.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include namespace UnitTest @@ -32,7 +33,7 @@ namespace UnitTest } }; - struct ConfigurableStackTests : public AllocatorsFixture + struct ConfigurableStackTests : public ScopedAllocatorSetupFixture { void Reflect(AZ::ReflectContext* context) { @@ -50,32 +51,29 @@ namespace UnitTest void SetUp() override { - AllocatorsFixture::SetUp(); + ScopedAllocatorSetupFixture::SetUp(); - m_serializeContext = aznew AZ::SerializeContext(); - m_jsonRegistrationContext = aznew AZ::JsonRegistrationContext(); + m_serializeContext = AZStd::make_unique(); + m_jsonRegistrationContext = AZStd::make_unique(); - Reflect(m_serializeContext); - Reflect(m_jsonRegistrationContext); + Reflect(m_serializeContext.get()); + Reflect(m_jsonRegistrationContext.get()); - m_deserializationSettings.m_registrationContext = m_jsonRegistrationContext; - m_deserializationSettings.m_serializeContext = m_serializeContext; + m_deserializationSettings.m_registrationContext = m_jsonRegistrationContext.get(); + m_deserializationSettings.m_serializeContext = m_serializeContext.get(); } void TearDown() { m_jsonRegistrationContext->EnableRemoveReflection(); - Reflect(m_jsonRegistrationContext); + Reflect(m_jsonRegistrationContext.get()); m_jsonRegistrationContext->DisableRemoveReflection(); m_serializeContext->EnableRemoveReflection(); - Reflect(m_serializeContext); + Reflect(m_serializeContext.get()); m_serializeContext->DisableRemoveReflection(); - delete m_jsonRegistrationContext; - delete m_serializeContext; - - AllocatorsFixture::TearDown(); + ScopedAllocatorSetupFixture::TearDown(); } void ObjectTest(AZStd::string_view jsonText) @@ -100,8 +98,8 @@ namespace UnitTest } } - AZ::SerializeContext* m_serializeContext; - AZ::JsonRegistrationContext* m_jsonRegistrationContext; + AZStd::unique_ptr m_serializeContext; + AZStd::unique_ptr m_jsonRegistrationContext; AZ::JsonDeserializerSettings m_deserializationSettings; }; From f91ea8de134fab0dfd5e5d258b7e28ff32577688 Mon Sep 17 00:00:00 2001 From: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> Date: Tue, 15 Feb 2022 13:10:10 -0800 Subject: [PATCH 07/12] Removed #pragma once in cpp file. Signed-off-by: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp index 75748e0f11..320219c7e5 100644 --- a/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp +++ b/Code/Framework/AzCore/AzCore/Settings/ConfigurableStack.cpp @@ -6,8 +6,6 @@ * */ -#pragma once - #include #include #include From 12d5a304b5e0f593ac2390bdc3113ac1add23cbf Mon Sep 17 00:00:00 2001 From: Chris Galvan Date: Tue, 15 Feb 2022 16:47:58 -0600 Subject: [PATCH 08/12] Improved performance of UNORM_SRGB pixel retrieval by switching to a pre-computed lookup table Signed-off-by: Chris Galvan --- .../RPI/Code/Source/RPI.Public/RPIUtils.cpp | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp index 5683a3effa..b2d1663857 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp @@ -15,6 +15,7 @@ #include #include +#include #include namespace AZ @@ -106,6 +107,30 @@ namespace AZ return ((value - origMin) / (origMax - origMin)) * (scaledMax - scaledMin) + scaledMin; } + // Pre-compute a lookup table for converting SRGB gamma to linear + // by specifying the AZ::u8 so we don't have to do the computation + // when retrieving pixels + using ConversionLookupTable = AZStd::array; + ConversionLookupTable CreateSrgbGammaToLinearLookupTable() + { + ConversionLookupTable lookupTable; + + for (size_t i = 0; i < lookupTable.array_size; ++i) + { + float srgbValue = i / static_cast(std::numeric_limits::max()); + lookupTable[i] = AZ::Color::ConvertSrgbGammaToLinear(srgbValue); + } + + return lookupTable; + } + + static ConversionLookupTable s_SrgbGammaToLinearLookupTable = CreateSrgbGammaToLinearLookupTable(); + + float ConvertSrgbGammaToLinearUint(AZ::u8 x) + { + return s_SrgbGammaToLinearLookupTable[x]; + } + float RetrieveFloatValue(const AZ::u8* mem, size_t index, AZ::RHI::Format format) { switch (format) @@ -123,8 +148,9 @@ namespace AZ case AZ::RHI::Format::R8G8B8A8_UNORM_SRGB: case AZ::RHI::Format::A8B8G8R8_UNORM_SRGB: { - float srgbValue = mem[index] / static_cast(std::numeric_limits::max()); - return AZ::Color::ConvertSrgbGammaToLinear(srgbValue); + // Use a variant of ConvertSrgbGammaToLinear that takes an AZ::u8 + // and a lookup table instead of a float for better performance + return ConvertSrgbGammaToLinearUint(mem[index]); } case AZ::RHI::Format::R8_SNORM: case AZ::RHI::Format::R8G8_SNORM: From 9a32d433f47871a30d00e26deef3620f722dc9a8 Mon Sep 17 00:00:00 2001 From: Chris Galvan Date: Tue, 15 Feb 2022 17:29:53 -0600 Subject: [PATCH 09/12] Changed to use the lookup table directly than have a helper method Signed-off-by: Chris Galvan --- Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp index b2d1663857..b1ae2d5698 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/RPIUtils.cpp @@ -126,11 +126,6 @@ namespace AZ static ConversionLookupTable s_SrgbGammaToLinearLookupTable = CreateSrgbGammaToLinearLookupTable(); - float ConvertSrgbGammaToLinearUint(AZ::u8 x) - { - return s_SrgbGammaToLinearLookupTable[x]; - } - float RetrieveFloatValue(const AZ::u8* mem, size_t index, AZ::RHI::Format format) { switch (format) @@ -148,9 +143,9 @@ namespace AZ case AZ::RHI::Format::R8G8B8A8_UNORM_SRGB: case AZ::RHI::Format::A8B8G8R8_UNORM_SRGB: { - // Use a variant of ConvertSrgbGammaToLinear that takes an AZ::u8 - // and a lookup table instead of a float for better performance - return ConvertSrgbGammaToLinearUint(mem[index]); + // Use a lookup table that takes an AZ::u8 instead of a float + // for better performance + return s_SrgbGammaToLinearLookupTable[mem[index]]; } case AZ::RHI::Format::R8_SNORM: case AZ::RHI::Format::R8G8_SNORM: From 16d6cbca2ad017ed5962429a55caad84f263ac2e Mon Sep 17 00:00:00 2001 From: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> Date: Tue, 15 Feb 2022 17:59:04 -0600 Subject: [PATCH 10/12] Fixed intermittent unit test failures. (#7651) The problem is that SurfacePoint doesn't have default values when constructed, and a few of the unit tests weren't setting m_position because the values weren't strictly needed for the test. However, the SurfacePointList checks them for validity and asserted when they couldn't be found, which was hit-and-miss due to it being uninitialized memory that was usually but not always 0xCCCCCCCC. Improved the assert message and set the position values everywhere. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> --- .../EditorGradientSurfaceDataComponent.cpp | 1 + .../Tests/GradientSignalReferencesTests.cpp | 27 ++++++++++--------- .../Code/Source/SurfacePointList.cpp | 6 ++++- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/Gems/GradientSignal/Code/Source/Editor/EditorGradientSurfaceDataComponent.cpp b/Gems/GradientSignal/Code/Source/Editor/EditorGradientSurfaceDataComponent.cpp index 91f1a1b661..201b658436 100644 --- a/Gems/GradientSignal/Code/Source/Editor/EditorGradientSurfaceDataComponent.cpp +++ b/Gems/GradientSignal/Code/Source/Editor/EditorGradientSurfaceDataComponent.cpp @@ -108,6 +108,7 @@ namespace GradientSignal // Create a fake surface point with the position we're sampling. AzFramework::SurfaceData::SurfacePoint point; point.m_position = params.m_position; + point.m_normal = AZ::Vector3::CreateAxisZ(); SurfaceData::SurfacePointList pointList = AZStd::span(&point, 1); // Send it into the component, see what emerges diff --git a/Gems/GradientSignal/Code/Tests/GradientSignalReferencesTests.cpp b/Gems/GradientSignal/Code/Tests/GradientSignalReferencesTests.cpp index 3eef714518..b0a83b670d 100644 --- a/Gems/GradientSignal/Code/Tests/GradientSignalReferencesTests.cpp +++ b/Gems/GradientSignal/Code/Tests/GradientSignalReferencesTests.cpp @@ -78,6 +78,7 @@ namespace UnitTest for (int x = 0; x < dataSize; x++) { float angle = AZ::DegToRad(inputAngles[(y * dataSize) + x]); + point.m_position = AZ::Vector3(aznumeric_cast(x), aznumeric_cast(y), 0.0f); point.m_normal = AZ::Vector3(sinf(angle), 0.0f, cosf(angle)); mockSurface->m_surfacePoints[AZStd::make_pair(static_cast(x), static_cast(y))] = AZStd::span(&point, 1); @@ -549,10 +550,10 @@ namespace UnitTest mockSurface->m_bounds = mockShapeComponentHandler.m_GetEncompassingAabb; AzFramework::SurfaceData::SurfacePoint mockOutputs[] = { - { AZ::Vector3(0.0f, 0.0f, 0.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 2.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 5.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 10.0f), AZ::Vector3::CreateZero() }, + { AZ::Vector3(0.0f, 0.0f, 0.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 2.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 5.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 10.0f), AZ::Vector3::CreateAxisZ() }, }; mockSurface->m_surfacePoints[AZStd::make_pair(0.0f, 0.0f)] = @@ -597,10 +598,10 @@ namespace UnitTest auto mockSurface = surfaceEntity->CreateComponent(); mockSurface->m_bounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(0.0f), AZ::Vector3(1.0f)); AzFramework::SurfaceData::SurfacePoint mockOutputs[] = { - { AZ::Vector3(0.0f, 0.0f, 0.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 2.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 5.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 10.0f), AZ::Vector3::CreateZero() }, + { AZ::Vector3(0.0f, 0.0f, 0.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 2.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 5.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 10.0f), AZ::Vector3::CreateAxisZ() }, }; mockSurface->m_surfacePoints[AZStd::make_pair(0.0f, 0.0f)] = @@ -667,10 +668,10 @@ namespace UnitTest auto mockSurface = surfaceEntity->CreateComponent(); mockSurface->m_bounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(0.0f), AZ::Vector3(1.0f)); AzFramework::SurfaceData::SurfacePoint mockOutputs[] = { - { AZ::Vector3(0.0f, 0.0f, -10.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, -5.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 15.0f), AZ::Vector3::CreateZero() }, - { AZ::Vector3(0.0f, 0.0f, 20.0f), AZ::Vector3::CreateZero() }, + { AZ::Vector3(0.0f, 0.0f, -10.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, -5.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 15.0f), AZ::Vector3::CreateAxisZ() }, + { AZ::Vector3(0.0f, 0.0f, 20.0f), AZ::Vector3::CreateAxisZ() }, }; // Altitude value below min - should result in 0.0f. @@ -724,6 +725,8 @@ namespace UnitTest { for (int x = 0; x < dataSize; x++) { + point.m_position = AZ::Vector3(aznumeric_cast(x), aznumeric_cast(y), 0.0f); + point.m_normal = AZ::Vector3::CreateAxisZ(); point.m_surfaceTags.clear(); point.m_surfaceTags.emplace_back(AZ_CRC_CE("test_mask"), expectedOutput[(y * dataSize) + x]); mockSurface->m_surfacePoints[AZStd::make_pair(static_cast(x), static_cast(y))] = diff --git a/Gems/SurfaceData/Code/Source/SurfacePointList.cpp b/Gems/SurfaceData/Code/Source/SurfacePointList.cpp index 4e9d618ba4..fea06e3ee5 100644 --- a/Gems/SurfaceData/Code/Source/SurfacePointList.cpp +++ b/Gems/SurfaceData/Code/Source/SurfacePointList.cpp @@ -32,7 +32,11 @@ namespace SurfaceData inPositionIndex = (inPositionIndex + 1) % m_inputPositions.size(); } - AZ_Assert(foundMatch, "Couldn't find input position!"); + AZ_Assert( + foundMatch, + "Couldn't find input position: (%0.7f, %0.7f, %0.7f), m_lastInputPositionIndex = %zu, m_inputPositions.size() = %zu", + inPosition.GetX(), inPosition.GetY(), inPosition.GetZ(), m_lastInputPositionIndex, m_inputPositions.size()); + m_lastInputPositionIndex = inPositionIndex; return inPositionIndex; } From 1413977aca2f3d7acf37c87f227afbf0c18e1307 Mon Sep 17 00:00:00 2001 From: Benjamin Jillich <43751992+amzn-jillich@users.noreply.github.com> Date: Wed, 16 Feb 2022 16:07:33 +0100 Subject: [PATCH 11/12] Animation Editor: Motion event colors aren't in sync with the event presets (#7582) The motion events in the timeview had the wrong colors assigned as they weren't correctly mapped to the given event presets. Signed-off-by: Benjamin Jillich --- .../Source/MotionEventPresetManager.cpp | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/Gems/EMotionFX/Code/EMotionFX/Tools/EMotionStudio/EMStudioSDK/Source/MotionEventPresetManager.cpp b/Gems/EMotionFX/Code/EMotionFX/Tools/EMotionStudio/EMStudioSDK/Source/MotionEventPresetManager.cpp index ac5dd80f4f..b807b2b80d 100644 --- a/Gems/EMotionFX/Code/EMotionFX/Tools/EMotionStudio/EMStudioSDK/Source/MotionEventPresetManager.cpp +++ b/Gems/EMotionFX/Code/EMotionFX/Tools/EMotionStudio/EMStudioSDK/Source/MotionEventPresetManager.cpp @@ -302,25 +302,28 @@ namespace EMStudio } } - // Check if motion event with this configuration exists and return color. AZ::u32 MotionEventPresetManager::GetEventColor(const EMotionFX::EventDataSet& eventDatas) const { for (const MotionEventPreset* preset : m_eventPresets) { - EMotionFX::EventDataSet commonDatas; const EMotionFX::EventDataSet& presetDatas = preset->GetEventDatas(); - const bool allMatch = AZStd::all_of(presetDatas.cbegin(), presetDatas.cend(), [eventDatas](const EMotionFX::EventDataPtr& presetData) + + const size_t numEventDatas = eventDatas.size(); + if (numEventDatas == presetDatas.size()) { - const auto thisPresetDataHasMatch = AZStd::find_if(eventDatas.cbegin(), eventDatas.cend(), [presetData](const EMotionFX::EventDataPtr& eventData) + for (size_t i = 0; i < numEventDatas; ++i) { - return ((presetData && eventData && *presetData == *eventData) || (!presetData && !eventData)); - }); - return thisPresetDataHasMatch != eventDatas.cend(); - }); - if (allMatch) - { - return preset->GetEventColor(); + const EMotionFX::EventDataPtr& eventData = eventDatas[i]; + const EMotionFX::EventDataPtr& presetData = presetDatas[i]; + + if (eventData && presetData && + eventData->RTTI_GetType() == presetData->RTTI_GetType() && + *eventData == *presetData) + { + return preset->GetEventColor(); + } + } } } From 0e328afcdd72a4c518b73d2cc109391f982379b4 Mon Sep 17 00:00:00 2001 From: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> Date: Wed, 16 Feb 2022 09:17:01 -0600 Subject: [PATCH 12/12] Optimize surface providers (#7631) * Add comparison operators to SurfaceTagWeight. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Changed AddSurfaceTagWeight to always combine weights. This simplifies the API a bit and defines the behavior if someone ever tries to add a duplicate tag. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Added benchmarks for measuring the performance-critical APIs. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Changed SurfaceTagWeights to a fixed_vector. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Add inPosition to AddSurfacePoint. This will be used to detect which input the surface point is associated with. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Add inPositionIndex to the appropriate APIs. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switched Gradient Surface benchmarks to use actual surface components. The gradient unit tests and benchmarks were previously using a mock surface data system, which led to misleading benchmark results. Now, the actual SurfaceData system gets constructed, and the tests use a mock provider, but the benchmarks use actual shape providers for more realistic benchmarking. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fixed unit tests to have better query ranges. Half of each previous range was querying outside the surface provider's data. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * First attempt at removing SurfacePointLists. This currently runs significantly slower than the previous code but passes the unit tests. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Another attempt at optimization. This one runs faster than the previous, but still slow. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fix the cmake dependency so that the gradient tests rebuild SurfaceData.dll when run. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switch SurfaceAltitudeGradient over to the new bulk API. Also, optimized the non-bulk API by having it reuse the SurfacePointList to avoid the repeated allocation / deallocation cost. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switched to using an indirect index so that all allocations are consecutive in our reserved buffer. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switched back to SurfaceTagWeight again. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Added runtime dependency to LmbrCentral for unit tests. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switched code over to use the full EnumeratePoints in most cases. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Added knowledge of max surface point creation into the system. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Add generic GetSurfacePointsFromList API implementation for surface providers. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fixed implementation to use the correct maximum number of input points based on the surface providers being queried. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fix out-of-bounds references on empty lists. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fix memory allocation that caused benchmark runs to crash. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Starting to clean up the API. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Move SurfacePointList into separate files for easier maintainability. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fixed bug where too many points were filtered out due to using the position Z as a part of the AABB check. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Made FilterPoints an internal part of SurfacePointList so we can choose when and how to perform the filtering. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Final cleanup / comments. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Added includes for non-unity builds. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Optimized GetValues() implementations. Also consolidated GetValue() and GetValues() down to a single implementation under the covers for easier maintenance. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Changed how unit tests initialize the mock lists to try and fix the linux errors. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Switch to explicit span declarations to help ensure this works with linux. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Fixed compile error. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Make the bulk terrain APIs take in const Vector instead of non-const. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Optimize the surface data providers for bulk queries. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> --- .../Terrain/TerrainDataRequestBus.h | 32 ++++----- .../Mocks/Terrain/MockTerrainDataRequestBus.h | 32 ++++----- .../SurfaceData/SurfaceDataMeshComponent.cpp | 52 +++++++------- .../SurfaceData/SurfaceDataMeshComponent.h | 2 +- .../SurfaceAltitudeGradientComponent.h | 20 ------ .../Components/SurfaceMaskGradientComponent.h | 19 ----- .../SurfaceSlopeGradientComponent.h | 33 --------- .../SurfaceAltitudeGradientComponent.cpp | 36 ++++------ .../SurfaceMaskGradientComponent.cpp | 50 +++++-------- .../SurfaceSlopeGradientComponent.cpp | 70 +++++++++++-------- .../Components/SurfaceDataColliderComponent.h | 1 + .../Components/SurfaceDataShapeComponent.h | 1 + .../SurfaceDataColliderComponent.cpp | 45 +++++++++--- .../Components/SurfaceDataShapeComponent.cpp | 46 +++++++++--- .../TerrainSurfaceDataSystemComponent.cpp | 35 +++++++--- .../TerrainSurfaceDataSystemComponent.h | 2 + .../Source/TerrainSystem/TerrainSystem.cpp | 48 ++++++------- .../Code/Source/TerrainSystem/TerrainSystem.h | 50 ++++++------- 18 files changed, 282 insertions(+), 292 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h index a7d65e2bb5..bff29334d1 100644 --- a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h +++ b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h @@ -140,28 +140,28 @@ namespace AzFramework //! Given a list of XY coordinates, call the provided callback function with surface data corresponding to each //! XY coordinate in the list. - virtual void ProcessHeightsFromList(const AZStd::span& inPositions, + virtual void ProcessHeightsFromList(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; - virtual void ProcessNormalsFromList(const AZStd::span& inPositions, + virtual void ProcessNormalsFromList(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; - virtual void ProcessSurfaceWeightsFromList(const AZStd::span& inPositions, + virtual void ProcessSurfaceWeightsFromList(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; - virtual void ProcessSurfacePointsFromList(const AZStd::span& inPositions, + virtual void ProcessSurfacePointsFromList(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; - virtual void ProcessHeightsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessHeightsFromListOfVector2(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; - virtual void ProcessNormalsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessNormalsFromListOfVector2(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; - virtual void ProcessSurfaceWeightsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessSurfaceWeightsFromListOfVector2(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; - virtual void ProcessSurfacePointsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessSurfacePointsFromListOfVector2(const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const = 0; @@ -272,42 +272,42 @@ namespace AzFramework //! Asynchronous versions of the various 'Process*' API functions declared above. //! It's the responsibility of the caller to ensure all callbacks are threadsafe. virtual AZStd::shared_ptr ProcessHeightsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; virtual AZStd::shared_ptr ProcessNormalsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; virtual AZStd::shared_ptr ProcessSurfaceWeightsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; virtual AZStd::shared_ptr ProcessSurfacePointsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; virtual AZStd::shared_ptr ProcessHeightsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; virtual AZStd::shared_ptr ProcessNormalsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; virtual AZStd::shared_ptr ProcessSurfaceWeightsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; virtual AZStd::shared_ptr ProcessSurfacePointsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const = 0; diff --git a/Code/Framework/AzFramework/Tests/Mocks/Terrain/MockTerrainDataRequestBus.h b/Code/Framework/AzFramework/Tests/Mocks/Terrain/MockTerrainDataRequestBus.h index 8f130ceeae..d038d5dd24 100644 --- a/Code/Framework/AzFramework/Tests/Mocks/Terrain/MockTerrainDataRequestBus.h +++ b/Code/Framework/AzFramework/Tests/Mocks/Terrain/MockTerrainDataRequestBus.h @@ -77,21 +77,21 @@ namespace UnitTest MOCK_CONST_METHOD5( GetSurfacePointFromFloats, void(float, float, AzFramework::SurfaceData::SurfacePoint&, Sampler, bool*)); MOCK_CONST_METHOD3( - ProcessHeightsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessHeightsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD3( - ProcessNormalsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessNormalsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD3( - ProcessSurfaceWeightsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessSurfaceWeightsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD3( - ProcessSurfacePointsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessSurfacePointsFromList, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD3( - ProcessHeightsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessHeightsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD3( - ProcessNormalsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessNormalsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD3( - ProcessSurfaceWeightsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessSurfaceWeightsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD3( - ProcessSurfacePointsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); + ProcessSurfacePointsFromListOfVector2, void(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler)); MOCK_CONST_METHOD2( GetNumSamplesFromRegion, AZStd::pair(const AZ::Aabb&, const AZ::Vector2&)); MOCK_CONST_METHOD4( @@ -107,21 +107,21 @@ namespace UnitTest MOCK_CONST_METHOD1( GetClosestIntersection, AzFramework::RenderGeometry::RayResult(const AzFramework::RenderGeometry::RayRequest&)); MOCK_CONST_METHOD4( - ProcessHeightsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessHeightsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD4( - ProcessNormalsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessNormalsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD4( - ProcessSurfaceWeightsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessSurfaceWeightsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD4( - ProcessSurfacePointsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessSurfacePointsFromListAsync, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD4( - ProcessHeightsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessHeightsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD4( - ProcessNormalsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessNormalsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD4( - ProcessSurfaceWeightsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessSurfaceWeightsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD4( - ProcessSurfacePointsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); + ProcessSurfacePointsFromListOfVector2Async, AZStd::shared_ptr(const AZStd::span&, AzFramework::Terrain::SurfacePointListFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD5( ProcessHeightsFromRegionAsync, AZStd::shared_ptr(const AZ::Aabb&, const AZ::Vector2&, AzFramework::Terrain::SurfacePointRegionFillCallback, Sampler, AZStd::shared_ptr)); MOCK_CONST_METHOD5( diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp index 117af76325..6d9dad33b3 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.cpp @@ -144,44 +144,46 @@ namespace SurfaceData return false; } - bool SurfaceDataMeshComponent::DoRayTrace(const AZ::Vector3& inPosition, AZ::Vector3& outPosition, AZ::Vector3& outNormal) const + void SurfaceDataMeshComponent::GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const { - AZStd::shared_lock lock(m_cacheMutex); + GetSurfacePointsFromList(AZStd::span(&inPosition, 1), surfacePointList); + } - // test AABB as first pass to claim the point - const AZ::Vector3 testPosition = AZ::Vector3( - inPosition.GetX(), - inPosition.GetY(), - (m_meshBounds.GetMax().GetZ() + m_meshBounds.GetMin().GetZ()) * 0.5f); + void SurfaceDataMeshComponent::GetSurfacePointsFromList( + AZStd::span inPositions, SurfacePointList& surfacePointList) const + { + AZ::Vector3 hitPosition; + AZ::Vector3 hitNormal; - if (!m_meshBounds.Contains(testPosition)) - { - return false; - } + AZStd::shared_lock lock(m_cacheMutex); AZ::RPI::ModelAsset* mesh = m_meshAssetData.GetAs(); if (!mesh) { - return false; + return; } - const AZ::Vector3 rayStart = AZ::Vector3(inPosition.GetX(), inPosition.GetY(), m_meshBounds.GetMax().GetZ() + s_rayAABBHeightPadding); - const AZ::Vector3 rayEnd = AZ::Vector3(inPosition.GetX(), inPosition.GetY(), m_meshBounds.GetMin().GetZ() - s_rayAABBHeightPadding); - return GetMeshRayIntersection( - *mesh, m_meshWorldTM, m_meshWorldTMInverse, m_meshNonUniformScale, rayStart, rayEnd, outPosition, outNormal); - } - - - void SurfaceDataMeshComponent::GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const - { - AZ::Vector3 hitPosition; - AZ::Vector3 hitNormal; - if (DoRayTrace(inPosition, hitPosition, hitNormal)) + for (auto& inPosition : inPositions) { - surfacePointList.AddSurfacePoint(GetEntityId(), inPosition, hitPosition, hitNormal, m_newPointWeights); + // test AABB as first pass to claim the point + if (SurfaceData::AabbContains2D(m_meshBounds, inPosition)) + { + const AZ::Vector3 rayStart = + AZ::Vector3(inPosition.GetX(), inPosition.GetY(), m_meshBounds.GetMax().GetZ() + s_rayAABBHeightPadding); + const AZ::Vector3 rayEnd = + AZ::Vector3(inPosition.GetX(), inPosition.GetY(), m_meshBounds.GetMin().GetZ() - s_rayAABBHeightPadding); + bool rayHit = GetMeshRayIntersection( + *mesh, m_meshWorldTM, m_meshWorldTMInverse, m_meshNonUniformScale, rayStart, rayEnd, hitPosition, hitNormal); + + if (rayHit) + { + surfacePointList.AddSurfacePoint(GetEntityId(), inPosition, hitPosition, hitNormal, m_newPointWeights); + } + } } } + AZ::Aabb SurfaceDataMeshComponent::GetSurfaceAabb() const { return m_meshBounds; diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h index 4a8d3cf427..8b993eb55e 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/SurfaceData/SurfaceDataMeshComponent.h @@ -80,9 +80,9 @@ namespace SurfaceData //////////////////////////////////////////////////////////////////////// // SurfaceDataProviderRequestBus void GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const override; + void GetSurfacePointsFromList(AZStd::span inPositions, SurfacePointList& surfacePointList) const override; private: - bool DoRayTrace(const AZ::Vector3& inPosition, AZ::Vector3& outPosition, AZ::Vector3& outNormal) const; void UpdateMeshData(); void OnCompositionChanged(); diff --git a/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceAltitudeGradientComponent.h b/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceAltitudeGradientComponent.h index 2984495979..5632d17095 100644 --- a/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceAltitudeGradientComponent.h +++ b/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceAltitudeGradientComponent.h @@ -108,29 +108,9 @@ namespace GradientSignal void AddTag(AZStd::string tag) override; private: - static float CalculateAltitudeRatio(const SurfaceData::SurfacePointList& points, size_t inPositionIndex, float altitudeMin, float altitudeMax) - { - if (points.IsEmpty(inPositionIndex)) - { - return 0.0f; - } - - // GetSurfacePoints (which was used to populate the points list) always returns points in decreasing height order, so the - // first point in the list contains the highest altitude. - const float highestAltitude = points.GetHighestSurfacePoint(inPositionIndex).m_position.GetZ(); - - // Turn the absolute altitude value into a 0-1 value by returning the % of the given altitude range that it falls at. - return GetRatio(altitudeMin, altitudeMax, highestAltitude); - } - mutable AZStd::shared_mutex m_cacheMutex; SurfaceAltitudeGradientConfig m_configuration; LmbrCentral::DependencyMonitor m_dependencyMonitor; AZStd::atomic_bool m_dirty{ false }; - - // m_surfacePointList exists here because it will more efficiently reuse memory across GetValue() calls if it stays constructed. - // The mutex exists to ensure that we don't try to use it from GetValue() in multiple threads at once. - mutable AZStd::recursive_mutex m_surfacePointListMutex; - mutable SurfaceData::SurfacePointList m_surfacePointList; }; } diff --git a/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceMaskGradientComponent.h b/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceMaskGradientComponent.h index bfb5767a1a..2571ce8ab1 100644 --- a/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceMaskGradientComponent.h +++ b/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceMaskGradientComponent.h @@ -82,25 +82,6 @@ namespace GradientSignal void AddTag(AZStd::string tag) override; private: - static float GetMaxSurfaceWeight(const SurfaceData::SurfacePointList& points) - { - float result = 0.0f; - points.EnumeratePoints([&result]( - [[maybe_unused]] size_t inPositionIndex, [[maybe_unused]] const AZ::Vector3& position, - [[maybe_unused]] const AZ::Vector3& normal, const SurfaceData::SurfaceTagWeights& masks) -> bool - { - masks.EnumerateWeights( - [&result]([[maybe_unused]] AZ::Crc32 surfaceType, float weight) -> bool - { - result = AZ::GetMax(AZ::GetClamp(weight, 0.0f, 1.0f), result); - return true; - }); - return true; - }); - - return result; - } - SurfaceMaskGradientConfig m_configuration; LmbrCentral::DependencyMonitor m_dependencyMonitor; }; diff --git a/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceSlopeGradientComponent.h b/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceSlopeGradientComponent.h index f54e58d0fc..d7b24e1928 100644 --- a/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceSlopeGradientComponent.h +++ b/Gems/GradientSignal/Code/Include/GradientSignal/Components/SurfaceSlopeGradientComponent.h @@ -124,39 +124,6 @@ namespace GradientSignal void SetFallOffMidpoint(float midpoint) override; private: - float GetSlopeRatio(const SurfaceData::SurfacePointList& points, float angleMin, float angleMax) const - { - constexpr size_t inPositionIndex = 0; - if (points.IsEmpty(inPositionIndex)) - { - return 0.0f; - } - - // Assuming our surface normal vector is actually normalized, we can get the slope - // by just grabbing the Z value. It's the same thing as normal.Dot(AZ::Vector3::CreateAxisZ()). - auto highestSurfacePoint = points.GetHighestSurfacePoint(inPositionIndex); - AZ_Assert( - highestSurfacePoint.m_normal.GetNormalized().IsClose(highestSurfacePoint.m_normal), - "Surface normals are expected to be normalized"); - const float slope = highestSurfacePoint.m_normal.GetZ(); - // Convert slope back to an angle so that we can lerp in "angular space", not "slope value space". - // (We want our 0-1 range to be linear across the range of angles) - const float slopeAngle = acosf(slope); - - switch (m_configuration.m_rampType) - { - case SurfaceSlopeGradientConfig::RampType::SMOOTH_STEP: - return m_configuration.m_smoothStep.GetSmoothedValue(GetRatio(angleMin, angleMax, slopeAngle)); - case SurfaceSlopeGradientConfig::RampType::LINEAR_RAMP_UP: - // For ramp up, linearly interpolate from min to max. - return GetRatio(angleMin, angleMax, slopeAngle); - case SurfaceSlopeGradientConfig::RampType::LINEAR_RAMP_DOWN: - default: - // For ramp down, linearly interpolate from max to min. - return GetRatio(angleMax, angleMin, slopeAngle); - } - } - SurfaceSlopeGradientConfig m_configuration; }; } diff --git a/Gems/GradientSignal/Code/Source/Components/SurfaceAltitudeGradientComponent.cpp b/Gems/GradientSignal/Code/Source/Components/SurfaceAltitudeGradientComponent.cpp index 41ccbafc7a..0206833a63 100644 --- a/Gems/GradientSignal/Code/Source/Components/SurfaceAltitudeGradientComponent.cpp +++ b/Gems/GradientSignal/Code/Source/Components/SurfaceAltitudeGradientComponent.cpp @@ -202,18 +202,9 @@ namespace GradientSignal float SurfaceAltitudeGradientComponent::GetValue(const GradientSampleParams& sampleParams) const { - // For GetValue(), we reuse our SurfacePointList for the GetSurfacePoints query to avoid the repeated cost of memory allocation - // and deallocation across GetValue() calls. However, this also means we can only use it from one thread at a time, so lock a - // mutex to ensure no other threads call GetValue() at the same time. - AZStd::unique_lock surfacePointLock(m_surfacePointListMutex); - - AZStd::shared_lock lock(m_cacheMutex); - - SurfaceData::SurfaceDataSystemRequestBus::Broadcast(&SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePoints, - sampleParams.m_position, m_configuration.m_surfaceTagsToSample, m_surfacePointList); - - constexpr size_t inPositionIndex = 0; - return CalculateAltitudeRatio(m_surfacePointList, inPositionIndex, m_configuration.m_altitudeMin, m_configuration.m_altitudeMax); + float result = 0.0f; + GetValues(AZStd::span(&sampleParams.m_position, 1), AZStd::span(&result, 1)); + return result; } void SurfaceAltitudeGradientComponent::GetValues(AZStd::span positions, AZStd::span outValues) const @@ -231,17 +222,20 @@ namespace GradientSignal &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromList, positions, m_configuration.m_surfaceTagsToSample, points); - if (points.IsEmpty()) + // For each position, turn the height into a 0-1 value based on our min/max altitudes. + for (size_t index = 0; index < positions.size(); index++) { - // No surface data, so no output values. - AZStd::fill(outValues.begin(), outValues.end(), 0.0f); - } - else - { - // For each position, turn the height into a 0-1 value based on our min/max altitudes. - for (size_t index = 0; index < positions.size(); index++) + if (!points.IsEmpty(index)) + { + // Get the point with the highest Z value and use that for the altitude. + const float highestAltitude = points.GetHighestSurfacePoint(index).m_position.GetZ(); + + // Turn the absolute altitude value into a 0-1 value by returning the % of the given altitude range that it falls at. + outValues[index] = GetRatio(m_configuration.m_altitudeMin, m_configuration.m_altitudeMax, highestAltitude); + } + else { - outValues[index] = CalculateAltitudeRatio(points, index, m_configuration.m_altitudeMin, m_configuration.m_altitudeMax); + outValues[index] = 0.0f; } } } diff --git a/Gems/GradientSignal/Code/Source/Components/SurfaceMaskGradientComponent.cpp b/Gems/GradientSignal/Code/Source/Components/SurfaceMaskGradientComponent.cpp index 56bb846ca4..1b8fd36148 100644 --- a/Gems/GradientSignal/Code/Source/Components/SurfaceMaskGradientComponent.cpp +++ b/Gems/GradientSignal/Code/Source/Components/SurfaceMaskGradientComponent.cpp @@ -162,16 +162,7 @@ namespace GradientSignal float SurfaceMaskGradientComponent::GetValue(const GradientSampleParams& params) const { float result = 0.0f; - - if (!m_configuration.m_surfaceTagList.empty()) - { - SurfaceData::SurfacePointList points; - SurfaceData::SurfaceDataSystemRequestBus::Broadcast(&SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePoints, - params.m_position, m_configuration.m_surfaceTagList, points); - - result = GetMaxSurfaceWeight(points); - } - + GetValues(AZStd::span(¶ms.m_position, 1), AZStd::span(&result, 1)); return result; } @@ -183,34 +174,31 @@ namespace GradientSignal return; } - bool valuesFound = false; + // Initialize all our output values to 0. + AZStd::fill(outValues.begin(), outValues.end(), 0.0f); if (!m_configuration.m_surfaceTagList.empty()) { - // Rather than calling GetSurfacePoints on the EBus repeatedly in a loop, we instead pass a lambda into the EBus that contains - // the loop within it so that we can avoid the repeated EBus-calling overhead. + SurfaceData::SurfacePointList points; SurfaceData::SurfaceDataSystemRequestBus::Broadcast( - [this, positions, &outValues, &valuesFound](SurfaceData::SurfaceDataSystemRequestBus::Events* surfaceDataRequests) + &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromList, positions, m_configuration.m_surfaceTagList, + points); + + // For each position, get the max surface weight that matches our filter and that appears at that position. + points.EnumeratePoints( + [&outValues]( + size_t inPositionIndex, [[maybe_unused]] const AZ::Vector3& position, [[maybe_unused]] const AZ::Vector3& normal, + const SurfaceData::SurfaceTagWeights& masks) -> bool { - // It's possible that there's nothing connected to the EBus, so keep track of the fact that we have valid results. - valuesFound = true; - SurfaceData::SurfacePointList points; - - for (size_t index = 0; index < positions.size(); index++) - { - points.Clear(); - surfaceDataRequests->GetSurfacePoints(positions[index], m_configuration.m_surfaceTagList, points); - outValues[index] = GetMaxSurfaceWeight(points); - } + masks.EnumerateWeights( + [inPositionIndex, &outValues]([[maybe_unused]] AZ::Crc32 surfaceType, float weight) -> bool + { + outValues[inPositionIndex] = AZ::GetMax(AZ::GetClamp(weight, 0.0f, 1.0f), outValues[inPositionIndex]); + return true; + }); + return true; }); } - - if (!valuesFound) - { - // No surface tags, so no output values. - AZStd::fill(outValues.begin(), outValues.end(), 0.0f); - } - } size_t SurfaceMaskGradientComponent::GetNumTags() const diff --git a/Gems/GradientSignal/Code/Source/Components/SurfaceSlopeGradientComponent.cpp b/Gems/GradientSignal/Code/Source/Components/SurfaceSlopeGradientComponent.cpp index fdfa9dd5f8..09f604c919 100644 --- a/Gems/GradientSignal/Code/Source/Components/SurfaceSlopeGradientComponent.cpp +++ b/Gems/GradientSignal/Code/Source/Components/SurfaceSlopeGradientComponent.cpp @@ -205,14 +205,9 @@ namespace GradientSignal float SurfaceSlopeGradientComponent::GetValue(const GradientSampleParams& sampleParams) const { - SurfaceData::SurfacePointList points; - SurfaceData::SurfaceDataSystemRequestBus::Broadcast(&SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePoints, - sampleParams.m_position, m_configuration.m_surfaceTagsToSample, points); - - const float angleMin = AZ::DegToRad(AZ::GetClamp(m_configuration.m_slopeMin, 0.0f, 90.0f)); - const float angleMax = AZ::DegToRad(AZ::GetClamp(m_configuration.m_slopeMax, 0.0f, 90.0f)); - - return GetSlopeRatio(points, angleMin, angleMax); + float result = 0.0f; + GetValues(AZStd::span(&sampleParams.m_position, 1), AZStd::span(&result, 1)); + return result; } void SurfaceSlopeGradientComponent::GetValues(AZStd::span positions, AZStd::span outValues) const @@ -223,32 +218,49 @@ namespace GradientSignal return; } - bool valuesFound = false; - - // Rather than calling GetSurfacePoints on the EBus repeatedly in a loop, we instead pass a lambda into the EBus that contains - // the loop within it so that we can avoid the repeated EBus-calling overhead. + SurfaceData::SurfacePointList points; SurfaceData::SurfaceDataSystemRequestBus::Broadcast( - [this, positions, &outValues, &valuesFound](SurfaceData::SurfaceDataSystemRequestBus::Events* surfaceDataRequests) - { - // It's possible that there's nothing connected to the EBus, so keep track of the fact that we have valid results. - valuesFound = true; - SurfaceData::SurfacePointList points; + &SurfaceData::SurfaceDataSystemRequestBus::Events::GetSurfacePointsFromList, positions, m_configuration.m_surfaceTagsToSample, + points); - const float angleMin = AZ::DegToRad(AZ::GetClamp(m_configuration.m_slopeMin, 0.0f, 90.0f)); - const float angleMax = AZ::DegToRad(AZ::GetClamp(m_configuration.m_slopeMax, 0.0f, 90.0f)); + const float angleMin = AZ::DegToRad(AZ::GetClamp(m_configuration.m_slopeMin, 0.0f, 90.0f)); + const float angleMax = AZ::DegToRad(AZ::GetClamp(m_configuration.m_slopeMax, 0.0f, 90.0f)); - for (size_t index = 0; index < positions.size(); index++) + for (size_t index = 0; index < positions.size(); index++) + { + if (points.IsEmpty(index)) + { + outValues[index] = 0.0f; + } + else + { + // Assuming our surface normal vector is actually normalized, we can get the slope + // by just grabbing the Z value. It's the same thing as normal.Dot(AZ::Vector3::CreateAxisZ()). + auto highestSurfacePoint = points.GetHighestSurfacePoint(index); + AZ_Assert( + highestSurfacePoint.m_normal.GetNormalized().IsClose(highestSurfacePoint.m_normal), + "Surface normals are expected to be normalized"); + const float slope = highestSurfacePoint.m_normal.GetZ(); + // Convert slope back to an angle so that we can lerp in "angular space", not "slope value space". + // (We want our 0-1 range to be linear across the range of angles) + const float slopeAngle = acosf(slope); + + switch (m_configuration.m_rampType) { - points.Clear(); - surfaceDataRequests->GetSurfacePoints(positions[index], m_configuration.m_surfaceTagsToSample, points); - outValues[index] = GetSlopeRatio(points, angleMin, angleMax); + case SurfaceSlopeGradientConfig::RampType::SMOOTH_STEP: + outValues[index] = m_configuration.m_smoothStep.GetSmoothedValue(GetRatio(angleMin, angleMax, slopeAngle)); + break; + case SurfaceSlopeGradientConfig::RampType::LINEAR_RAMP_UP: + // For ramp up, linearly interpolate from min to max. + outValues[index] = GetRatio(angleMin, angleMax, slopeAngle); + break; + case SurfaceSlopeGradientConfig::RampType::LINEAR_RAMP_DOWN: + default: + // For ramp down, linearly interpolate from max to min. + outValues[index] = GetRatio(angleMax, angleMin, slopeAngle); + break; } - }); - - if (!valuesFound) - { - // No surface tags, so no output values. - AZStd::fill(outValues.begin(), outValues.end(), 0.0f); + } } } diff --git a/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataColliderComponent.h b/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataColliderComponent.h index 2ef25774aa..1ff8ab6f4f 100644 --- a/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataColliderComponent.h +++ b/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataColliderComponent.h @@ -82,6 +82,7 @@ namespace SurfaceData //////////////////////////////////////////////////////////////////////// // SurfaceDataProviderRequestBus void GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const override; + void GetSurfacePointsFromList(AZStd::span inPositions, SurfacePointList& surfacePointList) const override; ////////////////////////////////////////////////////////////////////////// // SurfaceDataModifierRequestBus diff --git a/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataShapeComponent.h b/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataShapeComponent.h index 25ac491809..f43ae90574 100644 --- a/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataShapeComponent.h +++ b/Gems/SurfaceData/Code/Include/SurfaceData/Components/SurfaceDataShapeComponent.h @@ -66,6 +66,7 @@ namespace SurfaceData ////////////////////////////////////////////////////////////////////////// // SurfaceDataProviderRequestBus void GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const override; + void GetSurfacePointsFromList(AZStd::span inPositions, SurfacePointList& surfacePointList) const override; ////////////////////////////////////////////////////////////////////////// // SurfaceDataModifierRequestBus diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp index 5506d7640b..b145ef3181 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp @@ -185,8 +185,6 @@ namespace SurfaceData bool SurfaceDataColliderComponent::DoRayTrace(const AZ::Vector3& inPosition, bool queryPointOnly, AZ::Vector3& outPosition, AZ::Vector3& outNormal) const { - AZStd::shared_lock lock(m_cacheMutex); - // test AABB as first pass to claim the point const AZ::Vector3 testPosition = AZ::Vector3( inPosition.GetX(), @@ -229,18 +227,45 @@ namespace SurfaceData void SurfaceDataColliderComponent::GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const { - AZ::Vector3 hitPosition; - AZ::Vector3 hitNormal; + GetSurfacePointsFromList(AZStd::span(&inPosition, 1), surfacePointList); + } - // We want a full raycast, so don't just query the start point. - constexpr bool queryPointOnly = false; + void SurfaceDataColliderComponent::GetSurfacePointsFromList( + AZStd::span inPositions, SurfacePointList& surfacePointList) const + { + AzPhysics::SimulatedBodyComponentRequestsBus::Event( + GetEntityId(), + [this, inPositions, &surfacePointList](AzPhysics::SimulatedBodyComponentRequestsBus::Events* simBody) + { + AZStd::shared_lock lock(m_cacheMutex); - if (DoRayTrace(inPosition, queryPointOnly, hitPosition, hitNormal)) - { - surfacePointList.AddSurfacePoint(GetEntityId(), inPosition, hitPosition, hitNormal, m_newPointWeights); - } + AzPhysics::RayCastRequest request; + request.m_direction = -AZ::Vector3::CreateAxisZ(); + + for (auto& inPosition : inPositions) + { + // test AABB as first pass to claim the point + if (SurfaceData::AabbContains2D(m_colliderBounds, inPosition)) + { + // We're casting the ray to look for a collision, so start at the top of the collider and cast downwards + // the full height of the collider. + request.m_start = AZ::Vector3(inPosition.GetX(), inPosition.GetY(), m_colliderBounds.GetMax().GetZ()); + request.m_distance = m_colliderBounds.GetExtents().GetZ(); + + AzPhysics::SceneQueryHit result = simBody->RayCast(request); + + if (result) + { + surfacePointList.AddSurfacePoint( + GetEntityId(), inPosition, result.m_position, result.m_normal, m_newPointWeights); + } + } + } + + }); } + void SurfaceDataColliderComponent::ModifySurfacePoints(SurfacePointList& surfacePointList) const { AZStd::shared_lock lock(m_cacheMutex); diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp index ec81f06134..251fece203 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp @@ -143,24 +143,48 @@ namespace SurfaceData } void SurfaceDataShapeComponent::GetSurfacePoints(const AZ::Vector3& inPosition, SurfacePointList& surfacePointList) const + { + GetSurfacePointsFromList(AZStd::span(&inPosition, 1), surfacePointList); + } + + void SurfaceDataShapeComponent::GetSurfacePointsFromList( + AZStd::span inPositions, SurfacePointList& surfacePointList) const { AZStd::shared_lock lock(m_cacheMutex); - if (m_shapeBoundsIsValid && SurfaceData::AabbContains2D(m_shapeBounds, inPosition)) + if (!m_shapeBoundsIsValid) { - const AZ::Vector3 rayOrigin = AZ::Vector3(inPosition.GetX(), inPosition.GetY(), m_shapeBounds.GetMax().GetZ()); - const AZ::Vector3 rayDirection = -AZ::Vector3::CreateAxisZ(); - float intersectionDistance = 0.0f; - bool hitShape = false; - LmbrCentral::ShapeComponentRequestsBus::EventResult(hitShape, GetEntityId(), &LmbrCentral::ShapeComponentRequestsBus::Events::IntersectRay, rayOrigin, rayDirection, intersectionDistance); - if (hitShape) - { - AZ::Vector3 position = rayOrigin + intersectionDistance * rayDirection; - surfacePointList.AddSurfacePoint(GetEntityId(), inPosition, position, AZ::Vector3::CreateAxisZ(), m_newPointWeights); - } + return; } + + LmbrCentral::ShapeComponentRequestsBus::Event( + GetEntityId(), + [this, inPositions, &surfacePointList](LmbrCentral::ShapeComponentRequestsBus::Events* shape) + { + const AZ::Vector3 rayDirection = -AZ::Vector3::CreateAxisZ(); + + // Shapes don't currently have a way to query normals at a point intersection, so we'll just return a Z-up normal + // until they get support for it. + const AZ::Vector3 surfacePointNormal = AZ::Vector3::CreateAxisZ(); + + for (auto& inPosition : inPositions) + { + if (SurfaceData::AabbContains2D(m_shapeBounds, inPosition)) + { + const AZ::Vector3 rayOrigin = AZ::Vector3(inPosition.GetX(), inPosition.GetY(), m_shapeBounds.GetMax().GetZ()); + float intersectionDistance = 0.0f; + bool hitShape = shape->IntersectRay(rayOrigin, rayDirection, intersectionDistance); + if (hitShape) + { + AZ::Vector3 position = rayOrigin + intersectionDistance * rayDirection; + surfacePointList.AddSurfacePoint(GetEntityId(), inPosition, position, surfacePointNormal, m_newPointWeights); + } + } + } + }); } + void SurfaceDataShapeComponent::ModifySurfacePoints(SurfacePointList& surfacePointList) const { AZStd::shared_lock lock(m_cacheMutex); diff --git a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp index cf149c0f3c..5baf5540ab 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp @@ -145,29 +145,42 @@ namespace Terrain void TerrainSurfaceDataSystemComponent::GetSurfacePoints( const AZ::Vector3& inPosition, SurfaceData::SurfacePointList& surfacePointList) const + { + GetSurfacePointsFromList(AZStd::span(&inPosition, 1), surfacePointList); + } + + void TerrainSurfaceDataSystemComponent::GetSurfacePointsFromList( + AZStd::span inPositions, SurfaceData::SurfacePointList& surfacePointList) const { if (!m_terrainBoundsIsValid) { return; } - bool isTerrainValidAtPoint = false; - AzFramework::SurfaceData::SurfacePoint terrainSurfacePoint; - AzFramework::Terrain::TerrainDataRequestBus::Broadcast(&AzFramework::Terrain::TerrainDataRequestBus::Events::GetSurfacePoint, - inPosition, terrainSurfacePoint, AzFramework::Terrain::TerrainDataRequests::Sampler::BILINEAR, - &isTerrainValidAtPoint); + size_t inPositionIndex = 0; - const bool isHole = !isTerrainValidAtPoint; + AzFramework::Terrain::TerrainDataRequestBus::Broadcast( + &AzFramework::Terrain::TerrainDataRequestBus::Events::ProcessSurfacePointsFromList, inPositions, + [this, inPositions, &inPositionIndex, &surfacePointList] + (const AzFramework::SurfaceData::SurfacePoint& surfacePoint, bool terrainExists) + { + AZ_Assert(inPositionIndex < inPositions.size(), "Too many points returned from ProcessSurfacePointsFromList"); + + SurfaceData::SurfaceTagWeights weights(surfacePoint.m_surfaceTags); - SurfaceData::SurfaceTagWeights weights(terrainSurfacePoint.m_surfaceTags); + // Always add a "terrain" or "terrainHole" tag. + const AZ::Crc32 terrainTag = terrainExists ? Constants::s_terrainTagCrc : Constants::s_terrainHoleTagCrc; + weights.AddSurfaceTagWeight(terrainTag, 1.0f); - // Always add a "terrain" or "terrainHole" tag. - const AZ::Crc32 terrainTag = isHole ? Constants::s_terrainHoleTagCrc : Constants::s_terrainTagCrc; - weights.AddSurfaceTagWeight(terrainTag, 1.0f); + surfacePointList.AddSurfacePoint( + GetEntityId(), inPositions[inPositionIndex], surfacePoint.m_position, surfacePoint.m_normal, weights); - surfacePointList.AddSurfacePoint(GetEntityId(), inPosition, terrainSurfacePoint.m_position, terrainSurfacePoint.m_normal, weights); + inPositionIndex++; + }, + AzFramework::Terrain::TerrainDataRequests::Sampler::BILINEAR); } + AZ::Aabb TerrainSurfaceDataSystemComponent::GetSurfaceAabb() const { auto terrain = AzFramework::Terrain::TerrainDataRequestBus::FindFirstHandler(); diff --git a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.h b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.h index 4d9a5b7481..0f70cfbfd1 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.h +++ b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.h @@ -59,6 +59,8 @@ namespace Terrain ////////////////////////////////////////////////////////////////////////// // SurfaceDataProviderRequestBus void GetSurfacePoints(const AZ::Vector3& inPosition, SurfaceData::SurfacePointList& surfacePointList) const override; + void GetSurfacePointsFromList( + AZStd::span inPositions, SurfaceData::SurfacePointList& surfacePointList) const override; ////////////////////////////////////////////////////////////////////////// // AzFramework::Terrain::TerrainDataNotificationBus diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp index 2a8f1ba1ca..182cb84541 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp +++ b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp @@ -192,7 +192,7 @@ bool TerrainSystem::InWorldBounds(float x, float y) const } // Generate positions to be queried based on the sampler type. -void TerrainSystem::GenerateQueryPositions(const AZStd::span& inPositions, +void TerrainSystem::GenerateQueryPositions(const AZStd::span& inPositions, AZStd::vector& outPositions, Sampler sampler) const { @@ -253,7 +253,7 @@ AZStd::vector TerrainSystem::GenerateInputPositionsFromRegion( } void TerrainSystem::MakeBulkQueries( - const AZStd::span inPositions, + const AZStd::span inPositions, AZStd::span outPositions, AZStd::span outTerrainExists, AZStd::span outSurfaceWeights, @@ -296,7 +296,7 @@ void TerrainSystem::MakeBulkQueries( if (prevAreaId != AZ::EntityId()) { size_t spanLength = (windowEnd - windowStart) + 1; - queryCallback(AZStd::span(inPositions.begin() + windowStart, spanLength), + queryCallback(AZStd::span(inPositions.begin() + windowStart, spanLength), AZStd::span(outPositions.begin() + windowStart, spanLength), AZStd::span(outTerrainExists.begin() + windowStart, spanLength), AZStd::span(outSurfaceWeights.begin() + windowStart, spanLength), @@ -311,7 +311,7 @@ void TerrainSystem::MakeBulkQueries( } } -void TerrainSystem::GetHeightsSynchronous(const AZStd::span& inPositions, Sampler sampler, +void TerrainSystem::GetHeightsSynchronous(const AZStd::span& inPositions, Sampler sampler, AZStd::span heights, AZStd::span terrainExists) const { AZStd::shared_lock lock(m_areaMutex); @@ -328,7 +328,7 @@ void TerrainSystem::GetHeightsSynchronous(const AZStd::span& inPosi GenerateQueryPositions(inPositions, outPositions, sampler); - auto callback = []([[maybe_unused]] const AZStd::span inPositions, + auto callback = []([[maybe_unused]] const AZStd::span inPositions, AZStd::span outPositions, AZStd::span outTerrainExists, [[maybe_unused]] AZStd::span outSurfaceWeights, @@ -516,7 +516,7 @@ bool TerrainSystem::GetIsHoleFromFloats(float x, float y, Sampler sampler) const return !terrainExists; } -void TerrainSystem::GetNormalsSynchronous(const AZStd::span& inPositions, Sampler sampler, +void TerrainSystem::GetNormalsSynchronous(const AZStd::span& inPositions, Sampler sampler, AZStd::span normals, AZStd::span terrainExists) const { AZStd::vector directionVectors; @@ -685,7 +685,7 @@ AzFramework::RenderGeometry::RayResult TerrainSystem::GetClosestIntersection( } AZStd::shared_ptr TerrainSystem::ProcessHeightsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -695,7 +695,7 @@ AZStd::shared_ptr } AZStd::shared_ptr TerrainSystem::ProcessNormalsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -705,7 +705,7 @@ AZStd::shared_ptr } AZStd::shared_ptr TerrainSystem::ProcessSurfaceWeightsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -715,7 +715,7 @@ AZStd::shared_ptr } AZStd::shared_ptr TerrainSystem::ProcessSurfacePointsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -725,7 +725,7 @@ AZStd::shared_ptr } AZStd::shared_ptr TerrainSystem::ProcessHeightsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -735,7 +735,7 @@ AZStd::shared_ptr } AZStd::shared_ptr TerrainSystem::ProcessNormalsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -745,7 +745,7 @@ AZStd::shared_ptr } AZStd::shared_ptr TerrainSystem::ProcessSurfaceWeightsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -755,7 +755,7 @@ AZStd::shared_ptr } AZStd::shared_ptr TerrainSystem::ProcessSurfacePointsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -830,7 +830,7 @@ AZ::EntityId TerrainSystem::FindBestAreaEntityAtPosition(float x, float y, AZ::A } void TerrainSystem::GetOrderedSurfaceWeightsFromList( - const AZStd::span& inPositions, + const AZStd::span& inPositions, [[maybe_unused]] Sampler sampler, AZStd::span outSurfaceWeightsList, AZStd::span terrainExists) const @@ -841,7 +841,7 @@ void TerrainSystem::GetOrderedSurfaceWeightsFromList( GetHeightsSynchronous(inPositions, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, heights, terrainExists); } - auto callback = [](const AZStd::span inPositions, + auto callback = [](const AZStd::span inPositions, [[maybe_unused]] AZStd::span outPositions, [[maybe_unused]] AZStd::span outTerrainExists, AZStd::span outSurfaceWeights, @@ -929,7 +929,7 @@ const char* TerrainSystem::GetMaxSurfaceName( } void TerrainSystem::ProcessHeightsFromList( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { @@ -953,7 +953,7 @@ void TerrainSystem::ProcessHeightsFromList( } void TerrainSystem::ProcessNormalsFromList( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { @@ -977,7 +977,7 @@ void TerrainSystem::ProcessNormalsFromList( } void TerrainSystem::ProcessSurfaceWeightsFromList( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { @@ -1001,7 +1001,7 @@ void TerrainSystem::ProcessSurfaceWeightsFromList( } void TerrainSystem::ProcessSurfacePointsFromList( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { @@ -1034,7 +1034,7 @@ void TerrainSystem::ProcessSurfacePointsFromList( } void TerrainSystem::ProcessHeightsFromListOfVector2( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { @@ -1054,7 +1054,7 @@ void TerrainSystem::ProcessHeightsFromListOfVector2( } void TerrainSystem::ProcessNormalsFromListOfVector2( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { @@ -1074,7 +1074,7 @@ void TerrainSystem::ProcessNormalsFromListOfVector2( } void TerrainSystem::ProcessSurfaceWeightsFromListOfVector2( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { @@ -1094,7 +1094,7 @@ void TerrainSystem::ProcessSurfaceWeightsFromListOfVector2( } void TerrainSystem::ProcessSurfacePointsFromListOfVector2( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter) const { diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h index a4a309c0a7..502a89f711 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h +++ b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h @@ -140,28 +140,28 @@ namespace Terrain //! Given a list of XY coordinates, call the provided callback function with surface data corresponding to each //! XY coordinate in the list. - virtual void ProcessHeightsFromList(const AZStd::span& inPositions, + virtual void ProcessHeightsFromList(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; - virtual void ProcessNormalsFromList(const AZStd::span& inPositions, + virtual void ProcessNormalsFromList(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; - virtual void ProcessSurfaceWeightsFromList(const AZStd::span& inPositions, + virtual void ProcessSurfaceWeightsFromList(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; - virtual void ProcessSurfacePointsFromList(const AZStd::span& inPositions, + virtual void ProcessSurfacePointsFromList(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; - virtual void ProcessHeightsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessHeightsFromListOfVector2(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; - virtual void ProcessNormalsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessNormalsFromListOfVector2(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; - virtual void ProcessSurfaceWeightsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessSurfaceWeightsFromListOfVector2(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; - virtual void ProcessSurfacePointsFromListOfVector2(const AZStd::span& inPositions, + virtual void ProcessSurfacePointsFromListOfVector2(const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT) const override; @@ -194,42 +194,42 @@ namespace Terrain const AzFramework::RenderGeometry::RayRequest& ray) const override; AZStd::shared_ptr ProcessHeightsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; AZStd::shared_ptr ProcessNormalsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; AZStd::shared_ptr ProcessSurfaceWeightsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; AZStd::shared_ptr ProcessSurfacePointsFromListAsync( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; AZStd::shared_ptr ProcessHeightsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; AZStd::shared_ptr ProcessNormalsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; AZStd::shared_ptr ProcessSurfaceWeightsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; AZStd::shared_ptr ProcessSurfacePointsFromListOfVector2Async( - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const override; @@ -262,7 +262,7 @@ namespace Terrain template AZStd::shared_ptr ProcessFromListAsync( SynchronousFunctionType synchronousFunction, - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter = Sampler::DEFAULT, AZStd::shared_ptr params = nullptr) const; @@ -291,31 +291,31 @@ namespace Terrain AZ::Vector3 GetNormalSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const; typedef AZStd::function inPositions, + const AZStd::span inPositions, AZStd::span outPositions, AZStd::span outTerrainExists, AZStd::span outSurfaceWeights, AZ::EntityId areaId)> BulkQueriesCallback; void GetHeightsSynchronous( - const AZStd::span& inPositions, + const AZStd::span& inPositions, Sampler sampler, AZStd::span heights, AZStd::span terrainExists) const; void GetNormalsSynchronous( - const AZStd::span& inPositions, + const AZStd::span& inPositions, Sampler sampler, AZStd::span normals, AZStd::span terrainExists) const; void GetOrderedSurfaceWeightsFromList( - const AZStd::span& inPositions, Sampler sampler, + const AZStd::span& inPositions, Sampler sampler, AZStd::span outSurfaceWeightsList, AZStd::span terrainExists) const; void MakeBulkQueries( - const AZStd::span inPositions, + const AZStd::span inPositions, AZStd::span outPositions, AZStd::span outTerrainExists, AZStd::span outSurfaceWieghts, BulkQueriesCallback queryCallback) const; - void GenerateQueryPositions(const AZStd::span& inPositions, + void GenerateQueryPositions(const AZStd::span& inPositions, AZStd::vector& outPositions, Sampler sampler) const; AZStd::vector GenerateInputPositionsFromRegion( @@ -361,7 +361,7 @@ namespace Terrain template inline AZStd::shared_ptr TerrainSystem::ProcessFromListAsync( SynchronousFunctionType synchronousFunction, - const AZStd::span& inPositions, + const AZStd::span& inPositions, AzFramework::Terrain::SurfacePointListFillCallback perPositionCallback, Sampler sampleFilter, AZStd::shared_ptr params) const @@ -397,7 +397,7 @@ namespace Terrain const size_t subSpanCount = (i < numJobs - 1) ? numPositionsPerJob : AZStd::dynamic_extent; // Define the job function using the sub span of positions to process. - const AZStd::span& positionsToProcess = inPositions.subspan(subSpanOffset, subSpanCount); + const AZStd::span& positionsToProcess = inPositions.subspan(subSpanOffset, subSpanCount); auto jobFunction = [this, synchronousFunction, positionsToProcess, perPositionCallback, sampleFilter, jobContext, params]() { // Process the sub span of positions, unless the associated job context has been cancelled.