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