Re: [v4 2/4] winejoystick.drv/joystick_osx.c: fixes Slider mapping

[Ken Thomases]

On Jun 13, 2016, at 12:11 AM, David Lawrie  wrote:
> Sliders, Dials, and Wheels now map to Z, U (Ry), or V (Rx) depending on
> availability. Thus, also adds support for up to 3 such HID elements.
> dlls/winejoystick.drv/joystick_osx.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/winejoystick.drv/joystick_osx.c b/dlls/winejoystick.drv/joystick_osx.c
> index 77dbef8..4e59ca3 100644
> --- a/dlls/winejoystick.drv/joystick_osx.c
> +++ b/dlls/winejoystick.drv/joystick_osx.c
> @@ -442,8 +442,26 @@ static void collect_joystick_elements(joystick_t* joystick, IOHIDElementRef coll
>                         break;
>                     }
>                     case kHIDUsage_GD_Slider:
> -
>                    TRACE("kIOHIDElementTypeInput_Misc / kHIDUsage_GD_Slider; ignoring\n");
> -
>                case kHIDUsage_GD_Dial:
> -
>                case kHIDUsage_GD_Wheel:
> -
>                {
> -
>                    //if one axis is taken, fall to the next until axes are filled

Line comments (//) are not allowed in most of Wine.
> -
>                    int possible_axes[3] = {AXIS_Z,AXIS_RY,AXIS_RX};
> -
>                    int axis = 0;
> -
>                    TRACE("kIOHIDElementTypeInput_Misc / kHIDUsage_GD_<axis> (%d)", usage);
> -
>                    while(axis < 3 && joystick->axes[possible_axes[axis]].element)
> -
>                        axis++;
> -
>                    if (axis == 3)
> -
>                        TRACE("    ignoring\n");
> -
>                    else
> -
>                    {
> -
>                        TRACE(" axis %d\n", possible_axes[axis]);

Rather than breaking the trace line across multiple TRACEs, you should just move the TRACE to after the selection of an axis.  Note that in the existing code, the "ignoring" TRACEs are on a separate line, which is why they are indented.  It doesn't make sense to have that extra space if that isn't on a separate line.

Alternatively, you could put a newline on the first TRACE and indent the axis trace as a subordinate line.
> -
>                        joystick->axes[possible_axes[axis]].element = (IOHIDElementRef)CFRetain(child);
> -
>                        joystick->axes[possible_axes[axis]].min_value = IOHIDElementGetLogicalMin(child);
> -
>                        joystick->axes[possible_axes[axis]].max_value = IOHIDElementGetLogicalMax(child);
> -
>                    }
>                   break;
> -
>                }

Is there reason to expect that these elements (sliders, dials, wheels) will be listed after the "proper" axes (x, y, z, etc.) in the children array?  Since the proper axes are specific and these elements should only be mapped to unused axes, I think they should be enumerated in a separate pass after everything else has had a shot.  Note that collect_joystick_elements() is recursive, so it's not right to do it in a separate loop within collect_joystick_elements().  Probably, you'll want to call it twice from open_joystick(), with a flag to indicate which mode it should operate in.

-Ken

[v4 2/4] winejoystick.drv/joystick_osx.c: fixes Slider mapping

[DavidL]

On Mon, Jun 13, 2016 at 2:46 PM, Ken Thomases  wrote:
> On Jun 13, 2016, at 12:11 AM, David Lawrie  wrote:
> > Sliders, Dials, and Wheels now map to Z, U (Ry), or V (Rx) depending on
> > availability. Thus, also adds support for up to 3 such HID elements.
> > dlls/winejoystick.drv/joystick_osx.c | 20 +++++++++++++++++++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/winejoystick.drv/joystick_osx.c
>
> b/dlls/winejoystick.drv/joystick_osx.c
> > index 77dbef8..4e59ca3 100644
> > --- a/dlls/winejoystick.drv/joystick_osx.c
> > +++ b/dlls/winejoystick.drv/joystick_osx.c
> > @@ -442,8 +442,26 @@ static void collect_joystick_elements(joystick_t*
>
> joystick, IOHIDElementRef coll
> >                     break;
> >                 }
> >                 case kHIDUsage_GD_Slider:
> > -
> >                    TRACE("kIOHIDElementTypeInput_Misc /
>
> kHIDUsage_GD_Slider; ignoring\n");
> > -
> >                case kHIDUsage_GD_Dial:
> > -
> >                case kHIDUsage_GD_Wheel:
> > -
> >                {
> > -
> >                    //if one axis is taken, fall to the next until
>
> axes are filled
>
> Line comments (//) are not allowed in most of Wine.
> > -
> >                    int possible_axes[3] = {AXIS_Z,AXIS_RY,AXIS_RX};
> > -
> >                    int axis = 0;
> > -
> >                    TRACE("kIOHIDElementTypeInput_Misc /
>
> kHIDUsage_GD_<axis> (%d)", usage);
> > -
> >                    while(axis < 3 &&
>
> joystick->axes[possible_axes[axis]].element)
> > -
> >                        axis++;
> > -
> >                    if (axis == 3)
> > -
> >                        TRACE("    ignoring\n");
> > -
> >                    else
> > -
> >                    {
> > -
> >                        TRACE(" axis %d\n", possible_axes[axis]);
>
> Rather than breaking the trace line across multiple TRACEs, you should
> just move the TRACE to after the selection of an axis.  Note that in the
> existing code, the "ignoring" TRACEs are on a separate line, which is why
> they are indented.  It doesn't make sense to have that extra space if that
> isn't on a separate line.
>
> Alternatively, you could put a newline on the first TRACE and indent the
> axis trace as a subordinate line.

I'll try rationalize the TRACE commands.
> > -
> >                        joystick->axes[possible_axes[axis]].element
>
> = (IOHIDElementRef)CFRetain(child);
> > -
>
> joystick->axes[possible_axes[axis]].min_value =
> IOHIDElementGetLogicalMin(child);
> > -
>
> joystick->axes[possible_axes[axis]].max_value =
> IOHIDElementGetLogicalMax(child);
> > -
> >                    }
> >                   break;
> > -
> >                }
>
> Is there reason to expect that these elements (sliders, dials, wheels)
> will be listed after the "proper" axes (x, y, z, etc.) in the children
> array?  Since the proper axes are specific and these elements should only
> be mapped to unused axes, I think they should be enumerated in a separate
> pass after everything else has had a shot.  Note that
> collect_joystick_elements() is recursive, so it's not right to do it in a
> separate loop within collect_joystick_elements().  Probably, you'll want to
> call it twice from open_joystick(), with a flag to indicate which mode it
> should operate in.

This where the Microsoft source documentation get *really* fuzzy. There are
all sorts of possible mappings in the rare case where there is both a
HID/Dinput Z-axis and a slider/dial/wheel. Microsoft gave 4-5 different
variations of how it could go and in some of these mappings, the slider
would take precedence over the reported HID Z-axis when mapping to the
Winmm Z-axis such that slider -> WinmmZ is correct and the HID Z-axis is
then ignored. So I decided to follow the Linux Winmm version and simply
have whichever element is listed by the joystick first, wins. If it is the
axis, it's the axis, if it is the slider, it's the slider. This actually is
one of the ways Microsoft says to decide proper HID mappings to Winmm (and
it seems like that for the elements within a joystick, the elements are
always reported in the same order such that a user shouldn't experience the
axes mappings changing each time they start up Wine/reconnect a device*).
There is no 1 correct mapping for all cases. I guess ideally we would have
a way of allowing the user to remap the joystick axes in the registry
(maybe that would work even now? I don't know - I've seen some emails on
that already in the wine-devel list serve but I don't know what the status
of that kind of capability is) but even Microsoft states on of their pages
that this is a pretty rare corner case.

*Note: in my testing this is *not* the same for OS X (I guess macOS now)
reporting back devices - which human interface device is reported first
seems to be random in my testing when I had both a virtual and physical
joystick setup. Historically Winmm games would only take input from the
first reported device - under Mac-Wine, I've noticed the games seem to be
taking input from both my physical and virtual joysticks (sometimes
regardless of what the game was saying it was doing!). This has both good
and bad potential and is not necessarily behavior we should spend effort
trying to get rid of. Not sure how the Linux version behaves in this
situation in terms of joystick order and input from multiple sources.
> -Ken

[v4 2/4] winejoystick.drv/joystick_osx.c: fixes Slider mapping

[DavidL]

On Mon, Jun 13, 2016 at 11:23 PM, DavidL  wrote:
> On Mon, Jun 13, 2016 at 2:46 PM, Ken Thomases  wrote:
> > On Jun 13, 2016, at 12:11 AM, David Lawrie 
> > wrote:
> > > Sliders, Dials, and Wheels now map to Z, U (Ry), or V (Rx) depending on
> > > availability. Thus, also adds support for up to 3 such HID elements.
> > > dlls/winejoystick.drv/joystick_osx.c | 20 +++++++++++++++++++-
> > > 1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/dlls/winejoystick.drv/joystick_osx.c
> >
> > b/dlls/winejoystick.drv/joystick_osx.c
> > > index 77dbef8..4e59ca3 100644
> > > --- a/dlls/winejoystick.drv/joystick_osx.c
> > > +++ b/dlls/winejoystick.drv/joystick_osx.c
> > > @@ -442,8 +442,26 @@ static void collect_joystick_elements(joystick_t*
> >
> > joystick, IOHIDElementRef coll
> > >                     break;
> > >                 }
> > >                 case kHIDUsage_GD_Slider:
> > > -
> > >                    TRACE("kIOHIDElementTypeInput_Misc /
> >
> > kHIDUsage_GD_Slider; ignoring\n");
> > > -
> > >                case kHIDUsage_GD_Dial:
> > > -
> > >                case kHIDUsage_GD_Wheel:
> > > -
> > >                {
> > > -
> > >                    //if one axis is taken, fall to the next until
> >
> > axes are filled
> >
> > Line comments (//) are not allowed in most of Wine.

Sorry, I'll change that to a comment block.
> > > -
> > >                    int possible_axes[3] =
> >
> > {AXIS_Z,AXIS_RY,AXIS_RX};
> > > -
> > >                    int axis = 0;
> > > -
> > >                    TRACE("kIOHIDElementTypeInput_Misc /
> >
> > kHIDUsage_GD_<axis> (%d)", usage);
> > > -
> > >                    while(axis < 3 &&
> >
> > joystick->axes[possible_axes[axis]].element)
> > > -
> > >                        axis++;
> > > -
> > >                    if (axis == 3)
> > > -
> > >                        TRACE("    ignoring\n");
> > > -
> > >                    else
> > > -
> > >                    {
> > > -
> > >                        TRACE(" axis %d\n", possible_axes[axis]);
> >
> > Rather than breaking the trace line across multiple TRACEs, you should
> > just move the TRACE to after the selection of an axis.  Note that in the
> > existing code, the "ignoring" TRACEs are on a separate line, which is why
> > they are indented.  It doesn't make sense to have that extra space if that
> > isn't on a separate line.
> >
> > Alternatively, you could put a newline on the first TRACE and indent the
> > axis trace as a subordinate line.
>
> I'll try rationalize the TRACE commands.
> > > -
> >
> > joystick->axes[possible_axes[axis]].element =
> > (IOHIDElementRef)CFRetain(child);
> > > -
> >
> > joystick->axes[possible_axes[axis]].min_value =
> > IOHIDElementGetLogicalMin(child);
> > > -
> >
> > joystick->axes[possible_axes[axis]].max_value =
> > IOHIDElementGetLogicalMax(child);
> > > -
> > >                    }
> > >                   break;
> > > -
> > >                }
> >
> > Is there reason to expect that these elements (sliders, dials, wheels)
> > will be listed after the "proper" axes (x, y, z, etc.) in the children
> > array?  Since the proper axes are specific and these elements should only
> > be mapped to unused axes, I think they should be enumerated in a separate
> > pass after everything else has had a shot.  Note that
> > collect_joystick_elements() is recursive, so it's not right to do it in a
> > separate loop within collect_joystick_elements().  Probably, you'll want to
> > call it twice from open_joystick(), with a flag to indicate which mode it
> > should operate in.
>
> This where the Microsoft source documentation get *really* fuzzy. There
> are all sorts of possible mappings in the rare case where there is both a
> HID/Dinput Z-axis and a slider/dial/wheel. Microsoft gave 4-5 different
> variations of how it could go and in some of these mappings, the slider
> would take precedence over the reported HID Z-axis when mapping to the
> Winmm Z-axis such that slider -> WinmmZ is correct and the HID Z-axis is
> then ignored. So I decided to follow the Linux Winmm version and simply
> have whichever element is listed by the joystick first, wins. If it is the
> axis, it's the axis, if it is the slider, it's the slider. This actually is
> one of the ways Microsoft says to decide proper HID mappings to Winmm (and
> it seems like that for the elements within a joystick, the elements are
> always reported in the same order such that a user shouldn't experience the
> axes mappings changing each time they start up Wine/reconnect a device*).
> There is no 1 correct mapping for all cases. I guess ideally we would have
> a way of allowing the user to remap the joystick axes in the registry
> (maybe that would work even now? I don't know - I've seen some emails on
> that already in the wine-devel list serve but I don't know what the status
> of that kind of capability is) but even Microsoft states on of their pages
> that this is a pretty rare corner case.
>
> *Note: in my testing this is *not* the same for OS X (I guess macOS now)
> reporting back devices - which human interface device is reported first
> seems to be random in my testing when I had both a virtual and physical
> joystick setup. Historically Winmm games would only take input from the
> first reported device - under Mac-Wine, I've noticed the games seem to be
> taking input from both my physical and virtual joysticks (sometimes
> regardless of what the game was saying it was doing!). This has both good
> and bad potential and is not necessarily behavior we should spend effort
> trying to get rid of. Not sure how the Linux version behaves in this
> situation in terms of joystick order and input from multiple sources.
> > -Ken

[v4 2/4] winejoystick.drv/joystick_osx.c: fixes Slider mapping

[Ken Thomases]

On Jun 14, 2016, at 1:23 AM, DavidL  wrote:
> On Mon, Jun 13, 2016 at 2:46 PM, Ken Thomases <[e-mail] > wrote:
> Is there reason to expect that these elements (sliders, dials, wheels) will be listed after the "proper" axes (x, y, z, etc.) in the children array?  Since the proper axes are specific and these elements should only be mapped to unused axes, I think they should be enumerated in a separate pass after everything else has had a shot.  Note that collect_joystick_elements() is recursive, so it's not right to do it in a separate loop within collect_joystick_elements().  Probably, you'll want to call it twice from open_joystick(), with a flag to indicate which mode it should operate in.
>
> This where the Microsoft source documentation get *really* fuzzy. There are all sorts of possible mappings in the rare case where there is both a HID/Dinput Z-axis and a slider/dial/wheel. Microsoft gave 4-5 different variations of how it could go and in some of these mappings, the slider would take precedence over the reported HID Z-axis when mapping to the Winmm Z-axis such that slider -> WinmmZ is correct and the HID Z-axis is then ignored. So I decided to follow the Linux Winmm version and simply have whichever element is listed by the joystick first, wins. If it is the axis, it's the axis, if it is the slider, it's the slider. This actually is one of the ways Microsoft says to decide proper HID mappings to Winmm (and it seems like that for the elements within a joystick, the elements are always reported in the same order such that a user shouldn't experience the axes mappings changing each time they start up Wine/reconnect a device*). There is no 1 correct mapping for all cases. I guess ideally we would have a way of allowing the user to remap the joystick axes in the registry (maybe that would work even now? I don't know - I've seen some emails on that already in the wine-devel list serve but I don't know what the status of that kind of capability is) but even Microsoft states on of their pages that this is a pretty rare corner case.

OK, fine by me.
> *Note: in my testing this is *not* the same for OS X (I guess macOS now) reporting back devices - which human interface device is reported first seems to be random in my testing when I had both a virtual and physical joystick setup. Historically Winmm games would only take input from the first reported device - under Mac-Wine, I've noticed the games seem to be taking input from both my physical and virtual joysticks (sometimes regardless of what the game was saying it was doing!). This has both good and bad potential and is not necessarily behavior we should spend effort trying to get rid of. Not sure how the Linux version behaves in this situation in terms of joystick order and input from multiple sources.

It was taking input from both joysticks in the same run?  At the same time?  If so, can you figure out how that's happening?  It really shouldn't, as near as I can see.

Or was it just arbitrarily picking which was first and therefore active for any given run?  If this, then it's <https://bugs.winehq.org/show_bug.cgi?id=38997 https://bugs.winehq.org/show_bug.cgi?id=38997>, more or less.

-Ken