[Bug 45468] New: Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

Bug ID: 45468
           Summary: Phase Shift: tails on sustain notes not visible unless
                    GLSL is disabled
           Product: Wine
           Version: 3.12
          Hardware: x86
                OS: Linux
            Status: UNCONFIRMED
          Severity: minor
          Priority: P2
         Component: directx-d3d
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

Created attachment 61811
  --> https://bugs.winehq.org/attachment.cgi?id=61811
missing tail by default

During a song the notes that should be held (sustain notes) should have tails
rendered after the note indicating the hold. These tails are not visible by
default under wine. There are no visible artifacts. The notes instead appear
like normal, non-sustain notes.

Steps to reproduce:
* Install version 1.27 of Phase Shift
(http://www.dwsk.co.uk/phase_shift_downloads.html,
https://mega.co.nz/#!JkwSxayT!cKdLVz8GrRrla2jkDmOvlMWxtmsdbWvoboP_vmgLw3o)
* Run 'winetricks vcrun2013' to fix missing msvcr120.dll._strerror_s on startup
* Select Quickplay in main menu (use number 1 key to select)
* Select song Positive Force, a song that comes bundled with the full 1.27
download, or any other song with sustain notes
* Press 1 to join with keyboard
* Select Guitar intrument and Hard difficulty, or any other song and difficulty
combo with sustain notes
* Press 1 to continue and 1 again when prompted to press green to begin
* Observe that the first note of Positive Force should have a green tail
(follow by grey if the note is missed), but instead appears as a normal,
non-sustain note

The tails render properly after setting UseGLSL to disabled and MaxVersionGL to
0x30001 (cannot disable GLSL with GL 3.2 or newer).

I think this has something to do with how the generated ffp emulating shaders
calculate the alpha channel and apply an alpha test. Here is what I am seeing
leading to the pixels being discarded:

# relevant uniforms

specular_enable = [1, 1, 1, 0]
alpha_test_ref = 0.0117647061124444
ffp_material.diffuse = [0, 0, 0, 0]

# relevant bits of vertex shader

ffp_varying_diffuse.w = ffp_material.diffuse.w;

# relevant bits of fragment shader

// ffp_varying_diffuse.w = 0  ->  ret.w = 0 regardless of tex0.w
ret.w = tex0.w * ffp_varying_diffuse.w;

// specular_enable.w = 0 and ret.w = 0  ->  ps_out[0].a = 0
ps_out[0] = ffp_varying_specular * specular_enable + ret;

// ps_out[0].a = 0, alpha_test_ref = 0.01..., alpha test fails and discards
pixel
if (!(ps_out[0].a >= alpha_test_ref))
    discard;

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #1 from Andy Clayton  ---
Created attachment 61812
  --> https://bugs.winehq.org/attachment.cgi?id=61812
has tail when glsl is disabled

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #2 from Andy Clayton  ---
Created attachment 61813
  --> https://bugs.winehq.org/attachment.cgi?id=61813
full vertex shader

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #3 from Andy Clayton  ---
Created attachment 61814
  --> https://bugs.winehq.org/attachment.cgi?id=61814
full fragment shader

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #4 from Henri Verbeet  ---
Possibly, although I don't immediately see a reason to think it would be
incorrect to discard the fragment in that case.

Since you seem to have done some debugging on this already, does it help if you
simply don't generate the alpha-test code? (That's in
shader_glsl_generate_alpha_test().) What is the value of WINED3D_RS_ALPHAREF
set by the application? The value of alpha_test_ref would suggest "3".

Does it also help to set MaxVersionGL without disabling GLSL? When not using
core contexts, we use the fixed function alpha test, instead of doing it in the
shader.

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #5 from Andy Clayton  ---
Henri: thanks so much for reading and helping.

You are absolutely right about the discard being valid. I messed up. Even
without the discard nothing is rendered since ret.w always equals 0 and
blending is enabled with glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA).

It then makes sense that removing the alpha-test shader code does not help.
Setting MaxVersionGL while leaving GLSL enabled does remove the alpha-test code
but also does not help. You are correct that the value of WINED3D_RS_ALPHAREF
is 3.

It does help to change the ret.w assign from `ret.w = tex0.w *
ffp_varying_diffuse.w;` to `ret.w = tex0.w;`.

I am just picking gl and d3d up again so I have to do some more reading and
experiments to have a guess at what is going on with ffp_varying_diffuse's
alpha. Or if you have any more insights to share that would also be great. I
have been meaning to attach logs but need to cut them down to a reasonable size
without losing anything important.

Thanks again for looking and sorry for my confusion.

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #6 from Henri Verbeet  ---
(In reply to Andy Clayton from comment #5)
> I am just picking gl and d3d up again so I have to do some more reading and
> experiments to have a guess at what is going on with ffp_varying_diffuse's
> alpha. Or if you have any more insights to share that would also be great. I
> have been meaning to attach logs but need to cut them down to a reasonable
> size without losing anything important.

With GLSL disabled, you should be getting ARB_fragment_program shaders for FFP
(fragment) emulation. Those should be equivalent to the GLSL code, but it may
make sense to explicitly verify that.

We do use fixed-function vertex processing with GLSL disabled. The code for
setting that up is select_vertex_implementation(),
select_fragment_implementation() and select_shader_backend(). It should be
possible to use fixed-function vertex processing together with the GLSL shader
backend by returning "&ffp_vertex_pipe" in select_vertex_implementation(). It
may be worth giving that a try, but note that that combination hasn't been used
for several years now; it may not work at all anymore.

ffp_material.diffuse ultimately comes from the current material, set by
wined3d_device_set_material(). It's probably also a good idea to check the
ambient and emissive light and material values, particularly the alpha values.
Alpha is usually set to 0.0f for those, but I'm not entirely confident we
handle non-zero alpha values correctly there.

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #7 from Andy Clayton  ---
(In reply to Henri Verbeet from comment #6)
> ffp_material.diffuse ultimately comes from the current material, set by
> wined3d_device_set_material()

Do you think somehow WINED3D_RS_DIFFUSEMATERIALSOURCE = WINED3D_MCS_COLOR1
could be getting ignored? In the logs I see DIFFUSEMATERIALSOURCE set to
COLOR1:

0009:trace:d3d:wined3d_device_set_render_state device 0xd0fae0, state
WINED3D_RS_DIFFUSEMATERIALSOURCE (0x91), value 0x1.

And with fixed-function vertex processing wine issues
glColorMaterial(GL_FRONT_AND_BACK, GL_DIFFUSE) prior to the relevant
glDrawArrays().

Based on shader_glsl_ffp_mcs() it seems like if WINED3D_MCS_COLOR1 is applied
then the GLSL vertex shader would be

ffp_varying_diffuse.xyz = ffp_material.ambient.xyz * ambient +
ffp_attrib_diffuse.xyz * diffuse + ffp_material.emissive.xyz;
    ffp_varying_diffuse.w = ffp_attrib_diffuse.w;

rather than the WINED3D_MCS_MATERIAL/ffp_material.diffuse variant being used.

Changing the shader to use ffp_attrib_diffuse does result in the tails
appearing.

With GLSL there is a glVertexAttribPointer() call for location 5, which the
shader assigns to ffp_attrib_diffuse, so there are unused values being passed.
I did a little looking at the condition in wined3d_ffp_get_vs_settings(), but I
do see a trace from wined3d_stream_info_from_declaration() which seems to say
that the use_map flag FFP_DIFFUSE = 5 is getting flipped:

0009:trace:d3d:wined3d_stream_info_from_declaration Load fixed function
array 5 [usage WINED3D_DECL_USAGE_COLOR, usage_idx 0, input_slot 0, offset 24,
stride 36, format WINED3DFMT_B8G8R8A8_UNORM, class
WINED3D_INPUT_PER_VERTEX_DATA, step_rate 0].

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #8 from Henri Verbeet  ---
(In reply to Andy Clayton from comment #7)
> Do you think somehow WINED3D_RS_DIFFUSEMATERIALSOURCE = WINED3D_MCS_COLOR1
> could be getting ignored? In the logs I see DIFFUSEMATERIALSOURCE set to
> COLOR1:

That sounds plausible, but the question would be why.
> Based on shader_glsl_ffp_mcs() it seems like if WINED3D_MCS_COLOR1 is
> applied then the GLSL vertex shader would be
>
> ffp_varying_diffuse.xyz = ffp_material.ambient.xyz * ambient +
> ffp_attrib_diffuse.xyz * diffuse + ffp_material.emissive.xyz;
>    ffp_varying_diffuse.w = ffp_attrib_diffuse.w;
>
> rather than the WINED3D_MCS_MATERIAL/ffp_material.diffuse variant being used.

Right.
> passed. I did a little looking at the condition in
> wined3d_ffp_get_vs_settings(), but I do see a trace from
> wined3d_stream_info_from_declaration() which seems to say that the use_map
> flag FFP_DIFFUSE = 5 is getting flipped:

What does wined3d_ffp_get_vs_settings() end up setting
"settings->diffuse_source" to? Is that value used correctly by
shader_glsl_generate_ffp_vertex_shader()?

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #9 from Andy Clayton  ---
It (In reply to Henri Verbeet from comment #8)
> What does wined3d_ffp_get_vs_settings() end up setting
> "settings->diffuse_source" to? Is that value used correctly by
> shader_glsl_generate_ffp_vertex_shader()?

It looks like it does not get that far. set_glsl_shader_program() reuses
"ctx_data->glsl_program->vs" because the vertex shader hasn't been invalidated
in "shader_update_mask". And the prior draw happened to have a vertex
declaration without color/diffuse data, so the shader that carries over is
based on the wined3d_ffp_get_vs_settings() of a use_map without
WINED3D_FFP_DIFFUSE.

Throwing "context->shader_update_mask |= 1u << WINED3D_SHADER_TYPE_VERTEX;" in
glsl_vertex_pipe_vdecl() is enough for the tails to display. Would storing and
comparing "use_map" similar to "swizzle_map" make any sense?

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #10 from Henri Verbeet  ---
(In reply to Andy Clayton from comment #9)
> It looks like it does not get that far. set_glsl_shader_program() reuses
> "ctx_data->glsl_program->vs" because the vertex shader hasn't been
> invalidated in "shader_update_mask". And the prior draw happened to have a
> vertex declaration without color/diffuse data, so the shader that carries
> over is based on the wined3d_ffp_get_vs_settings() of a use_map without
> WINED3D_FFP_DIFFUSE.

Ah, that makes sense.
> Throwing "context->shader_update_mask |= 1u << WINED3D_SHADER_TYPE_VERTEX;"
> in glsl_vertex_pipe_vdecl() is enough for the tails to display. Would
> storing and comparing "use_map" similar to "swizzle_map" make any sense?

I think so.

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #11 from Henri Verbeet  ---
Created attachment 61921
  --> https://bugs.winehq.org/attachment.cgi?id=61921
patch

Does the attached patch help?

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

--- Comment #12 from Andy Clayton  ---
(In reply to Henri Verbeet from comment #11)
> Does the attached patch help?

It does!

Thanks so much for your help with this.

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

Henri Verbeet  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |5e3866cbdbbb63980eed056784d
                   |                            |afaf58d6adff5
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #13 from Henri Verbeet  ---
Should be fixed by commit 5e3866cbdbbb63980eed056784dafaf58d6adff5.

[Bug 45468] Phase Shift: tails on sustain notes not visible unless GLSL is disabled

https://bugs.winehq.org/show_bug.cgi?id=45468

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #14 from Alexandre Julliard  ---
Closing bugs fixed in 3.15.