-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Cull out-of-bounds triangles that were not clipped #9527
Conversation
I seem to remember it drops anything that even slightly goes outside the 4096x4096 grid - the one established by the viewport and offset parameters. But this might be affected by the clip flag (I think that was only depth?) -[Unknown] |
GPU/Common/ShaderUniforms.h
Outdated
int spline_count_u; | ||
int spline_count_v; | ||
int spline_type_u; | ||
int spline_type_v; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess these are VS parameters, not FS, aren't they?
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I forgot we had that split. Didn't really mean to include that move anyway, I'll move them back.
Yeah, I don't think the clip flag is involved but you never know. 4096x4096 sounds reasonable. Should probably transform that space into the clip space (using offset etc as you say) and pass the four "walls" in as uniforms... |
See here: I think these two are affected by the same issue. -[Unknown] |
Yup, good memory, all of these are almost certainly the same thing. Now just got to figure out how to best implement it... This method works for me, but only because nVidia cards are smart enough to reject triangles with NaN coordinates instead of going insane - I believe it's undefined behaviour so it could also turn the moon into cheese, or whatever. |
This sounds promising though: |
OK, so I really don't understand this. I was under the impression that the PSP worked like this:
This means that we should not kill triangles with w < 0 in the vertex shader as they would have gotten clipped by the PSP and thus might end up inside the 4096x4096. Anyway. I'm getting road triangle dropouts now that I'm sizing the box right: I decided to create a zoomed-out debug mode which instead of dropping triangles, colored the out-ouf-bound vertices purple: The game is performing manual clipping of the road into that big "road triangle" (clearly dropping triangles by moving a vertex way off to the right, causing the annoying stripes) but it happens that triangles that should be drawn have vertices way outside the allowed 4096x4096 square (not marked but should be obvious where it is) and reach all the way to the car, causing the visible triangle dropouts. I had expected those to be behind the camera but no, they aren't - they are behind the z=0 of the Z buffer though. So the near Z clip of the PSP doesn't work quite as I thought, otherwise we'd be having these dropouts on the real PSP as well. Grrr. I guess there is a possibility though that the restriction doesn't apply vertically but that would be strange. |
1380e70
to
6998732
Compare
For now I left out the bottom border. If this fixes the rest of the games, then .. meh :) though not really happy about it. Please test the other affected games again! |
Hmm, we can do more testing. Thinking about it more, from the testing I previously did (was some time ago now though):
With how it handles depth clipping (there's a "fuzz region" that is still clamped, iirc), I wouldn't put it past it to do something weird with the guard band, really... -[Unknown] |
GPU/Common/ShaderUniforms.cpp
Outdated
// We also assume that everything behind the near clipping plane gets clipped and will thus not in reality | ||
// exceed the guardband. This is a bit rough but should be ok. | ||
float offsetX = gstate.getOffsetX(); | ||
float offsetY = gstate.getOffsetY(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess worst case they would just not cull things they should, right? Maybe we remove these unused vars to reduce confusion?
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in theory if the game has the viewport say at the far left of the 4096x4096 square, it could draw something very wide only to the right that we'd now cull but shouldn't. I don't think that's much of a concern though.
Removed the vars, they were leftovers from a few previous variants.
GPU/D3D11/StateMappingD3D11.cpp
Outdated
@@ -427,10 +427,11 @@ void DrawEngineD3D11::ApplyDrawState(int prim) { | |||
if (rasterIter == rasterCache_.end()) { | |||
D3D11_RASTERIZER_DESC desc{}; | |||
desc.CullMode = (D3D11_CULL_MODE)(keys_.raster.cullMode); | |||
// desc.FillMode = gstate.isModeThrough() ? D3D11_FILL_SOLID : D3D11_FILL_WIREFRAME; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe remove these debugging comments?
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
This needs more work (and yes, HW testing) clearly, I'm getting some minor glitches in Wipeout on the left side. BTW, in D3D11, we can get the CLAMPED behaviour for Z by using rasterdesc.DepthClipEnable, but that shouldn't affect this issue (might solve something else though). |
6253b59
to
8c86b24
Compare
Did an experiment on an variant on D3D11/9 only - so testers (like @LunaMoo :), please try both OpenGL and D3D11. In this case, we simply don't cull if Z is out of range. |
FWIW, neither option seems to fix Brave Story (although I still think it's from culling), but the pre-experiment version removed a chunk of the triangle, the post-experiment removes none of it. -[Unknown] |
If it removed a chunk of the triangle, it has to be made out of multiple triangles... Detailed hardware tests will be required to figure this out. Though as-is it does fix some games at least... |
Driver 76 still works fine in d3d11(d3d9 looks about the same /to make it clear it's also fixed). Sangokushi IX now works in both OGL and d3d11 backends:) Edit: Found another problem in Driver 76 in build I made earlier before new changes here everything looks fine, in new build that works for all backends at certain angles in one place it doesn't draw part of the road:
Oh yeah I didn't notice, but that was the experiment;p, reverting 8c86b24 fixes the road there. |
Thanks for your testing @LunaMoo . This will be a tricky one to get completely right... |
Thanks @sum2012 , done. Though that can probably be fixed. |
Sangokushi IX draws the black thing via:
I think only the last rectangle matters here which looks like it's way higher than what should be affected by this branch now and is indeed in other backends, so why fails in d3d9? Maybe it's related to it being rectangle prim and first vertex at (0,0) is within range ~ isn't rectangle divided to two triangles? Or maybe because the texture is disabled for this in opposite to other cases where this branch does work in d3d9? |
This actually does help #5507, but it causes issues with missing UI elements (the tachometer during races, parts of the initial loading screens to be black instead of white, and various graphics on the main menu to disappear). |
Hm. Until this is understood better, maybe it makes sense to commit it behind a compat flag which we specifically enable for TOCA and the few other games it helps. But I'm a bit disappointed it isn't working better than it is, I don't understand why. |
I think we need to do some hardware testing - it could be the PPS hardware does the culling in some strange way. Maybe it doesn't even cull based on z at all, or something? -[Unknown] |
Okay, so summarizing hrydgard/pspautotests#194 based on what would achieve those results:
I think that covers it, but it's possible again that I've misunderstood how it works due to bias (see hrydgard/pspautotests#194). What do you think @hrydgard? -[Unknown] |
That all sounds good. but I expect that there might still be some remaining strange edge cases we can't foresee with the clipping or not and the resulting XY positions... Passing on a couple more floats for the guardband and a NaN value shouldn't be much of an issue though, we still have some fluff to save in our uniform structs. I don't know how reliable passing 0.0 for W is but maybe that works. We can also enable all this on a per-game basis using compat.ini although would be good to avoid that. |
There's definitely more to it. Persona 1 would draw hallways incorrectly from the above rules - see here: -[Unknown] |
Oh. Yeah .. no idea about that one. It's a bit annoying how the PSP's rasterizer is mostly so normal, but has these bizarre edge cases.... I'm gonna rebase this again soon, for whatever it's worth. |
…st for Toca. Really not sure how big the guard band should be, and the right way to do this would be to use a geometry shader instead of killing triangles by setting w = NaN, which I'm not sure will do sane things on all hardware. Unfortunately geometry shaders are not available everywhere. Assumes the viewport is centered in the 4096x4096 rectangle to save a uniform. Ignores the bottom for now, can't figure out TOCA :(
… the clipper works when Z is out of range, let's only cull when Z is in range.
e248845
to
2013eb3
Compare
Rebased this on master again (squashed a couple of commits to make the rebase easier, there was some back-and-forth going on) |
I've confirmed the following so far:
-[Unknown] |
GPU/Vulkan/ShaderManagerVulkan.cpp
Outdated
<<<<<<< HEAD | ||
======= | ||
OutputDebugStringA(LineNumberString(code).c_str()); | ||
>>>>>>> Experimental way to cull out-of-bounds triangles. Helps #5001, at least for Toca. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops.
-[Unknown]
…. Rebase away this next time.
Ah... I tried taking the coordinates through M, V, P, and then viewport. It still worked after MVP, which doubly confirmed the above - then I finally realized the obvious, d'oh. The vertices that are outside the 4096x4096 space in this case ALSO have depth outside 65535. If the depth is forced to a valid value, it kills the verts. So basically, when the depth clamp flag is enabled, clamping bypasses killing the vertex. But if it's not clamped, it's subject to the 4096x4096 cull. Now it makes sense again. -[Unknown] |
"bypasses killing the vertex" must then mean that the hardware performs some sort of clipping against the near plane, making the remaining verts in-bounds in X and Y, because clearly the 0-4096 thing is a hardware limitation. And that would mean that the clamp flag performs clipping, and maybe the clip flag performs clamping? :) |
That makes sense. In that way, I suppose it makes sense to call it a clipping flag. But, it's important to note that depth clamp will cause an object entirely behind or in front of the frustum (i.e. before the near plane or after the far plane) to still draw, and not clip to those planes. Meaning it only clips to the four planes on the sides. It definitely does clamp depth, though. If you consider the viewing frustum to be always 4096x4096 and depth 0-65535 (regardless of viewport or minz/maxz), then with depth clamp off, even a single vertex slightly outside the 0-65535 range will cause the triangle not to draw. Same as outside 4096x4096. But with it on - unless I'm confused - it will draw even the parts outside the frustum (on the near and far sides), just squished to the near and far sides like a deformed tin can. The minz/maxz params are applied after this, possibly at the fragment level. Though I still think it's appropriate to think of them as clipping, essentially. -[Unknown] |
Oh I see. The phenomenon where clamping depth makes triangles stay makes sense for vertices that are closer to the camera than the Z near plane, but not beyond W=0 , and beyond the Z far plane, but not beyond W=1. Those would, without clamping, end up with Z that are too large (or small, depending on the projection matrix) to interpolate and therefore the GPU throws them away. Maybe it's really that simple and games never really try to draw triangles that cross W=0.... |
It's possible I'm not testing crossing w=0 and w=1 properly, though. I'm not exactly sure how to cleanly test that, tbh. -[Unknown] |
Helps #5001, at least for Toca.
Looks like this may also help #2159, #8251, #5507 and #7026 .
This could also be ported to the other backends with a little bit of effort. Need a uniform to stash the NaN in.
Really not sure how big the guard band should be, and the right way to do this would be to use a geometry shader instead of killing triangles by setting w = NaN, which I'm not sure will do sane things on all hardware. Unfortunately geometry shaders are not available everywhere.