On 03/24/2017 03:31 AM, Matteo Bruni wrote:
2017-03-24 0:39 GMT+01:00 Paul Gofman gofmanp@gmail.com:
On 03/24/2017 01:57 AM, Matteo Bruni wrote:
2017-03-23 16:06 GMT+01:00 Paul Gofman gofmanp@gmail.com:
Signed-off-by: Paul Gofman gofmanp@gmail.com
dlls/d3dx9_36/preshader.c | 103 ++++++++++++++++++++++++++++++++++++------- dlls/d3dx9_36/tests/effect.c | 38 ++++++++-------- 2 files changed, 106 insertions(+), 35 deletions(-)
diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c index 9aedc99..2974ffd 100644 --- a/dlls/d3dx9_36/preshader.c +++ b/dlls/d3dx9_36/preshader.c @@ -191,6 +191,8 @@ struct d3dx_pres_operand /* offset is component index, not register index, e. g. offset for component c3.y is 13 (3 * 4 + 1) */ unsigned int offset;
- enum pres_reg_tables base_reg_table;
- unsigned int base_reg_offset; };
It would be nicer to create a struct for storing together table and offset and to pass it around to the various functions taking both arguments, like parse_pres_reg(). It should probably be a separate patch before this one.
Actually I can't guess the case when base_reg_table would be not a floating constant preshader input. Maybe I just remove this variable and check with FIXME during parsing?
Sure, that's fine. Still the struct seems useful for parameter passing, if not right there in struct d3dx_pres_operand.
- size = rs->table_sizes[table] *
table_info[table].reg_component_count;
- if (offset >= size)
- {
if ((offset & 255) < size)offset &= 255;elseoffset = offset % size;- }
Is this just to try and avoid the modulo operation in some cases or is there a deeper reason? If the former, you could extend that to all the power-of-two sizes by first checking if that's the case (!(size & size - 1)) and, if so, and-ing offset with the mask (size - 1). That's assuming the compiler doesn't do the same optimization already.
Let me know if there is some other reason instead.
Just doing (offset % size) will lead to different results and will break the tests. The logic in this out of range index checking is a bit mind breaking.
I guess there was a deeper reason indeed ;)
Is that visible in the tests included in this series? Either way, maybe add a comment so that it's obvious why that thing is there.
Oh, I was sure that yes, but just checked once again and it worked without that first check. It could happen that either (most likely) I need it on some other testing I made in process, or potentially could happen that it was "helping" before I figured out the correct way for floating constant table indexing. I will check that and either remove this check or add another test (probably as a separate patch, as it would likely require effect blob update).