Re: DxDiag Demo Patch

[Austin English]

On Fri, Mar 27, 2009 at 6:33 AM, Henri Verbeet wrote:
> 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.

In case anyone's interested, I've cleaned this up a bit:
C++ comments > C comments
Consolidated a bunch of files/headers.
Cleaned up formatting to be consistent.
Removed the bastardization of tabs that was everywhere.
Fixed the Makefile, which didn't even compile...
Fixed the headers, which weren't compiling properly (note, the
combined header is still really messy)
> 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.

This is still true.
> > programs/dxdiag/D3DTest.c     |  354 +++++++++++++++++++++++++++++++++++
> > ...
>
> It's probably a matter of taste, but please don't use caps in source filenames.

Fixed here.
> > +// Is a test instance currently running?
>
> Please don't use C++ style comments in Wine code.

Fixed.
> > +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.

Fixed.
> > +void D3DTest_initD3D();
>
> This needs to be "void D3DTest_initD3D(void);" in C. The same goes for
> a couple of other functions.

I received mixed feedback on #winehackers about this. So currently
left as is...is there a CFLAG/gcc switch to detect this, without using
-ansi (which gives a crapton of other warnings).
> Note that you can avoid the forward declarations by rearranging the
> order of the functions.

Fixed.
> > +»······if ( !gd3d_pD3D9 ) {
> > +»······»·······fprintf(stderr,"Direct3DCreate9() - Failed\n");
> > +»······»·······abort();
> > +»······»·······}
>
> You'll want to handle failure a bit more gracefully than calling abort().

Untouched.
> > +»······»·······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().

Added your comment.
> > - 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.

Untouched.
> > 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.

Fixed.
> > +»······ 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.

Untouched.

I've put it up on http://austinenglish.com/0001-dxdiag.txt and
attached to bug 14118 ( http://bugs.winehq.org/show_bug.cgi?id=14118
).

Allen, I'd really like to see this get into Wine. If you've got some
spare time to fix a few of the bugs y'all left in, I'll help y'all
split it up and get it into Wine. What do you say?

--
-Austin