d3dx9: d3dx9_base_effect_get_pass_desc

[Luis C. Busquets Pérez]

Dear developers,

I have tried to run Civ4BeyondtheSword.exe with the internal
d3dx9_33.dll

I can run the programme but when I start a game, it hangs. I saw that a
lot of FIXME's with the message:

fixme:d3dx:d3dx9_base_effect_get_pass_desc Pixel shader and vertex
shader are not supported, yet.

Therefore, I tried to finishing implementing the function. Find
below a patch that works. With this patch, the programme does not
hang. Fell free to study, redo it or rewrite it but please I look
forward to somebody who corrects it and manages to integrate that
function in the wine tree so that I do not need to compile the wine.

On the other hand, the settler image seems to something in the middle.
Furthermore, the leaders also seem to have something in the middle.
Actually, it seems that the image that should be behind the leader in
the bottom is in the middle of them and bend vertically when they bend.
I assume this is not because of the patch but I guess that this
information could serve to locate and correct another bug.

---
 dlls/d3dx9_36/effect.c | 46
++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
index 42f5aea..c69b73c 100644
--- a/dlls/d3dx9_36/effect.c
+++ b/dlls/d3dx9_36/effect.c
@@ -150,6 +150,7 @@ struct d3dx9_base_effect
     UINT parameter_count;
     UINT technique_count;
     UINT object_count;
+    BOOL clonable;
 
     struct d3dx_parameter *parameters;
     struct d3dx_technique *techniques;
@@ -999,6 +1000,11 @@ static HRESULT
d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
         D3DXHANDLE pass, D3DXPASS_DESC *desc)
 {
     struct d3dx_pass *p = get_valid_pass(base, pass);
+    struct d3dx_state *s;
+    struct d3dx_parameter *param;
+    IDirect3DVertexShader9 *vshader;
+    IDirect3DPixelShader9 *pshader;
+    UINT i, size;
 
     if (!desc || !p)
     {
@@ -1008,11 +1014,45 @@ static HRESULT
d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
 
     desc->Name = p->name;
     desc->Annotations = p->annotation_count;
-
-    FIXME("Pixel shader and vertex shader are not supported, yet.\n");
     desc->pVertexShaderFunction = NULL;
     desc->pPixelShaderFunction = NULL;
 
+    if (base->clonable)
+    {
+        for (i=0; i < p->state_count; i++)
+        {
+            s = p->states + i;
+            if (state_table[s->operation].class == SC_VERTEXSHADER)
+            {
+                param = &(s->parameter);
+                vshader = *(struct IDirect3DVertexShader9 **)param-
> data;

+                if (vshader)
+                {
+                    IDirect3DVertexShader9_GetFunction(vshader, NULL,
&size);
+                    if (desc->pVertexShaderFunction)
+                        free((void *)desc->pVertexShaderFunction);
+                    desc->pVertexShaderFunction = malloc(size);
+                    IDirect3DVertexShader9_GetFunction(vshader, (void
*)(desc->pVertexShaderFunction), &size);
+                }
+            }
+            if (state_table[s->operation].class == SC_PIXELSHADER)
+            {
+                param = &(s->parameter);
+                pshader = *(struct IDirect3DPixelShader9 **)param-
> data;

+                if (pshader)
+                {
+                    IDirect3DPixelShader9_GetFunction(pshader, NULL,
&size);
+                    if (desc->pPixelShaderFunction)
+                        free((void *)desc->pPixelShaderFunction);
+                    desc->pPixelShaderFunction = malloc(size);
+                    IDirect3DPixelShader9_GetFunction(pshader, (void
*)(desc->pPixelShaderFunction), &size);
+                }
+            }
+        }
+    }
+
+    TRACE("Pass '%s': %i annots. Vertexshader %p and Pixelshader
%p\n", desc->Name, desc->Annotations, desc->pVertexShaderFunction,
desc->pPixelShaderFunction);
+
     return D3D_OK;
 }
 
@@ -5955,6 +5995,8 @@ static HRESULT d3dx9_base_effect_init(struct
d3dx9_base_effect *base,
 
     base->effect = effect;
 
+    base->clonable = TRUE;
+
     read_dword(&ptr, &tag);
     TRACE("Tag: %x\n", tag);
 
-- 
2.7.4

[Luis C. Busquets Pérez]

Could anyone help improving this patch so that it makes it to the wine
tree?
> dlls/d3dx9_36/effect.c | 46
> ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> index 42f5aea..c69b73c 100644
> --- a/dlls/d3dx9_36/effect.c
> +++ b/dlls/d3dx9_36/effect.c
> @@ -150,6 +150,7 @@ struct d3dx9_base_effect
>      UINT parameter_count;
>      UINT technique_count;
>      UINT object_count;
> +    BOOL clonable;
>  
>      struct d3dx_parameter *parameters;
>      struct d3dx_technique *techniques;
> @@ -999,6 +1000,11 @@ static HRESULT
> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
>          D3DXHANDLE pass, D3DXPASS_DESC *desc)
>  {
>      struct d3dx_pass *p = get_valid_pass(base, pass);
> +    struct d3dx_state *s;
> +    struct d3dx_parameter *param;
> +    IDirect3DVertexShader9 *vshader;
> +    IDirect3DPixelShader9 *pshader;
> +    UINT i, size;
>  
>      if (!desc || !p)
>      {
> @@ -1008,11 +1014,45 @@ static HRESULT
> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
>  
>      desc->Name = p->name;
>      desc->Annotations = p->annotation_count;
> -
>
> -    FIXME("Pixel shader and vertex shader are not supported,
> yet.\n");
>      desc->pVertexShaderFunction = NULL;
>      desc->pPixelShaderFunction = NULL;
>  
> +    if (base->clonable)
> +    {
> +        for (i=0; i < p->state_count; i++)
> +        {
> +            s = p->states + i;
> +            if (state_table[s->operation].class == SC_VERTEXSHADER)
> +            {
> +                param = &(s->parameter);
> +                vshader = *(struct IDirect3DVertexShader9 **)param-
> > data;
>
> +                if (vshader)
> +                {
> +                    IDirect3DVertexShader9_GetFunction(vshader,
> NULL,
> &size);
> +                    if (desc->pVertexShaderFunction)
> +                        free((void *)desc->pVertexShaderFunction);
> +                    desc->pVertexShaderFunction = malloc(size);
> +                    IDirect3DVertexShader9_GetFunction(vshader,
> (void
> *)(desc->pVertexShaderFunction), &size);
> +                }
> +            }
> +            if (state_table[s->operation].class == SC_PIXELSHADER)
> +            {
> +                param = &(s->parameter);
> +                pshader = *(struct IDirect3DPixelShader9 **)param-
> > data;
>
> +                if (pshader)
> +                {
> +                    IDirect3DPixelShader9_GetFunction(pshader, NULL,
> &size);
> +                    if (desc->pPixelShaderFunction)
> +                        free((void *)desc->pPixelShaderFunction);
> +                    desc->pPixelShaderFunction = malloc(size);
> +                    IDirect3DPixelShader9_GetFunction(pshader, (void
> *)(desc->pPixelShaderFunction), &size);
> +                }
> +            }
> +        }
> +    }
> -
>
> +    TRACE("Pass '%s': %i annots. Vertexshader %p and Pixelshader
> %p\n", desc->Name, desc->Annotations, desc->pVertexShaderFunction,
> desc->pPixelShaderFunction);
> -
>
> return D3D_OK;
>  }
>  
> @@ -5955,6 +5995,8 @@ static HRESULT d3dx9_base_effect_init(struct
> d3dx9_base_effect *base,
>  
>      base->effect = effect;
>  
> +    base->clonable = TRUE;
> -
>
> read_dword(&ptr, &tag);
>      TRACE("Tag: %x\n", tag);
>  
> -- 
> 2.7.4

[Henri Verbeet]

On 29 October 2016 at 21:11, Luis C. Busquets Pérez
[email protected] wrote:
> Could anyone help improving this patch so that it makes it to the wine
> tree?

At first sight:
> > @@ -5955,6 +5995,8 @@ static HRESULT d3dx9_base_effect_init(struct
> > d3dx9_base_effect *base,
> >  base->effect = effect;
> >
> > - base->clonable = TRUE;
> > -

Did you perhaps intend to store "eflags" instead here?
> > - if (base->clonable)
> > - {

And then check "base->flags" against D3DXFX_NOT_CLONEABLE here.
> > -
> >            if (vshader)
> > -
> >            {
> > -
> >                IDirect3DVertexShader9_GetFunction(vshader,
> >
> > NULL,
> > &size);
> > -
> >                if (desc->pVertexShaderFunction)
> > -
> >                    free((void *)desc->pVertexShaderFunction);
> > -
> >                desc->pVertexShaderFunction = malloc(size);
> > -
> >                IDirect3DVertexShader9_GetFunction(vshader,
> >
> > (void
> > *)(desc->pVertexShaderFunction), &size);
> > -
> >            }

Why not move this out of the loop?
> > -
> >            if (pshader)
> > -
> >            {
> > -
> >                IDirect3DPixelShader9_GetFunction(pshader, NULL,
> >
> > &size);
> > -
> >                if (desc->pPixelShaderFunction)
> > -
> >                    free((void *)desc->pPixelShaderFunction);
> > -
> >                desc->pPixelShaderFunction = malloc(size);
> > -
> >                IDirect3DPixelShader9_GetFunction(pshader, (void
> >
> > *)(desc->pPixelShaderFunction), &size);
> > -
> >            }

Likewise.

[Matteo Bruni]

2016-10-29 21:11 GMT+02:00 Luis C. Busquets Pérez [email protected]:
> Could anyone help improving this patch so that it makes it to the wine
> tree?
> > dlls/d3dx9_36/effect.c | 46
> > ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> > index 42f5aea..c69b73c 100644
> > --- a/dlls/d3dx9_36/effect.c
> > +++ b/dlls/d3dx9_36/effect.c
> > @@ -150,6 +150,7 @@ struct d3dx9_base_effect
> >      UINT parameter_count;
> >      UINT technique_count;
> >      UINT object_count;
> > -
> >
> > BOOL clonable;
> >
> > struct d3dx_parameter *parameters;
> > struct d3dx_technique *techniques;
> >
> > @@ -999,6 +1000,11 @@ static HRESULT
> > d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
> >          D3DXHANDLE pass, D3DXPASS_DESC *desc)
> >  {
> >      struct d3dx_pass *p = get_valid_pass(base, pass);
> > -
> >
> > struct d3dx_state *s;
> > -
> >
> > struct d3dx_parameter *param;
> > -
> >
> > IDirect3DVertexShader9 *vshader;
> > -
> >
> > IDirect3DPixelShader9 *pshader;
> > -
> >
> > UINT i, size;
> >
> > if (!desc || !p)
> > {
> >
> > @@ -1008,11 +1014,45 @@ static HRESULT
> > d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
> >  desc->Name = p->name;
> >  desc->Annotations = p->annotation_count;
> > -
> > - FIXME("Pixel shader and vertex shader are not supported,
> >
> > yet.\n");
> >      desc->pVertexShaderFunction = NULL;
> >      desc->pPixelShaderFunction = NULL;
> > - if (base->clonable)
> > - {
> > -
> >    for (i=0; i < p->state_count; i++)
> > -
> >    {
> > -
> >        s = p->states + i;
> > -
> >        if (state_table[s->operation].class == SC_VERTEXSHADER)
> > -
> >        {
> > -
> >            param = &(s->parameter);
> > -
> >            vshader = *(struct IDirect3DVertexShader9 **)param-
> > > data;
> > -
> >            if (vshader)
> > -
> >            {
> > -
> >                IDirect3DVertexShader9_GetFunction(vshader,
> >
> > NULL,
> > &size);
> > -
> >                if (desc->pVertexShaderFunction)
> > -
> >                    free((void *)desc->pVertexShaderFunction);
> > -
> >                desc->pVertexShaderFunction = malloc(size);
> > -
> >                IDirect3DVertexShader9_GetFunction(vshader,
> >
> > (void
> > *)(desc->pVertexShaderFunction), &size);
> > -
> >            }
> > -
> >        }
> > -
> >        if (state_table[s->operation].class == SC_PIXELSHADER)
> > -
> >        {
> > -
> >            param = &(s->parameter);
> > -
> >            pshader = *(struct IDirect3DPixelShader9 **)param-
> > > data;
> > -
> >            if (pshader)
> > -
> >            {
> > -
> >                IDirect3DPixelShader9_GetFunction(pshader, NULL,
> >
> > &size);
> > -
> >                if (desc->pPixelShaderFunction)
> > -
> >                    free((void *)desc->pPixelShaderFunction);
> > -
> >                desc->pPixelShaderFunction = malloc(size);
> > -
> >                IDirect3DPixelShader9_GetFunction(pshader, (void
> >
> > *)(desc->pPixelShaderFunction), &size);
> > -
> >            }
> > -
> >        }
> > -
> >    }
> > - }

You probably want to take the shader code from the "objects" array instead.
Also that doesn't work for ST_ARRAY_SELECTOR vertex / pixel shader
states. It isn't even obvious what should be the returned value in
that case i.e. it needs tests.

I think Paul had a look at this at some point although I don't know if
there is any relevant patch attached to a bug or something.

[Paul Gofman]

No, I didn't test GetPassDesc shader return behaviour and did not
meet any application requiring it. But there is
test_effect_preshader_effect_blob in tests/effect.c which has a shader
chosen by selector which probably can be used for testing. I suppose the
checks for GetPassDesc could be inserted right into
test_effect_preshader() test which already tests shader switching
through selector. There is already a code which identifies a selected
shader, I hope the same can be used for checking GetPassDesc return.
    I don't know if Matteo will like this idea but the simplest way I
see to determine the shader actually selected for pass is to store
pointer to it right from d3dx9_apply_state function (in case
SC_VERTEXSHADER: and case SC_PIXELSHADER:), as at this place correct
shader is determined based on selector value (if selector is used). I
can't be sure without testing though what GetPassDesc should return
before BeginPass() was called (that is, before any apply_state was
called), and then  after BeginPass/EndPass was called (will it be last
selected? Or first shaders' array element? Or based on the current
selector value??). Probably this needs to be tested before deciding on
implementation.

On 10/31/2016 06:29 PM, Matteo Bruni wrote:
> 2016-10-29 21:11 GMT+02:00 Luis C. Busquets Pérez [email protected]:
> > Could anyone help improving this patch so that it makes it to the wine
> > tree?
> > > dlls/d3dx9_36/effect.c | 46
> > > ++++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 44 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> > > index 42f5aea..c69b73c 100644
> > > --- a/dlls/d3dx9_36/effect.c
> > > +++ b/dlls/d3dx9_36/effect.c
> > > @@ -150,6 +150,7 @@ struct d3dx9_base_effect
> > >      UINT parameter_count;
> > >      UINT technique_count;
> > >      UINT object_count;
> > > -
> > >
> > > BOOL clonable;
> > >
> > > struct d3dx_parameter *parameters;
> > > struct d3dx_technique *techniques;
> > >
> > > @@ -999,6 +1000,11 @@ static HRESULT
> > > d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
> > >          D3DXHANDLE pass, D3DXPASS_DESC *desc)
> > >  {
> > >      struct d3dx_pass *p = get_valid_pass(base, pass);
> > > -
> > >
> > > struct d3dx_state *s;
> > > -
> > >
> > > struct d3dx_parameter *param;
> > > -
> > >
> > > IDirect3DVertexShader9 *vshader;
> > > -
> > >
> > > IDirect3DPixelShader9 *pshader;
> > > -
> > >
> > > UINT i, size;
> > >
> > > if (!desc || !p)
> > > {
> > >
> > > @@ -1008,11 +1014,45 @@ static HRESULT
> > > d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base,
> > >  desc->Name = p->name;
> > >  desc->Annotations = p->annotation_count;
> > > -
> > > - FIXME("Pixel shader and vertex shader are not supported,
> > >
> > > yet.\n");
> > >      desc->pVertexShaderFunction = NULL;
> > >      desc->pPixelShaderFunction = NULL;
> > > - if (base->clonable)
> > > - {
> > > -
> > >    for (i=0; i < p->state_count; i++)
> > > -
> > >    {
> > > -
> > >        s = p->states + i;
> > > -
> > >        if (state_table[s->operation].class == SC_VERTEXSHADER)
> > > -
> > >        {
> > > -
> > >            param = &(s->parameter);
> > > -
> > >            vshader = *(struct IDirect3DVertexShader9 **)param-
> > > > data;
> > > -
> > >            if (vshader)
> > > -
> > >            {
> > > -
> > >                IDirect3DVertexShader9_GetFunction(vshader,
> > >
> > > NULL,
> > > &size);
> > > -
> > >                if (desc->pVertexShaderFunction)
> > > -
> > >                    free((void *)desc->pVertexShaderFunction);
> > > -
> > >                desc->pVertexShaderFunction = malloc(size);
> > > -
> > >                IDirect3DVertexShader9_GetFunction(vshader,
> > >
> > > (void
> > > *)(desc->pVertexShaderFunction), &size);
> > > -
> > >            }
> > > -
> > >        }
> > > -
> > >        if (state_table[s->operation].class == SC_PIXELSHADER)
> > > -
> > >        {
> > > -
> > >            param = &(s->parameter);
> > > -
> > >            pshader = *(struct IDirect3DPixelShader9 **)param-
> > > > data;
> > > -
> > >            if (pshader)
> > > -
> > >            {
> > > -
> > >                IDirect3DPixelShader9_GetFunction(pshader, NULL,
> > >
> > > &size);
> > > -
> > >                if (desc->pPixelShaderFunction)
> > > -
> > >                    free((void *)desc->pPixelShaderFunction);
> > > -
> > >                desc->pPixelShaderFunction = malloc(size);
> > > -
> > >                IDirect3DPixelShader9_GetFunction(pshader, (void
> > >
> > > *)(desc->pPixelShaderFunction), &size);
> > > -
> > >            }
> > > -
> > >        }
> > > -
> > >    }
> > > - }
>
> You probably want to take the shader code from the "objects" array instead.
> Also that doesn't work for ST_ARRAY_SELECTOR vertex / pixel shader
> states. It isn't even obvious what should be the returned value in
> that case i.e. it needs tests.
>
> I think Paul had a look at this at some point although I don't know if
> there is any relevant patch attached to a bug or something.