VideoCommon: Abstract bounding box

[Techjar]

This moves much of the duplicated bounding box code into VideoCommon, leaving only the specific buffer implementations in each backend.

I also accidentally fixed the bug where bounding box doesn't work on software renderer unless you select some other backend first.

[Techjar]

Nope, it's a non-existent file. Must've been leftover accidentally from some past refactor.

[iwubcode]

This looks really good @Techjar , thanks for doing this!!

One silly comment, thoughts on renaming VideoCommon/BoundingBox to VideoCommon/AbstractBoundingBox to match the other classes?

[iwubcode]

This check happens in every backend (save for D3D12, Software, and Null). Should it be moved to the abstract interface? Vulkan supports a log message which might be nice in general.

Additionally, why is this returning true? If it's not supported, seems like we would want to error out or possibly return a NullBoundingBox to avoid errors when it is being used.

[iwubcode]

Read/Write also check this same config option (but not in all backends).

[Techjar]

Returning false would cause us to abort backend initialization, which we don't want because games can still run without bounding box support. There's checks in place to avoid errors when things aren't initialized.

[iwubcode]

Ah ok. I see what you mean now. Well, let me amend my suggestion then to modifying Read in AbstractBoundingBox to:

std::vector<BBoxType> Read(int index, int length)
{
  if (!g_ActiveConfig.backend_info.bSupportsBBox)
    return {};

  return ReadImpl(index, length);
}

with a

private:
virtual std::vector<BBoxType> ReadImpl(int index, int length) = 0;

and same with Write() so that all backends get this behavior in a single place.

And I'd add that check to D3D12's Initialize() for consistency with D3D11, OpenGL, and Vulkan.

[Techjar]

I already addressed it in a slightly different way, but thanks. Didn't want to create forwarding functions, as the implementation here is pretty tight anyways.

D3D12 implicitly supports bounding box, so the check would be kinda pointless there. In any case I moved the initialization check to RenderBase.

[Techjar]

One silly comment, thoughts on renaming VideoCommon/BoundingBox to VideoCommon/AbstractBoundingBox to match the other classes?

I dunno. The naming of other things isn't entirely consistent either, something being AbstractX and others being XBase.

[Rumi-Larry]

One silly comment, thoughts on renaming VideoCommon/BoundingBox to VideoCommon/AbstractBoundingBox to match the other classes?

I dunno. The naming of other things isn't entirely consistent either, something being AbstractX and others being XBase.

My personal preference would be XBase being consistently used. Any piece of code is already an abstraction :P

[iwubcode]

Any piece of code is already an abstraction

That's...not what this is referring to at all, see https://www.tutorialspoint.com/cplusplus/cpp_interfaces.htm

I dunno. The naming of other things isn't entirely consistent either, something being AbstractX and others being XBase.

Well "Base" does not technically imply "Abstract" but I do understand what you're saying. Honestly, not a big deal to leave it either, the backend specific abbreviations are prepended onto the derived classes, so the distinction is there.

I'll take another pass later today but this code seemed mergeable to me.

[JMC47]

Is there anything left for this?

[iwubcode]

LGTM. Untested

[leoetlino]

Reviewed 41 of 41 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Techjar)

---

Source/Core/VideoBackends/Null/NullBoundingBox.h, line 2 at r1 (raw file):

// Copyright 2021 Dolphin Emulator Project
// Licensed under GPLv2+

should be // SPDX-License-Identifier: GPL-2.0-or-later

---

Source/Core/VideoBackends/Software/SWBoundingBox.h, line 2 at r1 (raw file):

// Copyright 2021 Dolphin Emulator Project
// Licensed under GPLv2+

license header

---

Source/Core/VideoBackends/Software/SWBoundingBox.cpp, line 2 at r1 (raw file):

// Copyright 2021 Dolphin Emulator Project
// Licensed under GPLv2+

license header

---

Source/Core/VideoCommon/BoundingBox.h, line 47 at r1 (raw file):

  bool m_is_active = false;

  std::array<BBoxType, NUM_BBOX_VALUES> m_values = {};

Shouldn't this default to {0x80, 0xA0, ...}?

---

Source/Core/VideoCommon/BoundingBox.cpp, line 43 at r1 (raw file):

      continue;
    }

I feel like it would be clearer to separate the m_dirty updates and the end index search:

u32 end = start + 1;
while (end < NUM_BBOX_VALUES && m_dirty[end])
  ++end;

for (u32 i = start; i < end; ++i)
  m_dirty[i] = false;

Having two for loops, one missing the iteration expression and the other missing an initialiser expression, is kind of difficult to read.

[Techjar]

Shouldn't this default to {0x80, 0xA0, ...}?

Is this the default value of the registers on hardware? Honestly I don't even know what these values are, I only see them in the software bbox implementation.

[leoetlino]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Techjar)