Re: DxDiag Demo Patch

[Henri Verbeet]

2009/3/25 Allen Hair :
> Attached is a patch that will add our code to the source tree. This
> patch is not meant to be
> committed, but should provide a demonstration of the application so
> far. Any feedback would be
> appreciated, I will try to incorporate any suggestions as I work on
> releasing incremental patches
> over the next few weeks.

Well, it dies on startup in getInputDeviceInfo() in InputInfo.c, line
13. Probably because of "IDxDiagContainer_GetChildContainer failed:
DxDiag_DirectInputGameports". Regardless, I have a couple of comments
from quickly looking the patch over.
> programs/dxdiag/D3DTest.c     |  354 +++++++++++++++++++++++++++++++++++
> ...

It's probably a matter of taste, but please don't use caps in source filenames.
> +// Is a test instance currently running?

Please don't use C++ style comments in Wine code.
> +C_SRCS = \
> +»······main.c \
> -
>    DxDiagCOM.c \
> -
>    System.c \
>
> ...

This won't work properly, you need to use tabs in Makefiles.
In general, the code mixes tabs and spaces (and from the looks of it
inconsistent tab width as well). Personally I'd prefer spaces, but the
important thing is consistency. The same goes for other style issues
like the position of braces, whitespace usage, etc.
> +void D3DTest_initD3D();

This needs to be "void D3DTest_initD3D(void);" in C. The same goes for
a couple of other functions.
Note that you can avoid the forward declarations by rearranging the
order of the functions.
> +»······if ( !gd3d_pD3D9 ) {
> +»······»·······fprintf(stderr,"Direct3DCreate9() - Failed\n");
> +»······»·······abort();
> +»······»·······}

You'll want to handle failure a bit more gracefully than calling abort().
> +»······»·······pData[i] = D3DCOLOR_XRGB(buffer[offset+bpp*i],buffer[offset+bpp*i+2],buffer[offset+bpp*i+1]);
> +»······»·······// Note: bmp file uses RBG instead of RGB!

That's not actually true, the data is typically stored as BGR(A), but
"offset" looks wrong. You don't want to parse the bitmap yourself
though, you should probably be using LoadBitmap() and GetDIBits().
> - IDirect3DDevice9_DrawPrimitive(gd3d_pDevice, D3DPT_TRIANGLESTRIP,  0, 2 );
> - IDirect3DDevice9_DrawPrimitive(gd3d_pDevice, D3DPT_TRIANGLESTRIP,  4, 2 );
> - IDirect3DDevice9_DrawPrimitive(gd3d_pDevice, D3DPT_TRIANGLESTRIP,  8, 2 );
> - IDirect3DDevice9_DrawPrimitive(gd3d_pDevice, D3DPT_TRIANGLESTRIP, 12, 2 );
> - IDirect3DDevice9_DrawPrimitive(gd3d_pDevice, D3DPT_TRIANGLESTRIP, 16, 2 );
> - IDirect3DDevice9_DrawPrimitive(gd3d_pDevice, D3DPT_TRIANGLESTRIP, 20, 2 );

This is not the most efficient way to render a cube.
> diff --git a/programs/dxdiag/D3DTest.h b/programs/dxdiag/D3DTest.h
> new file mode 100644
> index 0000000..f9e8c4d
> --- /dev/null
> +++ b/programs/dxdiag/D3DTest.h
> @@ -0,0 +1,6 @@
> +#ifndef D3DTEST_H
> +#define D3DTEST_H
> -
>
> +void D3DTest_testD3D();
> -
>
> +#endif

It's not really worth it to create separate header files if they only
contain a single function declaration. You'll probably want to merge
most of these.
> +»······ if (FAILED(DxDiagCOM_GetContainer(pcom->root, _T("DxDiag_DisplayDevices"), &displayEnum)))

_T() (or L"") won't work properly in Wine because wide strings are
different between Unix and Windows. Typically a wchar_t is 32 bit on
Unix and 16 bit on Windows. -fshort-wchar might help, but I don't
think that's portable. I general I think it's best to just avoid stuff
like TCHAR and make the program explicitly unicode.
> +»······/*
> +»······ * DxDiag
> +»······ *
> -
> - Copywrite 2008 Allen Hair
> -
> -

That's cute, but the commonly accepted spelling is "Copyright". The
other source files need appropriate headers as well, including names
of the other authors, unless they assigned copyright to you (or does
UCLA hold the copyright for this?).