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

[Stefan Dösinger]

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).
-------------- 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/20110614/b40c649f/attachment.pgp>