On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger <stefandoesin...@gmx.at> wrote: > On Saturday 28 May 2011 19:55:55 you wrote: > >> It was implemented by setting the vertex declaration to null when the >> declaration is invalid. That seems to match what happens on Windows, >> because if a program has set an invalid declaration and then calls >> GetDeclaration or DrawSubset it will fail with an access violation. >> Should Wine also fail with an access violation or should it be >> slightly more graceful and return E_FAIL instead (which my new patch >> does)? > Yes, I think returning E_FAIL is a good idea, but write a WARN if > CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not > be shown by default(unlike a FIXME), it is used when the app does something > fishy but you think you handled it correctly. > >> + if (FAILED(hr)) >> + new_vertex_declaration = NULL; >> + >> + /* Free old vertex declaration */ >> + if (This->vertex_declaration) >> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); > In this situation you can just release the old decl before calling > CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. > That simplified the code a bit. Don't forget to set This->vertex_declaration > to > NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration. >> + /* Set the position type to color instead of float3 (invalid > declaration) */ >> + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); >> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >> + "got %#x expected D3D_OK\n", hr); > Is this really invalid? I don't know it on top of my head, but in general > types and usage are independent, especially with shaders. I don't see a check > in d3d9 or wined3d for this case. You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests. > Beyond that I think the patches look OK. I'd recommend to send them to wine- > devel one more time and then next week to wine-patches. Henri is on Vacation > this week, and he's usually more picky than I am. I also hope some of the > people who regularly contribute to d3dx9 will have a look and comment. Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index cedef32..975c0e8 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 @@ -61,6 +62,93 @@ static BOOL compare_face(face a, face b) return (a[0]==b[0] && a[1] == b[1] && a[2] == b[2]); } +struct TestContext +{ + HWND hwnd; + IDirect3D9 *d3d; + IDirect3DDevice9 *device; +}; + +/* + * Initializes a TestContext. Use this to initialize DirectX. + * + * Returns NULL if an error occured. + */ +static struct TestContext* NewTestContext(void) +{ + HRESULT hr; + HWND hwnd = NULL; + IDirect3D9 *d3d = NULL; + IDirect3DDevice9 *device = NULL; + D3DPRESENT_PARAMETERS d3dpp = {0}; + struct TestContext *testContext; + + hwnd = CreateWindow("static", "d3dx9_test", 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL); + if (!hwnd) + { + skip("Couldn't create application window\n"); + goto error; + } + + d3d = Direct3DCreate9(D3D_SDK_VERSION); + if (!d3d) + { + skip("Couldn't create IDirect3D9 object\n"); + 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)) + { + skip("Couldn't create IDirect3DDevice9 object %#x\n", hr); + goto error; + } + + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); + if (!testContext) + { + skip("Couldn't allocate memory for TestContext\n"); + goto error; + } + testContext->hwnd = hwnd; + testContext->d3d = d3d; + testContext->device = device; + + return testContext; + +error: + if (device) + IDirect3DDevice9_Release(device); + + if (d3d) + IDirect3D9_Release(d3d); + + if (hwnd) + DestroyWindow(hwnd); + + return NULL; +} + +static void FreeTestContext(struct TestContext *testContext) +{ + if (!testContext) + return; + + if (testContext->device) + IDirect3DDevice9_Release(testContext->device); + + if (testContext->d3d) + IDirect3D9_Release(testContext->d3d); + + if (testContext->hwnd) + DestroyWindow(testContext->hwnd); + + HeapFree(GetProcessHeap(), 0, testContext); +} + struct mesh { DWORD number_of_vertices; @@ -3305,6 +3393,207 @@ static void D3DXGenerateAdjacencyTest(void) if (d3dxmesh) d3dxmesh->lpVtbl->Release(d3dxmesh); } +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() + }; + D3DVERTEXELEMENT9 declaration_pos_type_color[] = + { + {0, 0, D3DDECLTYPE_D3DCOLOR, 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() + }; + D3DVERTEXELEMENT9 declaration_double_usage[] = + { + {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, + {0, 12, 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() + }; + D3DVERTEXELEMENT9 declaration_undefined_type[] = + { + {0, 0, D3DDECLTYPE_UNUSED+1, 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() + }; + D3DVERTEXELEMENT9 declaration_not_4_byte_aligned_offset[] = + { + {0, 3, 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; + + tc = NewTestContext(); + if (!tc) + { + skip("Couldn't create a TestContext"); + 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); + + /* 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); + + /* + * The following test cases show that invalid declarations are accepted + * with a D3D_OK. An access violations occurs however, if any of the + * functions that use the declaration is called, i.e. GetDeclaration and + * DrawSubset. + */ + + /* Double usage (invalid 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); + + /* Set the position to an undefined type (invalid 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); + + /* Use a not 4 byte aligned offset (invalid 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, " + "got %#x expected D3D_OK\n", hr); + +cleanup: + if (mesh) + mesh->lpVtbl->Release(mesh); + + FreeTestContext(tc); +} + START_TEST(mesh) { D3DXBoundProbeTest(); @@ -3322,4 +3611,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 48ae20e..c283e34 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 @@ -105,7 +106,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); @@ -126,6 +128,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); @@ -193,6 +201,11 @@ static HRESULT WINAPI ID3DXMeshImpl_GetDeclaration(ID3DXMesh *iface, D3DVERTEXEL TRACE("(%p)\n", This); if (declaration == NULL) return D3DERR_INVALIDCALL; + if (!This->vertex_declaration) + { + WARN("Can't get vertex declaration because it's invalid.\n"); + return E_FAIL; + } return IDirect3DVertexDeclaration9_GetDeclaration(This->vertex_declaration, declaration, @@ -207,6 +220,12 @@ static DWORD WINAPI ID3DXMeshImpl_GetNumBytesPerVertex(ID3DXMesh *iface) TRACE("iface (%p)\n", This); + if (!This->vertex_declaration) + { + WARN("Can't get number of bytes per vertex of an invalid vertex declaration.\n"); + return E_FAIL; + } + IDirect3DVertexDeclaration9_GetDeclaration(This->vertex_declaration, declaration, &numelements); @@ -602,11 +621,32 @@ cleanup: static HRESULT WINAPI ID3DXMeshImpl_UpdateSemantics(ID3DXMesh *iface, D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE]) { + HRESULT hr; ID3DXMeshImpl *This = impl_from_ID3DXMesh(iface); + if (!declaration) + return D3DERR_INVALIDCALL; - FIXME("(%p)->(%p): stub\n", This, declaration); + TRACE("(%p)->(%p)\n", This, declaration); - return E_NOTIMPL; + 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 GetDeclaration, GetNumBytesPerVertex, and DrawSubset, + * which use the declaration, will fail. + */ + hr = IDirect3DDevice9_CreateVertexDeclaration(This->device, + declaration, + &This->vertex_declaration); + if (FAILED(hr)) + { + WARN("Invalid declaration passed to UpdateSemantics.\n"); + This->vertex_declaration = NULL; + } + + return D3D_OK; } /*** ID3DXMesh ***/ diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index 975c0e8..90d7599 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -3506,7 +3506,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)); @@ -3521,7 +3521,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); } } @@ -3544,12 +3544,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); /* @@ -3557,12 +3557,12 @@ 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); /* 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, " + ok(hr == D3D_OK, "Test UpdateSematics position type color, " "got %#x expected D3D_OK\n", hr); /* @@ -3574,17 +3574,17 @@ static void UpdateSemanticsTest(void) /* Double usage (invalid declaration) */ hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_double_usage); - todo_wine ok(hr == D3D_OK, "Test UpdateSematics double usage, " + ok(hr == D3D_OK, "Test UpdateSematics double usage, " "got %#x expected D3D_OK\n", hr); /* Set the position to an undefined type (invalid declaration) */ hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_undefined_type); - todo_wine ok(hr == D3D_OK, "Test UpdateSematics undefined type, " + ok(hr == D3D_OK, "Test UpdateSematics undefined type, " "got %#x expected D3D_OK\n", hr); /* Use a not 4 byte aligned offset (invalid 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); cleanup: