2017-06-06 14:28 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
+static void pres_float_to_int(void *in, void *out, unsigned int count)
"in" should probably be const in all of these helpers.
+static void pres_bool_to_int(void *in, void * out, unsigned int count)
Stray whitespace after *.
+{ + unsigned int i; + const float *in_bool = in; + int *out_int = out; + + for (i = 0; i < count; ++i) + out_int[i] = !!in_bool[i];
Is the double negation necessary here (or above)?
+static void regstore_set_data(struct d3dx_regstore *rs, unsigned int table, + unsigned int offset, unsigned int *in, unsigned int count, enum pres_value_type param_type) +{ + typedef void (*conv_func)(void *in, void *out, unsigned int count); + static conv_func set_const_funcs[PRES_VT_COUNT][PRES_VT_COUNT] =
Missing const.
+ { + {NULL, NULL, pres_float_to_int, pres_value_to_bool}, + {NULL, NULL, NULL, NULL}, + {pres_int_to_float, NULL, NULL, pres_value_to_bool}, + {pres_bool_to_float, NULL, pres_bool_to_int, NULL} + }; + enum pres_value_type table_type = table_info[table].type; + + if (table_type == param_type) + { + regstore_set_values(rs, table, in, offset, count); + return; + }
IIUC in this case regstore_set_values() will have in == out. That's a problem since it means memcpy()ing the data onto itself, which is undefined. It's also unnecessary, regstore_set_modified() would be enough, but only if in == out which is something the current regstore_set_data() interface doesn't enforce (and indeed patch 2/5 doesn't respect). I guess the root issue is that you're now using the regstore also as temporary storage and none of the existing (and new) code expects it. So it's a matter of fixing the code as necessary to make it work fine or to avoid this "new" usage of the regstore e.g. by means of a separate temporary buffer.
for (element = 0; element < const_set->element_count; ++element) { + unsigned int *out = (unsigned int *)rs->tables[table] + start_offset;
Aside from the issue I mentioned above, this works only because all the formats we're interesting in converting from / to happen be 4 bytes wide. It's somewhat assumed in a bunch of places already but adding more isn't too exciting. I'd like at least an assert checking for the precondition, if possible.
@@ -1083,23 +1155,15 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const WARN("Parameter data is too short, name %s, component %u.\n", debugstr_a(param->name), i); break; } - - in = (unsigned int *)data + param_offset; - switch (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[table].type); - break; - } - regstore_set_values(rs, table, &out, offset, 1); + out[offset] = data[param_offset];
I think separating the matrix transposing / "reshaping" from the data conversion is okay, if that allows further changes and optimizations like patch 2/5. You might want to add a small comment around here to clarify the split.
} } start_offset += get_offset_reg(table, const_set->register_count); data += param->rows * param->columns; } + start_offset = get_offset_reg(table, const_set->register_index); + regstore_set_data(rs, table, start_offset, (unsigned int *)rs->tables[table] + start_offset, + get_offset_reg(table, const_set->register_count) * const_set->element_count, param_type);
I understand that you want to avoid to use some separate storage for the "to be converted" data but, like above, this regstore table pointer cast makes me a bit uncomfortable.