Thanks for the lengthy feedback!
On Tue, May 24, 2011 at 3:36 PM, Stefan Dösinger <stefandoesin...@gmx.at> wrote:
> Hi,
> I'm at school right now, so I had only a very quick look. I'll take a closer
> look later in the evening.
>
> On Tuesday 24 May 2011 14:38:19 Michael Mc Donnell wrote:
>> This is my first try at implementing the UpdateSemantics mesh method.
>> It works fine here on my local machine, passes the new tests, and
>> works with my little interactive demo(not included). I still have a
>> few questions though.
>>
>> This implementation allocates new heap memory and I suspect that it
>> would be faster to re-use the already allocated memory.
> As far as I can see this happens only in the tests, where it doesn't really
> matter. Creating and destroying the device is more expensive than the heap
> allocation, so if you care about performance you should reuse your test
> context structure
Sorry I phrased that in a wrong way. In the UpdateSemantics function I
call IDirect3DDevice9_CreateVertexDeclaration which allocates a new
Vertex Declaration on the heap. I think it might slow things down if
UpdateSemantics is in a tight loop where it is used for animation.
I also noticed that I didn't call
IDirect3DDevice9_SetVertexDeclaration. I've added it to stick with the
defined locking scheme.
>> Btw should I roll all or some of the patches up into a single patch?
> I think you can merge the 3 test patches into one
Ok, I've rolled the three test patches into one.
>> Any other comments about the patches in general?
>
> *)
>> + * Copyright (C) 2011 Google (Michael Mc Donnell)
> Afaik the gsoc conditions say that you keep the copyright to your code :-)
Yes you are right. First paragraph in the faq about code :-)
> *) In your first test you forgot to check the HeapAlloc result.
Ok, I'll return E_OUTOFMEMORY in that case.
> *) You could write a test that passes an invalid D3DVERTEXELEMENT9 array to
> UpdateSemantics to see how it handles an invalid declaration.
I've already got one test with an invalid D3DVERTEXELEMENT9 array. It
happily accepts too big declarations even though msdn says "The call
is valid only if the old and new declaration formats have the same
vertex size". I've added test for null pointers, which it didn't
handle correctly. Should I try to make some more invalid
D3DVERTEXELEMENT9 arrays to see if I can provoke an error?
> A few style suggestions:
>
> *) memset(mem, 0, size) is preferred over ZeroMemory(mem, size);
Ok
> *) In the wined3d code(and its client libs) Henri and I avoid structure
> typedefs. The existing d3dx9 code has a few of them. I'm ok with either way,
> but maybe you and the other d3dx9 devs want to go the wined3d way.
Sure I can change them. So just normal structs? Is it to keep the
namespace clean?
> *) In the TestContext structure you can use gotos for error handling, for
> example:
>
> void *ptr1 = NULL, *ptr2 = NULL, *ptr3 = NULL;
>
> ptr1 = HeapAlloc(...);
> if(!ptr1) goto err;
> ptr2 = HeapAlloc(...);
> if(!ptr2) goto err;
> ptr3 = HeapAlloc(...);
> if(!ptr3) goto err;
>
> return success;
>
> err:
> HeapFree(ptr3);
> HeapFree(ptr2);
> HeapFree(ptr1);
> return error;
>
> That avoids ever-growing if (FAILED(hr)) blocks with repeated code.
Ok I've changed NewTestContext to use gotos instead. I've also changed
it so that it returns an HRESULT instead of a pointer to the new
structure.
I've attached the new versions of the patches.
diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c
index cedef32..77178fa 100644
--- a/dlls/d3dx9_36/tests/mesh.c
+++ b/dlls/d3dx9_36/tests/mesh.c
@@ -2,6 +2,7 @@
* Copyright 2008 David Adam
* Copyright 2008 Luis Busquets
* Copyright 2009 Henri Verbeet for CodeWeavers
+ * Copyright 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
@@ -3305,6 +3306,229 @@ static void D3DXGenerateAdjacencyTest(void)
if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh);
}
+struct TestContext
+{
+ HWND hwnd;
+ IDirect3D9 *d3d;
+ IDirect3DDevice9 *device;
+};
+
+/*
+ * Initializes a TestContext. Use this to initialize DirectX.
+ */
+HRESULT NewTestContext(struct TestContext **ppTestContext)
+{
+ HRESULT hr = D3D_OK;
+ HWND hwnd = NULL;
+ IDirect3D9 *d3d = NULL;
+ IDirect3DDevice9 *device = NULL;
+ D3DPRESENT_PARAMETERS d3dpp = {0};
+ struct TestContext *pTestContext;
+
+ hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL);
+ if (!hwnd)
+ {
+ hr = HRESULT_FROM_WIN32(GetLastError());
+ goto error;
+ }
+
+ d3d = Direct3DCreate9(D3D_SDK_VERSION);
+ if (!d3d) goto error;
+
+ memset(&d3dpp, 0, sizeof(d3dpp));
+ d3dpp.Windowed = TRUE;
+ d3dpp.SwapEffect = D3DSWAPEFFECT_DISCARD;
+ hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, hwnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp, &device);
+ if (FAILED(hr)) goto error;
+
+ pTestContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
+ if (pTestContext == NULL)
+ {
+ hr = E_OUTOFMEMORY;
+ goto error;
+ }
+ pTestContext->hwnd = hwnd;
+ pTestContext->d3d = d3d;
+ pTestContext->device = device;
+ (*ppTestContext) = pTestContext;
+
+ if (hr == D3D_OK)
+ return D3D_OK;
+error:
+ if (device != NULL)
+ IDirect3DDevice9_Release(device);
+
+ if (d3d != NULL)
+ IDirect3D9_Release(d3d);
+
+ if (hwnd != NULL)
+ DestroyWindow(hwnd);
+
+ return hr;
+}
+
+static void FreeTestContext(struct TestContext *testContext)
+{
+ if (testContext == NULL)
+ return;
+
+ if (testContext->device != NULL)
+ IDirect3DDevice9_Release(testContext->device);
+
+ if (testContext->d3d != NULL)
+ IDirect3D9_Release(testContext->d3d);
+
+ if (testContext->hwnd != NULL)
+ DestroyWindow(testContext->hwnd);
+
+ HeapFree(GetProcessHeap(), 0, testContext);
+}
+
+struct VertexTwoPositions
+{
+ float x0, y0, z0; /* Position 0 */
+ float x1, y1, z1; /* Position 1 */
+ float nx, ny, nz; /* Normal */
+ DWORD color;
+};
+
+static void UpdateSemanticsTest(void)
+{
+ HRESULT hr;
+ struct TestContext *tc = NULL;
+ LPD3DXMESH mesh;
+ D3DVERTEXELEMENT9 declaration0[] =
+ {
+ {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0},
+ {0, 24, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_NORMAL, 0},
+ {0, 36, D3DDECLTYPE_D3DCOLOR, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_COLOR, 0},
+ D3DDECL_END()
+ };
+ /* x0, y0, z0, x1, y1, z1 ,nx, ny, nz, color */
+ struct VertexTwoPositions vertices[] =
+ {
+ { 0.0f, 1.0f, 0.f, 1.0f, 0.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xffff0000, },
+ { 1.0f, -1.0f, 0.f, -1.0f, -1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xff00ff00, },
+ { -1.0f, -1.0f, 0.f, -1.0f, 1.0f, 0.f, 0.0f, 0.0f, 1.0f, 0xff0000ff, },
+ };
+ unsigned int faces[] = {0, 1, 2};
+ unsigned int attributes[] = {0};
+ unsigned int numFaces = 1;
+ unsigned int numVertices = 3;
+ int offset = 12;
+ DWORD options = D3DXMESH_32BIT | D3DXMESH_SYSTEMMEM;
+ LPVOID pVertices;
+ LPVOID pFaces;
+ LPDWORD pAttributes;
+ D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE];
+ D3DVERTEXELEMENT9 *pDecl;
+
+ hr = NewTestContext(&tc);
+ if (FAILED(hr))
+ {
+ skip("Couldn't create a test context %#x\n", hr);
+ goto cleanup;
+ }
+
+ /* Create the test mesh */
+ hr = D3DXCreateMesh(numFaces, numVertices, options, declaration0, tc->device, &mesh);
+ if (FAILED(hr))
+ {
+ skip("Couldn't create test mesh %#x\n", hr);
+ goto cleanup;
+ }
+
+ mesh->lpVtbl->LockVertexBuffer(mesh, 0, &pVertices);
+ memcpy(pVertices, vertices, sizeof(vertices));
+ mesh->lpVtbl->UnlockVertexBuffer(mesh);
+
+ mesh->lpVtbl->LockIndexBuffer(mesh, 0, &pFaces);
+ memcpy(pFaces, faces, sizeof(faces));
+ mesh->lpVtbl->UnlockIndexBuffer(mesh);
+
+ mesh->lpVtbl->LockAttributeBuffer(mesh, 0, &pAttributes);
+ memcpy(pAttributes, attributes, sizeof(attributes));
+ mesh->lpVtbl->UnlockAttributeBuffer(mesh);
+
+ /* Get the declaration and try to change it */
+ hr = mesh->lpVtbl->GetDeclaration(mesh, declaration);
+ if (FAILED(hr))
+ {
+ skip("Couldn't get mesh declaration %#x\n", hr);
+ goto cleanup;
+ }
+
+ for (pDecl = declaration; pDecl->Stream != 0xFF; pDecl++)
+ {
+ if (pDecl->Usage == D3DDECLUSAGE_POSITION)
+ {
+ /* Use second vertex position instead of first */
+ pDecl->Offset = offset;
+ }
+ }
+
+ hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration);
+ todo_wine 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));
+ hr = mesh->lpVtbl->GetDeclaration(mesh, declaration);
+ if (FAILED(hr))
+ {
+ skip("Couldn't get mesh declaration %#x\n", hr);
+ goto cleanup;
+ }
+
+ for (pDecl = declaration; pDecl->Stream != 0xFF; pDecl++)
+ {
+ if (pDecl->Usage == D3DDECLUSAGE_POSITION)
+ {
+ todo_wine ok(pDecl->Offset == offset, "Test UpdateSematics, got offset %d expected %d\n",
+ pDecl->Offset, offset);
+ }
+ }
+
+ /* UpdateSemantics does not check for a bigger vertex size */
+ memset(declaration, 0, sizeof(declaration));
+ hr = mesh->lpVtbl->GetDeclaration(mesh, declaration);
+ if (FAILED(hr))
+ {
+ skip("Couldn't get mesh declaration %#x\n", hr);
+ goto cleanup;
+ }
+
+ for (pDecl = declaration; pDecl->Stream != 0xFF; pDecl++)
+ {
+ if (pDecl->Type == D3DDECLTYPE_FLOAT3)
+ {
+ pDecl->Type = D3DDECLTYPE_FLOAT4;
+ }
+ }
+
+ hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration);
+ todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
+ "got %#x expected D3D_OK\n", hr);
+
+ /* Null pointer 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);
+
+ /*
+ * Two null pointers. Setting only the mesh to null will result in an
+ * exception on Windows.
+ */
+ hr = mesh->lpVtbl->UpdateSemantics(NULL, NULL);
+ todo_wine ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics null pointers, "
+ "got %#x expected D3DERR_INVALIDCALL\n", hr);
+
+cleanup:
+ if (mesh)
+ mesh->lpVtbl->Release(mesh);
+
+ FreeTestContext(tc);
+}
+
START_TEST(mesh)
{
D3DXBoundProbeTest();
@@ -3322,4 +3546,5 @@ START_TEST(mesh)
test_get_decl_vertex_size();
test_fvf_decl_conversion();
D3DXGenerateAdjacencyTest();
+ UpdateSemanticsTest();
}
diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c
index c5954b3..5854d4c 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
@@ -597,11 +598,31 @@ cleanup:
static HRESULT WINAPI ID3DXMeshImpl_UpdateSemantics(ID3DXMesh *iface, D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE])
{
+ HRESULT hr;
ID3DXMeshImpl *This = impl_from_ID3DXMesh(iface);
+ IDirect3DVertexDeclaration9 *new_vertex_declaration;
+ if (declaration == NULL)
+ return D3DERR_INVALIDCALL;
- FIXME("(%p)->(%p): stub\n", This, declaration);
+ TRACE("(%p)->(%p)\n", This, declaration);
- return E_NOTIMPL;
+ /* Create new vertex declaration */
+ hr = IDirect3DDevice9_CreateVertexDeclaration(This->device,
+ declaration,
+ &new_vertex_declaration);
+ if (FAILED(hr))
+ {
+ WARN("Unexpected return value %x from IDirect3DDevice9_CreateVertexDeclaration.\n",hr);
+ return hr;
+ }
+
+ /* Free old vertex declaration */
+ IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
+ /* Use new declaration */
+ This->vertex_declaration = new_vertex_declaration;
+ IDirect3DDevice9_SetVertexDeclaration(This->device, new_vertex_declaration);
+
+ return D3D_OK;
}
/*** ID3DXMesh ***/
diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c
index 77178fa..ac0eec0 100644
--- a/dlls/d3dx9_36/tests/mesh.c
+++ b/dlls/d3dx9_36/tests/mesh.c
@@ -3468,7 +3468,7 @@ static void UpdateSemanticsTest(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));
@@ -3483,7 +3483,7 @@ static void UpdateSemanticsTest(void)
{
if (pDecl->Usage == D3DDECLUSAGE_POSITION)
{
- todo_wine ok(pDecl->Offset == offset, "Test UpdateSematics, got offset %d expected %d\n",
+ ok(pDecl->Offset == offset, "Test UpdateSematics, got offset %d expected %d\n",
pDecl->Offset, offset);
}
}
@@ -3506,12 +3506,12 @@ static void UpdateSemanticsTest(void)
}
hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
+ ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
"got %#x expected D3D_OK\n", hr);
/* Null pointer declaration */
hr = mesh->lpVtbl->UpdateSemantics(mesh, NULL);
- todo_wine ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics null pointer declaration, "
+ ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics null pointer declaration, "
"got %#x expected D3DERR_INVALIDCALL\n", hr);
/*
@@ -3519,7 +3519,7 @@ static void UpdateSemanticsTest(void)
* exception on Windows.
*/
hr = mesh->lpVtbl->UpdateSemantics(NULL, NULL);
- todo_wine ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics null pointers, "
+ ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics null pointers, "
"got %#x expected D3DERR_INVALIDCALL\n", hr);
cleanup:
