Rework scissor handling

[Pokechu22]

This fixes the rendering issue in Major Minor's Majestic March.

There are some situations where the emulated scissor configuration results in multiple real scissors being needed. I couldn't find an easy way to implement that on the hardware backends, but I did implement it in software. For the hardware backends, I pick the biggest one. This affects Major Minor, but only briefly during scrolling loading screens (right near the end and only for ~4 frames). Compare frames 204 and 205 in major_minor_loading_long.dff. Here's what's drawn behind the loading text on those same frames (this is via the software renderer): 204 and 205.

Here's my hardware test, and a video of that test on hardware (this is probably something that can be changed into something for the hwtests repo, but I haven't gotten to that yet). The generated test results are also in that gist.

The software renderer generates mostly matching results for Scissor_info_vo.log - the colors don't match exactly, but it finds the exact same rectangles. The hardware renderers give accurate results, but only have on rectangle. Neither of them gives completely accurate results for Scissor_info_all.log, but that's more because of weird behavior with clipping, not with the scissor. The hardware renderers reject anything outside of the viewport, while the software renderer now sometimes draws things outside of the viewport (based on the xfmem configuration); this fixes the nintendo channel lettering on the software renderer. However, there seems to be additional oddities with clipping that haven't been fixed yet (even when clipping is enabled on hardware, stuff outside of the viewport can exist, and even with clipping disabled you only are allowed to go so far outside of the viewport). I don't currently plan on investigating clipping further at this time.

I also created an imgui-based tool to visualized the scissor rectangles, and have created some videos based on it. "Old" and "current" refer to behavior before and after PR #9660, "nooff" refers to having the scissor offset function completely disabled, and "new" is this PR.

Title screen (major_minor_title_long.dff)

title_old.mp4

title_current.mp4

title_nooff.mp4

title_new2.mp4

Loading screen (major_minor_loading_long.dff)

loading_old.mp4

loading_current.mp4

loading_nooff.mp4

loading_new2.mp4

Retry screen (major_minor_retry_long.dff)

retry_old.mp4

retry_current.mp4

retry_nooff.mp4

retry_new2.mp4

Super Mario Galaxy boss roars

Old:

Current:

No off (+ second screenshot with some objects disabled to see what's going on better):

New:

I also have some notes here, but these probably aren't worth reading, as they're messy and currently unfinished. An un-squashed version of this code (which also includes hack I used to framedump with imgui included) is here.

[JMC47]

Holy shit. I'd usually write something more on topic, but the level of depth and difficulty of this problem was astounding. Kudos to finally fixing it.

Does this per-chance affect that other game that rendered black over the screen? Or did that end up being separate?

[Pokechu22]

Looking more closely at the fifoci results above, different parts of this caused different changes. It's worth noting that several of the software renderer fixes come from the "fix scissor rectangle always being block-aligned" change; this was a bug in the software renderer that was not present in the hardware backends and anything that was fixed by that should have already worked outside of the software renderer.

The issue effectively was like this code existing (except this would affect all backends):

diff --git a/Source/Core/VideoCommon/BPStructs.cpp b/Source/Core/VideoCommon/BPStructs.cpp
index ec5ab6b348..4317be33dd 100644
--- a/Source/Core/VideoCommon/BPStructs.cpp
+++ b/Source/Core/VideoCommon/BPStructs.cpp
@@ -131,6 +131,10 @@ static void BPWritten(const BPCmd& bp)
   case BPMEM_SCISSORTL:      // Scissor Rectable Top, Left
   case BPMEM_SCISSORBR:      // Scissor Rectable Bottom, Right
   case BPMEM_SCISSOROFFSET:  // Scissor Offset
+    bpmem.scissorTL.x = bpmem.scissorTL.x & ~1;
+    bpmem.scissorTL.y = bpmem.scissorTL.y & ~1;
+    bpmem.scissorBR.x = bpmem.scissorBR.x | 1;
+    bpmem.scissorBR.y = bpmem.scissorBR.y | 1;
     VertexShaderManager::SetViewportChanged();
     GeometryShaderManager::SetViewportChanged();
     return;

Here are the individual differences and an explanation for why they changed:

• dbz-depth: The game uses a scissor of [0, 639) X [0, 447) with an offset of 0 for the start of the frame, and changed that scissor to [0, 640) X [0, 448) in object 313 (the start of a bloom effect). The first scissor causes black borders on the bottom and right of the screen; the black borders bleed inward from the bloom later on. The hardware renderers already handle this. I think this is probably a game bug and not a deliberate effect.

• jb-shadow: When drawing shadows, a scissor of [1, 127) X [1, 127) with offset 0 is used, and then an EFB copy of [0, 128) X [0, 128) is made. When the shadows are drawn, the texture from that EFB copy is set to clamp, so the pixels on the border get drawn for anything out of bounds. Thus those pixels should be transparent; the scissor is used to achieve this. The hardware renderers already handle this. Compare before and after (where red is originally transparent, and black is originally white).

• nfsu-reflections: A viewport of [343, 407) X [343, 407) is used (normally the viewport has a top-left of (342, 342)). The scissor is adjusted accordingly to use [343, 405) X [343, 405) (with raw coordinates). This would trigger the block-alignment issue. No scissor offset is used, though. I don't exactly know what's going on with rendering here, but the difference is probably caused by this. The hardware renderers already handled it correctly. This fifolog is somewhat broken visually already, and probably the game needs to be tested running normally (though that will be annoying when using the software renderer).

• nhl-slap: A stupid number of different scissor rectangle configurations are used here, 34 total (33 of which are for the UI; the game itself only uses one which covers the whole screen). Number 17 is relevant here, using [464, 343) X [598, 389). The block alignment issue meant that one extra column of pixels was rendered on the grey highlight behind "P. Marleau". The hardware renderers already handled this correctly.

• sms-water: D.E.B.S. uses a scissor of [45, 390) X [499, 415), which means the block alignment issue would affect it. Hardware was unaffected. I was unable to reproduce the change to the water on my machine. However, some parts of the scene do have clipping disabled, including object 350 (which comes right after the water renders).

• sw3-dt: The bloom effect draws a black rectangle using a scissor of [0, 82) X [0, 58). It then uses a scissor of [1, 81) X [1, 57) when drawing a small copy of the screen. In both cases, no scissor offset is used. Finally, it makes an EFB copy of the screen region [0, 82) X [0, 58), meaning a black border is included. This is what produces the vignette. Unlike dbz, this seems to be a deliberate choice. The hardware renderers already handled this correctly, but the fifolog also experiences vertex explosions after the first frame, so this should be tested by running the game normally too.

• nintendo-channel is fixed because of clipdisable.

• burnout2-vehicletextures: Very slight change, apparently introduced by "Use new scissor logic" - I think it might be floating-point rounding stuff since the scissor offset is no longer subtracted from vertex coordinates. I'm not entirely sure though.

• rs3-bumpmapping: This was affected by both "Use new scissor logic" and the clipdisable change. I think this is also a floating-point rounding difference. I'm also not sure about this one.

• djhero2-blend: This was caused by the clipdisable change. I think it's an integer overflow, as the floor triangles are fairly big: image and freelook image. This is a regression, but I think more investigation into clipping would be needed to fix it, so I don't know if it needs to be dealt with just yet.

This does not fix Gormiti: The Lords of Nature (the game that has the black screen), but I didn't expect it to either. And, smg-roar (which uses the scissor offset) continues to render correctly (which is good).

[Pokechu22]

After a bit of investigating, I've figured out why djhero2-blend is broken by the clipdisable change. Here are 3 relevant input vertices:

x	y	z
0	-32765	0
10451	-32765	-3170
10711	-32765	-2130

Their projected positions become these:

x	y	z	w
-1393.6	318.1	10.0	-22.7
143.8	97.8	10.1	331.6
136.5	-105.5	-10.1	451.1

Which then becomes this after the perspective division:

x	y	z
20269.4	-2618.1	24150130
800.8	637.3	16265513
758.8	623.3	16399608

As explained here, negative w is problematic for clipping; you'd expect a negative x to remain off-screen in the negative direction, but now it's a large positive value (and y is negative, and z is bigger than it should be). For now I think the easiest workaround is to still do clipping if any w value is negative, until further hardware testing explains how hardware actually works when disable_clipping_detection is used.

[neobrain]

Could we disable scissoring on the host altogether and instead implement it manually in the pixel shaders instead to fully support this case?

[Pokechu22]

I don't think so. Masking two different sides of the screen could be done using a pixel shader pretty easily, but I don't think moving things to different sides is possible in just a pixel shader.

Most graphics APIs do support multiple viewports and scissors internally, though, so I think it might be possible to implement it with that. I just haven't looked into the details.

I added some additional images with the software renderer where the loading text is removed which might help visualize it: Frame 204, ImGui and SW; Frame 205, ImGui and SW.

[neobrain]

Hmm not sure I'm following. Do I understand you correctly there are two components that to such scissor "overflows"?

1. Pixels on the very left and on the very right are rendered, leaving a big gap in the middle
2. The left subarea is not a true scissor rect, but instead content that ends up on the left subarea is rendered as if it had been rendered to the right of the right subarea, i.e. that content wraps around

Reading the comment it was easy to see 1 being the case, but 2 comes at a surprise. Granted, I may have glanced over the comment too quickly. If I didn't just miss it, I think it's a detail worth mentioning though.

[Pokechu22]

I think I just haven't explained things very well (I didn't do a writeup beforehand, other than my messy notes); it definitely could be clearer.

Both 1 and 2 are correct. But I think there might be a clearer explanation.

The scissor rectangle itself doesn't overflow. You can give it any set of values x0, y0, x1, and y1 all in the range [0, 2047] and things will render correctly: if x0 > x1 or y0 > y1, nothing shows up; otherwise, pixels between those ranges will be rasterized. This is after the viewport has been applied, so normally you want the viewport and scissor to match (and both the scissor and viewport have 342 added to their origins by GX functions). (The viewport is also based on floats, and uses a center+radius format, which is a bit confusing, but that can be ignored here). For instance, a 640 by 480 viewport and scissor would have the scissor coordinates 342, 342, 981, 821, and a viewport for 342, 342, 982, 822.

You can still increase the size of the scissor, and things behave normally for any size (even 0, 0, 2047, 2047), except that you'll start getting stuff that's outside of the viewport and instead in the clipper guardband. I suspect that if you change the viewport size to something larger, everything will behave normally, but in my testing I always used the same viewport.

The scissor offset is then used to choose where rasterized pixels end up in the EFB. Usually, both x_off and y_off set to 171, which becomes an offset of 342 when multiplied by 2. So the pixels that past the scissor test (those with x between 342 and 981 and y between 342 and 821) are offset by 342 into the EFB, ending up with x between 0 and 639 and y between 0 and 479.

The scissor offset step is the thing that can overflow. Textures on the GameCube/Wii are at max 1024 by 1024, and I guess operations on the EFB wrap around at that point. Suppose x_off was 427, which becomes an offset of 854 = 342 + 512. Then, a scissor with x0 of 342 and x1 of 981 ends up at -512 through 127. -512 becomes +512, and 512 is less than the EFB's width of 640, so the pixels at the left of the viewport (x=0 without the offset of 342) end up at x=512, and pixels at the right of the viewport (x=639 without the offset of 342) end up at x=127; x=512-639 and x=0-127 end up being drawn to and x=128-511 don't get drawn.

You can also get overflowing with a default x_off of 342 if the scissor has x1 - x0 > 1024; for instance, in a scissor with x0 of 342 and x1 of 2005 (639+342+1024), pixels with x between 342 and 2005 all pass the scissor test and then both x between 342-981 and x between 1366-2005 map to 0-639 on screen. With the default viewport, one of these is far out of bounds and will usually be clipped (always clipped in Dolphin, since it seems like we clip precisely to the viewport), but with clipping disabled (or presumably a large enough viewport) you can get primitives that draw over themselves, producing weird results.

All of this is also made more confusing by the fields for x0, y0, x1, and y1 being large enough to store [0, 4095] despite actually only going for [0, 2047] and x_off/y_off being large enough to store [0, 2046] (in jumps of 2). but instead only covering [0, 1022] (still in jumps of 2).

Hopefully this is enough to at least understand why things are happening. I'm not sure how to condense this down to a simpler, more useful explanation.

[neobrain]

Heh, what a mess (the general problem of course, not your description of it). Not sure I can help simplify this any further, but thanks for the explanation :)

[Pokechu22]

The difference for pbr-sfx is new since that fifolog was just added. Although at first glance this looks like it's a regression, this is actually how the game behaves on real hardware (and also how it behaves in Dolphin's other backends); see issue 12629 and my writeup on the subject. PR #10279 includes a game patch that fixes this behavior.

[dolphin-emu-bot]

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

• major-minor on ogl-lin-mesa: diff
• mario-golf-oob on ogl-lin-mesa: diff
• burnout2-vehicletextures on sw-lin-mesa: diff
• dbz-depth on sw-lin-mesa: diff
• djhero2-blend on sw-lin-mesa: diff
• jb-shadow on sw-lin-mesa: diff
• major-minor on sw-lin-mesa: diff
• mario-golf-oob on sw-lin-mesa: diff
• nfsu-reflections on sw-lin-mesa: diff
• nhl-slap on sw-lin-mesa: diff
• nintendo-channel on sw-lin-mesa: diff
• pbr-sfx on sw-lin-mesa: diff
• rs3-bumpmapping on sw-lin-mesa: diff
• sms-water on sw-lin-mesa: diff
• sw3-dt on sw-lin-mesa: diff
• major-minor on ogl-lin-radeon: diff
• mario-golf-oob on ogl-lin-radeon: diff
• major-minor on uberogl-lin-radeon: diff
• mario-golf-oob on uberogl-lin-radeon: diff

_{^{automated-fifoci-reporter}}

[JMC47]

This hasn't been looked at in a while and fixes a few games. Let's see if it breaks any.