Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
The error message you have got because you are trying to run a corrupted or missing d3dx9.dll file. Windows displays the above message when system unable to run d3dx9.dll file. To fix the issue, you can restore it. Here is the link to download d3dx9.dll file. http://www.d3dx9.net After download the file, restore it to your System32 folder. That should fix the issue. -- View this message in context: http://wine.1045685.n5.nabble.com/GSoC-2011-Implement-Missing-Mesh-Functions-in-Wine-s-D3DX9-tp4145990p4734631.html Sent from the Wine - Devel mailing list archive at Nabble.com.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sat, Aug 6, 2011 at 7:59 PM, Stefan Dösinger wrote: >> Btw. those FIXMEs might produce a lot of messages (one per vertex). Is >> there good way to limit the number of messages? > A common way is something like this > > { > static BOOL once = false; > if (!once) > { > FIXME("Report this bug to my aunt Tilda please\n"); > once = TRUE; > } > } > > I think there was once a discussion about a FIXME_ONCE macro that took care > of this. not sure what happened with that idea. I've sorta followed Dan's suggestion to use Alexandre's style, except that I have multiple fixme_once variables. Any idea if this would be acceptable? Another solution could be to check the vertex declaration before running the welding algorithm and print the FIXMEs there. But that would decouple the location of where the problem is from the printout, which could be confusing during debugging. From 6a0828e29d94de8407aedab07e3d271496d668fc Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Sun, 7 Aug 2011 11:59:28 +0200 Subject: d3dx9: Only print noisy FIXMEs once in D3DXWeldVertices. --- dlls/d3dx9_36/mesh.c | 35 +++ 1 files changed, 27 insertions(+), 8 deletions(-) diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c index a8139cb..0a46d1b 100644 --- a/dlls/d3dx9_36/mesh.c +++ b/dlls/d3dx9_36/mesh.c @@ -5901,6 +5901,10 @@ static BOOL weld_float16_4(void *to, void *from, FLOAT epsilon) /* Sets the vertex components to the same value if they are within epsilon. */ static BOOL weld_component(void *to, void *from, D3DDECLTYPE type, FLOAT epsilon) { +/* Quite FIXMEs as this is in a loop with potentially thousand of iterations. */ +BOOL fixme_once_unused = FALSE; +BOOL fixme_once_unknown = FALSE; + switch (type) { case D3DDECLTYPE_FLOAT1: @@ -5955,11 +5959,13 @@ static BOOL weld_component(void *to, void *from, D3DDECLTYPE type, FLOAT epsilon return weld_float16_4(to, from, epsilon); case D3DDECLTYPE_UNUSED: -FIXME("D3DDECLTYPE_UNUSED welding not implemented.\n"); +if (!fixme_once_unused++) +FIXME("D3DDECLTYPE_UNUSED welding not implemented.\n"); break; default: -FIXME("Welding of unknown declaration type %d is not implemented.\n", type); +if (!fixme_once_unknown++) +FIXME("Welding of unknown declaration type %d is not implemented.\n", type); break; } @@ -5969,6 +5975,13 @@ static BOOL weld_component(void *to, void *from, D3DDECLTYPE type, FLOAT epsilon static FLOAT get_component_epsilon(const D3DVERTEXELEMENT9 *decl_ptr, const D3DXWELDEPSILONS *epsilons) { FLOAT epsilon = 0.0f; +/* Quite FIXMEs as this is in a loop with potentially thousand of iterations. */ +static BOOL fixme_once_blendindices = FALSE; +static BOOL fixme_once_positiont = FALSE; +static BOOL fixme_once_fog = FALSE; +static BOOL fixme_once_depth = FALSE; +static BOOL fixme_once_sample = FALSE; +static BOOL fixme_once_unknown = FALSE; switch (decl_ptr->Usage) { @@ -6010,22 +6023,28 @@ static FLOAT get_component_epsilon(const D3DVERTEXELEMENT9 *decl_ptr, const D3DX epsilon = 1e-6f; break; case D3DDECLUSAGE_BLENDINDICES: -FIXME("D3DDECLUSAGE_BLENDINDICES welding not implemented.\n"); +if (!fixme_once_blendindices++) +FIXME("D3DDECLUSAGE_BLENDINDICES welding not implemented.\n"); break; case D3DDECLUSAGE_POSITIONT: -FIXME("D3DDECLUSAGE_POSITIONT welding not implemented.\n"); +if (!fixme_once_positiont++) +FIXME("D3DDECLUSAGE_POSITIONT welding not implemented.\n"); break; case D3DDECLUSAGE_FOG: -FIXME("D3DDECLUSAGE_FOG welding not implemented.\n"); +if (!fixme_once_fog++) +FIXME("D3DDECLUSAGE_FOG welding not implemented.\n"); break; case D3DDECLUSAGE_DEPTH: -FIXME("D3DDECLUSAGE_DEPTH welding not implemented.\n"); +if (!fixme_once_depth++) +FIXME("D3DDECLUSAGE_DEPTH welding not implemented.\n"); break; case D3DDECLUSAGE_SAMPLE: -FIXME("D3DDECLUSAGE_SAMPLE welding not implemented.\n"); +if (!fixme_once_sample++) +FIXME("D3DDECLUSAGE_SAMPLE welding not implemented.\n"); break; default: -ERR("Unknown usage %x\n", decl_ptr->Usage); +if (!fixme_once_unknown++) +FIXME("Unknown usage %x\n", decl_ptr->Usage); break; } -- 1.7.6
Re: Printing fixme's once (was: Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9)
On Sat, Aug 6, 2011 at 10:52 PM, Dan Kegel wrote: > Stefan wrote: >> I think there was once a discussion about a FIXME_ONCE macro that took care >> of this. not sure what happened with that idea. > > Judging by > http://source.winehq.org/git/wine.git/commitdiff/45ead7fe85e7107de91bd79a8ceeb1cb17e4f71d, > Alexandre's preferred idiom is > static BOOL fixme_once; > ... > if(!fixme_once++) FIXME("Report bug to Aunt Tilly\n"); Thanks Dan, I'll try to do something similar. > It occurs to me that > #define WINE_ONCE(s) { static int once; if (!once++) s; } > might suffice; you could then write > WINE_ONCE(FIXME("Report bug to Aunt Tilly\n")); > > The previous attempt was more complicated: > http://marc.info/?l=wine-devel&m=129396741411607&w=2 Thanks for giving a shot too. It's sad that there isn't a macro for this yet. My FIXMEs are in a loop that might be called thousands of times (potentially for each vertex in a mesh), so it be could be extremely noisy. Cheers, Michael Mc Donnell
Printing fixme's once (was: Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9)
Stefan wrote: > I think there was once a discussion about a FIXME_ONCE macro that took care > of this. not sure what happened with that idea. Judging by http://source.winehq.org/git/wine.git/commitdiff/45ead7fe85e7107de91bd79a8ceeb1cb17e4f71d, Alexandre's preferred idiom is static BOOL fixme_once; ... if(!fixme_once++) FIXME("Report bug to Aunt Tilly\n"); It occurs to me that #define WINE_ONCE(s) { static int once; if (!once++) s; } might suffice; you could then write WINE_ONCE(FIXME("Report bug to Aunt Tilly\n")); The previous attempt was more complicated: http://marc.info/?l=wine-devel&m=129396741411607&w=2 - Dan
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 06.08.2011 um 15:24 schrieb Michael Mc Donnell: > I can't find any definitions of UBYTE except for one in > dlls/itss/lzx.c. The BYTE is already defined [1] as a being unsigned > so isn't that ok? Oh, I thought BYTE is signed, and I thought I used UBYTE once. I guess I was mistaken then. So BYTE is OK > Btw. those FIXMEs might produce a lot of messages (one per vertex). Is > there good way to limit the number of messages? A common way is something like this { static BOOL once = false; if (!once) { FIXME("Report this bug to my aunt Tilda please\n"); once = TRUE; } } I think there was once a discussion about a FIXME_ONCE macro that took care of this. not sure what happened with that idea. -BEGIN PGP SIGNATURE- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJOPYD4AAoJEN0/YqbEcdMwCAMP/2snnVv1zympCsqAUtSpYRhV B1vY/VCPsE92XcuMJ9lXSSfydEj3fNEBPOFQLkDtndEFI6vNAEtJ2kk8Z2Lnbhon uveD7ufhEb8/RiL5Q1m72JUEJfighSUvrC6LNFs8lcl8ULjLLGqxA+4/gcKBliYd iVTybx6Ejw4+Nl8OTcGcJ7MbR/MSZ4znWmbAsDecLMrsYgZnxP0AeZVEdEg9YZkF XK5eMhKcULqIzC+swuGKbIg7X8nzwkZlIIigUKU/wASK7xGPr1FDLWFGqYTmpoYZ ZtAyoPq/8MUhKn1UEhWYeB/O0p2lQNNYOdB6adL7Z8fHdwuLOHcUxTA9v7ckAZZ2 ZaDOnwVbtHPL7MfS2zJqKdFotY6dL/QW7PXDphlLaQfBP/CbtMB0SkvSuHn9U71d AiPRMORknTJpZXbf6Knj76URw9fsDGqsQbEegzk9bmAr40Q6uBRQkDovuA0Np183 YEBtLW9JX1jMWNMbQ4ENbkr/xcnzH69osXHAW2nsONTc7UNERzGZ0c/lazZ/uGZS FYxv5AdGxqDMxAzAyXBn94SyFypI6A8c9SSLRUUMKbeEwpm55HhQmgKtEgcCB+VA mGVZXSatkVw6o7m7gTmh9+SVA5QNRTzjGVHKG8R2I2QbsLi+E0B54ysMsfnaHtdr DWZHo5PpOaY8s5kBNLNP =+Huh -END PGP SIGNATURE-
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Hi, A few more thinks I noticed: On Thursday 04 August 2011 11:21:22 Michael Mc Donnell wrote: > +#include In other libs this is guarded with #ifdef HAVE_FLOAT_H - not sure if there are any systems that don't have the header and the rest of the code still compiles, but I recommend to use the same. > +static BOOL weld_float1(void *to, void *from, FLOAT epsilon) > +FLOAT *v1 = to; > +FLOAT *v2 = from; > ... > +memcpy(to, from, sizeof(FLOAT)); A *v1 = *v2 assignment looks nicer. Similarly: > +static BOOL weld_float2(void *to, void *from, FLOAT epsilon) > +D3DXVECTOR2 *v1 = to; > +D3DXVECTOR2 *v2 = from; > ... > +memcpy(to, from, 2 * sizeof(FLOAT)); Either assign v1->x = v2->x and v1->y = v2->y, or use sizeof(D3DXVECTOR2) in the memcpy. Similarly in all the other functions. > +static BOOL weld_ubyte4(void *to, void *from, FLOAT epsilon) > +{ > +BYTE *b1 = to; > +BYTE *b2 = from; I think there's a UBYTE type, or maybe CHAR that is typedefed to unsigned char. > +static BOOL weld_component(void *to, void *from, D3DDECLTYPE type, FLOAT epsilon) > +{ > +switch (type) > +{ > ... > +case D3DDECLTYPE_UNUSED: > +FIXME("D3DDECLTYPE_UNUSED welding not implemented.\n"); > +break; > +} > + > +return FALSE; > +} I'd guess that UNUSED is not valid to create a vdecl in the first place. Since the Mesh API uses an attribute array rather than a IDirect3DVertexDeclaration9 interface it's probably worth a test(if you don't have one in the updateSemantics tests already). Either way since you have the FIXME you should probably make sure it is printed for any unrecognized type number. > +case D3DDECLUSAGE_BLENDINDICES: > +case D3DDECLUSAGE_POSITIONT: > +case D3DDECLUSAGE_FOG: > +case D3DDECLUSAGE_DEPTH: > +case D3DDECLUSAGE_SAMPLE: > +/* Not possible to weld five above usages. */ > +break; I'm not sure what you mean by "not possible", I haven't spotted any of these usages in the tests. But I got a bit lost in the tests, see below. From the tests: > +static void check_vertex_components(int line, int mesh_number, int vertex_number, BYTE *got_ptr, const BYTE *exp_ptr, D3DVERTEXELEMENT9 *declaration) > ... > +FLOAT got = *(got_ptr + decl_ptr->Offset); > +FLOAT exp = *(exp_ptr + decl_ptr->Offset); I think you need some pointer casts here before dereferencing. But since no test uses FLOAT1 this is essentially dead code. Overall the test is pretty hard to read because of the amount of code that is essentially repeated over and over with subtle differences. I don't really see a way around this. You could move some code around, but that won't really change things a lot. I guess we can live with it, it's only the tests after all. Cheers, Stefan On Thursday 04 August 2011 11:21:22 Michael Mc Donnell wrote: > On Thu, Aug 4, 2011 at 10:27 AM, Michael Mc Donnell > > wrote: > > On Thu, Aug 4, 2011 at 8:48 AM, Michael Mc Donnell wrote: > >> On Wed, Aug 3, 2011 at 2:39 PM, Michael Mc Donnell wrote: > >>> Anyway you're right it's probably a good idea to stay clear of them in > >>> order to avoid potential compiler issues. I've updated the patch to > >>> implement UDEC3 and UDEC3N using shifts and masks instead of > >>> bit-fields. It will still only work on little-endian machines though. > >>> Does it look ok? > >> > >> I've rebased my D3DXWeldVertices patch. Git was having trouble > >> applying it after Alexandre committed my ConvertPointRepsToAdjacency. > > > > Sorry for the noise. I accidentally moved some of the code around > > during the rebase merge. I've moved things back in the original order. > > Here's yet another update for D3DXWeldVertices. I'm working on writing > a test for CloneMesh and re-using some of the parts from the > D3DXWeldVertices test, which is why I keep finding these small bugs > > :-) > > In the test I had forgotten to replace all fabs with fabsf. I've also > changed the comparison test to find the maximum absolute difference > for FLOAT1-3. I've finally added a comparison test for FLOAT4. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Aug 4, 2011 at 2:35 PM, Henri Verbeet wrote: > I'm sure there's all kinds of macro abuse you can do to make it work > in a somewhat portable way, but I don't think it's worth it. Thanks for the explanation Max, but I agree with Henri that it's not worth the trouble for me. I've already implemented the conversion I needed using shifts and masks.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
I'm sure there's all kinds of macro abuse you can do to make it work in a somewhat portable way, but I don't think it's worth it.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 08/03/2011 09:28 AM, Henri Verbeet wrote: Well yes, it's implementation defined, not undefined. The point is that there isn't necessarily any relation to endianness. Just use shifts and masks. Correct. It is a different issue from endedness. But you can use configurable wrappers to cloak the compiler differences: Meta procedure: 1) Write a test to determine which order bit fields are assigned in: int main(){ union field_order { int an_int; int a_bit:1 } test; test.an_int=0; test.a_bit=1; return test.an_int == 1; } which returns 0 if bit fields are allocated most significant bits first, 1 otherwise. 2) Add a test to configure that checks the field allocation order. Record the result of the test as LO_ORDER_FIRST or something. 3) Write some macros that hide the declaration order: #if LO_ORDER_FIRST #define BIT_FIELDS_2(a,b) a;b; #define BIT_FIELDS_3(a,b,c) a;b;c; #define BIT_FIELDS_4(a,b,c,d) a;b;c;d; ... #else #define BIT_FIELDS_2(a,b) b;a; #define BIT_FIELDS_3(a,b,c) c;b;a; #define BIT_FIELDS_4(a,b,c,d) d;c;b;a; ... #endif 4) Constraint: The combined fields must have the same number of bits as the underlying data type. For example, if 'int' has 32 bits, the total size of all the fields in the group must be 32 bits. THIS HAS PROBABLY BEEN DONE ALREADY!, you just have to dig it out of the list of available configure tests. max (I'm in the middle of something else, otherwise, I'd dig it out myself...)
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Well yes, it's implementation defined, not undefined. The point is that there isn't necessarily any relation to endianness. Just use shifts and masks.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 08/03/2011 05:18 AM, Stefan Dösinger wrote: On Wednesday 03 August 2011 10:56:25 Michael Mc Donnell wrote: It is *technically* undefined, but all the compilers I have tested it on do the same thing. There may be a future compiler that behaves differently. You may get away with it right now, but it will cause pain in the rear sooner or later. (I didn't know bitfield layout is undefined, otherwise I wouldn't have advised you to keep using them) Technically they are 'implementation defined', not 'undefined', which means each compiler has to define them but different compilers may define them differently. Because different compilers DO define them differently, they are not portable. Since they are not portable, wine should not use them without cloaking them in macros that compensate for the differences. Which is a PITA. Has anybody done the cloaks already? Max
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wednesday 03 August 2011 10:56:25 Michael Mc Donnell wrote: > It is *technically* undefined, but all the compilers I have tested it > on do the same thing. There may be a future compiler that behaves differently. You may get away with it right now, but it will cause pain in the rear sooner or later. (I didn't know bitfield layout is undefined, otherwise I wouldn't have advised you to keep using them) signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 1 August 2011 14:08, Michael Mc Donnell wrote: >> With the bitfields I'm not sure about stuff like endianess. My gut feeling >> would be to use bitmasks and shifts to separate a DWORD instead, but >> bitfields >> certainly look nicer. Beyond that endianess is a somewhat academic >> consideration with an API that's available on x86 only. So I'd say keep the >> bitfields. > > Ok good. That'll keep me from the pain of doing bit twiddling on a > little endian machine :-) > You can't do that, even on x86. Bitfield memory layout is undefined.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sunday 31 July 2011 12:02:35 Michael Mc Donnell wrote: > I figured out how to do the conversion using bit-fields. I've updated > the patch to include a test and implementation of UDEC3 and DEC3N > welding. Nice, I was just about to reply not to bother too much about it. No Windows driver I know of supports those formats. Wined3d doesn't implement them either. With the bitfields I'm not sure about stuff like endianess. My gut feeling would be to use bitmasks and shifts to separate a DWORD instead, but bitfields certainly look nicer. Beyond that endianess is a somewhat academic consideration with an API that's available on x86 only. So I'd say keep the bitfields. d3dx9 has a few of those issues(and similar things like float vs double) that should be good for a search and replace job. I've fixed a few of those issues in wined3d and friends, but I'm sure more are remaining. On Saturday 30 July 2011 14:13:00 Michael Mc Donnell wrote: > I've implemented D3DCOLOR welding as UBYTE4N welding as you described. > Test 14 improves the color comparison to check that it is correct. > +static BOOL weld_short4(void *to, void *from, float epsilon) > ... > SHORT truncated_epsilon = epsilon; This will cause a precision loss warning on msvc. Adding a cast should do. Some nitpicking: struct D3DXWELDEPSILONS uses "FLOAT" not "float". It is advised to stick to the API types. This is quite academic since a "F+static BOOL weld_short4(void *to, void *from, float epsilon) LOAT" is afaik just a typedef for "float", but it helps when porting Wine to different CPUs. I think we had problems with LONG vs long on amd64 because win32 uses 32 bit LONGs or something like that. > I've also added tests 25 and 27 for color usage index larger than 1. > The usage index is not capped like TEXCOORD > 7. It instead uses the > default value of 1e-6f. I've changed the implementation so it uses the > default value instead of printing out an error message. That's interesting. I wonder what those people in Redmond are smoking :-) signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 26.07.2011 um 11:29 schrieb Michael Mc Donnell: >> Duplicating the code for 16 and 32 bit indices looks a bit ugly. Maybe you >> can use inline functions to read and write values from the proper buffer? >> The other possibility, which the ddraw blitting code uses is to write a sort >> of template as a macro and then generate both versions, but I don't like >> that idea. > > Yes I agree it's ugly. I've added the functions read_ib and write_ib > to handle 32 bit indices. Does that look like a good solution to you > too? Looks OK to me. >> Wrt the D3DDECLTYPE epsilon, it may be more efficient to scale the epsilon >> and calculate the diff and comparison as UBYTEs rather than floats. You'll >> need the diff functions for the other types like *BYTE*, *SHORT* anyway. >> Also tests would be helpful here, especially how the epsilon is treated in >> normalized values like D3DCOLOR and UBYTE4N and how it is treated in >> non-normalized values like UBYTE4. > > Ok I'll look into that and get back to you later. > >> Wrt the usage/index fields, what happens when a combination that isn't >> supported by the fixed function pipeline is used, e.g. NORMAL3 or POSITION5? >> Those are valid in a vertex declaration and can be used with shaders. > > It works because the code does not check the UsageIndex field, except > for differentiating between DIFFUSE and SPECULAR. I've added test 13 > that shows this (tested on Windows 7 and Linux). You may still get into trouble with something like TEXCOORD10 I'd recommend to change test 13(or add a new one) that tests a TEXCOORD > 7 and COLOR > 1 -BEGIN PGP SIGNATURE- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJOMWvFAAoJEN0/YqbEcdMwwxgP/2oGMavFpdFWaa1GSUY6sVds mkgW5c2vKsb6p5LDFegrkSbwj9m01Wlw9LNqy7nQAExSrm3eW8ZBdyg0z57dWch3 7XBtLZr7cE8VZ2KkS4U5pQd6ixxrj6+McJJuzxE2YBBof4eugUNVN/GA9PV4wG8e LISF6sTmvPxUlauSyxdQGstO7Hv+jx9vvq29du5Gs6CCRwhU16Ho6W/+2vtba+do aSVLqqMfc4w9elgigJSx7RjdjiZvNTtXYYOLA/XKGxsTk0RIDLamyUaAjKWYs0aq a8KvhJnVcpTuE8ZS0F9IuDQWQFrXBIDXGKD69m0KwBYWqmQt3gAH3N6NEuQHkyJF AzYBSA+7MxPmKMhCTzaJyE63YfzdbxaEM0CoCFPoCEk8atysUqhJDMar0o2Bj1dX 1wAEq7biDc8FVyWK8RkeAirFz8emhhTUjtJspSZoqtS9dXIdlTB3hUdTSvgcZOLs HsHqhkKUxKx7RUWqZN/1RtoSbqUWofUIMHM34R8dpJ/+sDkNQ53KRJW8lW3/WepM aL3pph+1uHCP47E7UiEi1+zqLnJRkIIyHCQllyUjM2m6E3TayfijzjJKIPUBAwAu YH2DKp883217SouA4pXhe5KAdQZWoNb/AFvVsK84grAzYKPCSy1BPKMFG0PE88Gr ZouqTTGYGAqdEyN2vel9 =cmM9 -END PGP SIGNATURE-
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Hi, A few more comments, with one day delay: > +static float max_abs_diff_vec2(D3DXVECTOR2 *v1, D3DXVECTOR2 *v2) > +{ > +float diff_x = fabs(v1->x - v2->x); > +float diff_y = fabs(v1->y - v2->y); > + > +return fmax(diff_x, diff_y); > +} fabs and fmax return doubles, probably use the float functions fabsf and fmaxf. gcc doesn't bother about the difference, but msvc generates a precision loss warning. Duplicating the code for 16 and 32 bit indices looks a bit ugly. Maybe you can use inline functions to read and write values from the proper buffer? The other possibility, which the ddraw blitting code uses is to write a sort of template as a macro and then generate both versions, but I don't like that idea. Wrt the D3DDECLTYPE epsilon, it may be more efficient to scale the epsilon and calculate the diff and comparison as UBYTEs rather than floats. You'll need the diff functions for the other types like *BYTE*, *SHORT* anyway. Also tests would be helpful here, especially how the epsilon is treated in normalized values like D3DCOLOR and UBYTE4N and how it is treated in non-normalized values like UBYTE4. Wrt the usage/index fields, what happens when a combination that isn't supported by the fixed function pipeline is used, e.g. NORMAL3 or POSITION5? Those are valid in a vertex declaration and can be used with shaders. In the test: > +int vertex_size_normal = sizeof(struct vertex_normal); > +int vertex_size_blendweight = sizeof(struct vertex_blendweight); > ... You could make those unsigned. I'm not quite done reading the test yet, I'll send another mail if I find more. Think about caching pointreps and adjacency? Am 24.07.2011 um 11:57 schrieb Michael Mc Donnell: > On Sun, Jul 24, 2011 at 12:18 AM, Stefan Dösinger > wrote: >> I just started reading this, I'll write more once I am done. Just a quick >> question: What do you mean with "built in function" in the comment above >> color_to_vector? > > I wanted to ask if there was a DirectX function or macro that can > convert a D3DCOLOR, which is four bytes in the range 0-255, to four > floats in the range 0-1 or a D3DXVECTOR4? There's none I know of. I vaguely remember that there was a macro to construct D3DCOLORs, but that's not too helpful here. > Reading this again I think color_to_vector should probably accept a > D3DCOLOR instead of "BYTE color[4]"? Yes, it would be cleaner. Also keep UBYTE4N in mind, which is pretty much the same as D3DCOLOR for your purposes. It just has a different component ordering(bgra in D3DCOLOR vs rgba in UBYTE4N if I'm not mistaken) when you're using it for drawing. As far as I understand WeldVertices it doesn't compare two attributes of different types, bit if you do you have to watch out there. -BEGIN PGP SIGNATURE- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJOLXfpAAoJEN0/YqbEcdMwWWsP/j0Ht34moNt0o4s+cAhiTjk8 1jL4elu9v2SBikvg22W6qjP6SuC+tanFAa0Wv+lsRP9PRBhlT8xkQFC/qdxveAxJ 2tRnWeqa7FJ/0SuOvsD0sTwnhBbPkO9qnv/Y5aBXw6a8O/d1xaNXLXk9rZzo0Aob dkyc9DCCPdf8+ke4dwm9GAvdu1O/y2sQgYENqUe7PrXwRgzeRcf3yZ7pMfdoEiyD A7yS74bywQyiO/mUvI/s/9sViAYD8KL/cRP5wRiz1T9QISnzCyxqW+5stuoXgHZS TVEREOdlMKvwpl24T37Aa6A614ESwlETHa9PUeo8C8900xb6Vmf3pcRsDIkTNyDo Jwk0VpxJlZ0Md2LBnyR4GdWhUzaPQx4r6yEtd74JOeZlWxxd8fYmrpHEYTIy3WeI JGsLNIe12oXVtCGxAgpQhYwtiiBgmrQx8rZyhfF+jewUKfQOcRCacn5uYMQLJTQ+ d2g7jsN4Y4BvCCt1RCFRwwWeuoZdAeo5IDApAJBNKhIOqiRORnV2Z4eDDIG1ng1F MtCatSwaeGsLus0ZZDbpJSVkmVkeaPYO9bYxf0NmAKk7tJDYJRT4Anbl/G2IVjKe RRyQVwICk9HS42AKqgt8ti3kh8khQ0/h3hdhlDZ67GEI+tdzPUVtpADGWKTzRS9f BrcWDV0x7reDLEIOLfR4 =hkUU -END PGP SIGNATURE-
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sun, Jul 24, 2011 at 12:18 AM, Stefan Dösinger wrote: > I just started reading this, I'll write more once I am done. Just a quick > question: What do you mean with "built in function" in the comment above > color_to_vector? I wanted to ask if there was a DirectX function or macro that can convert a D3DCOLOR, which is four bytes in the range 0-255, to four floats in the range 0-1 or a D3DXVECTOR4? Reading this again I think color_to_vector should probably accept a D3DCOLOR instead of "BYTE color[4]"? Thanks, Michael
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Hi, I just started reading this, I'll write more once I am done. Just a quick question: What do you mean with "built in function" in the comment above color_to_vector? Stefan On Friday 22 July 2011 12:56:14 Michael Mc Donnell wrote: > Here is my stab at D3DXWeldVertices in one big patch. It implements > most of the case I could think of except for attribute sorting (which > I don't completely understand how works). > > I have also attached a test case for attribute sorting as a separate > patch. I do not intend to submit that test case because it cannot > easily be made into a todo_wine without producing "succeeded inside > todo block" messages. It is simply here for reference if someone later > wants to implement it. I did not implement attribute sorting because I > think it might require some changes to OptimizeInPlace, which I > already use for compacting the mesh and building the vertex_remap > array. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Hi Alexandre Could I get you to commit my ConvertAdjacencyToPointReps patches? They have ID 76054 and 76055 on http://source.winehq.org/patches/ Dylan Smith thinks they look ok. Thanks, Michael Mc Donnell
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, Jul 6, 2011 at 1:54 PM, Michael Mc Donnell wrote: > On Wed, Jul 6, 2011 at 1:32 PM, Stefan Dösinger > wrote: >> On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote: >>> The D3DXWeldVertices function is stubbed in the spec file, so I needed >>> to add it as a real stub. Does this look correct(works here)? >> Looks good to me. Also check the other d3dx9 libs, they should forward their >> implementations to d3dx9_36 via the spec file if the function exists in the >> native version of that dll. > > I think they're already forwarded. In all the other spec files I find: > > @ stdcall D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) > d3dx9_36.D3DXWeldVertices > > That means they're all forwarded to d3dx9_36? > Btw. I just found a bug in the documentation, so here's the updated version. From db34038a78d6a355a8337844f89397f5c8b1fbce Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Tue, 5 Jul 2011 23:06:37 +0200 Subject: d3dx9: Add stub for D3DXWeldVertices. Fixed bug in documentation --- dlls/d3dx9_36/d3dx9_36.spec |2 +- dlls/d3dx9_36/mesh.c| 37 + 2 files changed, 38 insertions(+), 1 deletions(-) diff --git a/dlls/d3dx9_36/d3dx9_36.spec b/dlls/d3dx9_36/d3dx9_36.spec index 5973d20..27cc6e6 100644 --- a/dlls/d3dx9_36/d3dx9_36.spec +++ b/dlls/d3dx9_36/d3dx9_36.spec @@ -333,4 +333,4 @@ @ stdcall D3DXVec4Normalize(ptr ptr) @ stdcall D3DXVec4Transform(ptr ptr ptr) @ stdcall D3DXVec4TransformArray(ptr long ptr long ptr long) -@ stub D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) +@ stdcall D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c index 2807150..311a35c 100644 --- a/dlls/d3dx9_36/mesh.c +++ b/dlls/d3dx9_36/mesh.c @@ -5210,3 +5210,40 @@ error: return hr; } + +/* + * D3DXWeldVertices(D3DX9_36.@) + * + * Welds together close vertices. The distance between vert- + * ices can be the position and/or other components, e.g. + * normal and texture coordinates. + * + * PARAMS + * mesh [I] Mesh which vertices will be welded together. + * flags[I] D3DXWELDEPSILONSFLAGS specifying how to weld. + * epsilons [I] How close each attribute needs to be for welding. + * adjacency[I] Which faces are adjacent to other faces. + * adjacency_out[O] Updated adjacency after welding. + * face_remap_out [O] Which faces the old faces have been mapped to. + * vertex_remap_out [O] Which vertices the old vertices have been mapped to. + * + * RETURNS + * Success: D3D_OK. + * Failure: D3DERR_INVALIDCALL. + * + * BUGS + * Unimplemented + */ +HRESULT WINAPI D3DXWeldVertices(LPD3DXMESH mesh, +DWORD flags, +CONST D3DXWELDEPSILONS *epsilons, +CONST DWORD *adjacency, +DWORD *adjacency_out, +DWORD *face_remap_out, +LPD3DXBUFFER *vertex_remap_out) +{ +FIXME("(%p, %x, %p, %p, %p, %p, %p): stub\n", mesh, flags, epsilons, + adjacency, adjacency_out, face_remap_out, vertex_remap_out); + +return E_NOTIMPL; +} -- 1.7.5.4
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, Jul 6, 2011 at 1:32 PM, Stefan Dösinger wrote: > On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote: >> The D3DXWeldVertices function is stubbed in the spec file, so I needed >> to add it as a real stub. Does this look correct(works here)? > Looks good to me. Also check the other d3dx9 libs, they should forward their > implementations to d3dx9_36 via the spec file if the function exists in the > native version of that dll. I think they're already forwarded. In all the other spec files I find: @ stdcall D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) d3dx9_36.D3DXWeldVertices That means they're all forwarded to d3dx9_36?
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 6 July 2011 13:32, Stefan Dösinger wrote: > On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote: >> The D3DXWeldVertices function is stubbed in the spec file, so I needed >> to add it as a real stub. Does this look correct(works here)? > Looks good to me. Also check the other d3dx9 libs, they should forward their > implementations to d3dx9_36 via the spec file if the function exists in the > native version of that dll. > tools/make_specfiles should probably take care of that.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote: > The D3DXWeldVertices function is stubbed in the spec file, so I needed > to add it as a real stub. Does this look correct(works here)? Looks good to me. Also check the other d3dx9 libs, they should forward their implementations to d3dx9_36 via the spec file if the function exists in the native version of that dll. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
The D3DXWeldVertices function is stubbed in the spec file, so I needed to add it as a real stub. Does this look correct(works here)? From 58a513680ea0562cdb299f9e0b886188692e6f5e Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Tue, 5 Jul 2011 23:06:37 +0200 Subject: d3dx9: Add stub for D3DXWeldVertices. --- dlls/d3dx9_36/d3dx9_36.spec |2 +- dlls/d3dx9_36/mesh.c| 37 + 2 files changed, 38 insertions(+), 1 deletions(-) diff --git a/dlls/d3dx9_36/d3dx9_36.spec b/dlls/d3dx9_36/d3dx9_36.spec index 5973d20..27cc6e6 100644 --- a/dlls/d3dx9_36/d3dx9_36.spec +++ b/dlls/d3dx9_36/d3dx9_36.spec @@ -333,4 +333,4 @@ @ stdcall D3DXVec4Normalize(ptr ptr) @ stdcall D3DXVec4Transform(ptr ptr ptr) @ stdcall D3DXVec4TransformArray(ptr long ptr long ptr long) -@ stub D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) +@ stdcall D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c index 2807150..1f34af3 100644 --- a/dlls/d3dx9_36/mesh.c +++ b/dlls/d3dx9_36/mesh.c @@ -5210,3 +5210,40 @@ error: return hr; } + +/* + * D3DXWeldVertices(D3DX9_36.@) + * + * Welds together close vertices. The distance between vert- + * ices can be the position and/or other attributes, e.g. + * normal and color. + * + * PARAMS + * mesh [I] Mesh which vertices will be welded together. + * flags[I] D3DXWELDEPSILONSFLAGS specifying how to weld. + * epsilons [I] How close each attribute needs to be for welding. + * adjacency[I] Which faces are adjacent to other faces. + * adjacency_out[O] Updated adjacency after welding. + * face_remap_out [O] Which faces the old faces have been mapped to. + * vertex_remap_out [O] Which vertices the old vertices have been mapped to. + * + * RETURNS + * Success: D3D_OK. + * Failure: D3DERR_INVALIDCALL. + * + * BUGS + * Unimplemented + */ +HRESULT WINAPI D3DXWeldVertices(LPD3DXMESH mesh, +DWORD flags, +CONST D3DXWELDEPSILONS *epsilons, +CONST DWORD *adjacency, +DWORD *adjacency_out, +DWORD *face_remap_out, +LPD3DXBUFFER *vertex_remap_out) +{ +FIXME("(%p, %x, %p, %p, %p, %p, %p): stub\n", mesh, flags, epsilons, + adjacency, adjacency_out, face_remap_out, vertex_remap_out); + +return E_NOTIMPL; +} -- 1.7.5.4
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sun, Jul 3, 2011 at 6:25 PM, Dylan Smith wrote: > The latest ConvertPointRepsToAdjacency patches look good. Good I'll sit on a bit and send ConvertAdjacencyToPointReps to wine-patches in the meantime.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
The latest ConvertPointRepsToAdjacency patches look good.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sun, Jul 3, 2011 at 8:41 AM, Dylan Smith wrote: > On Sat, Jul 2, 2011 at 1:01 PM, Michael Mc Donnell > wrote: >> +static HRESULT init_edge_face_map(struct edge_face_map *edge_face_map, >> CONST DWORD *index_buffer, CONST DWORD *point_reps, CONST DWORD num_faces) >> +{ > ... >> + TRACE("(%p, %p, %p, %d)\n", edge_face_map, index_buffer, point_reps, >> num_faces); > > A TRACE call for an internal initialization function like this seems > unnecessary. They are commonly provided for external functions since > it captures application behavior. Ok, I've removed them. >> + if (!point_reps) /* Identity point reps */ >> + { >> + id_point_reps = generate_identity_point_reps(num_faces, >> num_vertices); >> + if (!id_point_reps) >> + { >> + hr = E_OUTOFMEMORY; >> + goto cleanup; > ... >> + hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, &ib_ptr); >> + if (FAILED(hr)) goto cleanup; > ... >> +cleanup: >> + HeapFree(GetProcessHeap(), 0, id_point_reps); >> + if (indices_are_16_bit) HeapFree(GetProcessHeap(), 0, ib); >> + free_edge_face_map(&edge_face_map); >> + iface->lpVtbl->UnlockIndexBuffer(iface); >> + return hr; >> } > > The index buffer is unconditionally unlocked in the cleanup code, but > goto cleanup can now be called before the index buffer is locked. I've added a check to see if it's locked. >> +static DWORD *generate_identity_point_reps(DWORD num_faces, DWORD >> num_vertices) >> +{ >> + DWORD *id_point_reps; >> + DWORD i; >> + >> + TRACE("(%d, %d)\n", num_faces, num_vertices); >> + >> + id_point_reps = HeapAlloc(GetProcessHeap(), 0, 3 * num_faces * >> sizeof(*id_point_reps)); >> + if (!id_point_reps) >> + return NULL; >> + >> + for (i = 0; i < num_vertices; i++) >> + { >> + id_point_reps[i] = i; >> + } >> + for (i = num_vertices; i < 3 * num_faces; i++) >> + { >> + id_point_reps[i] = -1; >> + } >> + >> + return id_point_reps; >> +} > > Why are there exra point reps allocated with -1 value? Won't only > num_vertices point_reps get used? Yes that was a mistake. Only num_vertices are used and not 3*num_faces. I have also removed some unnecessary -1 checks. From 629df56f46f72daf7e4731b406463ff46f5870a9 Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Thu, 23 Jun 2011 17:46:51 +0200 Subject: d3dx9/test: Implemented ConvertPointRepsToAdjacency test. Added shared vertices test. Removed unused indices in point_rep9. --- dlls/d3dx9_36/tests/mesh.c | 548 1 files changed, 548 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 98d77d9..06b6111 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -4904,6 +4904,553 @@ static void test_create_skin_info(void) ok(hr == D3DERR_INVALIDCALL, "Expected D3DERR_INVALIDCALL, got %#x\n", hr); } +static void test_convert_point_reps_to_adjacency(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +const DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +const DWORD options_16bit = D3DXMESH_SYSTEMMEM; +const D3DVERTEXELEMENT9 declaration[] = +{ +{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, +{0, 12, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, +{0, 24, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, +D3DDECL_END() +}; +const unsigned int VERTS_PER_FACE = 3; +void *vertex_buffer; +void *index_buffer; +DWORD *attributes_buffer; +int i, j; +enum color { RED = 0x, GREEN = 0xff00ff00, BLUE = 0xffff}; +struct vertex_pnc +{ +D3DXVECTOR3 position; +D3DXVECTOR3 normal; +enum color color; /* In case of manual visual inspection */ +}; +D3DXVECTOR3 up = {0.0f, 0.0f, 1.0f}; +/* mesh0 (one face) + * + * 0--1 + * | / + * |/ + * 2 + */ +const struct vertex_pnc vertices0[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices0[] = {0, 1, 2}; +const unsigned int num_vertices0 = ARRAY_SIZE(vertices0); +const unsigned int num_faces0 = num_vertices0 / VERTS_PER_FACE; +const DWORD exp_adjacency0[] = {-1, -1, -1}; +const DWORD exp_id_adjacency0[] = {-1, -1, -1}; +const DWORD point_rep0[] = {0, 1, 2}; +/* mesh1 (right) + * + * 0--1 3 + * | / /| + * |/ / | + * 2 5--4 + */ +const struct vertex_pnc vertices1[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{ 3.0f, 3.0f, 0.f}, up, GREEN}, +{{ 3.0f, 0.0f, 0.f}, up, RED}, +
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Fri, Jul 1, 2011 at 7:22 AM, Dylan Smith wrote: > On Thu, Jun 30, 2011 at 3:50 PM, Michael Mc Donnell > wrote: >> I've implemented the look-up scheme that you described. > >> + if (!point_reps) /* Indentity point reps */ >> + { >> + memset(adjacency, -1, 3 * num_faces * sizeof(*adjacency)); >> + return D3D_OK; >> + } > > I think you are missing the most common cases, where vertices are > reused between triangles. For example: > > 0--1 > | /| > |/ | > 2--3 > > If identity point reps are passed to your function it will find the > adjacent edge, but if NULL is passed to the function for point reps, > then it will find no adjacencies even though semantically they should > be the same. Ok, I've added that example to the test and fixed the implementation. > Also, note the spelling error: Indentity -> Identity Check :-) >> +static void free_edge_face_map(struct edge_face_map *edge_face_map) >> +{ >> + if (!edge_face_map) >> + return; >> + >> + HeapFree(GetProcessHeap(), 0, edge_face_map->lists); >> + HeapFree(GetProcessHeap(), 0, edge_face_map->entries); >> +} > ... >> static HRESULT WINAPI ID3DXMeshImpl_ConvertPointRepsToAdjacency(ID3DXMesh >> *iface, CONST DWORD *point_reps, DWORD *adjacency) >> { >> + struct edge_face_map *edge_face_map = NULL; > ... >> + free_edge_face_map(edge_face_map); >> + iface->lpVtbl->UnlockIndexBuffer(iface); >> + return hr; >> } > > edge_face_map isn't freed. Also, it doesn't need to be allocated on the heap. Ok, I've moved it to the stack. >> + hr = iface->lpVtbl->LockIndexBuffer(iface, 0, &ib_ptr); > > The LockIndexBuffer flags could be D3DLOCK_READONLY rather than 0 > (which seems to have read-write semantics). Fixed. Thank you for the good review. From 7fda9efa017444a9c47071afec1c95b9ed6dbb79 Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Thu, 23 Jun 2011 17:46:51 +0200 Subject: d3dx9/test: Implemented ConvertPointRepsToAdjacency test. Added shared vertices test. --- dlls/d3dx9_36/tests/mesh.c | 548 1 files changed, 548 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 98d77d9..e7cb404 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -4904,6 +4904,553 @@ static void test_create_skin_info(void) ok(hr == D3DERR_INVALIDCALL, "Expected D3DERR_INVALIDCALL, got %#x\n", hr); } +static void test_convert_point_reps_to_adjacency(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +const DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +const DWORD options_16bit = D3DXMESH_SYSTEMMEM; +const D3DVERTEXELEMENT9 declaration[] = +{ +{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, +{0, 12, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, +{0, 24, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, +D3DDECL_END() +}; +const unsigned int VERTS_PER_FACE = 3; +void *vertex_buffer; +void *index_buffer; +DWORD *attributes_buffer; +int i, j; +enum color { RED = 0x, GREEN = 0xff00ff00, BLUE = 0xffff}; +struct vertex_pnc +{ +D3DXVECTOR3 position; +D3DXVECTOR3 normal; +enum color color; /* In case of manual visual inspection */ +}; +D3DXVECTOR3 up = {0.0f, 0.0f, 1.0f}; +/* mesh0 (one face) + * + * 0--1 + * | / + * |/ + * 2 + */ +const struct vertex_pnc vertices0[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices0[] = {0, 1, 2}; +const unsigned int num_vertices0 = ARRAY_SIZE(vertices0); +const unsigned int num_faces0 = num_vertices0 / VERTS_PER_FACE; +const DWORD exp_adjacency0[] = {-1, -1, -1}; +const DWORD exp_id_adjacency0[] = {-1, -1, -1}; +const DWORD point_rep0[] = {0, 1, 2}; +/* mesh1 (right) + * + * 0--1 3 + * | / /| + * |/ / | + * 2 5--4 + */ +const struct vertex_pnc vertices1[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{ 3.0f, 3.0f, 0.f}, up, GREEN}, +{{ 3.0f, 0.0f, 0.f}, up, RED}, +{{ 1.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices1[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices1 = ARRAY_SIZE(vertices1); +const unsigned int num_faces1 = num_vertices1 / VERTS_PER_FACE; +const DWORD exp_adjacency1[] = {-1, 1, -1, -1, -1, 0}; +const DWORD exp_id_adjacency1[] = {-1, -1, -1, -1, -1, -1}; +const DWORD point_rep1[] = {0, 1, 2, 1, 4, 2}; +/* mesh2 (left) + * + *3 0--1 + * /| | / + * / | |/ + * 5--4 2 + */ +const struct vertex_pnc vertices2[] = +{ +{{
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 30 June 2011 18:42, Michael Mc Donnell wrote: > Btw I just searched source.winehq.org for hash_table and found several > hits in different dlls. I think it would be nice if they could all use > the same implementation like list.h and rbtree.h. Maybe. It would probably need to be looked at for each case separately.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sat, Jul 2, 2011 at 1:01 PM, Michael Mc Donnell wrote: > Thank you for the good review. Glad you appreciate them. Here is another. > +static HRESULT init_edge_face_map(struct edge_face_map *edge_face_map, CONST > DWORD *index_buffer, CONST DWORD *point_reps, CONST DWORD num_faces) > +{ ... > +TRACE("(%p, %p, %p, %d)\n", edge_face_map, index_buffer, point_reps, > num_faces); A TRACE call for an internal initialization function like this seems unnecessary. They are commonly provided for external functions since it captures application behavior. > +if (!point_reps) /* Identity point reps */ > +{ > +id_point_reps = generate_identity_point_reps(num_faces, > num_vertices); > +if (!id_point_reps) > +{ > +hr = E_OUTOFMEMORY; > +goto cleanup; ... > +hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, &ib_ptr); > +if (FAILED(hr)) goto cleanup; ... > +cleanup: > +HeapFree(GetProcessHeap(), 0, id_point_reps); > +if (indices_are_16_bit) HeapFree(GetProcessHeap(), 0, ib); > +free_edge_face_map(&edge_face_map); > +iface->lpVtbl->UnlockIndexBuffer(iface); > +return hr; > } The index buffer is unconditionally unlocked in the cleanup code, but goto cleanup can now be called before the index buffer is locked. > +static DWORD *generate_identity_point_reps(DWORD num_faces, DWORD > num_vertices) > +{ > +DWORD *id_point_reps; > +DWORD i; > + > +TRACE("(%d, %d)\n", num_faces, num_vertices); > + > +id_point_reps = HeapAlloc(GetProcessHeap(), 0, 3 * num_faces * > sizeof(*id_point_reps)); > +if (!id_point_reps) > +return NULL; > + > +for (i = 0; i < num_vertices; i++) > +{ > +id_point_reps[i] = i; > +} > +for (i = num_vertices; i < 3 * num_faces; i++) > +{ > +id_point_reps[i] = -1; > +} > + > +return id_point_reps; > +} Why are there exra point reps allocated with -1 value? Won't only num_vertices point_reps get used?
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 30, 2011 at 10:59 PM, Stefan Dösinger wrote: > On Thursday 30 June 2011 21:50:45 Michael Mc Donnell wrote: >> I have another question about my test. I've basically copied all the >> test data from my ConvertAdjacencyToPointReps test, and then just >> inverted the conditions. Should I merge the two tests, put the test >> data in a separate function, or just ignore the duplication? > IMO keeping the tests separated and as simple as possible is more important > than avoiding code duplication here, but I don't think we have a policy about > this. Ok, I'll keep what I have for now.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 30, 2011 at 3:50 PM, Michael Mc Donnell wrote: > I've implemented the look-up scheme that you described. > +if (!point_reps) /* Indentity point reps */ > +{ > +memset(adjacency, -1, 3 * num_faces * sizeof(*adjacency)); > +return D3D_OK; > +} I think you are missing the most common cases, where vertices are reused between triangles. For example: 0--1 | /| |/ | 2--3 If identity point reps are passed to your function it will find the adjacent edge, but if NULL is passed to the function for point reps, then it will find no adjacencies even though semantically they should be the same. Also, note the spelling error: Indentity -> Identity > +static void free_edge_face_map(struct edge_face_map *edge_face_map) > +{ > +if (!edge_face_map) > +return; > + > +HeapFree(GetProcessHeap(), 0, edge_face_map->lists); > +HeapFree(GetProcessHeap(), 0, edge_face_map->entries); > +} ... > static HRESULT WINAPI ID3DXMeshImpl_ConvertPointRepsToAdjacency(ID3DXMesh > *iface, CONST DWORD *point_reps, DWORD *adjacency) > { > +struct edge_face_map *edge_face_map = NULL; ... > +free_edge_face_map(edge_face_map); > +iface->lpVtbl->UnlockIndexBuffer(iface); > +return hr; > } edge_face_map isn't freed. Also, it doesn't need to be allocated on the heap. > +hr = iface->lpVtbl->LockIndexBuffer(iface, 0, &ib_ptr); The LockIndexBuffer flags could be D3DLOCK_READONLY rather than 0 (which seems to have read-write semantics).
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thursday 30 June 2011 21:50:45 Michael Mc Donnell wrote: > I have another question about my test. I've basically copied all the > test data from my ConvertAdjacencyToPointReps test, and then just > inverted the conditions. Should I merge the two tests, put the test > data in a separate function, or just ignore the duplication? IMO keeping the tests separated and as simple as possible is more important than avoiding code duplication here, but I don't think we have a policy about this. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 30, 2011 at 6:42 PM, Michael Mc Donnell wrote: > On Thu, Jun 30, 2011 at 4:10 PM, Henri Verbeet wrote: >> On 30 June 2011 14:49, Michael Mc Donnell wrote: >>> In init_edge_face_map I initialize an array of edge_face structs, and >>> in find_adjacent_face I do a linear search through that array. I would >>> like to replace the array with a hash table, so that the search time >>> becomes constant. I've found a standard list (wine/list.h) and >>> red-black tree implementation (wine/rbtree.h), but not a standard hash >>> table implementation. Is there a standard hash table implementation, >>> should I roll my own or find an LGPL'ed one? >>> >> I'm not sure if a hash table would be faster and how much, but an easy >> way to make the lookup cheaper would be to store the edge -> face map >> as a list for each vertex. >> >> ... >> >> It's then mostly trivial to determine adjacency. This assumes most >> vertices are only part of a handful of edges, but I don't think that's >> unreasonable in practice. > > Thanks for your suggestion. I think you're right that it is safe to > assume that most vertices will only be a part of a few edges. I'll > implement that for now. I've implemented the look-up scheme that you described. I have another question about my test. I've basically copied all the test data from my ConvertAdjacencyToPointReps test, and then just inverted the conditions. Should I merge the two tests, put the test data in a separate function, or just ignore the duplication? From 549a95e2f8f218541d3f54afd08ee522e4e27e3e Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Thu, 23 Jun 2011 17:46:51 +0200 Subject: d3dx9/test: Implemented ConvertPointRepsToAdjacency test. --- dlls/d3dx9_36/tests/mesh.c | 452 1 files changed, 452 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 98d77d9..1da6ed0 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -4904,6 +4904,457 @@ static void test_create_skin_info(void) ok(hr == D3DERR_INVALIDCALL, "Expected D3DERR_INVALIDCALL, got %#x\n", hr); } +static void test_convert_point_reps_to_adjacency(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +const DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +const DWORD options_16bit = D3DXMESH_SYSTEMMEM; +const D3DVERTEXELEMENT9 declaration[] = +{ +{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, +{0, 12, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, +{0, 24, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, +D3DDECL_END() +}; +const unsigned int VERTS_PER_FACE = 3; +void *vertex_buffer; +void *index_buffer; +DWORD *attributes_buffer; +int i, j; +enum color { RED = 0x, GREEN = 0xff00ff00, BLUE = 0xffff}; +struct vertex_pnc +{ +D3DXVECTOR3 position; +D3DXVECTOR3 normal; +enum color color; /* In case of manual visual inspection */ +}; +D3DXVECTOR3 up = {0.0f, 0.0f, 1.0f}; +/* mesh0 (one face) + * + * 0--1 + * | / + * |/ + * 2 + */ +const struct vertex_pnc vertices0[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices0[] = {0, 1, 2}; +const unsigned int num_vertices0 = ARRAY_SIZE(vertices0); +const DWORD exp_adjacency0[] = {-1, -1, -1}; +const DWORD point_rep0[] = {0, 1, 2}; +/* mesh1 (right) + * + * 0--1 3 + * | / /| + * |/ / | + * 2 5--4 + */ +const struct vertex_pnc vertices1[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{ 3.0f, 3.0f, 0.f}, up, GREEN}, +{{ 3.0f, 0.0f, 0.f}, up, RED}, +{{ 1.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices1[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices1 = ARRAY_SIZE(vertices1); +const DWORD exp_adjacency1[] = {-1, 1, -1, -1, -1, 0}; +const DWORD point_rep1[] = {0, 1, 2, 1, 4, 2}; +/* mesh2 (left) + * + *3 0--1 + * /| | / + * / | |/ + * 5--4 2 + */ +const struct vertex_pnc vertices2[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{-1.0f, 3.0f, 0.f}, up, RED}, +{{-1.0f, 0.0f, 0.f}, up, GREEN}, +{{-3.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices2[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices2 = ARRAY_SIZE(vertices2); +const DWORD exp_adjacency2[] = {-1, -1, 1, 0, -1, -1}; +const DWORD point_rep2[] = {0, 1, 2, 0, 2, 5}; +/* mesh3 (above) + * + *3 + * /| + *
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 30, 2011 at 4:10 PM, Henri Verbeet wrote: > On 30 June 2011 14:49, Michael Mc Donnell wrote: >> In init_edge_face_map I initialize an array of edge_face structs, and >> in find_adjacent_face I do a linear search through that array. I would >> like to replace the array with a hash table, so that the search time >> becomes constant. I've found a standard list (wine/list.h) and >> red-black tree implementation (wine/rbtree.h), but not a standard hash >> table implementation. Is there a standard hash table implementation, >> should I roll my own or find an LGPL'ed one? >> > I'm not sure if a hash table would be faster and how much, but an easy > way to make the lookup cheaper would be to store the edge -> face map > as a list for each vertex. > > So e.g. if you have these faces: > f0: v0, v2, v1 > f1: v1, v4, v3 > f2: v1, v2, v4 > f3: v2, v5, v4 > > You'd build these lists: > v0: {v2:f0} > v1: {v0:f0}, {v4:f1}, {v2:f2} > v2: {v1:f0}, {v4:f2}, {v5:f3} > v3: {v1:f1} > v4: {v3:f1}, {v1:f2}, {v2:f3} > v5: {v4:f3} > > It's then mostly trivial to determine adjacency. This assumes most > vertices are only part of a handful of edges, but I don't think that's > unreasonable in practice. Thanks for your suggestion. I think you're right that it is safe to assume that most vertices will only be a part of a few edges. I'll implement that for now. > We did have a hash table inside wined3d at some point, I suppose you > could also try resurrecting that from git history. I'll stick with your suggestion as it seems easier to implement :-) Btw I just searched source.winehq.org for hash_table and found several hits in different dlls. I think it would be nice if they could all use the same implementation like list.h and rbtree.h. That way it could also later be used for other parts of wine. Should I add it to http://wiki.winehq.org/JanitorialProjects?
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 30 June 2011 14:49, Michael Mc Donnell wrote: > In init_edge_face_map I initialize an array of edge_face structs, and > in find_adjacent_face I do a linear search through that array. I would > like to replace the array with a hash table, so that the search time > becomes constant. I've found a standard list (wine/list.h) and > red-black tree implementation (wine/rbtree.h), but not a standard hash > table implementation. Is there a standard hash table implementation, > should I roll my own or find an LGPL'ed one? > I'm not sure if a hash table would be faster and how much, but an easy way to make the lookup cheaper would be to store the edge -> face map as a list for each vertex. So e.g. if you have these faces: f0: v0, v2, v1 f1: v1, v4, v3 f2: v1, v2, v4 f3: v2, v5, v4 You'd build these lists: v0: {v2:f0} v1: {v0:f0}, {v4:f1}, {v2:f2} v2: {v1:f0}, {v4:f2}, {v5:f3} v3: {v1:f1} v4: {v3:f1}, {v1:f2}, {v2:f3} v5: {v4:f3} It's then mostly trivial to determine adjacency. This assumes most vertices are only part of a handful of edges, but I don't think that's unreasonable in practice. We did have a hash table inside wined3d at some point, I suppose you could also try resurrecting that from git history.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
I've implemented ConvertPointRepsToAdjacency, but would like some help to make it's worst case performance bearable. In init_edge_face_map I initialize an array of edge_face structs, and in find_adjacent_face I do a linear search through that array. I would like to replace the array with a hash table, so that the search time becomes constant. I've found a standard list (wine/list.h) and red-black tree implementation (wine/rbtree.h), but not a standard hash table implementation. Is there a standard hash table implementation, should I roll my own or find an LGPL'ed one? Thanks, Michael Mc Donnell From 549a95e2f8f218541d3f54afd08ee522e4e27e3e Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Thu, 23 Jun 2011 17:46:51 +0200 Subject: d3dx9/test: Implemented ConvertPointRepsToAdjacency test. --- dlls/d3dx9_36/tests/mesh.c | 452 1 files changed, 452 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 98d77d9..1da6ed0 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -4904,6 +4904,457 @@ static void test_create_skin_info(void) ok(hr == D3DERR_INVALIDCALL, "Expected D3DERR_INVALIDCALL, got %#x\n", hr); } +static void test_convert_point_reps_to_adjacency(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +const DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +const DWORD options_16bit = D3DXMESH_SYSTEMMEM; +const D3DVERTEXELEMENT9 declaration[] = +{ +{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, +{0, 12, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, +{0, 24, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, +D3DDECL_END() +}; +const unsigned int VERTS_PER_FACE = 3; +void *vertex_buffer; +void *index_buffer; +DWORD *attributes_buffer; +int i, j; +enum color { RED = 0x, GREEN = 0xff00ff00, BLUE = 0xffff}; +struct vertex_pnc +{ +D3DXVECTOR3 position; +D3DXVECTOR3 normal; +enum color color; /* In case of manual visual inspection */ +}; +D3DXVECTOR3 up = {0.0f, 0.0f, 1.0f}; +/* mesh0 (one face) + * + * 0--1 + * | / + * |/ + * 2 + */ +const struct vertex_pnc vertices0[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices0[] = {0, 1, 2}; +const unsigned int num_vertices0 = ARRAY_SIZE(vertices0); +const DWORD exp_adjacency0[] = {-1, -1, -1}; +const DWORD point_rep0[] = {0, 1, 2}; +/* mesh1 (right) + * + * 0--1 3 + * | / /| + * |/ / | + * 2 5--4 + */ +const struct vertex_pnc vertices1[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{ 3.0f, 3.0f, 0.f}, up, GREEN}, +{{ 3.0f, 0.0f, 0.f}, up, RED}, +{{ 1.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices1[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices1 = ARRAY_SIZE(vertices1); +const DWORD exp_adjacency1[] = {-1, 1, -1, -1, -1, 0}; +const DWORD point_rep1[] = {0, 1, 2, 1, 4, 2}; +/* mesh2 (left) + * + *3 0--1 + * /| | / + * / | |/ + * 5--4 2 + */ +const struct vertex_pnc vertices2[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{-1.0f, 3.0f, 0.f}, up, RED}, +{{-1.0f, 0.0f, 0.f}, up, GREEN}, +{{-3.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices2[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices2 = ARRAY_SIZE(vertices2); +const DWORD exp_adjacency2[] = {-1, -1, 1, 0, -1, -1}; +const DWORD point_rep2[] = {0, 1, 2, 0, 2, 5}; +/* mesh3 (above) + * + *3 + * /| + * / | + * 5--4 + * 0--1 + * | / + * |/ + * 2 + */ +struct vertex_pnc vertices3[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{ 2.0f, 7.0f, 0.f}, up, BLUE}, +{{ 2.0f, 4.0f, 0.f}, up, GREEN}, +{{ 0.0f, 4.0f, 0.f}, up, RED}, +}; +const DWORD indices3[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices3 = ARRAY_SIZE(vertices3); +const DWORD exp_adjacency3[] = {1, -1, -1, -1, 0, -1}; +const DWORD point_rep3[] = {0, 1, 2, 3, 1, 0}; +/* mesh4 (below, tip against tip) + * + * 0--1 + * | / + * |/ + * 2 + * 3 + * |\ + * | \ + * 5--4 + */ +struct vertex_pnc vertices4[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Fri, Jun 24, 2011 at 6:12 AM, Dylan Smith wrote: > On Thu, Jun 23, 2011 at 8:18 AM, Michael Mc Donnell > wrote: >> I've change the test a bit. I split up the complex example (mesh6) >> into two simpler examples. I also got inspired by your ASCII >> illustrations and have added my own in the comments. The examples have >> also been updated so that they are easier to draw by hand (except for >> mesh5). >> > > Looks good. Great, thanks for the review. I'll sit on them for a little bit more so others can get a chance to comment on them.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 23, 2011 at 8:18 AM, Michael Mc Donnell wrote: > I've change the test a bit. I split up the complex example (mesh6) > into two simpler examples. I also got inspired by your ASCII > illustrations and have added my own in the comments. The examples have > also been updated so that they are easier to draw by hand (except for > mesh5). > Looks good.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 23, 2011 at 5:50 AM, Dylan Smith wrote: > On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger > wrote: >> I'll have to do some more background reading on the kind of mesh data >> structures used here before I can give qualified comments on your patches, >> so for now I'll yield to Dylan :-) > > I looked over the latest patches, and the code looks good now, but I > have a couple of nitpicks for the comments. Sounds good :-) > +/* ConvertAdjacencyToPointReps helper function. > + * > + * Goes around the edges of a face and replaces the vertices in any adjacent > > (changed word is in caps, but capitalization not needed) > * Goes around the edges of EACH face... Check. > + * The vertices in a point representation must be ordered sequentially, e.g. > + * index 3 holds the index of the vertex that replaces vertex 3, i.e. if > + * vertex 3 is replaced by vertex 5 then index 3 would contain 5. If no > vertex > + * replaces it, then it contains the same number as the index itself, e.g. > + * index 3 would contain 3. */ > > Your example has vertex 5 replacing vertex 3, which doesn't seem > possible if lower vertex indices always replace higher vertex indices. You're right it is the other way around. Thanks for reading it thoroughly. I've change the test a bit. I split up the complex example (mesh6) into two simpler examples. I also got inspired by your ASCII illustrations and have added my own in the comments. The examples have also been updated so that they are easier to draw by hand (except for mesh5). From 14c25fde0240779a948dd0382e4ca672b6edbff1 Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Fri, 27 May 2011 16:24:18 +0200 Subject: d3dx9/tests: Implemented ConvertAdjacencyToPointReps test. --- dlls/d3dx9_36/tests/mesh.c | 408 1 files changed, 408 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 26a0c10..1f5a37e 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -4676,6 +4676,413 @@ cleanup: free_test_context(test_context); } +static void test_convert_adjacency_to_point_reps(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +const DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +const DWORD options_16bit = D3DXMESH_SYSTEMMEM; +const D3DVERTEXELEMENT9 declaration[] = +{ +{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, +{0, 12, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, +{0, 24, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, +D3DDECL_END() +}; +const unsigned int VERTS_PER_FACE = 3; +void *vertex_buffer; +void *index_buffer; +DWORD *attributes_buffer; +int i, j; +enum color { RED = 0x, GREEN = 0xff00ff00, BLUE = 0xffff}; +struct vertex_pnc +{ +D3DXVECTOR3 position; +D3DXVECTOR3 normal; +enum color color; /* In case of manual visual inspection */ +}; +D3DXVECTOR3 up = {0.0f, 0.0f, 1.0f}; +/* mesh0 (one face) + * + * 0--1 + * | / + * |/ + * 2 + */ +const struct vertex_pnc vertices0[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices0[] = {0, 1, 2}; +const unsigned int num_vertices0 = ARRAY_SIZE(vertices0); +const DWORD adjacency0[] = {-1, -1, -1}; +DWORD point_rep0[num_vertices0]; +const DWORD exp_point_rep0[] = {0, 1, 2}; +/* mesh1 (right) + * + * 0--1 3 + * | / /| + * |/ / | + * 2 5--4 + */ +const struct vertex_pnc vertices1[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{ 3.0f, 3.0f, 0.f}, up, GREEN}, +{{ 3.0f, 0.0f, 0.f}, up, RED}, +{{ 1.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices1[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices1 = ARRAY_SIZE(vertices1); +const DWORD adjacency1[] = {-1, 1, -1, -1, -1, 0}; +DWORD point_rep1[num_vertices1]; +const DWORD exp_point_rep1[] = {0, 1, 2, 1, 4, 2}; +/* mesh2 (left) + * + *3 0--1 + * /| | / + * / | |/ + * 5--4 2 + */ +const struct vertex_pnc vertices2[] = +{ +{{ 0.0f, 3.0f, 0.f}, up, RED}, +{{ 2.0f, 3.0f, 0.f}, up, GREEN}, +{{ 0.0f, 0.0f, 0.f}, up, BLUE}, + +{{-1.0f, 3.0f, 0.f}, up, RED}, +{{-1.0f, 0.0f, 0.f}, up, GREEN}, +{{-3.0f, 0.0f, 0.f}, up, BLUE}, +}; +const DWORD indices2[] = {0, 1, 2, 3, 4, 5}; +const unsigned int num_vertices2 = ARRAY_SIZE(vertices2); +const DWORD adjacency2[] = {-1, -1, 1, 0, -1, -1}; +DWORD point_rep2[num_vertices2]; +const DWORD exp_point_rep2[] = {0, 1, 2, 0, 2
GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Sorry I made a mistake. On Thu, Jun 23, 2011 at 11:07 AM, Michael Mc Donnell wrote: > Yes that's the basics of it. It does some other funny things too. For > example it re-orders the indices, so if you have > > 0--1 6 3 > | / / | | \ > | / / | | \ > 2 8--7 5--4 > > That corresponds to: > > index buffer = [ 0,1, 2, 6, 7,8, 3, 4,5] > adjacency = [-1,1,-1, 2,-1,0, -1,-1,1] > point rep = [ 0,1, 2, 1, 4,7, 1, 7,2] Index 5 is not replaced by 7 because the lowest index is always used. So it should have been the other way around: point rep = [ 0,1, 2, 1, 4,5, 1, 5,2] > If the indices had not been re-ordered then the point representation > would have been > [0,1,2, 1,7,2, 1,4,7]. And similarly [0,1,2, 1,5,2, 1,4,5]. I'll put this example into the test.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 23, 2011 at 3:30 AM, Dylan Smith wrote: > On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger > wrote: >> I'll have to do some more background reading on the kind of mesh data >> structures used here before I can give qualified comments on your patches, >> so for now I'll yield to Dylan :-) > > I couldn't actually find information on point representative data (aka > PointReps), but a quick search found others wondering what it is. I used chapter 19 of "The Direct3D Graphics Pipeline" by Richard Thomson (his draft book): http://www.xmission.com/~legalize/book/download/19-D3DX%20Mesh%20Objects.pdf > Basically it is an array of vertex indices, one for each vertex, each > with either its own index or the index of an co-located vertex. > e.g. If you have triangles ABC and DEF (as shown below), with two sets > of points co-located, and the vertex buffer stored in alphabetical > order. Then the point_reps array would be [0, 1, 2, 2, 1, 5] to > indicate that vertices B & C can replace E & D respectively. > > A---(B/E) > | / | > | / | > (C/D)--F Yes that's the basics of it. It does some other funny things too. For example it re-orders the indices, so if you have 0--16 3 | // || \ | / / || \ 28--7 5--4 That corresponds to: index buffer = [ 0,1, 2, 6, 7,8, 3, 4,5] adjacency= [-1,1,-1, 2,-1,0, -1,-1,1] point rep = [ 0,1, 2, 1, 4,7, 1, 7,2] If the indices had not been re-ordered then the point representation would have been [0,1,2, 1,7,2, 1,4,7]. Here is an example that shows how it expands collapsed triangles: 0--13 | // | | / / | 24--5 If the triangle 3-4-5 is collapsed by repeating it's first vertex in the index buffer, then it will not be adjacent to 0-1-2: index buffer = [ 0, 1, 2, 3, 3, 3] adjacency= [-1,-1,-1, -1,-1,-1] point rep = [ 0,1, 2, 3, 4,5] The vertices of 3-4-5 are included anyway in the point representation, even though they do not form a real triangle. All these things are tested in mesh6, but I can see now that they probably should be split up into separate test cases.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger wrote: > I'll have to do some more background reading on the kind of mesh data > structures used here before I can give qualified comments on your patches, so > for now I'll yield to Dylan :-) I looked over the latest patches, and the code looks good now, but I have a couple of nitpicks for the comments. +/* ConvertAdjacencyToPointReps helper function. + * + * Goes around the edges of a face and replaces the vertices in any adjacent (changed word is in caps, but capitalization not needed) * Goes around the edges of EACH face... + * The vertices in a point representation must be ordered sequentially, e.g. + * index 3 holds the index of the vertex that replaces vertex 3, i.e. if + * vertex 3 is replaced by vertex 5 then index 3 would contain 5. If no vertex + * replaces it, then it contains the same number as the index itself, e.g. + * index 3 would contain 3. */ Your example has vertex 5 replacing vertex 3, which doesn't seem possible if lower vertex indices always replace higher vertex indices.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger wrote: > I'll have to do some more background reading on the kind of mesh data > structures used here before I can give qualified comments on your patches, so > for now I'll yield to Dylan :-) I couldn't actually find information on point representative data (aka PointReps), but a quick search found others wondering what it is. Basically it is an array of vertex indices, one for each vertex, each with either its own index or the index of an co-located vertex. e.g. If you have triangles ABC and DEF (as shown below), with two sets of points co-located, and the vertex buffer stored in alphabetical order. Then the point_reps array would be [0, 1, 2, 2, 1, 5] to indicate that vertices B & C can replace E & D respectively. A---(B/E) | / | | / | (C/D)--F The adjacency buffer is documented well enough on MSDN for GenerateAdjacency (http://msdn.microsoft.com/en-us/library/bb205737(v=VS.85).aspx) and seems to be used in this method for determining which vertices are co-located. I hope that helps.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 22.06.2011 um 21:48 schrieb Michael Mc Donnell: >> HeapFree checks for NULL for you, so remove the NULL check before >> calling HeapFree. > > Ok. I see that now in the Wine source. That's a lot nicer than on Windows. Windows also checks for NULL pointers in HeapFree. Otherwise Wine wouldn't do so, and we'd need the NULL checks to be able to run our code on Windows. > My UpdateSemantics patch finally got applied Congratulations! I'll have to do some more background reading on the kind of mesh data structures used here before I can give qualified comments on your patches, so for now I'll yield to Dylan :-) -BEGIN PGP SIGNATURE- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJOAkvaAAoJEN0/YqbEcdMwxssP/j1BOavQHLCSJdarIwV6B+NL wkIB9W6L6EsvGxr/8we6mTSVAQ3VMBK74F1yB5BBDNls1YXVugDHm7GI5Yrku4lR mE8YhwudJVZ3i3eKeHBvjRIzkGPW+eUpNkkmJQtQVkpU/Lu/igVDB+pxiFSRxhc4 aAA26bIZ3Uer/wm/1YdIi3geJ05CH4uZpDpWaslARURSP4zofsXiGj5m4KI0WCup 7BtlDdwA9SzamxVHRBTXld0Y4vcg80jBOlmvd6soifD+VI4LCXA31xHBCLR//BAK 8mbdDcPF00mJGRwJKXbL/FJunXRocaOSu0ujtc9Tx9BQbtLGAs0C6VWKgfvvh47M jZ0RFc5ACStWQTkSNPrhvycS188oJ/5mW3aISG+AVuTme3H9zSrNccJYnBpjR+iy GRbn2DmmxRziq/XS4GjW7+oCapHwsESoyZNDxO1fqI0k+gODmGWvR41oONN52/wv QbvAxjsLknwMKvX2MTNVAUoeQWsPB9OIV8HxIPjwxNGFsvM8/ETOprHRyZP2dgNc XcnDQM5/5O2amNRCrRIA4tQaE8QbhxQfe0M9fwqf4XatzYx8tzW1kus9lm9HMEp5 j/FZjBnlU+RjSa+u5TDZxeW5nYwPbrw2ygWcTRom4taZydvWHxWBY4t6IkMfSe+g hBPbh6pFtYZwhpFKWD7u =/xC4 -END PGP SIGNATURE-
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, Jun 22, 2011 at 4:53 PM, Dylan Smith wrote: > On Wed, Jun 22, 2011 at 10:12 AM, Michael Mc Donnell > wrote: >> Thanks your comments. I've attached an updated version. > >> + if (indices) HeapFree(GetProcessHeap(), 0, indices); >> + } >> + if (new_indices) HeapFree(GetProcessHeap(), 0, new_indices); > > HeapFree checks for NULL for you, so remove the NULL check before > calling HeapFree. Ok. I see that now in the Wine source. That's a lot nicer than on Windows. >> + struct >> + { >> + ID3DXMesh *meshes[ARRAY_SIZE(vertices)]; >> + const DWORD *indices[ARRAY_SIZE(vertices)]; >> + const DWORD num_vertices[ARRAY_SIZE(vertices)]; >> + const DWORD *adjacencies[ARRAY_SIZE(vertices)]; >> + DWORD *point_reps[ARRAY_SIZE(vertices)]; >> + const DWORD *exp_point_reps[ARRAY_SIZE(vertices)]; >> + const DWORD options[ARRAY_SIZE(vertices)]; >> + } >> + tc = >> + { >> + {NULL, NULL, NULL, NULL, NULL, NULL, NULL}, >> + {indices0, indices1, indices2, indices3, indices4, indices5, >> indices6, (DWORD*)indices6_16bit}, >> + {num_vertices0, num_vertices1, num_vertices2, num_vertices3, >> num_vertices4, num_vertices5, num_vertices6, num_vertices6}, >> + {adjacency0, adjacency1, adjacency2, adjacency3, adjacency4, >> adjacency5, adjacency6, adjacency6}, >> + {point_rep0, point_rep1, point_rep2, point_rep3, point_rep4, >> point_rep5, point_rep6, point_rep6}, >> + {exp_point_rep0, exp_point_rep1, exp_point_rep2, exp_point_rep3, >> exp_point_rep4, exp_point_rep5, exp_point_rep6, exp_point_rep6}, >> + {options, options, options, options, options, options, options, >> options_16bit} >> + }; > > I actually meant something more like: > > --- snip --- > struct { > const DWORD *indices, > const DWORD num_vertices, > const DWORD *adjacencies, > DWORD *point_reps, > const DWORD *exp_point_reps, > const DWORD options, > } tc[] = { > {indices0, num_vertices0, adjacency0, point_rep0, exp_point_rep0, options}, > {indices1, num_vertices1, adjacency1, point_rep1, exp_point_rep1, options}, > ... > } Ah ok :-) That makes more sense. > ... > > for (i = 0; i < ARRAY_SIZE(tc); i++) > { > ... > } > --- snip --- > > I don't think storing an array of meshes is necessary. You can just > release the mesh and set the mesh variable to NULL (as needed to avoid > re-releasing) before creating another one. I've changed it so that it only keeps the first mesh around for the NULL checks. My UpdateSemantics patch finally got applied so I've removed the duplicate functions too. From 039b27048842da6823f5f8a621cbd22a3c632c6c Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Fri, 27 May 2011 16:24:18 +0200 Subject: d3dx9/tests: Implemented ConvertAdjacencyToPointReps test. --- dlls/d3dx9_36/tests/mesh.c | 332 1 files changed, 332 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 26a0c10..ee9ad67 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -4676,6 +4676,337 @@ cleanup: free_test_context(test_context); } +static void test_convert_adjacency_to_point_reps(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +const DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +const DWORD options_16bit = D3DXMESH_SYSTEMMEM; +const D3DVERTEXELEMENT9 declaration[] = +{ +{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, +{0, 12, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, +{0, 24, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, +D3DDECL_END() +}; +const unsigned int VERTS_PER_FACE = 3; +void *vertex_buffer; +void *index_buffer; +DWORD *attributes_buffer; +int i, j; +struct vertex_pnc +{ +FLOAT x, y, z; /* Position */ +FLOAT nx, ny, nz; /* Normal */ +DWORD color; /* In case of manual visual inspection */ +}; +/* mesh0 (one face) */ +const struct vertex_pnc vertices0[] = +{ +{0.0f, 1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0x}, +{1.0f, -1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xff00ff00}, +{-1.0f, -1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xffff}, +}; +const DWORD indices0[] = {0, 1, 2}; +const unsigned int num_vertices0 = ARRAY_SIZE(vertices0); +const DWORD adjacency0[] = {-1, -1, -1}; +DWORD point_rep0[num_vertices0]; +const DWORD exp_point_rep0[] = {0, 1, 2}; +/* mesh1 (right) */ +const struct vertex_pnc vertices1[] = +{ +{0.0f, 1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0x}, +{1.0f, -1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xff00ff00}, +{-1.0f, -1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xffff}, + +{0.1f, 1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0x}, +{2.1f, 1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xffff}, +
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, Jun 22, 2011 at 10:12 AM, Michael Mc Donnell wrote: > Thanks your comments. I've attached an updated version. > +if (indices) HeapFree(GetProcessHeap(), 0, indices); > +} > +if (new_indices) HeapFree(GetProcessHeap(), 0, new_indices); HeapFree checks for NULL for you, so remove the NULL check before calling HeapFree. > +struct > +{ > + ID3DXMesh *meshes[ARRAY_SIZE(vertices)]; > + const DWORD *indices[ARRAY_SIZE(vertices)]; > + const DWORD num_vertices[ARRAY_SIZE(vertices)]; > + const DWORD *adjacencies[ARRAY_SIZE(vertices)]; > + DWORD *point_reps[ARRAY_SIZE(vertices)]; > + const DWORD *exp_point_reps[ARRAY_SIZE(vertices)]; > + const DWORD options[ARRAY_SIZE(vertices)]; > +} > +tc = > +{ > +{NULL, NULL, NULL, NULL, NULL, NULL, NULL}, > +{indices0, indices1, indices2, indices3, indices4, indices5, > indices6, (DWORD*)indices6_16bit}, > +{num_vertices0, num_vertices1, num_vertices2, num_vertices3, > num_vertices4, num_vertices5, num_vertices6, num_vertices6}, > +{adjacency0, adjacency1, adjacency2, adjacency3, adjacency4, > adjacency5, adjacency6, adjacency6}, > +{point_rep0, point_rep1, point_rep2, point_rep3, point_rep4, > point_rep5, point_rep6, point_rep6}, > +{exp_point_rep0, exp_point_rep1, exp_point_rep2, exp_point_rep3, > exp_point_rep4, exp_point_rep5, exp_point_rep6, exp_point_rep6}, > +{options, options, options, options, options, options, options, > options_16bit} > +}; I actually meant something more like: --- snip --- struct { const DWORD *indices, const DWORD num_vertices, const DWORD *adjacencies, DWORD *point_reps, const DWORD *exp_point_reps, const DWORD options, } tc[] = { {indices0, num_vertices0, adjacency0, point_rep0, exp_point_rep0, options}, {indices1, num_vertices1, adjacency1, point_rep1, exp_point_rep1, options}, ... } ... for (i = 0; i < ARRAY_SIZE(tc); i++) { ... } --- snip --- I don't think storing an array of meshes is necessary. You can just release the mesh and set the mesh variable to NULL (as needed to avoid re-releasing) before creating another one.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, Jun 21, 2011 at 5:36 PM, Dylan Smith wrote: > On Tue, Jun 21, 2011 at 6:32 AM, Michael Mc Donnell > wrote: >> Here is my test and implementation of ConvertAdjacencyToPointReps. > > >> + hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, >> (void**)&indices); >> + if (FAILED(hr)) goto cleanup; >> + memcpy(new_indices, indices, This->numvertices * sizeof(*indices)); > > I think you want the number of faces * 3 rather than the number of > vertices. And the size of an index is not constant, it is 32-bit if > This->options has the D3DXMESH_32BIT bit set and 16-bit if the bit > isn't set. Yes you're right, I'll change that. I had completely forgotten about 16-bit indices, so that required a lot more changes and a test. >> +static void test_convert_adjacency_to_point_reps(void) >> +{ > ... >> + /* All mesh data */ >> + struct vertex_pnc *vertices[] = {vertices0, vertices1, vertices2, >> vertices3, vertices4, vertices5, vertices6}; >> + ID3DXMesh *meshes[ARRAY_SIZE(vertices)]; >> + DWORD *indices[] = {indices0, indices1, indices2, indices3, indices4, >> indices5, indices6}; >> + DWORD num_vertices[] = {num_vertices0, num_vertices1, num_vertices2, >> num_vertices3, num_vertices4, num_vertices5, num_vertices6}; >> + DWORD num_faces[] = {num_faces0, num_faces1, num_faces2, num_faces3, >> num_faces4, num_faces5, num_faces6}; >> + DWORD *adjacencies[] = {adjacency0, adjacency1, adjacency2, adjacency3, >> adjacency4, adjacency5, adjacency6}; >> + DWORD *point_reps[] = {point_rep0, point_rep1, point_rep2, point_rep3, >> point_rep4, point_rep5, point_rep6}; >> + DWORD *exp_point_reps[] = {exp_point_rep0, exp_point_rep1, >> exp_point_rep2, exp_point_rep3, exp_point_rep4, exp_point_rep5, >> exp_point_rep6}; > > I think it would make sense to put all these arrays of the same length > into a structure (e.g. struct test_case) array. This would make it > clear what needs to be updated to add another test case, and less > error prone than making sure each relevant individual array is > updated. Ok I've tried to make a struct for them. > Also, several of these and similar array seem like they can be made constant. Check, I've const'ified everything I could find. >> +static void test_convert_adjacency_to_point_reps(void) >> +{ > ... >> + for (i = 0; i < ARRAY_SIZE(meshes); i++) >> + { > ... >> + hr = meshes[i]->lpVtbl->ConvertAdjacencyToPointReps(meshes[i], >> adjacencies[i], point_reps[i]); >> + todo_wine ok(hr == D3D_OK, "ConvertAdjacencyToPointReps failed. " >> + "Got %x expected D3D_OK\n", hr); > > The value i should probably be printed in this error message. > Otherwise, if you see a failure on test.winehq.org that you can't > reproduce, it will be hard to figure out what might have caused the > failure. Yes, good idea. > That's it for now. Thanks your comments. I've attached an updated version. From be56fecfb515d3407d7fe133ba00a8463745a973 Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Fri, 27 May 2011 16:24:18 +0200 Subject: d3dx9/tests: Implemented ConvertAdjacencyToPointReps test. --- dlls/d3dx9_36/tests/mesh.c | 352 1 files changed, 352 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 668097f..ae4dd26 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -89,6 +90,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct test_context +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* Initializes a test context struct. Use it to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct test_context *new_test_context(void) +{ +HRESULT hr; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct test_context *test_context; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +skip("Couldn't create application window\n"); +goto error; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) +{ +skip("Couldn't create IDirect3D9 object\n"); +goto error; +} + +memset(&d3dpp, 0, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, + D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) +{ +sk
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tuesday 21 June 2011 12:32:28 Michael Mc Donnell wrote: > Note that two of the helper functions and a struct in the test were > also included in my UpdateSemantics patch. I'll remove them once my > UpdateSemantics patch is in the official tree. Your patches aren't applied yet and http://source.winehq.org/patches/ lists them as "New". I recommend to ask Alexandre why he didn't push them. I guess he is waiting for confirmation from a d3dx9 developer, but I think Dylan already gave his OK, and so did I, but I am strictly speaking not involved in d3dx9. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, Jun 21, 2011 at 6:32 AM, Michael Mc Donnell wrote: > Here is my test and implementation of ConvertAdjacencyToPointReps. > + hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, > (void**)&indices); > + if (FAILED(hr)) goto cleanup; > + memcpy(new_indices, indices, This->numvertices * sizeof(*indices)); I think you want the number of faces * 3 rather than the number of vertices. And the size of an index is not constant, it is 32-bit if This->options has the D3DXMESH_32BIT bit set and 16-bit if the bit isn't set. > +static void test_convert_adjacency_to_point_reps(void) > +{ ... > +/* All mesh data */ > +struct vertex_pnc *vertices[] = {vertices0, vertices1, vertices2, > vertices3, vertices4, vertices5, vertices6}; > +ID3DXMesh *meshes[ARRAY_SIZE(vertices)]; > +DWORD *indices[] = {indices0, indices1, indices2, indices3, indices4, > indices5, indices6}; > +DWORD num_vertices[] = {num_vertices0, num_vertices1, num_vertices2, > num_vertices3, num_vertices4, num_vertices5, num_vertices6}; > +DWORD num_faces[] = {num_faces0, num_faces1, num_faces2, num_faces3, > num_faces4, num_faces5, num_faces6}; > +DWORD *adjacencies[] = {adjacency0, adjacency1, adjacency2, adjacency3, > adjacency4, adjacency5, adjacency6}; > +DWORD *point_reps[] = {point_rep0, point_rep1, point_rep2, point_rep3, > point_rep4, point_rep5, point_rep6}; > +DWORD *exp_point_reps[] = {exp_point_rep0, exp_point_rep1, > exp_point_rep2, exp_point_rep3, exp_point_rep4, exp_point_rep5, > exp_point_rep6}; I think it would make sense to put all these arrays of the same length into a structure (e.g. struct test_case) array. This would make it clear what needs to be updated to add another test case, and less error prone than making sure each relevant individual array is updated. Also, several of these and similar array seem like they can be made constant. > +static void test_convert_adjacency_to_point_reps(void) > +{ ... > +for (i = 0; i < ARRAY_SIZE(meshes); i++) > +{ ... > +hr = meshes[i]->lpVtbl->ConvertAdjacencyToPointReps(meshes[i], > adjacencies[i], point_reps[i]); > +todo_wine ok(hr == D3D_OK, "ConvertAdjacencyToPointReps failed. " > + "Got %x expected D3D_OK\n", hr); The value i should probably be printed in this error message. Otherwise, if you see a failure on test.winehq.org that you can't reproduce, it will be hard to figure out what might have caused the failure. That's it for now.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Here is my test and implementation of ConvertAdjacencyToPointReps. Note that two of the helper functions and a struct in the test were also included in my UpdateSemantics patch. I'll remove them once my UpdateSemantics patch is in the official tree. Any comments? Thanks, Michael Mc Donnell From 012ccb29ee8360a33e0c6809ab00ab0ff352fb4a Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Fri, 27 May 2011 16:24:18 +0200 Subject: d3dx9/tests: Implemented ConvertAdjacencyToPointReps test. --- dlls/d3dx9_36/tests/mesh.c | 335 1 files changed, 335 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 668097f..6275511 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -89,6 +90,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct test_context +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* Initializes a test context struct. Use it to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct test_context *new_test_context(void) +{ +HRESULT hr; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct test_context *test_context; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +skip("Couldn't create application window\n"); +goto error; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) +{ +skip("Couldn't create IDirect3D9 object\n"); +goto error; +} + +memset(&d3dpp, 0, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, + D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) +{ +skip("Couldn't create IDirect3DDevice9 object %#x\n", hr); +goto error; +} + +test_context = HeapAlloc(GetProcessHeap(), 0, sizeof(*test_context)); +if (!test_context) +{ +skip("Couldn't allocate memory for test_context\n"); +goto error; +} +test_context->hwnd = hwnd; +test_context->d3d = d3d; +test_context->device = device; + +return test_context; + +error: +if (device) +IDirect3DDevice9_Release(device); + +if (d3d) +IDirect3D9_Release(d3d); + +if (hwnd) +DestroyWindow(hwnd); + +return NULL; +} + +static void free_test_context(struct test_context *test_context) +{ +if (!test_context) +return; + +if (test_context->device) +IDirect3DDevice9_Release(test_context->device); + +if (test_context->d3d) +IDirect3D9_Release(test_context->d3d); + +if (test_context->hwnd) +DestroyWindow(test_context->hwnd); + +HeapFree(GetProcessHeap(), 0, test_context); +} + struct mesh { DWORD number_of_vertices; @@ -4259,6 +4347,252 @@ static void D3DXGenerateAdjacencyTest(void) if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh); } +static void test_convert_adjacency_to_point_reps(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +D3DVERTEXELEMENT9 declaration[] = +{ +{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, +{0, 12, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, +{0, 24, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, +D3DDECL_END() +}; +const unsigned int VERTICES_PER_FACE = 3; +void *vertex_buffer; +void *index_buffer; +DWORD *attributes_buffer; +int i, j; +struct vertex_pnc +{ +FLOAT x, y, z; /* Position */ +FLOAT nx, ny, nz; /* Normal */ +DWORD color; /* In case of manual visual inspection */ +}; +/* mesh0 (one face) */ +struct vertex_pnc vertices0[] = +{ +{0.0f, 1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0x}, +{1.0f, -1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xff00ff00}, +{-1.0f, -1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xffff}, +}; +DWORD indices0[] = {0, 1, 2}; +const unsigned int num_vertices0 = ARRAY_SIZE(vertices0); +const unsigned int num_faces0 = num_vertices0 / VERTICES_PER_FACE; +DWORD adjacency0[] = {-1, -1, -1}; +DWORD point_rep0[num_vertices0]; +DWORD exp_point_rep0[] = {0, 1, 2}; +/* mesh1 (right) */ +struct vertex_pnc vertices1[] = +{ +
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Fri, Jun 17, 2011 at 3:02 PM, Stefan Dösinger wrote: > On Friday 17 June 2011 14:27:55 Michael Mc Donnell wrote: >> Ok I've sent them to wine-patches. > They didn't make it through yet. Are you subscribed? (If you're not > subscribed, don't resend them yet, Newman will most likely let them through > manually later today) Nope I'm not subscribed to wine-patches. I'll wait.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Friday 17 June 2011 14:27:55 Michael Mc Donnell wrote: > Ok I've sent them to wine-patches. They didn't make it through yet. Are you subscribed? (If you're not subscribed, don't resend them yet, Newman will most likely let them through manually later today) signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Fri, Jun 17, 2011 at 1:07 PM, Stefan Dösinger wrote: > On Friday 17 June 2011 12:14:59 Michael Mc Donnell wrote: >> Ok I've added WARN message for every case where it returns an error to >> the application. I've also reworded the WARN where it uses an invalid >> declaration but still must return D3D_OK. > Looks good! Ok I've sent them to wine-patches.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Friday 17 June 2011 12:14:59 Michael Mc Donnell wrote: > Ok I've added WARN message for every case where it returns an error to > the application. I've also reworded the WARN where it uses an invalid > declaration but still must return D3D_OK. Looks good! signature.asc Description: This is a digitally signed message part.
Fwd: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Hi Stefan sorry for the duplicate email. I forgot to also send it to wine-devel. On Fri, Jun 17, 2011 at 12:02 AM, Stefan Dösinger wrote: > On Thursday 16 June 2011 10:49:19 Michael Mc Donnell wrote: >> I've added the test you outlined. It shows you're correct that >> GetDeclaration only writes up to D3DDECL_END(). I've changed the >> implementation so that it caches the number of elements and only >> writes up to D3DDECL_END(). > Looks good to me. Great! >> You're right it could potentially read undefined memory depending on >> the compiler semantics. I think the safest thing is just to read up to >> D3DDECL_END() in the passed in declaration, then it will never read >> undefined memory (except if the programmer makes a mistake). > Looks good, but please write a WARN message in every case where you return an > error to the application. We believe that we handled the error condition > correctly, so there's no need for a FIXME, but the broken app behavior may > have been triggered by some other problem in Wine, so the message is more > important than a simple TRACE. Ok I've added WARN message for every case where it returns an error to the application. I've also reworded the WARN where it uses an invalid declaration but still must return D3D_OK. From b83b22b8d7d338490b33af6129764b1846f36ba2 Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Tue, 24 May 2011 19:44:48 +0200 Subject: d3dx9: Implement UpdateSemantics mesh method. Added a chached vertex declaration and a declaration size field. Changed copy method to not write past D3DECL_END() marker. Handle non-zero stream values. Added more WARN messages. Re-worded WARN message. --- dlls/d3dx9_36/mesh.c | 95 +-- dlls/d3dx9_36/tests/mesh.c | 46 +++--- 2 files changed, 104 insertions(+), 37 deletions(-) diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c index cfadb75..0455ea7 100644 --- a/dlls/d3dx9_36/mesh.c +++ b/dlls/d3dx9_36/mesh.c @@ -6,6 +6,7 @@ * Copyright (C) 2009 David Adam * Copyright (C) 2010 Tony Wasserka * Copyright (C) 2011 Dylan Smith + * Copyright (C) 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -52,7 +53,10 @@ typedef struct ID3DXMeshImpl DWORD options; DWORD fvf; IDirect3DDevice9 *device; +D3DVERTEXELEMENT9 cached_declaration[MAX_FVF_DECL_SIZE]; IDirect3DVertexDeclaration9 *vertex_declaration; +UINT vertex_declaration_size; +UINT num_elem; IDirect3DVertexBuffer9 *vertex_buffer; IDirect3DIndexBuffer9 *index_buffer; DWORD *attrib_buffer; @@ -106,7 +110,8 @@ static ULONG WINAPI ID3DXMeshImpl_Release(ID3DXMesh *iface) { IDirect3DIndexBuffer9_Release(This->index_buffer); IDirect3DVertexBuffer9_Release(This->vertex_buffer); -IDirect3DVertexDeclaration9_Release(This->vertex_declaration); +if (This->vertex_declaration) +IDirect3DVertexDeclaration9_Release(This->vertex_declaration); IDirect3DDevice9_Release(This->device); HeapFree(GetProcessHeap(), 0, This->attrib_buffer); HeapFree(GetProcessHeap(), 0, This->attrib_table); @@ -127,6 +132,12 @@ static HRESULT WINAPI ID3DXMeshImpl_DrawSubset(ID3DXMesh *iface, DWORD attrib_id TRACE("(%p)->(%u)\n", This, attrib_id); +if (!This->vertex_declaration) +{ +WARN("Can't draw a mesh with an invalid vertex declaration.\n"); +return E_FAIL; +} + vertex_size = iface->lpVtbl->GetNumBytesPerVertex(iface); hr = IDirect3DDevice9_SetVertexDeclaration(This->device, This->vertex_declaration); @@ -186,32 +197,31 @@ static DWORD WINAPI ID3DXMeshImpl_GetFVF(ID3DXMesh *iface) return This->fvf; } +static void copy_declaration(D3DVERTEXELEMENT9 *dst, const D3DVERTEXELEMENT9 *src, UINT num_elem) +{ +memcpy(dst, src, num_elem * sizeof(*src)); +} + static HRESULT WINAPI ID3DXMeshImpl_GetDeclaration(ID3DXMesh *iface, D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE]) { ID3DXMeshImpl *This = impl_from_ID3DXMesh(iface); -UINT numelements; TRACE("(%p)\n", This); if (declaration == NULL) return D3DERR_INVALIDCALL; -return IDirect3DVertexDeclaration9_GetDeclaration(This->vertex_declaration, - declaration, - &numelements); +copy_declaration(declaration, This->cached_declaration, This->num_elem); + +return D3D_OK; } static DWORD WINAPI ID3DXMeshImpl_GetNumBytesPerVertex(ID3DXMesh *iface) { ID3DXMeshImpl *This = impl_from_ID3DXMesh(iface); -UINT numelements; -D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE] = { D3DDECL_END() }; TRACE("iface (%p)\n", This); -IDirect3DVertexDeclaration9_GetDeclaration(This->vertex_declaration, - declarati
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, Jun 16, 2011 at 4:49 AM, Michael Mc Donnell wrote: > > Yeah it turned out to be a lot harder than I had expected to get all > the details correct. I have also added a check for non-zero stream > values that Dylan Smith wanted me to add. > Thanks. Looks good to me too now.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, Jun 14, 2011 at 1:43 PM, Stefan Dösinger wrote: > On Tuesday 14 June 2011 12:57:35 Michael Mc Donnell wrote: >> I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size >> of the vertex declaration, so that they can be used by >> GetNumBytesPerVertex and GetDeclaration when an invalid declaration >> has been passed. > > In GetDeclaration: > >> + memcpy(declaration, This->cached_declaration, sizeof(This- >>cached_declaration)); > You should probably test if native really writes the full MAX_FVF_DECL_SIZE > declaration elements, or only up to the D3DDECL_END() marker in the real > declaration. A similar consideration applies to UpdateSemantics, but that is > harder to test. E.g.: > > + D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE]; > + D3DVERTEXELEMENT9 declaration0[] = > + { > ... > > memset(declaration, 0xaa, sizeof(declaration)); > memcpy(declaration, declaration0, sizeof(declaration0)); > UpdateSemantics(declaration) > memset(declaration, 0xbb, sizeof(declaration)); > GetDeclaration(declaration); > > Now look at the bytes in 'declaration' after the D3DDECL_END() marker up to > the end of the array. They could be 0xaa(UpdateSemantics reads everything, > GetDeclaration writes everything), 0xbb(UpdateSemantics is unknown, > GetDeclaration writes only the defined part), or something else(e.g. > UpdateSemantics and/or GetDeclaration clear set the extra bytes to 0). I've added the test you outlined. It shows you're correct that GetDeclaration only writes up to D3DDECL_END(). I've changed the implementation so that it caches the number of elements and only writes up to D3DDECL_END(). > In the way your implementation and tests are currently set up UpdateSemantics > will read undefined memory because your test declarations like declaration0 > don't have the full MAX_FVF_DECL_SIZE array size. Otoh I am not quite sure > about the semantics of passing an array on the stack like this. Maybe the > compiler allocates a new array with the full size and copies the content > around. You're right it could potentially read undefined memory depending on the compiler semantics. I think the safest thing is just to read up to D3DDECL_END() in the passed in declaration, then it will never read undefined memory (except if the programmer makes a mistake). I've changed the implementation to count the number of elements in the new declaration, cache the number, and then copy the contents of new declaration into the cached declaration. Counting the elements doesn't add any extra overhead as I also needed it to check for non-zero stream values. > Also I think this method has been pretty thoroughly tested and debated by now. > If we implement all the functions this way we'd probably be pretty bug-free > :-) Yeah it turned out to be a lot harder than I had expected to get all the details correct. I have also added a check for non-zero stream values that Dylan Smith wanted me to add. > Also, there's the obligatory style nitpick: >> + IDirect3DDevice9 *device; >> ... >> +static struct test_context* new_test_context(void) > For consistency this should probably be static struct test_context > *new_test_context(void). Check :-) From c1c2bc576a5ba3758c15d65d4b21404e80a3c5be Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Tue, 24 May 2011 19:43:47 +0200 Subject: d3dx9/test: Add UpdateSemantics test. Removed a superfluous test. Added NULL check before releasing mesh. Changed struct declaration style. Changed the struct declaration style again. Added GetNumBytesPerVertex test Added GetDeclaration tests Added equals check Renamed larger declaration test to overlapping declaration test. Added smaller and larger declarations. Added D3DECL_END() marker check Moved pointer * to new_test_context (nitpick) Added multiple streams test --- dlls/d3dx9_36/tests/mesh.c | 418 1 files changed, 418 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 668097f..f4b1bbf 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -89,6 +90,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct test_context +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* Initializes a test context struct. Use it to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct test_context *new_test_context(void) +{ +HRESULT hr; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct test_context *test_context; + +
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thursday 16 June 2011 10:49:19 Michael Mc Donnell wrote: > I've added the test you outlined. It shows you're correct that > GetDeclaration only writes up to D3DDECL_END(). I've changed the > implementation so that it caches the number of elements and only > writes up to D3DDECL_END(). Looks good to me. > You're right it could potentially read undefined memory depending on > the compiler semantics. I think the safest thing is just to read up to > D3DDECL_END() in the passed in declaration, then it will never read > undefined memory (except if the programmer makes a mistake). Looks good, but please write a WARN message in every case where you return an error to the application. We believe that we handled the error condition correctly, so there's no need for a FIXME, but the broken app behavior may have been triggered by some other problem in Wine, so the message is more important than a simple TRACE. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, Jun 14, 2011 at 3:42 PM, Dylan Smith wrote: > On Fri, Jun 10, 2011 at 2:08 AM, Dylan Smith wrote: >> D3DXCreateMesh fails when the declaration contains a non-zero Stream >> value. I would expect UpdateSemantics to be as strict as D3DXCreateMesh, >> otherwise a test could validate the different behaviour. > > Seems like you missed this comment I made before. > > /* e.g. */ > memcpy(declaration, declaration0, sizeof(declaration0)); > declaration[1].Stream = 1; > hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration); > ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics using multiple streams, " > "got %#x expected D3DERR_INVALIDCALL\n", hr); Yes you're right, it returns D3DERR_INVALIDCALL when a declaration contains a non-zero Stream value. I'll update my tests and the implementation.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
I have added more tests and changed the UpdateSemantics implementation quite a bit. Dylan Smith made me aware that GetNumBytesPerVertex and GetDeclaration work with an invalid declaration, so I've added tests for that and changed the implementation to do the same. I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size of the vertex declaration, so that they can be used by GetNumBytesPerVertex and GetDeclaration when an invalid declaration has been passed. Any comments? From de65e5c40631806b290a739a3d9bc09a008b9a6d Mon Sep 17 00:00:00 2001 From: Michael Mc Donnell Date: Tue, 24 May 2011 19:43:47 +0200 Subject: d3dx9/test: Add UpdateSemantics test. Removed a superfluous test. Added NULL check before releasing mesh. Changed struct declaration style. Changed the struct declaration style again. Added GetNumBytesPerVertex test Added GetDeclaration tests Added equals check Renamed larger declaration test to overlapping declaration test. Added smaller and larger declarations. --- dlls/d3dx9_36/tests/mesh.c | 369 1 files changed, 369 insertions(+), 0 deletions(-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 668097f..1c84fb7 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -89,6 +90,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct test_context +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* Initializes a test context struct. Use it to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct test_context* new_test_context(void) +{ +HRESULT hr; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct test_context *test_context; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +skip("Couldn't create application window\n"); +goto error; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) +{ +skip("Couldn't create IDirect3D9 object\n"); +goto error; +} + +memset(&d3dpp, 0, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, + D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) +{ +skip("Couldn't create IDirect3DDevice9 object %#x\n", hr); +goto error; +} + +test_context = HeapAlloc(GetProcessHeap(), 0, sizeof(*test_context)); +if (!test_context) +{ +skip("Couldn't allocate memory for test_context\n"); +goto error; +} +test_context->hwnd = hwnd; +test_context->d3d = d3d; +test_context->device = device; + +return test_context; + +error: +if (device) +IDirect3DDevice9_Release(device); + +if (d3d) +IDirect3D9_Release(d3d); + +if (hwnd) +DestroyWindow(hwnd); + +return NULL; +} + +static void free_test_context(struct test_context *test_context) +{ +if (!test_context) +return; + +if (test_context->device) +IDirect3DDevice9_Release(test_context->device); + +if (test_context->d3d) +IDirect3D9_Release(test_context->d3d); + +if (test_context->hwnd) +DestroyWindow(test_context->hwnd); + +HeapFree(GetProcessHeap(), 0, test_context); +} + struct mesh { DWORD number_of_vertices; @@ -4259,6 +4347,286 @@ static void D3DXGenerateAdjacencyTest(void) if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh); } +static void test_update_semantics(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +ID3DXMesh *mesh = NULL; +D3DVERTEXELEMENT9 declaration0[] = +{ + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, + {0, 24, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, + {0, 36, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, + D3DDECL_END() +}; +D3DVERTEXELEMENT9 declaration_smaller[] = +{ + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, + {0, 24, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, + D3DDECL_END() +}; +D3DVERTEXELEMENT9 declaration_larger[] = +{ + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, + {0, 24, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Fri, Jun 10, 2011 at 2:08 AM, Dylan Smith wrote: > D3DXCreateMesh fails when the declaration contains a non-zero Stream > value. I would expect UpdateSemantics to be as strict as D3DXCreateMesh, > otherwise a test could validate the different behaviour. Seems like you missed this comment I made before. /* e.g. */ memcpy(declaration, declaration0, sizeof(declaration0)); declaration[1].Stream = 1; hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration); ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics using multiple streams, " "got %#x expected D3DERR_INVALIDCALL\n", hr);
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tuesday 14 June 2011 12:57:35 Michael Mc Donnell wrote: > I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size > of the vertex declaration, so that they can be used by > GetNumBytesPerVertex and GetDeclaration when an invalid declaration > has been passed. In GetDeclaration: > +memcpy(declaration, This->cached_declaration, sizeof(This- >cached_declaration)); You should probably test if native really writes the full MAX_FVF_DECL_SIZE declaration elements, or only up to the D3DDECL_END() marker in the real declaration. A similar consideration applies to UpdateSemantics, but that is harder to test. E.g.: +D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE]; +D3DVERTEXELEMENT9 declaration0[] = +{ ... memset(declaration, 0xaa, sizeof(declaration)); memcpy(declaration, declaration0, sizeof(declaration0)); UpdateSemantics(declaration) memset(declaration, 0xbb, sizeof(declaration)); GetDeclaration(declaration); Now look at the bytes in 'declaration' after the D3DDECL_END() marker up to the end of the array. They could be 0xaa(UpdateSemantics reads everything, GetDeclaration writes everything), 0xbb(UpdateSemantics is unknown, GetDeclaration writes only the defined part), or something else(e.g. UpdateSemantics and/or GetDeclaration clear set the extra bytes to 0). In the way your implementation and tests are currently set up UpdateSemantics will read undefined memory because your test declarations like declaration0 don't have the full MAX_FVF_DECL_SIZE array size. Otoh I am not quite sure about the semantics of passing an array on the stack like this. Maybe the compiler allocates a new array with the full size and copies the content around. I have to say though that the way the API is defined is a bit odd. But I checked it, and UpdateSemantics and GetDeclaration are defined in this way in the DirectX SDK headers. Also I think this method has been pretty thoroughly tested and debated by now. If we implement all the functions this way we'd probably be pretty bug-free :-) Also, there's the obligatory style nitpick: > +IDirect3DDevice9 *device; > ... > +static struct test_context* new_test_context(void) For consistency this should probably be static struct test_context *new_test_context(void). signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, Jun 8, 2011 at 12:59 PM, Henri Verbeet wrote: > On 8 June 2011 11:33, Michael Mc Donnell wrote: >> Henri, do you have any comments about my UpdateSemantics patches >> before I send them off to wine-patches? >> > Looks fine to me. Good :-) > On the subject of style though, two things: > - I really dislike things like LPDWORD and LPD3DXMESH. For one, > there's the issue that "const LPDWORD" doesn't do what you want, so > you'd need something like "LPCDWORD", which doesn't exist. The other > issue is that unless typedefs are used for some kind of abstraction, > like e.g. uint32_t, they're just obfuscation. I.e., "typedef type > *LPTYPE;" just hides that "LPTYPE" is a pointer, and "typedef > enum/struct x x;" just hides that "x" is a struct or enum. Thank you for the explanation. I've changed those. > - "There’s a standard naming scheme for C, and it’s all lower case > with underscores as separators." Ah yes, I've been writing too much C++ and C# lately :-) I've changed that too. I've also moved the declaration of a vertex struct inside the test function, as it was not needed outside of the test function. Thank you for your comments. diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..ee62d18 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -61,6 +62,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct test_context +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* Initializes a test context struct. Use it to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct test_context* new_test_context(void) +{ +HRESULT hr; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct test_context *test_context; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +skip("Couldn't create application window\n"); +goto error; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) +{ +skip("Couldn't create IDirect3D9 object\n"); +goto error; +} + +memset(&d3dpp, 0, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, + D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) +{ +skip("Couldn't create IDirect3DDevice9 object %#x\n", hr); +goto error; +} + +test_context = HeapAlloc(GetProcessHeap(), 0, sizeof(*test_context)); +if (!test_context) +{ +skip("Couldn't allocate memory for test_context\n"); +goto error; +} +test_context->hwnd = hwnd; +test_context->d3d = d3d; +test_context->device = device; + +return test_context; + +error: +if (device) +IDirect3DDevice9_Release(device); + +if (d3d) +IDirect3D9_Release(d3d); + +if (hwnd) +DestroyWindow(hwnd); + +return NULL; +} + +static void free_test_context(struct test_context *test_context) +{ +if (!test_context) +return; + +if (test_context->device) +IDirect3DDevice9_Release(test_context->device); + +if (test_context->d3d) +IDirect3D9_Release(test_context->d3d); + +if (test_context->hwnd) +DestroyWindow(test_context->hwnd); + +HeapFree(GetProcessHeap(), 0, test_context); +} + struct mesh { DWORD number_of_vertices; @@ -3305,6 +3393,200 @@ static void D3DXGenerateAdjacencyTest(void) if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh); } +static void test_update_semantics(void) +{ +HRESULT hr; +struct test_context *test_context = NULL; +ID3DXMesh *mesh; +D3DVERTEXELEMENT9 declaration0[] = +{ + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, + {0, 24, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, + {0, 36, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, + D3DDECL_END() +}; +D3DVERTEXELEMENT9 declaration_pos_type_color[] = +{ + {0, 0, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, + {0, 24, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, + {0, 36, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, + D3DDECL_END() +}; +D3DVERTEXELEMENT9 declaration_double_usage[] = +{ + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETH
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 8 June 2011 11:33, Michael Mc Donnell wrote: > Henri, do you have any comments about my UpdateSemantics patches > before I send them off to wine-patches? > Looks fine to me. On the subject of style though, two things: - I really dislike things like LPDWORD and LPD3DXMESH. For one, there's the issue that "const LPDWORD" doesn't do what you want, so you'd need something like "LPCDWORD", which doesn't exist. The other issue is that unless typedefs are used for some kind of abstraction, like e.g. uint32_t, they're just obfuscation. I.e., "typedef type *LPTYPE;" just hides that "LPTYPE" is a pointer, and "typedef enum/struct x x;" just hides that "x" is a struct or enum. - "There’s a standard naming scheme for C, and it’s all lower case with underscores as separators."
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, May 31, 2011 at 2:30 PM, Michael Mc Donnell wrote: > On Tue, May 31, 2011 at 12:51 PM, Michael Mc Donnell > wrote: >> On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni >> wrote: >>> 2011/5/30 Michael Mc Donnell : Thanks for the comments Matteo! I have a few questions below. On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni wrote: > 2011/5/29 Michael Mc Donnell : >> On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger >> wrote: >>> On Saturday 28 May 2011 19:55:55 you wrote: >>> It was implemented by setting the vertex declaration to null when the declaration is invalid. That seems to match what happens on Windows, because if a program has set an invalid declaration and then calls GetDeclaration or DrawSubset it will fail with an access violation. Should Wine also fail with an access violation or should it be slightly more graceful and return E_FAIL instead (which my new patch does)? >>> Yes, I think returning E_FAIL is a good idea, but write a WARN if >>> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN >>> will not >>> be shown by default(unlike a FIXME), it is used when the app does >>> something >>> fishy but you think you handled it correctly. >>> + if (FAILED(hr)) + new_vertex_declaration = NULL; + + /* Free old vertex declaration */ + if (This->vertex_declaration) + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); >>> In this situation you can just release the old decl before calling >>> CreateVertexDeclaration and pass &This->vertex_declaration to >>> CreateVdecl. >>> That simplified the code a bit. Don't forget to set >>> This->vertex_declaration to >>> NULL in case of an error though >> >> Ok I've simplified the code. I've also inserted three WARNs. I've >> skipped a WARN when freeing the mesh because that works fine on >> Windows with an invalid declaration. >> + /* Set the position type to color instead of float3 (invalid >>> declaration) */ + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " + "got %#x expected D3D_OK\n", hr); >>> Is this really invalid? I don't know it on top of my head, but in >>> general >>> types and usage are independent, especially with shaders. I don't see a >>> check >>> in d3d9 or wined3d for this case. >> >> You're right it isn't invalid. It doesn't crash, it just doesn't draw >> anything. I've change that comment and re-ordered the tests. >> >>> Beyond that I think the patches look OK. I'd recommend to send them to >>> wine- >>> devel one more time and then next week to wine-patches. Henri is on >>> Vacation >>> this week, and he's usually more picky than I am. I also hope some of >>> the >>> people who regularly contribute to d3dx9 will have a look and comment. >> >> Ok, I've attached the newest version. I'll let the patches sit here >> for a week, and start working on the other functions in the mean time. >> > > The patches are OK with me, just some nitpicking: That sounds great :-) > + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct > TestContext)); > > We usually use something like HeapAlloc(GetProcessHeap(), 0, > sizeof(*testContext)); instead. Ok, I guess that's better in case the type of the variable changes at some point? >>> >>> Yes, that's a reason. The other one is simply consistency. >> >> Ok, I've changed that. >> > I think you can remove some obvious comments (like /* Create the test > mesh */). Also, check your whitespaces. Is it too much or too little whitespace? Could you give me an example? >>> >>> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex >>> size, " >>> >>> + "got %#x expected D3D_OK\n", hr); >>> >>> Here you put 4 spaces on the line continuation, which is neither the >>> "align to be below the '(' + 1 space" used in most of the file, nor >>> the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a >>> nice and consistent coding style (I'm sure you noticed that), so >>> that's mostly trying to not mix even more style variants... >> >> Ok I've changed it to the "align to be below the '(' + 1 space" convention. >> >>> There was also something wrong with spaces in a comment that caught my >>> eye, I think. >> >> The only thing that stood out to me was that I had extra newlines in >> my multi-line comments. I've removed those. >> >>> I told you I was nitpicking... :) >> >> That's ok, I prefer consistency too :-) > > Oops the patches I sent earlier had a tab character and
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, May 31, 2011 at 12:51 PM, Michael Mc Donnell wrote: > On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni > wrote: >> 2011/5/30 Michael Mc Donnell : >>> Thanks for the comments Matteo! I have a few questions below. >>> >>> On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni >>> wrote: 2011/5/29 Michael Mc Donnell : > On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger > wrote: >> On Saturday 28 May 2011 19:55:55 you wrote: >> >>> It was implemented by setting the vertex declaration to null when the >>> declaration is invalid. That seems to match what happens on Windows, >>> because if a program has set an invalid declaration and then calls >>> GetDeclaration or DrawSubset it will fail with an access violation. >>> Should Wine also fail with an access violation or should it be >>> slightly more graceful and return E_FAIL instead (which my new patch >>> does)? >> Yes, I think returning E_FAIL is a good idea, but write a WARN if >> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN >> will not >> be shown by default(unlike a FIXME), it is used when the app does >> something >> fishy but you think you handled it correctly. >> >>> + if (FAILED(hr)) >>> + new_vertex_declaration = NULL; >>> + >>> + /* Free old vertex declaration */ >>> + if (This->vertex_declaration) >>> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); >> In this situation you can just release the old decl before calling >> CreateVertexDeclaration and pass &This->vertex_declaration to >> CreateVdecl. >> That simplified the code a bit. Don't forget to set >> This->vertex_declaration to >> NULL in case of an error though > > Ok I've simplified the code. I've also inserted three WARNs. I've > skipped a WARN when freeing the mesh because that works fine on > Windows with an invalid declaration. > >>> + /* Set the position type to color instead of float3 (invalid >> declaration) */ >>> + hr = mesh->lpVtbl->UpdateSemantics(mesh, >>> declaration_wrong_type_color); >>> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >>> + "got %#x expected D3D_OK\n", hr); >> Is this really invalid? I don't know it on top of my head, but in general >> types and usage are independent, especially with shaders. I don't see a >> check >> in d3d9 or wined3d for this case. > > You're right it isn't invalid. It doesn't crash, it just doesn't draw > anything. I've change that comment and re-ordered the tests. > >> Beyond that I think the patches look OK. I'd recommend to send them to >> wine- >> devel one more time and then next week to wine-patches. Henri is on >> Vacation >> this week, and he's usually more picky than I am. I also hope some of the >> people who regularly contribute to d3dx9 will have a look and comment. > > Ok, I've attached the newest version. I'll let the patches sit here > for a week, and start working on the other functions in the mean time. > The patches are OK with me, just some nitpicking: >>> >>> That sounds great :-) >>> + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead. >>> >>> Ok, I guess that's better in case the type of the variable changes at >>> some point? >>> >> >> Yes, that's a reason. The other one is simply consistency. > > Ok, I've changed that. > I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces. >>> >>> Is it too much or too little whitespace? Could you give me an example? >> >> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, >> " >> >> + "got %#x expected D3D_OK\n", hr); >> >> Here you put 4 spaces on the line continuation, which is neither the >> "align to be below the '(' + 1 space" used in most of the file, nor >> the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a >> nice and consistent coding style (I'm sure you noticed that), so >> that's mostly trying to not mix even more style variants... > > Ok I've changed it to the "align to be below the '(' + 1 space" convention. > >> There was also something wrong with spaces in a comment that caught my >> eye, I think. > > The only thing that stood out to me was that I had extra newlines in > my multi-line comments. I've removed those. > >> I told you I was nitpicking... :) > > That's ok, I prefer consistency too :-) Oops the patches I sent earlier had a tab character and some superfluous whitespace. diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..0876dda 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni wrote: > 2011/5/30 Michael Mc Donnell : >> Thanks for the comments Matteo! I have a few questions below. >> >> On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni >> wrote: >>> 2011/5/29 Michael Mc Donnell : On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger wrote: > On Saturday 28 May 2011 19:55:55 you wrote: > >> It was implemented by setting the vertex declaration to null when the >> declaration is invalid. That seems to match what happens on Windows, >> because if a program has set an invalid declaration and then calls >> GetDeclaration or DrawSubset it will fail with an access violation. >> Should Wine also fail with an access violation or should it be >> slightly more graceful and return E_FAIL instead (which my new patch >> does)? > Yes, I think returning E_FAIL is a good idea, but write a WARN if > CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will > not > be shown by default(unlike a FIXME), it is used when the app does > something > fishy but you think you handled it correctly. > >> + if (FAILED(hr)) >> + new_vertex_declaration = NULL; >> + >> + /* Free old vertex declaration */ >> + if (This->vertex_declaration) >> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); > In this situation you can just release the old decl before calling > CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. > That simplified the code a bit. Don't forget to set > This->vertex_declaration to > NULL in case of an error though Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration. >> + /* Set the position type to color instead of float3 (invalid > declaration) */ >> + hr = mesh->lpVtbl->UpdateSemantics(mesh, >> declaration_wrong_type_color); >> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >> + "got %#x expected D3D_OK\n", hr); > Is this really invalid? I don't know it on top of my head, but in general > types and usage are independent, especially with shaders. I don't see a > check > in d3d9 or wined3d for this case. You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests. > Beyond that I think the patches look OK. I'd recommend to send them to > wine- > devel one more time and then next week to wine-patches. Henri is on > Vacation > this week, and he's usually more picky than I am. I also hope some of the > people who regularly contribute to d3dx9 will have a look and comment. Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time. >>> >>> The patches are OK with me, just some nitpicking: >> >> That sounds great :-) >> >>> + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct >>> TestContext)); >>> >>> We usually use something like HeapAlloc(GetProcessHeap(), 0, >>> sizeof(*testContext)); instead. >> >> Ok, I guess that's better in case the type of the variable changes at >> some point? >> > > Yes, that's a reason. The other one is simply consistency. Ok, I've changed that. >>> I think you can remove some obvious comments (like /* Create the test >>> mesh */). Also, check your whitespaces. >> >> Is it too much or too little whitespace? Could you give me an example? > > + todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, " > > + "got %#x expected D3D_OK\n", hr); > > Here you put 4 spaces on the line continuation, which is neither the > "align to be below the '(' + 1 space" used in most of the file, nor > the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a > nice and consistent coding style (I'm sure you noticed that), so > that's mostly trying to not mix even more style variants... Ok I've changed it to the "align to be below the '(' + 1 space" convention. > There was also something wrong with spaces in a comment that caught my > eye, I think. The only thing that stood out to me was that I had extra newlines in my multi-line comments. I've removed those. > I told you I was nitpicking... :) That's ok, I prefer consistency too :-) diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..447b498 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
2011/5/30 Michael Mc Donnell : > Thanks for the comments Matteo! I have a few questions below. > > On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni > wrote: >> 2011/5/29 Michael Mc Donnell : >>> On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger >>> wrote: On Saturday 28 May 2011 19:55:55 you wrote: > It was implemented by setting the vertex declaration to null when the > declaration is invalid. That seems to match what happens on Windows, > because if a program has set an invalid declaration and then calls > GetDeclaration or DrawSubset it will fail with an access violation. > Should Wine also fail with an access violation or should it be > slightly more graceful and return E_FAIL instead (which my new patch > does)? Yes, I think returning E_FAIL is a good idea, but write a WARN if CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not be shown by default(unlike a FIXME), it is used when the app does something fishy but you think you handled it correctly. > + if (FAILED(hr)) > + new_vertex_declaration = NULL; > + > + /* Free old vertex declaration */ > + if (This->vertex_declaration) > + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); In this situation you can just release the old decl before calling CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. That simplified the code a bit. Don't forget to set This->vertex_declaration to NULL in case of an error though >>> >>> Ok I've simplified the code. I've also inserted three WARNs. I've >>> skipped a WARN when freeing the mesh because that works fine on >>> Windows with an invalid declaration. >>> > + /* Set the position type to color instead of float3 (invalid declaration) */ > + hr = mesh->lpVtbl->UpdateSemantics(mesh, > declaration_wrong_type_color); > + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " > + "got %#x expected D3D_OK\n", hr); Is this really invalid? I don't know it on top of my head, but in general types and usage are independent, especially with shaders. I don't see a check in d3d9 or wined3d for this case. >>> >>> You're right it isn't invalid. It doesn't crash, it just doesn't draw >>> anything. I've change that comment and re-ordered the tests. >>> Beyond that I think the patches look OK. I'd recommend to send them to wine- devel one more time and then next week to wine-patches. Henri is on Vacation this week, and he's usually more picky than I am. I also hope some of the people who regularly contribute to d3dx9 will have a look and comment. >>> >>> Ok, I've attached the newest version. I'll let the patches sit here >>> for a week, and start working on the other functions in the mean time. >>> >> >> The patches are OK with me, just some nitpicking: > > That sounds great :-) > >> + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct >> TestContext)); >> >> We usually use something like HeapAlloc(GetProcessHeap(), 0, >> sizeof(*testContext)); instead. > > Ok, I guess that's better in case the type of the variable changes at > some point? > Yes, that's a reason. The other one is simply consistency. >> I think you can remove some obvious comments (like /* Create the test >> mesh */). Also, check your whitespaces. > > Is it too much or too little whitespace? Could you give me an example? +todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, " +"got %#x expected D3D_OK\n", hr); Here you put 4 spaces on the line continuation, which is neither the "align to be below the '(' + 1 space" used in most of the file, nor the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a nice and consistent coding style (I'm sure you noticed that), so that's mostly trying to not mix even more style variants... There was also something wrong with spaces in a comment that caught my eye, I think. I told you I was nitpicking... :) > > Thanks! >
GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Thanks for the comments Matteo! I have a few questions below. On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni wrote: > 2011/5/29 Michael Mc Donnell : >> On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger >> wrote: >>> On Saturday 28 May 2011 19:55:55 you wrote: >>> It was implemented by setting the vertex declaration to null when the declaration is invalid. That seems to match what happens on Windows, because if a program has set an invalid declaration and then calls GetDeclaration or DrawSubset it will fail with an access violation. Should Wine also fail with an access violation or should it be slightly more graceful and return E_FAIL instead (which my new patch does)? >>> Yes, I think returning E_FAIL is a good idea, but write a WARN if >>> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will >>> not >>> be shown by default(unlike a FIXME), it is used when the app does something >>> fishy but you think you handled it correctly. >>> + if (FAILED(hr)) + new_vertex_declaration = NULL; + + /* Free old vertex declaration */ + if (This->vertex_declaration) + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); >>> In this situation you can just release the old decl before calling >>> CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. >>> That simplified the code a bit. Don't forget to set >>> This->vertex_declaration to >>> NULL in case of an error though >> >> Ok I've simplified the code. I've also inserted three WARNs. I've >> skipped a WARN when freeing the mesh because that works fine on >> Windows with an invalid declaration. >> + /* Set the position type to color instead of float3 (invalid >>> declaration) */ + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " + "got %#x expected D3D_OK\n", hr); >>> Is this really invalid? I don't know it on top of my head, but in general >>> types and usage are independent, especially with shaders. I don't see a >>> check >>> in d3d9 or wined3d for this case. >> >> You're right it isn't invalid. It doesn't crash, it just doesn't draw >> anything. I've change that comment and re-ordered the tests. >> >>> Beyond that I think the patches look OK. I'd recommend to send them to wine- >>> devel one more time and then next week to wine-patches. Henri is on Vacation >>> this week, and he's usually more picky than I am. I also hope some of the >>> people who regularly contribute to d3dx9 will have a look and comment. >> >> Ok, I've attached the newest version. I'll let the patches sit here >> for a week, and start working on the other functions in the mean time. >> > > The patches are OK with me, just some nitpicking: That sounds great :-) > + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); > > We usually use something like HeapAlloc(GetProcessHeap(), 0, > sizeof(*testContext)); instead. Ok, I guess that's better in case the type of the variable changes at some point? > I think you can remove some obvious comments (like /* Create the test > mesh */). Also, check your whitespaces. Is it too much or too little whitespace? Could you give me an example? Thanks!
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
2011/5/29 Michael Mc Donnell : > On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger > wrote: >> On Saturday 28 May 2011 19:55:55 you wrote: >> >>> It was implemented by setting the vertex declaration to null when the >>> declaration is invalid. That seems to match what happens on Windows, >>> because if a program has set an invalid declaration and then calls >>> GetDeclaration or DrawSubset it will fail with an access violation. >>> Should Wine also fail with an access violation or should it be >>> slightly more graceful and return E_FAIL instead (which my new patch >>> does)? >> Yes, I think returning E_FAIL is a good idea, but write a WARN if >> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not >> be shown by default(unlike a FIXME), it is used when the app does something >> fishy but you think you handled it correctly. >> >>> + if (FAILED(hr)) >>> + new_vertex_declaration = NULL; >>> + >>> + /* Free old vertex declaration */ >>> + if (This->vertex_declaration) >>> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); >> In this situation you can just release the old decl before calling >> CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. >> That simplified the code a bit. Don't forget to set This->vertex_declaration >> to >> NULL in case of an error though > > Ok I've simplified the code. I've also inserted three WARNs. I've > skipped a WARN when freeing the mesh because that works fine on > Windows with an invalid declaration. > >>> + /* Set the position type to color instead of float3 (invalid >> declaration) */ >>> + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); >>> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >>> + "got %#x expected D3D_OK\n", hr); >> Is this really invalid? I don't know it on top of my head, but in general >> types and usage are independent, especially with shaders. I don't see a check >> in d3d9 or wined3d for this case. > > You're right it isn't invalid. It doesn't crash, it just doesn't draw > anything. I've change that comment and re-ordered the tests. > >> Beyond that I think the patches look OK. I'd recommend to send them to wine- >> devel one more time and then next week to wine-patches. Henri is on Vacation >> this week, and he's usually more picky than I am. I also hope some of the >> people who regularly contribute to d3dx9 will have a look and comment. > > Ok, I've attached the newest version. I'll let the patches sit here > for a week, and start working on the other functions in the mean time. > The patches are OK with me, just some nitpicking: +testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead. I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger wrote: > On Saturday 28 May 2011 19:55:55 you wrote: > >> It was implemented by setting the vertex declaration to null when the >> declaration is invalid. That seems to match what happens on Windows, >> because if a program has set an invalid declaration and then calls >> GetDeclaration or DrawSubset it will fail with an access violation. >> Should Wine also fail with an access violation or should it be >> slightly more graceful and return E_FAIL instead (which my new patch >> does)? > Yes, I think returning E_FAIL is a good idea, but write a WARN if > CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not > be shown by default(unlike a FIXME), it is used when the app does something > fishy but you think you handled it correctly. > >> + if (FAILED(hr)) >> + new_vertex_declaration = NULL; >> + >> + /* Free old vertex declaration */ >> + if (This->vertex_declaration) >> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); > In this situation you can just release the old decl before calling > CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. > That simplified the code a bit. Don't forget to set This->vertex_declaration > to > NULL in case of an error though Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration. >> + /* Set the position type to color instead of float3 (invalid > declaration) */ >> + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); >> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >> + "got %#x expected D3D_OK\n", hr); > Is this really invalid? I don't know it on top of my head, but in general > types and usage are independent, especially with shaders. I don't see a check > in d3d9 or wined3d for this case. You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests. > Beyond that I think the patches look OK. I'd recommend to send them to wine- > devel one more time and then next week to wine-patches. Henri is on Vacation > this week, and he's usually more picky than I am. I also hope some of the > people who regularly contribute to d3dx9 will have a look and comment. Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time. diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..975c0e8 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -61,6 +62,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct TestContext +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* + * Initializes a TestContext. Use this to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct TestContext* NewTestContext(void) +{ +HRESULT hr; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct TestContext *testContext; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +skip("Couldn't create application window\n"); +goto error; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) +{ +skip("Couldn't create IDirect3D9 object\n"); +goto error; +} + +memset(&d3dpp, 0, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) +{ +skip("Couldn't create IDirect3DDevice9 object %#x\n", hr); +goto error; +} + +testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); +if (!testContext) +{ +skip("Couldn't allocate memory for TestContext\n"); +goto error; +} +testContext->hwnd = hwnd; +testContext->d3d = d3d; +testContext->device = device; + +return testContext; + +error: +if (device) +IDirect3DDevice9_Release(device); + +if (d3d) +IDirect3D9_Release(d3d); + +if (hwnd) +DestroyWindow(hwnd); + +return NULL; +} + +static void FreeTestContext(struct TestContext *testContext) +{ +if (!testContext) +return; + +if (testContext->device) +IDirect3DDevice9_Release(testContext->device
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 26.05.2011 um 16:27 schrieb Michael Mc Donnell: > Yeah I'd prefer it too to be 100% compatible. However, I think it is > highly unlikely that any programs depend on an exception being thrown > by EndScene in a drawing method. What EndScene method is this? The device's methods? I'd be surprised if this method crashed. > But you never know, some people do > crazy things :-) I think the current implementation is good enough as > long as there is no evidence of programs depending on that behavior. > > The problem is that UpdateSemantics in the d3dx9 library depends on > d3d9 and the error code is generated by vertexdeclaration_init in > d3d9. Do the d3d9 vertexdeclaration tests pass on this Windows machine? > I thought of managing an array of D3DVERTEXELEMENT9 in ID3DXMeshImpl > instead of a IDirect3DVertexDeclaration9, and then have > ID3DXMeshImpl_DrawSubset call IDirect3DDevice9_CreateVertexDeclaration > just before IDirect3DDevice9_SetVertexDeclaration. I suspect that this is what native does. This also fits the API design because UpdateSemantics accepts a D3DVERTEXELEMENT9 array and GetDeclaration returns a D3DVERTEXELEMENT9 array. Why did MS name those functions UpdateSemantics and GetDeclaration??? I don't think you'd have extra d3d9 calls by delaying the vdecl creation. Of course you have to keep the vdecl until the next UpdateSemantic call and not recreate it every time you draw. -BEGIN PGP SIGNATURE- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIbBAEBAgAGBQJN3sIsAAoJEN0/YqbEcdMwIjwP+IxUGwlGAFCtgrJBuiVc9p6u wEeG7eK412dCjQBtgjwab4SI4XEZ8+Si555ZtorcVE2LV7jhTmzpEj32VPJYuKKo asmaNmDk9X45aR9kn7/3lwX9alI0mqwFmyB4pp7C9yr9IHhPBE8+6AAspXjgBKgw QIU1yluABO2OvEsFFjEXW5jPG1PTcMoB496iOX/1bDKlm6JjcCYtlv+6jKda2fMm xBe7xTAds4whdIBQ0xbsbcWQoSTLYmRrtkyiHXolKV3IRDtKlc4FHzOkCn3CURz7 gH7z+Z4Oj5HiY42smOAgJtifCD/mwszWj7bTKtxsG4fE2rnWHu/D/8g6oi4dzufp RpWE7uMxYwGirpgcMTIESkocyfj0LHOUzcanBhHnAxdeiNYrit8G9zaF+I58YASC JBBQtWXSTOzgZ0R8G2GcVXOcrTg2bQpsZ8nd5mJVOGQAPj16cRRgBc8iJfTlUZqY S9NAzkqBQly+8cGlTxKi7pWKBcJR64T+/KoLlN3VzoannwkBvvF5nk4uB9WvIgEO +OLmZI3+5bbWBbfHhc7hWs7MYcT0pnZoePRCFjmaKeV6r+z+6XZAUWKCNDhbukXJ Sna+YSaLo47RFnNL2P1zYC8gQ3ohhHKfJbZDJ6DX+bBKsRh45OFA6H0RlYVRRkAP 8GKf5K/pnG+FBaWEIYE= =QHcz -END PGP SIGNATURE-
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Thu, May 26, 2011 at 1:14 PM, Joris Huizer wrote: > On 05/26/2011 10:33 AM, Michael Mc Donnell wrote: >> I've added some more tests to see if I could make it fail. Microsoft's >> UpdateSemantics is not very picky. I can't get it to return anything >> but D3D_OK except for when I pass a null pointer. My implementation >> follows this behavior except for two cases: it returns E_FAIL when >> passing an undefined type and when the offset is not 4 byte aligned. >> This is because the values are checked inside vertexdeclaration_init. >> So Wine is stricter. I don't think that it is necessarily a problem >> because applications that pass bogus declarations like those will >> likely not work anyway. My own tests with a small interactive demo >> show that in those cases the application will crash with an access >> violation when it tries to re-draw the scene. > > I don't know whether it applies here, but if I understand correctly, > there have been cases before that Wine must not be stricter than Windows. > Tthe reason is that a program may 'depend' on a function crashing (it having > an exception caught in that case). > In such a situation, Wine's version of the function not crashing would cause > a code path being executed > that normally never is (causing incorrect behavior even) > > Again I don't know whether it is relevant in this cause. Yeah I'd prefer it too to be 100% compatible. However, I think it is highly unlikely that any programs depend on an exception being thrown by EndScene in a drawing method. But you never know, some people do crazy things :-) I think the current implementation is good enough as long as there is no evidence of programs depending on that behavior. The problem is that UpdateSemantics in the d3dx9 library depends on d3d9 and the error code is generated by vertexdeclaration_init in d3d9. That means d3d9 would have to be changed to make it less picky. That might on the other hand break other things that depend on it being picky (I haven't looked at the tests in d3d9). I thought of managing an array of D3DVERTEXELEMENT9 in ID3DXMeshImpl instead of a IDirect3DVertexDeclaration9, and then have ID3DXMeshImpl_DrawSubset call IDirect3DDevice9_CreateVertexDeclaration just before IDirect3DDevice9_SetVertexDeclaration. But that would not solve the problem as it would just shift the bad call to ID3DXMeshImpl_DrawSubset and not result in an exception in EndScene. It would also result in a lot of extra calls depending on the number of subsets. So I can't see any good way around modifying d3d9, but that might cause other problems as mentioned earlier. So to recapitulate, I think the best solution is to ignore that little difference until we see programs that depend on it. Michael Mc Donnell
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On 05/26/2011 10:33 AM, Michael Mc Donnell wrote: I've added some more tests to see if I could make it fail. Microsoft's UpdateSemantics is not very picky. I can't get it to return anything but D3D_OK except for when I pass a null pointer. My implementation follows this behavior except for two cases: it returns E_FAIL when passing an undefined type and when the offset is not 4 byte aligned. This is because the values are checked inside vertexdeclaration_init. So Wine is stricter. I don't think that it is necessarily a problem because applications that pass bogus declarations like those will likely not work anyway. My own tests with a small interactive demo show that in those cases the application will crash with an access violation when it tries to re-draw the scene. I don't know whether it applies here, but if I understand correctly, there have been cases before that Wine must not be stricter than Windows. Tthe reason is that a program may 'depend' on a function crashing (it having an exception caught in that case). In such a situation, Wine's version of the function not crashing would cause a code path being executed that normally never is (causing incorrect behavior even) Again I don't know whether it is relevant in this cause. HTH, Joris
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Wed, May 25, 2011 at 9:53 AM, Michael Mc Donnell wrote: > On Tue, May 24, 2011 at 8:32 PM, Stefan Dösinger > wrote: >> On Tuesday 24 May 2011 19:56:06 Michael Mc Donnell wrote: >>> > *) In your first test you forgot to check the HeapAlloc result. >>> >>> Ok, I'll return E_OUTOFMEMORY in that case. >> I guess the callers will only abort the tests if NewTestContext fails, so I >> think the NULL / non-NULL return value was better. I've reverted those changes, so it's back to NULL / non-NULL. I've also moved it near the top of the file, so that other tests will be able to re-use the test context creation. >>> Should I try to make some more invalid >>> D3DVERTEXELEMENT9 arrays to see if I can provoke an error? >> I was thinking about something that would make CreateVertexDeclaration fail, >> e.g. declaring a usage+usage index twice or using an undefined type. >> dlls/d3d9/vertexdeclaration.c and dlls/wined3d/vertexdeclaration.c check for >> a >> few error conditions in their vertexdeclaration_init functions. Just pick one >> of them(no need to check all of them) >> But watch out that you don't open a can of worms here. Our >> CreateVertexDeclaration probably doesn't catch all error conditions. I >> recommend that you pick one it does catch, otherwise you'll have to fix >> d3d9.dll and wined3d.dll too for a small test. > > Ok thanks I'll do that. I've added some more tests to see if I could make it fail. Microsoft's UpdateSemantics is not very picky. I can't get it to return anything but D3D_OK except for when I pass a null pointer. My implementation follows this behavior except for two cases: it returns E_FAIL when passing an undefined type and when the offset is not 4 byte aligned. This is because the values are checked inside vertexdeclaration_init. So Wine is stricter. I don't think that it is necessarily a problem because applications that pass bogus declarations like those will likely not work anyway. My own tests with a small interactive demo show that in those cases the application will crash with an access violation when it tries to re-draw the scene. diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..0ca4b25 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -61,6 +62,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct TestContext +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* + * Initializes a TestContext. Use this to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct TestContext* NewTestContext(void) +{ +HRESULT hr; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct TestContext *testContext; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +skip("Couldn't create application window\n"); +goto error; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) +{ +skip("Couldn't create IDirect3D9 object\n"); +goto error; +} + +memset(&d3dpp, 0, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) +{ +skip("Couldn't create IDirect3DDevice9 object %#x\n", hr); +goto error; +} + +testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); +if (!testContext) +{ +skip("Couldn't allocate memory for TestContext\n"); +goto error; +} +testContext->hwnd = hwnd; +testContext->d3d = d3d; +testContext->device = device; + +return testContext; + +error: +if (device) +IDirect3DDevice9_Release(device); + +if (d3d) +IDirect3D9_Release(d3d); + +if (hwnd) +DestroyWindow(hwnd); + +return NULL; +} + +static void FreeTestContext(struct TestContext *testContext) +{ +if (!testContext) +return; + +if (testContext->device) +IDirect3DDevice9_Release(testContext->device); + +if (testContext->d3d) +IDirect3D9_Release(testContext->d3d); + +if (testContext->hwnd) +DestroyWindow(testContext->hwnd); + +HeapFree(GetProcessHeap(), 0, testContext); +} + struct mesh { DWORD number_of_vertices; @@ -3305,6 +3393,200 @@ static void D3DXGenerateAdjacencyTest(void) if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh); } +struct VertexTwoPositions +{ +float x0, y0, z0;
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, May 24, 2011 at 8:32 PM, Stefan Dösinger wrote: > On Tuesday 24 May 2011 19:56:06 Michael Mc Donnell wrote: > >> Sorry I phrased that in a wrong way. In the UpdateSemantics function I >> call IDirect3DDevice9_CreateVertexDeclaration which allocates a new >> Vertex Declaration on the heap. > Is there an alternative? I guess you could cache existing declarations. The > d3d9 API doesn't give you an option to reuse an existing declaration. > > I doubt apps will regularly change the declaration, usually this doesn't make > sense without also changing the data. Of course that doesn't mean there isn't > a broken app out there that does this. Yeah it might not be a problem at all. I'll give it a quick second look to see if it's possible to cache things. I anyway need to read some more code too to see exactly how the APIs interact. >> I also noticed that I didn't call >> IDirect3DDevice9_SetVertexDeclaration. I've added it to stick with the >> defined locking scheme. > Locking scheme? I've just started reading into into the Mesh API, but I don't > think you're supposed to apply the declaration until the mesh is used for > drawing. Ok good, I started to doubt this last night. I'll remove it and try to get a better sense of how the API works. >> > *) In your first test you forgot to check the HeapAlloc result. >> >> Ok, I'll return E_OUTOFMEMORY in that case. > I guess the callers will only abort the tests if NewTestContext fails, so I > think the NULL / non-NULL return value was better. > >> Should I try to make some more invalid >> D3DVERTEXELEMENT9 arrays to see if I can provoke an error? > I was thinking about something that would make CreateVertexDeclaration fail, > e.g. declaring a usage+usage index twice or using an undefined type. > dlls/d3d9/vertexdeclaration.c and dlls/wined3d/vertexdeclaration.c check for a > few error conditions in their vertexdeclaration_init functions. Just pick one > of them(no need to check all of them) > But watch out that you don't open a can of worms here. Our > CreateVertexDeclaration probably doesn't catch all error conditions. I > recommend that you pick one it does catch, otherwise you'll have to fix > d3d9.dll and wined3d.dll too for a small test. Ok thanks I'll do that. >> > *) In the wined3d code(and its client libs) Henri and I avoid structure >> > typedefs. The existing d3dx9 code has a few of them. I'm ok with either >> > way, but maybe you and the other d3dx9 devs want to go the wined3d way. >> >> Sure I can change them. So just normal structs? Is it to keep the >> namespace clean? > Henri started with this change, I think keeping the namespace clean was a > consideration. What you do is up to you and the other devs working on d3dx9. > I'm only suggesting that you discuss the code style and agree on a format. > > Either way it is a fairly minor point IMO, but it tends to end up in holy war. Ok :-) I don't have any strong feelings about it, so I'll just follow your convention until somebody complains.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tuesday 24 May 2011 19:56:06 Michael Mc Donnell wrote: > Sorry I phrased that in a wrong way. In the UpdateSemantics function I > call IDirect3DDevice9_CreateVertexDeclaration which allocates a new > Vertex Declaration on the heap. Is there an alternative? I guess you could cache existing declarations. The d3d9 API doesn't give you an option to reuse an existing declaration. I doubt apps will regularly change the declaration, usually this doesn't make sense without also changing the data. Of course that doesn't mean there isn't a broken app out there that does this. > I also noticed that I didn't call > IDirect3DDevice9_SetVertexDeclaration. I've added it to stick with the > defined locking scheme. Locking scheme? I've just started reading into into the Mesh API, but I don't think you're supposed to apply the declaration until the mesh is used for drawing. > > *) In your first test you forgot to check the HeapAlloc result. > > Ok, I'll return E_OUTOFMEMORY in that case. I guess the callers will only abort the tests if NewTestContext fails, so I think the NULL / non-NULL return value was better. > Should I try to make some more invalid > D3DVERTEXELEMENT9 arrays to see if I can provoke an error? I was thinking about something that would make CreateVertexDeclaration fail, e.g. declaring a usage+usage index twice or using an undefined type. dlls/d3d9/vertexdeclaration.c and dlls/wined3d/vertexdeclaration.c check for a few error conditions in their vertexdeclaration_init functions. Just pick one of them(no need to check all of them) But watch out that you don't open a can of worms here. Our CreateVertexDeclaration probably doesn't catch all error conditions. I recommend that you pick one it does catch, otherwise you'll have to fix d3d9.dll and wined3d.dll too for a small test. > > *) In the wined3d code(and its client libs) Henri and I avoid structure > > typedefs. The existing d3dx9 code has a few of them. I'm ok with either > > way, but maybe you and the other d3dx9 devs want to go the wined3d way. > > Sure I can change them. So just normal structs? Is it to keep the > namespace clean? Henri started with this change, I think keeping the namespace clean was a consideration. What you do is up to you and the other devs working on d3dx9. I'm only suggesting that you discuss the code style and agree on a format. Either way it is a fairly minor point IMO, but it tends to end up in holy war. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Thanks for the lengthy feedback! On Tue, May 24, 2011 at 3:36 PM, Stefan Dösinger wrote: > Hi, > I'm at school right now, so I had only a very quick look. I'll take a closer > look later in the evening. > > On Tuesday 24 May 2011 14:38:19 Michael Mc Donnell wrote: >> This is my first try at implementing the UpdateSemantics mesh method. >> It works fine here on my local machine, passes the new tests, and >> works with my little interactive demo(not included). I still have a >> few questions though. >> >> This implementation allocates new heap memory and I suspect that it >> would be faster to re-use the already allocated memory. > As far as I can see this happens only in the tests, where it doesn't really > matter. Creating and destroying the device is more expensive than the heap > allocation, so if you care about performance you should reuse your test > context structure Sorry I phrased that in a wrong way. In the UpdateSemantics function I call IDirect3DDevice9_CreateVertexDeclaration which allocates a new Vertex Declaration on the heap. I think it might slow things down if UpdateSemantics is in a tight loop where it is used for animation. I also noticed that I didn't call IDirect3DDevice9_SetVertexDeclaration. I've added it to stick with the defined locking scheme. >> Btw should I roll all or some of the patches up into a single patch? > I think you can merge the 3 test patches into one Ok, I've rolled the three test patches into one. >> Any other comments about the patches in general? > > *) >> + * Copyright (C) 2011 Google (Michael Mc Donnell) > Afaik the gsoc conditions say that you keep the copyright to your code :-) Yes you are right. First paragraph in the faq about code :-) > *) In your first test you forgot to check the HeapAlloc result. Ok, I'll return E_OUTOFMEMORY in that case. > *) You could write a test that passes an invalid D3DVERTEXELEMENT9 array to > UpdateSemantics to see how it handles an invalid declaration. I've already got one test with an invalid D3DVERTEXELEMENT9 array. It happily accepts too big declarations even though msdn says "The call is valid only if the old and new declaration formats have the same vertex size". I've added test for null pointers, which it didn't handle correctly. Should I try to make some more invalid D3DVERTEXELEMENT9 arrays to see if I can provoke an error? > A few style suggestions: > > *) memset(mem, 0, size) is preferred over ZeroMemory(mem, size); Ok > *) In the wined3d code(and its client libs) Henri and I avoid structure > typedefs. The existing d3dx9 code has a few of them. I'm ok with either way, > but maybe you and the other d3dx9 devs want to go the wined3d way. Sure I can change them. So just normal structs? Is it to keep the namespace clean? > *) In the TestContext structure you can use gotos for error handling, for > example: > > void *ptr1 = NULL, *ptr2 = NULL, *ptr3 = NULL; > > ptr1 = HeapAlloc(...); > if(!ptr1) goto err; > ptr2 = HeapAlloc(...); > if(!ptr2) goto err; > ptr3 = HeapAlloc(...); > if(!ptr3) goto err; > > return success; > > err: > HeapFree(ptr3); > HeapFree(ptr2); > HeapFree(ptr1); > return error; > > That avoids ever-growing if (FAILED(hr)) blocks with repeated code. Ok I've changed NewTestContext to use gotos instead. I've also changed it so that it returns an HRESULT instead of a pointer to the new structure. I've attached the new versions of the patches. diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..77178fa 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Michael Mc Donnell * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -3305,6 +3306,229 @@ static void D3DXGenerateAdjacencyTest(void) if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh); } +struct TestContext +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +}; + +/* + * Initializes a TestContext. Use this to initialize DirectX. + */ +HRESULT NewTestContext(struct TestContext **ppTestContext) +{ +HRESULT hr = D3D_OK; +HWND hwnd = NULL; +IDirect3D9 *d3d = NULL; +IDirect3DDevice9 *device = NULL; +D3DPRESENT_PARAMETERS d3dpp = {0}; +struct TestContext *pTestContext; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +hr = HRESULT_FROM_WIN32(GetLastError()); +goto error; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) goto error; + +memset(&d3dpp, 0, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) goto error; + +pT
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Hi, I'm at school right now, so I had only a very quick look. I'll take a closer look later in the evening. On Tuesday 24 May 2011 14:38:19 Michael Mc Donnell wrote: > This is my first try at implementing the UpdateSemantics mesh method. > It works fine here on my local machine, passes the new tests, and > works with my little interactive demo(not included). I still have a > few questions though. > > This implementation allocates new heap memory and I suspect that it > would be faster to re-use the already allocated memory. As far as I can see this happens only in the tests, where it doesn't really matter. Creating and destroying the device is more expensive than the heap allocation, so if you care about performance you should reuse your test context structure > Btw should I roll all or some of the patches up into a single patch? I think you can merge the 3 test patches into one > Any other comments about the patches in general? *) > + * Copyright (C) 2011 Google (Michael Mc Donnell) Afaik the gsoc conditions say that you keep the copyright to your code :-) *) In your first test you forgot to check the HeapAlloc result. *) You could write a test that passes an invalid D3DVERTEXELEMENT9 array to UpdateSemantics to see how it handles an invalid declaration. A few style suggestions: *) memset(mem, 0, size) is preferred over ZeroMemory(mem, size); *) In the wined3d code(and its client libs) Henri and I avoid structure typedefs. The existing d3dx9 code has a few of them. I'm ok with either way, but maybe you and the other d3dx9 devs want to go the wined3d way. *) In the TestContext structure you can use gotos for error handling, for example: void *ptr1 = NULL, *ptr2 = NULL, *ptr3 = NULL; ptr1 = HeapAlloc(...); if(!ptr1) goto err; ptr2 = HeapAlloc(...); if(!ptr2) goto err; ptr3 = HeapAlloc(...); if(!ptr3) goto err; return success; err: HeapFree(ptr3); HeapFree(ptr2); HeapFree(ptr1); return error; That avoids ever-growing if (FAILED(hr)) blocks with repeated code. signature.asc Description: This is a digitally signed message part.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Hi Stefan This is my first try at implementing the UpdateSemantics mesh method. It works fine here on my local machine, passes the new tests, and works with my little interactive demo(not included). I still have a few questions though. This implementation allocates new heap memory and I suspect that it would be faster to re-use the already allocated memory. That would, however, require quite a bit of re-engineering. Should I submit the current version or wait and try to do an optimized version? Btw should I roll all or some of the patches up into a single patch? Any other comments about the patches in general? Cheers, Michael Mc Donnell diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..976c758 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2,6 +2,7 @@ * Copyright 2008 David Adam * Copyright 2008 Luis Busquets * Copyright 2009 Henri Verbeet for CodeWeavers + * Copyright 2011 Google (Michael Mc Donnell) * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -3305,6 +3306,165 @@ static void D3DXGenerateAdjacencyTest(void) if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh); } +typedef struct +{ +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +} TestContext; + +/* + * Initializes a TestContext. Use this to initialize DirectX. + */ +static TestContext* NewTestContext(void) +{ +HRESULT hr; +TestContext* testContext; +HWND hwnd; +IDirect3D9 *d3d; +IDirect3DDevice9 *device; +D3DPRESENT_PARAMETERS d3dpp; + +hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); +if (!hwnd) +{ +skip("Couldn't create application window\n"); +return NULL; +} + +d3d = Direct3DCreate9(D3D_SDK_VERSION); +if (!d3d) +{ +skip("Couldn't create IDirect3D9 object\n"); +DestroyWindow(hwnd); +return NULL; +} + +ZeroMemory(&d3dpp, sizeof(d3dpp)); +d3dpp.Windowed = TRUE; +d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD; +hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device); +if (FAILED(hr)) +{ +skip("Failed to create IDirect3DDevice9 object %#x\n", hr); +IDirect3D9_Release(d3d); +DestroyWindow(hwnd); +return NULL; +} + +testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(TestContext)); +testContext->hwnd = hwnd; +testContext->d3d = d3d; +testContext->device = device; + +return testContext; +} + +static void FreeTestContext(TestContext *testContext) +{ +if (testContext == NULL) +return; + +if (testContext->device != NULL) +IDirect3DDevice9_Release(testContext->device); + +if (testContext->d3d != NULL) +IDirect3D9_Release(testContext->d3d); + +if (testContext->hwnd != NULL) +DestroyWindow(testContext->hwnd); + +HeapFree(GetProcessHeap(), 0, testContext); +} + +typedef struct +{ +float x0, y0, z0; /* Position 0 */ +float x1, y1, z1; /* Position 1 */ +float nx, ny, nz; /* Normal */ +DWORD color; +} VertexTwoPositions; + +static void UpdateSemanticsTest(void) +{ +HRESULT hr; +TestContext *tc = NewTestContext(); +LPD3DXMESH mesh; +D3DVERTEXELEMENT9 declaration0[] = +{ + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, + {0, 24, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0}, + {0, 36, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0}, + D3DDECL_END() +}; +/* x0, y0, z0, x1, y1, z1 ,nx, ny, nz, color */ +VertexTwoPositions vertices[] = +{ +{ 0.0f, 1.0f, 0.f,1.0f, 0.0f, 0.f,0.0f, 0.0f, 1.0f, 0x, }, +{ 1.0f, -1.0f, 0.f, -1.0f, -1.0f, 0.f,0.0f, 0.0f, 1.0f, 0xff00ff00, }, +{ -1.0f, -1.0f, 0.f, -1.0f, 1.0f, 0.f,0.0f, 0.0f, 1.0f, 0xffff, }, +}; +unsigned int faces[] = {0, 1, 2}; +unsigned int attributes[] = {0}; +unsigned int numFaces = 1; +unsigned int numVertices = 3; +DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM; +LPVOID pVertices; +LPVOID pFaces; +LPDWORD pAttributes; +D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE]; +D3DVERTEXELEMENT9 *pDecl; + +if (tc == NULL) +return; + +/* Create the test mesh */ +hr = D3DXCreateMesh(numFaces, numVertices, options, declaration0, tc->device, &mesh); +if (FAILED(hr)) +{ +skip("Couldn't create test mesh %#x\n", hr); +goto cleanup; +} + +mesh->lpVtbl->LockVertexBuffer(mesh, 0, &pVertices); +memcpy(pVertices, vertices, sizeof(vertices)); +mesh->lpVtbl->UnlockVertexBuffer(mesh); + +mesh->lpVtbl->LockIndexBuffer(mesh, 0, &pFaces); +memcpy(pFaces, faces, sizeof(faces)); +mesh->lpVtbl->Unlo
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
My plans have changed a bit to avoid duplication of work. The updated plan is: Before May 23: * Read up on DirectX in general Week 1-3 (May 23rd-June 10th): * Implement test for UpdateSemantics function * Implement UpdateSemantics function Week 4-6 (June 13th-July 1st): * Implement test for ConvertPointRepsToAdjacency function * Implement test for ConvertAdjacencyToPointReps function * Implement ConvertPointRepsToAdjacency function * Implement ConvertAdjacencyToPointReps function Week 7-9 (July 4th-22th): * Implement test for D3DXWeldVertices * Implement D3DXWeldVertices Week 10-12 (July 25th-August 12th): * Improve clone function if possible * Improve implemented functions as needed. Week 13 (Auguest 15th-19th): * Make sure that all code has been integrated in the official tree. * Last code clean-up for code that has not already been accepted.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, Apr 26, 2011 at 10:30 PM, Dylan Smith wrote: > On Tue, Apr 26, 2011 at 4:15 PM, Michael Mc Donnell > wrote: >> >> > Completely unimplemented: >> > - ConvertPointRepsToAdjacency >> > - ConvertAdjacencyToPointReps >> > - UpdateSemantics >> >> Ok so I can still work on ConvertPointRepsToAdjacency and >> ConvertAdjacencyToPointReps? >> > Yes. I had no plans on working on them. > Nor do I plan on fully implementing CloneMesh, so I'll try to submit what I > have in order to let you can build on it. Great I'll work on those first and then plan to improve CloneMesh later in the summer.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, Apr 26, 2011 at 4:15 PM, Michael Mc Donnell wrote: > > > Completely unimplemented: > > - ConvertPointRepsToAdjacency > > - ConvertAdjacencyToPointReps > > - UpdateSemantics > > Ok so I can still work on ConvertPointRepsToAdjacency and > ConvertAdjacencyToPointReps? > > Yes. I had no plans on working on them. Nor do I plan on fully implementing CloneMesh, so I'll try to submit what I have in order to let you can build on it.
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Tue, Apr 26, 2011 at 6:34 AM, Dylan Smith wrote: > On Sun, Mar 20, 2011 at 1:09 PM, Michael Mc Donnell > wrote: >> I would like implement some of the missing mesh functions in Wine's >> D3DX9 for Google Summer of Code 2011. I would like to implement the >> following functions: >> - CloneMesh >> - CloneMeshFVF >> - ConvertPointRepsToAdjacency >> - ConvertAdjacencyToPointReps >> - GenerateAdjacency > > Hi Michael, > > Sorry for not noticing your before, but it seems as if some work that > I have been doing as some overlap with your proposed Google Summer of > Code project. I have implemented GenerateAdjacency, and partially > implemented CloneMesh{,FVF} for the basic case where the vertex > formats are exactly the same, but I am still trying to incrementally > submit my work to wine-patches. That's ok Dylan, better now than in a couple of months :-). I only picked those functions because I had a good idea about how to implement them. I just got accepted into GSoC 2011, so I'll try to find some other mesh functions in D3DX9 to implement. > I'll try to avoid stepping on your toes in future. Don't worry about it. No offence taken :-) > Completely unimplemented: > - ConvertPointRepsToAdjacency > - ConvertAdjacencyToPointReps > - UpdateSemantics Ok so I can still work on ConvertPointRepsToAdjacency and ConvertAdjacencyToPointReps? > There are also plenty of other functions. Just try `grep stub > d3dx9_36.spec | grep Mesh` for Mesh related functions. There are also > some unimplemented functions to create meshes for certain shapes (e.g. > D3DXCreateTorus, D3DXCreatePolygon). Thanks for the suggestions. It seems like Misha Koshelev is working on D3DXCreateTorus, and David Adam is working on D3DXCreatePolygon? I'll have a look around and see if there are any other good functions. Any other suggestions from anyone are of course welcome. Cheers, Michael
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
On Sun, Mar 20, 2011 at 1:09 PM, Michael Mc Donnell wrote: > I would like implement some of the missing mesh functions in Wine's > D3DX9 for Google Summer of Code 2011. I would like to implement the > following functions: > - CloneMesh > - CloneMeshFVF > - ConvertPointRepsToAdjacency > - ConvertAdjacencyToPointReps > - GenerateAdjacency Hi Michael, Sorry for not noticing your before, but it seems as if some work that I have been doing as some overlap with your proposed Google Summer of Code project. I have implemented GenerateAdjacency, and partially implemented CloneMesh{,FVF} for the basic case where the vertex formats are exactly the same, but I am still trying to incrementally submit my work to wine-patches. I have actually been working on the D3DXLoadMeshFromX and D3DXLoadMeshHierarchyFromX functions. However, D3DXLoadMeshHierarchyFromX is given a set of callbacks that is called with the adjacency for the mesh (ID3DXAllocateMeshHierarchy::CreateMeshContainer). It is for this reason that I implemented GenerateAdjacency, since it wasn't possible to detect whether the adjacency information would be used by the user provided callback in order to create a partial implementation. I also provided a partial implementation of Optimize{,Inplace} since it seemed like the load mesh functions were sorting the faces by the attribute values (i.e. D3DXMESHOPT_ATTRSORT). I had Optimize use CloneMesh with the same vertex format in order to call OptimizeInplace, which does the actual optimization work. The relevant patches are in my development repository (http://dylansmith.ca/git) in the loadmesh branch. I am trying to send in my patches, but currently I am waiting on the review of my latest patch to generate rmxftmpl.h, which is a re-requisite for load mesh patches. I'll try to avoid stepping on your toes in future. Summary of ID3DXMesh methods I have implemented: - LockAttributeBuffer - UnlockAttributeBuffer - GetAttributeTable - SetAttributeTable - DrawSubset - GenerateAdjacency Partially implemented: OptimizeInplace: unimplemented support for D3DXMESHOPT_VERTEXCACHE, D3DXMESHOPT_STRIPREORDER CloneMesh: unimplemented support for vertex buffer conversion Completely unimplemented: - ConvertPointRepsToAdjacency - ConvertAdjacencyToPointReps - UpdateSemantics There are also plenty of other functions. Just try `grep stub d3dx9_36.spec | grep Mesh` for Mesh related functions. There are also some unimplemented functions to create meshes for certain shapes (e.g. D3DXCreateTorus, D3DXCreatePolygon).
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Thanks for the reply Stefan! It sounds like it is possible to do this project. I'll send my application to Google on the 28th. On Sun, Mar 20, 2011 at 11:32 PM, Stefan Dösinger wrote: > Hi, > Thanks for your proposal! > > I don't know the functions you suggest well enough personally to judge the > amount of work that is needed. Potential mentors are Roderick, Matteo, Henri > and me. I don't know who has time and is willing to mentor, but I think it is > safe to say that we'll find somebody :-) > > We had two d3dx9-centered gsoc projects in the past, namely Matteo's shader > assembler work and Tony Wasserka's texture loading work. Matteo's project > succeeded and the code is in Wine now. Tony's code didn't make it in during > the project, but at least parts(everything?) of it got committed later. > > As you can see we have another developer who's interested in d3dx9. Two > projects on the same library are doable if they target independent parts. > Luckily d3dx9 is a big collection of mostly independent helper functions. > > I don't want to jump to conclusions just yet, the formal application process > will decide which proposals are accepted. > > Best regards > Stefan > > On Sunday 20 March 2011 18:09:24 Michael Mc Donnell wrote: >> Dear Wine developers, >> >> I would like implement some of the missing mesh functions in Wine's >> D3DX9 for Google Summer of Code 2011. I would like to implement the >> following functions: >> - CloneMesh >> - CloneMeshFVF >> - ConvertPointRepsToAdjacency >> - ConvertAdjacencyToPointReps >> - GenerateAdjacency >> >> They seem to be suitable in size to implement during a summer, and I >> have a good idea of how to implement them. I expect to first implement >> tests and then implement the functions. >> >> I am a masters student at the Technical University of Denmark nearing >> the end of my studies, and I have specialized in 3D computer graphics, >> studying both real-time and physically based techniques, as well as >> how to use existing 3D modeling tools to create animations. The mesh >> functions should be fairly straight forward for me to implement as I >> have recently taken an advanced course on geometry processing which >> involved a lot of mesh manipulation. I have, furthermore, already >> tests (for a shell32 function) in the wine test suite so I have some >> knowledge of Wine development. >> >> Roderick Coldenbrander is listed as a possible mentor for "Direct3D - >> Implement missing D3D9_xx DLLs" on the wiki. Are you still interested, >> or would someone else like to be a mentor? >> >> Regards, >> Michael Mc Donnell >
Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Hi, Thanks for your proposal! I don't know the functions you suggest well enough personally to judge the amount of work that is needed. Potential mentors are Roderick, Matteo, Henri and me. I don't know who has time and is willing to mentor, but I think it is safe to say that we'll find somebody :-) We had two d3dx9-centered gsoc projects in the past, namely Matteo's shader assembler work and Tony Wasserka's texture loading work. Matteo's project succeeded and the code is in Wine now. Tony's code didn't make it in during the project, but at least parts(everything?) of it got committed later. As you can see we have another developer who's interested in d3dx9. Two projects on the same library are doable if they target independent parts. Luckily d3dx9 is a big collection of mostly independent helper functions. I don't want to jump to conclusions just yet, the formal application process will decide which proposals are accepted. Best regards Stefan On Sunday 20 March 2011 18:09:24 Michael Mc Donnell wrote: > Dear Wine developers, > > I would like implement some of the missing mesh functions in Wine's > D3DX9 for Google Summer of Code 2011. I would like to implement the > following functions: > - CloneMesh > - CloneMeshFVF > - ConvertPointRepsToAdjacency > - ConvertAdjacencyToPointReps > - GenerateAdjacency > > They seem to be suitable in size to implement during a summer, and I > have a good idea of how to implement them. I expect to first implement > tests and then implement the functions. > > I am a masters student at the Technical University of Denmark nearing > the end of my studies, and I have specialized in 3D computer graphics, > studying both real-time and physically based techniques, as well as > how to use existing 3D modeling tools to create animations. The mesh > functions should be fairly straight forward for me to implement as I > have recently taken an advanced course on geometry processing which > involved a lot of mesh manipulation. I have, furthermore, already > tests (for a shell32 function) in the wine test suite so I have some > knowledge of Wine development. > > Roderick Coldenbrander is listed as a possible mentor for "Direct3D - > Implement missing D3D9_xx DLLs" on the wiki. Are you still interested, > or would someone else like to be a mentor? > > Regards, > Michael Mc Donnell signature.asc Description: This is a digitally signed message part.
GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
Dear Wine developers, I would like implement some of the missing mesh functions in Wine's D3DX9 for Google Summer of Code 2011. I would like to implement the following functions: - CloneMesh - CloneMeshFVF - ConvertPointRepsToAdjacency - ConvertAdjacencyToPointReps - GenerateAdjacency They seem to be suitable in size to implement during a summer, and I have a good idea of how to implement them. I expect to first implement tests and then implement the functions. I am a masters student at the Technical University of Denmark nearing the end of my studies, and I have specialized in 3D computer graphics, studying both real-time and physically based techniques, as well as how to use existing 3D modeling tools to create animations. The mesh functions should be fairly straight forward for me to implement as I have recently taken an advanced course on geometry processing which involved a lot of mesh manipulation. I have, furthermore, already tests (for a shell32 function) in the wine test suite so I have some knowledge of Wine development. Roderick Coldenbrander is listed as a possible mentor for "Direct3D - Implement missing D3D9_xx DLLs" on the wiki. Are you still interested, or would someone else like to be a mentor? Regards, Michael Mc Donnell