DxDiag Demo Patch

[Allen Hair]

Hello Wine Developers,

Over the last couple of months I have been part of a team of students
at UCLA who have been
developing an implementation of the DxDiag application to release with
Wine. After a long
quarter, we have produced a working implementation. Some of the
features are still missing but
most of the functionality has been completed. I hope that our work
proves useful to the Wine
project.

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.

- Allen Hair

[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?).

[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

[Henri Verbeet]

2009/8/7 Austin English :
> > > +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).

You really want to add the "void". In the current form it's an K&R
style non-prototype function declaration. I.e., the number of
arguments to the function is undefined, rather than 0.

[Henri Verbeet]

2009/8/7 Henri Verbeet :
> 2009/8/7 Austin English :
> > 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).
>
> You really want to add the "void". In the current form it's an K&R
> style non-prototype function declaration. I.e., the number of
> arguments to the function is undefined, rather than 0.

However, what really needs clarification is the copyright status of this.

[Juan Lang]

> > +C_SRCS = \
> > +»······main.c \
> > - DxDiagCOM.c \
> > - System.c \
> >
> > ...
>
> This won't work properly, you need to use tabs in Makefiles.

Minor point:  in this case, tabs are not necessary.  Tabs are only
necessary in Makefile rules, as in:
foo: bar
     compile bar <-- this needs a tab

But mixing tabs and spaces is poor style and best avoided.
--Juan

[Joel Holdsworth]

If you want, I can draw a Tango-style icon for it, as part of my work in
progress wine icon refresh: http://www.airwebreathe.org.uk/wine-icon/

Wine icon refresh

[Detlef Riekenberg]

On Sa, 2009-08-08 at 20:42 +0100, Joel Holdsworth wrote:
> If you want, I can draw a Tango-style icon for it, as part of my work in
> progress wine icon refresh: http://www.airwebreathe.org.uk/wine-icon/

They look nice,
but I have some comments:

idb_std_large.bmp and idb_std_small.bmp:
  The print preview icon looks like a printer and has a lens in on size.
  The printing icon looks like a printer and has a lens in on size
  The search icon looks like the print preview icon on Windows

You should use seperate icons and properly use the lens.
  For printing, use a printer icon (Windows also use a printer icon
here)
  For the print preview, use something different.
  (Windows is using a full paper sheet with a lens)

You can test your icons with: wine wordpad

programs:
  The Wine glass do not have a consistent size.
  I prefer using the smaller Wine glass, but then the small icons
(16/22)
  are hard to merge.

winetest:
  The icons for all other programs have the Wine glass on the other
side.