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

VideoCommon: Only include centered pixels in bounding box #9801

Merged
merged 1 commit into from Jun 11, 2021

Conversation

Techjar
Copy link
Contributor

@Techjar Techjar commented Jun 8, 2021

At higher resolutions, our bounding box dimensions end up being slightly larger than original hardware in some cases. This is not necessarily wrong, it's just an artifact of rendering at a higher resolution, due to bringing out detail that wouldn't have appeared on original hardware. It causes a texel to fall partially on what would have been a single pixel at native resolution, resulting in the coordinates getting bumped up to the next valid value. In many cases, these slightly larger bounding boxes are perfectly fine, as games don't hard-code expected dimensions. It is problematic in Paper Mario TTYD though, for a somewhat complicated reason.

Paper Mario TTYD frequently uses EFB copies to pre-render a bunch of animation frames for a character sprite (especially in Chapter 2), so that it can then render 100 or more of them without bringing the GameCube to its knees. Based on my observation, the game seems to set aside a region of memory to store these EFB copies. This region is obviously fairly small, as the GameCube only has 24MB of RAM. There are 2 rooms in Chapter 2 where you fight a horde of as many as 100 Jabbies, which are also rendered using EFB copies, so in this room the game ends up making 130(!) EFB copies just for Puni and Jabbi sprites. This seems to nearly fill the region of memory it set aside for them. Unfortunately, our slightly larger bounding boxes at higher resolutions results in overflowing this memory, causing very strange behavior. Some EFB copies partially overlap game state, resulting in reading it as a garbage RGB5A3 texture that constantly changes. Others apparently somehow trigger a corner case in our persistent buffer mapping, causing them to partially overwrite earlier EFB copies.

What this change does is only include the screen coordinates that align with the equivalent native resolution pixel centers, which generally results in the bounding boxes being more in line with original hardware. It isn't perfect, but it's enough to fix Paper Mario TTYD's Jabbi rooms by avoiding the buffer overflow. Notably, it is more accurate at odd resolutions than at even resolutions. Native resolution is completely unaffected by this change, as should be the case. This change may also have a small positive impact on shader performance at higher resolutions, as there will be less atomic operations performed.

Here's some diffs comparing to master at the same resolution, as well as high resolution vs native resolution on this PR (native resolution is unchanged from master, as mentioned).
Master vs This PR @ 4x Native
Master vs This PR @ 5x Native
1x Native vs 4x Native
1x Native vs 5x Native

@JMC47 JMC47 changed the title VideoCommon: Bounding box high resoution rounding hack VideoCommon: Bounding box high resolution rounding hack Jun 9, 2021
@JMC47
Copy link
Contributor

JMC47 commented Jun 9, 2021

Disney Hide & Sneak: Works on all Internal Resolutions
Disney's Magical Mirror: Works on all Internal Resolutions. Verified the Silverware crash does not happen on any internal resolution.
Ultimate Spider-Man: Verified that all backends worked while switching internal resolutions through the Rhino mission.
Paper Mario: The Thousand Year Door: Verified Punies don't glitch, the Step doesn't become erect, and the game doesn't hang when eavesdropping on D3D11/D3D12 above 1x IR.
Super Paper Mario: Verified that Chapter 3 does not have disappearing characters and the shop in Flopside doesn't hang.

Source/Core/VideoCommon/PixelShaderGen.cpp Outdated Show resolved Hide resolved
@Techjar Techjar force-pushed the bbox-rounding-hack branch 3 times, most recently from 0f6a8c4 to 7d99f40 Compare June 10, 2021 00:37
Copy link
Member

@phire phire left a comment

Choose a reason for hiding this comment

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

As discussed on IRC, I think this is the wrong approach.

Instead, I think we should adjust the pixel center correction code in VertexShaderGen to make sure that one of the pixels in the higher IR grid (2x2 for 2xIR, 3x3 for 3xIR etc) lines up with the pixel centre of the original 1xIR grid. Ideally this will be the center pixel, but for even IRs it will have to be off-center.

Then you can simply adjust the pixel shader bounding box code it only updates the atomics on pixel shader invocations that on 1xIR grid.


Rather than being a hack, this should result in almost identical results to running at 1xIR

@Techjar Techjar changed the title VideoCommon: Bounding box high resolution rounding hack VideoCommon: Only include centered pixels in bounding box Jun 10, 2021
@Techjar Techjar force-pushed the bbox-rounding-hack branch 3 times, most recently from 9d429f8 to 8adb0eb Compare June 10, 2021 13:04
@Techjar
Copy link
Contributor Author

Techjar commented Jun 10, 2021

@phire Thanks for the help. I think I've come up with a pretty good implementation of what you suggested, and it's producing significantly more accurate results than the previous attempt, not to mention looking a fair bit cleaner. I also looked at the pixel center correction, and it already seems to be as accurate as we can make it for higher-than-native resolutions, so there wasn't any changes to be made there.

@phire
Copy link
Member

phire commented Jun 10, 2021

Ok, the new version of this PR is much better IMO.

Doesn't produce perfect results, that might require playing with the vertex shader code to get the pixel centres aligned correctly, but I think this is good enough for now.

Copy link
Member

@phire phire left a comment

Choose a reason for hiding this comment

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

Approved dependant on testing from JMC47

At higher resolutions, our bounding box dimensions end up being
slightly larger than original hardware in some cases. This is not
necessarily wrong, it's just an artifact of rendering at a higher
resolution, due to bringing out detail that wouldn't have appeared on
original hardware. It causes a texel to fall partially on what would
have been a single pixel at native resolution, resulting in the
coordinates getting bumped up to the next valid value. In many cases,
these slightly larger bounding boxes are perfectly fine, as games don't
hard-code expected dimensions. It is problematic in Paper Mario TTYD
though, for a somewhat complicated reason.

Paper Mario TTYD frequently uses EFB copies to pre-render a bunch of
animation frames for a character sprite (especially in Chapter 2), so
that it can then render 100 or more of them without bringing the
GameCube to its knees. Based on my observation, the game seems to set
aside a region of memory to store these EFB copies. This region is
obviously fairly small, as the GameCube only has 24MB of RAM. There are
2 rooms in Chapter 2 where you fight a horde of as many as 100 Jabbies,
which are also rendered using EFB copies, so in this room the game ends
up making 130(!) EFB copies just for Puni and Jabbi sprites. This seems
to nearly fill the region of memory it set aside for them.
Unfortunately, our slightly larger bounding boxes at higher resolutions
results in overflowing this memory, causing very strange behavior. Some
EFB copies partially overlap game state, resulting in reading it as a
garbage RGB5A3 texture that constantly changes. Others apparently
somehow trigger a corner case in our persistent buffer mapping, causing
them to partially overwrite earlier EFB copies.

What this change does is only include the screen coordinates that align
with the equivalent native resolution pixel centers, which generally
results in the bounding boxes being more in line with original
hardware. It isn't perfect, but it's enough to fix Paper Mario TTYD's
Jabbi rooms by avoiding the buffer overflow. Notably, it is more
accurate at odd resolutions than at even resolutions. Native resolution
is completely unaffected by this change, as should be the case. This
change may also have a small positive impact on shader performance at
higher resolutions, as there will be less atomic operations performed.
@JMC47 JMC47 merged commit 0c6e00c into dolphin-emu:master Jun 11, 2021
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants