From 92b6446da1211ad02571161849f4855134aca1a5 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Thu, 12 Jan 2023 13:28:42 -0800 Subject: [PATCH 1/5] UnitTests: Add custom main that calls RegisterMsgAlertHandler This prevents a failed assertion from hanging on the MSVC buildbots. --- Source/UnitTests/CMakeLists.txt | 4 +++- Source/UnitTests/UnitTests.vcxproj | 2 +- Source/UnitTests/UnitTestsMain.cpp | 30 ++++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 Source/UnitTests/UnitTestsMain.cpp diff --git a/Source/UnitTests/CMakeLists.txt b/Source/UnitTests/CMakeLists.txt index 50167617a0..c3a3f23782 100644 --- a/Source/UnitTests/CMakeLists.txt +++ b/Source/UnitTests/CMakeLists.txt @@ -4,6 +4,8 @@ add_custom_command(TARGET unittests POST_BUILD COMMAND ${CMAKE_CTEST_COMMAND}) string(APPEND CMAKE_RUNTIME_OUTPUT_DIRECTORY "/Tests") +add_library(unittests_main OBJECT UnitTestsMain.cpp) +target_link_libraries(unittests_main PUBLIC fmt gtest) # Since this is a Core dependency, it can't be linked as a normal library. # Otherwise CMake inserts the library after core, but before other core # dependencies like videocommon which also use Host_ functions, which makes the @@ -16,7 +18,7 @@ macro(add_dolphin_test target) $ ) set_target_properties(${target} PROPERTIES FOLDER Tests) - target_link_libraries(${target} PRIVATE core uicommon gtest_main) + target_link_libraries(${target} PRIVATE core uicommon unittests_main) add_dependencies(unittests ${target}) add_test(NAME ${target} COMMAND ${target}) endmacro() diff --git a/Source/UnitTests/UnitTests.vcxproj b/Source/UnitTests/UnitTests.vcxproj index 6cf886c6c6..2114af7c7c 100644 --- a/Source/UnitTests/UnitTests.vcxproj +++ b/Source/UnitTests/UnitTests.vcxproj @@ -36,8 +36,8 @@ - + diff --git a/Source/UnitTests/UnitTestsMain.cpp b/Source/UnitTests/UnitTestsMain.cpp new file mode 100644 index 0000000000..aa4df43f48 --- /dev/null +++ b/Source/UnitTests/UnitTestsMain.cpp @@ -0,0 +1,30 @@ +// Copyright 2023 Dolphin Emulator Project +// SPDX-License-Identifier: GPL-2.0-or-later +// Based on gtest_main.cc + +#include +#include + +#include "Common/MsgHandler.h" + +#include "gtest/gtest.h" + +namespace +{ +bool TestMsgHandler(const char* caption, const char* text, bool yes_no, Common::MsgType style) +{ + fmt::print(stderr, "{}\n", text); + ADD_FAILURE(); + // Return yes to any question (we don't need Dolphin to break on asserts) + return true; +} +} // namespace + +int main(int argc, char** argv) +{ + fmt::print(stderr, "Running main() from UnitTestsMain.cpp\n"); + Common::RegisterMsgAlertHandler(TestMsgHandler); + + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From c681d96d468eb63e8b322226c634eb72cdc10d01 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Thu, 12 Jan 2023 12:39:20 -0800 Subject: [PATCH 2/5] VertexLoaderTester: Use asserts instead of logs Logs don't show up in unit tests, and since this is debugging functionality (though not enabled for tests by default) it's better to do it this way. --- Source/Core/VideoCommon/VertexLoaderBase.cpp | 24 ++++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/Source/Core/VideoCommon/VertexLoaderBase.cpp b/Source/Core/VideoCommon/VertexLoaderBase.cpp index dae75ddd6a..e8b38ba345 100644 --- a/Source/Core/VideoCommon/VertexLoaderBase.cpp +++ b/Source/Core/VideoCommon/VertexLoaderBase.cpp @@ -64,22 +64,16 @@ public: int count_a = a->RunVertices(src, buffer_a.data(), count); int count_b = b->RunVertices(src, buffer_b.data(), count); - if (count_a != count_b) - { - ERROR_LOG_FMT( - VIDEO, - "The two vertex loaders have loaded a different amount of vertices (a: {}, b: {}).", - count_a, count_b); - } + ASSERT_MSG(VIDEO, count_a == count_b, + "The two vertex loaders have loaded a different amount of vertices (a: {}, b: {}).", + count_a, count_b); - if (memcmp(buffer_a.data(), buffer_b.data(), - std::min(count_a, count_b) * m_native_vtx_decl.stride)) - { - ERROR_LOG_FMT(VIDEO, - "The two vertex loaders have loaded different data. Configuration:" - "\nVertex desc:\n{}\n\nVertex attr:\n{}", - m_VtxDesc, m_VtxAttr); - } + ASSERT_MSG(VIDEO, + memcmp(buffer_a.data(), buffer_b.data(), + std::min(count_a, count_b) * m_native_vtx_decl.stride) == 0, + "The two vertex loaders have loaded different data. Configuration:" + "\nVertex desc:\n{}\n\nVertex attr:\n{}", + m_VtxDesc, m_VtxAttr); memcpy(dst, buffer_a.data(), count_a * m_native_vtx_decl.stride); m_numLoadedVertices += count; From 2d53b7364363b8d32f62905dbdbff01531ebe06d Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Thu, 12 Jan 2023 12:54:01 -0800 Subject: [PATCH 3/5] VertexLoaderTester: Add assertions for position/binormal/tangent caches --- Source/Core/VideoCommon/VertexLoaderBase.cpp | 80 +++++++++++++++++++- 1 file changed, 78 insertions(+), 2 deletions(-) diff --git a/Source/Core/VideoCommon/VertexLoaderBase.cpp b/Source/Core/VideoCommon/VertexLoaderBase.cpp index e8b38ba345..f04580a074 100644 --- a/Source/Core/VideoCommon/VertexLoaderBase.cpp +++ b/Source/Core/VideoCommon/VertexLoaderBase.cpp @@ -13,11 +13,13 @@ #include #include "Common/Assert.h" +#include "Common/BitUtils.h" #include "Common/CommonTypes.h" #include "Common/Logging/Log.h" #include "Common/MsgHandler.h" #include "VideoCommon/VertexLoader.h" +#include "VideoCommon/VertexLoaderManager.h" #include "VideoCommon/VertexLoader_Color.h" #include "VideoCommon/VertexLoader_Normal.h" #include "VideoCommon/VertexLoader_Position.h" @@ -61,8 +63,36 @@ public: buffer_a.resize(count * a->m_native_vtx_decl.stride + 4); buffer_b.resize(count * b->m_native_vtx_decl.stride + 4); - int count_a = a->RunVertices(src, buffer_a.data(), count); - int count_b = b->RunVertices(src, buffer_b.data(), count); + const std::array old_position_matrix_index_cache = + VertexLoaderManager::position_matrix_index_cache; + const std::array, 3> old_position_cache = + VertexLoaderManager::position_cache; + const std::array old_tangent_cache = VertexLoaderManager::tangent_cache; + const std::array old_binormal_cache = VertexLoaderManager::binormal_cache; + + const int count_a = a->RunVertices(src, buffer_a.data(), count); + + const std::array a_position_matrix_index_cache = + VertexLoaderManager::position_matrix_index_cache; + const std::array, 3> a_position_cache = + VertexLoaderManager::position_cache; + const std::array a_tangent_cache = VertexLoaderManager::tangent_cache; + const std::array a_binormal_cache = VertexLoaderManager::binormal_cache; + + // Reset state before running b + VertexLoaderManager::position_matrix_index_cache = old_position_matrix_index_cache; + VertexLoaderManager::position_cache = old_position_cache; + VertexLoaderManager::tangent_cache = old_tangent_cache; + VertexLoaderManager::binormal_cache = old_binormal_cache; + + const int count_b = b->RunVertices(src, buffer_b.data(), count); + + const std::array b_position_matrix_index_cache = + VertexLoaderManager::position_matrix_index_cache; + const std::array, 3> b_position_cache = + VertexLoaderManager::position_cache; + const std::array b_tangent_cache = VertexLoaderManager::tangent_cache; + const std::array b_binormal_cache = VertexLoaderManager::binormal_cache; ASSERT_MSG(VIDEO, count_a == count_b, "The two vertex loaders have loaded a different amount of vertices (a: {}, b: {}).", @@ -75,6 +105,52 @@ public: "\nVertex desc:\n{}\n\nVertex attr:\n{}", m_VtxDesc, m_VtxAttr); + ASSERT_MSG(VIDEO, a_position_matrix_index_cache == b_position_matrix_index_cache, + "Expected matching position matrix caches after loading (a: {}; b: {})", + fmt::join(a_position_matrix_index_cache, ", "), + fmt::join(b_position_matrix_index_cache, ", ")); + + // Some games (e.g. Donkey Kong Country Returns) have a few draws that contain NaN. + // Since NaN != NaN, we need to compare the bits instead. + const auto bit_equal = [](float a, float b) { + return Common::BitCast(a) == Common::BitCast(b); + }; + + // The last element is allowed to be garbage for SIMD overwrites. + // For XY, the last 2 are garbage. + const bool positions_match = [&] { + const size_t max_component = m_VtxAttr.g0.PosElements == CoordComponentCount::XYZ ? 3 : 2; + for (size_t vertex = 0; vertex < 3; vertex++) + { + if (!std::equal(a_position_cache[vertex].begin(), + a_position_cache[vertex].begin() + max_component, + b_position_cache[vertex].begin(), bit_equal)) + { + return false; + } + } + return true; + }(); + + ASSERT_MSG(VIDEO, positions_match, + "Expected matching position caches after loading (a: {} / {} / {}; b: {} / {} / {})", + fmt::join(a_position_cache[0], ", "), fmt::join(a_position_cache[1], ", "), + fmt::join(a_position_cache[2], ", "), fmt::join(b_position_cache[0], ", "), + fmt::join(b_position_cache[1], ", "), fmt::join(b_position_cache[2], ", ")); + + // The last element is allowed to be garbage for SIMD overwrites + ASSERT_MSG(VIDEO, + std::equal(a_tangent_cache.begin(), a_tangent_cache.begin() + 3, + b_tangent_cache.begin(), b_tangent_cache.begin() + 3, bit_equal), + "Expected matching tangent caches after loading (a: {}; b: {})", + fmt::join(a_tangent_cache, ", "), fmt::join(b_tangent_cache, ", ")); + + ASSERT_MSG(VIDEO, + std::equal(a_binormal_cache.begin(), a_binormal_cache.begin() + 3, + b_binormal_cache.begin(), b_binormal_cache.begin() + 3, bit_equal), + "Expected matching binormal caches after loading (a: {}; b: {})", + fmt::join(a_binormal_cache, ", "), fmt::join(b_binormal_cache, ", ")); + memcpy(dst, buffer_a.data(), count_a * m_native_vtx_decl.stride); m_numLoadedVertices += count; return count_a; From 16c0593a52ddaf3a5bde17a2f56f46b9618e5928 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Thu, 12 Jan 2023 15:00:51 -0800 Subject: [PATCH 4/5] VertexLoader: Fix loading tangent/binormal caches with NormalIndex3 --- Source/Core/VideoCommon/VertexLoader_Normal.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/Source/Core/VideoCommon/VertexLoader_Normal.cpp b/Source/Core/VideoCommon/VertexLoader_Normal.cpp index c713e4c9c6..abb6d27da7 100644 --- a/Source/Core/VideoCommon/VertexLoader_Normal.cpp +++ b/Source/Core/VideoCommon/VertexLoader_Normal.cpp @@ -38,12 +38,13 @@ constexpr float FracAdjust(float val) return val; } -template +template void ReadIndirect(VertexLoader* loader, const T* data) { static_assert(3 == N || 9 == N, "N is only sane as 3 or 9!"); + static_assert(!(Offset != 0 && 9 == N), "N == 9 only makes sense if offset == 0"); - for (u32 i = 0; i < N; ++i) + for (u32 i = Offset; i < N + Offset; ++i) { const float value = FracAdjust(Common::FromBigEndian(data[i])); if (loader->m_remaining == 0) @@ -63,7 +64,7 @@ template void Normal_ReadDirect(VertexLoader* loader) { const auto source = reinterpret_cast(DataGetPosition()); - ReadIndirect(loader, source); + ReadIndirect(loader, source); DataSkip(); } @@ -73,10 +74,10 @@ void Normal_ReadIndex_Offset(VertexLoader* loader) static_assert(std::is_unsigned_v, "Only unsigned I is sane!"); const auto index = DataRead(); - const auto data = reinterpret_cast( - VertexLoaderManager::cached_arraybases[CPArray::Normal] + - (index * g_main_cp_state.array_strides[CPArray::Normal]) + sizeof(T) * 3 * Offset); - ReadIndirect(loader, data); + const auto data = + reinterpret_cast(VertexLoaderManager::cached_arraybases[CPArray::Normal] + + (index * g_main_cp_state.array_strides[CPArray::Normal])); + ReadIndirect(loader, data); } template From 3910bdd68b227bbf62c9822d173879e4dee3ccb3 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Thu, 12 Jan 2023 16:35:57 -0800 Subject: [PATCH 5/5] VertexLoader: Don't write position_cache if vertex is skipped This is the behavior in the x64 and ARM64 vertex loaders. I don't know if it makes sense (the whole skipped vertex system seems jank, but several games behave incorrectly without it). --- Source/Core/VideoCommon/VertexLoader_Position.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/VideoCommon/VertexLoader_Position.cpp b/Source/Core/VideoCommon/VertexLoader_Position.cpp index 42ebf4985f..946dacdde1 100644 --- a/Source/Core/VideoCommon/VertexLoader_Position.cpp +++ b/Source/Core/VideoCommon/VertexLoader_Position.cpp @@ -62,7 +62,7 @@ void Pos_ReadIndex(VertexLoader* loader) for (int i = 0; i < N; ++i) { const float value = PosScale(Common::FromBigEndian(data[i]), scale); - if (loader->m_remaining < 3) + if (loader->m_remaining < 3 && !loader->m_vertexSkip) VertexLoaderManager::position_cache[loader->m_remaining][i] = value; DataWrite(value); }