D3D9 Changes

[Ann and Jason Edmeades]

Hi Oliver, 

I said I'd give this a review, and there's some great work here. 

I've had a chance to go through these changes, and some mostly trivial
comments. Apologies if some of these are inaccurate, because some of the
diffs look complex and I was purely reviewing the patch files! There seems a
lot here, but most are simple...

1. D3DLOCK_ values, D3DUSAGE_ values --> You might as well put these in
order.

2. Any of the d3drs_ (or similar) changes where you remove a value which is
in d3d8 but not d3d9 - Can you please define a wined3d_ version in the
wined3d private header. The problem here is wined3d is compiled with the
d3d9 includes but needs to understand the d3d8 specifics. I think there's
only one or two of these (d3dorder? Some d3drs_ values)

3. Stick your name in the copyright of files you do significant changes to -
Theres a couple I think you missed.

4. Changes to cubetexture.c I mentioned before - You cant change
-    return IWineD3DResourceImpl_GetType((IWineD3DResource *)iface);
+    return IWineD3DResource_GetType((IWineD3DResource *)iface);
As this is recursive. You must leave the Impl version in there

5. You cant do this as the size isn't a member of the surface desc in d3d9
(it is in 8)

-        object->currentDesc_size = ((max(Width,4) * object->bytesPerPixel)
* max(Height,4)) / 2; /* DXT1 is half byte per pixel */
+        object->currentDesc.Size = ((max(Width,4) * object->bytesPerPixel)
* max(Height,4)) / 2; /* DXT1 is half byte per pixel */


6. There's a number of places where whitespace have increased the patch
size, or indenting is changed where it didn't need to be. Its really trivial
and I wont care if it goes in, its just something I noticed 

Examples include
-        case D3DZB_FALSE:
+        case D3DZB_FALSE:            

-
-  TRACE("(%p) : freeing StateBlock %p\n", This, pSB);
+  
+  TRACE("(%p) : freeing StateBlock %p\n", This , pSB);

and

             /* glDrawPixels transforms the raster position as though it was
a vertex -
-               we want to draw at screen position 0,0 - Set up ortho (rhw)
mode as   
-               per drawprim (and leave set - it will sort itself out due to
last_was_rhw */
+            we want to draw at screen position 0,0 - Set up ortho (rhw)
mode as   
+            per drawprim (and leave set - it will sort itself out due to
last_was_rhw */

or

-DWORD WINAPI IWineD3DTextureImpl_GetPriority(IWineD3DTexture *iface) {
+DWORD   WINAPI IWineD3DTextureImpl_GetPriority(IWineD3DTexture *iface) {

(I was trying to remove such spaces as I went along!)


7. You cant comment out d3d8 functionality - We can check the version if we
have to, but we must maintain the support. This relates to (2)
+      
+#if 0 /*FIXME: d3d8 support*/
     case D3DRS_SOFTWAREVERTEXPROCESSING  :

There's a number of these places mostly relating to texture states, render
states and stateblocks - We MUST maintain d3d8 support.

8. Be careful adding in new GL calls with new gl constants, without updating
the winegl_core header - We frequently break old compiles with out of date
opengl headers, and this was one attempt to avoid the issue. 
+           glTexEnvf( GL_POINT_SPRITE_ARB, GL_COORD_REPLACE_ARB, GL_TRUE);

Similarly please use the d3dcore header for 
+        #ifndef GL_MAX_DRAW_BUFFERS_ATI
+        #define GL_MAX_DRAW_BUFFERS_ATI  0x00008824
+        #endif


9. I'm not sure on this but does d3d9 really support 16 concurrent textures
in non-shader drawing modes? (ie do we really need to reapply all the states
for them?). I really haven't checked, I was just surprised by it!

10. Following on from 9, don't the changes to the sampler support break d3d8
settexturestagestate for d3d8 applications for the values like
D3DTSS_MAXMIPLEVEL?

11. Can you explain the problem with the following addition please, just
interested!
+               || (dataLocations.u.s.pSize.lpData   == NULL  /*My ATI9600
doesn't like this combination of nothing!*/

12. You cant use C++ style comments
+//        PLIGHTINFOEL *src;

13. There is a section of code "Experemental memory management that releases
the allocated memory from a surface when the texture is loaded until a lock
is requested on the surface.". This needs to be a separate patch and needs
confirming the memory benefits are not outweighed by the performance hit. I
thought the problem was that reading the contents returned an upside down
texture? 

14. Again, why lines like
+
+
+#include <math.h>
+#include <stdarg.h>
+
+#define NONAMELESSUNION
+#define NONAMELESSSTRUCT
+#include "windef.h"
+#include "winbase.h"
+#include "winuser.h"
+#include "wingdi.h"
+#include "wine/debug.h"
+
+
These are already #included in d3d9_private.h and are not required here (I
had removed them in earlier patches)

----------------------------------------------------------------------------

To sum up I think there is some fabulous work in there, and we definitely
need to get it into cvs. Please do not take the above as criticism, as there
is so much there I wanted to give useful feedback.

I would, however, suggest you split functionality into smaller patches
rather than put lots of new code in at the same time. (And yes, I was guilty
of that but only when it was pure cut and paste from d3d8 and no new
function)

If I read the patches correctly you have the following which probably could
be split into separate patches

- Removing the d3d9 code taking it down to stubs where there was initial
dummy functionality when the skeleton was originally set up
- Correcting the public d3d9* headers, and making the utils functions format
out the data correctly
- Using a single surface call back function (Neat, btw)
- Adding point support
- Adding some memory management
- Support for texture sampler states
- Other minor changes

Jason