-
-
Notifications
You must be signed in to change notification settings - Fork 19.9k
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
Rename many global enums relating to input #38054
Conversation
This comment has been minimized.
This comment has been minimized.
e378aab
to
fe4e82c
Compare
6db1987
to
aa96a61
Compare
c9c8229
to
4f943eb
Compare
core/input/input.cpp
Outdated
} | ||
|
||
JoyAxisList Input::_get_output_axis(String output) { | ||
::JoyAxis Input::_get_output_axis(String output) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit unfortunate to have a name clash with the other JoyAxis
. Not sure if/how to solve it, it's not too bad I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could rename struct JoyAxis
to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, maybe JoyAxisValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good change to me overall. Would like some extra opinions before merging though.
@@ -104,12 +104,12 @@ static Vector<_CoreConstant> _global_constants; | |||
|
|||
#endif | |||
|
|||
VARIANT_ENUM_CAST(KeyList); | |||
VARIANT_ENUM_CAST(Key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only remaining concern that I have is that some users might not be happy with the fairly generic (3 letter) Key
identifier being taken in the global scope. Might lead to issues if one wants to name a class Key
for a key item, or naming a public property Key
in C# (which I think uses PascalCase for public properties).
Of course I agree that the name is correct for the enum and matches the constants' naming scheme. But that might be the reason why this (and other enums, for consistency) was named KeyList
and not Key
in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also already have name conflicts between properties and non-properties in the core Godot API for C#.
One option is to make KeyList
be the exception, or we could do something else like Keyboard
or KeyCode
. Key
seems fine to me though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KeyCode
sounds good. InputEventKey has a property with this name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess KeyCode
might be fine. It does re-add an inconsistency, but to some extent the KeyList
enumerated values are actually different from the other enums. They're internal keycodes while the rest are indices (0 to max).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have KEY_ENTER
or KEY_CODE_ENTER
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KEY_ENTER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we can't use KeyCode
, it conflicts with X11:
/usr/include/X11/X.h:108:23: note: 'KeyCode' has a previous declaration here
108 | typedef unsigned char KeyCode;
| ^~~~~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give it a go with Key
and see what happens. If it turns out problematic, we can just do the few renames necessary before the 4.0 release.
Thanks! |
Implements #16863 (comment) + the following 2 comments. Implements and closes godotengine/godot-proposals#1817.
ButtonList
->MouseButton
and members now start withMOUSE_BUTTON_
KeyList
->Key
and all members are the same.JoyButtonList
->JoyButton
andJoyAxisList
->JoyAxis
.MidiMessageList
->MIDIMessage
for consistency with the above.