Dear Wine developers,
I would like implement some of the missing mesh functions in Wine's D3DX9 for Google Summer of Code 2011. I would like to implement the following functions: - CloneMesh - CloneMeshFVF - ConvertPointRepsToAdjacency - ConvertAdjacencyToPointReps - GenerateAdjacency
They seem to be suitable in size to implement during a summer, and I have a good idea of how to implement them. I expect to first implement tests and then implement the functions.
I am a masters student at the Technical University of Denmark nearing the end of my studies, and I have specialized in 3D computer graphics, studying both real-time and physically based techniques, as well as how to use existing 3D modeling tools to create animations. The mesh functions should be fairly straight forward for me to implement as I have recently taken an advanced course on geometry processing which involved a lot of mesh manipulation. I have, furthermore, already tests (for a shell32 function) in the wine test suite so I have some knowledge of Wine development.
Roderick Coldenbrander is listed as a possible mentor for "Direct3D - Implement missing D3D9_xx DLLs" on the wiki. Are you still interested, or would someone else like to be a mentor?
Regards, Michael Mc Donnell
Hi, Thanks for your proposal!
I don't know the functions you suggest well enough personally to judge the amount of work that is needed. Potential mentors are Roderick, Matteo, Henri and me. I don't know who has time and is willing to mentor, but I think it is safe to say that we'll find somebody :-)
We had two d3dx9-centered gsoc projects in the past, namely Matteo's shader assembler work and Tony Wasserka's texture loading work. Matteo's project succeeded and the code is in Wine now. Tony's code didn't make it in during the project, but at least parts(everything?) of it got committed later.
As you can see we have another developer who's interested in d3dx9. Two projects on the same library are doable if they target independent parts. Luckily d3dx9 is a big collection of mostly independent helper functions.
I don't want to jump to conclusions just yet, the formal application process will decide which proposals are accepted.
Best regards Stefan
On Sunday 20 March 2011 18:09:24 Michael Mc Donnell wrote:
Dear Wine developers,
I would like implement some of the missing mesh functions in Wine's D3DX9 for Google Summer of Code 2011. I would like to implement the following functions:
- CloneMesh
- CloneMeshFVF
- ConvertPointRepsToAdjacency
- ConvertAdjacencyToPointReps
- GenerateAdjacency
They seem to be suitable in size to implement during a summer, and I have a good idea of how to implement them. I expect to first implement tests and then implement the functions.
I am a masters student at the Technical University of Denmark nearing the end of my studies, and I have specialized in 3D computer graphics, studying both real-time and physically based techniques, as well as how to use existing 3D modeling tools to create animations. The mesh functions should be fairly straight forward for me to implement as I have recently taken an advanced course on geometry processing which involved a lot of mesh manipulation. I have, furthermore, already tests (for a shell32 function) in the wine test suite so I have some knowledge of Wine development.
Roderick Coldenbrander is listed as a possible mentor for "Direct3D - Implement missing D3D9_xx DLLs" on the wiki. Are you still interested, or would someone else like to be a mentor?
Regards, Michael Mc Donnell
Thanks for the reply Stefan!
It sounds like it is possible to do this project. I'll send my application to Google on the 28th.
On Sun, Mar 20, 2011 at 11:32 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
Hi, Thanks for your proposal!
I don't know the functions you suggest well enough personally to judge the amount of work that is needed. Potential mentors are Roderick, Matteo, Henri and me. I don't know who has time and is willing to mentor, but I think it is safe to say that we'll find somebody :-)
We had two d3dx9-centered gsoc projects in the past, namely Matteo's shader assembler work and Tony Wasserka's texture loading work. Matteo's project succeeded and the code is in Wine now. Tony's code didn't make it in during the project, but at least parts(everything?) of it got committed later.
As you can see we have another developer who's interested in d3dx9. Two projects on the same library are doable if they target independent parts. Luckily d3dx9 is a big collection of mostly independent helper functions.
I don't want to jump to conclusions just yet, the formal application process will decide which proposals are accepted.
Best regards Stefan
On Sunday 20 March 2011 18:09:24 Michael Mc Donnell wrote:
Dear Wine developers,
I would like implement some of the missing mesh functions in Wine's D3DX9 for Google Summer of Code 2011. I would like to implement the following functions: - CloneMesh - CloneMeshFVF - ConvertPointRepsToAdjacency - ConvertAdjacencyToPointReps - GenerateAdjacency
They seem to be suitable in size to implement during a summer, and I have a good idea of how to implement them. I expect to first implement tests and then implement the functions.
I am a masters student at the Technical University of Denmark nearing the end of my studies, and I have specialized in 3D computer graphics, studying both real-time and physically based techniques, as well as how to use existing 3D modeling tools to create animations. The mesh functions should be fairly straight forward for me to implement as I have recently taken an advanced course on geometry processing which involved a lot of mesh manipulation. I have, furthermore, already tests (for a shell32 function) in the wine test suite so I have some knowledge of Wine development.
Roderick Coldenbrander is listed as a possible mentor for "Direct3D - Implement missing D3D9_xx DLLs" on the wiki. Are you still interested, or would someone else like to be a mentor?
Regards, Michael Mc Donnell
On Sun, Mar 20, 2011 at 1:09 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
I would like implement some of the missing mesh functions in Wine's D3DX9 for Google Summer of Code 2011. I would like to implement the following functions: - CloneMesh - CloneMeshFVF - ConvertPointRepsToAdjacency - ConvertAdjacencyToPointReps - GenerateAdjacency
Hi Michael,
Sorry for not noticing your before, but it seems as if some work that I have been doing as some overlap with your proposed Google Summer of Code project. I have implemented GenerateAdjacency, and partially implemented CloneMesh{,FVF} for the basic case where the vertex formats are exactly the same, but I am still trying to incrementally submit my work to wine-patches.
I have actually been working on the D3DXLoadMeshFromX and D3DXLoadMeshHierarchyFromX functions. However, D3DXLoadMeshHierarchyFromX is given a set of callbacks that is called with the adjacency for the mesh (ID3DXAllocateMeshHierarchy::CreateMeshContainer). It is for this reason that I implemented GenerateAdjacency, since it wasn't possible to detect whether the adjacency information would be used by the user provided callback in order to create a partial implementation.
I also provided a partial implementation of Optimize{,Inplace} since it seemed like the load mesh functions were sorting the faces by the attribute values (i.e. D3DXMESHOPT_ATTRSORT). I had Optimize use CloneMesh with the same vertex format in order to call OptimizeInplace, which does the actual optimization work.
The relevant patches are in my development repository (http://dylansmith.ca/git) in the loadmesh branch. I am trying to send in my patches, but currently I am waiting on the review of my latest patch to generate rmxftmpl.h, which is a re-requisite for load mesh patches. I'll try to avoid stepping on your toes in future.
Summary of ID3DXMesh methods I have implemented: - LockAttributeBuffer - UnlockAttributeBuffer - GetAttributeTable - SetAttributeTable - DrawSubset - GenerateAdjacency
Partially implemented: OptimizeInplace: unimplemented support for D3DXMESHOPT_VERTEXCACHE, D3DXMESHOPT_STRIPREORDER CloneMesh: unimplemented support for vertex buffer conversion
Completely unimplemented: - ConvertPointRepsToAdjacency - ConvertAdjacencyToPointReps - UpdateSemantics
There are also plenty of other functions. Just try `grep stub d3dx9_36.spec | grep Mesh` for Mesh related functions. There are also some unimplemented functions to create meshes for certain shapes (e.g. D3DXCreateTorus, D3DXCreatePolygon).
On Tue, Apr 26, 2011 at 6:34 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Sun, Mar 20, 2011 at 1:09 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
I would like implement some of the missing mesh functions in Wine's D3DX9 for Google Summer of Code 2011. I would like to implement the following functions: - CloneMesh - CloneMeshFVF - ConvertPointRepsToAdjacency - ConvertAdjacencyToPointReps - GenerateAdjacency
Hi Michael,
Sorry for not noticing your before, but it seems as if some work that I have been doing as some overlap with your proposed Google Summer of Code project. I have implemented GenerateAdjacency, and partially implemented CloneMesh{,FVF} for the basic case where the vertex formats are exactly the same, but I am still trying to incrementally submit my work to wine-patches.
That's ok Dylan, better now than in a couple of months :-). I only picked those functions because I had a good idea about how to implement them. I just got accepted into GSoC 2011, so I'll try to find some other mesh functions in D3DX9 to implement.
I'll try to avoid stepping on your toes in future.
Don't worry about it. No offence taken :-)
Completely unimplemented:
- ConvertPointRepsToAdjacency
- ConvertAdjacencyToPointReps
- UpdateSemantics
Ok so I can still work on ConvertPointRepsToAdjacency and ConvertAdjacencyToPointReps?
There are also plenty of other functions. Just try `grep stub d3dx9_36.spec | grep Mesh` for Mesh related functions. There are also some unimplemented functions to create meshes for certain shapes (e.g. D3DXCreateTorus, D3DXCreatePolygon).
Thanks for the suggestions. It seems like Misha Koshelev is working on D3DXCreateTorus, and David Adam is working on D3DXCreatePolygon?
I'll have a look around and see if there are any other good functions. Any other suggestions from anyone are of course welcome.
Cheers, Michael
On Tue, Apr 26, 2011 at 4:15 PM, Michael Mc Donnell michael@mcdonnell.dkwrote:
Completely unimplemented:
- ConvertPointRepsToAdjacency
- ConvertAdjacencyToPointReps
- UpdateSemantics
Ok so I can still work on ConvertPointRepsToAdjacency and ConvertAdjacencyToPointReps?
Yes. I had no plans on working on them.
Nor do I plan on fully implementing CloneMesh, so I'll try to submit what I have in order to let you can build on it.
On Tue, Apr 26, 2011 at 10:30 PM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Tue, Apr 26, 2011 at 4:15 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Completely unimplemented:
- ConvertPointRepsToAdjacency
- ConvertAdjacencyToPointReps
- UpdateSemantics
Ok so I can still work on ConvertPointRepsToAdjacency and ConvertAdjacencyToPointReps?
Yes. I had no plans on working on them. Nor do I plan on fully implementing CloneMesh, so I'll try to submit what I have in order to let you can build on it.
Great I'll work on those first and then plan to improve CloneMesh later in the summer.
My plans have changed a bit to avoid duplication of work. The updated plan is:
Before May 23: * Read up on DirectX in general
Week 1-3 (May 23rd-June 10th): * Implement test for UpdateSemantics function * Implement UpdateSemantics function
Week 4-6 (June 13th-July 1st): * Implement test for ConvertPointRepsToAdjacency function * Implement test for ConvertAdjacencyToPointReps function * Implement ConvertPointRepsToAdjacency function * Implement ConvertAdjacencyToPointReps function
Week 7-9 (July 4th-22th): * Implement test for D3DXWeldVertices * Implement D3DXWeldVertices
Week 10-12 (July 25th-August 12th): * Improve clone function if possible * Improve implemented functions as needed.
Week 13 (Auguest 15th-19th): * Make sure that all code has been integrated in the official tree. * Last code clean-up for code that has not already been accepted.
Hi Stefan
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. That would, however, require quite a bit of re-engineering. Should I submit the current version or wait and try to do an optimized version?
Btw should I roll all or some of the patches up into a single patch?
Any other comments about the patches in general?
Cheers, Michael Mc Donnell
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
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
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 :-)
*) In your first test you forgot to check the HeapAlloc result.
*) You could write a test that passes an invalid D3DVERTEXELEMENT9 array to UpdateSemantics to see how it handles an invalid declaration.
A few style suggestions:
*) memset(mem, 0, size) is preferred over ZeroMemory(mem, size);
*) 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.
*) 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.
Thanks for the lengthy feedback!
On Tue, May 24, 2011 at 3:36 PM, Stefan Dösinger stefandoesinger@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.
On Tuesday 24 May 2011 19:56:06 Michael Mc Donnell wrote:
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.
Is there an alternative? I guess you could cache existing declarations. The d3d9 API doesn't give you an option to reuse an existing declaration.
I doubt apps will regularly change the declaration, usually this doesn't make sense without also changing the data. Of course that doesn't mean there isn't a broken app out there that does this.
I also noticed that I didn't call IDirect3DDevice9_SetVertexDeclaration. I've added it to stick with the defined locking scheme.
Locking scheme? I've just started reading into into the Mesh API, but I don't think you're supposed to apply the declaration until the mesh is used for drawing.
*) In your first test you forgot to check the HeapAlloc result.
Ok, I'll return E_OUTOFMEMORY in that case.
I guess the callers will only abort the tests if NewTestContext fails, so I think the NULL / non-NULL return value was better.
Should I try to make some more invalid D3DVERTEXELEMENT9 arrays to see if I can provoke an error?
I was thinking about something that would make CreateVertexDeclaration fail, e.g. declaring a usage+usage index twice or using an undefined type. dlls/d3d9/vertexdeclaration.c and dlls/wined3d/vertexdeclaration.c check for a few error conditions in their vertexdeclaration_init functions. Just pick one of them(no need to check all of them)
But watch out that you don't open a can of worms here. Our CreateVertexDeclaration probably doesn't catch all error conditions. I recommend that you pick one it does catch, otherwise you'll have to fix d3d9.dll and wined3d.dll too for a small test.
*) 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?
Henri started with this change, I think keeping the namespace clean was a consideration. What you do is up to you and the other devs working on d3dx9. I'm only suggesting that you discuss the code style and agree on a format.
Either way it is a fairly minor point IMO, but it tends to end up in holy war.
On Tue, May 24, 2011 at 8:32 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Tuesday 24 May 2011 19:56:06 Michael Mc Donnell wrote:
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.
Is there an alternative? I guess you could cache existing declarations. The d3d9 API doesn't give you an option to reuse an existing declaration.
I doubt apps will regularly change the declaration, usually this doesn't make sense without also changing the data. Of course that doesn't mean there isn't a broken app out there that does this.
Yeah it might not be a problem at all. I'll give it a quick second look to see if it's possible to cache things. I anyway need to read some more code too to see exactly how the APIs interact.
I also noticed that I didn't call IDirect3DDevice9_SetVertexDeclaration. I've added it to stick with the defined locking scheme.
Locking scheme? I've just started reading into into the Mesh API, but I don't think you're supposed to apply the declaration until the mesh is used for drawing.
Ok good, I started to doubt this last night. I'll remove it and try to get a better sense of how the API works.
*) In your first test you forgot to check the HeapAlloc result.
Ok, I'll return E_OUTOFMEMORY in that case.
I guess the callers will only abort the tests if NewTestContext fails, so I think the NULL / non-NULL return value was better.
Should I try to make some more invalid D3DVERTEXELEMENT9 arrays to see if I can provoke an error?
I was thinking about something that would make CreateVertexDeclaration fail, e.g. declaring a usage+usage index twice or using an undefined type. dlls/d3d9/vertexdeclaration.c and dlls/wined3d/vertexdeclaration.c check for a few error conditions in their vertexdeclaration_init functions. Just pick one of them(no need to check all of them) But watch out that you don't open a can of worms here. Our CreateVertexDeclaration probably doesn't catch all error conditions. I recommend that you pick one it does catch, otherwise you'll have to fix d3d9.dll and wined3d.dll too for a small test.
Ok thanks I'll do that.
*) 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?
Henri started with this change, I think keeping the namespace clean was a consideration. What you do is up to you and the other devs working on d3dx9. I'm only suggesting that you discuss the code style and agree on a format.
Either way it is a fairly minor point IMO, but it tends to end up in holy war.
Ok :-) I don't have any strong feelings about it, so I'll just follow your convention until somebody complains.
On Wed, May 25, 2011 at 9:53 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Tue, May 24, 2011 at 8:32 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Tuesday 24 May 2011 19:56:06 Michael Mc Donnell wrote:
*) In your first test you forgot to check the HeapAlloc result.
Ok, I'll return E_OUTOFMEMORY in that case.
I guess the callers will only abort the tests if NewTestContext fails, so I think the NULL / non-NULL return value was better.
I've reverted those changes, so it's back to NULL / non-NULL. I've also moved it near the top of the file, so that other tests will be able to re-use the test context creation.
Should I try to make some more invalid D3DVERTEXELEMENT9 arrays to see if I can provoke an error?
I was thinking about something that would make CreateVertexDeclaration fail, e.g. declaring a usage+usage index twice or using an undefined type. dlls/d3d9/vertexdeclaration.c and dlls/wined3d/vertexdeclaration.c check for a few error conditions in their vertexdeclaration_init functions. Just pick one of them(no need to check all of them) But watch out that you don't open a can of worms here. Our CreateVertexDeclaration probably doesn't catch all error conditions. I recommend that you pick one it does catch, otherwise you'll have to fix d3d9.dll and wined3d.dll too for a small test.
Ok thanks I'll do that.
I've added some more tests to see if I could make it fail. Microsoft's UpdateSemantics is not very picky. I can't get it to return anything but D3D_OK except for when I pass a null pointer. My implementation follows this behavior except for two cases: it returns E_FAIL when passing an undefined type and when the offset is not 4 byte aligned. This is because the values are checked inside vertexdeclaration_init. So Wine is stricter. I don't think that it is necessarily a problem because applications that pass bogus declarations like those will likely not work anyway. My own tests with a small interactive demo show that in those cases the application will crash with an access violation when it tries to re-draw the scene.
On 05/26/2011 10:33 AM, Michael Mc Donnell wrote:
I've added some more tests to see if I could make it fail. Microsoft's UpdateSemantics is not very picky. I can't get it to return anything but D3D_OK except for when I pass a null pointer. My implementation follows this behavior except for two cases: it returns E_FAIL when passing an undefined type and when the offset is not 4 byte aligned. This is because the values are checked inside vertexdeclaration_init. So Wine is stricter. I don't think that it is necessarily a problem because applications that pass bogus declarations like those will likely not work anyway. My own tests with a small interactive demo show that in those cases the application will crash with an access violation when it tries to re-draw the scene.
I don't know whether it applies here, but if I understand correctly, there have been cases before that Wine must not be stricter than Windows. Tthe reason is that a program may 'depend' on a function crashing (it having an exception caught in that case). In such a situation, Wine's version of the function not crashing would cause a code path being executed that normally never is (causing incorrect behavior even)
Again I don't know whether it is relevant in this cause.
HTH, Joris
On Thu, May 26, 2011 at 1:14 PM, Joris Huizer joris_huizer@yahoo.com wrote:
On 05/26/2011 10:33 AM, Michael Mc Donnell wrote:
I've added some more tests to see if I could make it fail. Microsoft's UpdateSemantics is not very picky. I can't get it to return anything but D3D_OK except for when I pass a null pointer. My implementation follows this behavior except for two cases: it returns E_FAIL when passing an undefined type and when the offset is not 4 byte aligned. This is because the values are checked inside vertexdeclaration_init. So Wine is stricter. I don't think that it is necessarily a problem because applications that pass bogus declarations like those will likely not work anyway. My own tests with a small interactive demo show that in those cases the application will crash with an access violation when it tries to re-draw the scene.
I don't know whether it applies here, but if I understand correctly, there have been cases before that Wine must not be stricter than Windows. Tthe reason is that a program may 'depend' on a function crashing (it having an exception caught in that case). In such a situation, Wine's version of the function not crashing would cause a code path being executed that normally never is (causing incorrect behavior even)
Again I don't know whether it is relevant in this cause.
Yeah I'd prefer it too to be 100% compatible. However, I think it is highly unlikely that any programs depend on an exception being thrown by EndScene in a drawing method. But you never know, some people do crazy things :-) I think the current implementation is good enough as long as there is no evidence of programs depending on that behavior.
The problem is that UpdateSemantics in the d3dx9 library depends on d3d9 and the error code is generated by vertexdeclaration_init in d3d9. That means d3d9 would have to be changed to make it less picky. That might on the other hand break other things that depend on it being picky (I haven't looked at the tests in d3d9).
I thought of managing an array of D3DVERTEXELEMENT9 in ID3DXMeshImpl instead of a IDirect3DVertexDeclaration9, and then have ID3DXMeshImpl_DrawSubset call IDirect3DDevice9_CreateVertexDeclaration just before IDirect3DDevice9_SetVertexDeclaration. But that would not solve the problem as it would just shift the bad call to ID3DXMeshImpl_DrawSubset and not result in an exception in EndScene. It would also result in a lot of extra calls depending on the number of subsets. So I can't see any good way around modifying d3d9, but that might cause other problems as mentioned earlier.
So to recapitulate, I think the best solution is to ignore that little difference until we see programs that depend on it.
Michael Mc Donnell
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 26.05.2011 um 16:27 schrieb Michael Mc Donnell:
Yeah I'd prefer it too to be 100% compatible. However, I think it is highly unlikely that any programs depend on an exception being thrown by EndScene in a drawing method.
What EndScene method is this? The device's methods? I'd be surprised if this method crashed.
But you never know, some people do crazy things :-) I think the current implementation is good enough as long as there is no evidence of programs depending on that behavior.
The problem is that UpdateSemantics in the d3dx9 library depends on d3d9 and the error code is generated by vertexdeclaration_init in d3d9.
Do the d3d9 vertexdeclaration tests pass on this Windows machine?
I thought of managing an array of D3DVERTEXELEMENT9 in ID3DXMeshImpl instead of a IDirect3DVertexDeclaration9, and then have ID3DXMeshImpl_DrawSubset call IDirect3DDevice9_CreateVertexDeclaration just before IDirect3DDevice9_SetVertexDeclaration.
I suspect that this is what native does. This also fits the API design because UpdateSemantics accepts a D3DVERTEXELEMENT9 array and GetDeclaration returns a D3DVERTEXELEMENT9 array. Why did MS name those functions UpdateSemantics and GetDeclaration???
I don't think you'd have extra d3d9 calls by delaying the vdecl creation. Of course you have to keep the vdecl until the next UpdateSemantic call and not recreate it every time you draw.
Here is my stab at D3DXWeldVertices in one big patch. It implements most of the case I could think of except for attribute sorting (which I don't completely understand how works).
I have also attached a test case for attribute sorting as a separate patch. I do not intend to submit that test case because it cannot easily be made into a todo_wine without producing "succeeded inside todo block" messages. It is simply here for reference if someone later wants to implement it. I did not implement attribute sorting because I think it might require some changes to OptimizeInPlace, which I already use for compacting the mesh and building the vertex_remap array.
Hi,
I just started reading this, I'll write more once I am done. Just a quick question: What do you mean with "built in function" in the comment above color_to_vector?
Stefan
On Friday 22 July 2011 12:56:14 Michael Mc Donnell wrote:
Here is my stab at D3DXWeldVertices in one big patch. It implements most of the case I could think of except for attribute sorting (which I don't completely understand how works).
I have also attached a test case for attribute sorting as a separate patch. I do not intend to submit that test case because it cannot easily be made into a todo_wine without producing "succeeded inside todo block" messages. It is simply here for reference if someone later wants to implement it. I did not implement attribute sorting because I think it might require some changes to OptimizeInPlace, which I already use for compacting the mesh and building the vertex_remap array.
On Sun, Jul 24, 2011 at 12:18 AM, Stefan Dösinger stefandoesinger@gmx.at wrote:
I just started reading this, I'll write more once I am done. Just a quick question: What do you mean with "built in function" in the comment above color_to_vector?
I wanted to ask if there was a DirectX function or macro that can convert a D3DCOLOR, which is four bytes in the range 0-255, to four floats in the range 0-1 or a D3DXVECTOR4?
Reading this again I think color_to_vector should probably accept a D3DCOLOR instead of "BYTE color[4]"?
Thanks, Michael
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
A few more comments, with one day delay:
+static float max_abs_diff_vec2(D3DXVECTOR2 *v1, D3DXVECTOR2 *v2) +{
- float diff_x = fabs(v1->x - v2->x);
- float diff_y = fabs(v1->y - v2->y);
- return fmax(diff_x, diff_y);
+}
fabs and fmax return doubles, probably use the float functions fabsf and fmaxf. gcc doesn't bother about the difference, but msvc generates a precision loss warning.
Duplicating the code for 16 and 32 bit indices looks a bit ugly. Maybe you can use inline functions to read and write values from the proper buffer? The other possibility, which the ddraw blitting code uses is to write a sort of template as a macro and then generate both versions, but I don't like that idea.
Wrt the D3DDECLTYPE epsilon, it may be more efficient to scale the epsilon and calculate the diff and comparison as UBYTEs rather than floats. You'll need the diff functions for the other types like *BYTE*, *SHORT* anyway. Also tests would be helpful here, especially how the epsilon is treated in normalized values like D3DCOLOR and UBYTE4N and how it is treated in non-normalized values like UBYTE4.
Wrt the usage/index fields, what happens when a combination that isn't supported by the fixed function pipeline is used, e.g. NORMAL3 or POSITION5? Those are valid in a vertex declaration and can be used with shaders.
In the test:
- int vertex_size_normal = sizeof(struct vertex_normal);
- int vertex_size_blendweight = sizeof(struct vertex_blendweight);
...
You could make those unsigned.
I'm not quite done reading the test yet, I'll send another mail if I find more.
Think about caching pointreps and adjacency? Am 24.07.2011 um 11:57 schrieb Michael Mc Donnell:
On Sun, Jul 24, 2011 at 12:18 AM, Stefan Dösinger stefandoesinger@gmx.at wrote:
I just started reading this, I'll write more once I am done. Just a quick question: What do you mean with "built in function" in the comment above color_to_vector?
I wanted to ask if there was a DirectX function or macro that can convert a D3DCOLOR, which is four bytes in the range 0-255, to four floats in the range 0-1 or a D3DXVECTOR4?
There's none I know of. I vaguely remember that there was a macro to construct D3DCOLORs, but that's not too helpful here.
Reading this again I think color_to_vector should probably accept a D3DCOLOR instead of "BYTE color[4]"?
Yes, it would be cleaner. Also keep UBYTE4N in mind, which is pretty much the same as D3DCOLOR for your purposes. It just has a different component ordering(bgra in D3DCOLOR vs rgba in UBYTE4N if I'm not mistaken) when you're using it for drawing. As far as I understand WeldVertices it doesn't compare two attributes of different types, bit if you do you have to watch out there.
On Mon, Jul 25, 2011 at 4:04 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
A few more comments, with one day delay:
Thanks that was quick!
+static float max_abs_diff_vec2(D3DXVECTOR2 *v1, D3DXVECTOR2 *v2) +{
- float diff_x = fabs(v1->x - v2->x);
- float diff_y = fabs(v1->y - v2->y);
- return fmax(diff_x, diff_y);
+}
fabs and fmax return doubles, probably use the float functions fabsf and fmaxf. gcc doesn't bother about the difference, but msvc generates a precision loss warning.
Ok. I've changed all occurrences of fmax to fmaxf and fabs to fabsf.
Duplicating the code for 16 and 32 bit indices looks a bit ugly. Maybe you can use inline functions to read and write values from the proper buffer? The other possibility, which the ddraw blitting code uses is to write a sort of template as a macro and then generate both versions, but I don't like that idea.
Yes I agree it's ugly. I've added the functions read_ib and write_ib to handle 32 bit indices. Does that look like a good solution to you too?
Wrt the D3DDECLTYPE epsilon, it may be more efficient to scale the epsilon and calculate the diff and comparison as UBYTEs rather than floats. You'll need the diff functions for the other types like *BYTE*, *SHORT* anyway. Also tests would be helpful here, especially how the epsilon is treated in normalized values like D3DCOLOR and UBYTE4N and how it is treated in non-normalized values like UBYTE4.
Ok I'll look into that and get back to you later.
Wrt the usage/index fields, what happens when a combination that isn't supported by the fixed function pipeline is used, e.g. NORMAL3 or POSITION5? Those are valid in a vertex declaration and can be used with shaders.
It works because the code does not check the UsageIndex field, except for differentiating between DIFFUSE and SPECULAR. I've added test 13 that shows this (tested on Windows 7 and Linux).
Btw. looking at the code again I found a small bug introduced during a cleanup. The default in the switch case in get_component_epsilon is to write an ERR. It should also catch and ignore some of the usages that it is not possible to specify an epsilon for. I've put that back.
In the test:
- int vertex_size_normal = sizeof(struct vertex_normal);
- int vertex_size_blendweight = sizeof(struct vertex_blendweight);
...
You could make those unsigned.
Check.
I'm not quite done reading the test yet, I'll send another mail if I find more.
Thanks! I appreciate the feedback.
Think about caching pointreps and adjacency? Am 24.07.2011 um 11:57 schrieb Michael Mc Donnell:
On Sun, Jul 24, 2011 at 12:18 AM, Stefan Dösinger stefandoesinger@gmx.at wrote:
I just started reading this, I'll write more once I am done. Just a quick question: What do you mean with "built in function" in the comment above color_to_vector?
I wanted to ask if there was a DirectX function or macro that can convert a D3DCOLOR, which is four bytes in the range 0-255, to four floats in the range 0-1 or a D3DXVECTOR4?
There's none I know of. I vaguely remember that there was a macro to construct D3DCOLORs, but that's not too helpful here.
Ok. I've removed the comment.
Reading this again I think color_to_vector should probably accept a D3DCOLOR instead of "BYTE color[4]"?
Yes, it would be cleaner. Also keep UBYTE4N in mind, which is pretty much the same as D3DCOLOR for your purposes. It just has a different component ordering(bgra in D3DCOLOR vs rgba in UBYTE4N if I'm not mistaken) when you're using it for drawing. As far as I understand WeldVertices it doesn't compare two attributes of different types, bit if you do you have to watch out there.
I'll keep it as it is for now and have a look into generalizing it to UBYTE4N.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 26.07.2011 um 11:29 schrieb Michael Mc Donnell:
Duplicating the code for 16 and 32 bit indices looks a bit ugly. Maybe you can use inline functions to read and write values from the proper buffer? The other possibility, which the ddraw blitting code uses is to write a sort of template as a macro and then generate both versions, but I don't like that idea.
Yes I agree it's ugly. I've added the functions read_ib and write_ib to handle 32 bit indices. Does that look like a good solution to you too?
Looks OK to me.
Wrt the D3DDECLTYPE epsilon, it may be more efficient to scale the epsilon and calculate the diff and comparison as UBYTEs rather than floats. You'll need the diff functions for the other types like *BYTE*, *SHORT* anyway. Also tests would be helpful here, especially how the epsilon is treated in normalized values like D3DCOLOR and UBYTE4N and how it is treated in non-normalized values like UBYTE4.
Ok I'll look into that and get back to you later.
Wrt the usage/index fields, what happens when a combination that isn't supported by the fixed function pipeline is used, e.g. NORMAL3 or POSITION5? Those are valid in a vertex declaration and can be used with shaders.
It works because the code does not check the UsageIndex field, except for differentiating between DIFFUSE and SPECULAR. I've added test 13 that shows this (tested on Windows 7 and Linux).
You may still get into trouble with something like TEXCOORD10
I'd recommend to change test 13(or add a new one) that tests a TEXCOORD > 7 and COLOR > 1
On Thu, Jul 28, 2011 at 4:01 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
Am 26.07.2011 um 11:29 schrieb Michael Mc Donnell:
Wrt the D3DDECLTYPE epsilon, it may be more efficient to scale the epsilon and calculate the diff and comparison as UBYTEs rather than floats. You'll need the diff functions for the other types like *BYTE*, *SHORT* anyway. Also tests would be helpful here, especially how the epsilon is treated in normalized values like D3DCOLOR and UBYTE4N and how it is treated in non-normalized values like UBYTE4.
Ok I'll look into that and get back to you later.
I've implemented D3DCOLOR welding as UBYTE4N welding as you described. Test 14 improves the color comparison to check that it is correct.
Wrt the usage/index fields, what happens when a combination that isn't supported by the fixed function pipeline is used, e.g. NORMAL3 or POSITION5? Those are valid in a vertex declaration and can be used with shaders.
It works because the code does not check the UsageIndex field, except for differentiating between DIFFUSE and SPECULAR. I've added test 13 that shows this (tested on Windows 7 and Linux).
You may still get into trouble with something like TEXCOORD10
I'd recommend to change test 13(or add a new one) that tests a TEXCOORD > 7 and COLOR > 1
Good catch. I've added test 25 that shows TEXCOORD is capped at 7 so it uses the epsilon for TEXCOORD7 when it welds TEXCOORD10. I've also updated the implementation to cap it at 7.
I've also added tests 25 and 27 for color usage index larger than 1. The usage index is not capped like TEXCOORD > 7. It instead uses the default value of 1e-6f. I've changed the implementation so it uses the default value instead of printing out an error message.
Test 15 to 23 tests welding of UBYTE4N, UBYTE4, SHORT2, SHORT2N, SHORT4, SHORT4N, FLOAT16_2, and FLOAT16_4. I've also improved the implementation to weld these types.
I still haven't figured out how to weld UDEC3 and DEC3N as I'm unsure how to convert three integers to the 3*10-bit + 2-bit format. Do you know any good sources?
On Sat, Jul 30, 2011 at 2:13 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
I still haven't figured out how to weld UDEC3 and DEC3N as I'm unsure how to convert three integers to the 3*10-bit + 2-bit format. Do you know any good sources?
I figured out how to do the conversion using bit-fields. I've updated the patch to include a test and implementation of UDEC3 and DEC3N welding.
On Sunday 31 July 2011 12:02:35 Michael Mc Donnell wrote:
I figured out how to do the conversion using bit-fields. I've updated the patch to include a test and implementation of UDEC3 and DEC3N welding.
Nice, I was just about to reply not to bother too much about it. No Windows driver I know of supports those formats. Wined3d doesn't implement them either.
With the bitfields I'm not sure about stuff like endianess. My gut feeling would be to use bitmasks and shifts to separate a DWORD instead, but bitfields certainly look nicer. Beyond that endianess is a somewhat academic consideration with an API that's available on x86 only. So I'd say keep the bitfields.
d3dx9 has a few of those issues(and similar things like float vs double) that should be good for a search and replace job. I've fixed a few of those issues in wined3d and friends, but I'm sure more are remaining.
On Saturday 30 July 2011 14:13:00 Michael Mc Donnell wrote:
I've implemented D3DCOLOR welding as UBYTE4N welding as you described. Test 14 improves the color comparison to check that it is correct.
+static BOOL weld_short4(void *to, void *from, float epsilon) ... SHORT truncated_epsilon = epsilon;
This will cause a precision loss warning on msvc. Adding a cast should do.
Some nitpicking: struct D3DXWELDEPSILONS uses "FLOAT" not "float". It is advised to stick to the API types. This is quite academic since a "F+static BOOL weld_short4(void *to, void *from, float epsilon) LOAT" is afaik just a typedef for "float", but it helps when porting Wine to different CPUs. I think we had problems with LONG vs long on amd64 because win32 uses 32 bit LONGs or something like that.
I've also added tests 25 and 27 for color usage index larger than 1. The usage index is not capped like TEXCOORD > 7. It instead uses the default value of 1e-6f. I've changed the implementation so it uses the default value instead of printing out an error message.
That's interesting. I wonder what those people in Redmond are smoking :-)
On 07/31/2011 08:12 PM, Stefan Dösinger wrote: <snip>
With the bitfields I'm not sure about stuff like endianess. My gut feeling would be to use bitmasks and shifts to separate a DWORD instead, but bitfields certainly look nicer. Beyond that endianess is a somewhat academic consideration with an API that's available on x86 only. So I'd say keep the bitfields.
<snip>
Sorry to disturb your conversation but the subject is worth discussing.
I'm currently trying to add UDF support on wine based on Steven Wallace work. Quoting the specification : "On the media the UDF structures are stored little endian" as windows API. But wine is not limited on x86 so what's the rule ? Currently, my code is
int i = 1; char *p = (char *)&i; BOOL isBigEndian = p[0]==1;
/* Tag location (Uint32) at offset 12, little-endian */ *offs = (bloc[20+0] & 0xFF) << ( isBigEndian ? 0 : 24); *offs += (bloc[20+1] & 0xFF) << ( isBigEndian ? 8 : 16); *offs += (bloc[20+2] & 0xFF) << ( isBigEndian ? 16 : 8); *offs += (bloc[20+3] & 0xFF) << ( isBigEndian ? 24 : 0);
Is it correct ? Any thoughts ?
Thanks
On Mon, Aug 01, 2011 at 08:31:50AM +0200, GOUJON Alexandre wrote:
Sorry to disturb your conversation but the subject is worth discussing.
I'm currently trying to add UDF support on wine based on Steven Wallace work. Quoting the specification : "On the media the UDF structures are stored little endian" as windows API. But wine is not limited on x86 so what's the rule ? Currently, my code is
int i = 1; char *p = (char *)&i; BOOL isBigEndian = p[0]==1; /* Tag location (Uint32) at offset 12, little-endian */ *offs = (bloc[20+0] & 0xFF) << ( isBigEndian ? 0 : 24); *offs += (bloc[20+1] & 0xFF) << ( isBigEndian ? 8 : 16); *offs += (bloc[20+2] & 0xFF) << ( isBigEndian ? 16 : 8); *offs += (bloc[20+3] & 0xFF) << ( isBigEndian ? 24 : 0);Is it correct ?
Depends what the data you are playing with is.... But it looks a bit dubious to me.
Any thoughts ?
1) You want endianness to be a compile time constant, not run time. 2) If that code is trying to read a 32bit LE value from a disk sector it is wrong. 3) If you care how fast the code runs, don't repeatedly write through a pointer.
If you have 'unsigned char bloc[]' and want to read a 32 bit LE value you can do: value = bloc[20 + 0]; value |= bloc[20 + 1] << 8; value |= bloc[20 + 2] << 16; value |= bloc[20 + 3] << 24; *offs = value; And that is correct on all architectures.
To write a LE value use: buf[0] = value; buf[1] = value >>= 8; buf[2] = value >>= 8; buf[3] = value >>= 8;
(For BE reverse the indexes)
If the item is known to be correctly aligned (or the architecture supports mis-aligned reads) then you can just do (eg): value = *(uint32_t *)(bloc + 20); value = le32toh(value); but you'll have to chase around the OS headers to find how to spell le32toh(). If you define a C struct that matches the data area (packed if it might have misaligned data), the compiler will generate the shifts and masks to read a native word - so you only need the le32toh() macro. (le32toh() is from NetBSD, can't remember what Linux calls it!)
It is also worth noting that some architectures (eg ppc) have instructions for reading and writing memory with byteswap, but no instruction for swapping a register. Newer gccs can be taught how to use these instructions for specific source sequences.
David
On 08/01/2011 09:09 AM, David Laight wrote:
If you have 'unsigned char bloc[]' and want to read a 32 bit LE value you can do: value = bloc[20 + 0]; value |= bloc[20 + 1]<< 8; value |= bloc[20 + 2]<< 16; value |= bloc[20 + 3]<< 24; *offs = value; And that is correct on all architectures.
I have a BYTE[] so I will do that. I was worried about compatibility across architectures but as you pointed out, my code is slow and even wrong. As always, KISS !
To write a LE value use: buf[0] = value; buf[1] = value>>= 8; buf[2] = value>>= 8; buf[3] = value>>= 8;
(For BE reverse the indexes)
Thanks for remembering me this compact syntax.
If the item is known to be correctly aligned (or the architecture supports mis-aligned reads) then you can just do (eg): value = *(uint32_t *)(bloc + 20); value = le32toh(value); but you'll have to chase around the OS headers to find how to spell le32toh().
If I had something like uint128_t (I doubt it will ever exist), then yeah, I would try to avoid 32 lines of code and bit shifting but I prefer the above solution in my case.
If you define a C struct that matches the data area (packed if it might
I thought about it but I only use the value once.
Thanks for all your advices. Will "star" this e-mail if I need it later.
David
On Sun, Jul 31, 2011 at 8:12 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Sunday 31 July 2011 12:02:35 Michael Mc Donnell wrote:
I figured out how to do the conversion using bit-fields. I've updated the patch to include a test and implementation of UDEC3 and DEC3N welding.
Nice, I was just about to reply not to bother too much about it. No Windows driver I know of supports those formats. Wined3d doesn't implement them either.
With the bitfields I'm not sure about stuff like endianess. My gut feeling would be to use bitmasks and shifts to separate a DWORD instead, but bitfields certainly look nicer. Beyond that endianess is a somewhat academic consideration with an API that's available on x86 only. So I'd say keep the bitfields.
Ok good. That'll keep me from the pain of doing bit twiddling on a little endian machine :-)
On Saturday 30 July 2011 14:13:00 Michael Mc Donnell wrote:
I've implemented D3DCOLOR welding as UBYTE4N welding as you described. Test 14 improves the color comparison to check that it is correct.
+static BOOL weld_short4(void *to, void *from, float epsilon) ... SHORT truncated_epsilon = epsilon;
This will cause a precision loss warning on msvc. Adding a cast should do.
Ok. I've also added casts for the other types.
Some nitpicking: struct D3DXWELDEPSILONS uses "FLOAT" not "float". It is advised to stick to the API types. This is quite academic since a "F+static BOOL weld_short4(void *to, void *from, float epsilon) LOAT" is afaik just a typedef for "float", but it helps when porting Wine to different CPUs. I think we had problems with LONG vs long on amd64 because win32 uses 32 bit LONGs or something like that.
Ok I hadn't thought about portability. I've changed float to FLOAT, int to INT and unsigned int to UINT.
I've also added tests 25 and 27 for color usage index larger than 1. The usage index is not capped like TEXCOORD > 7. It instead uses the default value of 1e-6f. I've changed the implementation so it uses the default value instead of printing out an error message.
That's interesting. I wonder what those people in Redmond are smoking :-)
He he yeah I found that quite odd too :-)
Btw. I just noticed I have 1e-6 as a value by itself in get_component_epsilon and as a constant DEFAULT_EPSILON in D3DXWeldVertices. Would it be ok if I move the constant to d3dx9_36_private.h or is that header file meant for something else?
On 1 August 2011 14:08, Michael Mc Donnell michael@mcdonnell.dk wrote:
With the bitfields I'm not sure about stuff like endianess. My gut feeling would be to use bitmasks and shifts to separate a DWORD instead, but bitfields certainly look nicer. Beyond that endianess is a somewhat academic consideration with an API that's available on x86 only. So I'd say keep the bitfields.
Ok good. That'll keep me from the pain of doing bit twiddling on a little endian machine :-)
You can't do that, even on x86. Bitfield memory layout is undefined.
On Tue, Aug 2, 2011 at 2:26 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 1 August 2011 14:08, Michael Mc Donnell michael@mcdonnell.dk wrote:
With the bitfields I'm not sure about stuff like endianess. My gut feeling would be to use bitmasks and shifts to separate a DWORD instead, but bitfields certainly look nicer. Beyond that endianess is a somewhat academic consideration with an API that's available on x86 only. So I'd say keep the bitfields.
Ok good. That'll keep me from the pain of doing bit twiddling on a little endian machine :-)
You can't do that, even on x86. Bitfield memory layout is undefined.
It is *technically* undefined, but all the compilers I have tested it on do the same thing. The little test I've attached has been tested on Visual Studio x86, GCC x86 and x86-64, llvm/clang x86, and suncc on a SPARC-Enterprise-T5220, and they all behave the same way. The raw byte ordering on the SPARC is different because it's big endian, but the bit-fields still work the same way, in that it extracts the correct values. Do you know any common compiler and architecture combinations where it does not work?
I've removed the support for DEC3N and UDEC3 so it doesn't block the patch.
Thanks for the review, Michael
On Wed, Aug 3, 2011 at 10:56 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Tue, Aug 2, 2011 at 2:26 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 1 August 2011 14:08, Michael Mc Donnell michael@mcdonnell.dk wrote:
With the bitfields I'm not sure about stuff like endianess. My gut feeling would be to use bitmasks and shifts to separate a DWORD instead, but bitfields certainly look nicer. Beyond that endianess is a somewhat academic consideration with an API that's available on x86 only. So I'd say keep the bitfields.
Ok good. That'll keep me from the pain of doing bit twiddling on a little endian machine :-)
You can't do that, even on x86. Bitfield memory layout is undefined.
It is *technically* undefined, but all the compilers I have tested it on do the same thing. The little test I've attached has been tested on Visual Studio x86, GCC x86 and x86-64, llvm/clang x86, and suncc on a SPARC-Enterprise-T5220, and they all behave the same way. The raw byte ordering on the SPARC is different because it's big endian, but the bit-fields still work the same way, in that it extracts the correct values. Do you know any common compiler and architecture combinations where it does not work?
I've removed the support for DEC3N and UDEC3 so it doesn't block the patch.
Oops here is the patch where I have actually removed it :-)
On Wednesday 03 August 2011 10:56:25 Michael Mc Donnell wrote:
It is *technically* undefined, but all the compilers I have tested it on do the same thing.
There may be a future compiler that behaves differently. You may get away with it right now, but it will cause pain in the rear sooner or later.
(I didn't know bitfield layout is undefined, otherwise I wouldn't have advised you to keep using them)
On Wed, Aug 3, 2011 at 11:18 AM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Wednesday 03 August 2011 10:56:25 Michael Mc Donnell wrote:
It is *technically* undefined, but all the compilers I have tested it on do the same thing.
There may be a future compiler that behaves differently. You may get away with it right now, but it will cause pain in the rear sooner or later.
(I didn't know bitfield layout is undefined, otherwise I wouldn't have advised you to keep using them)
Looking at the ISO C99 draft I can't find anything about the memory layout being undefined. Only that things are implementation specific like alignment, padding, and endianness. Not much unlike the definition of int :-)
Anyway you're right it's probably a good idea to stay clear of them in order to avoid potential compiler issues. I've updated the patch to implement UDEC3 and UDEC3N using shifts and masks instead of bit-fields. It will still only work on little-endian machines though. Does it look ok?
Thanks, Michael
On Wed, Aug 3, 2011 at 2:39 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Anyway you're right it's probably a good idea to stay clear of them in order to avoid potential compiler issues. I've updated the patch to implement UDEC3 and UDEC3N using shifts and masks instead of bit-fields. It will still only work on little-endian machines though. Does it look ok?
I've rebased my D3DXWeldVertices patch. Git was having trouble applying it after Alexandre committed my ConvertPointRepsToAdjacency.
On Thu, Aug 4, 2011 at 8:48 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Wed, Aug 3, 2011 at 2:39 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Anyway you're right it's probably a good idea to stay clear of them in order to avoid potential compiler issues. I've updated the patch to implement UDEC3 and UDEC3N using shifts and masks instead of bit-fields. It will still only work on little-endian machines though. Does it look ok?
I've rebased my D3DXWeldVertices patch. Git was having trouble applying it after Alexandre committed my ConvertPointRepsToAdjacency.
Sorry for the noise. I accidentally moved some of the code around during the rebase merge. I've moved things back in the original order.
On Thu, Aug 4, 2011 at 10:27 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Thu, Aug 4, 2011 at 8:48 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Wed, Aug 3, 2011 at 2:39 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Anyway you're right it's probably a good idea to stay clear of them in order to avoid potential compiler issues. I've updated the patch to implement UDEC3 and UDEC3N using shifts and masks instead of bit-fields. It will still only work on little-endian machines though. Does it look ok?
I've rebased my D3DXWeldVertices patch. Git was having trouble applying it after Alexandre committed my ConvertPointRepsToAdjacency.
Sorry for the noise. I accidentally moved some of the code around during the rebase merge. I've moved things back in the original order.
Here's yet another update for D3DXWeldVertices. I'm working on writing a test for CloneMesh and re-using some of the parts from the D3DXWeldVertices test, which is why I keep finding these small bugs :-)
In the test I had forgotten to replace all fabs with fabsf. I've also changed the comparison test to find the maximum absolute difference for FLOAT1-3. I've finally added a comparison test for FLOAT4.
Hi,
A few more thinks I noticed:
On Thursday 04 August 2011 11:21:22 Michael Mc Donnell wrote:
+#include <float.h>
In other libs this is guarded with #ifdef HAVE_FLOAT_H - not sure if there are any systems that don't have the header and the rest of the code still compiles, but I recommend to use the same.
+static BOOL weld_float1(void *to, void *from, FLOAT epsilon)
- FLOAT *v1 = to;
- FLOAT *v2 = from;
...
memcpy(to, from, sizeof(FLOAT));
A *v1 = *v2 assignment looks nicer.
Similarly:
+static BOOL weld_float2(void *to, void *from, FLOAT epsilon)
- D3DXVECTOR2 *v1 = to;
- D3DXVECTOR2 *v2 = from;
...
memcpy(to, from, 2 * sizeof(FLOAT));
Either assign v1->x = v2->x and v1->y = v2->y, or use sizeof(D3DXVECTOR2) in the memcpy. Similarly in all the other functions.
+static BOOL weld_ubyte4(void *to, void *from, FLOAT epsilon) +{
- BYTE *b1 = to;
- BYTE *b2 = from;
I think there's a UBYTE type, or maybe CHAR that is typedefed to unsigned char.
+static BOOL weld_component(void *to, void *from, D3DDECLTYPE type, FLOAT
epsilon)
+{
- switch (type)
- {
...
case D3DDECLTYPE_UNUSED:FIXME("D3DDECLTYPE_UNUSED welding not implemented.\n");break;- }
- return FALSE;
+}
I'd guess that UNUSED is not valid to create a vdecl in the first place. Since the Mesh API uses an attribute array rather than a IDirect3DVertexDeclaration9 interface it's probably worth a test(if you don't have one in the updateSemantics tests already). Either way since you have the FIXME you should probably make sure it is printed for any unrecognized type number.
case D3DDECLUSAGE_BLENDINDICES:case D3DDECLUSAGE_POSITIONT:case D3DDECLUSAGE_FOG:case D3DDECLUSAGE_DEPTH:case D3DDECLUSAGE_SAMPLE:/* Not possible to weld five above usages. */break;
I'm not sure what you mean by "not possible", I haven't spotted any of these usages in the tests. But I got a bit lost in the tests, see below.
From the tests:
+static void check_vertex_components(int line, int mesh_number, int
vertex_number, BYTE *got_ptr, const BYTE *exp_ptr, D3DVERTEXELEMENT9 *declaration)
...
FLOAT got = *(got_ptr + decl_ptr->Offset);FLOAT exp = *(exp_ptr + decl_ptr->Offset);
I think you need some pointer casts here before dereferencing. But since no test uses FLOAT1 this is essentially dead code.
Overall the test is pretty hard to read because of the amount of code that is essentially repeated over and over with subtle differences. I don't really see a way around this. You could move some code around, but that won't really change things a lot. I guess we can live with it, it's only the tests after all.
Cheers, Stefan
On Thursday 04 August 2011 11:21:22 Michael Mc Donnell wrote:
On Thu, Aug 4, 2011 at 10:27 AM, Michael Mc Donnell
michael@mcdonnell.dk wrote:
On Thu, Aug 4, 2011 at 8:48 AM, Michael Mc Donnell michael@mcdonnell.dk
wrote:
On Wed, Aug 3, 2011 at 2:39 PM, Michael Mc Donnell michael@mcdonnell.dk
wrote:
Anyway you're right it's probably a good idea to stay clear of them in order to avoid potential compiler issues. I've updated the patch to implement UDEC3 and UDEC3N using shifts and masks instead of bit-fields. It will still only work on little-endian machines though. Does it look ok?
I've rebased my D3DXWeldVertices patch. Git was having trouble applying it after Alexandre committed my ConvertPointRepsToAdjacency.
Sorry for the noise. I accidentally moved some of the code around during the rebase merge. I've moved things back in the original order.
Here's yet another update for D3DXWeldVertices. I'm working on writing a test for CloneMesh and re-using some of the parts from the D3DXWeldVertices test, which is why I keep finding these small bugs
:-)
In the test I had forgotten to replace all fabs with fabsf. I've also changed the comparison test to find the maximum absolute difference for FLOAT1-3. I've finally added a comparison test for FLOAT4.
On Fri, Aug 5, 2011 at 7:01 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Thursday 04 August 2011 11:21:22 Michael Mc Donnell wrote:
+#include <float.h>
In other libs this is guarded with #ifdef HAVE_FLOAT_H - not sure if there are any systems that don't have the header and the rest of the code still compiles, but I recommend to use the same.
Ok, I've added this guard:
#ifdef HAVE_FLOAT_H # include <float.h> #endif
+static BOOL weld_float1(void *to, void *from, FLOAT epsilon)
- FLOAT *v1 = to;
- FLOAT *v2 = from;
...
- memcpy(to, from, sizeof(FLOAT));
A *v1 = *v2 assignment looks nicer.
I've changed it to an assignment as you suggested.
Similarly:
+static BOOL weld_float2(void *to, void *from, FLOAT epsilon)
- D3DXVECTOR2 *v1 = to;
- D3DXVECTOR2 *v2 = from;
...
- memcpy(to, from, 2 * sizeof(FLOAT));
Either assign v1->x = v2->x and v1->y = v2->y, or use sizeof(D3DXVECTOR2) in the memcpy. Similarly in all the other functions.
I've changed them to use sizeof(D3DXVECTORn) together with the memcpy.
+static BOOL weld_ubyte4(void *to, void *from, FLOAT epsilon) +{
- BYTE *b1 = to;
- BYTE *b2 = from;
I think there's a UBYTE type, or maybe CHAR that is typedefed to unsigned char.
I can't find any definitions of UBYTE except for one in dlls/itss/lzx.c. The BYTE is already defined [1] as a being unsigned so isn't that ok?
[1] http://msdn.microsoft.com/en-us/library/aa505945.aspx
Btw. I just noticed I take the abs of an unsigned value which doesn't make sense. I've changed it to this instead:
BYTE diff_x = b1[0] > b2[0] ? b1[0] - b2[0] : b2[0] - b1[0]; ...
I've also fixed it for the other unsigned types.
+static BOOL weld_component(void *to, void *from, D3DDECLTYPE type, FLOAT
epsilon)
+{
- switch (type)
- {
...
- case D3DDECLTYPE_UNUSED:
- FIXME("D3DDECLTYPE_UNUSED welding not implemented.\n");
- break;
- }
- return FALSE;
+}
I'd guess that UNUSED is not valid to create a vdecl in the first place. Since the Mesh API uses an attribute array rather than a IDirect3DVertexDeclaration9 interface it's probably worth a test(if you don't have one in the updateSemantics tests already). Either way since you have the FIXME you should probably make sure it is printed for any unrecognized type number.
I have no idea how to test this, so I'll leave the FIXME for D3DDECLTYPE_UNUSED. I've also added a FIXME for any unrecognized type number.
- case D3DDECLUSAGE_BLENDINDICES:
- case D3DDECLUSAGE_POSITIONT:
- case D3DDECLUSAGE_FOG:
- case D3DDECLUSAGE_DEPTH:
- case D3DDECLUSAGE_SAMPLE:
- /* Not possible to weld five above usages. */
- break;
I'm not sure what you mean by "not possible", I haven't spotted any of these usages in the tests. But I got a bit lost in the tests, see below.
Yeah that was a bad wording. It's not possible to specify an epsilon for those usages. I don't know whether they can be welded or not, so I've changed them to FIXMEs instead. I should probably add a test at some point.
Btw. those FIXMEs might produce a lot of messages (one per vertex). Is there good way to limit the number of messages?
From the tests:
+static void check_vertex_components(int line, int mesh_number, int
vertex_number, BYTE *got_ptr, const BYTE *exp_ptr, D3DVERTEXELEMENT9 *declaration)
...
- FLOAT got = *(got_ptr + decl_ptr->Offset);
- FLOAT exp = *(exp_ptr + decl_ptr->Offset);
I think you need some pointer casts here before dereferencing. But since no test uses FLOAT1 this is essentially dead code.
Test 10 uses FLOAT1. I've changed the comment to better explain that. I've also changed the comparison so that it uses the pointers just like the comparisons below it.
Overall the test is pretty hard to read because of the amount of code that is essentially repeated over and over with subtle differences. I don't really see a way around this. You could move some code around, but that won't really change things a lot. I guess we can live with it, it's only the tests after all.
Yeah I don't see any good way either. Usually when I do unit testing for other things I use one function per test, and often one file per method, but that seems not to be the way it's done in Wine.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 06.08.2011 um 15:24 schrieb Michael Mc Donnell:
I can't find any definitions of UBYTE except for one in dlls/itss/lzx.c. The BYTE is already defined [1] as a being unsigned so isn't that ok?
Oh, I thought BYTE is signed, and I thought I used UBYTE once. I guess I was mistaken then. So BYTE is OK
Btw. those FIXMEs might produce a lot of messages (one per vertex). Is there good way to limit the number of messages?
A common way is something like this
{ static BOOL once = false; if (!once) { FIXME("Report this bug to my aunt Tilda please\n"); once = TRUE; } }
I think there was once a discussion about a FIXME_ONCE macro that took care of this. not sure what happened with that idea.
On Sat, Aug 6, 2011 at 7:59 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
Btw. those FIXMEs might produce a lot of messages (one per vertex). Is there good way to limit the number of messages?
A common way is something like this
{ static BOOL once = false; if (!once) { FIXME("Report this bug to my aunt Tilda please\n"); once = TRUE; } }
I think there was once a discussion about a FIXME_ONCE macro that took care of this. not sure what happened with that idea.
I've sorta followed Dan's suggestion to use Alexandre's style, except that I have multiple fixme_once variables. Any idea if this would be acceptable?
Another solution could be to check the vertex declaration before running the welding algorithm and print the FIXMEs there. But that would decouple the location of where the problem is from the printout, which could be confusing during debugging.
Hi Stefan
I've added some small fixes and a improvements to the D3DXWeldVertices test. These fixes are ports of things I found during my work on the CloneMesh test:
* Fixed ok() for FLOAT1 in check_vertex_components. * Added USHORT2N and USHORT4N comparison in test. * Fixed udec3 and dec3n struct to DWORD conversion in test. * Improved init_test_mesh function to support null arguments.
Do you think the patch is ready for wine-patches?
Cheers, Michael Mc Donnell
On 08/03/2011 05:18 AM, Stefan Dösinger wrote:
On Wednesday 03 August 2011 10:56:25 Michael Mc Donnell wrote:
It is *technically* undefined, but all the compilers I have tested it on do the same thing.
There may be a future compiler that behaves differently. You may get away with it right now, but it will cause pain in the rear sooner or later.
(I didn't know bitfield layout is undefined, otherwise I wouldn't have advised you to keep using them)
Technically they are 'implementation defined', not 'undefined', which means each compiler has to define them but different compilers may define them differently. Because different compilers DO define them differently, they are not portable. Since they are not portable, wine should not use them without cloaking them in macros that compensate for the differences. Which is a PITA. Has anybody done the cloaks already?
Max
Well yes, it's implementation defined, not undefined. The point is that there isn't necessarily any relation to endianness. Just use shifts and masks.
On 08/03/2011 09:28 AM, Henri Verbeet wrote:
Well yes, it's implementation defined, not undefined. The point is that there isn't necessarily any relation to endianness. Just use shifts and masks.
Correct. It is a different issue from endedness.
But you can use configurable wrappers to cloak the compiler differences:
Meta procedure:
1) Write a test to determine which order bit fields are assigned in:
int main(){ union field_order { int an_int; int a_bit:1 } test; test.an_int=0; test.a_bit=1; return test.an_int == 1; }
which returns 0 if bit fields are allocated most significant bits first, 1 otherwise.
2) Add a test to configure that checks the field allocation order. Record the result of the test as LO_ORDER_FIRST or something.
3) Write some macros that hide the declaration order:
#if LO_ORDER_FIRST #define BIT_FIELDS_2(a,b) a;b; #define BIT_FIELDS_3(a,b,c) a;b;c; #define BIT_FIELDS_4(a,b,c,d) a;b;c;d; ... #else #define BIT_FIELDS_2(a,b) b;a; #define BIT_FIELDS_3(a,b,c) c;b;a; #define BIT_FIELDS_4(a,b,c,d) d;c;b;a; ... #endif
4) Constraint: The combined fields must have the same number of bits as the underlying data type. For example, if 'int' has 32 bits, the total size of all the fields in the group must be 32 bits.
THIS HAS PROBABLY BEEN DONE ALREADY!, you just have to dig it out of the list of available configure tests.
max
(I'm in the middle of something else, otherwise, I'd dig it out myself...)
I'm sure there's all kinds of macro abuse you can do to make it work in a somewhat portable way, but I don't think it's worth it.
On Thu, Aug 4, 2011 at 2:35 PM, Henri Verbeet hverbeet@gmail.com wrote:
I'm sure there's all kinds of macro abuse you can do to make it work in a somewhat portable way, but I don't think it's worth it.
Thanks for the explanation Max, but I agree with Henri that it's not worth the trouble for me. I've already implemented the conversion I needed using shifts and masks.
On Thu, Aug 04, 2011 at 08:30:41AM -0400, max wrote:
On 08/03/2011 09:28 AM, Henri Verbeet wrote:
Well yes, it's implementation defined, not undefined. The point is that there isn't necessarily any relation to endianness. Just use shifts and masks.
...
union field_order { int an_int; int a_bit:1 } test; test.an_int=0; test.a_bit=1;
...
There are 4 likely values for test.an_int (1, 0x80, 0x80000000, 0x01000000), and all might be generated regardless of the system endianness.
David
On 08/04/2011 12:36 PM, David Laight wrote:
On Thu, Aug 04, 2011 at 08:30:41AM -0400, max wrote:
On 08/03/2011 09:28 AM, Henri Verbeet wrote:
Well yes, it's implementation defined, not undefined. The point is that there isn't necessarily any relation to endianness. Just use shifts and masks.
...
union field_order { int an_int; int a_bit:1 } test; test.an_int=0; test.a_bit=1;...
There are 4 likely values for test.an_int (1, 0x80, 0x80000000, 0x01000000), and all might be generated regardless of the system endianness.
David
Hmm... 0x80 and 0x01000000 would be very hard to handle. So hard in fact that I would consider any implementation that produced them to be broken, but you're right, THWI!
Max
The error message you have got because you are trying to run a corrupted or missing d3dx9.dll file. Windows displays the above message when system unable to run d3dx9.dll file. To fix the issue, you can restore it. Here is the link to download d3dx9.dll file.
After download the file, restore it to your System32 folder. That should fix the issue.
-- View this message in context: http://wine.1045685.n5.nabble.com/GSoC-2011-Implement-Missing-Mesh-Functions... Sent from the Wine - Devel mailing list archive at Nabble.com.
-
David Laight -
Dylan Smith -
GOUJON Alexandre -
Henri Verbeet -
Joris Huizer -
max -
Michael Mc Donnell -
sa.anderson009 -
Stefan Dösinger