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 copy dependency for depth and color textures #4365

Merged
merged 3 commits into from Oct 31, 2023

Conversation

gdkchan
Copy link
Member

@gdkchan gdkchan commented Feb 4, 2023

Luigi's Mansion 3 renders a texture using the R16Unorm format, and samples it using the D16Unorm format. We did not support that case before since those formats are not view compatible. This change adds support for this case using copies (copy dependency).

Fixes shadow issues on Luigi's Mansion 3.
Before:

RySd097vX5.mp4

After:

tmL7A8Gm9D.mp4

Thanks to @riperiperi for finding the issue.

Fixes #2791.

@gdkchan gdkchan added gpu Related to Ryujinx.Graphics fix Fix something labels Feb 4, 2023
@gdkchan
Copy link
Member Author

gdkchan commented Feb 6, 2023

Now that the OpenGL regression was fixed and the other PR was merged, I was able to test it on OpenGL. Seems to be working correctly, so ready for review. It is using PBO copies on OpenGL which are usually slow, so I'm a bit worried about the performance there. But it's hard to tell since Luigi's Mansion 3 performance on OpenGL is already extremely poor here, at least without the mod to disable dynamic res that I don't have.

@gdkchan gdkchan marked this pull request as ready for review February 6, 2023 03:34
@Bjorn29512
Copy link

No noticeable regressions in botw, bayo 3 or smo with this PR, will update as i test more titles.

@riperiperi
Copy link
Member

riperiperi commented Feb 16, 2023

This PR causes BOTW to copy a depth texture back and forth every frame:

image

Normally:

image

It could definitely be a lot worse, but this could hurt weaker GPUs and might cause issues when scaling. I think the rules need to be changed to prefer the most recently modified texture instead of a "perfect" match. One weird case with that would be 2D copy to depth texture (as R32), I guess you'd still want the recently modified match as it will probably be D32 due to being used as a depth attachment.

riperiperi added a commit to riperiperi/Ryujinx that referenced this pull request May 11, 2023
@gdkchan pointed out that UI textures in TOTK seemed to be setting their texture swizzle incorrectly (texture was RGB but was sampling A, swizzle for A was wrong), so I determined that SwizzleComponentMatches was the problem and set on eliminating it. This PR combines existing work to select the most recently modified texture (now used when selecting which aliased texture to use) with some additional changes to remove the swizzle check and support aliased view creation.

The original observation (Ryujinx#1538) was that we wanted to match depth textures for the purposes of aliasing with color textures, but they often had different swizzle from what was sampled (as it's generally the identity swizzle once rendered). At the time, I decided to allow swizzles to match if only the defined components matched, which fixed the issue in all known cases but could easily be broken by a game _expecting_ a given swizzle, such as a 1/0 value on a component.

This error case could also occur in textures that don't even depth alias, such as R11G11B10, as the rule was created to generally apply to all cases.

The solution is now to fail this exact match test, and allow the search for an R32 texture to create a swizzled view of a D32 texture (and other such cases). This allows the creation of a view that mismatches the requested format, which wasn't present before and was the reason for the swizzle matching approach.

The exact match and view creation rules now follow the same rules over what textures to select when there are multiple options (such as a "perfect" match and an "aliased" match at the same time). It now selects the most recently modified texture, which is done with a new sequence number in the GpuContext (because we don't have enough of these).

Reportedly fixes UI having weird coloured backgrounds in TOTK. This also fixes an issue in MK8D where returning from a race resulted in the character selection cubemaps being broken. May work around issues introduced by the "short texture cache" PR due to modification ordering, though they won't be truly fixed.

Should allow (Ryujinx#4365) to avoid copies in more cases. Need to test that.

I tested a bunch of games Ryujinx#1538 originally affected and they seem to be fine. This change affects all games so it would be good to get some wide testing on it.
gdkchan pushed a commit that referenced this pull request May 12, 2023
* GPU: Remove swizzle undefined matching and rework depth aliasing

@gdkchan pointed out that UI textures in TOTK seemed to be setting their texture swizzle incorrectly (texture was RGB but was sampling A, swizzle for A was wrong), so I determined that SwizzleComponentMatches was the problem and set on eliminating it. This PR combines existing work to select the most recently modified texture (now used when selecting which aliased texture to use) with some additional changes to remove the swizzle check and support aliased view creation.

The original observation (#1538) was that we wanted to match depth textures for the purposes of aliasing with color textures, but they often had different swizzle from what was sampled (as it's generally the identity swizzle once rendered). At the time, I decided to allow swizzles to match if only the defined components matched, which fixed the issue in all known cases but could easily be broken by a game _expecting_ a given swizzle, such as a 1/0 value on a component.

This error case could also occur in textures that don't even depth alias, such as R11G11B10, as the rule was created to generally apply to all cases.

The solution is now to fail this exact match test, and allow the search for an R32 texture to create a swizzled view of a D32 texture (and other such cases). This allows the creation of a view that mismatches the requested format, which wasn't present before and was the reason for the swizzle matching approach.

The exact match and view creation rules now follow the same rules over what textures to select when there are multiple options (such as a "perfect" match and an "aliased" match at the same time). It now selects the most recently modified texture, which is done with a new sequence number in the GpuContext (because we don't have enough of these).

Reportedly fixes UI having weird coloured backgrounds in TOTK. This also fixes an issue in MK8D where returning from a race resulted in the character selection cubemaps being broken. May work around issues introduced by the "short texture cache" PR due to modification ordering, though they won't be truly fixed.

Should allow (#4365) to avoid copies in more cases. Need to test that.

I tested a bunch of games #1538 originally affected and they seem to be fine. This change affects all games so it would be good to get some wide testing on it.

* Address feedback 1, fix an issue

* Workaround: Do not allow copies for format alias.

These should be removed when D32<->R32 copy dependencies become legal
@gdkchan
Copy link
Member Author

gdkchan commented Sep 14, 2023

I rebased it, but BOTW still copies between a R32Float and a D32Float texture on every frame. Not sure what was the plan to mitigate this, maybe keeping the FormatAlias compatibility and let it sample as the other format?

Update:
I also tried:

            if (lhsFormat.Format.IsDepthOrStencil() || rhsFormat.Format.IsDepthOrStencil())
            {
                bool forSampler = flags.HasFlag(TextureSearchFlags.ForSampler);
                bool depthAlias = flags.HasFlag(TextureSearchFlags.DepthAlias);

                TextureMatchQuality matchQuality = FormatMatches(lhs, rhs, forSampler, depthAlias);

                if (matchQuality == TextureMatchQuality.Perfect)
                {
                    return TextureViewCompatibility.Full;
                }
                else if (matchQuality == TextureMatchQuality.FormatAlias)
                {
                    return TextureViewCompatibility.FormatAlias;
                }
                else if (IsValidColorAsDepthAlias(lhsFormat.Format, rhsFormat.Format) || IsValidDepthAsColorAlias(lhsFormat.Format, rhsFormat.Format))
                {
                    return TextureViewCompatibility.CopyOnly;
                }
                else
                {
                    return TextureViewCompatibility.Incompatible;
                }
            }

(While keeping the FormatAlias compatibility) and the result was the same on BOTW, still doing copies every frame.

@gdkchan gdkchan marked this pull request as draft September 14, 2023 23:59
@riperiperi
Copy link
Member

riperiperi commented Sep 16, 2023

The texture lookup should prefer the last used texture when depth aliasing is possible. There's a possibility it selects the wrong one when the texture has a cached entry on the texture pool (it used it as R32 in the past, but the recent modifications were all D32)... But I'd imagine that should be random and happen after a while, not happen immediately. It might be necessary to remove pool entries in cases like this.

@gdkchan
Copy link
Member Author

gdkchan commented Oct 3, 2023

This should be ready for review again, #5719 stopped BOTW from copying depth textures every frame, so I think there's nothing blocking it now.

@gdkchan gdkchan marked this pull request as ready for review October 3, 2023 23:49
@github-actions github-actions bot added graphics-backend:vulkan Graphical bugs when using the Vulkan API graphics-backend:opengl Graphical bugs when using the OpenGL API labels Oct 3, 2023
@ryujinx-mako ryujinx-mako bot requested review from marysaka and a team October 3, 2023 23:49
Copy link
Contributor

@marysaka marysaka left a comment

Choose a reason for hiding this comment

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

lgtm

@Linear524
Copy link

Can't wait for this to merge into master branch :)

Copy link
Member

@riperiperi riperiperi left a comment

Choose a reason for hiding this comment

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

LGTM, tested on a few games and everything looks good. Nice work!

@gdkchan gdkchan merged commit 841dd56 into Ryujinx:master Oct 31, 2023
10 checks passed
@gdkchan gdkchan deleted the depth-color-copydep branch October 31, 2023 22:00
@Linear524
Copy link

Works like a charm !
Thanks @gdkchan and @riperiperi !
I've waited a loooooong for this fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fix something gpu Related to Ryujinx.Graphics graphics-backend:opengl Graphical bugs when using the OpenGL API graphics-backend:vulkan Graphical bugs when using the Vulkan API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REGRESSION] Luigi's Mansion 3 - Shadow Maps not clearing.
5 participants