[PATCH 0/1] MR4031: d3dx9: D3DXLoadMeshHierarchyFromXInMemory support empty mesh.
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/d3dx9_36/mesh.c | 29 +++++++++++++++++++----- dlls/d3dx9_36/tests/mesh.c | 46 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/dlls/d3dx9_36/mesh.c b/dlls/d3dx9_36/mesh.c index 7aa70783735..e761781e94c 100644 --- a/dlls/d3dx9_36/mesh.c +++ b/dlls/d3dx9_36/mesh.c @@ -3506,6 +3506,15 @@ HRESULT WINAPI D3DXLoadSkinMeshFromXof(struct ID3DXFileData *filedata, DWORD opt hr = parse_mesh(filedata, &mesh_data, provide_flags); if (FAILED(hr)) goto cleanup; + if (mesh_data.num_vertices == 0) { + if (adjacency_out) *adjacency_out = NULL; + if (materials_out) *materials_out = NULL; + if (effects_out) *effects_out = NULL; + *mesh_out = NULL; + hr = D3D_OK; + goto cleanup; + } + total_vertices = mesh_data.num_vertices; if (mesh_data.fvf & D3DFVF_NORMAL) { /* duplicate vertices with multiple normals */ @@ -3815,7 +3824,8 @@ static HRESULT load_mesh_container(struct ID3DXFileData *filedata, DWORD options hr = filedata_get_name(filedata, &name); if (FAILED(hr)) goto cleanup; - hr = alloc_hier->lpVtbl->CreateMeshContainer(alloc_hier, name, &mesh_data, + if (mesh_data.pMesh) + hr = alloc_hier->lpVtbl->CreateMeshContainer(alloc_hier, name, &mesh_data, materials ? ID3DXBuffer_GetBufferPointer(materials) : NULL, effects ? ID3DXBuffer_GetBufferPointer(effects) : NULL, num_materials, @@ -4200,13 +4210,18 @@ static HRESULT parse_frame(struct ID3DXFileData *filedata, DWORD options, struct hr = E_OUTOFMEMORY; goto err; } - list_add_tail(container_list, &container->entry); - container->transform = transform; hr = D3DXLoadSkinMeshFromXof(child, options, device, (provide_flags & PROVIDE_ADJACENCY) ? &container->adjacency : NULL, (provide_flags & PROVIDE_MATERIALS) ? &container->materials : NULL, NULL, &container->num_materials, NULL, &container->mesh); + + if (container->mesh) { + list_add_tail(container_list, &container->entry); + container->transform = transform; + } else { + HeapFree(GetProcessHeap(), HEAP_ZERO_MEMORY, container); + } } else if (IsEqualGUID(&type, &TID_D3DRMFrameTransformMatrix)) { D3DXMATRIX new_transform; hr = parse_transform_matrix(child, &new_transform); @@ -4294,13 +4309,17 @@ HRESULT WINAPI D3DXLoadMeshFromXInMemory(const void *memory, DWORD memory_size, hr = E_OUTOFMEMORY; goto cleanup; } - list_add_tail(&container_list, &container_ptr->entry); - D3DXMatrixIdentity(&container_ptr->transform); hr = D3DXLoadSkinMeshFromXof(filedata, options, device, (provide_flags & PROVIDE_ADJACENCY) ? &container_ptr->adjacency : NULL, (provide_flags & PROVIDE_MATERIALS) ? &container_ptr->materials : NULL, NULL, &container_ptr->num_materials, NULL, &container_ptr->mesh); + if (container_ptr->mesh) { + list_add_tail(&container_list, &container_ptr->entry); + D3DXMatrixIdentity(&container_ptr->transform); + } else { + HeapFree(GetProcessHeap(), 0, container_ptr); + } } else if (IsEqualGUID(&guid, &TID_D3DRMFrame)) { hr = parse_frame(filedata, options, device, &identity, &container_list, provide_flags); } diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index e5ba6e24996..b4021844c93 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -2068,6 +2068,14 @@ static void D3DXLoadMeshTest(void) "}" "Mesh { 3; 0.0; 0.0; 0.0;, 0.0; 1.0; 0.0;, 3.0; 1.0; 0.0;; 1; 3; 0, 1, 2;; }" "}"; + static const char framed_xfile2[] = + "xof 0303txt 0032" + "Frame Box01 {" + "Mesh { 0;; 0;;" + "MeshNormals { 0;; 0;; }" + "}" + "}"; + static const WORD framed_index_buffer[] = { 0, 1, 2 }; static const D3DXVECTOR3 framed_vertex_buffers[3][3] = { {{0.0, 0.0, 0.0}, {0.0, 1.0, 0.0}, {1.0, 1.0, 0.0}}, @@ -2513,6 +2521,19 @@ static void D3DXLoadMeshTest(void) frame_hier = NULL; } + hr = D3DXLoadMeshHierarchyFromXInMemory(framed_xfile2, sizeof(framed_xfile2) - 1, + D3DXMESH_MANAGED, device, &alloc_hier, NULL, &frame_hier, NULL); + ok(hr == D3D_OK, "Expected D3D_OK, got %#lx\n", hr); + if (SUCCEEDED(hr)) { + D3DXMESHCONTAINER *container = frame_hier->pMeshContainer; + + ok(!strcmp(frame_hier->Name, "Box01"), "Expected '', got '%s'\n", frame_hier->Name); + ok(container == NULL, "Expected NULL, got %p\n", container); + + hr = D3DXFrameDestroy(frame_hier, &alloc_hier); + ok(hr == D3D_OK, "Expected D3D_OK, got %#lx\n", hr); + frame_hier = NULL; + } hr = D3DXLoadMeshFromXInMemory(NULL, 0, D3DXMESH_MANAGED, device, NULL, NULL, NULL, NULL, &mesh); @@ -11323,6 +11344,9 @@ static void test_load_skin_mesh_from_xof(void) "1;" "3; 0, 1, 2;;" "}"; + static const char simple_xfile_empty[] = + "xof 0303txt 0032" + "Mesh { 0;; 0;; }"; static const D3DVERTEXELEMENT9 expected_declaration[] = { {0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0}, @@ -11434,6 +11458,28 @@ static void test_load_skin_mesh_from_xof(void) mesh->lpVtbl->Release(mesh); adjacency->lpVtbl->Release(adjacency); file_data->lpVtbl->Release(file_data); + + /* Empty Mesh Test */ + file_data = get_mesh_data(simple_xfile_empty, sizeof(simple_xfile_empty) - 1); + ok(!!file_data, "Failed to load mesh data.\n"); + + adjacency = materials = effects = (void *)0xdeadbeef; + count = ~0u; + skin_info = (void *)0xdeadbeef; + mesh = (void *)0xdeadbeef; + + hr = D3DXLoadSkinMeshFromXof(file_data, 0, device, &adjacency, &materials, &effects, &count, + &skin_info, &mesh); + todo_wine ok(hr == D3DXERR_LOADEDMESHASNODATA, "Got unexpected hr %#lx.\n", hr); + ok(!adjacency, "Got unexpected value %p.\n", adjacency); + ok(!materials, "Got unexpected value %p.\n", materials); + ok(!effects, "Got unexpected value %p.\n", effects); + ok(count == ~0u, "Got unexpected value %lu.\n", count); + ok(skin_info == (void *)0xdeadbeef, "Got unexpected value %p.\n", skin_info); + ok(!mesh, "Got unexpected value %p.\n", mesh); + + file_data->lpVtbl->Release(file_data); + refcount = IDirect3DDevice9_Release(device); ok(!refcount, "Device has %lu references left.\n", refcount); DestroyWindow(hwnd); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4031
Matteo Bruni (@Mystral) commented about dlls/d3dx9_36/tests/mesh.c:
frame_hier = NULL; }
+ hr = D3DXLoadMeshHierarchyFromXInMemory(framed_xfile2, sizeof(framed_xfile2) - 1, + D3DXMESH_MANAGED, device, &alloc_hier, NULL, &frame_hier, NULL); + ok(hr == D3D_OK, "Expected D3D_OK, got %#lx\n", hr); + if (SUCCEEDED(hr)) { + D3DXMESHCONTAINER *container = frame_hier->pMeshContainer; + + ok(!strcmp(frame_hier->Name, "Box01"), "Expected '', got '%s'\n", frame_hier->Name); + ok(container == NULL, "Expected NULL, got %p\n", container); + + hr = D3DXFrameDestroy(frame_hier, &alloc_hier); + ok(hr == D3D_OK, "Expected D3D_OK, got %#lx\n", hr); + frame_hier = NULL; + }
It would be best if we followed the current style conventions in new code instead of adding more code using the old ones. Specifically: - Just an "Unexpected hr %#lx.\n" (or "name", or whatever the message calls for) is an appropriate ok() message - No need for the if (SUCCEEDED(hr)) {}, this is never supposed to fail - '{' on its own line - !container instead of == NULL -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4031#note_47922
Matteo Bruni (@Mystral) commented about dlls/d3dx9_36/tests/mesh.c:
mesh->lpVtbl->Release(mesh); adjacency->lpVtbl->Release(adjacency); file_data->lpVtbl->Release(file_data); + + /* Empty Mesh Test */ + file_data = get_mesh_data(simple_xfile_empty, sizeof(simple_xfile_empty) - 1); + ok(!!file_data, "Failed to load mesh data.\n"); + + adjacency = materials = effects = (void *)0xdeadbeef; + count = ~0u;
Can you please initialize this to 0xdeadbeefu or so? Just to clarify that it's somehow not initialized by the function to ~0u (which would be weird but not as much as a 0xdeadbeef value...) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4031#note_47923
Matteo Bruni (@Mystral) commented about dlls/d3dx9_36/mesh.c:
hr = parse_mesh(filedata, &mesh_data, provide_flags); if (FAILED(hr)) goto cleanup;
+ if (mesh_data.num_vertices == 0) { + if (adjacency_out) *adjacency_out = NULL; + if (materials_out) *materials_out = NULL; + if (effects_out) *effects_out = NULL; + *mesh_out = NULL; + hr = D3D_OK; + goto cleanup; + } +
Same as above (!var instead of var == 0, '{' on its own line), but additionally please put the if () body on its own line. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4031#note_47924
Matteo Bruni (@Mystral) commented about dlls/d3dx9_36/mesh.c:
hr = filedata_get_name(filedata, &name); if (FAILED(hr)) goto cleanup;
- hr = alloc_hier->lpVtbl->CreateMeshContainer(alloc_hier, name, &mesh_data, + if (mesh_data.pMesh) + hr = alloc_hier->lpVtbl->CreateMeshContainer(alloc_hier, name, &mesh_data, materials ? ID3DXBuffer_GetBufferPointer(materials) : NULL, effects ? ID3DXBuffer_GetBufferPointer(effects) : NULL, num_materials,
I prefer to have brackets around multiline (even though single statement) blocks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4031#note_47925
This looks mostly good, I only left a bunch of minor comments. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4031#note_47926
participants (3)
-
Alistair Leslie-Hughes -
Alistair Leslie-Hughes (＠alesliehughes)
-
Matteo Bruni (＠Mystral)