IWineD3DDeviceImpl_SetRenderState

[Chris Ahrendt]

H. Verbeet wrote:
> Unrelated to the patch, but if you intend to work on Wine, I'd
> strongly recommend you setup git first.

Got git yesterday and the repository just wanted to make sure the add 
was correct before I comitted and worked with git.


> You shouldn't use d3d8 or d3d9 definitions in wined3d.
> WINED3DBLEND_INVSRCCOLOR is already defined in
> include/wine/wined3d_types.h

I will double check but one of the problems I had was when compiling it
was it said this and the below were missing.. but I will double check again.

> 
>> +#define WINED3DDMT_ENABLE 0
>> +#define WINED3DDMT_DISABLE 1
>> +#define WINED3DBLEND_INVSRCCOLOR2 17
>> +#define WINED3DBLEND_SRCCOLOR2 16
> These should be enums, and they should be declared in
> include/wine/wined3d_types.h. You should also make sure the
> corresponding definitions exist in ddraw/d3d8/d3d9.

Ok  will move them there.... and will make sure they are in those as well.

> 
>> +          if ((Value == TRUE) ||
>> +              (Value == FALSE)) break;
> Comparing booleans like that is questionable.
> 

Removed.

>> +          if ((Value == WINED3DBLEND_ZERO) ||
>> +              (Value == WINED3DBLEND_ONE)  ||
>> +              (Value == WINED3DBLEND_SRCCOLOR) ||
>> +              (Value == WINED3DBLEND_INVSRCCOLOR) ||
>> +              (Value == WINED3DBLEND_SRCALPHA)    ||
>> +              (Value == WINED3DBLEND_INVSRCALPHA) ||
>> +              (Value == WINED3DBLEND_DESTALPHA)   ||
>> +              (Value == WINED3DBLEND_INVDESTALPHA) ||
>> +              (Value == WINED3DBLEND_DESTCOLOR) ||
>> +              (Value == WINED3DBLEND_INVDESTCOLOR) ||
>> +              (Value == WINED3DBLEND_SRCALPHASAT) ||
>> +              (Value == WINED3DBLEND_BOTHSRCALPHA) ||
>> +              (Value == WINED3DBLEND_BOTHINVSRCALPHA) ||
>> +              (Value == WINED3DBLEND_BLENDFACTOR) ||
>> +              (Value == WINED3DBLEND_INVBLENDFACTOR) ||
>> +              (Value == WINED3DBLEND_SRCCOLOR2) ||
>> +              (Value == WINED3DBLEND_INVSRCCOLOR2)) break;
>> +          return WINED3DERR_INVALIDCALL;
> Considering these are enum values, I think it makes more sense to test
> the range, rather every individual value within it.
> 

Problem with this is if any of the above values change for whatever 
reason then  it breaks the verification.  I agree for the most part that 
testing the range makes sense but I think in this case it is safer to do 
it this way. But if the general consensus is to use the range that is an 
easy change.

>> +          if ((Value >= 0) ||
> Since DWORDs are unsigned, this will always evaluate as true.
> 
>> +          if ((Value >= 0) ||
>> +              (Value <=0xFFFFFFFF)) break;
> This doesn't make sense.
> 

Removed


>> +     Default:
> Did you try to compile your code?
> 

Yes and got warnings but no errors... and that leads to another question 
I have..  why would I get the warnings on the switch statement about the 
case items being unhandled? I looked it up and the suggested thing to 
fix the warnings was to add the default which was already there.
That warning didn't make sense to me...

> As a general comment, you're not going to get a patch like this in
> without appropriate test cases in ddraw, d3d8 and d3d9.

So I need to write the test cases for d3d9 since this is the module 
which would be effected.

Chris