Thanks for the comments Matteo! I have a few questions below.
On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/29 Michael Mc Donnell michael@mcdonnell.dk:
On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Saturday 28 May 2011 19:55:55 you wrote:
It was implemented by setting the vertex declaration to null when the declaration is invalid. That seems to match what happens on Windows, because if a program has set an invalid declaration and then calls GetDeclaration or DrawSubset it will fail with an access violation. Should Wine also fail with an access violation or should it be slightly more graceful and return E_FAIL instead (which my new patch does)?
Yes, I think returning E_FAIL is a good idea, but write a WARN if CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not be shown by default(unlike a FIXME), it is used when the app does something fishy but you think you handled it correctly.
- if (FAILED(hr))
- new_vertex_declaration = NULL;
- /* Free old vertex declaration */
- if (This->vertex_declaration)
- IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
In this situation you can just release the old decl before calling CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. That simplified the code a bit. Don't forget to set This->vertex_declaration to NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration.
- /* Set the position type to color instead of float3 (invalid
declaration) */
- hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, "
- "got %#x expected D3D_OK\n", hr);
Is this really invalid? I don't know it on top of my head, but in general types and usage are independent, especially with shaders. I don't see a check in d3d9 or wined3d for this case.
You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests.
Beyond that I think the patches look OK. I'd recommend to send them to wine- devel one more time and then next week to wine-patches. Henri is on Vacation this week, and he's usually more picky than I am. I also hope some of the people who regularly contribute to d3dx9 will have a look and comment.
Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
The patches are OK with me, just some nitpicking:
That sounds great :-)
- testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead.
Ok, I guess that's better in case the type of the variable changes at some point?
I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
Is it too much or too little whitespace? Could you give me an example?
Thanks!
2011/5/30 Michael Mc Donnell michael@mcdonnell.dk:
Thanks for the comments Matteo! I have a few questions below.
On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/29 Michael Mc Donnell michael@mcdonnell.dk:
On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Saturday 28 May 2011 19:55:55 you wrote:
It was implemented by setting the vertex declaration to null when the declaration is invalid. That seems to match what happens on Windows, because if a program has set an invalid declaration and then calls GetDeclaration or DrawSubset it will fail with an access violation. Should Wine also fail with an access violation or should it be slightly more graceful and return E_FAIL instead (which my new patch does)?
Yes, I think returning E_FAIL is a good idea, but write a WARN if CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not be shown by default(unlike a FIXME), it is used when the app does something fishy but you think you handled it correctly.
- if (FAILED(hr))
- new_vertex_declaration = NULL;
- /* Free old vertex declaration */
- if (This->vertex_declaration)
- IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
In this situation you can just release the old decl before calling CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. That simplified the code a bit. Don't forget to set This->vertex_declaration to NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration.
- /* Set the position type to color instead of float3 (invalid
declaration) */
- hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, "
- "got %#x expected D3D_OK\n", hr);
Is this really invalid? I don't know it on top of my head, but in general types and usage are independent, especially with shaders. I don't see a check in d3d9 or wined3d for this case.
You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests.
Beyond that I think the patches look OK. I'd recommend to send them to wine- devel one more time and then next week to wine-patches. Henri is on Vacation this week, and he's usually more picky than I am. I also hope some of the people who regularly contribute to d3dx9 will have a look and comment.
Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
The patches are OK with me, just some nitpicking:
That sounds great :-)
- testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead.
Ok, I guess that's better in case the type of the variable changes at some point?
Yes, that's a reason. The other one is simply consistency.
I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
Is it too much or too little whitespace? Could you give me an example?
+ todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
+ "got %#x expected D3D_OK\n", hr);
Here you put 4 spaces on the line continuation, which is neither the "align to be below the '(' + 1 space" used in most of the file, nor the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a nice and consistent coding style (I'm sure you noticed that), so that's mostly trying to not mix even more style variants... There was also something wrong with spaces in a comment that caught my eye, I think.
I told you I was nitpicking... :)
Thanks!
On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/30 Michael Mc Donnell michael@mcdonnell.dk:
Thanks for the comments Matteo! I have a few questions below.
On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/29 Michael Mc Donnell michael@mcdonnell.dk:
On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Saturday 28 May 2011 19:55:55 you wrote:
It was implemented by setting the vertex declaration to null when the declaration is invalid. That seems to match what happens on Windows, because if a program has set an invalid declaration and then calls GetDeclaration or DrawSubset it will fail with an access violation. Should Wine also fail with an access violation or should it be slightly more graceful and return E_FAIL instead (which my new patch does)?
Yes, I think returning E_FAIL is a good idea, but write a WARN if CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not be shown by default(unlike a FIXME), it is used when the app does something fishy but you think you handled it correctly.
- if (FAILED(hr))
- new_vertex_declaration = NULL;
- /* Free old vertex declaration */
- if (This->vertex_declaration)
- IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
In this situation you can just release the old decl before calling CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. That simplified the code a bit. Don't forget to set This->vertex_declaration to NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration.
- /* Set the position type to color instead of float3 (invalid
declaration) */
- hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, "
- "got %#x expected D3D_OK\n", hr);
Is this really invalid? I don't know it on top of my head, but in general types and usage are independent, especially with shaders. I don't see a check in d3d9 or wined3d for this case.
You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests.
Beyond that I think the patches look OK. I'd recommend to send them to wine- devel one more time and then next week to wine-patches. Henri is on Vacation this week, and he's usually more picky than I am. I also hope some of the people who regularly contribute to d3dx9 will have a look and comment.
Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
The patches are OK with me, just some nitpicking:
That sounds great :-)
- testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead.
Ok, I guess that's better in case the type of the variable changes at some point?
Yes, that's a reason. The other one is simply consistency.
Ok, I've changed that.
I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
Is it too much or too little whitespace? Could you give me an example?
todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
"got %#x expected D3D_OK\n", hr);
Here you put 4 spaces on the line continuation, which is neither the "align to be below the '(' + 1 space" used in most of the file, nor the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a nice and consistent coding style (I'm sure you noticed that), so that's mostly trying to not mix even more style variants...
Ok I've changed it to the "align to be below the '(' + 1 space" convention.
There was also something wrong with spaces in a comment that caught my eye, I think.
The only thing that stood out to me was that I had extra newlines in my multi-line comments. I've removed those.
I told you I was nitpicking... :)
That's ok, I prefer consistency too :-)
On Tue, May 31, 2011 at 12:51 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/30 Michael Mc Donnell michael@mcdonnell.dk:
Thanks for the comments Matteo! I have a few questions below.
On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/29 Michael Mc Donnell michael@mcdonnell.dk:
On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Saturday 28 May 2011 19:55:55 you wrote:
> It was implemented by setting the vertex declaration to null when the > declaration is invalid. That seems to match what happens on Windows, > because if a program has set an invalid declaration and then calls > GetDeclaration or DrawSubset it will fail with an access violation. > Should Wine also fail with an access violation or should it be > slightly more graceful and return E_FAIL instead (which my new patch > does)? Yes, I think returning E_FAIL is a good idea, but write a WARN if CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not be shown by default(unlike a FIXME), it is used when the app does something fishy but you think you handled it correctly.
> + if (FAILED(hr)) > + new_vertex_declaration = NULL; > + > + /* Free old vertex declaration */ > + if (This->vertex_declaration) > + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); In this situation you can just release the old decl before calling CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. That simplified the code a bit. Don't forget to set This->vertex_declaration to NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration.
> + /* Set the position type to color instead of float3 (invalid declaration) */ > + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); > + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " > + "got %#x expected D3D_OK\n", hr); Is this really invalid? I don't know it on top of my head, but in general types and usage are independent, especially with shaders. I don't see a check in d3d9 or wined3d for this case.
You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests.
Beyond that I think the patches look OK. I'd recommend to send them to wine- devel one more time and then next week to wine-patches. Henri is on Vacation this week, and he's usually more picky than I am. I also hope some of the people who regularly contribute to d3dx9 will have a look and comment.
Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
The patches are OK with me, just some nitpicking:
That sounds great :-)
- testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead.
Ok, I guess that's better in case the type of the variable changes at some point?
Yes, that's a reason. The other one is simply consistency.
Ok, I've changed that.
I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
Is it too much or too little whitespace? Could you give me an example?
todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
"got %#x expected D3D_OK\n", hr);
Here you put 4 spaces on the line continuation, which is neither the "align to be below the '(' + 1 space" used in most of the file, nor the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a nice and consistent coding style (I'm sure you noticed that), so that's mostly trying to not mix even more style variants...
Ok I've changed it to the "align to be below the '(' + 1 space" convention.
There was also something wrong with spaces in a comment that caught my eye, I think.
The only thing that stood out to me was that I had extra newlines in my multi-line comments. I've removed those.
I told you I was nitpicking... :)
That's ok, I prefer consistency too :-)
Oops the patches I sent earlier had a tab character and some superfluous whitespace.
On Tue, May 31, 2011 at 2:30 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Tue, May 31, 2011 at 12:51 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/30 Michael Mc Donnell michael@mcdonnell.dk:
Thanks for the comments Matteo! I have a few questions below.
On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/29 Michael Mc Donnell michael@mcdonnell.dk:
On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote: > On Saturday 28 May 2011 19:55:55 you wrote: > >> It was implemented by setting the vertex declaration to null when the >> declaration is invalid. That seems to match what happens on Windows, >> because if a program has set an invalid declaration and then calls >> GetDeclaration or DrawSubset it will fail with an access violation. >> Should Wine also fail with an access violation or should it be >> slightly more graceful and return E_FAIL instead (which my new patch >> does)? > Yes, I think returning E_FAIL is a good idea, but write a WARN if > CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not > be shown by default(unlike a FIXME), it is used when the app does something > fishy but you think you handled it correctly. > >> + if (FAILED(hr)) >> + new_vertex_declaration = NULL; >> + >> + /* Free old vertex declaration */ >> + if (This->vertex_declaration) >> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); > In this situation you can just release the old decl before calling > CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. > That simplified the code a bit. Don't forget to set This->vertex_declaration to > NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration.
>> + /* Set the position type to color instead of float3 (invalid > declaration) */ >> + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); >> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >> + "got %#x expected D3D_OK\n", hr); > Is this really invalid? I don't know it on top of my head, but in general > types and usage are independent, especially with shaders. I don't see a check > in d3d9 or wined3d for this case.
You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests.
> Beyond that I think the patches look OK. I'd recommend to send them to wine- > devel one more time and then next week to wine-patches. Henri is on Vacation > this week, and he's usually more picky than I am. I also hope some of the > people who regularly contribute to d3dx9 will have a look and comment.
Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
The patches are OK with me, just some nitpicking:
That sounds great :-)
- testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead.
Ok, I guess that's better in case the type of the variable changes at some point?
Yes, that's a reason. The other one is simply consistency.
Ok, I've changed that.
I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
Is it too much or too little whitespace? Could you give me an example?
todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
"got %#x expected D3D_OK\n", hr);
Here you put 4 spaces on the line continuation, which is neither the "align to be below the '(' + 1 space" used in most of the file, nor the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a nice and consistent coding style (I'm sure you noticed that), so that's mostly trying to not mix even more style variants...
Ok I've changed it to the "align to be below the '(' + 1 space" convention.
There was also something wrong with spaces in a comment that caught my eye, I think.
The only thing that stood out to me was that I had extra newlines in my multi-line comments. I've removed those.
I told you I was nitpicking... :)
That's ok, I prefer consistency too :-)
Oops the patches I sent earlier had a tab character and some superfluous whitespace.
Henri, do you have any comments about my UpdateSemantics patches before I send them off to wine-patches?
Thanks
On 8 June 2011 11:33, Michael Mc Donnell michael@mcdonnell.dk wrote:
Henri, do you have any comments about my UpdateSemantics patches before I send them off to wine-patches?
Looks fine to me.
On the subject of style though, two things: - I really dislike things like LPDWORD and LPD3DXMESH. For one, there's the issue that "const LPDWORD" doesn't do what you want, so you'd need something like "LPCDWORD", which doesn't exist. The other issue is that unless typedefs are used for some kind of abstraction, like e.g. uint32_t, they're just obfuscation. I.e., "typedef type *LPTYPE;" just hides that "LPTYPE" is a pointer, and "typedef enum/struct x x;" just hides that "x" is a struct or enum. - "There’s a standard naming scheme for C, and it’s all lower case with underscores as separators."
On Wed, Jun 8, 2011 at 12:59 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 8 June 2011 11:33, Michael Mc Donnell michael@mcdonnell.dk wrote:
Henri, do you have any comments about my UpdateSemantics patches before I send them off to wine-patches?
Looks fine to me.
Good :-)
On the subject of style though, two things: - I really dislike things like LPDWORD and LPD3DXMESH. For one, there's the issue that "const LPDWORD" doesn't do what you want, so you'd need something like "LPCDWORD", which doesn't exist. The other issue is that unless typedefs are used for some kind of abstraction, like e.g. uint32_t, they're just obfuscation. I.e., "typedef type *LPTYPE;" just hides that "LPTYPE" is a pointer, and "typedef enum/struct x x;" just hides that "x" is a struct or enum.
Thank you for the explanation. I've changed those.
- "There’s a standard naming scheme for C, and it’s all lower case with underscores as separators."
Ah yes, I've been writing too much C++ and C# lately :-) I've changed that too.
I've also moved the declaration of a vertex struct inside the test function, as it was not needed outside of the test function.
Thank you for your comments.
I have added more tests and changed the UpdateSemantics implementation quite a bit. Dylan Smith made me aware that GetNumBytesPerVertex and GetDeclaration work with an invalid declaration, so I've added tests for that and changed the implementation to do the same.
I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size of the vertex declaration, so that they can be used by GetNumBytesPerVertex and GetDeclaration when an invalid declaration has been passed.
Any comments?
On Tuesday 14 June 2011 12:57:35 Michael Mc Donnell wrote:
I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size of the vertex declaration, so that they can be used by GetNumBytesPerVertex and GetDeclaration when an invalid declaration has been passed.
In GetDeclaration:
- memcpy(declaration, This->cached_declaration, sizeof(This-
cached_declaration));
You should probably test if native really writes the full MAX_FVF_DECL_SIZE declaration elements, or only up to the D3DDECL_END() marker in the real declaration. A similar consideration applies to UpdateSemantics, but that is harder to test. E.g.:
+ D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE]; + D3DVERTEXELEMENT9 declaration0[] = + { ...
memset(declaration, 0xaa, sizeof(declaration)); memcpy(declaration, declaration0, sizeof(declaration0)); UpdateSemantics(declaration) memset(declaration, 0xbb, sizeof(declaration)); GetDeclaration(declaration);
Now look at the bytes in 'declaration' after the D3DDECL_END() marker up to the end of the array. They could be 0xaa(UpdateSemantics reads everything, GetDeclaration writes everything), 0xbb(UpdateSemantics is unknown, GetDeclaration writes only the defined part), or something else(e.g. UpdateSemantics and/or GetDeclaration clear set the extra bytes to 0).
In the way your implementation and tests are currently set up UpdateSemantics will read undefined memory because your test declarations like declaration0 don't have the full MAX_FVF_DECL_SIZE array size. Otoh I am not quite sure about the semantics of passing an array on the stack like this. Maybe the compiler allocates a new array with the full size and copies the content around.
I have to say though that the way the API is defined is a bit odd. But I checked it, and UpdateSemantics and GetDeclaration are defined in this way in the DirectX SDK headers.
Also I think this method has been pretty thoroughly tested and debated by now. If we implement all the functions this way we'd probably be pretty bug-free :-)
Also, there's the obligatory style nitpick:
- IDirect3DDevice9 *device;
... +static struct test_context* new_test_context(void)
For consistency this should probably be static struct test_context *new_test_context(void).
On Tue, Jun 14, 2011 at 1:43 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Tuesday 14 June 2011 12:57:35 Michael Mc Donnell wrote:
I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size of the vertex declaration, so that they can be used by GetNumBytesPerVertex and GetDeclaration when an invalid declaration has been passed.
In GetDeclaration:
- memcpy(declaration, This->cached_declaration, sizeof(This-
cached_declaration));
You should probably test if native really writes the full MAX_FVF_DECL_SIZE declaration elements, or only up to the D3DDECL_END() marker in the real declaration. A similar consideration applies to UpdateSemantics, but that is harder to test. E.g.:
- D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE];
- D3DVERTEXELEMENT9 declaration0[] =
- {
...
memset(declaration, 0xaa, sizeof(declaration)); memcpy(declaration, declaration0, sizeof(declaration0)); UpdateSemantics(declaration) memset(declaration, 0xbb, sizeof(declaration)); GetDeclaration(declaration);
Now look at the bytes in 'declaration' after the D3DDECL_END() marker up to the end of the array. They could be 0xaa(UpdateSemantics reads everything, GetDeclaration writes everything), 0xbb(UpdateSemantics is unknown, GetDeclaration writes only the defined part), or something else(e.g. UpdateSemantics and/or GetDeclaration clear set the extra bytes to 0).
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().
In the way your implementation and tests are currently set up UpdateSemantics will read undefined memory because your test declarations like declaration0 don't have the full MAX_FVF_DECL_SIZE array size. Otoh I am not quite sure about the semantics of passing an array on the stack like this. Maybe the compiler allocates a new array with the full size and copies the content around.
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). I've changed the implementation to count the number of elements in the new declaration, cache the number, and then copy the contents of new declaration into the cached declaration. Counting the elements doesn't add any extra overhead as I also needed it to check for non-zero stream values.
Also I think this method has been pretty thoroughly tested and debated by now. If we implement all the functions this way we'd probably be pretty bug-free :-)
Yeah it turned out to be a lot harder than I had expected to get all the details correct. I have also added a check for non-zero stream values that Dylan Smith wanted me to add.
Also, there's the obligatory style nitpick:
- IDirect3DDevice9 *device;
... +static struct test_context* new_test_context(void)
For consistency this should probably be static struct test_context *new_test_context(void).
Check :-)
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.
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.
On Thu, Jun 16, 2011 at 4:49 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Yeah it turned out to be a lot harder than I had expected to get all the details correct. I have also added a check for non-zero stream values that Dylan Smith wanted me to add.
Thanks. Looks good to me too now.
On Fri, Jun 10, 2011 at 2:08 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
D3DXCreateMesh fails when the declaration contains a non-zero Stream value. I would expect UpdateSemantics to be as strict as D3DXCreateMesh, otherwise a test could validate the different behaviour.
Seems like you missed this comment I made before.
/* e.g. */ memcpy(declaration, declaration0, sizeof(declaration0)); declaration[1].Stream = 1; hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration); ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics using multiple streams, " "got %#x expected D3DERR_INVALIDCALL\n", hr);
On Tue, Jun 14, 2011 at 3:42 PM, Dylan Smith dylan.ah.smith@gmail.com wrote:
On Fri, Jun 10, 2011 at 2:08 AM, Dylan Smith dylan.ah.smith@gmail.com wrote:
D3DXCreateMesh fails when the declaration contains a non-zero Stream value. I would expect UpdateSemantics to be as strict as D3DXCreateMesh, otherwise a test could validate the different behaviour.
Seems like you missed this comment I made before.
/* e.g. */ memcpy(declaration, declaration0, sizeof(declaration0)); declaration[1].Stream = 1; hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration); ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics using multiple streams, " "got %#x expected D3DERR_INVALIDCALL\n", hr);
Yes you're right, it returns D3DERR_INVALIDCALL when a declaration contains a non-zero Stream value. I'll update my tests and the implementation.
-
Dylan Smith -
Henri Verbeet -
Matteo Bruni -
Michael Mc Donnell -
Stefan Dösinger