IWineD3DDeviceImpl_SetRenderState

[Chris Ahrendt]

Attached  to this note is a patch I have been working on for the past 
few days to finish out the above function.
What I did was use MSDN for the definition :

/*****
  * Get / Set Render States
  * TODO: Verify against dx9 definitions
  *

Direct X 9 Definition:
  Parameters
  State
     [in] Device state variable that is being modified. This parameter
     can be any member of the D3DRENDERSTATETYPE enumerated type.
     Value[in] New value for the device render state to be set.
     The meaning of this parameter is dependent on the value specified
     for State. For example, if State were D3DRS_SHADEMODE, the second
     parameter would be one member of the D3DSHADEMODE enumerated type.

Return Values

If the method succeeds, the return value is D3D_OK. 
D3DERR_INVALIDCALL is returned if one of the arguments is invalid.
Requirements

Header: Declared in D3D9.h

should I open a bug and then post this to the patches list...
Please let me know what you think.

The Following States have no rules for their valid values associated 
with them as far as I can find.

WINED3DRS_BLENDOPALPHA:
           WINED3DRS_DEPTHBIAS:
           WINED3DRS_SRGBWRITEENABLE:
           WINED3DRS_BLENDFACTOR:
           WINED3DRS_COLORWRITEENABLE1:
           WINED3DRS_COLORWRITEENABLE2:
           WINED3DRS_COLORWRITEENABLE3:
           WINED3DRS_ADAPTIVETESS_X:
           WINED3DRS_ADAPTIVETESS_Y:
           WINED3DRS_ADAPTIVETESS_Z:
           WINED3DRS_ADAPTIVETESS_W:
           WINED3DRS_SLOPESCALEDEPTHBIAS:
           WINED3DRS_TWEENFACTOR:
           WINED3DRS_COLORWRITEENABLE:
           WINED3DRS_POINTSIZE_MAX:
           WINED3DRS_WRAP0:
           WINED3DRS_WRAP1:
           WINED3DRS_WRAP2:
           WINED3DRS_WRAP3:
           WINED3DRS_WRAP4:
           WINED3DRS_WRAP5:
           WINED3DRS_WRAP6:
           WINED3DRS_WRAP7:
           WINED3DRS_WRAP8:
           WINED3DRS_WRAP9:
           WINED3DRS_WRAP10:
           WINED3DRS_WRAP11:
           WINED3DRS_WRAP12:
           WINED3DRS_WRAP13:
           WINED3DRS_WRAP14:
           WINED3DRS_WRAP15:
           WINED3DRS_STENCILREF:
           WINED3DRS_FOGSTART:
           WINED3DRS_FOGEND:
           WINED3DRS_FOGDENSITY:
           WINED3DRS_CLIPPLANEENABLE:
           WINED3DRS_POINTSIZE:
           WINED3DRS_POINTSIZE_MIN:
           WINED3DRS_POINTSCALE_A:
           WINED3DRS_POINTSCALE_B:
           WINED3DRS_POINTSCALE_C:

Chris

--- device_old.c    2008-07-25 11:42:25.000000000 -0400
+++ device.c    2008-07-26 21:07:45.000000000 -0400
@@ -34,6 +34,11 @@

WINE_DEFAULT_DEBUG_CHANNEL(d3d);
 #define GLINFO_LOCATION This->adapter->gl_info
+#define D3DBLEND_INVSRCCOLOR 4 
+#define WINED3DDMT_ENABLE 0 
+#define WINED3DDMT_DISABLE 1 
+#define WINED3DBLEND_INVSRCCOLOR2 17  
+#define WINED3DBLEND_SRCCOLOR2 16

/* Define the default light parameters as specified by MSDN */
 const WINED3DLIGHT WINED3D_default_light = {
@@ -3249,14 +3254,280 @@
 /*****
  * Get / Set Render States
  * TODO: Verify against dx9 definitions
- *****/
+ *
+ 
+ Direct X 9 Definition:
+ Parameters
+ State
+    [in] Device state variable that is being modified. This parameter 
+    can be any member of the D3DRENDERSTATETYPE enumerated type. 
+    Value[in] New value for the device render state to be set. 
+    The meaning of this parameter is dependent on the value specified 
+    for State. For example, if State were D3DRS_SHADEMODE, the second 
+    parameter would be one member of the D3DSHADEMODE enumerated type. 
+
+ Return Values
+
+   If the method succeeds, the return value is D3D_OK. D3DERR_INVALIDCALL is returned if one of the arguments is invalid.
+Requirements
+
+Header: Declared in D3D9.h
+
+ ****/
 static HRESULT WINAPI IWineD3DDeviceImpl_SetRenderState(IWineD3DDevice *iface, WINED3DRENDERSTATETYPE State, DWORD Value) {

IWineD3DDeviceImpl  *This     = (IWineD3DDeviceImpl *)iface;
     DWORD oldValue = This->stateBlock->renderState[State];

TRACE("(%p)->state = %s(%d), value = %d\n", This, debug_d3drenderstate(State), State, Value);
+    
+    switch(State)
+    {
+     case WINED3DRS_SEPARATEALPHABLENDENABLE:
+          WINED3DRS_TWOSIDEDSTENCILMODE:
+          WINED3DRS_ENABLEADAPTIVETESSELLATION:
+          WINED3DRS_ANTIALIASEDLINEENABLE:
+          WINED3DRS_SCISSORTESTENABLE:
+          WINED3DRS_INDEXEDVERTEXBLENDENABLE:
+          WINED3DRS_MULTISAMPLEANTIALIAS:
+          WINED3DRS_POINTSPRITEENABLE:
+          WINED3DRS_POINTSCALEENABLE:
+          WINED3DRS_NORMALIZENORMALS:
+          WINED3DRS_LOCALVIEWER:
+          WINED3DRS_COLORVERTEX:
+          WINED3DRS_CLIPPING:
+          WINED3DRS_ZENABLE:
+          WINED3DRS_STENCILENABLE:
+          WINED3DRS_RANGEFOGENABLE:
+          WINED3DRS_ALPHABLENDENABLE:     
+          WINED3DRS_DITHERENABLE:
+          WINED3DRS_ZWRITEENABLE:
+          WINED3DRS_ALPHATESTENABLE:
+          WINED3DRS_FOGENABLE:
+          WINED3DRS_SPECULARENABLE:
+          WINED3DRS_LIGHTING:
+          WINED3DRS_LASTPIXEL:
+          if ((Value == TRUE) || 
+              (Value == FALSE)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_FILLMODE:
+          if ((Value == WINED3DFILL_POINT) || 
+              (Value == WINED3DFILL_WIREFRAME) || 
+              (Value == WINED3DFILL_SOLID)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_SHADEMODE:
+          if ((Value == WINED3DSHADE_FLAT) || 
+              (Value == WINED3DSHADE_GOURAUD) || 
+              (Value == WINED3DSHADE_PHONG)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_SRCBLENDALPHA:
+     case WINED3DRS_DESTBLENDALPHA:
+     case WINED3DRS_DESTBLEND:
+     case WINED3DRS_SRCBLEND:
+          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;
+     break;
+
+     case WINED3DRS_CULLMODE:
+          if ((Value == WINED3DCULL_NONE) || 
+              (Value == WINED3DCULL_CW) || 
+              (Value == WINED3DCULL_CCW)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+     
+     case WINED3DRS_CCW_STENCILFUNC:
+          WINED3DRS_STENCILFUNC:
+          WINED3DRS_ALPHAFUNC:
+          WINED3DRS_ZFUNC:
+          if ((Value == WINED3DCMP_NEVER) || 
+              (Value == WINED3DCMP_LESS) || 
+              (Value == WINED3DCMP_EQUAL) || 
+              (Value == WINED3DCMP_LESSEQUAL) || 
+              (Value == WINED3DCMP_GREATER) || 
+              (Value == WINED3DCMP_NOTEQUAL) || 
+              (Value == WINED3DCMP_GREATEREQUAL) || 
+              (Value == WINED3DCMP_ALWAYS)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_ALPHAREF:
+          /* Valid values are between 0 and FF */
+          if ((Value >= 0) || 
+              (Value <= 0xFF)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_FOGCOLOR:
+          /* Valid Values are between 0 and FFFF (4 bytes alpha, red, green, and blue)) */
+          if ((Value >= 0) || 
+              (Value <= 0xFFFF)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_FOGVERTEXMODE:
+          WINED3DRS_FOGTABLEMODE:
+          if ((Value == WINED3DFOG_NONE) || 
+              (Value == WINED3DFOG_EXP) || 
+              (Value == WINED3DFOG_EXP2) || 
+              (Value == WINED3DFOG_LINEAR)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_CCW_STENCILFAIL:
+          WINED3DRS_CCW_STENCILZFAIL:
+          WINED3DRS_CCW_STENCILPASS:
+          WINED3DRS_STENCILPASS:
+          WINED3DRS_STENCILZFAIL:
+          WINED3DRS_STENCILFAIL:
+          if ((Value == WINED3DSTENCILOP_KEEP) || 
+              (Value == WINED3DSTENCILOP_ZERO) || 
+              (Value == WINED3DSTENCILOP_REPLACE) || 
+              (Value == WINED3DSTENCILOP_INCRSAT) || 
+              (Value == WINED3DSTENCILOP_DECRSAT) || 
+              (Value == WINED3DSTENCILOP_INVERT) || 
+              (Value == WINED3DSTENCILOP_INCR) || 
+              (Value == WINED3DSTENCILOP_DECR)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+     
+     case WINED3DRS_MULTISAMPLEMASK:
+          WINED3DRS_AMBIENT:
+          WINED3DRS_TEXTUREFACTOR:
+          WINED3DRS_STENCILWRITEMASK:
+          WINED3DRS_STENCILMASK:
+          if ((Value >= 0) || 
+              (Value <=0xFFFFFFFF)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_SPECULARMATERIALSOURCE:
+          WINED3DRS_AMBIENTMATERIALSOURCE:
+          WINED3DRS_EMISSIVEMATERIALSOURCE:
+          WINED3DRS_DIFFUSEMATERIALSOURCE:
+         if ((Value == WINED3DMCS_MATERIAL) || 
+             (Value == WINED3DMCS_COLOR1) || 
+             (Value == WINED3DMCS_COLOR2)) break;
+          return WINED3DERR_INVALIDCALL;
+     break;
+
+     case WINED3DRS_VERTEXBLEND:
+         if ((Value == WINED3DVBF_DISABLE) || 
+             (Value == WINED3DVBF_1WEIGHTS) || 
+             (Value == WINED3DVBF_2WEIGHTS) || 
+             (Value == WINED3DVBF_3WEIGHTS) || 
+             (Value == WINED3DVBF_TWEENING) || 
+             (Value == WINED3DVBF_0WEIGHTS)) break;
+          return WINED3DERR_INVALIDCALL;
+          break;
+
+     case WINED3DRS_PATCHEDGESTYLE:
+          if ((Value == WINED3DPATCHEDGE_DISCRETE) || 
+              (Value == WINED3DPATCHEDGE_CONTINUOUS)) break;
+          return WINED3DERR_INVALIDCALL;
+          break;
+
+     case WINED3DRS_DEBUGMONITORTOKEN:
+          if ((Value == WINED3DDMT_ENABLE) || 
+              (Value == WINED3DDMT_DISABLE)) break;
+          return WINED3DERR_INVALIDCALL;
+          break;
+
+     case WINED3DRS_BLENDOP:
+         if ((Value == WINED3DBLENDOP_ADD) || 
+             (Value == WINED3DBLENDOP_SUBTRACT) || 
+             (Value == WINED3DBLENDOP_REVSUBTRACT) || 
+             (Value == WINED3DBLENDOP_MIN) || 
+             (Value == WINED3DBLENDOP_MAX))  break;
+          return WINED3DERR_INVALIDCALL;
+          break;
+
+     case WINED3DRS_NORMALDEGREE:
+          WINED3DRS_POSITIONDEGREE:
+          if ((Value == WINED3DDEGREE_LINEAR) || 
+              (Value == WINED3DDEGREE_QUADRATIC) || 
+              (Value == WINED3DDEGREE_CUBIC) || 
+              (Value == WINED3DDEGREE_QUINTIC))  break;
+          return WINED3DERR_INVALIDCALL;
+          break;
+
+     case WINED3DRS_MINTESSELLATIONLEVEL:
+          WINED3DRS_MAXTESSELLATIONLEVEL:
+          if ((Value == WINED3DDEGREE_LINEAR) || 
+              (Value == WINED3DDEGREE_QUADRATIC) || 
+              (Value == WINED3DDEGREE_CUBIC) || 
+              (Value == WINED3DDEGREE_QUINTIC))  break;
+          return WINED3DERR_INVALIDCALL;
+          break;
+
+     case WINED3DRS_BLENDOPALPHA:
+          WINED3DRS_DEPTHBIAS:
+          WINED3DRS_SRGBWRITEENABLE:
+          WINED3DRS_BLENDFACTOR:
+          WINED3DRS_COLORWRITEENABLE1:
+          WINED3DRS_COLORWRITEENABLE2:
+          WINED3DRS_COLORWRITEENABLE3:
+          WINED3DRS_ADAPTIVETESS_X:
+          WINED3DRS_ADAPTIVETESS_Y:
+          WINED3DRS_ADAPTIVETESS_Z:
+          WINED3DRS_ADAPTIVETESS_W:
+          WINED3DRS_SLOPESCALEDEPTHBIAS:
+          WINED3DRS_TWEENFACTOR:     
+          WINED3DRS_COLORWRITEENABLE:
+          WINED3DRS_POINTSIZE_MAX:
+          WINED3DRS_WRAP0:
+          WINED3DRS_WRAP1:
+          WINED3DRS_WRAP2:
+          WINED3DRS_WRAP3:
+          WINED3DRS_WRAP4:
+          WINED3DRS_WRAP5:
+          WINED3DRS_WRAP6:
+          WINED3DRS_WRAP7:
+          WINED3DRS_WRAP8:
+          WINED3DRS_WRAP9:
+          WINED3DRS_WRAP10:
+          WINED3DRS_WRAP11:
+          WINED3DRS_WRAP12:
+          WINED3DRS_WRAP13:
+          WINED3DRS_WRAP14:
+          WINED3DRS_WRAP15:
+          WINED3DRS_STENCILREF:
+          WINED3DRS_FOGSTART:
+          WINED3DRS_FOGEND:
+          WINED3DRS_FOGDENSITY:
+          WINED3DRS_CLIPPLANEENABLE:
+          WINED3DRS_POINTSIZE:
+          WINED3DRS_POINTSIZE_MIN:
+          WINED3DRS_POINTSCALE_A:
+          WINED3DRS_POINTSCALE_B:
+          WINED3DRS_POINTSCALE_C:
+          break;

+     Default:
+        return WINED3DERR_INVALIDCALL;
+    } 
+ 
     This->updateStateBlock->changed.renderState[State] = TRUE;
     This->updateStateBlock->renderState[State] = Value;

[John Klehm]

On Sat, Jul 26, 2008 at 8:25 PM, Chris Ahrendt  wrote:
> Attached  to this note is a patch I have been working on for the past few
> days to finish out the above function.
> What I did was use MSDN for the definition :

Copyright issues ensue when you copy and paste from MSDN.  You'll have
to rewrite the comment portion.

Best of luck,
--John Klehm

[Chris Ahrendt]

John Klehm wrote:
> On Sat, Jul 26, 2008 at 8:25 PM, Chris Ahrendt  wrote:
> > Attached  to this note is a patch I have been working on for the past few
> > days to finish out the above function.
> > What I did was use MSDN for the definition :
>
> Copyright issues ensue when you copy and paste from MSDN.  You'll have
> to rewrite the comment portion.
>
> Best of luck,
> --John Klehm

I did not copy and paste from MSDN.. I used the rules defined in the 
statelemt.. I can remove the comment if that is what you are talking 
about? But the code itself I did myself...

Chris

[H. Verbeet]

2008/7/27 Chris Ahrendt :
> I did not copy and paste from MSDN.. I used the rules defined in the
> statelemt.. I can remove the comment if that is what you are talking
> about? But the code itself I did myself...

Documentation is copyrighted.

[Chris Ahrendt]

H. Verbeet wrote:
> 2008/7/27 Chris Ahrendt :
> > I did not copy and paste from MSDN.. I used the rules defined in the
> > statelemt.. I can remove the comment if that is what you are talking
> > about? But the code itself I did myself...
>
> Documentation is copyrighted.

I removed the documentation I just put it in there for this so people 
can understand what was done... it is removed

chris

[James McKenzie]

Chris Ahrendt wrote:
> H. Verbeet wrote:
> > 2008/7/27 Chris Ahrendt :
> > > I did not copy and paste from MSDN.. I used the rules defined in the
> > > statelemt.. I can remove the comment if that is what you are talking
> > > about? But the code itself I did myself...
> >
> > Documentation is copyrighted.
>
> I removed the documentation I just put it in there for this so people 
> can understand what was done... it is removed

Chris:

You CAN write your own documentation describing what any function does 
and how to properly use it.  You CANNOT copy verbatim documentation from 
any source, MSDN included.

James McKenzie

[John Klehm]

On Sun, Jul 27, 2008 at 7:59 AM, Chris Ahrendt  wrote:
> I removed the documentation I just put it in there for this so people can
> understand what was done... it is removed

Documentation is a good thing.  You just can't use the exact same
wording from a copyrighted source (MSDN).

Regards,
--John

[H. Verbeet]

> --- device_old.c    2008-07-25 11:42:25.000000000 -0400
> +++ device.c    2008-07-26 21:07:45.000000000 -0400

Unrelated to the patch, but if you intend to work on Wine, I'd
strongly recommend you setup git first.
> +#define D3DBLEND_INVSRCCOLOR 4

You shouldn't use d3d8 or d3d9 definitions in wined3d.
WINED3DBLEND_INVSRCCOLOR is already defined in
include/wine/wined3d_types.h
> +#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.
> -
>      if ((Value == TRUE) ||
> -
>          (Value == FALSE)) break;

Comparing booleans like that is questionable.
> -
>      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.
> -
>      if ((Value >= 0) ||

Since DWORDs are unsigned, this will always evaluate as true.
> -
>      if ((Value >= 0) ||
> -
>          (Value <=0xFFFFFFFF)) break;

This doesn't make sense.
> -
> Default:

Did you try to compile your code?

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

[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

[H. Verbeet]

2008/7/28 Chris Ahrendt :
> 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.

The values aren't just going to change, they're essentially part of
the D3D interface.
> > > -
> > > 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...

C is case-sensitive, "Default" is not the same as "default". In
general, you shouldn't submit patches that generate warnings.
> So I need to write the test cases for d3d9 since this is the module which
> would be effected.

These changes affect all of ddraw, d3d8 and d3d9, so you'd need tests
for all of them. Note that for d3d9 there are already some tests for
SetRenderState() in dlls/d3d9/tests/stateblock.c

[Stefan Dösinger]

> /*****
> - Get / Set Render States
> - TODO: Verify against dx9 definitions
> -

As Henri said, this needs a test case. So far we have not found any Set*
function that returns an error. For SetTexture, SetSamplerState, ..., if the
sampler index is out of bounds, the native implementation happily corrupts
memory and returns D3D_OK rather than checking anything.

Also even if tests confirm it is correct, I don't think this patch is a good
idea unless we have a game that breaks because it expects an error from this
function back. The setter functions are highly performance critical. Those
value checks add a lot of code(if-then-else constructs in the compiled code)
which slows things down. In benchmarks every single if statement in the
setter function hurts.

[Chris Ahrendt]

Stefan Dösinger wrote:
> > /*****
> > - Get / Set Render States
> > - TODO: Verify against dx9 definitions
> > -
>
> As Henri said, this needs a test case. So far we have not found any Set*
> function that returns an error. For SetTexture, SetSamplerState, ..., if the
> sampler index is out of bounds, the native implementation happily corrupts
> memory and returns D3D_OK rather than checking anything.
>
> Also even if tests confirm it is correct, I don't think this patch is a good
> idea unless we have a game that breaks because it expects an error from this
> function back. The setter functions are highly performance critical. Those
> value checks add a lot of code(if-then-else constructs in the compiled code)
> which slows things down. In benchmarks every single if statement in the
> setter function hurts.

Yes i am aware of the time problems stefan. However with this fix and 
the one associated with 14072 (I duped it against another which escapes 
me at the moment)  the problem with EverQuest2.exe goes away. EQ2 seems 
to be loading the fog array until it get the error back. Without this 
patch and the one mentioned above I get the no pixel shader found error.
This is using the latest Git Tree (1.1.2+)

Chris

[Chris Ahrendt]

Stefan Dösinger wrote:
> > /*****
> > - Get / Set Render States
> > - TODO: Verify against dx9 definitions
> > -
>
> As Henri said, this needs a test case. So far we have not found any Set*
> function that returns an error. For SetTexture, SetSamplerState, ..., if the
> sampler index is out of bounds, the native implementation happily corrupts
> memory and returns D3D_OK rather than checking anything.
>
> Also even if tests confirm it is correct, I don't think this patch is a good
> idea unless we have a game that breaks because it expects an error from this
> function back. The setter functions are highly performance critical. Those
> value checks add a lot of code(if-then-else constructs in the compiled code)
> which slows things down. In benchmarks every single if statement in the
> setter function hurts.

Oh I also forgot... I believe that set render state is primarily called 
at the init of the app so should not overly effect the performance of 
the application. When EQ2 is loading it sets fogcolor to FFFFFF
the maximum allowed value for fog color according to MSDN is :

+     case WINED3DRS_FOGCOLOR:
+          /* Valid Values are between 0 and FFFF (4 bytes alpha, red, 
green, and blue)) */
+          if ((Value >= 0) ||
+              (Value <= 0xFFFF)) break;

So when EQ2 sets the fog color to FFFFFF its an invalid value and should
return WINED3DERR_INVALIDCALL. Without this EQ2 gives the pixel format 
error and crashes wine. I could change the patch to just catch this.

Chris

[H. Verbeet]

2008/7/27 Chris Ahrendt :
> Oh I also forgot... I believe that set render state is primarily called
> at the init of the app so should not overly effect the performance of
> the application.

Well, no, it pretty much gets called all the time.
> -
>      /* Valid Values are between 0 and FFFF (4 bytes alpha, red,
>
> green, and blue)) */

There are 8 bits in a byte...

[Chris Ahrendt]

H. Verbeet wrote:
> 2008/7/27 Chris Ahrendt :
> > Oh I also forgot... I believe that set render state is primarily called
> > at the init of the app so should not overly effect the performance of
> > the application.
>
> Well, no, it pretty much gets called all the time.
> > -
> >      /* Valid Values are between 0 and FFFF (4 bytes alpha, red,
> >
> > green, and blue)) */
>
> There are 8 bits in a byte...

yes thus FFFF = 4 bytes...

If this is not placed in the code EQ2 gives the no pixel shader error...

chris

[Stefan Dösinger]

> -
> case WINED3DRS_FOGCOLOR:
> -
>      /* Valid Values are between 0 and FFFF (4 bytes alpha, red,
>
> green, and blue)) */
> -
>      if ((Value >= 0) ||
> -
>          (Value <= 0xFFFF)) break;

Humm, this does not sound right to me. The fog color used to be A8R8G8B8,
thus equal values are between 0xFFFFFFFF and 0x00000000. I don't know what
native D3D does with the alpha in fog though.

As you can see in dlls/d3d9/tests/visual.c, if you search for
D3DRS_FOGCOLOR, there are lots of calls which have color values > 0xFFFF and
succeed:

hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGCOLOR, 0xFF00FF00 /* A
nice green */);
ok(hr == D3D_OK, "Turning on fog calculations returned %08x\n", hr);
hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGCOLOR, 0xFF00FF00 /* A
nice green */);
ok(hr == D3D_OK, "Setting fog color failed (%08x)\n", hr);
hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGCOLOR, 0xffffffff);
ok(hr == D3D_OK, "IDirect3DDevice9_SetRenderState returned %08x\n", hr);

[Chris Ahrendt]

Stefan Dösinger wrote:
> > -
> > case WINED3DRS_FOGCOLOR:
> > -
> >      /* Valid Values are between 0 and FFFF (4 bytes alpha, red,
> >
> > green, and blue)) */
> > -
> >      if ((Value >= 0) ||
> > -
> >          (Value <= 0xFFFF)) break;
>
> Humm, this does not sound right to me. The fog color used to be A8R8G8B8,
> thus equal values are between 0xFFFFFFFF and 0x00000000. I don't know what
> native D3D does with the alpha in fog though.
>
> As you can see in dlls/d3d9/tests/visual.c, if you search for
> D3DRS_FOGCOLOR, there are lots of calls which have color values > 0xFFFF and
> succeed:
>
> hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGCOLOR, 0xFF00FF00 /* A
> nice green */);
> ok(hr == D3D_OK, "Turning on fog calculations returned %08x\n", hr);
> hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGCOLOR, 0xFF00FF00 /* A
> nice green */);
> ok(hr == D3D_OK, "Setting fog color failed (%08x)\n", hr);
> hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGCOLOR, 0xffffffff);
> ok(hr == D3D_OK, "IDirect3DDevice9_SetRenderState returned %08x\n", hr);

hmmmm ok...
here is the msdn site I used....

http://doc.51windows.net/Directx9_SDK/?url=/directx9_sdk/graphics/reference/...

ok here is the the definition on the site...
typedef DWORD D3DCOLOR

but  it says the highest 8 bits are not used... so I am off on the max 
value it should be actually FFFFFF.

here is the other thing I missed...
"Fog color for both pixel and vertex fog is set through the 
D3DRS_FOGCOLOR render state. The render state values can be any RGB 
color, specified as an RGBA color. The alpha component is ignored. "

so valid values would be max FFFFFFFF= ALPHA(FF)RED(FF)GREEN(FF)BLUE(FF)

I can change that.. no biggie... how often is SetRenderState called? and 
if you look most of the  if's except a few are relatively small...

Chris

[Stefan Dösinger]

> so valid values would be max FFFFFFFF=
> ALPHA(FF)RED(FF)GREEN(FF)BLUE(FF)

Which is the whole range of a DWORD. A DWORD is between 0 and 0xFFFFFFFF. So
the if check will never evaluate to true. (Also checking a DWORD for <0 is
redundant because a DWORD is unsigned, thus never < 0)
> I can change that.. no biggie... how often is SetRenderState called?
> and
> if you look most of the  if's except a few are relatively small...

SetRenderState is called *often*. The problem with the ifs isn't how big the
block in them is, but if there's an if at all.

Of course, if there is an application that runs into problems because we
should reject a SetRenderState call, I won't argue about performance any
longer; But for now that isn't the case.

[Chris Ahrendt]

Stefan Dösinger wrote:
> > so valid values would be max FFFFFFFF=
> > ALPHA(FF)RED(FF)GREEN(FF)BLUE(FF)
>
> Which is the whole range of a DWORD. A DWORD is between 0 and 0xFFFFFFFF. So
> the if check will never evaluate to true. (Also checking a DWORD for <0 is
> redundant because a DWORD is unsigned, thus never < 0)
> > I can change that.. no biggie... how often is SetRenderState called?
> > and
> > if you look most of the  if's except a few are relatively small...
>
> SetRenderState is called *often*. The problem with the ifs isn't how big the
> block in them is, but if there's an if at all.
>
> Of course, if there is an application that runs into problems because we
> should reject a SetRenderState call, I won't argue about performance any
> longer; But for now that isn't the case.

hmmmm...  I do know without the patch EQ2 doesn't run and with it it 
then gets  into the start of the game.. so it might be another one of 
the if's... I can add the fog to the break with no if's...let me try 
that. Right now I am running just the git 1.1.2+ tree from this morning 
plus the fix in 12929 which does this :
> > Just a guess, but try disabling GL_ATI_separate_stencil and
> > GL_ATI_envmap_bumpmap by commenting them out at line 57 and 60 of
> > dlls/wined3d/directx.c

and without the check in set render state I get the usual no ipixel format.

When I put it in I get the app to start loading.. there is some other 
issues but those are not related to the ipixel.  The opengl error
I found the bug has already been discovered and resolved in 13335 and 
marked 14072 as a dupe of this.

Chris

[Jochen]

Chris Ahrendt schrieb:
[...]
> hmmmm...  I do know without the patch EQ2 doesn't run and with it it 
> then gets  into the start of the game.. so it might be another one of 
> the if's... I can add the fog to the break with no if's...let me try 
> that. Right now I am running just the git 1.1.2+ tree from this morning 
> plus the fix in 12929 which does this :

your patch contained this (shortened):
> - switch(State)
> - {

[...]
> -
> case WINED3DRS_FOGCOLOR:
> -
>      /* Valid Values are between 0 and FFFF (4 bytes alpha, red, green, and blue)) */
> -
>      if ((Value >= 0) || 
> -
>          (Value <= 0xFFFF)) break;
> -
>      return WINED3DERR_INVALIDCALL;
> -
> break;

[...]
> -
> -
> case WINED3DRS_BLENDOPALPHA:
> -
>      WINED3DRS_DEPTHBIAS:

[...]
> -
>      WINED3DRS_FOGSTART:
> -
>      WINED3DRS_FOGEND:
> -
>      WINED3DRS_FOGDENSITY:
> -
>      WINED3DRS_CLIPPLANEENABLE:
> -
>      WINED3DRS_POINTSIZE:
> -
>      WINED3DRS_POINTSIZE_MIN:
> -
>      WINED3DRS_POINTSCALE_A:
> -
>      WINED3DRS_POINTSCALE_B:
> -
>      WINED3DRS_POINTSCALE_C:
> -
>      break;
>
> -
> Default:
> -
>    return WINED3DERR_INVALIDCALL;
> -
>
> }

so unless you changed
> > -
> >      if ((Value >= 0) || 
> > -
> >          (Value <= 0xFFFF)) break;
> > -
> >      return WINED3DERR_INVALIDCALL;

into:
> > -
> >      break;

or added WINED3DRS_FOGCOLOR to the case block that doesn't do any checks 
(before the default), it wouldn't make me wonder if it still works

just as an idea

bye jochen