On Wednesday 08 June 2011 18:51:41 Michael Mc Donnell wrote:
unsigned int faces[] = {0, 1, 2}; unsigned int num_faces = sizeof(faces) / 3;
Does this do what you want? As far as I can see you want ARRAY_SIZE(faces) / 3.
- struct {
D3DXVECTOR3 position0;D3DXVECTOR3 position1;D3DXVECTOR3 normal;DWORD color;- } vertices[] = {
...
Another style nitpick: Bracket placing.
- /* Two null pointers. Setting only the mesh to null will result in an
* exception on Windows.*/- hr = mesh->lpVtbl->UpdateSemantics(NULL, NULL);
I think setting the instance pointer to NULL when invoking a method doesn't need a test, it will give you odd results. What you show here is that native checks the only method parameter before it accesses the object instance.
Remember, usually apps will invoke this via C++:
mesh->UpdateSemantics(declaration);
Similarly it isn't necessary to check iface or This against NULL in the implementations.
Otherwise this looks OK
On Wed, Jun 8, 2011 at 8:32 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Wednesday 08 June 2011 18:51:41 Michael Mc Donnell wrote:
unsigned int faces[] = {0, 1, 2}; unsigned int num_faces = sizeof(faces) / 3;
Does this do what you want? As far as I can see you want ARRAY_SIZE(faces) / 3.
You're right that was a bug
- struct {
- D3DXVECTOR3 position0;
- D3DXVECTOR3 position1;
- D3DXVECTOR3 normal;
- DWORD color;
- } vertices[] = {
...
Another style nitpick: Bracket placing.
Change it to brackets on new lines? I actually copied the style from the method above mine. Is this better?:
+ struct + { + D3DXVECTOR3 position0; + D3DXVECTOR3 position1; + D3DXVECTOR3 normal; + DWORD color; + } vertices[] = + { + { { 0.0f, 1.0f, 0.f}, { 1.0f, 0.0f, 0.f}, {0.0f, 0.0f, 1.0f}, 0xffff0000 }, + { { 1.0f, -1.0f, 0.f}, {-1.0f, -1.0f, 0.f}, {0.0f, 0.0f, 1.0f}, 0xff00ff00 }, + { {-1.0f, -1.0f, 0.f}, {-1.0f, 1.0f, 0.f}, {0.0f, 0.0f, 1.0f}, 0xff0000ff }, + };
- /* Two null pointers. Setting only the mesh to null will result in an
- * exception on Windows.
- */
- hr = mesh->lpVtbl->UpdateSemantics(NULL, NULL);
I think setting the instance pointer to NULL when invoking a method doesn't need a test, it will give you odd results. What you show here is that native checks the only method parameter before it accesses the object instance. Remember, usually apps will invoke this via C++:
mesh->UpdateSemantics(declaration);
Similarly it isn't necessary to check iface or This against NULL in the implementations.
Ok, I've removed that check. That wasn't what was causing the exception though. The problem was that it failed to get a device on the testbot. I had forgotten to set the mesh to NULL, so in the cleanup part it tried to release a random pointer, which caused an exception.
On a side note, failing to get a device means that the d3dx tests are always skipped on the testbot?
Otherwise this looks OK
Ok, I'll try again later.
On 9 June 2011 13:01, Michael Mc Donnell michael@mcdonnell.dk wrote:
On a side note, failing to get a device means that the d3dx tests are always skipped on the testbot?
The ones that need a device, yes. The testbot machines are all VMs, so they don't have real display hardware. For a realistic setup with real hardware we'd probably need at least 2 different Windows versions (e.g Win 7 and XP), and at least the current and the previous generation of both AMD and NVIDIA display hardware. We could probably manage the hardware costs for that, but I imagine that the setup and admin overhead would be significant compared to the VMs.
2011/6/9 Henri Verbeet hverbeet@gmail.com
On 9 June 2011 13:01, Michael Mc Donnell michael@mcdonnell.dk wrote:
On a side note, failing to get a device means that the d3dx tests are always skipped on the testbot?
The ones that need a device, yes. The testbot machines are all VMs, so they don't have real display hardware. For a realistic setup with real hardware we'd probably need at least 2 different Windows versions (e.g Win 7 and XP), and at least the current and the previous generation of both AMD and NVIDIA display hardware. We could probably manage the hardware costs for that, but I imagine that the setup and admin overhead would be significant compared to the VMs.
you can usually find someone on irc (like me) with a windows setup where you can test these patches, until there is some proper testing platform.
On Thu, Jun 9, 2011 at 1:56 PM, Ricardo Filipe ricardojdfilipe@gmail.com wrote:
2011/6/9 Henri Verbeet hverbeet@gmail.com
On 9 June 2011 13:01, Michael Mc Donnell michael@mcdonnell.dk wrote:
On a side note, failing to get a device means that the d3dx tests are always skipped on the testbot?
The ones that need a device, yes. The testbot machines are all VMs, so they don't have real display hardware. For a realistic setup with real hardware we'd probably need at least 2 different Windows versions (e.g Win 7 and XP), and at least the current and the previous generation of both AMD and NVIDIA display hardware. We could probably manage the hardware costs for that, but I imagine that the setup and admin overhead would be significant compared to the VMs.
Yeah I haven't seen any good way of virtualizing graphics hardware, and it sounds like it would be difficult to setup real machines, as they would need to be able to roll back automatically after each test run.
you can usually find someone on irc (like me) with a windows setup where you can test these patches, until there is some proper testing platform.
Thanks for the offer, but I already own a Windows machine. It's just a hassle to switch between Linux and Windows, and it would be nice to have automated testing for regression purposes.
On Thursday 09 June 2011 13:22:27 Henri Verbeet wrote:
The ones that need a device, yes. The testbot machines are all VMs, so they don't have real display hardware.
Maybe we can set up the refrast? It has some problems, and not all our tests will pass(intentionally), but it would allow to run most of the tests, and probably help d3dx9 a lot. The refrast will slow down testbot a lot though, at least when we run the d3d9:visual tests. The last time I tested them in qemu it took about 10 minutes to run them. That was about 2 years ago.
On Thursday 09 June 2011 13:01:17 Michael Mc Donnell wrote:
Change it to brackets on new lines? I actually copied the style from the method above mine. Is this better?:
- struct
- {
...
- } vertices[] =
- {
Yes, although I'd also move the vertices[] in a separate line.
The other tests in this file aren't entirely consistent, but I've seen at least one that used your old style and one that uses the new style. In wined3d and related dlls we use the new style.
-
Henri Verbeet -
Michael Mc Donnell -
Ricardo Filipe -
Stefan Dösinger