Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Software: Fix zfreeze with CullMode::All #10255

Merged
merged 3 commits into from Apr 14, 2022
Merged

Conversation

Pokechu22
Copy link
Contributor

CullMode::All is often used with zfreeze to draw a triangle for the reference plane without it showing up (see the note on GX_SetCoPlanar). However, this was broken in the software renderer. I've fixed it by re-using the infrastructure in VertexManagerBase used by the hardware backends, but this is a bit jank.

@Pokechu22
Copy link
Contributor Author

It seems like there was a second issue: the slope used with zfreeze should use offsets from the screen's origin, but instead it was using offsets from the triangle's origin. This is what caused the triangles in the sky in RS2: half of them had the first vertex at the top of the triangle, and the other half had the first vertex at the bottom of the triangle.

@Pokechu22 Pokechu22 marked this pull request as draft December 1, 2021 02:50
@Pokechu22 Pokechu22 force-pushed the sw-zfreeze branch 2 times, most recently from bd93522 to 93a203c Compare December 1, 2021 03:07
@phire
Copy link
Member

phire commented Dec 1, 2021

but this is a bit jank.

Indeed. A better fix is to disable the code in vertex manager which filters out CULL_ALL draw calls and forward them to video software. And then filter them out per triable, in the Rasterizer::Draw after the z slope has been calculated.

@Pokechu22 Pokechu22 force-pushed the sw-zfreeze branch 5 times, most recently from d5aec87 to 9facce2 Compare December 2, 2021 01:17
@Pokechu22 Pokechu22 marked this pull request as ready for review December 2, 2021 01:32
@Pokechu22 Pokechu22 force-pushed the sw-zfreeze branch 3 times, most recently from 3d2168b to bb1e9d3 Compare December 2, 2021 23:16
Comment on lines +53 to +70
Slope(float f0, float f1, float f2, const SlopeContext& ctx) : f0(f0)
{
float delta_20 = f2 - f0;
float delta_10 = f1 - f0;

// x2 - x0 y1 - y0 x1 - x0 y2 - y0
float a = delta_20 * ctx.dy10 - delta_10 * ctx.dy20;
float b = ctx.dx20 * delta_10 - ctx.dx10 * delta_20;
float c = ctx.dx20 * ctx.dy10 - ctx.dx10 * ctx.dy20;

dfdx = a / c;
dfdy = b / c;

x0 = ctx.x0;
y0 = ctx.y0;
xOff = ctx.xOff;
yOff = ctx.yOff;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equivalent code in the hardware backends looks like this:

float dx31 = out[8] - out[0];
float dx12 = out[0] - out[4];
float dy12 = out[1] - out[5];
float dy31 = out[9] - out[1];
float DF31 = out[10] - out[2];
float DF21 = out[6] - out[2];
float a = DF31 * -dy12 - DF21 * dy31;
float b = dx31 * DF21 + dx12 * DF31;
float c = -dx12 * dy31 - dx31 * -dy12;
// Sometimes we process de-generate triangles. Stop any divide by zeros
if (c == 0)
return;
m_zslope.dfdx = -a / c;
m_zslope.dfdy = -b / c;
m_zslope.f0 = out[2] - (out[0] * m_zslope.dfdx + out[1] * m_zslope.dfdy);
m_zslope.dirty = true;

Apart from some of the excessive minus signs and other confusion that also originally existed in InitSlope, there's a divide by zero check that doesn't exist in the software renderer. Does this actually matter?

The check was added in commit add59b3, but #1812 (comment) says it shouldn't happen with zfreeze specifically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my comment is saying this based on the fact that games that use zfreeze should be sane enough to use non-degenerate triangles for their z reference planes.

No idea what happens on real hardware. These are zero-sized triangles that won't even get rasertized, right? Which is why they don't have a valid slope. It's entirely possible they have already been discarded by this point in the pipeline.

These aren't particularly useful, and make the code a bit more confusing.  If for some reason someone wants to test what happens when these functions are disabled, it's easier to just edit the code that implements them.  They aren't exposed in the UI, so one would need to restart Dolphin to do it anyways.
This is needed since we need a separate offset for zfreeze to work correctly.  It also makes the code a bit less jank.
@dolphin-emu-bot
Copy link
Contributor

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

  • djfny-menu on sw-lin-mesa: diff
  • mtennis-zfreeze on sw-lin-mesa: diff
  • rs2-skybox on sw-lin-mesa: diff
  • rs2-zfreeze on sw-lin-mesa: diff
  • rs3-bumpmapping on sw-lin-mesa: diff
  • rs3-skybox2 on sw-lin-mesa: diff

automated-fifoci-reporter

@phire phire merged commit c5c4169 into dolphin-emu:master Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants