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

[Michael Mc Donnell]

On Mon, Jul 25, 2011 at 4:04 PM, Stefan Dösinger <stefandoesinger at gmx.at> wrote:
> A few more comments, with one day delay:

Thanks that was quick!

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

Ok. I've changed all occurrences of fmax to fmaxf and fabs to fabsf.

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

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

Btw. looking at the code again I found a small bug introduced during a
cleanup. The default in the switch case in get_component_epsilon is to
write an ERR. It should also catch and ignore some of the usages that
it is not possible to specify an epsilon for. I've put that back.

> In the test:
>> +    int vertex_size_normal = sizeof(struct vertex_normal);
>> +    int vertex_size_blendweight = sizeof(struct vertex_blendweight);
>> ...
> You could make those unsigned.

Check.

> I'm not quite done reading the test yet, I'll send another mail if I find more.

Thanks! I appreciate the feedback.

>
> 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
>> <stefandoesinger at gmx.at> 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.

Ok. I've removed the comment.

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

I'll keep it as it is for now and have a look into generalizing it to UBYTE4N.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-Implemented-D3DXWeldVertices-and-test.patch
Type: text/x-patch
Size: 56518 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110726/3d67d131/attachment-0001.bin>