Merge pull request #9803 from Techjar/bbox-videocommon

VideoCommon: Abstract bounding box

9 files changed, 172 insertions, 91 deletions

diff --git a/Source/Core/VideoCommon/BPStructs.cpp b/Source/Core/VideoCommon/BPStructs.cpp
index 1ea2cbb5c2..fb6372f09c 100644
--- a/Source/Core/VideoCommon/BPStructs.cpp
+++ b/Source/Core/VideoCommon/BPStructs.cpp
@@ -255,7 +255,7 @@ static void BPWritten(const BPCmd& bp)
         if (PE_copy.copy_to_xfb == 1)
         {
           // Make sure we disable Bounding box to match the side effects of the non-failure path
-          BoundingBox::Disable();
+          g_renderer->BBoxDisable();
         }
 
         return;
@@ -287,7 +287,7 @@ static void BPWritten(const BPCmd& bp)
       // We should be able to get away with deactivating the current bbox tracking
       // here. Not sure if there's a better spot to put this.
       // the number of lines copied is determined by the y scale * source efb height
-      BoundingBox::Disable();
+      g_renderer->BBoxDisable();
 
       float yScale;
       if (PE_copy.scale_invert)
@@ -452,7 +452,7 @@ static void BPWritten(const BPCmd& bp)
   case BPMEM_CLEARBBOX2:
   {
     const u8 offset = bp.address & 2;
-    BoundingBox::Enable();
+    g_renderer->BBoxEnable();
 
     g_renderer->BBoxWrite(offset, bp.newvalue & 0x3ff);
     g_renderer->BBoxWrite(offset + 1, bp.newvalue >> 10);
diff --git a/Source/Core/VideoCommon/BoundingBox.cpp b/Source/Core/VideoCommon/BoundingBox.cpp
index d4c8c57bf2..d056efa167 100644
--- a/Source/Core/VideoCommon/BoundingBox.cpp
+++ b/Source/Core/VideoCommon/BoundingBox.cpp
@@ -3,73 +3,115 @@
 
 #include "VideoCommon/BoundingBox.h"
 
-#include <algorithm>
-#include <array>
-
+#include "Common/Assert.h"
 #include "Common/ChunkFile.h"
 #include "Common/CommonTypes.h"
 #include "VideoCommon/PixelShaderManager.h"
+#include "VideoCommon/VideoConfig.h"
 
-namespace BoundingBox
-{
-namespace
-{
-// Whether or not bounding box is enabled.
-bool s_is_active = false;
-
-// Current bounding box coordinates.
-std::array<u16, 4> s_coordinates{
-    0x80,
-    0xA0,
-    0x80,
-    0xA0,
-};
-}  // Anonymous namespace
-
-void Enable()
+void BoundingBox::Enable()
 {
-  s_is_active = true;
-  PixelShaderManager::SetBoundingBoxActive(s_is_active);
+  m_is_active = true;
+  PixelShaderManager::SetBoundingBoxActive(m_is_active);
 }
 
-void Disable()
+void BoundingBox::Disable()
 {
-  s_is_active = false;
-  PixelShaderManager::SetBoundingBoxActive(s_is_active);
+  m_is_active = false;
+  PixelShaderManager::SetBoundingBoxActive(m_is_active);
 }
 
-bool IsEnabled()
+void BoundingBox::Flush()
 {
-  return s_is_active;
+  if (!g_ActiveConfig.backend_info.bSupportsBBox)
+    return;
+
+  m_is_valid = false;
+
+  if (std::none_of(m_dirty.begin(), m_dirty.end(), [](bool dirty) { return dirty; }))
+    return;
+
+  // TODO: Does this make any difference over just writing all the values?
+  // Games only ever seem to write all 4 values at once anyways.
+  for (u32 start = 0; start < NUM_BBOX_VALUES; ++start)
+  {
+    if (!m_dirty[start])
+      continue;
+
+    u32 end = start + 1;
+    while (end < NUM_BBOX_VALUES && m_dirty[end])
+      ++end;
+
+    for (u32 i = start; i < end; ++i)
+      m_dirty[i] = false;
+
+    Write(start, std::vector<BBoxType>(m_values.begin() + start, m_values.begin() + end));
+  }
 }
 
-u16 GetCoordinate(Coordinate coordinate)
+void BoundingBox::Readback()
 {
-  return s_coordinates[static_cast<u32>(coordinate)];
+  if (!g_ActiveConfig.backend_info.bSupportsBBox)
+    return;
+
+  auto read_values = Read(0, NUM_BBOX_VALUES);
+
+  // Preserve dirty values, that way we don't need to sync.
+  for (u32 i = 0; i < NUM_BBOX_VALUES; i++)
+  {
+    if (!m_dirty[i])
+      m_values[i] = read_values[i];
+  }
+
+  m_is_valid = true;
 }
 
-void SetCoordinate(Coordinate coordinate, u16 value)
+u16 BoundingBox::Get(u32 index)
 {
-  s_coordinates[static_cast<u32>(coordinate)] = value;
+  ASSERT(index < NUM_BBOX_VALUES);
+
+  if (!m_is_valid)
+    Readback();
+
+  return static_cast<u16>(m_values[index]);
 }
 
-void Update(u16 left, u16 right, u16 top, u16 bottom)
+void BoundingBox::Set(u32 index, u16 value)
 {
-  const u16 new_left = std::min(left, GetCoordinate(Coordinate::Left));
-  const u16 new_right = std::max(right, GetCoordinate(Coordinate::Right));
-  const u16 new_top = std::min(top, GetCoordinate(Coordinate::Top));
-  const u16 new_bottom = std::max(bottom, GetCoordinate(Coordinate::Bottom));
-
-  SetCoordinate(Coordinate::Left, new_left);
-  SetCoordinate(Coordinate::Right, new_right);
-  SetCoordinate(Coordinate::Top, new_top);
-  SetCoordinate(Coordinate::Bottom, new_bottom);
+  ASSERT(index < NUM_BBOX_VALUES);
+
+  if (m_is_valid && m_values[index] == value)
+    return;
+
+  m_values[index] = value;
+  m_dirty[index] = true;
 }
 
-void DoState(PointerWrap& p)
+// FIXME: This may not work correctly if we're in the middle of a draw.
+// We should probably ensure that state saves only happen on frame boundaries.
+// Nonetheless, it has been designed to be as safe as possible.
+void BoundingBox::DoState(PointerWrap& p)
 {
-  p.Do(s_is_active);
-  p.DoArray(s_coordinates);
-}
+  p.Do(m_is_active);
+  p.DoArray(m_values);
+  p.DoArray(m_dirty);
+  p.Do(m_is_valid);
+
+  // We handle saving the backend values specially rather than using Readback() and Flush() so that
+  // we don't mess up the current cache state
+  std::vector<BBoxType> backend_values(NUM_BBOX_VALUES);
+  if (p.GetMode() == PointerWrap::MODE_READ)
+  {
+    p.Do(backend_values);
 
-}  // namespace BoundingBox
+    if (g_ActiveConfig.backend_info.bSupportsBBox)
+      Write(0, backend_values);
+  }
+  else
+  {
+    if (g_ActiveConfig.backend_info.bSupportsBBox)
+      backend_values = Read(0, NUM_BBOX_VALUES);
+
+    p.Do(backend_values);
+  }
+}
diff --git a/Source/Core/VideoCommon/BoundingBox.h b/Source/Core/VideoCommon/BoundingBox.h
index caf123a13e..8583986a36 100644
--- a/Source/Core/VideoCommon/BoundingBox.h
+++ b/Source/Core/VideoCommon/BoundingBox.h
@@ -3,40 +3,48 @@
 
 #pragma once
 
+#include <array>
+#include <vector>
+
 #include "Common/CommonTypes.h"
 
 class PointerWrap;
 
-// Bounding Box manager
-namespace BoundingBox
-{
-// Indicates a coordinate of the bounding box.
-enum class Coordinate
+using BBoxType = s32;
+constexpr u32 NUM_BBOX_VALUES = 4;
+
+class BoundingBox
 {
-  Left,    // The X coordinate of the left side of the bounding box.
-  Right,   // The X coordinate of the right side of the bounding box.
-  Top,     // The Y coordinate of the top of the bounding box.
-  Bottom,  // The Y coordinate of the bottom of the bounding box.
-};
+public:
+  explicit BoundingBox() = default;
+  virtual ~BoundingBox() = default;
 
-// Enables bounding box.
-void Enable();
+  bool IsEnabled() const { return m_is_active; }
+  void Enable();
+  void Disable();
 
-// Disables bounding box.
-void Disable();
+  void Flush();
 
-// Determines if bounding box is enabled.
-bool IsEnabled();
+  u16 Get(u32 index);
+  void Set(u32 index, u16 value);
 
-// Gets a particular coordinate for the bounding box.
-u16 GetCoordinate(Coordinate coordinate);
+  void DoState(PointerWrap& p);
 
-// Sets a particular coordinate for the bounding box.
-void SetCoordinate(Coordinate coordinate, u16 value);
+  // Initialize, Read, and Write are only safe to call if the backend supports bounding box,
+  // otherwise unexpected exceptions can occur
+  virtual bool Initialize() = 0;
 
-// Updates all bounding box coordinates.
-void Update(u16 left, u16 right, u16 top, u16 bottom);
+protected:
+  virtual std::vector<BBoxType> Read(u32 index, u32 length) = 0;
+  // TODO: This can likely use std::span once we're on C++20
+  virtual void Write(u32 index, const std::vector<BBoxType>& values) = 0;
 
-// Save state
-void DoState(PointerWrap& p);
-}  // namespace BoundingBox
+private:
+  void Readback();
+
+  bool m_is_active = false;
+
+  std::array<BBoxType, NUM_BBOX_VALUES> m_values = {};
+  std::array<bool, NUM_BBOX_VALUES> m_dirty = {};
+  bool m_is_valid = true;
+};
diff --git a/Source/Core/VideoCommon/PixelEngine.cpp b/Source/Core/VideoCommon/PixelEngine.cpp
index e73cb936e9..87d58eb010 100644
--- a/Source/Core/VideoCommon/PixelEngine.cpp
+++ b/Source/Core/VideoCommon/PixelEngine.cpp
@@ -18,6 +18,7 @@
 #include "VideoCommon/BoundingBox.h"
 #include "VideoCommon/Fifo.h"
 #include "VideoCommon/PerfQueryBase.h"
+#include "VideoCommon/RenderBase.h"
 #include "VideoCommon/VideoBackendBase.h"
 
 namespace PixelEngine
@@ -231,7 +232,7 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
   for (int i = 0; i < 4; ++i)
   {
     mmio->Register(base | (PE_BBOX_LEFT + 2 * i), MMIO::ComplexRead<u16>([i](u32) {
-                     BoundingBox::Disable();
+                     g_renderer->BBoxDisable();
                      return g_video_backend->Video_GetBoundingBox(i);
                    }),
                    MMIO::InvalidWrite<u16>());
diff --git a/Source/Core/VideoCommon/PixelShaderGen.cpp b/Source/Core/VideoCommon/PixelShaderGen.cpp
index 8827c47868..5f5b41e081 100644
--- a/Source/Core/VideoCommon/PixelShaderGen.cpp
+++ b/Source/Core/VideoCommon/PixelShaderGen.cpp
@@ -14,6 +14,7 @@
 #include "VideoCommon/DriverDetails.h"
 #include "VideoCommon/LightingShaderGen.h"
 #include "VideoCommon/NativeVertexFormat.h"
+#include "VideoCommon/RenderBase.h"
 #include "VideoCommon/RenderState.h"
 #include "VideoCommon/VertexLoaderManager.h"
 #include "VideoCommon/VideoCommon.h"
@@ -178,7 +179,7 @@ PixelShaderUid GetPixelShaderUid()
   uid_data->genMode_numindstages = bpmem.genMode.numindstages;
   uid_data->genMode_numtevstages = bpmem.genMode.numtevstages;
   uid_data->genMode_numtexgens = bpmem.genMode.numtexgens;
-  uid_data->bounding_box = g_ActiveConfig.bBBoxEnable && BoundingBox::IsEnabled();
+  uid_data->bounding_box = g_ActiveConfig.bBBoxEnable && g_renderer->IsBBoxEnabled();
   uid_data->rgba6_format =
       bpmem.zcontrol.pixel_format == PixelFormat::RGBA6_Z24 && !g_ActiveConfig.bForceTrueColor;
   uid_data->dither = bpmem.blendmode.dither && uid_data->rgba6_format;
diff --git a/Source/Core/VideoCommon/RenderBase.cpp b/Source/Core/VideoCommon/RenderBase.cpp
index 6771df83a3..45e42e7931 100644
--- a/Source/Core/VideoCommon/RenderBase.cpp
+++ b/Source/Core/VideoCommon/RenderBase.cpp
@@ -57,6 +57,7 @@
 #include "VideoCommon/AbstractTexture.h"
 #include "VideoCommon/BPFunctions.h"
 #include "VideoCommon/BPMemory.h"
+#include "VideoCommon/BoundingBox.h"
 #include "VideoCommon/CPMemory.h"
 #include "VideoCommon/CommandProcessor.h"
 #include "VideoCommon/FPSCounter.h"
@@ -124,6 +125,13 @@ bool Renderer::Initialize()
   if (!m_post_processor->Initialize(m_backbuffer_format))
     return false;
 
+  m_bounding_box = CreateBoundingBox();
+  if (g_ActiveConfig.backend_info.bSupportsBBox && !m_bounding_box->Initialize())
+  {
+    PanicAlertFmt("Failed to initialize bounding box.");
+    return false;
+  }
+
   return true;
 }
 
@@ -137,6 +145,7 @@ void Renderer::Shutdown()
   ShutdownFrameDumping();
   ShutdownImGui();
   m_post_processor.reset();
+  m_bounding_box.reset();
 }
 
 void Renderer::BeginUtilityDrawing()
@@ -184,15 +193,30 @@ void Renderer::ReinterpretPixelData(EFBReinterpretType convtype)
   g_framebuffer_manager->ReinterpretPixelData(convtype);
 }
 
-u16 Renderer::BBoxRead(int index)
+bool Renderer::IsBBoxEnabled() const
+{
+  return m_bounding_box->IsEnabled();
+}
+
+void Renderer::BBoxEnable()
+{
+  m_bounding_box->Enable();
+}
+
+void Renderer::BBoxDisable()
+{
+  m_bounding_box->Disable();
+}
+
+u16 Renderer::BBoxRead(u32 index)
 {
   if (!g_ActiveConfig.bBBoxEnable || !g_ActiveConfig.backend_info.bSupportsBBox)
     return m_bounding_box_fallback[index];
 
-  return BBoxReadImpl(index);
+  return m_bounding_box->Get(index);
 }
 
-void Renderer::BBoxWrite(int index, u16 value)
+void Renderer::BBoxWrite(u32 index, u16 value)
 {
   if (!g_ActiveConfig.bBBoxEnable || !g_ActiveConfig.backend_info.bSupportsBBox)
   {
@@ -200,12 +224,15 @@ void Renderer::BBoxWrite(int index, u16 value)
     return;
   }
 
-  BBoxWriteImpl(index, value);
+  m_bounding_box->Set(index, value);
 }
 
 void Renderer::BBoxFlush()
 {
-  BBoxFlushImpl();
+  if (!g_ActiveConfig.bBBoxEnable || !g_ActiveConfig.backend_info.bSupportsBBox)
+    return;
+
+  m_bounding_box->Flush();
 }
 
 u32 Renderer::AccessEFB(EFBAccessType type, u32 x, u32 y, u32 poke_data)
@@ -1761,6 +1788,8 @@ void Renderer::DoState(PointerWrap& p)
   p.Do(m_last_xfb_height);
   p.DoArray(m_bounding_box_fallback);
 
+  m_bounding_box->DoState(p);
+
   if (p.GetMode() == PointerWrap::MODE_READ)
   {
     // Force the next xfb to be displayed.
diff --git a/Source/Core/VideoCommon/RenderBase.h b/Source/Core/VideoCommon/RenderBase.h
index ddc86cd3b3..380225b133 100644
--- a/Source/Core/VideoCommon/RenderBase.h
+++ b/Source/Core/VideoCommon/RenderBase.h
@@ -38,6 +38,7 @@ class AbstractPipeline;
 class AbstractShader;
 class AbstractTexture;
 class AbstractStagingTexture;
+class BoundingBox;
 class NativeVertexFormat;
 class NetPlayChatUI;
 class PointerWrap;
@@ -213,8 +214,11 @@ public:
   virtual u32 AccessEFB(EFBAccessType type, u32 x, u32 y, u32 poke_data);
   virtual void PokeEFB(EFBAccessType type, const EfbPokeData* points, size_t num_points);
 
-  u16 BBoxRead(int index);
-  void BBoxWrite(int index, u16 value);
+  bool IsBBoxEnabled() const;
+  void BBoxEnable();
+  void BBoxDisable();
+  u16 BBoxRead(u32 index);
+  void BBoxWrite(u32 index, u16 value);
   void BBoxFlush();
 
   virtual void Flush() {}
@@ -303,9 +307,7 @@ protected:
   // Should be called with the ImGui lock held.
   void DrawImGui();
 
-  virtual u16 BBoxReadImpl(int index) = 0;
-  virtual void BBoxWriteImpl(int index, u16 value) = 0;
-  virtual void BBoxFlushImpl() {}
+  virtual std::unique_ptr<BoundingBox> CreateBoundingBox() const = 0;
 
   AbstractFramebuffer* m_current_framebuffer = nullptr;
   const AbstractPipeline* m_current_pipeline = nullptr;
@@ -396,6 +398,8 @@ private:
   u32 m_last_xfb_stride = 0;
   u32 m_last_xfb_height = 0;
 
+  std::unique_ptr<BoundingBox> m_bounding_box;
+
   // Nintendo's SDK seems to write "default" bounding box values before every draw (1023 0 1023 0
   // are the only values encountered so far, which happen to be the extents allowed by the BP
   // registers) to reset the registers for comparison in the pixel engine, and presumably to detect
diff --git a/Source/Core/VideoCommon/VertexManagerBase.cpp b/Source/Core/VideoCommon/VertexManagerBase.cpp
index f78ec9fca2..73ab9af3c2 100644
--- a/Source/Core/VideoCommon/VertexManagerBase.cpp
+++ b/Source/Core/VideoCommon/VertexManagerBase.cpp
@@ -269,7 +269,7 @@ void VertexManagerBase::CommitBuffer(u32 num_vertices, u32 vertex_stride, u32 nu
 void VertexManagerBase::DrawCurrentBatch(u32 base_index, u32 num_indices, u32 base_vertex)
 {
   // If bounding box is enabled, we need to flush any changes first, then invalidate what we have.
-  if (BoundingBox::IsEnabled() && g_ActiveConfig.bBBoxEnable &&
+  if (g_renderer->IsBBoxEnabled() && g_ActiveConfig.bBBoxEnable &&
       g_ActiveConfig.backend_info.bSupportsBBox)
   {
     g_renderer->BBoxFlush();
diff --git a/Source/Core/VideoCommon/VideoState.cpp b/Source/Core/VideoCommon/VideoState.cpp
index bef1afe888..f954ca7c30 100644
--- a/Source/Core/VideoCommon/VideoState.cpp
+++ b/Source/Core/VideoCommon/VideoState.cpp
@@ -5,7 +5,6 @@
 
 #include "Common/ChunkFile.h"
 #include "VideoCommon/BPMemory.h"
-#include "VideoCommon/BoundingBox.h"
 #include "VideoCommon/CPMemory.h"
 #include "VideoCommon/CommandProcessor.h"
 #include "VideoCommon/Fifo.h"
@@ -71,9 +70,6 @@ void VideoCommon_DoState(PointerWrap& p)
   g_vertex_manager->DoState(p);
   p.DoMarker("VertexManager");
 
-  BoundingBox::DoState(p);
-  p.DoMarker("BoundingBox");
-
   g_framebuffer_manager->DoState(p);
   p.DoMarker("FramebufferManager");