Depth scale cleanup

[hrydgard]

This tests that some depth related functions match each other better, and makes them purer (useflags are passed in as a parameter now, for ease of testing).

Later, I intend to make GetDepthScaleFactors the only way to get these, with the returned DepthScaleFactors object giving you all the needed functionality.

There's nothing here that changes behavior, only some renaming, except that calling DepthScaleFactor on its own now respects GPU_ACCURATE_DEPTH.

However, the new unit test fails when DEPTH_CLAMP is set, so I think we have some problems... Also, we always set DEPTH_CLAMP and ACCURATE_DEPTH together in GPUCommon, which seems like a contradiction - DEPTH_CLAMP seems to mean that we always use the full range 0-1 in depth buffers, while ACCURATE_DEPTH seems to mean that we don't... I'm a little confused.

EDIT: Removed misunderstanding, added better comments.

[hrydgard]

Anyway, what I'm thinking is:

• Is USE_DEPTH_CLAMP really supposed to mean to use the full 0-1 depth range, as DepthSliceFactor indicates?
• If so, does it really make sense to use USE_ACCURATE_DEPTH together with USE_DEPTH_CLAMP? I thought USE_ACCURATE_DEPTH meant to use a small centered range, and GPU_SCALE_DEPTH_FROM_24BIT_TO_16BIT same but an even smaller one, to get the reduced precision we want?

[unknownbrackets]

Haven't looked yet but quickly -

USE_ACCURATE_DEPTH is in contrast to using old-style GL depth. So:

• Accurate depth: Z in the framebuffer matches the range of Z used on the PSP in some way. Clip depth based on minz/maxz, and viewport is just a means to scale and center the value, not clipping or mapping to stored values.
• Non-accurate depth: effectively ignore minz/maxz. Map Z values based on viewport, which clamps. This skews depth in many instances. Depth can be inverted in this mode if viewport says. This is completely wrong, but works in some cases (probably because some game devs assumed it was how it worked) and avoids some depth clamp issues.

We can change its meaning, but that's the only thing accurate depth CURRENTLY means in the code. Additionally:

• Depth clamp: only viable in accurate depth mode, clamps depth and therefore uses the full 0-1 range. Using the full 0-1 range is not what accurate means, it's implied by depth clamp (which also means we're clamping.)
• Scale 24 to 16: only viable in accurate depth mode, means to use a range of the 24-bit depth values available from the GPU to represent the 16-bit values the PSP had, to try to make everything round and z-fight (close to) the same way cheaply.
• Otherwise, if accurate depth mode: use a subset of depth so that the lack of clamping as a buffer will sometimes work, which resolves issues in some games.

Maybe we should more clearly separate the "clamp simulation mode" as a sub-option to accurate depth. I feel like your pull description is conflating the two concepts. To me it's a hierarchy. Accurate depth, at least historically, just meant "don't use the wrong old depth." It'd be a confusing name for the subset method itself, because obviously DEPTH_CLAMP is the more accurate between those two (well, arguably, maybe 24-to-16 is more accurate in some ways... either way, it's the least accurate of those three options, if we're using that word.)

-[Unknown]

[hrydgard]

Right, I think I'm indeed getting it wrong, and the code is a little confused too, both the original and after my changes.

I really don't want to change the meaning of ACCURATE_DEPTH, that's accidental - I did misunderstand what it means, it seems. The idea with writing the test is to challenge my assumptions and force us to get this right.

Let's get this stuff fully cleared up once and for all in the code and in the naming of things, and let's centralize any other depth value conversions that are floating around in the code to use DepthScaleFactors (there are a few here and there that do their own checks and math).

[hrydgard]

Yeah I understand the flag/mode hiearchy now, thanks! I will fix up this PR accordingly tomorrow, and it will involve mostly commenting. Then after this is in, I'll centralize the remaining similar depth calculations, and finally, I'll bring back #16947 which will make all the shader uses of these factors dynamic.

[hrydgard]

Oh, one more question - do you know if there's a reason we don't remove GPU_USE_DEPTH_CLAMP when we activate GPU_SCALE_DEPTH_FROM_24BIT_TO_16BIT ? Because there we do have a contradiction of sorts. Though I guess it can still clamp some wildly out of range values in theory that would otherwise be missed, however unlikely that is... Anyway, in the depth calculation, GPU_SCALE_DEPTH_FROM_24BIT_TO_16BIT should obviously take precedence over GPU_USE_DEPTH_CLAMP.

Also this one I'm having a hard time figuring out, in the various shader managers:

		// Account for the half pixel offset.
		float viewZCenter = minz + (DepthSliceFactor(gstate_c.UseFlags()) / 256.0f) * 0.5f;

Do we count depth in pixels? I don't remember where 1/256th of the depth range comes into the picture. When converti
ng depth to color somehow?

[hrydgard]

Eh, went ahead and did it now already :) There was no bug to fix, just a bit of a misunderstanding, now corrected and commented to avoid it re-appearing in my head. I'm keeping the change in DepthSliceFactor, I don't think it's wrong.

Next step will be to fix the scale factor to make GetDepthScaleFactors just as accurate as ToScaledDepthFromIntegerScale, and then replace the latter with the former.

[unknownbrackets]

Do we count depth in pixels? I don't remember where 1/256th of the depth range comes into the picture. When converting depth to color somehow?

#8454 - I think this was copy pasta'd into D3D 11 and Vulkan and I don't know that it's correct there. Specifically see this comment:
#8454 (comment)

-[Unknown]

[unknownbrackets]

The armips revert looks accidental.

Don't remember why I didn't change Apply with ApplyReverse, I feel like that was me...

-[Unknown]