Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

[Michael Mc Donnell]

On Fri, Aug 5, 2011 at 7:01 PM, Stefan Dösinger <stefandoesinger at gmx.at> wrote:
> On Thursday 04 August 2011 11:21:22 Michael Mc Donnell wrote:
>> +#include <float.h>
> 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.

Ok, I've added this guard:

#ifdef HAVE_FLOAT_H
# include <float.h>
#endif

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

I've changed it to an assignment as you suggested.

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

I've changed them to use sizeof(D3DXVECTORn) together with the memcpy.

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

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?

[1] http://msdn.microsoft.com/en-us/library/aa505945.aspx

Btw. I just noticed I take the abs of an unsigned value which doesn't
make sense. I've changed it to this instead:

BYTE diff_x = b1[0] > b2[0] ? b1[0] - b2[0] : b2[0] - b1[0];
...

I've also fixed it for the other unsigned types.

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

I have no idea how to test this, so I'll leave the FIXME for
D3DDECLTYPE_UNUSED. I've also added a FIXME 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.

Yeah that was a bad wording. It's not possible to specify an epsilon
for those usages. I don't know whether they can be welded or not, so
I've changed them to FIXMEs instead. I should probably add a test at
some point.

Btw. those FIXMEs might produce a lot of messages (one per vertex). Is
there good way to limit the number of messages?

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

Test 10 uses FLOAT1. I've changed the comment to better explain that.
I've also changed the comparison so that it uses the pointers just
like the comparisons below it.

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

Yeah I don't see any good way either. Usually when I do unit testing
for other things I use one function per test, and often one file per
method, but that seems not to be the way it's done in Wine.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-Implemented-D3DXWeldVertices-and-test.patch
Type: text/x-patch
Size: 109150 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110806/6b6edd8f/attachment-0001.bin>