[PATCH] d3dx9_36: Implement ID3DXSkinInfoImpl_UpdateSkinnedMesh.
This patch fixes last problem of bug 32572. https://bugs.winehq.org/show_bug.cgi?id=32572 From: Christian Costa <titan.costa(a)gmail.com> Signed-off-by: Vijay Kiran Kamuju <infyquest(a)gmail.com> --- dlls/d3dx9_36/skin.c | 86 ++++++++++++++++++++++++++++++++++++-- dlls/d3dx9_36/tests/mesh.c | 83 ++++++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 3 deletions(-) diff --git a/dlls/d3dx9_36/skin.c b/dlls/d3dx9_36/skin.c index f197d33582a..c27d0c7a8a8 100644 --- a/dlls/d3dx9_36/skin.c +++ b/dlls/d3dx9_36/skin.c @@ -2,6 +2,7 @@ * Skin Info operations specific to D3DX9. * * Copyright (C) 2011 Dylan Smith + * Copyright (C) 2013 Christian Costa * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -377,10 +378,89 @@ static HRESULT WINAPI d3dx9_skin_info_GetDeclaration(ID3DXSkinInfo *iface, static HRESULT WINAPI d3dx9_skin_info_UpdateSkinnedMesh(ID3DXSkinInfo *iface, const D3DXMATRIX *bone_transforms, const D3DXMATRIX *bone_inv_transpose_transforms, const void *src_vertices, void *dst_vertices) { - FIXME("iface %p, bone_transforms %p, bone_inv_transpose_transforms %p, src_vertices %p, dst_vertices %p stub!\n", - iface, bone_transforms, bone_inv_transpose_transforms, src_vertices, dst_vertices); + struct d3dx9_skin_info *skin = impl_from_ID3DXSkinInfo(iface); + DWORD size = D3DXGetFVFVertexSize(skin->fvf); + DWORD i, j; - return E_NOTIMPL; + TRACE("iface %p, bone_transforms %p, bone_inv_transpose_transforms %p, src_vertices %p, dst_vertices %p\n", + skin, bone_transforms, bone_inv_transpose_transforms, src_vertices, dst_vertices); + + if (bone_inv_transpose_transforms) + FIXME("Skinning vertices with two position elements not supported\n"); + + if ((skin->fvf & D3DFVF_POSITION_MASK) != D3DFVF_XYZ) { + FIXME("Vertex type %#x not supported\n", skin->fvf & D3DFVF_POSITION_MASK); + return E_FAIL; + } + + /* Reset all positions */ + for (i = 0; i < skin->num_vertices; i++) { + D3DXVECTOR3 *position = (D3DXVECTOR3*)((BYTE*)dst_vertices + size * i); + position->x = 0.0f; + position->y = 0.0f; + position->z = 0.0f; + } + + /* Update positions that are influenced by bones */ + for (i = 0; i < skin->num_bones; i++) { + D3DXMATRIX bone_inverse, matrix; + + D3DXMatrixInverse(&bone_inverse, NULL, &skin->bones[i].transform); + D3DXMatrixMultiply(&matrix, &bone_transforms[i], &bone_inverse); + D3DXMatrixMultiply(&matrix, &matrix, &skin->bones[i].transform); + + for (j = 0; j < skin->bones[i].num_influences; j++) { + D3DXVECTOR3 position; + D3DXVECTOR3 *position_src = (D3DXVECTOR3*)((BYTE*)src_vertices + size * skin->bones[i].vertices[j]); + D3DXVECTOR3 *position_dest = (D3DXVECTOR3*)((BYTE*)dst_vertices + size * skin->bones[i].vertices[j]); + FLOAT weight = skin->bones[i].weights[j]; + + D3DXVec3TransformCoord(&position, position_src, &matrix); + position_dest->x += weight * position.x; + position_dest->y += weight * position.y; + position_dest->z += weight * position.z; + } + } + + if (skin->fvf & D3DFVF_NORMAL) { + /* Reset all normals */ + for (i = 0; i < skin->num_vertices; i++) { + D3DXVECTOR3 *normal = (D3DXVECTOR3*)((BYTE*)dst_vertices + size * i + sizeof(D3DXVECTOR3)); + normal->x = 0.0f; + normal->y = 0.0f; + normal->z = 0.0f; + } + + /* Update normals that are influenced by bones */ + for (i = 0; i < skin->num_bones; i++) { + D3DXMATRIX bone_inverse, matrix; + + D3DXMatrixInverse(&bone_inverse, NULL, &skin->bones[i].transform); + D3DXMatrixMultiply(&matrix, &skin->bones[i].transform, &bone_transforms[i]); + + for (j = 0; j < skin->bones[i].num_influences; j++) { + D3DXVECTOR3 normal; + D3DXVECTOR3 *normal_src = (D3DXVECTOR3*)((BYTE*)src_vertices + size * skin->bones[i].vertices[j] + sizeof(D3DXVECTOR3)); + D3DXVECTOR3 *normal_dest = (D3DXVECTOR3*)((BYTE*)dst_vertices + size * skin->bones[i].vertices[j] + sizeof(D3DXVECTOR3)); + FLOAT weight = skin->bones[i].weights[j]; + + D3DXVec3TransformNormal(&normal, normal_src, &bone_inverse); + D3DXVec3TransformNormal(&normal, &normal, &matrix); + normal_dest->x += weight * normal.x; + normal_dest->y += weight * normal.y; + normal_dest->z += weight * normal.z; + } + } + + /* Normalize all normals that are influenced by bones*/ + for (i = 0; i < skin->num_vertices; i++) { + D3DXVECTOR3 *normal_dest = (D3DXVECTOR3*)((BYTE*)dst_vertices + (i * size) + sizeof(D3DXVECTOR3)); + if ((normal_dest->x != 0.0f) && (normal_dest->y != 0.0f) && (normal_dest->z != 0.0f)) + D3DXVec3Normalize(normal_dest, normal_dest); + } + } + + return D3D_OK; } static HRESULT WINAPI d3dx9_skin_info_ConvertToBlendedMesh(ID3DXSkinInfo *iface, ID3DXMesh *mesh_in, diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c index da9b80c578b..13dfb5286fd 100644 --- a/dlls/d3dx9_36/tests/mesh.c +++ b/dlls/d3dx9_36/tests/mesh.c @@ -5193,6 +5193,88 @@ static void test_create_skin_info(void) ok(hr == D3DERR_INVALIDCALL, "Expected D3DERR_INVALIDCALL, got %#x\n", hr); } +static void test_update_skinned_mesh(void) +{ + static DWORD bone0_vertices[2] = { 1, 3 }; + static FLOAT bone0_weights[2] = { 1.0f, 0.5f }; + static DWORD bone1_vertices[2] = { 2, 3 }; + static FLOAT bone1_weights[2] = { 1.0f, 0.5f }; + static D3DMATRIX bones_matrix[2] = + { { { { + 1.0f, 0.0f, 0.0f, 0.0f, + 0.0f, 1.0f, 0.0f, 0.0f, + 0.0f, 0.0f, 1.0f, 0.0f, + 2.0f, 2.0f, 4.0f, 1.0f + } } }, + { { { + 1.0f, 0.0f, 0.0f, 0.0f, + 0.0f, 1.0f, 0.0f, 0.0f, + 0.0f, 0.0f, 1.0f, 0.0f, + -4.0f, -4.0f, 4.0f, 1.0f + } } } }; + static D3DVECTOR vertices_src[] = {{ 1.0f, 1.0f, 1.0f }, + { 1.0f, 0.0f, 0.0f }, + { 1.0f, 1.0f, -1.0f }, + { 0.0f, 1.0f, 0.0f }, + { -1.0f, -1.0f, 1.0f }, + { 0.0f, 0.0f, 1.0f }, + { -1.0f, -1.0f, -1.0f }, + { -1.0f, 0.0f, 0.0f }, + }; + static D3DVECTOR vertices_ref[] = {{ 0.0f, 0.0f, 0.0f }, + { 0.0f, 0.0f, 0.0f }, + { 3.0f, 3.0f, 3.0f }, + { 0.0f, 1.0f, 0.0f }, + { -5.0f, -5.0f, 5.0f }, + { 0.0f, 0.0f, 1.0f }, + { -2.0f, -2.0f, 3.0f }, + { -1.0f, 0.0f, 0.0f }, + }; + D3DVECTOR vertices_dest[8]; + HRESULT hr; + ID3DXSkinInfo *skin_info; + D3DXMATRIX matrix; + int i; + + D3DXMatrixIdentity(&matrix); + for (i = 0; i < 8; i++) + { + vertices_dest[i].x = 10000.0f; + vertices_dest[i].y = 10000.0f; + vertices_dest[i].z = 10000.0f; + } + + hr = D3DXCreateSkinInfoFVF(4, D3DFVF_XYZ | D3DFVF_NORMAL, 2, &skin_info); + ok(hr == D3D_OK, "Expected D3D_OK, got %#x\n", hr); + + skin_info->lpVtbl->SetBoneInfluence(skin_info, 0, 2, bone0_vertices, bone0_weights); + ok(hr == D3D_OK, "Expected D3D_OK, got %#x\n", hr); + skin_info->lpVtbl->SetBoneOffsetMatrix(skin_info, 0, &matrix); + ok(hr == D3D_OK, "Expected D3D_OK, got %#x\n", hr); + skin_info->lpVtbl->SetBoneInfluence(skin_info, 1, 2, bone1_vertices, bone1_weights); + ok(hr == D3D_OK, "Expected D3D_OK, got %#x\n", hr); + skin_info->lpVtbl->SetBoneOffsetMatrix(skin_info, 1, &matrix); + ok(hr == D3D_OK, "Expected D3D_OK, got %#x\n", hr); + skin_info->lpVtbl->UpdateSkinnedMesh(skin_info, bones_matrix, NULL, vertices_src, vertices_dest); + ok(hr == D3D_OK, "Expected D3D_OK, got %#x\n", hr); + for (i = 0; i < 4; i++) + { + ok(compare(vertices_dest[i*2].x, vertices_ref[i*2].x), "Vertex[%d].position.x: got %g, expected %g\n", + i, vertices_dest[i*2].x, vertices_ref[i*2].x); + ok(compare(vertices_dest[i*2].y, vertices_ref[i*2].y), "Vertex[%d].position.y: got %g, expected %g\n", + i, vertices_dest[i*2].y, vertices_ref[i*2].y); + ok(compare(vertices_dest[i*2].z, vertices_ref[i*2].z), "Vertex[%d].position.z: got %g, expected %g\n", + i, vertices_dest[i*2].z, vertices_ref[i*2].z); + ok(compare(vertices_dest[i*2+1].x, vertices_ref[i*2+1].x), "Vertex[%d].normal.x: got %g, expected %g\n", + i, vertices_dest[i*2+1].x, vertices_ref[i*2+1].x); + ok(compare(vertices_dest[i*2+1].y, vertices_ref[i*2+1].y), "Vertex[%d].normal.y: got %g, expected %g\n", + i, vertices_dest[i*2+1].y, vertices_ref[i*2+1].y); + ok(compare(vertices_dest[i*2+1].z, vertices_ref[i*2+1].z), "Vertex[%d].normal.z: got %g, expected %g\n", + i, vertices_dest[i*2+1].z, vertices_ref[i*2+1].z); + } + skin_info->lpVtbl->Release(skin_info); +} + static void test_convert_adjacency_to_point_reps(void) { HRESULT hr; @@ -11393,6 +11475,7 @@ START_TEST(mesh) D3DXGenerateAdjacencyTest(); test_update_semantics(); test_create_skin_info(); + test_update_skinned_mesh(); test_convert_adjacency_to_point_reps(); test_convert_point_reps_to_adjacency(); test_weld_vertices(); -- 2.17.0
On Sat, Apr 6, 2019 at 10:50 AM Vijay Kiran Kamuju <infyquest(a)gmail.com> wrote:
This patch fixes last problem of bug 32572. https://bugs.winehq.org/show_bug.cgi?id=32572
From: Christian Costa <titan.costa(a)gmail.com> Signed-off-by: Vijay Kiran Kamuju <infyquest(a)gmail.com> ---
There are many stylistic and practical issues with this patch which I'll not get into the details of. I'll point out just one:
+ /* Update positions that are influenced by bones */ + for (i = 0; i < skin->num_bones; i++) { + D3DXMATRIX bone_inverse, matrix; + + D3DXMatrixInverse(&bone_inverse, NULL, &skin->bones[i].transform); + D3DXMatrixMultiply(&matrix, &bone_transforms[i], &bone_inverse); + D3DXMatrixMultiply(&matrix, &matrix, &skin->bones[i].transform);
This looks to me like a very expensive way of doing nothing.
On Tue, Apr 9, 2019 at 7:06 PM Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
On Sat, Apr 6, 2019 at 10:50 AM Vijay Kiran Kamuju <infyquest(a)gmail.com> wrote:
This patch fixes last problem of bug 32572. https://bugs.winehq.org/show_bug.cgi?id=32572
From: Christian Costa <titan.costa(a)gmail.com> Signed-off-by: Vijay Kiran Kamuju <infyquest(a)gmail.com> ---
There are many stylistic and practical issues with this patch which I'll not get into the details of. I'll point out just one:
+ /* Update positions that are influenced by bones */ + for (i = 0; i < skin->num_bones; i++) { + D3DXMATRIX bone_inverse, matrix; + + D3DXMatrixInverse(&bone_inverse, NULL, &skin->bones[i].transform); + D3DXMatrixMultiply(&matrix, &bone_transforms[i], &bone_inverse); + D3DXMatrixMultiply(&matrix, &matrix, &skin->bones[i].transform);
I will change the above code to like this for (i = 0; i < skin->num_bones; i++) { /* need to copy the contents of bone_transforms using another loop may be */ D3DXMATRIX matrix = bone_transforms[i]; float det_bone; det_bone = D3DXMatrixDeterminant(&skin->bones[i].transform); D3DXMatrixScaling(&matrix, det_bone, det_bone, det_bone);
This looks to me like a very expensive way of doing nothing.
I hope this is not expensive as far as I remember my Math, the above turns to det(skin->bones[i].transform) * bone_transforms[i]
On Wed, Apr 10, 2019 at 12:10 PM Vijay Kiran Kamuju <infyquest(a)gmail.com> wrote:
On Tue, Apr 9, 2019 at 7:06 PM Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
On Sat, Apr 6, 2019 at 10:50 AM Vijay Kiran Kamuju <infyquest(a)gmail.com> wrote:
This patch fixes last problem of bug 32572. https://bugs.winehq.org/show_bug.cgi?id=32572
From: Christian Costa <titan.costa(a)gmail.com> Signed-off-by: Vijay Kiran Kamuju <infyquest(a)gmail.com> ---
There are many stylistic and practical issues with this patch which I'll not get into the details of. I'll point out just one:
+ /* Update positions that are influenced by bones */ + for (i = 0; i < skin->num_bones; i++) { + D3DXMATRIX bone_inverse, matrix; + + D3DXMatrixInverse(&bone_inverse, NULL, &skin->bones[i].transform); + D3DXMatrixMultiply(&matrix, &bone_transforms[i], &bone_inverse); + D3DXMatrixMultiply(&matrix, &matrix, &skin->bones[i].transform);
I will change the above code to like this for (i = 0; i < skin->num_bones; i++) { /* need to copy the contents of bone_transforms using another loop may be */ D3DXMATRIX matrix = bone_transforms[i]; float det_bone;
det_bone = D3DXMatrixDeterminant(&skin->bones[i].transform); D3DXMatrixScaling(&matrix, det_bone, det_bone, det_bone);
This looks to me like a very expensive way of doing nothing.
I hope this is not expensive as far as I remember my Math, the above turns to det(skin->bones[i].transform) * bone_transforms[i]
My point is that the piece of code I referenced really does nothing useful with skin->bones[i].transform, which suggests deeper issues with the patch. The tests pass even with just "matrix = bone_transforms[i];" in place of that, FWIW.
On Wednesday, April 10, 2019, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
On Wed, Apr 10, 2019 at 12:10 PM Vijay Kiran Kamuju <infyquest(a)gmail.com> wrote:
On Tue, Apr 9, 2019 at 7:06 PM Matteo Bruni <matteo.mystral(a)gmail.com>
wrote:
On Sat, Apr 6, 2019 at 10:50 AM Vijay Kiran Kamuju <
infyquest(a)gmail.com> wrote:
This patch fixes last problem of bug 32572. https://bugs.winehq.org/show_bug.cgi?id=32572
From: Christian Costa <titan.costa(a)gmail.com> Signed-off-by: Vijay Kiran Kamuju <infyquest(a)gmail.com> ---
There are many stylistic and practical issues with this patch which I'll not get into the details of. I'll point out just one:
+ /* Update positions that are influenced by bones */ + for (i = 0; i < skin->num_bones; i++) { + D3DXMATRIX bone_inverse, matrix; + + D3DXMatrixInverse(&bone_inverse, NULL, &skin->bones[i].transform); + D3DXMatrixMultiply(&matrix, &bone_transforms[i], &bone_inverse); + D3DXMatrixMultiply(&matrix, &matrix, &skin->bones[i].transform);
I will change the above code to like this for (i = 0; i < skin->num_bones; i++) { /* need to copy the contents of bone_transforms using another loop may be */ D3DXMATRIX matrix = bone_transforms[i]; float det_bone;
det_bone = D3DXMatrixDeterminant(&skin->bones[i].transform); D3DXMatrixScaling(&matrix, det_bone, det_bone, det_bone);
This looks to me like a very expensive way of doing nothing.
I hope this is not expensive as far as I remember my Math, the above turns to det(skin->bones[i].transform) * bone_transforms[i]
My point is that the piece of code I referenced really does nothing useful with skin->bones[i].transform, which suggests deeper issues with the patch. The tests pass even with just "matrix = bone_transforms[i];" in place of that, FWIW.
It seems I have to do a deeper analysis of the code. I checked only whether the tests are successful or not.
On 04/10/2019 12:40 PM, Vijay Kiran Kamuju wrote:
It seems I have to do a deeper analysis of the code. I checked only whether the tests are successful or not.
For what it's worth (and since the concern was brought up to me offline by someone else), I feel like I should point out that Staging patches are not necessarily correct, either functionally or stylistically. That's the basic reason (even if it's not the only reason) that those patches are in Staging. Hence anyone who's interested in upstreaming Staging patches should treat the patch as their own responsibility, and should ensure that the patch is correct, and ideally have also tested it themself. That's part of what the signoff means. It's also the reason why Alistair and I are slow to upstream patches from Staging. There were a few hundred we tried to push upstream at an early point when we took over the repository, but most of those were the easy ones. We're still stuck with 800 patches which are nontrivial or take a fair amount of background knowledge to understand (like everything involved with ACLs) or, worse, which have no bug report or test application attached to them and thus no way to ensure that they are actually doing the right thing and fixing the problem (or, even more critically, will still be doing the right thing if we make some modifications.) Bottom line is, inasmuch as you should be expected to understand and write your own patches and ensure they're correct, the same rules apply to patches someone else wrote. You can't implicitly trust that they're correct, even according to that person's standards, and you certainly can't trust that they're correct just because they're in Staging.
On Wed, Apr 17, 2019 at 7:20 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
On 04/10/2019 12:40 PM, Vijay Kiran Kamuju wrote:
It seems I have to do a deeper analysis of the code. I checked only whether the tests are successful or not.
For what it's worth (and since the concern was brought up to me offline by someone else), I feel like I should point out that Staging patches are not necessarily correct, either functionally or stylistically. That's the basic reason (even if it's not the only reason) that those patches are in Staging. Hence anyone who's interested in upstreaming Staging patches should treat the patch as their own responsibility, and should ensure that the patch is correct, and ideally have also tested it themself. That's part of what the signoff means. It's also the reason why Alistair and I are slow to upstream patches from Staging. There were a few hundred we tried to push upstream at an early point when we took over the repository, but most of those were the easy ones. We're still stuck with 800 patches which are nontrivial or take a fair amount of background knowledge to understand (like everything involved with ACLs) or, worse, which have no bug report or test application attached to them and thus no way to ensure that they are actually doing the right thing and fixing the problem (or, even more critically, will still be doing the right thing if we make some modifications.)
I am trying to write tests and improve upon or rewrite the patches. For most of the patches tests are present.
Bottom line is, inasmuch as you should be expected to understand and write your own patches and ensure they're correct, the same rules apply to patches someone else wrote. You can't implicitly trust that they're correct, even according to that person's standards, and you certainly can't trust that they're correct just because they're in Staging.
Sometime I try to contact the author, but for some old patches I doubt the author remembers the application or why he/she has written the patch. We have to try to push the patches upstream regardless, as there are no explanations or rejection reasons given. This makes it a bit harder to improve upon the existing patch. The upstreaming is currently a slow process and reviews are incomplete. So I am trying to pick some easy ones and old ones so we get a proper review at least this time. Rejection without proper comments is always a major pain point for Wine.
participants (3)
-
Matteo Bruni -
Vijay Kiran Kamuju -
Zebediah Figura