daemon3000 / InputManager

Custom InputManager for Unity
Other
587 stars 88 forks source link

Discrepancy between UI and code when working with axes #17

Closed TheSniperFan closed 8 years ago

TheSniperFan commented 8 years ago

When working with axes, the axis variable of the AxisConfiguration is off by one. The UI says that the mouse wheel is the third axis, but the code returns a 2 when accessing it. The included joystick mapping explorer tells you that the XBox 360 controller uses the fourth and fifth axis for the right stick, but the code return 3 and 4 respectively.

I haven't made a patch, because I'm not really familiar with the code base and am unsure as to whether changing the numbering to start at 1 instead of 0 would break something elsewhere. But in order to prevent confusion, the UI and the variable should be consistent. (Besides that, having a '0-th' axis doesn't make a whole lot of sense.)

If updating the data-structure is out of the question (breaking backwards compatibility for example), I'd propose to make the axis variable private (or at the very least internal) and implement a getter like this:

public int GetAxisNumber() {
    return axis + 1;
}
daemon3000 commented 8 years ago

In code, axis values start at zero because that's how it's implemented by Unity(behind the scenes I still use the built-in Input class to get the input).

In the editor and UI the axis values are referred to as: X(0), Y(1), 3rd(2), 4th(3), etc. This is to keep it consistent with Unity's naming convention and not confuse people when they switch from the built-in input to this plugin.

The easiest thing to do would be to change the naming convention in the editor/UI to: 0th, 1st, 2nd, etc.

Changing the code to use axis values starting from 1 is probably doable but not practical.

And you'd need to refactor the axis variable into something like this:

//  Make the "axis" variable private and rename it to "_axis".
//  It's important you make the variable private and then rename it so it only gets renamed in the AxisConfiguration file.
[SerializedField]
[FormerlySerializedAs("axis")]
private int _axis;

//  Create a property to replace the old field. Give it the same name so the code keeps compiling.
public int axis
{
    get
    {
        return _axis + 1;
    }
    set
    {
        _axis = Mathf.Max(value - 1, 0);
    }
}

You are free to make these changes in your fork if you want but I will not accept a pull request because I don't consider this a real issue.

TheSniperFan commented 8 years ago

I see. If it's that way to stay consistent with Unity's own input manager, I understand why you did this and want to keep it that way.

To avoid further confusion, I'll add this information to the documentation and create a pull request tomorrow.

It is rather confusing after all.