GS:HW: Unify TFX shaders among backends. - #14850
Conversation
2ff34a0 to
c067222
Compare
c067222 to
8bea2fa
Compare
There was a problem hiding this comment.
Had a look though only the PS stuff
I was comparing against DX and VK shaders and seeing what differed code wise
I did notice that some of our constants vary across backends
I.e. 255.5f vs 255.0f, value255.5f instead of (value255f)+0.5f. etc
Perhaps we should consider what should be used instead of just using what Metal does, given that Metal is the least used backend and thus not as battle tested.
| { | ||
| FLOAT4 num = FLOAT4(0.0f, 0.0f, 0.0f, 0.0f); | ||
| FLOAT2 segment = (2.0f * aniso_line) / aniso_ratio; | ||
| for (int i = 0; i < aniso_ratio; i++) |
There was a problem hiding this comment.
You should bring back int aniso_ratio_i = (int)aniso_ratio;
FXC (the DX11 and 12 shader compiler) will perform an float to int conversion each loop if we don't
| t = fetch_c(state, uv) * 255.f; | ||
| } | ||
|
|
||
| // macOS 10.15 ICE's on bool3(t.rgb), so use != 0 instead |
There was a problem hiding this comment.
Is this still true for our min macOS 11?
There was a problem hiding this comment.
This may need feedback from @TellowKrinkle
|
|
||
| FLOAT4 fetch_red(IN_PARAM(PSMainState, state)) | ||
| { | ||
| float rt = PS_TEX_IS_DEPTH ? float(fetch_raw_depth(state) & 0xFFu) / 255.f : fetch_raw_color(state).r; |
There was a problem hiding this comment.
PS_TEX_IS_DEPTH vs (PS_DEPTH_FMT == 1) || (PS_DEPTH_FMT == 2) (as in DX code)?
Is the intention that we should fetch raw colour when PS_TALES_OF_ABYSS_HLE or PS_URBAN_CHAOS_HLE are true even when PS_DEPTH_FMT is 1 or 2?
ditto for the other fetch_colours
There was a problem hiding this comment.
It seems that the PS_TEX_IS_DEPTH definition PS_URBAN_CHAOS_HLE != FALSE || PS_TALES_OF_ABYSS_HLE != FALSE || PS_DEPTH_FMT == 1 || PS_DEPTH_FMT == 2 is slightly redundant. In GSRendererHW::EmulateChannelShuffle() the two HLE macros are only set when the source texture is depth-like, which I believe will mean that PS_DEPTH_FMT > 0 is set in GSRendererHW::EmulateTextureSampler(). The shader code seems to assume that PS_DEPTH_FMT == 3 (depth converted to RGBA) and a HLE macro can never happen at the same time since we use fetch_raw_depth() in those paths in sample_depth().
TLDR: It seems that we expect that PS_TEX_IS_DEPTH is always equivalent to PS_DEPTH_FMT == 1 || PS_DEPTH_FMT == 2.
It may be good if we add an assertion on the C++ side to ensure this, and fix things if it appears to not be true.
lightningterror
left a comment
There was a problem hiding this comment.
Currently gl is broken:
[ 38.2015] 0(489) : error C0000: syntax error, unexpected identifier, expecting reserved word or reserved word "in" or reserved word "out" or reserved word "uniform" at token "buffer"
[ 38.2018] 0(501) : error C0000: syntax error, unexpected identifier, expecting reserved word or reserved word "in" or reserved word "out" or reserved word "uniform" at token "buffer"
[ 38.2021] 0(511) : error C1503: undefined variable "index_buffer"
[ 38.2026] 0(516) : error C1503: undefined variable "vertex_buffer"
[ 38.2029] 0(548) : error C1503: undefined variable "index_buffer"
[ 38.2033] 0(553) : error C1503: undefined variable "vertex_buffer"
I also did a pass over the PS code and found a couple few other places that had some differences:
We should probably use a consistent constants throughout the shader, though for now it may be easier to match DX/VK to make checking the dump runs easier. |
…fx.glsl on master.
7037169 to
d7e4176
Compare
Might be driver specific as I'm unable to repro on my end. The lines causing the issue appear to be the ones with but just to be sure could I have the full shader that is triggering the error? |
|
I had disable vertex shader expand toggled, that should break it. |
Status: draft until tested/reviewed.
Description of Changes
Unifies backend TFX (i.e. GS emulation) shaders:
tfx_defs.inc,tfx_vs.inc,tfx_ps.inc.tfx.fx(DX11/12),tfx.glsl(GL/VK),tfx.metal(Metal).The generic code was based on
tfx.metal, as it uses dynamic branch for selectors, which can be used later for spec constants in VK and uber shaders in all backends.Metal likely doesn't compile yet and needs some work.
Outline for this approach was partly due to @TellowKrinkle.
Details
ps_main()into a new helperps_shuffle().PS_CHANNEL_FETCHtoPS_CHANNELandPS_ANISOTRIPIC_FILTERINGtoPS_SW_ANSIOon non-Metal backends.VS_PROVOING_VERTEX_LASTmacro in VK (was unused).GS_) since we no longer use geometry shaders.max_depthconstants in VS and PS tomax_depth_vsandmax_depth_ps, since GL has issues with constants with the same name in different constant buffers.bin/resources/shaders/dx11tobin/resources/shaders/dx, since it's used for both DX11/12.bin/resources/shaders/vulkan_openglfolder since the language specific TFX shader code is now shared between VK/GL.Todo:
Rationale behind Changes
Reduce the amount of duplicated code in TFX shaders and make it easier to make changes.
Possible drawbacks:
Suggested Testing Steps
Use any HW renderer and make sure it works identically to master.
We should make sure there aren't any performance impacts on GPU or shader compilation.
So far, I've checked a couple optimized DX12/VK vertex/pixels shaders in renderdoc and they seem to be around the same size as master.
Did you use AI to help find, test, or implement this issue or feature?
Likely to ask general programming questions about C++, shading languages, etc.