Re: Add a simple 3D APIs demo
2016-08-19 21:41 GMT+02:00 Ruslan Kabatsayev <b7.10110111(a)gmail.com>:
This adds a 3d-demo wine application. It will allow users to check that their system can handle at least basic WGL, D3D8 and D3D9 before they need to look deeper into their problem.
Signed-off-by: Ruslan Kabatsayev <b7.10110111(a)gmail.com> --- configure | 26 +- configure.ac | 1 + programs/3d-demo/Makefile.in | 14 + programs/3d-demo/common.c | 107 ++++++ programs/3d-demo/common.h | 36 ++ programs/3d-demo/d3d.c | 605 ++++++++++++++++++++++++++++++++ programs/3d-demo/d3d8.c | 2 + programs/3d-demo/d3d9.c | 2 + programs/3d-demo/demo.rc | 61 ++++ programs/3d-demo/main.c | 175 +++++++++ programs/3d-demo/resource.h | 33 ++ programs/3d-demo/wgl.c | 473 +++++++++++++++++++++++++ programs/3d-demo/winehq_logo_glass.png | Bin 0 -> 31512 bytes 13 files changed, 1517 insertions(+), 18 deletions(-) create mode 100644 programs/3d-demo/Makefile.in create mode 100644 programs/3d-demo/common.c create mode 100644 programs/3d-demo/common.h create mode 100644 programs/3d-demo/d3d.c create mode 100644 programs/3d-demo/d3d8.c create mode 100644 programs/3d-demo/d3d9.c create mode 100644 programs/3d-demo/demo.rc create mode 100644 programs/3d-demo/main.c create mode 100644 programs/3d-demo/resource.h create mode 100644 programs/3d-demo/wgl.c create mode 100644 programs/3d-demo/winehq_logo_glass.png
I have a few pretty generic suggestions. You don't need to include configure changes in the patch, they will be regenerated by Alexandre anyway since those depend on the version of the autotools. Did you look into adding those tests to dxdiag instead of as a separate program? I don't think a separate program is necessarily bad but if integrating into dxdiag is reasonable it's probably better to do that from the start. Avoid camelCase or similar mixed-case identifier styles, just use lowercase_with_underscores. For pointer declarations you want to stick the '*' to the variable, not the type. Also please avoid LPTYPES, just use explicit pointer types. C++ comments (// comment) aren't allowed in Wine sources. Please replace them with normal C comments (/* comment */), or drop them entirely if they don't add anything that isn't clear from just looking at the code. It would be better to use explicit float constants when assigning to float variables (e.g. D3DMATERIAL). There aren't many other general style rules in Wine (it's mostly about using the style already used in the component so for new components you're mostly free to pick your own) as long as there is consistency. I see you mixing space after ',' with no space, pick one (preferably the space variant IMO) and stick to it. We usually use parentheses with the "sizeof" operator in Wine sources, so while it's not strictly required it would probably be nice to follow the general trend. More specific comments: I'd split this over multiple patches. You can probably make an initial patch adding the program with no real functionality, then multiple patches adding the various graphical API demos. In that regard, the d3d8 / d3d9 "merged" handling via macros seems a bit obfuscating to me. Just replicating the code twice seems cleaner. Maybe you can create a few helper functions for the common code. BTW, you can probably make things a bit more structured by moving the main loop to common code and have it call API backend functions. You'd have for each backend a structure like: static const struct render_backend d3d9_backend { d3d9_list_modes, d3d9_init, d3d9_draw, d3d9_wndproc, d3d9_destroy, }; (just an idea WRT the actual functions, I haven't really looked in detail) and you'd pass the structure from the selected API to the "main loop" function. I don't think you're using GLU anymore (good) so you should drop the glu32 import. I haven't looked in detail at the actual demos / renderers but I think there is enough stuff to work on already :)
On Mon, Aug 22, 2016 at 12:44 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
I have a few pretty generic suggestions. You don't need to include configure changes in the patch, they will be regenerated by Alexandre anyway since those depend on the version of the autotools.
I was thinking of this, so looked at some other patches which changed configure.ac, which also appeared to alter configure script, so did this. Will remove that change, it's cleaner this way indeed.
Did you look into adding those tests to dxdiag instead of as a separate program? I don't think a separate program is necessarily bad but if integrating into dxdiag is reasonable it's probably better to do that from the start.
Yes, I've even made a dialog for dxdiag with the System tab, but when I began working on the Display tab, it appeared to require some functionality which Wine's dxdiag.dll doesn't even support — providing information about video card, its memory etc. I decided it'd be ugly to just show several buttons to launch demos with no other information, so did the demos a separate program. In the future it'll be not that hard to integrate the demo as just a button triggering CreateProcess(). The demos do return specific error codes on each error, so the dxdiag UI would be able to record more or less explicit log of why demo failed. Also, I think it'd be better to make the demos runnable even if for some strange reason dxdiag framework were broken, so this was another reason why I decided to make it a separate program.
Avoid camelCase or similar mixed-case identifier styles, just use lowercase_with_underscores. For pointer declarations you want to stick the '*' to the variable, not the type. Also please avoid LPTYPES, just use explicit pointer types. C++ comments (// comment) aren't allowed in Wine sources. Please replace them with normal C comments (/* comment */), or drop them entirely if they don't add anything that isn't clear from just looking at the code.
Oh yeah, I was trying to avoid C99 comments, but as GCC doesn't seem to have a switch to warn about using them, some did slip through.
It would be better to use explicit float constants when assigning to float variables (e.g. D3DMATERIAL).
There aren't many other general style rules in Wine (it's mostly about using the style already used in the component so for new components you're mostly free to pick your own) as long as there is consistency. I see you mixing space after ',' with no space, pick one (preferably the space variant IMO) and stick to it. We usually use parentheses with the "sizeof" operator in Wine sources, so while it's not strictly required it would probably be nice to follow the general trend.
More specific comments: I'd split this over multiple patches. You can probably make an initial patch adding the program with no real functionality, then multiple patches adding the various graphical API demos. In that regard, the d3d8 / d3d9 "merged" handling via macros seems a bit obfuscating to me. Just replicating the code twice seems cleaner. Maybe you can create a few helper functions for the common code. D3D code for 8 and 9 being merged was an attempt to avoid forgetting to fix one while fixing another. D3D 8&9 are too similar to need separate files. But OK, I'll split it then. BTW, you can probably make things a bit more structured by moving the main loop to common code and have it call API backend functions. You'd have for each backend a structure like:
static const struct render_backend d3d9_backend { d3d9_list_modes, d3d9_init, d3d9_draw, d3d9_wndproc, d3d9_destroy, };
(just an idea WRT the actual functions, I haven't really looked in detail) and you'd pass the structure from the selected API to the "main loop" function.
I don't think you're using GLU anymore (good) so you should drop the glu32 import. Yeah, just forgot about the import.
Hello, I've addressed most of the concerns, but would like to get some clarifications before I continue with the rest. On Mon, Aug 22, 2016 at 12:44 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
It would be better to use explicit float constants when assigning to float variables (e.g. D3DMATERIAL). Do you mean that numeric constants are to never be implicitly converted? Or only integral->floating-point? Or maybe only double->float?
More specific comments: I'd split this over multiple patches. You can probably make an initial patch adding the program with no real functionality, then multiple patches adding the various graphical API demos. In that regard, the d3d8 / d3d9 "merged" handling via macros seems a bit obfuscating to me. Just replicating the code twice seems cleaner. Maybe you can create a few helper functions for the common code. I've tried looking into splitting D3D code, but from the experience of fixing some issues like CamelCase->snake_case and similar it seems the split would be counter-productive for future changes. What I mean: the D3D 8 and 9 (as used in this code) only differ by interface version suffix and added/removed parameters in some methods (the parameters removed are zero here anyway, since the demos ideally should give identical results with different APIs), plus a few names changed. It seems to me that any change in functionality would require one to edit both D3D8 and D3D9 code in a _very_ similar way, and the differences between the split codes wouldn't be easy to understand: diff would give lots of unrelated differences, while substantial ones could be missed (resulting in subtly non-identical results). While this is inevitable between D3D and WGL, it's easily avoidable for such similar (in such basic use) APIs as D3D8 and D3D9. Do you still think the code should really be split?
Regards, Ruslan
2016-08-26 21:12 GMT+02:00 Ruslan Kabatsayev <b7.10110111(a)gmail.com>:
Hello, I've addressed most of the concerns, but would like to get some clarifications before I continue with the rest.
On Mon, Aug 22, 2016 at 12:44 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
It would be better to use explicit float constants when assigning to float variables (e.g. D3DMATERIAL). Do you mean that numeric constants are to never be implicitly converted? Or only integral->floating-point? Or maybe only double->float?
It's not a "NEVER!!11!"-level rule, but if the conversion can be avoided by writing the constant properly e.g. simply adding the appropriate suffix, then it's good practice to do that. In that specific case, you should write 1.0f (or whatever the actual value is) explicitly.
More specific comments: I'd split this over multiple patches. You can probably make an initial patch adding the program with no real functionality, then multiple patches adding the various graphical API demos. In that regard, the d3d8 / d3d9 "merged" handling via macros seems a bit obfuscating to me. Just replicating the code twice seems cleaner. Maybe you can create a few helper functions for the common code. I've tried looking into splitting D3D code, but from the experience of fixing some issues like CamelCase->snake_case and similar it seems the split would be counter-productive for future changes. What I mean: the D3D 8 and 9 (as used in this code) only differ by interface version suffix and added/removed parameters in some methods (the parameters removed are zero here anyway, since the demos ideally should give identical results with different APIs), plus a few names changed. It seems to me that any change in functionality would require one to edit both D3D8 and D3D9 code in a _very_ similar way, and the differences between the split codes wouldn't be easy to understand: diff would give lots of unrelated differences, while substantial ones could be missed (resulting in subtly non-identical results). While this is inevitable between D3D and WGL, it's easily avoidable for such similar (in such basic use) APIs as D3D8 and D3D9. Do you still think the code should really be split?
Yes. I think looking at the d3d8 and d3d9 code side-by-side should be quite fine to check for unintended differences. At least I'd prefer that to the macros. Also once you want to do something different between the two APIs with the macros solution you'll end up sprinkling #ifdefs around and that can get ugly fast. As I mentioned in the previous email, it doesn't mean you can't or shouldn't share code between the two, just that it would be better IMO to share the code in a different fashion. You can probably use a single shared main loop for all the APIs, create helpers for the common tasks (setting up the matrices, wndproc, ...) etc. There shouldn't be a lot of code actually ending up "replicated". That's just my opinion, of course. I'll listen to counterarguments :)
On Sep 1, 2016, at 11:50 AM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2016-08-26 21:12 GMT+02:00 Ruslan Kabatsayev <b7.10110111(a)gmail.com>:
Hello, I've addressed most of the concerns, but would like to get some clarifications before I continue with the rest.
On Mon, Aug 22, 2016 at 12:44 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
It would be better to use explicit float constants when assigning to float variables (e.g. D3DMATERIAL). Do you mean that numeric constants are to never be implicitly converted? Or only integral->floating-point? Or maybe only double->float?
It's not a "NEVER!!11!"-level rule, but if the conversion can be avoided by writing the constant properly e.g. simply adding the appropriate suffix, then it's good practice to do that. In that specific case, you should write 1.0f (or whatever the actual value is) explicitly.
The type conversion from assigning an integer constant to a double variable happens at compile time. Either coding style produces equivalent results. -Ken
2016-09-01 21:27 GMT+02:00 Ken Thomases <ken(a)codeweavers.com>:
On Sep 1, 2016, at 11:50 AM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2016-08-26 21:12 GMT+02:00 Ruslan Kabatsayev <b7.10110111(a)gmail.com>:
Hello, I've addressed most of the concerns, but would like to get some clarifications before I continue with the rest.
On Mon, Aug 22, 2016 at 12:44 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
It would be better to use explicit float constants when assigning to float variables (e.g. D3DMATERIAL). Do you mean that numeric constants are to never be implicitly converted? Or only integral->floating-point? Or maybe only double->float?
It's not a "NEVER!!11!"-level rule, but if the conversion can be avoided by writing the constant properly e.g. simply adding the appropriate suffix, then it's good practice to do that. In that specific case, you should write 1.0f (or whatever the actual value is) explicitly.
The type conversion from assigning an integer constant to a double variable happens at compile time. Either coding style produces equivalent results.
Yep. The point stands though.
On 2016-09-01 20:27, Ken Thomases wrote:
The type conversion from assigning an integer constant to a double variable happens at compile time. Either coding style produces equivalent results. Visual Studio generates a warning when you do float f = 1; or float f = 1.0. In the first one it complains about integer-to-float, in the second one double-to-float. It seems to have special behavior for float f = 0 or float f = 0.0 though.
participants (4)
-
Ken Thomases -
Matteo Bruni -
Ruslan Kabatsayev -
Stefan Dösinger