Re: [v5 8/8] d3dx9: Implement setting named shader constants in effect.

[Matteo Bruni]

2016-03-21 14:55 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
> +HRESULT d3dx_set_shader_const_fxlc(struct ID3DXEffectImpl *effect, struct d3dx_pass *pass,
> +        struct d3dx_parameter *param, unsigned int vs)

Using a BOOL for the vs flag seems nicer.
> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
> index fec9de4..1770046 100644
> --- a/dlls/d3dx9_36/preshader.c
> +++ b/dlls/d3dx9_36/preshader.c
> @@ -242,7 +242,7 @@ static void regstore_set_values(struct d3dx_regstore *rs, unsigned int table, vo
>  static unsigned int regstore_is_val_set_reg(struct d3dx_regstore *rs, unsigned int table, unsigned int reg_idx)
>  {
>      return rs->table_value_set[table][reg_idx / PRES_BITMASK_BLOCK_SIZE] &
> -            (1ul << (reg_idx % PRES_BITMASK_BLOCK_SIZE));
> +            (1u << (reg_idx % PRES_BITMASK_BLOCK_SIZE));

When you end up changing stuff introduced in a previous patch in the
series you know there is something wrong ;)
I guess it's a rebase glitch (by any chance, was this a remnant of a
try with the uint_ptr idea you mentioned earlier?)
> +HRESULT d3dx_param_eval_set_shader_constants(struct IDirect3DDevice9 *device, struct d3dx_param_eval *peval)
> +{
> +    HRESULT hr, res;
> +    struct d3dx_preshader *pres = &peval->pres;
> +    struct d3dx_regstore *rs = &pres->regs;
> +
> +    TRACE("device %p, peval %p, param_type %u.\n", device, peval, peval->param_type);
> +
> +    if (FAILED(hr = set_constants(rs, &pres->inputs)))
> +        return hr;
> +    if (FAILED(hr = execute_preshader(pres)))
> +        return hr;
> +    if (FAILED(hr = set_constants(rs, &peval->shader_inputs)))
> +        return hr;
> +
> +    #define SET_TABLE(table, func_suff, type) \
> +        { \
> +            unsigned int is, n; \
> +            \
> +            is = 0; \
> +            while (is < rs->table_sizes[table]) \
> +            {\
> +                n = 0; \
> +                while (is < rs->table_sizes[table] && !regstore_is_val_set_reg(rs, table, is)) \
> +                    is++; \
> +                while (is + n < rs->table_sizes[table] && regstore_is_val_set_reg(rs, table, is + n)) \
> +                    n++; \
> +                if (!n) \
> +                    continue; \
> +                TRACE("setting %u consts at %u.\n", n, is); \
> +                hr = IDirect3DDevice9_##func_suff(device, is, \
> +                        (const type *)rs->tables[table] + is * table_info[table].reg_component_count, n); \
> +                if (FAILED(hr)) \
> +                { \
> +                    ERR(#func_suff " failed, hr %#x.\n", hr); \
> +                    res = hr; \
> +                } \
> +                is += n; \
> +            } \
> +            regstore_reset_table(rs, table); \
> +        }
> +    res = D3D_OK;
> +    if (peval->param_type == D3DXPT_VERTEXSHADER)
> +    {
> +        SET_TABLE(PRES_REGTAB_OCONST, SetVertexShaderConstantF, float);
> +        SET_TABLE(PRES_REGTAB_OICONST, SetVertexShaderConstantI, int);
> +        SET_TABLE(PRES_REGTAB_OBCONST, SetVertexShaderConstantB, BOOL);
> +    }
> +    else if (peval->param_type == D3DXPT_PIXELSHADER)
> +    {
> +        SET_TABLE(PRES_REGTAB_OCONST, SetPixelShaderConstantF, float);
> +        SET_TABLE(PRES_REGTAB_OICONST, SetPixelShaderConstantI, int);
> +        SET_TABLE(PRES_REGTAB_OBCONST, SetPixelShaderConstantB, BOOL);
> +    }
> +    else
> +    {
> +        FIXME("Unexpected parameter type %u.\n", peval->param_type);
> +        return D3DERR_INVALIDCALL;
> +    }
> +    #undef SET_TABLE

This is certainly hard to handle nicely because of the multiple
similar d3d9 functions with slightly different signatures but I think
it's better to avoid macros (especially such large ones) as much as
possible.

There are multiple ways to achieve that. Perhaps the easiest one is to
turn SET_TABLE into a function taking an array of enum PRES_REG_TABLES
(which, btw, should be lowercase) and the shader type as additional
parameters, loop over the tables and call the correct d3d9 function by
using two levels of switches (or ifs). The type isn't really
necessary, you can calculate the offset with
table_info[].component_size.

[v5 8/8] d3dx9: Implement setting named shader constants in effect.

[Paul Gofman]

On 03/23/2016 01:09 AM, Matteo Bruni wrote:
> > diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
> > index fec9de4..1770046 100644
> > --- a/dlls/d3dx9_36/preshader.c
> > +++ b/dlls/d3dx9_36/preshader.c
> > @@ -242,7 +242,7 @@ static void regstore_set_values(struct d3dx_regstore *rs, unsigned int table, vo
> >  static unsigned int regstore_is_val_set_reg(struct d3dx_regstore *rs, unsigned int table, unsigned int reg_idx)
> >  {
> >      return rs->table_value_set[table][reg_idx / PRES_BITMASK_BLOCK_SIZE] &
> > -            (1ul << (reg_idx % PRES_BITMASK_BLOCK_SIZE));
> > +            (1u << (reg_idx % PRES_BITMASK_BLOCK_SIZE));
> > When you end up changing stuff introduced in a previous patch in the
> > series you know there is something wrong ;)
> > I guess it's a rebase glitch (by any chance, was this a remnant of a
> > try with the uint_ptr idea you mentioned earlier?)

Yes, I put uint_ptr in, then realized that it would require an extra
include to the common header file, or using ULONG_PTR instead (for which
I was not sure if it is decent enough :), so I gave up as the difference
introduced by this looked not important really. So I got back in patch
stack and changed 1ul's, which were introduced in two different patches,
and took a wrong (later) patch for the second change. So I saw 1u in the
final file and I did not notice this mess in between, sorry.

[v5 8/8] d3dx9: Implement setting named shader constants in effect.

[Matteo Bruni]

2016-03-22 23:48 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
> On 03/23/2016 01:09 AM, Matteo Bruni wrote:
> > > diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
> > > index fec9de4..1770046 100644
> > > --- a/dlls/d3dx9_36/preshader.c
> > > +++ b/dlls/d3dx9_36/preshader.c
> > > @@ -242,7 +242,7 @@ static void regstore_set_values(struct d3dx_regstore *rs, unsigned int table, vo
> > >  static unsigned int regstore_is_val_set_reg(struct d3dx_regstore *rs, unsigned int table, unsigned int reg_idx)
> > >  {
> > >      return rs->table_value_set[table][reg_idx / PRES_BITMASK_BLOCK_SIZE] &
> > > -            (1ul << (reg_idx % PRES_BITMASK_BLOCK_SIZE));
> > > +            (1u << (reg_idx % PRES_BITMASK_BLOCK_SIZE));
> > > When you end up changing stuff introduced in a previous patch in the
> > > series you know there is something wrong ;)
> > > I guess it's a rebase glitch (by any chance, was this a remnant of a
> > > try with the uint_ptr idea you mentioned earlier?)
>
> Yes, I put uint_ptr in, then realized that it would require an extra
> include to the common header file, or using ULONG_PTR instead (for which
> I was not sure if it is decent enough :), so I gave up as the difference
> introduced by this looked not important really. So I got back in patch
> stack and changed 1ul's, which were introduced in two different patches,
> and took a wrong (later) patch for the second change. So I saw 1u in the
> final file and I did not notice this mess in between, sorry.

No problem at all :)