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

[Stefan Dösinger]

Hi,

A few more thinks I noticed:

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.

> +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
> 
> <michael at mcdonnell.dk> wrote:
> > On Thu, Aug 4, 2011 at 8:48 AM, Michael Mc Donnell <michael at mcdonnell.dk> 
wrote:
> >> On Wed, Aug 3, 2011 at 2:39 PM, Michael Mc Donnell <michael at mcdonnell.dk> 
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110805/0e4d4fc6/attachment.pgp>