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

Implement basic depth texturing for OpenGL #14042

Merged
merged 5 commits into from
Jul 9, 2021
Merged

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Jan 31, 2021

Dug up a couple of old branches and put them together and got it working.

Only works on desktop, this currently requires GL_ARB_depth_clamp which is not really supported on mobile (there is an extension, GL_EXT_depth_clamp, but few mobile devices support it).

Need to figure out what to do about the extended Z mappings we use when either extension is not available.

Helps #6411 for OpenGL on desktop primarily, and certain other games affected by #13256
are likely working too like the fog in Harry Potter.

Let's merge after 1.11.

@hrydgard hrydgard added GE emulation Backend-independent GPU issues Depth Texture An effect needs texturing from depth buffers. labels Jan 31, 2021
@hrydgard hrydgard added this to the v1.12.0 milestone Jan 31, 2021
@Panderner
Copy link
Contributor

What required OpenGL version to support GL_DEPTH_CLAMP?

@hrydgard
Copy link
Owner Author

I believe desktop GL 3.2 or newer is enough, or that exposes the extention GL_ARB_depth_clamp .

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

In the case where we enable this, shouldn't we also enable accurate depth? Otherwise the depth values we're texturing from will be skewed and wrong in some cases.

-[Unknown]

caps_.depthClampSupported = gl_extensions.ARB_depth_clamp;

// Interesting potential hack for emulating GL_DEPTH_CLAMP (use a separate varying, force depth in fragment shader):
// https://stackoverflow.com/questions/5960757/how-to-emulate-gl-depth-clamp-nv
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably should do this, but I'd think we might need a setting - it might make things a lot slower in some games where people might be willing to withstand weird clipping issues near the edges for speed...

In some games depending on how they draw, it might not be that big a difference I suppose.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, will depend a lot on the GPU architectures too, changing gl_FragDepth can have between nearly none to a very large performance penalty, I believe. So a blanket enable is probably not realistic, adding that to the comment.

GPU/GLES/StateMappingGLES.cpp Outdated Show resolved Hide resolved

if (pixelFormat == GE_FORMAT_DEPTH16) {
DepthScaleFactors factors = GetDepthScaleFactors();
WRITE(p, "const float z_scale = %f;\n", factors.scale);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, I'd recommend factors.scale * 1.0f / 65535.0f. Then below it'd just be color = (color - z_offset) * z_scale; right?

These values wouldn't vary, so we could avoid the formula when offset is 0 and scale is 65535, which will be true if clamp is enabled.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hm, I don't see how dividing by 65535 helps here. Then we'd just have to scale it up again to turn it into an integer index later...

Indeed, the values are constant, but I'm thinking that we might be able to also support this when we use the depth rescaling/offset stuff to simulate clamping, in some cases, so want to keep the flexibility. The distortion you mentioned earlier kind of prevents that though I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I missed the other code below that wants it as an int. This doesn't already work with the 1/4 scale thing?

Also in that case, how does WRITE(p, " float color = tex.Sample(texSamp, v_texcoord0).x;\n"); work with color.x later?

-[Unknown]

Copy link
Owner Author

@hrydgard hrydgard Feb 1, 2021

Choose a reason for hiding this comment

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

I'm not sure honestly if it works correctly with the 1/4 scale. But hopefully it does.

You can take .x of a float in HLSL, it's effectively a vec1. However it does look confusing...

@Panderner
Copy link
Contributor

Any android devices that supports GL_EXT_depth_clamp?

@brujo5
Copy link

brujo5 commented Feb 1, 2021

@Panderner Maybe the nvidia shield android tv

@glebm
Copy link
Contributor

glebm commented Feb 1, 2021

I think many mali GPUs support GL_EXT_depth_clamp

@hrydgard
Copy link
Owner Author

hrydgard commented Feb 1, 2021

@ghost
Copy link

ghost commented Jun 8, 2021

@hrydgard This pr only help desktop opengl but not android without GL_EXT_depth_clamp?
Screenshot_2021-06-08-15-35-02-704_org ppsspp ppsspp

Should make support for depth texturing quite easy.

Unfortunately, this extension does not exist on OpenGL ES. There we'll
have to use ugly tricks with gl_FragDepth if we want this.
…now.

Need to figure out what to do about other Z mappings.

Helps #6411 for OpenGL on desktop primarily, and certain other games affected by #13256
are likely working too like the fog in Harry Potter.
@hrydgard
Copy link
Owner Author

hrydgard commented Jul 9, 2021

Rebased on master again, surprisingly no conflicts.

And no, this won't help on GLES unfortunately, only desktop-gl for now.

And in response to a previous comment, this does enable accurate-depth when the required extension is available, so the depth buffer scaling issues and such should not apply.

I do think this might be ready for merge as-is.

@hrydgard
Copy link
Owner Author

hrydgard commented Jul 9, 2021

Eh, let's just try it.

@hrydgard hrydgard merged commit 1f16edd into master Jul 9, 2021
@hrydgard hrydgard deleted the depth-texturing-gl branch July 9, 2021 20:50
if (!gstate_c.Supports(GPU_SUPPORTS_32BIT_INT_FSHADER)) {
useShaderDepal = false;
depth = false; // Can't support this
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, is it better to use the color part of the framebuf or just ignore the attachment I wonder?

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

The color part probably isn't usually very meaningful when you're trying to read the depth, so ignoring is probably better... unfortunately currently no good way to bind an empty texture here? Hm.

Copy link
Collaborator

@unknownbrackets unknownbrackets Jul 17, 2021

Choose a reason for hiding this comment

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

I guess the better thing would be a Supports flag and handle this when choosing candidates... (maybe we already even do that?)

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I think we already do...

@Panderner
Copy link
Contributor

Note: This also fixes Hokuto no Ken blackscreen issue for OpenGL for desktop systems (#7932).

@unknownbrackets
Copy link
Collaborator

unknownbrackets commented Sep 13, 2021

That's just because it enables depth clamp on desktop OpenGL. Accurate depth (with its "simulation" of depth clamp) would probably fix it, too.

-[Unknown]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Depth Texture An effect needs texturing from depth buffers. GE emulation Backend-independent GPU issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants