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

15 Mar 2016

2016-03-10 19:50 GMT+01:00 Paul Gofman gofmanp@gmail.com:
...
Signed-off-by: Paul Gofman gofmanp@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.

    

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