VideoCommon: Rework scissor handling

This increases accuracy, fixing the white rendering in Major Minor's Majestic March.  However, the hardware backends can only have one viewport and scissor rectangle at a time, while sometimes multiple are needed to accurately emulate what is happening.  If possible, this will need to be fixed later.

Source/Core/VideoBackends/OGL/OGLRender.cpp

@@ -958,7 +958,7 @@ void Renderer::ClearScreen(const MathUtil::Rectangle<int>& rc, bool colorEnable,
     glDepthMask(m_current_depth_state.updateenable);
 
   // Scissor rect must be restored.
-  BPFunctions::SetScissor();
+  BPFunctions::SetScissorAndViewport();
 }
 
 void Renderer::RenderXFBToScreen(const MathUtil::Rectangle<int>& target_rc,

Source/Core/VideoCommon/BPFunctions.cpp

@@ -7,6 +7,7 @@
 #include <cmath>
 #include <string_view>
 
+#include "Common/Assert.h"
 #include "Common/CommonTypes.h"
 #include "Common/Logging/Log.h"
 
@@ -37,48 +38,172 @@ void SetGenerationMode()
   g_vertex_manager->SetRasterizationStateChanged();
 }
 
-void SetScissor()
+int ScissorRect::GetArea() const
 {
-  /* NOTE: the minimum value here for the scissor rect is -342.
-   * GX SDK functions internally add an offset of 342 to scissor coords to
-   * ensure that the register was always unsigned.
-   *
-   * The code that was here before tried to "undo" this offset, but
-   * since we always take the difference, the +342 added to both
-   * sides cancels out. */
-
-  /* NOTE: With a positive scissor offset, the scissor rect is shifted left and/or up;
-   * With a negative scissor offset, the scissor rect is shifted right and/or down.
-   *
-   * GX SDK functions internally add an offset of 342 to scissor offset.
-   * The scissor offset is always even, so to save space, the scissor offset register
-   * is scaled down by 2. So, if somebody calls GX_SetScissorBoxOffset(20, 20);
-   * the registers will be set to ((20 + 342) / 2 = 181, 181).
-   *
-   * The scissor offset register is 10bit signed [-512, 511].
-   * e.g. In Super Mario Galaxy 1 and 2, during the "Boss roar effect",
-   * for a scissor offset of (0, -464), the scissor offset register will be set to
-   * (171, (-464 + 342) / 2 = -61).
-   */
-  s32 xoff = bpmem.scissorOffset.x * 2;
-  s32 yoff = bpmem.scissorOffset.y * 2;
+  return rect.GetWidth() * rect.GetHeight();
+}
+
+int ScissorResult::GetViewportArea(const ScissorRect& rect) const
+{
+  int x0 = std::clamp<int>(rect.rect.left + rect.x_off, viewport_left, viewport_right);
+  int x1 = std::clamp<int>(rect.rect.right + rect.x_off, viewport_left, viewport_right);
+
+  int y0 = std::clamp<int>(rect.rect.top + rect.y_off, viewport_top, viewport_bottom);
+  int y1 = std::clamp<int>(rect.rect.bottom + rect.y_off, viewport_top, viewport_bottom);
+
+  return (x1 - x0) * (y1 - y0);
+}
+
+// Compare so that a sorted collection of rectangles has the best one last, so that if they're drawn
+// in order, the best one is the one that is drawn last (and thus over the rest).
+// The exact iteration order on hardware hasn't been tested, but silly things can happen where a
+// polygon can intersect with itself; this only applies outside of the viewport region (in areas
+// that would normally be affected by clipping).  No game is known to care about this.
+bool ScissorResult::IsWorse(const ScissorRect& lhs, const ScissorRect& rhs) const
+{
+  // First, penalize any rect that is not in the viewport
+  int lhs_area = GetViewportArea(lhs);
+  int rhs_area = GetViewportArea(rhs);
+
+  if (lhs_area != rhs_area)
+    return lhs_area < rhs_area;
+
+  // Now compare on total areas, without regard for the viewport
+  return lhs.GetArea() < rhs.GetArea();
+}
+
+namespace
+{
+// Dynamically sized small array of ScissorRanges (used as an heap-less alternative to std::vector
+// to reduce allocation overhead)
+struct RangeList
+{
+  static constexpr u32 MAX_RANGES = 9;
 
-  MathUtil::Rectangle<int> native_rc(bpmem.scissorTL.x - xoff, bpmem.scissorTL.y - yoff,
-                                     bpmem.scissorBR.x - xoff + 1, bpmem.scissorBR.y - yoff + 1);
-  native_rc.ClampUL(0, 0, EFB_WIDTH, EFB_HEIGHT);
+  u32 m_num_ranges = 0;
+  std::array<ScissorRange, MAX_RANGES> m_ranges{};
 
-  auto target_rc = g_renderer->ConvertEFBRectangle(native_rc);
+  void AddRange(int offset, int start, int end)
+  {
+    DEBUG_ASSERT(m_num_ranges < MAX_RANGES);
+    m_ranges[m_num_ranges] = ScissorRange(offset, start, end);
+    m_num_ranges++;
+  }
+  auto begin() const { return m_ranges.begin(); }
+  auto end() const { return m_ranges.begin() + m_num_ranges; }
+
+  u32 size() { return m_num_ranges; }
+};
+
+static RangeList ComputeScissorRanges(int start, int end, int offset, int efb_dim)
+{
+  RangeList ranges;
+
+  for (int extra_off = -4096; extra_off <= 4096; extra_off += 1024)
+  {
+    int new_off = offset + extra_off;
+    int new_start = std::clamp(start - new_off, 0, efb_dim);
+    int new_end = std::clamp(end - new_off + 1, 0, efb_dim);
+    if (new_start < new_end)
+    {
+      ranges.AddRange(new_off, new_start, new_end);
+    }
+  }
+
+  return ranges;
+}
+}  // namespace
+
+ScissorResult::ScissorResult(const BPMemory& bpmemory, const XFMemory& xfmemory)
+    : ScissorResult(bpmemory,
+                    std::minmax(xfmemory.viewport.xOrig - xfmemory.viewport.wd,
+                                xfmemory.viewport.xOrig + xfmemory.viewport.wd),
+                    std::minmax(xfmemory.viewport.yOrig - xfmemory.viewport.ht,
+                                xfmemory.viewport.yOrig + xfmemory.viewport.ht))
+{
+}
+ScissorResult::ScissorResult(const BPMemory& bpmemory, std::pair<float, float> viewport_x,
+                             std::pair<float, float> viewport_y)
+    : scissor_tl{.hex = bpmemory.scissorTL.hex}, scissor_br{.hex = bpmemory.scissorBR.hex},
+      scissor_off{.hex = bpmemory.scissorOffset.hex}, viewport_left(viewport_x.first),
+      viewport_right(viewport_x.second), viewport_top(viewport_y.first),
+      viewport_bottom(viewport_y.second)
+{
+  // Range is [left, right] and [top, bottom] (closed intervals)
+  const int left = scissor_tl.x;
+  const int right = scissor_br.x;
+  const int top = scissor_tl.y;
+  const int bottom = scissor_br.y;
+  // When left > right or top > bottom, nothing renders (even with wrapping from the offsets)
+  if (left > right || top > bottom)
+    return;
+
+  // Note that both the offsets and the coordinates have 342 added to them internally by GX
+  // functions (for the offsets, this is before they are divided by 2/right shifted). This code
+  // could undo both sets of offsets, but it doesn't need to since they cancel out when subtracting
+  // (and those offsets actually matter for the left > right and top > bottom checks).
+  const int x_off = scissor_off.x << 1;
+  const int y_off = scissor_off.y << 1;
+
+  RangeList x_ranges = ComputeScissorRanges(left, right, x_off, EFB_WIDTH);
+  RangeList y_ranges = ComputeScissorRanges(top, bottom, y_off, EFB_HEIGHT);
+
+  m_result.reserve(x_ranges.size() * y_ranges.size());
+
+  // Now we need to form actual rectangles from the x and y ranges,
+  // which is a simple Cartesian product of x_ranges_clamped and y_ranges_clamped.
+  // Each rectangle is also a Cartesian product of x_range and y_range, with
+  // the rectangles being half-open (of the form [x0, x1) X [y0, y1)).
+  for (const auto& x_range : x_ranges)
+  {
+    DEBUG_ASSERT(x_range.start < x_range.end);
+    DEBUG_ASSERT(x_range.end <= EFB_WIDTH);
+    for (const auto& y_range : y_ranges)
+    {
+      DEBUG_ASSERT(y_range.start < y_range.end);
+      DEBUG_ASSERT(y_range.end <= EFB_HEIGHT);
+      m_result.emplace_back(x_range, y_range);
+    }
+  }
+
+  auto cmp = [&](const ScissorRect& lhs, const ScissorRect& rhs) { return IsWorse(lhs, rhs); };
+  std::sort(m_result.begin(), m_result.end(), cmp);
+}
+
+ScissorRect ScissorResult::Best() const
+{
+  // For now, simply choose the best rectangle (see ScissorResult::IsWorse).
+  // This does mean we calculate all rectangles and only choose one, which is not optimal, but this
+  // is called infrequently.  Eventually, all backends will support multiple scissor rects.
+  if (!m_result.empty())
+  {
+    return m_result.back();
+  }
+  else
+  {
+    // But if we have no rectangles, use a bogus one that's out of bounds.
+    // Ideally, all backends will support multiple scissor rects, in which case this won't be
+    // needed.
+    return ScissorRect(ScissorRange{0, 1000, 1001}, ScissorRange{0, 1000, 1001});
+  }
+}
+
+ScissorResult ComputeScissorRects()
+{
+  return ScissorResult{bpmem, xfmem};
+}
+
+void SetScissorAndViewport()
+{
+  auto native_rc = ComputeScissorRects().Best();
+
+  auto target_rc = g_renderer->ConvertEFBRectangle(native_rc.rect);
   auto converted_rc =
       g_renderer->ConvertFramebufferRectangle(target_rc, g_renderer->GetCurrentFramebuffer());
   g_renderer->SetScissorRect(converted_rc);
-}
 
-void SetViewport()
-{
-  const s32 xoff = bpmem.scissorOffset.x * 2;
-  const s32 yoff = bpmem.scissorOffset.y * 2;
-  float raw_x = xfmem.viewport.xOrig - xfmem.viewport.wd - xoff;
-  float raw_y = xfmem.viewport.yOrig + xfmem.viewport.ht - yoff;
+  float raw_x = (xfmem.viewport.xOrig - native_rc.x_off) - xfmem.viewport.wd;
+  float raw_y = (xfmem.viewport.yOrig - native_rc.y_off) + xfmem.viewport.ht;
   float raw_width = 2.0f * xfmem.viewport.wd;
   float raw_height = -2.0f * xfmem.viewport.ht;
   if (g_ActiveConfig.UseVertexRounding())

Source/Core/VideoCommon/BPFunctions.h

@@ -7,16 +7,131 @@
 
 #pragma once
 
-#include "Common/MathUtil.h"
+#include <utility>
+#include <vector>
 
-struct BPCmd;
+#include "Common/MathUtil.h"
+#include "VideoCommon/BPMemory.h"
+struct XFMemory;
 
 namespace BPFunctions
 {
+struct ScissorRange
+{
+  constexpr ScissorRange() = default;
+  constexpr ScissorRange(int offset, int start, int end) : offset(offset), start(start), end(end) {}
+  int offset = 0;
+  int start = 0;
+  int end = 0;
+};
+
+struct ScissorRect
+{
+  constexpr ScissorRect(ScissorRange x_range, ScissorRange y_range)
+      :  // Rectangle ctor takes x0, y0, x1, y1.
+        rect(x_range.start, y_range.start, x_range.end, y_range.end), x_off(x_range.offset),
+        y_off(y_range.offset)
+  {
+  }
+
+  MathUtil::Rectangle<int> rect;
+  int x_off;
+  int y_off;
+
+  int GetArea() const;
+};
+
+// Although the GameCube/Wii have only one scissor configuration and only one viewport
+// configuration, some values can result in multiple parts of the screen being updated.
+// This can happen if the scissor offset combined with the bottom or right coordinate ends up
+// exceeding 1024; then, both sides of the screen will be drawn to, while the middle is not.
+// Major Minor's Majestic March causes this to happen during loading screens and other scrolling
+// effects, though it draws on top of one of them.
+// This can also happen if the scissor rectangle is particularly large, but this will usually
+// involve drawing content outside of the viewport, which Dolphin does not currently handle.
+//
+// The hardware backends can currently only use one viewport and scissor rectangle, so we need to
+// pick the "best" rectangle based on how much of the viewport would be rendered to the screen.
+// If we choose the wrong one, then content might not actually show up when the game is expecting it
+// to.  This does happen on Major Minor's Majestic March for the final few frames of the horizontal
+// scrolling animation, but it isn't that important.  Note that the assumption that a "best"
+// rectangle exists is based on games only wanting to draw one rectangle, and accidentally
+// configuring the scissor offset and size of the scissor rectangle such that multiple show up;
+// there are no known games where this is not the case.
+struct ScissorResult
+{
+  ScissorResult(const BPMemory& bpmem, const XFMemory& xfmem);
+  ~ScissorResult() = default;
+  ScissorResult(const ScissorResult& other)
+      : scissor_tl{.hex = other.scissor_tl.hex}, scissor_br{.hex = other.scissor_br.hex},
+        scissor_off{.hex = other.scissor_off.hex}, viewport_left{other.viewport_left},
+        viewport_right{other.viewport_right}, viewport_top{other.viewport_top},
+        viewport_bottom{other.viewport_bottom}, m_result{other.m_result}
+  {
+  }
+  ScissorResult& operator=(const ScissorResult& other)
+  {
+    if (this == &other)
+      return *this;
+    scissor_tl.hex = other.scissor_tl.hex;
+    scissor_br.hex = other.scissor_br.hex;
+    scissor_off.hex = other.scissor_off.hex;
+    viewport_left = other.viewport_left;
+    viewport_right = other.viewport_right;
+    viewport_top = other.viewport_top;
+    viewport_bottom = other.viewport_bottom;
+    m_result = other.m_result;
+    return *this;
+  }
+  ScissorResult(ScissorResult&& other)
+      : scissor_tl{.hex = other.scissor_tl.hex}, scissor_br{.hex = other.scissor_br.hex},
+        scissor_off{.hex = other.scissor_off.hex}, viewport_left{other.viewport_left},
+        viewport_right{other.viewport_right}, viewport_top{other.viewport_top},
+        viewport_bottom{other.viewport_bottom}, m_result{std::move(other.m_result)}
+  {
+  }
+  ScissorResult& operator=(ScissorResult&& other)
+  {
+    if (this == &other)
+      return *this;
+    scissor_tl.hex = other.scissor_tl.hex;
+    scissor_br.hex = other.scissor_br.hex;
+    scissor_off.hex = other.scissor_off.hex;
+    viewport_left = other.viewport_left;
+    viewport_right = other.viewport_right;
+    viewport_top = other.viewport_top;
+    viewport_bottom = other.viewport_bottom;
+    m_result = std::move(other.m_result);
+    return *this;
+  }
+
+  // Input values, for use in statistics
+  ScissorPos scissor_tl;
+  ScissorPos scissor_br;
+  ScissorOffset scissor_off;
+  float viewport_left;
+  float viewport_right;
+  float viewport_top;
+  float viewport_bottom;
+
+  // Actual result
+  std::vector<ScissorRect> m_result;
+
+  ScissorRect Best() const;
+
+private:
+  ScissorResult(const BPMemory& bpmem, std::pair<float, float> viewport_x,
+                std::pair<float, float> viewport_y);
+
+  int GetViewportArea(const ScissorRect& rect) const;
+  bool IsWorse(const ScissorRect& lhs, const ScissorRect& rhs) const;
+};
+
+ScissorResult ComputeScissorRects();
+
 void FlushPipeline();
 void SetGenerationMode();
-void SetScissor();
-void SetViewport();
+void SetScissorAndViewport();
 void SetDepthMode();
 void SetBlendMode();
 void ClearScreen(const MathUtil::Rectangle<int>& rc);

Source/Core/VideoCommon/BPStructs.cpp

@@ -131,8 +131,6 @@ static void BPWritten(const BPCmd& bp, int cycles_into_future)
   case BPMEM_SCISSORTL:      // Scissor Rectable Top, Left
   case BPMEM_SCISSORBR:      // Scissor Rectable Bottom, Right
   case BPMEM_SCISSOROFFSET:  // Scissor Offset
-    SetScissor();
-    SetViewport();
     VertexShaderManager::SetViewportChanged();
     GeometryShaderManager::SetViewportChanged();
     return;
@@ -1272,8 +1270,7 @@ void BPReload()
   // let's not risk actually replaying any writes.
   // note that PixelShaderManager is already covered since it has its own DoState.
   SetGenerationMode();
-  SetScissor();
-  SetViewport();
+  SetScissorAndViewport();
   SetDepthMode();
   SetBlendMode();
   OnPixelFormatChange();

Source/Core/VideoCommon/RenderBase.cpp

@@ -160,8 +160,7 @@ void Renderer::EndUtilityDrawing()
 {
   // Reset framebuffer/scissor/viewport. Pipeline will be reset at next draw.
   g_framebuffer_manager->BindEFBFramebuffer();
-  BPFunctions::SetScissor();
-  BPFunctions::SetViewport();
+  BPFunctions::SetScissorAndViewport();
 }
 
 void Renderer::SetFramebuffer(AbstractFramebuffer* framebuffer)
@@ -543,8 +542,7 @@ void Renderer::CheckForConfigChanges()
   // Viewport and scissor rect have to be reset since they will be scaled differently.
   if (changed_bits & CONFIG_CHANGE_BIT_TARGET_SIZE)
   {
-    BPFunctions::SetViewport();
-    BPFunctions::SetScissor();
+    BPFunctions::SetScissorAndViewport();
   }
 
   // Stereo mode change requires recompiling our post processing pipeline and imgui pipelines for