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: Add vertex shader point/line expansion #10890

Merged
merged 9 commits into from Oct 23, 2022

Conversation

TellowKrinkle
Copy link
Contributor

@TellowKrinkle TellowKrinkle commented Jul 25, 2022

Adds support for expanding lines and points using the vertex shader to the Vulkan and Metal backends

(Didn't bother with the other backends since they're guaranteed to have geometry shaders. If someone else wants to port to those for the speed benefits, they're welcome to do so, but I don't currently have any plans to do it. DX12 should just need some messing with the root descriptor, but DX11 and OGL will need to get dynamic vertex loader support added as well. Now implemented everywhere but DX11.)

This finally adds wide points/lines for Metal and MoltenVK!

Before and after screenshots

Metroid Prime 2 map before:
image

After:
image

I also added an option in the config to use it even on GPUs that support geometry shaders. It's noticeably faster in Metroid Prime 2's map on my AMD Radeon Pro 5600M.

Random profiling results

Radeon Pro 5600M execution of polygons from Metroid Prime 2's map in VS expand:
RGP profile of VS Expand

Same set of polygons in GS expand (graph scale is approximately the same number of µs):
RGP profile of GS Expand

Docs on what these graphs mean

The VS expand one seems limited by how quickly the command processor can queue up work, which is mostly limited by the large number of context rolls (caused by pipeline changes). On the other hand, in GS expand, work is being queued way earlier than executed. Something somewhere else in the GPU is delaying execution.

Implementation details

When VS expansion is enabled, indices are modified to use the bottom two bits for details about which point in the point/line expanded to a quad they represent. The vertex shader then loads and transforms the position for all associated vertices and uses it to calculate the appropriate offset to add.

Note: Requires #10781

Does anyone have any games that use points/lines with texture offsets? I haven't tested that because I don't know what games to test.

@Pokechu22
Copy link
Contributor

Does anyone have any games that use points/lines with texture offsets? I haven't tested that because I don't know what games to test.

The best game to test with for this is the heat man fight in Mega Man Network Transmission (fifolog, fifoci example). Note that there's something with this that makes the stage background not get recorded into the fifolog, and the game itself is apparently quite difficult (I don't own it; this is what JMC said), so it can be a bit annoying to get there to test, but the actual line/point texture offset effects are visible with the fifolog.

NES virtual console games also use it, I think; at least, they use lines as a part of their rendering (which I don't fully understand). There are fifologs of SMB and Zelda 1.

@dvessel
Copy link
Contributor

dvessel commented Sep 19, 2022

I would love to see this merged. It’s diverged from #10781 which needs a minor update itself.

@dvessel
Copy link
Contributor

dvessel commented Sep 19, 2022

I was subbed to @Pokechu22’s PR doing the same thing #10295 which I completely forgot about.

Since you are both here, I did a quick screen grab for both PR’s.

1x native scaled up 400% for reference:
1x_nonexpansion

4x This PR.
4x_tellowkrinkle_VertexLineExpand

4x #10295
4x_Pokechu22_no-gs-points-lines

@Pokechu22
Copy link
Contributor

#10295 is definitely incorrect here. FifoCI (our automated graphics difference detector) shows 10295 resulting in a lot of incorrect behaviors, while this pull request shows no differences.

@dvessel
Copy link
Contributor

dvessel commented Sep 19, 2022

Does FifoCI diff at native resolution? That’s what it looks like. It shouldn’t be affected when at 1x. It’s at higher resolution where it can become invisible.

This is from #10295 at 1x upscaled 400%.
1x_expand

@Pokechu22
Copy link
Contributor

FifoCI does use native resolution. The rain and minimap from 10295 both look wrong to me though (at both 1x and 4x resolution) while they look fine here (visible with metroid-visor).

@TellowKrinkle
Copy link
Contributor Author

I would love to see this merged. It’s diverged from #10781 which needs a minor update itself.

I'll probably rebase it once #10781 is merged. I also still need to test texture offsetting.

@TellowKrinkle TellowKrinkle force-pushed the VertexLineExpand branch 2 times, most recently from 8ed4b2f to 292eac2 Compare September 30, 2022 22:47
@TellowKrinkle
Copy link
Contributor Author

The best game to test with for this is the heat man fight in Mega Man Network Transmission (fifolog, fifoci example). Note that there's something with this that makes the stage background not get recorded into the fifolog, and the game itself is apparently quite difficult (I don't own it; this is what JMC said), so it can be a bit annoying to get there to test, but the actual line/point texture offset effects are visible with the fifolog.

NES virtual console games also use it, I think; at least, they use lines as a part of their rendering (which I don't fully understand). There are fifologs of SMB and Zelda 1.

Mega Man looks to be working correctly

Screenshots

Before:
image

After:
image

The two VC dffs seem to look correct both before and after

@TellowKrinkle TellowKrinkle marked this pull request as ready for review September 30, 2022 22:50
@dvessel
Copy link
Contributor

dvessel commented Oct 2, 2022

Found an interesting bug. These emblems are usually invisible. Also, the line/point expand checkbox doesn’t do anything. It’s always enabled.

GSWE64_2022-10-02_16-53-52

btw, RS2 won’t start until #11117 is merged.

@TellowKrinkle
Copy link
Contributor Author

TellowKrinkle commented Oct 3, 2022

Also, the line/point expand checkbox doesn’t do anything. It’s always enabled

There's a reason it's prefer VS for line/point expansion
On Metal/MVK, it isn't disableable since the alternative is no expansion
As it says on the tooltip, it only affects backends with support for both VS and GS expansion
Do you think I should remove the checkbox on macOS, where none of our backends support both? (OpenGL is GS only due to macOS's ancient driver not having enough OpenGL features, and MVK/Metal are VS only)

For the bug, can you supply a dff or is this a game that doesn't make working dffs?

Also, can you confirm that it's not just them being completely broken without line width support? Try running an older build under OpenGL and see if it has them (since OpenGL supports GS even on macOS).

@JMC47
Copy link
Contributor

JMC47 commented Oct 3, 2022

As far as I can tell, that looks correct.

@dvessel
Copy link
Contributor

dvessel commented Oct 3, 2022

Do you think I should remove the checkbox on macOS, where none of our backends support both? (OpenGL is GS only due to macOS's ancient driver not having enough OpenGL features, and MVK/Metal are VS only)

Yes, I think that’s more sensible when the option is either correct or not. We don’t need the option.

As far as I can tell, that looks correct.

I have not played it on real hardware in ages so I completely forgot about it. It was never supported on Vulkan/Metal on Mac’s so I haven’t seen it recently. So this fixes it! :D

@dvessel
Copy link
Contributor

dvessel commented Oct 3, 2022

It’s visible here. It was incomplete before and the PR fixes it. 😀 https://youtu.be/eFd8spzXqss?t=188

Screen Shot 2022-10-03 at 5 01 50 AM

@Pokechu22
Copy link
Contributor

I believe (though my experience is only in Dolphin and I haven't retested before commenting) that the other emblems go away when one is activated, but re-appear afterwards.

@JMC47
Copy link
Contributor

JMC47 commented Oct 20, 2022

Tested this is Rogue Squadron 3 and Mega Man Network Transmission with it forced on in Vulkan and OpenGL. Performance is roughly the same on desktop, but on Android Vulkan specifically, it's actually a little bit faster in Mega Man Network Transmission when forced on in the INI.

On my desktop, it might be 1 - 2% faster, but that could be random variation as well. It's definitely not leaning slower, though.

Source/Core/Core/State.cpp Show resolved Hide resolved
@@ -244,12 +255,14 @@ bool ObjectCache::CreatePipelineLayouts()
static_cast<u32>(compute_sets.size()), compute_sets.data(), 0, nullptr},
}};

const bool ssbos_in_standard =
g_ActiveConfig.backend_info.bSupportsBBox || g_ActiveConfig.UseVSForLinePointExpand();
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "standard"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ssbos_in_standard => PIPELINE_LAYOUT_STANDARD uses SSBOs

Copy link
Contributor

Choose a reason for hiding this comment

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

After looking at this further, yeah, this is reasonable and fairly obvious from context. We might want to change PIPELINE_LAYOUT_STANDARD to PIPELINE_LAYOUT_SPECIALIZED or something like that since we've started referring to the opposite of ubershaders as "specialized shaders", but that's not something that needs to be handled in this PR.

Source/Core/VideoCommon/IndexGenerator.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/ShaderCache.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/VertexManagerBase.cpp Outdated Show resolved Hide resolved
out.Write("ATTRIBUTE_LOCATION({}) in float4 rawcolor1;\n", SHADER_COLOR1_ATTRIB);

for (u32 i = 0; i < 8; ++i)
if (uid_data->vs_expand == VSExpand::None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to check host_config.backend_dynamic_vertex_loader, since that more directly explains what the difference in behavior is, even if we don't normally use the dynamic vertex loader with specialized shaders? I assume (but haven't checked) that host_config.backend_dynamic_vertex_loader is implicitly set to true if vs expansion is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't actually the case, because I leave backend_dynamic_vertex_loader false on APIs that didn't previously have issues with ubershader compilation stutter (OpenGL and DX11), even if they support VS expand.
This allows them to default to dynamic vertex loader disabled, but advertise support for VS expand, which will then enable dynamic vertex loader if the setting to use VS expand is enabled.
Do you think I should rename it to backend_prefers_dynamic_vertex_loader? Or rename the option in VideoConfig, and set backend_dynamic_vertex_loader to g_ActiveConfig.backend_info.bPrefersDynamicVertexLoader || g_ActiveConfig.UseVSForLinePointExpand()?

Also, in this specific case, backend_dynamic_vertex_loader only enables expansion for ubershaders. For non-uber shaders, we only need it for shaders that actually expand things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, in this specific case, backend_dynamic_vertex_loader only enables expansion for ubershaders. For non-uber shaders, we only need it for shaders that actually expand things.

From this, it's probably fine to leave the existing behavior alone. I don't really have an opinion on changing backend_dynamic_vertex_loader or not at this point.

@TellowKrinkle TellowKrinkle force-pushed the VertexLineExpand branch 3 times, most recently from c7ac6fe to 7d5c6f4 Compare October 21, 2022 20:20
@Pokechu22
Copy link
Contributor

Pokechu22 commented Oct 22, 2022

Could you disable the prefer checkbox when a backend that doesn't support both options is enabled? (See #11101 for an example). I'm also not sure if it actually needs to be disabled while emulation is running - is there anything that makes it conceptually difficult for the option to be changed at runtime?

Otherwise, this looks good to me.

Copy link
Contributor

@Pokechu22 Pokechu22 left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I haven't done much testing, though.

@TellowKrinkle
Copy link
Contributor Author

Could you disable the prefer checkbox when a backend that doesn't support both options is enabled? (See #11101 for an example).

I added the disabling along with a message to the tooltip for the reason why

I'm also not sure if it actually needs to be disabled while emulation is running - is there anything that makes it conceptually difficult for the option to be changed at runtime?

I just needed to add a change handler for the index generator to reload its functions. Should be working now, but I haven't tested it if anyone else wants to do so. If it doesn't work properly, you'll get a lot of broken geometry when changing it, so it should be pretty obvious.

@JMC47
Copy link
Contributor

JMC47 commented Oct 23, 2022

I tested this pull request and did a lot of switching while games were running. The first time it crashed (device lost) but it appeared to be a random crash related to Dolphin recompiling shaders, and I never reproduced it.

Tested this in the big scenarios. Rogue Squadron 2/3, Mega Man Network Transmission, and NES VC games. I didn't notice any differences in rendering or performance.

Considering the old implementation isn't going anywhere (yet), I'm fairly confident in merging this. Long term, this should probably replace GS Line-Width.

@JMC47 JMC47 merged commit cdcbe51 into dolphin-emu:master Oct 23, 2022
11 checks passed
@TellowKrinkle TellowKrinkle deleted the VertexLineExpand branch October 23, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants