My xinput implementation please test, plus I am getting segfault with spore

[Andrew Fenn]

Hi all,

I am getting a segfault with the release version of Spore (I didn't
test 1.1) right after my code implementation of xinput is handled by
the game. Is there something I am doing wrong here or is it the game?
I also tested Assasin's Creed which uses xinput1_3 and that happens to
load my implementation fine and my test cases work on windows xp SP3
too.

I have included my patches for the xinput implementation. If you could
help me test and find out the spore problem before I submit this to
wine patches it would be very helpful.

I have included test cases with the patch. When testing please make
sure that you don't have any xinput dlls in your program dir or
system32. A list of xinput games can be found here,
http://en.wikipedia.org/wiki/List_of_XInput_games

Thanks and regards,
Andrew

[Henri Verbeet]

2008/10/25 Andrew Fenn :
> Hi all,
>
> I am getting a segfault with the release version of Spore (I didn't
> test 1.1) right after my code implementation of xinput is handled by
> the game. Is there something I am doing wrong here or is it the game?

...
> +DWORD XInputGetState(DWORD dwUserIndex, XINPUT_STATE* pState)

As Stefan already mentioned, this function should be WINAPI. Keeping
it like this will screw up the stack after calls to XInputGetState(),
which could be the cause for the problem with Spore.
> -
>    // Get the state

Don't use C++ comments.
> -
>    ok(result != ERROR_BAD_ARGUMENTS, "XInputGetState failed with (%d)\n", result);

Writing the test like means the test will succeed if the call fails
with a return code other than ERROR_BAD_ARGUMENTS . It would be better
to use "result == ERROR_SUCCESS || result ==
ERROR_DEVICE_NOT_CONNECTED" instead.
> -
>        printf("-- Results for controller %d --\n", controllerNum);
> -
>        printf("XInputGetState: %d\n", result);
> -
>        printf("State->dwPacketNumber: %d\n", controllerState.dwPacketNumber);
> -
>        printf("Gamepad Variables --\n");
> -
>        printf("Gamepad.wButtons: %#x\n", controllerState.Gamepad.wButtons);
> -
>        printf("Gamepad.bLeftTrigger: 0x%08x\n", controllerState.Gamepad.bLeftTrigger);
> -
>        printf("Gamepad.bRightTrigger: 0x%08x\n", controllerState.Gamepad.bRightTrigger);
> -
>        printf("Gamepad.sThumbLX: %d\n", controllerState.Gamepad.sThumbLX);
> -
>        printf("Gamepad.sThumbLY: %d\n", controllerState.Gamepad.sThumbLY);
> -
>        printf("Gamepad.sThumbRX: %d\n", controllerState.Gamepad.sThumbRX);
> -
>        printf("Gamepad.sThumbRY: %d\n", controllerState.Gamepad.sThumbRY);

If you really need this information, you should use trace() instead of
printf(). I suspect you don't need the information though.
> - FIXME("Stub \n");

You don't need the space there. Also, it's often useful to print the
function's arguments in FIXMEs like these, although it's less of an
issue for regular function than for COM methods.
> -#define COBJMACROS
> -

If you don't want this, you shouldn't add it in your first patch,
rather than removing it in the second patch.

- I don't think you should duplicate the test for each dll. If you
find differences between different versions of the dll it might be
worth it to add tests for those specific differences, but if the
behaviour is the same it'll just make it harder to maintain the tests.
  - You should add a prototype for XInputGetState() to xinput.h
  - Try to be a bit more consistent with in your style. eg.:
> - for(controllerNum=0; controllerNum < 4; controllerNum++) {

vs.
> - if (hXinput)
> - {

[Andrew Fenn]

Hi Henri,

Thanks for the input, could you take a look at my patches I sent on
wine patches instead as I have fixed all these issues and updated my
diffs.

http://www.winehq.org/pipermail/wine-patches/2008-October/063800.html
http://www.winehq.org/pipermail/wine-patches/2008-October/063801.html
http://www.winehq.org/pipermail/wine-patches/2008-October/063802.html

Also I managed to find out why spore was crashing too. Thank you for
replying (I thought the email never arrived). I didn't put any
information into the Stub. I was thinking about it but I wasn't sure
how to format it correctly in "wine style".

If you find and error with those patches please let me know.

Regards,
Andrew

On Tue, Oct 28, 2008 at 9:24 PM, Henri Verbeet  wrote:
> 2008/10/25 Andrew Fenn :
> > Hi all,
> >
> > I am getting a segfault with the release version of Spore (I didn't
> > test 1.1) right after my code implementation of xinput is handled by
> > the game. Is there something I am doing wrong here or is it the game?
>
> ...
> > +DWORD XInputGetState(DWORD dwUserIndex, XINPUT_STATE* pState)
>
> As Stefan already mentioned, this function should be WINAPI. Keeping
> it like this will screw up the stack after calls to XInputGetState(),
> which could be the cause for the problem with Spore.
> > -
> >    // Get the state
>
> Don't use C++ comments.
> > -
> >    ok(result != ERROR_BAD_ARGUMENTS, "XInputGetState failed with (%d)\n", result);
>
> Writing the test like means the test will succeed if the call fails
> with a return code other than ERROR_BAD_ARGUMENTS . It would be better
> to use "result == ERROR_SUCCESS || result ==
> ERROR_DEVICE_NOT_CONNECTED" instead.
> > -
> >        printf("-- Results for controller %d --\n", controllerNum);
> > -
> >        printf("XInputGetState: %d\n", result);
> > -
> >        printf("State->dwPacketNumber: %d\n", controllerState.dwPacketNumber);
> > -
> >        printf("Gamepad Variables --\n");
> > -
> >        printf("Gamepad.wButtons: %#x\n", controllerState.Gamepad.wButtons);
> > -
> >        printf("Gamepad.bLeftTrigger: 0x%08x\n", controllerState.Gamepad.bLeftTrigger);
> > -
> >        printf("Gamepad.bRightTrigger: 0x%08x\n", controllerState.Gamepad.bRightTrigger);
> > -
> >        printf("Gamepad.sThumbLX: %d\n", controllerState.Gamepad.sThumbLX);
> > -
> >        printf("Gamepad.sThumbLY: %d\n", controllerState.Gamepad.sThumbLY);
> > -
> >        printf("Gamepad.sThumbRX: %d\n", controllerState.Gamepad.sThumbRX);
> > -
> >        printf("Gamepad.sThumbRY: %d\n", controllerState.Gamepad.sThumbRY);
>
> If you really need this information, you should use trace() instead of
> printf(). I suspect you don't need the information though.
> > - FIXME("Stub \n");
>
> You don't need the space there. Also, it's often useful to print the
> function's arguments in FIXMEs like these, although it's less of an
> issue for regular function than for COM methods.
> > -#define COBJMACROS
> > -
>
> If you don't want this, you shouldn't add it in your first patch,
> rather than removing it in the second patch.
> - I don't think you should duplicate the test for each dll. If you
>
> find differences between different versions of the dll it might be
> worth it to add tests for those specific differences, but if the
> behaviour is the same it'll just make it harder to maintain the tests.
> - You should add a prototype for XInputGetState() to xinput.h
> - Try to be a bit more consistent with in your style. eg.:
> > - for(controllerNum=0; controllerNum < 4; controllerNum++) {
>
> vs.
> > - if (hXinput)
> > - {

[Henri Verbeet]

2008/10/28 Andrew Fenn :
> Also I managed to find out why spore was crashing too. Thank you for
> replying (I thought the email never arrived). I didn't put any

Just noticed the mail is 3 days old. It probably went through
moderation because the patches are relatively large.
> If you find and error with those patches please let me know.

Most problems are fixed there, but these are still there:
> > -
> >    ok(result != ERROR_BAD_ARGUMENTS, "XInputGetState failed with (%d)\n", result);
>
> Writing the test like means the test will succeed if the call fails
> with a return code other than ERROR_BAD_ARGUMENTS . It would be better
> to use "result == ERROR_SUCCESS || result ==
> ERROR_DEVICE_NOT_CONNECTED" instead.
> > -
> > - The Wine project - Xinput Joystick Library
> >
> > -#define COBJMACROS
> > -
>
> If you don't want this, you shouldn't add it in your first patch,
> rather than removing it in the second patch.

Note that this also goes for the change to dlls/xinput9_1_0/version.rc
> - You should add a prototype for XInputGetState() to xinput.h
> - Try to be a bit more consistent with in your style. eg.:
> > - for(controllerNum=0; controllerNum < 4; controllerNum++) {
>
> vs.
> > - if (hXinput)
> > - {

Since the second patch is relatively large, it might be worth it to
split in three parts, one for the header, one for the implementation,
and one for the test.