Merge pull request #11432 from Pokechu22/generic-vertex-loader-test-e…

…rror

VertexLoader: Fix loading tangent/binormal caches with NormalIndex3

Source/Core/VideoCommon/VertexLoaderBase.cpp

@@ -13,11 +13,13 @@
 #include <fmt/format.h>
 
 #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,25 +63,93 @@ class VertexLoaderTester : public VertexLoaderBase
     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);
-
-    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);
-    }
-
-    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);
-    }
+    const std::array<u32, 3> old_position_matrix_index_cache =
+        VertexLoaderManager::position_matrix_index_cache;
+    const std::array<std::array<float, 4>, 3> old_position_cache =
+        VertexLoaderManager::position_cache;
+    const std::array<float, 4> old_tangent_cache = VertexLoaderManager::tangent_cache;
+    const std::array<float, 4> old_binormal_cache = VertexLoaderManager::binormal_cache;
+
+    const int count_a = a->RunVertices(src, buffer_a.data(), count);
+
+    const std::array<u32, 3> a_position_matrix_index_cache =
+        VertexLoaderManager::position_matrix_index_cache;
+    const std::array<std::array<float, 4>, 3> a_position_cache =
+        VertexLoaderManager::position_cache;
+    const std::array<float, 4> a_tangent_cache = VertexLoaderManager::tangent_cache;
+    const std::array<float, 4> 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<u32, 3> b_position_matrix_index_cache =
+        VertexLoaderManager::position_matrix_index_cache;
+    const std::array<std::array<float, 4>, 3> b_position_cache =
+        VertexLoaderManager::position_cache;
+    const std::array<float, 4> b_tangent_cache = VertexLoaderManager::tangent_cache;
+    const std::array<float, 4> 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: {}).",
+               count_a, count_b);
+
+    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);
+
+    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<u32>(a) == Common::BitCast<u32>(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;

Source/Core/VideoCommon/VertexLoader_Normal.cpp

@@ -38,12 +38,13 @@ constexpr float FracAdjust(float val)
   return val;
 }
 
-template <typename T, u32 N>
+template <typename T, u32 N, u32 Offset>
 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 <typename T, u32 N>
 void Normal_ReadDirect(VertexLoader* loader)
 {
   const auto source = reinterpret_cast<const T*>(DataGetPosition());
-  ReadIndirect<T, N * 3>(loader, source);
+  ReadIndirect<T, N * 3, 0>(loader, source);
   DataSkip<N * 3 * sizeof(T)>();
 }
 
@@ -73,10 +74,10 @@ void Normal_ReadIndex_Offset(VertexLoader* loader)
   static_assert(std::is_unsigned_v<I>, "Only unsigned I is sane!");
 
   const auto index = DataRead<I>();
-  const auto data = reinterpret_cast<const T*>(
-      VertexLoaderManager::cached_arraybases[CPArray::Normal] +
-      (index * g_main_cp_state.array_strides[CPArray::Normal]) + sizeof(T) * 3 * Offset);
-  ReadIndirect<T, N * 3>(loader, data);
+  const auto data =
+      reinterpret_cast<const T*>(VertexLoaderManager::cached_arraybases[CPArray::Normal] +
+                                 (index * g_main_cp_state.array_strides[CPArray::Normal]));
+  ReadIndirect<T, N * 3, Offset * 3>(loader, data);
 }
 
 template <typename I, typename T, u32 N>

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);
   }

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)
     $<TARGET_OBJECTS:unittests_stubhost>
   )
   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()

Source/UnitTests/UnitTests.vcxproj

@@ -36,8 +36,8 @@
   <ItemGroup>
     <!--gtest is rather small, so just include it into the build here-->
     <ClCompile Include="$(ExternalsDir)gtest\src\gtest-all.cc" />
-    <ClCompile Include="$(ExternalsDir)gtest\src\gtest_main.cc" />
     <!--Lump all of the tests (and supporting code) into one binary-->
+    <ClCompile Include="UnitTestsMain.cpp" />
     <ClCompile Include="Common\BitFieldTest.cpp" />
     <ClCompile Include="Common\BitSetTest.cpp" />
     <ClCompile Include="Common\BitUtilsTest.cpp" />

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 <cstdio>
+#include <fmt/format.h>
+
+#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();
+}