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