[v3 resend 2/6] d3dx9: add preshaders to effect.

[Matteo Bruni]

2016-03-10 19:50 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>

Looks pretty good to me in general. I also like d3dx_param_eval and
related names.

> +        for (i = 0; i < param->element_count; i++)
> +        {
> +            if (param->members[i].type != param->type)
> +            {
> +                FIXME("Unexpected member parameter type %u (need %u).\n", param->members[i].type, param->type);

I would use "expected" or something similar instead of "need" in this message.

> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
> new file mode 100644
> index 0000000..409c00f
> --- /dev/null
> +++ b/dlls/d3dx9_36/preshader.c
> @@ -0,0 +1,688 @@
> +/*
> + * FX preshader parsing & execution, setting shader constants
> + *     from effect parameters.
> + *
> + * Copyright 2016 Paul Gofman
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + */
> +
> +#include "config.h"
> +#include "wine/port.h"
> +
> +#include "windef.h"
> +#include "wingdi.h"

Not your fault but those shouldn't be there. wingdi.h is already
included by d3dx9_36_private.h, while windef.h is not but still it's
necessary to avoid errors when including d3dx9_36_private.h. I'm
sending a patch cleaning up the includes in d3dx9 (and renaming
d3dx9_36_private.h to d3dx9_private.h while at it), please update this
patch accordingly.

> +static const struct
> +{
> +    unsigned int value_size;
> +    unsigned int reg_value_cnt;

Just my preference, I would go with component_size and component_count
(or maybe reg_component_count).

> +    {sizeof(float),  4, PRES_VT_FLOAT }  /* PRES_REGTAB_REG */

I think I had tested this in the past and it turned out temporaries
are stored as doubles. I'll see if I can find something.

> +static const char *table_symbol[] =
> +{
> +    "(null)", "imm", "c", "v", "oc", "ob", "oi", "r"
> +};
> +
> +static const char *xyzw_str = "xyzw";

I haven't checked but maybe you can put those directly in the function
using them.

> +
> +#define OFFSET2REG(table, offset) ((offset) / table_info[table].reg_value_cnt)
> +
> +static const enum PRES_REG_TABLES pres_regset2table[] =
> +{
> +    PRES_REGTAB_OBCONST,  /* D3DXRS_BOOL */
> +    PRES_REGTAB_OICONST,  /* D3DXRS_INT4 */
> +    PRES_REGTAB_CONST,    /* D3DXRS_FLOAT4 */
> +    PRES_REGTAB_NONE,     /* D3DXRS_SAMPLER */
> +};
> +
> +static const enum PRES_REG_TABLES shad_regset2table[] =
> +{
> +    PRES_REGTAB_OBCONST,  /* D3DXRS_BOOL */
> +    PRES_REGTAB_OICONST,  /* D3DXRS_INT4 */
> +    PRES_REGTAB_OCONST,   /* D3DXRS_FLOAT4 */
> +    PRES_REGTAB_NONE,     /* D3DXRS_SAMPLER */
> +};
> +
> +struct d3dx_pres_operand
> +{
> +    enum PRES_REG_TABLES table;
> +    /* offset is value index, not register index, e. g.
> +       offset for value c3.y is 17 (3 * 4 + 1) */

As I mentioned already, I would refer to "components" instead of "values".

> +    unsigned int offset;
> +};
> +
> +struct d3dx_pres_ins
> +{
> +    enum PRES_OPS op;
> +    /* first input argument is scalar,
> +       scalar component is propagated */
> +    BOOL scalar_op;
> +    unsigned int ncomps;
> +    unsigned int ninp_args;

Something like "component_count" and "operands_count" (or
"input_count", or whatever) seems nicer. Similarly for the other
counts.

> +
> +struct d3dx_param_eval
> +{
> +    enum param_eval_type peval_type;

I feel like I might have already asked this, sorry if that's the case:
do you really need this field? At least for what I can see at the
moment you can do without since it's used only in
d3dx_create_param_eval(), which is where you set the field, and in
d3dx_param_eval_set_shader_constants(), where you don't really need
it.

> +    D3DXPARAMETER_TYPE param_type;

This one can also probably be avoided (e.g. by passing it down the
call chain from the d3dx_parameter struct) although it's probably a
bit more useful so I don't mind.

> +
> +    struct d3dx_preshader pres;
> +    struct d3dx_const_tab shader_inputs;
> +};
> +
> +static HRESULT regstore_alloc_table(struct d3dx_regstore *rs, unsigned int table)
> +{
> +    unsigned int sz;
> +
> +    sz = rs->table_sizes[table] * table_info[table].reg_value_cnt * table_info[table].value_size;
> +    if (sz)
> +    {
> +        rs->tables[table] = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sz);
> +        rs->table_value_set[table] = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> +                    sizeof(*rs->table_value_set[table]) *
> +                    ((rs->table_sizes[table] + PRES_VS_BITS_PER_WORD - 1) / PRES_VS_BITS_PER_WORD));
> +        if (!rs->tables[table] || !rs->table_value_set[table])
> +            return E_OUTOFMEMORY;
> +    }
> +    return D3D_OK;
> +}
> +
> +static void regstore_free_tables(struct d3dx_regstore *rs)
> +{
> +    unsigned int i;
> +
> +    for (i = PRES_REGTAB_IMMED; i < PRES_REGTAB_MAX; i++)

Maybe add a couple of enum PRES_REGTAB_FIRST and PRES_REGTAB_LAST and
use them here?

> +        for (j = 0; j < n; j++)
> +            TRACE("0x%08X,", words[i + j]);

Please use lowercase hexadecimals.

> +    if (ninp_args != pres_op_info[i].ninp_args)
> +    {
> +        FIXME("Actual input args %u, expected %u, instruction %s.\n", ninp_args,
> +                pres_op_info[i].ninp_args, pres_op_info[i].mnem);
> +        return NULL;
> +    }
> +    ins->ninp_args = ninp_args;

It shouldn't be necessary to store the number of arguments in the
instruction structure at the moment, although maybe there is some
variable-arguments opcode?

> +    hr = E_FAIL;

This seems unnecessary.

> +
> --

git am complains about that empty line at the end of the file.