2016-03-17 12:59 GMT+01:00 Paul Gofman gofmanp@gmail.com:
+enum PRES_REG_TABLES +{
- PRES_REGTAB_NONE,
- PRES_REGTAB_IMMED, /* immediate double constants from CLIT */
- PRES_REGTAB_CONST,
- PRES_REGTAB_VERTEX, /* not used */
- PRES_REGTAB_OCONST,
- PRES_REGTAB_OBCONST,
- PRES_REGTAB_OICONST,
- PRES_REGTAB_REG,
- PRES_REGTAB_MAX,
- PRES_REGTAB_FIRST = PRES_REGTAB_IMMED,
- PRES_REGTAB_LAST = PRES_REGTAB_REG,
+};
You can drop PRES_REGTAB_NONE unless it's going to be used.
+++ b/dlls/d3dx9_36/preshader.c @@ -0,0 +1,627 @@ +/*
- FX preshader parsing & execution, setting shader constants
from effect parameters.
It looks like that kind of general file description is omitted in newer source files. Maybe drop it? I don't particularly care though.
- /* TODO: use double precision for x64 arch */
- {sizeof(float), 4, PRES_VT_FLOAT } /* PRES_REGTAB_REG */
Nitpick, just mention "64 bit" or something, we don't want to make all the non-Intel archs angry ;)
+#define PRES_VS_BITS_PER_WORD (sizeof(unsigned int) * 8)
What does the _VS_ part of the name mean? I'm also a bit thrown off by the _WORD part, mostly because in Win32 WORD is a 16 bit while DWORD is 32 bit and unsigned int is also 32 bit. Maybe it's just me, I don't know.
+static void dump_shader_bytecode(void *data, unsigned int size) +{
- unsigned int *words = (unsigned int *)data;
Same here. I guess you could call it "bytecode" or such.
+static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc) +{
- while ((*ptr & 0xFFFF) == 0xFFFE)
A couple leftover uppercase hexadecimal constants.
- hr = D3DXGetShaderConstantTable(byte_code, &ctab);
- if (FAILED(hr) || !ctab)
- {
TRACE("Could not get CTAB data, hr %#x.\n", hr);/* returning OK is not a typo */return D3D_OK;- }
Sorry, back to that comment again. I guess it would be better to spell out why that's the intended return value instead (e.g. "Not returning an error, shaders without a CTAB are valid.").
+static void dump_arg(struct d3dx_regstore *rs, struct d3dx_pres_operand *arg, int component_count) +{
- static const char *xyzw_str = "xyzw";
- unsigned int i, table;
- table = arg->table;
- if (table == PRES_REGTAB_IMMED)
- {
TRACE("(");for (i = 0; i < component_count; ++i)TRACE(i < component_count - 1 ? "%lf, " : "%lf",((double *)rs->tables[PRES_REGTAB_IMMED])[arg->offset + i]);
%lf is not right, %f already expects a double. I'd just use %.16e (which has the added benefit of showing something different from 0 in the "precision" test from the previous patch). Another option would be to use the same format used by MS when disassembling shaders, although this seems to be a case where it's worth diverging.
+static void dump_ins(struct d3dx_regstore *rs, struct d3dx_pres_ins *ins) +{
- unsigned int i;
- TRACE(" %s ", pres_op_info[ins->op].mnem);
- dump_arg(rs, &ins->output, pres_op_info[ins->op].func_all_comps ? 1 : ins->component_count);
- for (i = 0; i < pres_op_info[ins->op].input_count; ++i)
- {
TRACE(",");
With a blank after the comma it would be prettier.
There is still some inconsistent formatting (e.g. "== NULL" vs "!", uppercase hexadecimals, cnt vs count).
A possible idea to split this patch further: what if you move the addition of (most of) the contents of the various functions in preshader.c to a separate patch? That is, make a patch which adds the various calls to d3dx_create_param_eval(), d3dx_free_param_eval() and similar (i.e. most if not all the changes to effect.c) but only introduces a skeleton implementation of those functions in preshader.c. Those are actually fleshed out only in the next patch. Many of the new preshader-related definitions can probably also go into the second patch. The traces of the disassembled preshader can also potentially go to a separate patch (leaving only dump_shader_bytecode() in the initial implementation - btw, I think it should be called dump_preshader_bytecode() given the renaming of the other stuff).