VideoCommon: fix uint shader compiler error when using d3d

[iwubcode]

When doing some testing on a separate PR, I noticed that the NES Legend of Zelda (on the Gamecube Collector's Edition) would give a shader compiler error when using D3D.

The compiler error was:

ERROR: 0:160: 'assign' : cannot convert from ' const 4-component vector of float' to 'layout( location=0 index=0) out 4-component vector of uint'

This PR fixes that error.

[Pokechu22]

I'm a bit confused why the same initialization isn't done with ocol1 (see also b573319 / #6031) and why it's being restricted to D3D - I guess other platforms might allow implicit conversion from vec4 to uvec4 (also odd that we're using those instead of the uint4/float4 aliases we define), but it seems like it would make sense to always assign with uint4 when uint_output is in use.

[iwubcode]

I'm a bit confused why the same initialization isn't done with ocol1

@Pokechu22 - actually, I think it should be. I believe before we didn't need it because d3d was either uint with ocol0 only or float with both ocol0 and ocol1. When I scrapped hlsl, I just made both ocol0 and ocol1 be uint if uint was defined. I'm not sure why the original hlsl code didn't allow uint for ocol1 as well.

why it's being restricted to D3D

See this comment:

https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/VideoCommon/PixelShaderGen.cpp#L1808

I guess other platforms might allow implicit conversion from vec4 to uvec4

I can't find it now but I recall seeing a comment related to that idea.

--

And just in general, as I've been reviewing @TellowKrinkle 's code, I've noticed our pixel shader has a lot of holes. Places where it should be this or that but we instead we do if this and if that.

[Pokechu22]

uint_output will always be false on non-D3D backends, though:

dolphin/Source/Core/VideoCommon/PixelShaderGen.cpp

Lines 311 to 320 in fa30ba1

	void ClearUnusedPixelShaderUidBits(APIType api_type, const ShaderHostConfig& host_config,
	PixelShaderUid* uid)
	{
	pixel_shader_uid_data* const uid_data = uid->GetUidData();
	// OpenGL and Vulkan convert implicitly normalized color outputs to their uint representation.
	// Therefore, it is not necessary to use a uint output on these backends. We also disable the
	// uint output when logic op is not supported (i.e. driver/device does not support D3D11.1).
	if (api_type != APIType::D3D || !host_config.backend_logic_op)
	uid_data->uint_output = 0;

IMO it'd be clearer to remove the extra D3D since if ocol0 is a uint, it'd always be best to write to it as a uint instead of only on D3D. (And the code that defines ocol0 doesn't check D3D (it may have in the past?)).

[Pokechu22]

It looks like UberShaderPixel still has a case of (api_type == APIType::D3D && uid_data->uint_output). I don't think it'll generate a warning but it still might be a good idea to change those.

[iwubcode]

@Pokechu22 - good catch. Sorry, I tend to focus on the specialized shaders. I'll fix that too

[Pokechu22]

I'm still a bit confused about ocol1, as it's initialized in 2 places and one is always float4:

dolphin/Source/Core/VideoCommon/PixelShaderGen.cpp

Lines 848 to 854 in 7b2b559

	if (!uid_data->no_dual_src)
	{
	out.Write("{} out {} ocol1;\n",
	has_broken_decoration ? "FRAGMENT_OUTPUT_LOCATION(1)" :
	"FRAGMENT_OUTPUT_LOCATION_INDEXED(0, 1)",
	uid_data->uint_output ? "uvec4" : "vec4");
	}

dolphin/Source/Core/VideoCommon/PixelShaderGen.cpp

Lines 917 to 920 in 7b2b559

	if (uid_data->blend_enable)
	{
	out.Write("\tfloat4 ocol1;\n");
	}

and WriteColor always uses float4 for ocol1, but uid_data->uint_output doesn't ever write ocol1. This is probably something to handle in a separate PR, though. For now, this looks good to me.

[iwubcode]

@Pokechu22 - agreed. I think there are some scenarios to resolve (some of these seem like they aren't hittable, which if so, means the code should be written to reflect that) but I was just aiming to fix the bug in this PR. Appreciate the review!

[TellowKrinkle]

I'm still a bit confused about ocol1, as it's initialized in 2 places and one is always float4:

Since ocol1 is used for blending, while uint_output is used for logic ops, they should never both be enabled at once

Maybe put in an assertion for that?