Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update joy button and stick names, enums and documentation #43591

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

madmiraal
Copy link
Contributor

Closes #43520.

@madmiraal madmiraal changed the title Update joy button and stick names and update get_joy_X_string documentation Update joy button and stick names, enums and documentation Nov 16, 2020
@madmiraal
Copy link
Contributor Author

madmiraal commented Nov 16, 2020

Updated the enums to match the standardised names with numerical, SDL and controller specific alternatives mapping to the standardised one. Also updated the documentation to reflect this.

@madmiraal madmiraal force-pushed the fix-43520 branch 2 times, most recently from 6d4ffbf to 3dc64aa Compare November 16, 2020 17:17
@akien-mga
Copy link
Member

Hm I'm a bit conflicted about these changes, it's a huge amount of bindings in a list that was already pretty unwieldy with all those aliases.

Should we go in the other direction and drop unnecessarily redundant bindings? IMO we should standardize on a standard gamepad "picture" much like SDL does, and drop all the Xbox, Sony or Nintendo equivalents. I don't see much point in using them, documentation can cover the equivalence between our own "standard" gamepad and the face buttons on mainstream consoles.

As for all the JOY_BUTTON_0-35, IMO they're also a bit noisy in the docs, and not particularly useful. At this point users had better use integers instead of constants whose names are mapped to integers - they can loop over JOY_BUTTON_MAX if they need to access all valid button indices.

For 4.0 we should really use the opportunity and make this list actually useful and easy to read.

As for the "standard" gamepad naming for the "action" buttons, I don't have a strong opinion on whether we should JOY_BUTTON_<LOCATION>_ACTION or keep the SDL ones inspired from Xbox controllers (which are often used as the lingua franca).
My suggestion to change the names was because all button mappings had "SDL" in their description, so I missed that the JOY_BUTTON_A, etc. are actually the SDL names for their own "standard" GameController interface: https://wiki.libsdl.org/SDL_GameControllerButton?highlight=%28%5CbCategoryGameController%5Cb%29%7C%28CategoryEnum%29
As seen here, SDL gets away with a much cleaner API without redundant bindings, and I think we should aim for that too.

CC @groud @27thLiz

@madmiraal
Copy link
Contributor Author

madmiraal commented Nov 17, 2020

I'm in favour of just having the basic SDL enums:

enum JoyButtonList {
	JOY_INVALID_BUTTON = -1,
	JOY_BUTTON_A = 0,
	JOY_BUTTON_B = 1,
	JOY_BUTTON_X = 2,
	JOY_BUTTON_Y = 3,
	JOY_BUTTON_BACK = 4,
	JOY_BUTTON_GUIDE = 5,
	JOY_BUTTON_START = 6,
	JOY_BUTTON_LEFT_STICK = 7,
	JOY_BUTTON_RIGHT_STICK = 8,
	JOY_BUTTON_LEFT_SHOULDER = 9,
	JOY_BUTTON_RIGHT_SHOULDER = 10,
	JOY_BUTTON_DPAD_UP = 11,
	JOY_BUTTON_DPAD_DOWN = 12,
	JOY_BUTTON_DPAD_LEFT = 13,
	JOY_BUTTON_DPAD_RIGHT = 14,
	JOY_SDL_BUTTONS = 15,
	JOY_BUTTON_MAX = 36 // Android supports up to 36 buttons.
};
enum JoyAxisList {
	JOY_INVALID_AXIS = -1,
	JOY_AXIS_LEFT_X = 0,
	JOY_AXIS_LEFT_Y = 1,
	JOY_AXIS_RIGHT_X = 2,
	JOY_AXIS_RIGHT_Y = 3,
	JOY_AXIS_TRIGGER_LEFT = 4,
	JOY_AXIS_TRIGGER_RIGHT = 5,
	JOY_SDL_AXES = 6,
	JOY_AXIS_MAX = 10 // OpenVR supports up to 5 Joysticks making a total of 10 axes.
};

However, there were objections when other equivalents were being deleted in #38151.

I'm also in favour of deleting the methods:

  • int get_joy_axis_index_from_string ( String axis )
  • String get_joy_axis_string ( int axis_index )
  • int get_joy_button_index_from_string ( String button )
  • String get_joy_button_string ( int button_index )

I don't know of a need for String equivalents, and having them causes the confusion raised in #43520. The serialising argument is moot, because they would be serialised as ints.

@akien-mga
Copy link
Member

However, there were objections when other equivalents were being deleted in #38151.

Keeping the VR aliases might make sense as those aliases are different from a "standard" Xbox/Sony/Nintendo controller, and the mapping of VR buttons to SDL buttons might not be obvious. Paging @BastiaanOlij @aaronfranke.
But the rest can go IMO.

Some thoughts on the proposed enums:

  • Should we make them all use the same JOY_BUTTON_ and JOY_AXIS_ prefixes for consistency? So e.g. JOY_INVALID_BUTTON would become JOY_BUTTON_INVALID (less "correct" in English, but consistent with the rest).
  • Similarly, I guess JOY_SDL_BUTTONS and JOY_SDL_AXES are the "max" values for SDL style mappings? If so maybe we can name them JOY_BUTTON_SDL_MAX and JOY_AXIS_SDL_MAX.

I don't know of a need for String equivalents, and having them causes the confusion raised in #43520. The serialising argument is moot, because they would be serialised as ints.

Serializing is maybe not the best use case indeed, but I think those methods can be used to display human-readable names for the buttons. If you want to make a controls remapping UI, you need a way to present the user with a name, and this is one option if you're not implementing your own names map.

I thought the InputMapEditor used them, but it seems to have its own hardcoded names for those. At the very least we should likely sync them / make the InputMapEditor use the Input methods... otherwise it's indeed a sign that the Input string methods might not be that useful.

@groud
Copy link
Member

groud commented Nov 17, 2020

To be honest I don't have a strong opinion on these. I'd say the aliases are fine, as, usually, users are only fine with one type of layout (I still can't remember about the position of A, B, X and Y on a Xbox controller ^^). I may be biased here, but I think the aliases do not hurt that much.

But I am fine with a smaller enum too.

Should we make them all use the same JOY_BUTTON_ and JOY_AXIS_ prefixes for consistency? So e.g. JOY_INVALID_BUTTON would become JOY_BUTTON_INVALID (less "correct" in English, but consistent with the rest).

I agree on that.

Serializing is maybe not the best use case indeed, but I think those methods can be used to display human-readable names for the buttons. If you want to make a controls remapping UI, you need a way to present the user with a name, and this is one option if you're not implementing your own names map.

I would keep only the getters then. I don't think the setting from string is needed.

"Bottom Action",
"Right Action",
"Left Action",
"Top Action",
Copy link
Member

@aaronfranke aaronfranke Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this change at all. Why "Top Action" but not "Select Action" or "Guide Action"? What is "Top" the top of, if we're not specifying that these are the face buttons?

I'm also wondering if it would be a better idea to just go with "A button", "B button", etc. Pretty much every controller except for Sony and Nintendo controllers use the SDL layout (based on the Xbox layout), including Xbox controllers, Stadia controller, Steam controllers, most Android controllers, and most other generic controllers.

Even if the names would be inconsistent with Sony/Nintendo, every game developer knows about the difference in game controllers, and if they're developing/testing with a Nintendo controller then the mismatch will be immediately clear so that they know to use the SDL/Xbox/Steam/Stadia/Android/etc layout in code.

@aaronfranke
Copy link
Member

The redundancy in the enum is not really a problem, but I agree that just listing out numbers 0-35 is rather pointless.

I await the return of the VR enum values :)

Should we make them all use the same JOY_BUTTON_ and JOY_AXIS_ prefixes for consistency? So e.g. JOY_INVALID_BUTTON would become JOY_BUTTON_INVALID (less "correct" in English, but consistent with the rest).

I already did this as part of #38054, but it's awaiting your review. These PRs are absolutely going to conflict with each other...

@BastiaanOlij
Copy link
Contributor

@akien-mga if this isn't going to be backported to 3.2 and solely a breaking change in 4.0 I wouldn't worry about VR.

If all goes through with the work we discussed around OpenXR the current line of thinking is to adopt Valve and OpenXRs approach using action based inputs. Those inputs can then be mapped to our standard buttons for each given VR controller.

Even if we stick to the current approach those SDL buttons cover everything, I much rather have us stick to proper button mappings so the plugins adhere to Godots button definition then having a separate set like we do now. The original approach stems from a time that nobody knew how this would evolve.

@madmiraal
Copy link
Contributor Author

madmiraal commented Nov 18, 2020

Updated to reflect all the above feedback as best I can.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicks and suggestions, but otherwise looks great to me!

@madmiraal
Copy link
Contributor Author

Added trailing commas, slightly updated the editor mapping descriptions and updated the documentation.

@akien-mga akien-mga merged commit 6a683f8 into godotengine:master Nov 19, 2020
@akien-mga
Copy link
Member

Thanks!

@madmiraal madmiraal deleted the fix-43520 branch November 20, 2020 06:45
TTRC("Joystick 2 Up"),
TTRC("Joystick 2 Down, Right Trigger, R2, RT"),
TTRC("Right Trigger, R2, RT, Joystick 2 Down"),
Copy link
Contributor

@EricEzaM EricEzaM Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here for some reason "Sony" and "Xbox" were left out of L2/R2 and LT/RT, whereas they are present in descriptions everywhere else. I will add this in #43660

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finalise names for joypad buttons and document them
7 participants