Hi Stefan sorry for the duplicate email. I forgot to also send it to wine-devel.
On Fri, Jun 17, 2011 at 12:02 AM, Stefan Dösinger
<stefandoesin...@gmx.at> wrote:
> 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.
Great!
>> 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.
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.
From b83b22b8d7d338490b33af6129764b1846f36ba2 Mon Sep 17 00:00:00 2001
From: Michael Mc Donnell <mich...@mcdonnell.dk>
Date: Tue, 24 May 2011 19:44:48 +0200
Subject: d3dx9: Implement UpdateSemantics mesh method.
Added a chached vertex declaration and a declaration size field.
Changed copy method to not write past D3DECL_END() marker.
Handle non-zero stream values.
Added more WARN messages.
Re-worded WARN message.
---
dlls/d3dx9_36/mesh.c | 95 +++++++++++++++++++++++++++++++++++++------
dlls/d3dx9_36/tests/mesh.c | 46 +++++++++++-----------
2 files changed, 104 insertions(+), 37 deletions(-)
diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c
index cfadb75..0455ea7 100644
--- a/dlls/d3dx9_36/mesh.c
+++ b/dlls/d3dx9_36/mesh.c
@@ -6,6 +6,7 @@
* Copyright (C) 2009 David Adam
* Copyright (C) 2010 Tony Wasserka
* Copyright (C) 2011 Dylan Smith
+ * Copyright (C) 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
@@ -52,7 +53,10 @@ typedef struct ID3DXMeshImpl
DWORD options;
DWORD fvf;
IDirect3DDevice9 *device;
+ D3DVERTEXELEMENT9 cached_declaration[MAX_FVF_DECL_SIZE];
IDirect3DVertexDeclaration9 *vertex_declaration;
+ UINT vertex_declaration_size;
+ UINT num_elem;
IDirect3DVertexBuffer9 *vertex_buffer;
IDirect3DIndexBuffer9 *index_buffer;
DWORD *attrib_buffer;
@@ -106,7 +110,8 @@ static ULONG WINAPI ID3DXMeshImpl_Release(ID3DXMesh *iface)
{
IDirect3DIndexBuffer9_Release(This->index_buffer);
IDirect3DVertexBuffer9_Release(This->vertex_buffer);
- IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
+ if (This->vertex_declaration)
+ IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
IDirect3DDevice9_Release(This->device);
HeapFree(GetProcessHeap(), 0, This->attrib_buffer);
HeapFree(GetProcessHeap(), 0, This->attrib_table);
@@ -127,6 +132,12 @@ static HRESULT WINAPI ID3DXMeshImpl_DrawSubset(ID3DXMesh *iface, DWORD attrib_id
TRACE("(%p)->(%u)\n", This, attrib_id);
+ if (!This->vertex_declaration)
+ {
+ WARN("Can't draw a mesh with an invalid vertex declaration.\n");
+ return E_FAIL;
+ }
+
vertex_size = iface->lpVtbl->GetNumBytesPerVertex(iface);
hr = IDirect3DDevice9_SetVertexDeclaration(This->device, This->vertex_declaration);
@@ -186,32 +197,31 @@ static DWORD WINAPI ID3DXMeshImpl_GetFVF(ID3DXMesh *iface)
return This->fvf;
}
+static void copy_declaration(D3DVERTEXELEMENT9 *dst, const D3DVERTEXELEMENT9 *src, UINT num_elem)
+{
+ memcpy(dst, src, num_elem * sizeof(*src));
+}
+
static HRESULT WINAPI ID3DXMeshImpl_GetDeclaration(ID3DXMesh *iface, D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE])
{
ID3DXMeshImpl *This = impl_from_ID3DXMesh(iface);
- UINT numelements;
TRACE("(%p)\n", This);
if (declaration == NULL) return D3DERR_INVALIDCALL;
- return IDirect3DVertexDeclaration9_GetDeclaration(This->vertex_declaration,
- declaration,
- &numelements);
+ copy_declaration(declaration, This->cached_declaration, This->num_elem);
+
+ return D3D_OK;
}
static DWORD WINAPI ID3DXMeshImpl_GetNumBytesPerVertex(ID3DXMesh *iface)
{
ID3DXMeshImpl *This = impl_from_ID3DXMesh(iface);
- UINT numelements;
- D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE] = { D3DDECL_END() };
TRACE("iface (%p)\n", This);
- IDirect3DVertexDeclaration9_GetDeclaration(This->vertex_declaration,
- declaration,
- &numelements);
- return D3DXGetDeclVertexSize(declaration, 0);
+ return This->vertex_declaration_size;
}
static DWORD WINAPI ID3DXMeshImpl_GetOptions(ID3DXMesh *iface)
@@ -603,11 +613,61 @@ cleanup:
static HRESULT WINAPI ID3DXMeshImpl_UpdateSemantics(ID3DXMesh *iface, D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE])
{
+ HRESULT hr;
ID3DXMeshImpl *This = impl_from_ID3DXMesh(iface);
+ UINT vertex_declaration_size;
+ int i;
- FIXME("(%p)->(%p): stub\n", This, declaration);
+ TRACE("(%p)->(%p)\n", This, declaration);
- return E_NOTIMPL;
+ if (!declaration)
+ {
+ WARN("Invalid declaration. Can't use NULL declaration.\n");
+ return D3DERR_INVALIDCALL;
+ }
+
+ /* New declaration must be same size as original */
+ vertex_declaration_size = D3DXGetDeclVertexSize(declaration, declaration[0].Stream);
+ if (vertex_declaration_size != This->vertex_declaration_size)
+ {
+ WARN("Invalid declaration. New vertex size does not match the orginal vertex size.\n");
+ return D3DERR_INVALIDCALL;
+ }
+
+ /* New declaration must not contain non-zero Stream value */
+ for (i = 0; declaration[i].Stream != 0xff; i++)
+ {
+ if (declaration[i].Stream != 0)
+ {
+ WARN("Invalid declaration. New declaration contains non-zero Stream value.\n");
+ return D3DERR_INVALIDCALL;
+ }
+ }
+
+ This->num_elem = i + 1;
+ copy_declaration(This->cached_declaration, declaration, This->num_elem);
+
+ if (This->vertex_declaration)
+ IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
+
+ /* An application can pass an invalid declaration to UpdateSemantics and
+ * still expect D3D_OK (see tests). If the declaration is invalid, then
+ * subsequent calls to DrawSubset will fail. This is handled by setting the
+ * vertex declaration to NULL.
+ * GetDeclaration, GetNumBytesPerVertex must, however, use the new
+ * invalid declaration. This is handled by them using the cached vertex
+ * declaration instead of the actual vertex declaration.
+ */
+ hr = IDirect3DDevice9_CreateVertexDeclaration(This->device,
+ declaration,
+ &This->vertex_declaration);
+ if (FAILED(hr))
+ {
+ WARN("Using invalid declaration. Calls to DrawSubset will fail.\n");
+ This->vertex_declaration = NULL;
+ }
+
+ return D3D_OK;
}
/*** ID3DXMesh ***/
@@ -1652,6 +1712,8 @@ HRESULT WINAPI D3DXCreateMesh(DWORD numfaces, DWORD numvertices, DWORD options,
HRESULT hr;
DWORD fvf;
IDirect3DVertexDeclaration9 *vertex_declaration;
+ UINT vertex_declaration_size;
+ UINT num_elem;
IDirect3DVertexBuffer9 *vertex_buffer;
IDirect3DIndexBuffer9 *index_buffer;
DWORD *attrib_buffer;
@@ -1674,6 +1736,7 @@ HRESULT WINAPI D3DXCreateMesh(DWORD numfaces, DWORD numvertices, DWORD options,
for (i = 0; declaration[i].Stream != 0xff; i++)
if (declaration[i].Stream != 0)
return D3DERR_INVALIDCALL;
+ num_elem = i + 1;
if (options & D3DXMESH_32BIT)
index_format = D3DFMT_INDEX32;
@@ -1734,10 +1797,11 @@ HRESULT WINAPI D3DXCreateMesh(DWORD numfaces, DWORD numvertices, DWORD options,
WARN("Unexpected return value %x from IDirect3DDevice9_CreateVertexDeclaration.\n",hr);
return hr;
}
+ vertex_declaration_size = D3DXGetDeclVertexSize(declaration, declaration[0].Stream);
/* Create vertex buffer */
hr = IDirect3DDevice9_CreateVertexBuffer(device,
- numvertices * D3DXGetDeclVertexSize(declaration, declaration[0].Stream),
+ numvertices * vertex_declaration_size,
vertex_usage,
fvf,
vertex_pool,
@@ -1787,7 +1851,10 @@ HRESULT WINAPI D3DXCreateMesh(DWORD numfaces, DWORD numvertices, DWORD options,
object->device = device;
IDirect3DDevice9_AddRef(device);
+ copy_declaration(object->cached_declaration, declaration, num_elem);
object->vertex_declaration = vertex_declaration;
+ object->vertex_declaration_size = vertex_declaration_size;
+ object->num_elem = num_elem;
object->vertex_buffer = vertex_buffer;
object->index_buffer = index_buffer;
object->attrib_buffer = attrib_buffer;
diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c
index f4b1bbf..2fe9692 100644
--- a/dlls/d3dx9_36/tests/mesh.c
+++ b/dlls/d3dx9_36/tests/mesh.c
@@ -4490,7 +4490,7 @@ static void test_update_semantics(void)
}
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics, got %#x expected %#x\n", hr, D3D_OK);
+ ok(hr == D3D_OK, "Test UpdateSematics, got %#x expected %#x\n", hr, D3D_OK);
/* Check that declaration was written by getting it again */
memset(declaration, 0, sizeof(declaration));
@@ -4505,8 +4505,8 @@ static void test_update_semantics(void)
{
if (decl_ptr->Usage == D3DDECLUSAGE_POSITION)
{
- todo_wine ok(decl_ptr->Offset == offset, "Test UpdateSematics, got offset %d expected %d\n",
- decl_ptr->Offset, offset);
+ ok(decl_ptr->Offset == offset, "Test UpdateSematics, got offset %d expected %d\n",
+ decl_ptr->Offset, offset);
}
}
@@ -4516,7 +4516,7 @@ static void test_update_semantics(void)
memset(declaration, filler_a, sizeof(declaration));
memcpy(declaration, declaration0, sizeof(declaration0));
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics, "
+ ok(hr == D3D_OK, "Test UpdateSematics, "
"got %#x expected D3D_OK\n", hr);
memset(declaration, filler_b, sizeof(declaration));
hr = mesh->lpVtbl->GetDeclaration(mesh, declaration);
@@ -4549,13 +4549,13 @@ static void test_update_semantics(void)
}
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics for overlapping fields, "
- "got %#x expected D3D_OK\n", hr);
+ ok(hr == D3D_OK, "Test UpdateSematics for overlapping fields, "
+ "got %#x expected D3D_OK\n", hr);
/* Set the position type to color instead of float3 */
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_pos_type_color);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics position type color, "
- "got %#x expected D3D_OK\n", hr);
+ ok(hr == D3D_OK, "Test UpdateSematics position type color, "
+ "got %#x expected D3D_OK\n", hr);
/* The following test cases show that NULL, smaller or larger declarations,
* and declarations with non-zero Stream values are not accepted.
@@ -4567,8 +4567,8 @@ static void test_update_semantics(void)
/* Null declaration (invalid declaration) */
mesh->lpVtbl->UpdateSemantics(mesh, declaration0); /* Set a valid declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, NULL);
- todo_wine ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics null pointer declaration, "
- "got %#x expected D3DERR_INVALIDCALL\n", hr);
+ ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics null pointer declaration, "
+ "got %#x expected D3DERR_INVALIDCALL\n", hr);
vertex_size = mesh->lpVtbl->GetNumBytesPerVertex(mesh);
ok(vertex_size == exp_vertex_size, "Got vertex declaration size %u, expected %u\n",
vertex_size, exp_vertex_size);
@@ -4581,8 +4581,8 @@ static void test_update_semantics(void)
/* Smaller vertex declaration (invalid declaration) */
mesh->lpVtbl->UpdateSemantics(mesh, declaration0); /* Set a valid declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_smaller);
- todo_wine ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics for smaller vertex declaration, "
- "got %#x expected D3DERR_INVALIDCALL\n", hr);
+ ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics for smaller vertex declaration, "
+ "got %#x expected D3DERR_INVALIDCALL\n", hr);
vertex_size = mesh->lpVtbl->GetNumBytesPerVertex(mesh);
ok(vertex_size == exp_vertex_size, "Got vertex declaration size %u, expected %u\n",
vertex_size, exp_vertex_size);
@@ -4595,8 +4595,8 @@ static void test_update_semantics(void)
/* Larger vertex declaration (invalid declaration) */
mesh->lpVtbl->UpdateSemantics(mesh, declaration0); /* Set a valid declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_larger);
- todo_wine ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics for larger vertex declaration, "
- "got %#x expected D3DERR_INVALIDCALL\n", hr);
+ ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics for larger vertex declaration, "
+ "got %#x expected D3DERR_INVALIDCALL\n", hr);
vertex_size = mesh->lpVtbl->GetNumBytesPerVertex(mesh);
ok(vertex_size == exp_vertex_size, "Got vertex declaration size %u, expected %u\n",
vertex_size, exp_vertex_size);
@@ -4609,7 +4609,7 @@ static void test_update_semantics(void)
/* Use multiple streams and keep the same vertex size (invalid declaration) */
mesh->lpVtbl->UpdateSemantics(mesh, declaration0); /* Set a valid declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_multiple_streams);
- todo_wine ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics using multiple streams, "
+ ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics using multiple streams, "
"got %#x expected D3DERR_INVALIDCALL\n", hr);
vertex_size = mesh->lpVtbl->GetNumBytesPerVertex(mesh);
ok(vertex_size == exp_vertex_size, "Got vertex declaration size %u, expected %u\n",
@@ -4629,8 +4629,8 @@ static void test_update_semantics(void)
/* Double usage (invalid declaration) */
mesh->lpVtbl->UpdateSemantics(mesh, declaration0); /* Set a valid declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_double_usage);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics double usage, "
- "got %#x expected D3D_OK\n", hr);
+ ok(hr == D3D_OK, "Test UpdateSematics double usage, "
+ "got %#x expected D3D_OK\n", hr);
vertex_size = mesh->lpVtbl->GetNumBytesPerVertex(mesh);
ok(vertex_size == exp_vertex_size, "Got vertex declaration size %u, expected %u\n",
vertex_size, exp_vertex_size);
@@ -4638,13 +4638,13 @@ static void test_update_semantics(void)
hr = mesh->lpVtbl->GetDeclaration(mesh, declaration);
ok(hr == D3D_OK, "Couldn't get vertex declaration. Got %#x, expected D3D_OK\n", hr);
equal = memcmp(declaration, declaration_double_usage, sizeof(declaration_double_usage));
- todo_wine ok(equal == 0, "Vertex declarations were not equal\n");
+ ok(equal == 0, "Vertex declarations were not equal\n");
/* Set the position to an undefined type (invalid declaration) */
mesh->lpVtbl->UpdateSemantics(mesh, declaration0); /* Set a valid declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_undefined_type);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics undefined type, "
- "got %#x expected D3D_OK\n", hr);
+ ok(hr == D3D_OK, "Test UpdateSematics undefined type, "
+ "got %#x expected D3D_OK\n", hr);
vertex_size = mesh->lpVtbl->GetNumBytesPerVertex(mesh);
ok(vertex_size == exp_vertex_size, "Got vertex declaration size %u, expected %u\n",
vertex_size, exp_vertex_size);
@@ -4652,12 +4652,12 @@ static void test_update_semantics(void)
hr = mesh->lpVtbl->GetDeclaration(mesh, declaration);
ok(hr == D3D_OK, "Couldn't get vertex declaration. Got %#x, expected D3D_OK\n", hr);
equal = memcmp(declaration, declaration_undefined_type, sizeof(declaration_undefined_type));
- todo_wine ok(equal == 0, "Vertex declarations were not equal\n");
+ ok(equal == 0, "Vertex declarations were not equal\n");
/* Use a not 4 byte aligned offset (invalid declaration) */
mesh->lpVtbl->UpdateSemantics(mesh, declaration0); /* Set a valid declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_not_4_byte_aligned_offset);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics not 4 byte aligned offset, "
+ ok(hr == D3D_OK, "Test UpdateSematics not 4 byte aligned offset, "
"got %#x expected D3D_OK\n", hr);
vertex_size = mesh->lpVtbl->GetNumBytesPerVertex(mesh);
ok(vertex_size == exp_vertex_size, "Got vertex declaration size %u, expected %u\n",
@@ -4667,7 +4667,7 @@ static void test_update_semantics(void)
ok(hr == D3D_OK, "Couldn't get vertex declaration. Got %#x, expected D3D_OK\n", hr);
equal = memcmp(declaration, declaration_not_4_byte_aligned_offset,
sizeof(declaration_not_4_byte_aligned_offset));
- todo_wine ok(equal == 0, "Vertex declarations were not equal\n");
+ ok(equal == 0, "Vertex declarations were not equal\n");
cleanup:
if (mesh)
--
1.7.5.4
