Handle guardband clip/cull better for hardware backends #14833
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is really super cool! Finally we have an explanation for most/all of the culling weirdness, and we'll be able to delete a lot of stuff from compat.ini, I think, even if not fully implemented on D3D9 at least the guardband culling should work better with this. I'll look at this in detail later today. But yes, will probably wait until post 1.12 - the main danger is indeed driver bugs. As for Mali, I want to try later to implement a geometry shader path that does the clipping and culling, and see if it's too slow or not. As previously mentioned, most recent Mali devices do (reluctantly!) support geometry shaders. |
hrydgard
added
GE emulation
|
This pr also help #1788? |
ghost
mentioned this pull request
I didn't try it, but I already thought accurate depth avoided those issues. I assume these changes would make any remaining issues work better too. That said, this doesn't remove any of those hack settings - yet. So it doesn't change the Phantasy Star Portable behavior. -[Unknown] |
|
Tried experimenting with that, and apparently this causes all kinds of issues on macOS 11.5 with OpenGL backend on AMD RX 590 with Valkyria Chronicles II. Is that known? Vulkan seems to be ok.
|
|
No but now it is! Thanks for reporting. |
|
Sounds like it is having hard times with the trash GL backend on macOS: log |
|
This looks incorrect to me: caps_.clipCullDistanceSupported = gl_extensions.EXT_clip_cull_distance || (!gl_extensions.IsGLES && gl_extensions.VersionGEThan(3, 0));
Can we implement the feature without relying on |
|
Yeah, seems we need to split that caps_ bool into two. Clearly not supported by Apple's outdated OpenGL implementation. |
vit9696
reviewed
Sep 19, 2021
Common/GPU/OpenGL/thin3d_gl.cpp
Outdated
| @@ -534,6 +534,7 @@ OpenGLContext::OpenGLContext() { | |||
| caps_.framebufferBlitSupported = gl_extensions.NV_framebuffer_blit || gl_extensions.ARB_framebuffer_object; | |||
| caps_.framebufferDepthBlitSupported = caps_.framebufferBlitSupported; | |||
| caps_.depthClampSupported = gl_extensions.ARB_depth_clamp; | |||
| caps_.clipCullDistanceSupported = gl_extensions.EXT_clip_cull_distance || (!gl_extensions.IsGLES && gl_extensions.VersionGEThan(3, 0)); | |||
There was a problem hiding this comment.
gl_CullDistance is part of OpenGL 4.5, not 3.0.
Suggested change
| caps_.clipCullDistanceSupported = gl_extensions.EXT_clip_cull_distance || (!gl_extensions.IsGLES && gl_extensions.VersionGEThan(3, 0)); | |
| caps_.clipCullDistanceSupported = gl_extensions.EXT_clip_cull_distance || (!gl_extensions.IsGLES && gl_extensions.VersionGEThan(4, 5)); |
|
Might be nice to support clip only if needed, but wasn't sure if I wanted to dedicate a whole supports bit to it since we're running low... I guess could bite the bullet and switch to 64-bit if needed... -[Unknown] |
ghost
mentioned this pull request
|
Added (untested) support for APPLE_clip_distance, since those devices apparently typically don't support cull distance. Also made it so clip-only is supported, and at least fixes the bugs only clipping would fix. -[Unknown] |
|
Good job @unknownbrackets, this fixes #11399 (comment). I understand that it could be risky, but this fixes an annoying regression so I hope it finds its way into 1.12. |
Yes I agree this should be added to v1.12.3 milestone as it's fixes a lot of graphical issue. |
unknownbrackets
force-pushed
the
guardband
branch
from
5660c15
to
759b697
Compare
ghost
mentioned this pull request
5 tasks
Both TL and BR must be outside in the same direction to be culled when depth clamp is enabled.
If any vert is outside Z, it's culled when not clamping/clipping.
This culls based on pre-viewport Z and avoids culling based on the clip range at negative Z.
Pretty limited on GLES3+. Also D3D11. Seems like doing it on D3D9 might be a bit tricky.
On GLES, saw a texture bound to slot 1 when UI started to draw after an emu frame, which caused a crash because there was no sampler. Let's just explicitly flush.
Following PSP rules of -1 to 1 pre-viewport Z. This also enables it for GLES/OpenGL.
GL_ARB_cull_distance is needed, sometimes available on older GL.
It's really a bug (might even ideally cap the version?), and we already have other bugs handled the same way.
Older GL devices, and it seems Apple devices, may not support cull.
Seems modern Apple mobile chips only support clip.
unknownbrackets
force-pushed
the
guardband
branch
from
759b697
to
275bacc
Compare
|
|
|
There, Kraken released as requested @Gamemulatorer . Will let the buildbot complete this one before merging any more, want a separate build for this... |
|
After this pr I think DisableRangeCulling compat.ini can be remove now, because I tested a few games listed on that like |
|
That's right, but this method is not supported yet on D3D9 and also not fully on Mali GPUs, so some additional work is needed before we remove the section. However, we can probably just start ignoring it entirely. where this method works. |
5 tasks
|
I'll do a separate pull to ignore that compat when these things are fully supported soon. Hopefully we can entirely remove indeed. -[Unknown] |
This was referenced Oct 19, 2021
ghost
mentioned this pull request
5 tasks
This was referenced Oct 2, 2022
unknownbrackets
added a commit
to unknownbrackets/ppsspp
that referenced
this pull request
Oct 5, 2022
This hack was used because culling previously incorrectly handled Z, which was fixed in hrydgard#14833.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This doesn't really fix software transform, but it does implement user clip planes for Vulkan, GL/GLES, and D3D11. There is a way to set a clip plane with a constant buffer in D3D9, but it's more work and I don't think it can do the culling anyway.
This is probably dangerous to merge before 1.12.x, but it does fix a bunch of annoying/visible bugs in certain games and things work generally correctly. I fully expect weird driver bugs.
Features still missing (for follow up):
For software transform specifically, I did implement a change that implements culling, at least:
unknownbrackets@da50f87
But I don't really like it for a number of reasons:
Anyway, outside #10914, this takes care of the issues linked to #12058 that I have dumps for, at least on my PC. Haven't tested any wide range of devices at this point. Any reports of success or failure are welcome.
-[Unknown]