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 stefandoesinger@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.
On Friday 17 June 2011 12:14:59 Michael Mc Donnell wrote:
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.
Looks good!
On Fri, Jun 17, 2011 at 1:07 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Friday 17 June 2011 12:14:59 Michael Mc Donnell wrote:
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.
Looks good!
Ok I've sent them to wine-patches.
On Friday 17 June 2011 14:27:55 Michael Mc Donnell wrote:
Ok I've sent them to wine-patches.
They didn't make it through yet. Are you subscribed? (If you're not subscribed, don't resend them yet, Newman will most likely let them through manually later today)
On Fri, Jun 17, 2011 at 3:02 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Friday 17 June 2011 14:27:55 Michael Mc Donnell wrote:
Ok I've sent them to wine-patches.
They didn't make it through yet. Are you subscribed? (If you're not subscribed, don't resend them yet, Newman will most likely let them through manually later today)
Nope I'm not subscribed to wine-patches. I'll wait.
Here is my test and implementation of ConvertAdjacencyToPointReps.
Note that two of the helper functions and a struct in the test were also included in my UpdateSemantics patch. I'll remove them once my UpdateSemantics patch is in the official tree.
Any comments?
Thanks, Michael Mc Donnell
On Tue, Jun 21, 2011 at 6:32 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Here is my test and implementation of ConvertAdjacencyToPointReps.
- hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, (void**)&indices);
- if (FAILED(hr)) goto cleanup;
- memcpy(new_indices, indices, This->numvertices * sizeof(*indices));
I think you want the number of faces * 3 rather than the number of vertices. And the size of an index is not constant, it is 32-bit if This->options has the D3DXMESH_32BIT bit set and 16-bit if the bit isn't set.
+static void test_convert_adjacency_to_point_reps(void) +{
...
- /* All mesh data */
- struct vertex_pnc *vertices[] = {vertices0, vertices1, vertices2, vertices3, vertices4, vertices5, vertices6};
- ID3DXMesh *meshes[ARRAY_SIZE(vertices)];
- DWORD *indices[] = {indices0, indices1, indices2, indices3, indices4, indices5, indices6};
- DWORD num_vertices[] = {num_vertices0, num_vertices1, num_vertices2, num_vertices3, num_vertices4, num_vertices5, num_vertices6};
- DWORD num_faces[] = {num_faces0, num_faces1, num_faces2, num_faces3, num_faces4, num_faces5, num_faces6};
- DWORD *adjacencies[] = {adjacency0, adjacency1, adjacency2, adjacency3, adjacency4, adjacency5, adjacency6};
- DWORD *point_reps[] = {point_rep0, point_rep1, point_rep2, point_rep3, point_rep4, point_rep5, point_rep6};
- DWORD *exp_point_reps[] = {exp_point_rep0, exp_point_rep1, exp_point_rep2, exp_point_rep3, exp_point_rep4, exp_point_rep5, exp_point_rep6};
I think it would make sense to put all these arrays of the same length into a structure (e.g. struct test_case) array. This would make it clear what needs to be updated to add another test case, and less error prone than making sure each relevant individual array is updated.
Also, several of these and similar array seem like they can be made constant.
+static void test_convert_adjacency_to_point_reps(void) +{
...
- for (i = 0; i < ARRAY_SIZE(meshes); i++)
- {
...
hr = meshes[i]->lpVtbl->ConvertAdjacencyToPointReps(meshes[i], adjacencies[i], point_reps[i]);todo_wine ok(hr == D3D_OK, "ConvertAdjacencyToPointReps failed. ""Got %x expected D3D_OK\n", hr);
The value i should probably be printed in this error message. Otherwise, if you see a failure on test.winehq.org that you can't reproduce, it will be hard to figure out what might have caused the failure.
That's it for now.
On Tue, Jun 21, 2011 at 5:36 PM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Tue, Jun 21, 2011 at 6:32 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Here is my test and implementation of ConvertAdjacencyToPointReps.
- hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, (void**)&indices);
- if (FAILED(hr)) goto cleanup;
- memcpy(new_indices, indices, This->numvertices * sizeof(*indices));
I think you want the number of faces * 3 rather than the number of vertices. And the size of an index is not constant, it is 32-bit if This->options has the D3DXMESH_32BIT bit set and 16-bit if the bit isn't set.
Yes you're right, I'll change that. I had completely forgotten about 16-bit indices, so that required a lot more changes and a test.
+static void test_convert_adjacency_to_point_reps(void) +{
...
- /* All mesh data */
- struct vertex_pnc *vertices[] = {vertices0, vertices1, vertices2, vertices3, vertices4, vertices5, vertices6};
- ID3DXMesh *meshes[ARRAY_SIZE(vertices)];
- DWORD *indices[] = {indices0, indices1, indices2, indices3, indices4, indices5, indices6};
- DWORD num_vertices[] = {num_vertices0, num_vertices1, num_vertices2, num_vertices3, num_vertices4, num_vertices5, num_vertices6};
- DWORD num_faces[] = {num_faces0, num_faces1, num_faces2, num_faces3, num_faces4, num_faces5, num_faces6};
- DWORD *adjacencies[] = {adjacency0, adjacency1, adjacency2, adjacency3, adjacency4, adjacency5, adjacency6};
- DWORD *point_reps[] = {point_rep0, point_rep1, point_rep2, point_rep3, point_rep4, point_rep5, point_rep6};
- DWORD *exp_point_reps[] = {exp_point_rep0, exp_point_rep1, exp_point_rep2, exp_point_rep3, exp_point_rep4, exp_point_rep5, exp_point_rep6};
I think it would make sense to put all these arrays of the same length into a structure (e.g. struct test_case) array. This would make it clear what needs to be updated to add another test case, and less error prone than making sure each relevant individual array is updated.
Ok I've tried to make a struct for them.
Also, several of these and similar array seem like they can be made constant.
Check, I've const'ified everything I could find.
+static void test_convert_adjacency_to_point_reps(void) +{
...
- for (i = 0; i < ARRAY_SIZE(meshes); i++)
- {
...
- hr = meshes[i]->lpVtbl->ConvertAdjacencyToPointReps(meshes[i], adjacencies[i], point_reps[i]);
- todo_wine ok(hr == D3D_OK, "ConvertAdjacencyToPointReps failed. "
- "Got %x expected D3D_OK\n", hr);
The value i should probably be printed in this error message. Otherwise, if you see a failure on test.winehq.org that you can't reproduce, it will be hard to figure out what might have caused the failure.
Yes, good idea.
That's it for now.
Thanks your comments. I've attached an updated version.
On Wed, Jun 22, 2011 at 10:12 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Thanks your comments. I've attached an updated version.
if (indices) HeapFree(GetProcessHeap(), 0, indices);- }
- if (new_indices) HeapFree(GetProcessHeap(), 0, new_indices);
HeapFree checks for NULL for you, so remove the NULL check before calling HeapFree.
- struct
- {
ID3DXMesh *meshes[ARRAY_SIZE(vertices)];const DWORD *indices[ARRAY_SIZE(vertices)];const DWORD num_vertices[ARRAY_SIZE(vertices)];const DWORD *adjacencies[ARRAY_SIZE(vertices)];DWORD *point_reps[ARRAY_SIZE(vertices)];const DWORD *exp_point_reps[ARRAY_SIZE(vertices)];const DWORD options[ARRAY_SIZE(vertices)];- }
- tc =
- {
{NULL, NULL, NULL, NULL, NULL, NULL, NULL},{indices0, indices1, indices2, indices3, indices4, indices5, indices6, (DWORD*)indices6_16bit},{num_vertices0, num_vertices1, num_vertices2, num_vertices3, num_vertices4, num_vertices5, num_vertices6, num_vertices6},{adjacency0, adjacency1, adjacency2, adjacency3, adjacency4, adjacency5, adjacency6, adjacency6},{point_rep0, point_rep1, point_rep2, point_rep3, point_rep4, point_rep5, point_rep6, point_rep6},{exp_point_rep0, exp_point_rep1, exp_point_rep2, exp_point_rep3, exp_point_rep4, exp_point_rep5, exp_point_rep6, exp_point_rep6},{options, options, options, options, options, options, options, options_16bit}- };
I actually meant something more like:
--- snip --- struct { const DWORD *indices, const DWORD num_vertices, const DWORD *adjacencies, DWORD *point_reps, const DWORD *exp_point_reps, const DWORD options, } tc[] = { {indices0, num_vertices0, adjacency0, point_rep0, exp_point_rep0, options}, {indices1, num_vertices1, adjacency1, point_rep1, exp_point_rep1, options}, ... }
...
for (i = 0; i < ARRAY_SIZE(tc); i++) { ... } --- snip ---
I don't think storing an array of meshes is necessary. You can just release the mesh and set the mesh variable to NULL (as needed to avoid re-releasing) before creating another one.
On Wed, Jun 22, 2011 at 4:53 PM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Wed, Jun 22, 2011 at 10:12 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Thanks your comments. I've attached an updated version.
- if (indices) HeapFree(GetProcessHeap(), 0, indices);
- }
- if (new_indices) HeapFree(GetProcessHeap(), 0, new_indices);
HeapFree checks for NULL for you, so remove the NULL check before calling HeapFree.
Ok. I see that now in the Wine source. That's a lot nicer than on Windows.
- struct
- {
- ID3DXMesh *meshes[ARRAY_SIZE(vertices)];
- const DWORD *indices[ARRAY_SIZE(vertices)];
- const DWORD num_vertices[ARRAY_SIZE(vertices)];
- const DWORD *adjacencies[ARRAY_SIZE(vertices)];
- DWORD *point_reps[ARRAY_SIZE(vertices)];
- const DWORD *exp_point_reps[ARRAY_SIZE(vertices)];
- const DWORD options[ARRAY_SIZE(vertices)];
- }
- tc =
- {
- {NULL, NULL, NULL, NULL, NULL, NULL, NULL},
- {indices0, indices1, indices2, indices3, indices4, indices5, indices6, (DWORD*)indices6_16bit},
- {num_vertices0, num_vertices1, num_vertices2, num_vertices3, num_vertices4, num_vertices5, num_vertices6, num_vertices6},
- {adjacency0, adjacency1, adjacency2, adjacency3, adjacency4, adjacency5, adjacency6, adjacency6},
- {point_rep0, point_rep1, point_rep2, point_rep3, point_rep4, point_rep5, point_rep6, point_rep6},
- {exp_point_rep0, exp_point_rep1, exp_point_rep2, exp_point_rep3, exp_point_rep4, exp_point_rep5, exp_point_rep6, exp_point_rep6},
- {options, options, options, options, options, options, options, options_16bit}
- };
I actually meant something more like:
--- snip --- struct { const DWORD *indices, const DWORD num_vertices, const DWORD *adjacencies, DWORD *point_reps, const DWORD *exp_point_reps, const DWORD options, } tc[] = { {indices0, num_vertices0, adjacency0, point_rep0, exp_point_rep0, options}, {indices1, num_vertices1, adjacency1, point_rep1, exp_point_rep1, options}, ... }
Ah ok :-) That makes more sense.
...
for (i = 0; i < ARRAY_SIZE(tc); i++) { ... } --- snip ---
I don't think storing an array of meshes is necessary. You can just release the mesh and set the mesh variable to NULL (as needed to avoid re-releasing) before creating another one.
I've changed it so that it only keeps the first mesh around for the NULL checks.
My UpdateSemantics patch finally got applied so I've removed the duplicate functions too.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 22.06.2011 um 21:48 schrieb Michael Mc Donnell:
HeapFree checks for NULL for you, so remove the NULL check before calling HeapFree.
Ok. I see that now in the Wine source. That's a lot nicer than on Windows.
Windows also checks for NULL pointers in HeapFree. Otherwise Wine wouldn't do so, and we'd need the NULL checks to be able to run our code on Windows.
My UpdateSemantics patch finally got applied
Congratulations!
I'll have to do some more background reading on the kind of mesh data structures used here before I can give qualified comments on your patches, so for now I'll yield to Dylan :-)
On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
I'll have to do some more background reading on the kind of mesh data structures used here before I can give qualified comments on your patches, so for now I'll yield to Dylan :-)
I couldn't actually find information on point representative data (aka PointReps), but a quick search found others wondering what it is. Basically it is an array of vertex indices, one for each vertex, each with either its own index or the index of an co-located vertex.
e.g. If you have triangles ABC and DEF (as shown below), with two sets of points co-located, and the vertex buffer stored in alphabetical order. Then the point_reps array would be [0, 1, 2, 2, 1, 5] to indicate that vertices B & C can replace E & D respectively.
A---(B/E) | / | | / | (C/D)--F
The adjacency buffer is documented well enough on MSDN for GenerateAdjacency (http://msdn.microsoft.com/en-us/library/bb205737(v=VS.85).aspx) and seems to be used in this method for determining which vertices are co-located.
I hope that helps.
On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
I'll have to do some more background reading on the kind of mesh data structures used here before I can give qualified comments on your patches, so for now I'll yield to Dylan :-)
I looked over the latest patches, and the code looks good now, but I have a couple of nitpicks for the comments.
+/* ConvertAdjacencyToPointReps helper function. + * + * Goes around the edges of a face and replaces the vertices in any adjacent
(changed word is in caps, but capitalization not needed) * Goes around the edges of EACH face...
+ * The vertices in a point representation must be ordered sequentially, e.g. + * index 3 holds the index of the vertex that replaces vertex 3, i.e. if + * vertex 3 is replaced by vertex 5 then index 3 would contain 5. If no vertex + * replaces it, then it contains the same number as the index itself, e.g. + * index 3 would contain 3. */
Your example has vertex 5 replacing vertex 3, which doesn't seem possible if lower vertex indices always replace higher vertex indices.
On Thu, Jun 23, 2011 at 5:50 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
I'll have to do some more background reading on the kind of mesh data structures used here before I can give qualified comments on your patches, so for now I'll yield to Dylan :-)
I looked over the latest patches, and the code looks good now, but I have a couple of nitpicks for the comments.
Sounds good :-)
+/* ConvertAdjacencyToPointReps helper function.
- Goes around the edges of a face and replaces the vertices in any adjacent
(changed word is in caps, but capitalization not needed) * Goes around the edges of EACH face...
Check.
- The vertices in a point representation must be ordered sequentially, e.g.
- index 3 holds the index of the vertex that replaces vertex 3, i.e. if
- vertex 3 is replaced by vertex 5 then index 3 would contain 5. If no vertex
- replaces it, then it contains the same number as the index itself, e.g.
- index 3 would contain 3. */
Your example has vertex 5 replacing vertex 3, which doesn't seem possible if lower vertex indices always replace higher vertex indices.
You're right it is the other way around. Thanks for reading it thoroughly.
I've change the test a bit. I split up the complex example (mesh6) into two simpler examples. I also got inspired by your ASCII illustrations and have added my own in the comments. The examples have also been updated so that they are easier to draw by hand (except for mesh5).
On Thu, Jun 23, 2011 at 8:18 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
I've change the test a bit. I split up the complex example (mesh6) into two simpler examples. I also got inspired by your ASCII illustrations and have added my own in the comments. The examples have also been updated so that they are easier to draw by hand (except for mesh5).
Looks good.
On Fri, Jun 24, 2011 at 6:12 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Thu, Jun 23, 2011 at 8:18 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
I've change the test a bit. I split up the complex example (mesh6) into two simpler examples. I also got inspired by your ASCII illustrations and have added my own in the comments. The examples have also been updated so that they are easier to draw by hand (except for mesh5).
Looks good.
Great, thanks for the review. I'll sit on them for a little bit more so others can get a chance to comment on them.
I've implemented ConvertPointRepsToAdjacency, but would like some help to make it's worst case performance bearable.
In init_edge_face_map I initialize an array of edge_face structs, and in find_adjacent_face I do a linear search through that array. I would like to replace the array with a hash table, so that the search time becomes constant. I've found a standard list (wine/list.h) and red-black tree implementation (wine/rbtree.h), but not a standard hash table implementation. Is there a standard hash table implementation, should I roll my own or find an LGPL'ed one?
Thanks, Michael Mc Donnell
On 30 June 2011 14:49, Michael Mc Donnell michael@mcdonnell.dk wrote:
In init_edge_face_map I initialize an array of edge_face structs, and in find_adjacent_face I do a linear search through that array. I would like to replace the array with a hash table, so that the search time becomes constant. I've found a standard list (wine/list.h) and red-black tree implementation (wine/rbtree.h), but not a standard hash table implementation. Is there a standard hash table implementation, should I roll my own or find an LGPL'ed one?
I'm not sure if a hash table would be faster and how much, but an easy way to make the lookup cheaper would be to store the edge -> face map as a list for each vertex.
So e.g. if you have these faces: f0: v0, v2, v1 f1: v1, v4, v3 f2: v1, v2, v4 f3: v2, v5, v4
You'd build these lists: v0: {v2:f0} v1: {v0:f0}, {v4:f1}, {v2:f2} v2: {v1:f0}, {v4:f2}, {v5:f3} v3: {v1:f1} v4: {v3:f1}, {v1:f2}, {v2:f3} v5: {v4:f3}
It's then mostly trivial to determine adjacency. This assumes most vertices are only part of a handful of edges, but I don't think that's unreasonable in practice.
We did have a hash table inside wined3d at some point, I suppose you could also try resurrecting that from git history.
On Thu, Jun 30, 2011 at 4:10 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 30 June 2011 14:49, Michael Mc Donnell michael@mcdonnell.dk wrote:
In init_edge_face_map I initialize an array of edge_face structs, and in find_adjacent_face I do a linear search through that array. I would like to replace the array with a hash table, so that the search time becomes constant. I've found a standard list (wine/list.h) and red-black tree implementation (wine/rbtree.h), but not a standard hash table implementation. Is there a standard hash table implementation, should I roll my own or find an LGPL'ed one?
I'm not sure if a hash table would be faster and how much, but an easy way to make the lookup cheaper would be to store the edge -> face map as a list for each vertex.
So e.g. if you have these faces: f0: v0, v2, v1 f1: v1, v4, v3 f2: v1, v2, v4 f3: v2, v5, v4
You'd build these lists: v0: {v2:f0} v1: {v0:f0}, {v4:f1}, {v2:f2} v2: {v1:f0}, {v4:f2}, {v5:f3} v3: {v1:f1} v4: {v3:f1}, {v1:f2}, {v2:f3} v5: {v4:f3}
It's then mostly trivial to determine adjacency. This assumes most vertices are only part of a handful of edges, but I don't think that's unreasonable in practice.
Thanks for your suggestion. I think you're right that it is safe to assume that most vertices will only be a part of a few edges. I'll implement that for now.
We did have a hash table inside wined3d at some point, I suppose you could also try resurrecting that from git history.
I'll stick with your suggestion as it seems easier to implement :-)
Btw I just searched source.winehq.org for hash_table and found several hits in different dlls. I think it would be nice if they could all use the same implementation like list.h and rbtree.h. That way it could also later be used for other parts of wine. Should I add it to http://wiki.winehq.org/JanitorialProjects?
On Thu, Jun 30, 2011 at 6:42 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Thu, Jun 30, 2011 at 4:10 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 30 June 2011 14:49, Michael Mc Donnell michael@mcdonnell.dk wrote:
In init_edge_face_map I initialize an array of edge_face structs, and in find_adjacent_face I do a linear search through that array. I would like to replace the array with a hash table, so that the search time becomes constant. I've found a standard list (wine/list.h) and red-black tree implementation (wine/rbtree.h), but not a standard hash table implementation. Is there a standard hash table implementation, should I roll my own or find an LGPL'ed one?
I'm not sure if a hash table would be faster and how much, but an easy way to make the lookup cheaper would be to store the edge -> face map as a list for each vertex.
...
It's then mostly trivial to determine adjacency. This assumes most vertices are only part of a handful of edges, but I don't think that's unreasonable in practice.
Thanks for your suggestion. I think you're right that it is safe to assume that most vertices will only be a part of a few edges. I'll implement that for now.
I've implemented the look-up scheme that you described.
I have another question about my test. I've basically copied all the test data from my ConvertAdjacencyToPointReps test, and then just inverted the conditions. Should I merge the two tests, put the test data in a separate function, or just ignore the duplication?
On Thursday 30 June 2011 21:50:45 Michael Mc Donnell wrote:
I have another question about my test. I've basically copied all the test data from my ConvertAdjacencyToPointReps test, and then just inverted the conditions. Should I merge the two tests, put the test data in a separate function, or just ignore the duplication?
IMO keeping the tests separated and as simple as possible is more important than avoiding code duplication here, but I don't think we have a policy about this.
On Thu, Jun 30, 2011 at 10:59 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Thursday 30 June 2011 21:50:45 Michael Mc Donnell wrote:
I have another question about my test. I've basically copied all the test data from my ConvertAdjacencyToPointReps test, and then just inverted the conditions. Should I merge the two tests, put the test data in a separate function, or just ignore the duplication?
IMO keeping the tests separated and as simple as possible is more important than avoiding code duplication here, but I don't think we have a policy about this.
Ok, I'll keep what I have for now.
On Thu, Jun 30, 2011 at 3:50 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
I've implemented the look-up scheme that you described.
- if (!point_reps) /* Indentity point reps */
- {
memset(adjacency, -1, 3 * num_faces * sizeof(*adjacency));return D3D_OK;- }
I think you are missing the most common cases, where vertices are reused between triangles. For example:
0--1 | /| |/ | 2--3
If identity point reps are passed to your function it will find the adjacent edge, but if NULL is passed to the function for point reps, then it will find no adjacencies even though semantically they should be the same.
Also, note the spelling error: Indentity -> Identity
+static void free_edge_face_map(struct edge_face_map *edge_face_map) +{
- if (!edge_face_map)
return;- HeapFree(GetProcessHeap(), 0, edge_face_map->lists);
- HeapFree(GetProcessHeap(), 0, edge_face_map->entries);
+}
...
static HRESULT WINAPI ID3DXMeshImpl_ConvertPointRepsToAdjacency(ID3DXMesh *iface, CONST DWORD *point_reps, DWORD *adjacency) {
- struct edge_face_map *edge_face_map = NULL;
...
- free_edge_face_map(edge_face_map);
- iface->lpVtbl->UnlockIndexBuffer(iface);
- return hr;
}
edge_face_map isn't freed. Also, it doesn't need to be allocated on the heap.
- hr = iface->lpVtbl->LockIndexBuffer(iface, 0, &ib_ptr);
The LockIndexBuffer flags could be D3DLOCK_READONLY rather than 0 (which seems to have read-write semantics).
On Fri, Jul 1, 2011 at 7:22 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Thu, Jun 30, 2011 at 3:50 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
I've implemented the look-up scheme that you described.
- if (!point_reps) /* Indentity point reps */
- {
- memset(adjacency, -1, 3 * num_faces * sizeof(*adjacency));
- return D3D_OK;
- }
I think you are missing the most common cases, where vertices are reused between triangles. For example:
0--1 | /| |/ | 2--3
If identity point reps are passed to your function it will find the adjacent edge, but if NULL is passed to the function for point reps, then it will find no adjacencies even though semantically they should be the same.
Ok, I've added that example to the test and fixed the implementation.
Also, note the spelling error: Indentity -> Identity
Check :-)
+static void free_edge_face_map(struct edge_face_map *edge_face_map) +{
- if (!edge_face_map)
- return;
- HeapFree(GetProcessHeap(), 0, edge_face_map->lists);
- HeapFree(GetProcessHeap(), 0, edge_face_map->entries);
+}
...
static HRESULT WINAPI ID3DXMeshImpl_ConvertPointRepsToAdjacency(ID3DXMesh *iface, CONST DWORD *point_reps, DWORD *adjacency) {
- struct edge_face_map *edge_face_map = NULL;
...
- free_edge_face_map(edge_face_map);
- iface->lpVtbl->UnlockIndexBuffer(iface);
- return hr;
}
edge_face_map isn't freed. Also, it doesn't need to be allocated on the heap.
Ok, I've moved it to the stack.
- hr = iface->lpVtbl->LockIndexBuffer(iface, 0, &ib_ptr);
The LockIndexBuffer flags could be D3DLOCK_READONLY rather than 0 (which seems to have read-write semantics).
Fixed.
Thank you for the good review.
On Sat, Jul 2, 2011 at 1:01 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Thank you for the good review.
Glad you appreciate them. Here is another.
+static HRESULT init_edge_face_map(struct edge_face_map *edge_face_map, CONST DWORD *index_buffer, CONST DWORD *point_reps, CONST DWORD num_faces) +{
...
- TRACE("(%p, %p, %p, %d)\n", edge_face_map, index_buffer, point_reps, num_faces);
A TRACE call for an internal initialization function like this seems unnecessary. They are commonly provided for external functions since it captures application behavior.
- if (!point_reps) /* Identity point reps */
- {
id_point_reps = generate_identity_point_reps(num_faces, num_vertices);if (!id_point_reps){hr = E_OUTOFMEMORY;goto cleanup;
...
- hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, &ib_ptr);
- if (FAILED(hr)) goto cleanup;
...
+cleanup:
- HeapFree(GetProcessHeap(), 0, id_point_reps);
- if (indices_are_16_bit) HeapFree(GetProcessHeap(), 0, ib);
- free_edge_face_map(&edge_face_map);
- iface->lpVtbl->UnlockIndexBuffer(iface);
- return hr;
}
The index buffer is unconditionally unlocked in the cleanup code, but goto cleanup can now be called before the index buffer is locked.
+static DWORD *generate_identity_point_reps(DWORD num_faces, DWORD num_vertices) +{
DWORD *id_point_reps;DWORD i;TRACE("(%d, %d)\n", num_faces, num_vertices);id_point_reps = HeapAlloc(GetProcessHeap(), 0, 3 * num_faces * sizeof(*id_point_reps));if (!id_point_reps)return NULL;for (i = 0; i < num_vertices; i++){id_point_reps[i] = i;}for (i = num_vertices; i < 3 * num_faces; i++){id_point_reps[i] = -1;}return id_point_reps;+}
Why are there exra point reps allocated with -1 value? Won't only num_vertices point_reps get used?
On Sun, Jul 3, 2011 at 8:41 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Sat, Jul 2, 2011 at 1:01 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
+static HRESULT init_edge_face_map(struct edge_face_map *edge_face_map, CONST DWORD *index_buffer, CONST DWORD *point_reps, CONST DWORD num_faces) +{
...
- TRACE("(%p, %p, %p, %d)\n", edge_face_map, index_buffer, point_reps, num_faces);
A TRACE call for an internal initialization function like this seems unnecessary. They are commonly provided for external functions since it captures application behavior.
Ok, I've removed them.
- if (!point_reps) /* Identity point reps */
- {
- id_point_reps = generate_identity_point_reps(num_faces, num_vertices);
- if (!id_point_reps)
- {
- hr = E_OUTOFMEMORY;
- goto cleanup;
...
- hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, &ib_ptr);
- if (FAILED(hr)) goto cleanup;
...
+cleanup:
- HeapFree(GetProcessHeap(), 0, id_point_reps);
- if (indices_are_16_bit) HeapFree(GetProcessHeap(), 0, ib);
- free_edge_face_map(&edge_face_map);
- iface->lpVtbl->UnlockIndexBuffer(iface);
- return hr;
}
The index buffer is unconditionally unlocked in the cleanup code, but goto cleanup can now be called before the index buffer is locked.
I've added a check to see if it's locked.
+static DWORD *generate_identity_point_reps(DWORD num_faces, DWORD num_vertices) +{
- DWORD *id_point_reps;
- DWORD i;
- TRACE("(%d, %d)\n", num_faces, num_vertices);
- id_point_reps = HeapAlloc(GetProcessHeap(), 0, 3 * num_faces * sizeof(*id_point_reps));
- if (!id_point_reps)
- return NULL;
- for (i = 0; i < num_vertices; i++)
- {
- id_point_reps[i] = i;
- }
- for (i = num_vertices; i < 3 * num_faces; i++)
- {
- id_point_reps[i] = -1;
- }
- return id_point_reps;
+}
Why are there exra point reps allocated with -1 value? Won't only num_vertices point_reps get used?
Yes that was a mistake. Only num_vertices are used and not 3*num_faces. I have also removed some unnecessary -1 checks.
The latest ConvertPointRepsToAdjacency patches look good.
On Sun, Jul 3, 2011 at 6:25 PM, Dylan Smith dylan.ah.smith@gmail.com wrote:
The latest ConvertPointRepsToAdjacency patches look good.
Good I'll sit on a bit and send ConvertAdjacencyToPointReps to wine-patches in the meantime.
The D3DXWeldVertices function is stubbed in the spec file, so I needed to add it as a real stub. Does this look correct(works here)?
On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote:
The D3DXWeldVertices function is stubbed in the spec file, so I needed to add it as a real stub. Does this look correct(works here)?
Looks good to me. Also check the other d3dx9 libs, they should forward their implementations to d3dx9_36 via the spec file if the function exists in the native version of that dll.
On 6 July 2011 13:32, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote:
The D3DXWeldVertices function is stubbed in the spec file, so I needed to add it as a real stub. Does this look correct(works here)?
Looks good to me. Also check the other d3dx9 libs, they should forward their implementations to d3dx9_36 via the spec file if the function exists in the native version of that dll.
tools/make_specfiles should probably take care of that.
On Wed, Jul 6, 2011 at 1:32 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote:
The D3DXWeldVertices function is stubbed in the spec file, so I needed to add it as a real stub. Does this look correct(works here)?
Looks good to me. Also check the other d3dx9 libs, they should forward their implementations to d3dx9_36 via the spec file if the function exists in the native version of that dll.
I think they're already forwarded. In all the other spec files I find:
@ stdcall D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) d3dx9_36.D3DXWeldVertices
That means they're all forwarded to d3dx9_36?
On Wed, Jul 6, 2011 at 1:54 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Wed, Jul 6, 2011 at 1:32 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Wednesday 06 July 2011 11:04:17 Michael Mc Donnell wrote:
The D3DXWeldVertices function is stubbed in the spec file, so I needed to add it as a real stub. Does this look correct(works here)?
Looks good to me. Also check the other d3dx9 libs, they should forward their implementations to d3dx9_36 via the spec file if the function exists in the native version of that dll.
I think they're already forwarded. In all the other spec files I find:
@ stdcall D3DXWeldVertices(ptr long ptr ptr ptr ptr ptr) d3dx9_36.D3DXWeldVertices
That means they're all forwarded to d3dx9_36?
Btw. I just found a bug in the documentation, so here's the updated version.
Hi Alexandre
Could I get you to commit my ConvertAdjacencyToPointReps patches? They have ID 76054 and 76055 on http://source.winehq.org/patches/
Dylan Smith thinks they look ok.
Thanks, Michael Mc Donnell
On 30 June 2011 18:42, Michael Mc Donnell michael@mcdonnell.dk wrote:
Btw I just searched source.winehq.org for hash_table and found several hits in different dlls. I think it would be nice if they could all use the same implementation like list.h and rbtree.h.
Maybe. It would probably need to be looked at for each case separately.
On Thu, Jun 23, 2011 at 3:30 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Wed, Jun 22, 2011 at 4:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
I'll have to do some more background reading on the kind of mesh data structures used here before I can give qualified comments on your patches, so for now I'll yield to Dylan :-)
I couldn't actually find information on point representative data (aka PointReps), but a quick search found others wondering what it is.
I used chapter 19 of "The Direct3D Graphics Pipeline" by Richard Thomson (his draft book): http://www.xmission.com/~legalize/book/download/19-D3DX%20Mesh%20Objects.pdf
Basically it is an array of vertex indices, one for each vertex, each with either its own index or the index of an co-located vertex. e.g. If you have triangles ABC and DEF (as shown below), with two sets of points co-located, and the vertex buffer stored in alphabetical order. Then the point_reps array would be [0, 1, 2, 2, 1, 5] to indicate that vertices B & C can replace E & D respectively.
A---(B/E) | / | | / | (C/D)--F
Yes that's the basics of it. It does some other funny things too. For example it re-orders the indices, so if you have
0--1 6 3 | / / | | \ | / / | | \ 2 8--7 5--4
That corresponds to:
index buffer = [ 0,1, 2, 6, 7,8, 3, 4,5] adjacency = [-1,1,-1, 2,-1,0, -1,-1,1] point rep = [ 0,1, 2, 1, 4,7, 1, 7,2]
If the indices had not been re-ordered then the point representation would have been [0,1,2, 1,7,2, 1,4,7].
Here is an example that shows how it expands collapsed triangles:
0--1 3 | / / | | / / | 2 4--5
If the triangle 3-4-5 is collapsed by repeating it's first vertex in the index buffer, then it will not be adjacent to 0-1-2:
index buffer = [ 0, 1, 2, 3, 3, 3] adjacency = [-1,-1,-1, -1,-1,-1] point rep = [ 0,1, 2, 3, 4,5]
The vertices of 3-4-5 are included anyway in the point representation, even though they do not form a real triangle.
All these things are tested in mesh6, but I can see now that they probably should be split up into separate test cases.
On Tuesday 21 June 2011 12:32:28 Michael Mc Donnell wrote:
Note that two of the helper functions and a struct in the test were also included in my UpdateSemantics patch. I'll remove them once my UpdateSemantics patch is in the official tree.
Your patches aren't applied yet and http://source.winehq.org/patches/ lists them as "New". I recommend to ask Alexandre why he didn't push them. I guess he is waiting for confirmation from a d3dx9 developer, but I think Dylan already gave his OK, and so did I, but I am strictly speaking not involved in d3dx9.
-
Dylan Smith -
Henri Verbeet -
Michael Mc Donnell -
Stefan Dösinger