Made serialization of Command toggleable when saving InputEvents.

[EricEzaM]

Made serialization of Command optional. If command is serialized, Control (On Win/Linux) or Meta (on Mac) are not.
Example use case: You are on Windows and you set a shortcut to be Control + E. Currently, this would serialize as Command=true and Control=true. If you then run this project on Mac, you would need to press Command AND Control to activate the shortcut - which is not what is intended. Now, you can set store_command to true, and it will only serialize to Command = true (no Control serialized). On Windows, this means Control. On Mac, it means only command.

This was discussed at length with Reduz on IRC, and he seemed happy with this solution. Closes #42351 in some ways... but doesn't fully close it until a UI is created to actually change this option in the editor.

This is the third PR of a larger series based on #42600. In this PR, there is no UI to allow users to edit this setting. This comes in a later PR. This is the InputEvent implementation only.

[akien-mga]

Could you add documentation in doc/classes/InputEventWithModifiers.xml for the new property? You can generate the template with godot --doctool ..

[akien-mga]

The code changes are fine, and if @reduz agreed with them we could merge already, but I'd like to take the opportunity to discuss the IMO confusing unions that we use on Apple/non-Apple in this class:

godot/core/input/input_event.h

Lines 208 to 226 in 2ca1c17

	class InputEventWithModifiers : public InputEventFromWindow {
	GDCLASS(InputEventWithModifiers, InputEventFromWindow);
	bool shift = false;
	bool alt = false;
	#ifdef APPLE_STYLE_KEYS
	union {
	bool command;
	bool meta = false; //< windows/mac key
	};
	bool control = false;
	#else
	union {
	bool command; //< windows/mac key
	bool control = false;
	};
	bool meta = false; //< windows/mac key
	#endif

First the comments are buggy - we shouldn't allow @reduz to write trailing comments as he always just copy pastes lines without ever editing comments :P

So what we have currently is:

• Apple has Command (== Meta) and Control.
• Non-Apple have Control (== Command) and Meta.

Then we use Command as "main" modifiers for the typical Ctrl+C / Cmd+C types of shortcuts.

I understand why we do it this way but IMO it's pretty confusing, and I wonder if there couldn't be a better solution.

One option could be to introduce an intermediate representation for what "Ctrl on PC and Cmd on Mac" should be (let's say control_command even though it's a terrible name. We could then change all shortcut bindings currently using KEY_COMMAND or wrongly using KEY_CONTROL to use KEY_CONTROL_COMMAND, which would be the only platform-dependent modifier.

The rest would stay exposed as what they are:

• Control (exists both on PC and Mac - different place and possibly different uses, but IMO we could let users decide if/when they want to use it <-- that's the main point which can make my suggestion moot if we think that there is never a use case for Control treated as the same key on both platforms)
• Command (Mac only)
• Meta (PC only)

[bruvzg]

Control (exists both on PC and Mac - different place and possibly different uses

It's in the same place, just different use. Alt/Opt and Meta/Cmd are swapped. It's the same on the other side, Cmd is right after space, and most mac keyboards only have one Ctrl and two Cmd and Opt keys.

One option could be to introduce an intermediate representation for what "Ctrl on PC and Cmd on Mac"

So, get rid of unions and have separate KEY_COMMAND for mac only, KEY_META for PC only, KEY_CONTROL for Ctrl on both and KEY_CONTROL_COMMAND for Ctrl on PC and Cmd on mac, seems reasonable if it's documented well enough and have good UI. I guess it should default to KEY_CONTROL_COMMAND, but not sure what's the best UI to toggle mapping to KEY_COMMAND/KEY_CONTRL/KEY_META.

[bruvzg]

This PR should do the job, but I do not think it's in intuitive for the users. At least adding some not to the description of the InputEvent docs would be nice.

[EricEzaM]

So I added some doco... surprisingly difficult to write since this 'feature' needs to be added because of of something that is hidden from the user (the unions in InputEventWithModifiers).

So you are kind of saying "this is here as a workaround of something you dont know about."

[groud]

I am ok with the feature for now, but yeah, I agree the situation is not ideal. :/

My only concern for now is the function naming, I think it's not explicit enough on what it does.
Maybe something like "is/set_control_command_compatibility(...)" or something like that would be more explicit.

[akien-mga]

So I added some doco... surprisingly difficult to write since this 'feature' needs to be added because of of something that is hidden from the user (the unions in InputEventWithModifiers).

So you are kind of saying "this is here as a workaround of something you dont know about."

Yeah that's why I'm suggesting reworking all this. Writing documentation forced you to expose this hack of a hacky interface as what it is ;)

For anyone interested, we discussed this further on IRC at length: https://freenode.logbot.info/godotengine-devel/20201119#c5881618-c5882583

If there's interest I may formalize all this in a proper proposal. (This shouldn't prevent merging this PR in the meantime to workaround the current limitation.)

[EricEzaM]

It would be ideal to revisit this asap while it is fresh in our minds.

[akien-mga]

My only concern for now is the function naming, I think it's not explicit enough on what it does.
Maybe something like "is/set_control_command_compatibility(...)" or something like that would be more explicit.

Yeah I agree the name is not the clearest. control_command_compatibility is quite a mouthful, but it might be OK.

Aside from that I think the PR is good to go.

[akien-mga]

Further discussion on naming: https://freenode.logbot.info/godotengine-devel/20201119#c5883844-c5883935

Let's go with this.

[akien-mga]

Thanks!