[v3 resend 2/6] d3dx9: add preshaders to effect.
Matteo Bruni
matteo.mystral at gmail.com
Mon Mar 14 21:53:54 CDT 2016
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.
More information about the wine-devel
mailing list