2016-05-26 20:28 GMT+02:00 Paul Gofman gofmanp@gmail.com:
Signed-off-by: Paul Gofman gofmanp@gmail.com
dlls/d3dx9_36/d3dx9_private.h | 25 +++- dlls/d3dx9_36/preshader.c | 259 +++++++++++++++++++++++++++++++++++------- 2 files changed, 237 insertions(+), 47 deletions(-)
diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h index 9938d83..970ef2f 100644 --- a/dlls/d3dx9_36/d3dx9_private.h +++ b/dlls/d3dx9_36/d3dx9_private.h @@ -129,14 +129,33 @@ enum pres_reg_tables PRES_REGTAB_FIRST_SHADER = PRES_REGTAB_CONST, };
+enum const_param_copy_state_type +{
- COPY_STATE_TYPE_PARAM,
- COPY_STATE_TYPE_TABLE,
- COPY_STATE_TYPE_PARAM_OFFSET,
- COPY_STATE_TYPE_TABLE_OFFSET,
- COPY_STATE_TYPE_COPY_COUNT
The name of the last enumerator is a bit ambiguous (it might be mistaken to be a _COUNT entry == last actual enumerator + 1). I would either change the name a bit or move it from the last place. Although this might be moot, see below.
+struct d3dx_const_copy_state
I'm not entirely happy with the name, maybe d3dx_const_param_eval_output or something in that fashion?
@@ -811,10 +820,10 @@ static void d3dx_free_const_tab(struct d3dx_const_tab *ctab) { HeapFree(GetProcessHeap(), 0, ctab->inputs); HeapFree(GetProcessHeap(), 0, ctab->inputs_param);
- if (ctab->ctab)
ID3DXConstantTable_Release(ctab->ctab);
- HeapFree(GetProcessHeap(), 0, ctab->const_set);
}
static void d3dx_free_preshader(struct d3dx_preshader *pres)
That extra blank line doesn't seem necessary.
@@ -835,14 +844,181 @@ void d3dx_free_param_eval(struct d3dx_param_eval *peval) HeapFree(GetProcessHeap(), 0, peval); }
-static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab,
D3DXHANDLE hc, struct d3dx_parameter *param)+static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab) +{
- unsigned int i;
- struct d3dx_const_copy_state set_state;
- memset(&set_state, 0, sizeof(set_state));
- for (i = 0; i < const_tab->const_set_count; ++i)
- {
switch (const_tab->const_set[i].copy_state_type){case COPY_STATE_TYPE_PARAM:set_state.param = const_tab->const_set[i].state_change.param;break;case COPY_STATE_TYPE_TABLE:set_state.table = const_tab->const_set[i].state_change.value;break;case COPY_STATE_TYPE_PARAM_OFFSET:set_state.param_offset = const_tab->const_set[i].state_change.value;break;case COPY_STATE_TYPE_TABLE_OFFSET:set_state.table_offset = const_tab->const_set[i].state_change.value;break;case COPY_STATE_TYPE_COPY_COUNT:{unsigned int count;struct d3dx_parameter *param;enum pres_value_type table_type;unsigned int j;count = const_tab->const_set[i].state_change.value;param = set_state.param;table_type = table_info[set_state.table].type;if ((set_state.table_offset + count - 1) / table_info[set_state.table].reg_component_count>= rs->table_sizes[set_state.table]){FIXME("Insufficient table space allocated.\n");break;}if ((param->type == D3DXPT_FLOAT && table_type == PRES_VT_FLOAT)|| (param->type == D3DXPT_INT && table_type == PRES_VT_INT)|| (param->type == D3DXPT_BOOL && table_type == PRES_VT_BOOL)){regstore_set_values(rs, set_state.table, (unsigned int *)param->data + set_state.param_offset,set_state.table_offset, count);}else{for (j = 0; j < count; ++j){unsigned int out;unsigned int *in = (unsigned int *)param->data + set_state.param_offset + j;switch (table_info[set_state.table].type){case PRES_VT_FLOAT: set_number(&out, D3DXPT_FLOAT, in, param->type); break;case PRES_VT_INT: set_number(&out, D3DXPT_INT, in, param->type); break;case PRES_VT_BOOL: set_number(&out, D3DXPT_BOOL, in, param->type); break;default:FIXME("Unexpected type %#x.\n", table_info[set_state.table].type);break;}regstore_set_values(rs, set_state.table, &out, set_state.table_offset + j, 1);}}set_state.param_offset += count;set_state.table_offset += count;break;}}- }
+}
This seems overly complicated and fragile. Instead of this kind of "5 instructions bytecode" isn't it easier to just store and then lookup a struct with all the info you need? I guess the end result would be something akin to storing an array of struct d3dx_const_copy_state in place of struct d3dx_const_param_set. I don't know how the code would exactly look like but I would start by moving building all the struct d3dx_const_copy_param to init time. BTW the coalescing you currently do in add_const_set() could be potentially done as a separate pass after you generate all the "d3dx_const_copy_state-like" structures (but still at init time), which might be simpler.
Does this make sense? Am I missing something?
+static HRESULT add_const_set_value(struct d3dx_const_tab *const_tab, unsigned int value,
unsigned int *current_value, enum const_param_copy_state_type type)+{
- struct d3dx_const_param_set set;
- HRESULT hr;
- if (*current_value != value)
- {
*current_value = value;set.copy_state_type = type;set.state_change.value = value;if (FAILED(hr=append_const_set(const_tab, &set)))
Whitespace.
@@ -931,21 +1107,17 @@ static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_t major_stride = max(minor, table_info[table].reg_component_count); n = min(major * major_stride, desc.RegisterCount * table_info[table].reg_component_count + major_stride - 1) / major_stride;
- for (i = 0; i < n; ++i) { for (j = 0; j < minor; ++j) {
unsigned int out;unsigned int *in; unsigned int offset; offset = start_offset + i * major_stride + j;if (offset / table_info[table].reg_component_count >= rs->table_sizes[table]){if (table_info[table].reg_component_count != 1)FIXME("Output offset exceeds table size, name %s, component %u.\n", desc.Name, i);
if ((offset - start_offset) / table_info[table].reg_component_count >= desc.RegisterCount) break;
Maybe keeping a WARN or something makes sense here?