Re: 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