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

Regression in Outrun's environment mapping, assert hit #16941

Closed
hrydgard opened this issue Feb 8, 2023 · 5 comments · Fixed by #16947
Closed

Regression in Outrun's environment mapping, assert hit #16941

hrydgard opened this issue Feb 8, 2023 · 5 comments · Fixed by #16947
Labels
GE emulation Backend-independent GPU issues
Milestone

Comments

@hrydgard
Copy link
Owner

hrydgard commented Feb 8, 2023

Outrun builds a sphere map by first drawing most of a cube map and then converting to a sphere map which PSP's env map can handle. The middle map seems to get stuck with some noise, which isn't very pretty.

There's also an assert that we hit, the new FB_USAGE_COLOR_MIXED_DEPTH flag prevents downsizing buffers but not completely, this fails for example:

			if (vfb->usageFlags & FB_USAGE_COLOR_MIXED_DEPTH) {
				ResizeFramebufFBO(vfb, vfb->width, vfb->height, true);
				_assert_(vfb->renderScaleFactor != 1);
			}

We hit the assert since Resize ends up its force1x path, so the image remains 1x after the resize, hitting the assert.

In this case, it's about a 64x64 texture, and FB_USAGE_COLOR_MIXED_DEPTH comes from DrawPixels with channel == DEPTH, which happens on the first frame for this texture due to lack of clearing it seems. There's no actual color/depth mixed usage going on.

Here's where the code was added that sets FB_USAGE_COLOR_MIXED_DEPTH:

e5d67119a8#diff-66fdbd9653ac45b624beb6fb8f2259fc5a13b83a25f48a89383145cafd58ba43R1155

I guess that's needed for the Burnout lens flare effect...

@hrydgard hrydgard added the GE emulation Backend-independent GPU issues label Feb 8, 2023
@hrydgard hrydgard added this to the v1.15.0 milestone Feb 8, 2023
@unknownbrackets
Copy link
Collaborator

Well, this is caused by #16908 because it made the assert stop passing. The goal of that assert was to ensure we're not needlessly resizing the framebuffer all the time, so it's a good thing it tripped.

-[Unknown]

@hrydgard
Copy link
Owner Author

hrydgard commented Feb 9, 2023

Oh yeah, good point that the assert did catch something really undesirable! And thanks for pointing out the real break.

@hrydgard
Copy link
Owner Author

hrydgard commented Feb 9, 2023

The garbage in the environment map is because of some noise that the depth buffer of it (which it makes no use of, but tests against with GREATER_EQUAL, probably a bug) gets initialized with. The Z and W of the vertices are all set to 1, so would figure it would always pass but it doesn't... It looks like it's using a compressed depth range (ACCURATE_DEPTH style), but the depth buffer initialization doesn't match it. So that's a bug - if the depth buffer noise was in the correct range, it would work, so that's what we should fix.

And/or it could be that we're switching from full depth range to depth rounding, without converting existing depth buffers... because this one is sitting around unchanged between frames.

hrydgard added a commit that referenced this issue Feb 9, 2023
And thus change of depth buffer scale/offset.

Previously, old depth buffers with values that now are out of range
could stick around, causing #16941. This clears them to the expected 0
value, which helps Outrun. Ideally we should convert depth buffers to
the new format, but if we can get away without that, that's also nice.
@hrydgard
Copy link
Owner Author

#16947 will fix it once finished.

hrydgard added a commit that referenced this issue Feb 10, 2023
And thus change of depth buffer scale/offset.

Previously, old depth buffers with values that now are out of range
could stick around, causing #16941. This clears them to the expected 0
value, which helps Outrun. Ideally we should convert depth buffers to
the new format, but if we can get away without that, that's also nice.

This is enough for #16941.
@hrydgard
Copy link
Owner Author

Actually, this is at least superficially fine now with clearing, so I'm closing this specific issue. A depth calculation cleanup is coming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GE emulation Backend-independent GPU issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants