From 4670003aaf01fd52bc5688cbb5724a3bb3f32543 Mon Sep 17 00:00:00 2001 From: Steve Pham <82231385+spham-amzn@users.noreply.github.com> Date: Fri, 10 Dec 2021 14:54:08 -0800 Subject: [PATCH] Fix failed Atom rpi tests linux (#6319) - Remove trait AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS - Add template specialization for ConstantsData::SetConstant and ConstantsData::GetConstant - Remove trait AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS from source code - Fix ConstantsData::SetConstantArray to pass unit test SetConstant_GetConstant_FalsePackedInGarbage_Bool on Linux, cause by an unwanted optimization (comments describe the situation). This will make this behavior consistent across platforms Signed-off-by: Steve Pham <82231385+spham-amzn@users.noreply.github.com> --- .../Platform/Linux/AzTest_Traits_Linux.h | 2 - .../RHI/Code/Include/Atom/RHI/ConstantsData.h | 7 ++++ .../RHI/Code/Source/RHI/ConstantsData.cpp | 41 ++++++++++++++++++- .../Material/LuaMaterialFunctorTests.cpp | 4 -- .../RPI/Code/Tests/Material/MaterialTests.cpp | 12 ------ ...ShaderResourceGroupConstantBufferTests.cpp | 4 -- 6 files changed, 47 insertions(+), 23 deletions(-) diff --git a/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h b/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h index dfe4bdb3c5..c2ef308dbb 100644 --- a/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h +++ b/Code/Framework/AzTest/AzTest/Platform/Linux/AzTest_Traits_Linux.h @@ -13,8 +13,6 @@ #define AZ_TRAIT_UNIT_TEST_ENTITY_ID_GEN_TEST_COUNT 10000 #define AZ_TRAIT_UNIT_TEST_DILLER_TRIGGER_EVENT_COUNT 100000 -#define AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS true - #define AZ_TRAIT_DISABLE_FAILED_MULTIPLAYER_GRIDMATE_TESTS true #define AZ_TRAIT_DISABLE_FAILED_NATIVE_WINDOWS_TESTS true #define AZ_TRAIT_DISABLE_FAILED_EMOTION_FX_TESTS true diff --git a/Gems/Atom/RHI/Code/Include/Atom/RHI/ConstantsData.h b/Gems/Atom/RHI/Code/Include/Atom/RHI/ConstantsData.h index 82b0b7146b..fd38e4c08d 100644 --- a/Gems/Atom/RHI/Code/Include/Atom/RHI/ConstantsData.h +++ b/Gems/Atom/RHI/Code/Include/Atom/RHI/ConstantsData.h @@ -8,6 +8,7 @@ #pragma once #include +#include #include #include #include @@ -147,6 +148,9 @@ namespace AZ template <> bool ConstantsData::SetConstant(ShaderInputConstantIndex inputIndex, const Vector4& value); + template <> + bool ConstantsData::SetConstant(ShaderInputConstantIndex inputIndex, const Color& value); + template <> bool ConstantsData::SetConstantArray(ShaderInputConstantIndex inputIndex, AZStd::array_view values); @@ -171,6 +175,9 @@ namespace AZ template <> Vector4 ConstantsData::GetConstant(ShaderInputConstantIndex inputIndex) const; + template <> + Color ConstantsData::GetConstant(ShaderInputConstantIndex inputIndex) const; + template bool ConstantsData::SetConstantMatrixRows(ShaderInputConstantIndex inputIndex, const T& value, uint32_t rowCount) { diff --git a/Gems/Atom/RHI/Code/Source/RHI/ConstantsData.cpp b/Gems/Atom/RHI/Code/Source/RHI/ConstantsData.cpp index 1524883e54..1a5d793cb1 100644 --- a/Gems/Atom/RHI/Code/Source/RHI/ConstantsData.cpp +++ b/Gems/Atom/RHI/Code/Source/RHI/ConstantsData.cpp @@ -153,9 +153,21 @@ namespace AZ { bool isValidAll = true; uint32_t offset = 0; + + // Rather than doing the direct validation against values directly, we have to convert the array_view + // to a raw byte array, and then check against the bytes to determine whether or not to set the + // uint32 value to 1 (true) or 0 (false). Clang when building in non-debug builds was optimizing out + // the actual 1 and 0 values, so an expression like: + // + // const uint32_t fourByteValue = values[i] ? 1 : 0; + // + // when values[0] == 205, will instead set 'fourByteValue' is assigned to '205', instead of '1'. + // In debug builds, and other microsoft compilers (debug+release), this type of optimization doesnt + // occur and we get the expected results instead + const AZ::u8* byteValues = reinterpret_cast(values.data()); for (size_t i = 0; i < values.size(); i++) { - const uint32_t fourByteValue = values[i] ? 1 : 0; + const uint32_t fourByteValue = byteValues[i] ? 1 : 0; const bool isValid = SetConstantRaw(inputIndex, &fourByteValue, offset, elementSize); isValidAll &= isValid; @@ -273,6 +285,21 @@ namespace AZ return false; } + template <> + bool ConstantsData::SetConstant(ShaderInputConstantIndex inputIndex, const Color& value) + { + constexpr size_t sizeOfColor = sizeof(Color); + if (ValidateConstantAccess(inputIndex, ValidateConstantAccessExpect::Complete, 0, aznumeric_caster(sizeOfColor))) + { + const Interval interval = GetLayout()->GetInterval(inputIndex); + float* vectorValue = reinterpret_cast(&m_constantData[interval.m_min]); + value.StoreToFloat4(vectorValue); + + return true; + } + return false; + } + bool ConstantsData::SetConstantMatrixRows(ShaderInputConstantIndex inputIndex, const Matrix3x3& value, uint32_t rowCount) { // See the packing comments in ConstantsData::SetConstant for an explanation of why we only use @@ -389,6 +416,18 @@ namespace AZ return Vector4(); } + template <> + Color ConstantsData::GetConstant(ShaderInputConstantIndex inputIndex) const + { + constexpr size_t colorSize = sizeof(Color); + if (ValidateConstantAccess(inputIndex, ValidateConstantAccessExpect::Complete, 0, aznumeric_caster(colorSize))) + { + AZStd::array_view constantBytes = GetConstantRaw(inputIndex); + return Color::CreateFromFloat4(reinterpret_cast(constantBytes.data())); + } + return Color(); + } + AZStd::array_view ConstantsData::GetConstantRaw(ShaderInputConstantIndex inputIndex) const { const Interval interval = GetLayout()->GetInterval(inputIndex); diff --git a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp index 37d0930d97..2608ac3a9b 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp @@ -425,11 +425,7 @@ namespace UnitTest EXPECT_EQ(Vector4(1.0f, 2.0f, 3.0f, 4.0f) / 4.0f, testData.GetMaterial()->GetRHIShaderResourceGroup()->GetData().GetConstant(testData.GetSrgConstantIndex())); } -#if AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS - TEST_F(LuaMaterialFunctorTests, DISABLED_LuaMaterialFunctor_RuntimeContext_GetMaterialProperty_SetShaderConstant_Color) -#else TEST_F(LuaMaterialFunctorTests, LuaMaterialFunctor_RuntimeContext_GetMaterialProperty_SetShaderConstant_Color) -#endif // AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS { using namespace AZ::RPI; diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp index 61999d808e..477978875b 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp @@ -211,21 +211,13 @@ namespace UnitTest EXPECT_NE(materialInstance3, materialInstance4); } -#if AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS - TEST_F(MaterialTests, DISABLED_TestInitialValuesFromMaterial) -#else TEST_F(MaterialTests, TestInitialValuesFromMaterial) -#endif // AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS { Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); ValidateInitialValuesFromMaterial(material); } -#if AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS - TEST_F(MaterialTests, DISABLED_TestSetPropertyValue) -#else TEST_F(MaterialTests, TestSetPropertyValue) -#endif // AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS { Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); @@ -713,11 +705,7 @@ namespace UnitTest AZ_TEST_STOP_ASSERTTEST(2); } -#if AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS - TEST_F(MaterialTests, DISABLED_Error_SetPropertyValue_WrongDataType) -#else TEST_F(MaterialTests, Error_SetPropertyValue_WrongDataType) -#endif // AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS { Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); diff --git a/Gems/Atom/RPI/Code/Tests/ShaderResourceGroup/ShaderResourceGroupConstantBufferTests.cpp b/Gems/Atom/RPI/Code/Tests/ShaderResourceGroup/ShaderResourceGroupConstantBufferTests.cpp index f8b0d34a35..97404aafbb 100644 --- a/Gems/Atom/RPI/Code/Tests/ShaderResourceGroup/ShaderResourceGroupConstantBufferTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/ShaderResourceGroup/ShaderResourceGroupConstantBufferTests.cpp @@ -242,11 +242,7 @@ namespace UnitTest ExpectEqual({ 0 /*false*/, 1 /*true*/ }, resultInUint); } } -#if AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS - TEST_F(ShaderResourceGroupConstantBufferTests, DISABLED_SetConstant_GetConstant_FalsePackedInGarbage_Bool) -#else TEST_F(ShaderResourceGroupConstantBufferTests, SetConstant_GetConstant_FalsePackedInGarbage_Bool) -#endif // AZ_TRAIT_DISABLE_FAILED_ATOM_RPI_TESTS { using namespace AZ;