Make GridMap shortcuts editable and not conflict with other plugins

[geowarin]

This allows users to change shortcuts for the gridmap and also makes them more discoverable.

It now uses physical keycodes, where appropriate, to make the tool more accessible to non-qwerty keyboard users.

This also prevents shortcut events to be forwarded to the 3D editor plugin, avoiding conflicts.

fixes: #67497.
supersedes: #79377
supersedes: #75243
Closes: godotengine/godot-proposals#3839

[geowarin]

When using editor shortcuts, we notice that most of the shortcuts of the gridmap are inaccessible because they are conflicting with the 3D Editor shortcuts:

Previous floor (Q) is the same as Select Mode
Next floor (E) is the same as Rotate Mode, ...

However, I found a way to prevent forwarding the shortcuts events to the node_3d_editor_plugin by iterating on all the menu item shortcuts in forward_spatial_input_event and accepting the events when appropriate.

I wonder if it's the right way to do it. Looking forward (no pun intended) for a review.

[KoBeWi]

The implementation looks alright, but the physical shortcuts are questionable. We don't use physical keys for shortcuts (see godotengine/godot-proposals#7303). Also your choice of physical keys is rather arbitrary; at best it only helps users with a specific layout (I assume AZERTY or similar).

However, I found a way to prevent forwarding the shortcuts events to the node_3d_editor_plugin by iterating on all the menu item shortcuts in forward_spatial_input_event and accepting the events when appropriate.

I don't remember seeing menu shortcuts iterated like this before, but it looks correct.

[geowarin]

Physicals are used because I'm assuming theses keys are meant to be in close physical proximity.

QWE (floors and clear rotation)
ASD (rotation x/y/z)
ZXC (edit axis x/y/z)

Are clearly meant to be grouped together. I agree that making all of them (rather than cherry-picked ones) physical would make more sense.

You're right I'm using azerty!
For me it makes the tool way more usable out of the box.
Anyway, this is just a personal opinion. Since the keys become configurable, I can remove that part of the code if you wish 😄

[KoBeWi]

The implementation looks alright.

I still think that physical keys shouldn't be used. CC @Calinou

[geowarin]

@KoBeWi
I'm willing to remove the physical keys but I want to defend my case one last time with pictures.
And if you're not convinced, I will remove them 😄

Here is what the keyboard shortcuts look like on a QWERTY keyboard:

Notice how they are nicely laid out.

Red keys control the floor elevation and clear the mesh rotation.
Green control the rotation of the mesh.
Blue control the rotation of the grid.

Here it what it looks like in AZERTY:

It's super confusing!

I agree with you that physical keycodes should be avoided most of the time but I think this feature, just like the FreeCam, justify their use.

What do you think?

[KoBeWi]

I borrowed your idea to open another PR (#80317) and actually discovered a bug that also exists in your code.

You can use matches_event() directly on the InputEvent, because it will also match released and echo events. So e.g. Cursor Rotate shortcut will rotate the mesh by 180° instead of 90, because it will also use both the press and release events. I solved it by using a condition before the for loop:

Ref<InputEventKey> k = p_event;
if (k.is_valid() && k->is_pressed() && !k->is_echo()) {

You should do the same.

[geowarin]

@KoBeWi Done! Thanks for the suggestion.
@Calinou Can I get your opinion on the use of physical keys?

[akien-mga]

I think physical keys make sense here.

The criterion is simple:

• Is the shortcut meant to be linked to a letter/symbol, regardless of its position on the keyboard? If yes, then use "normal" key that follows the keyboard layout. E.g. "M" for toggling Minimap. Or Ctrl+Z for Undo because that's well established after decades of use on different keyboard layouts.
• Is the shortcut meant to be located in a specific position, such as WASD for first-person movement? If yes, it should use physical keys. There's no link between "W" and "Up", aside from it being positioned on top of "ASD" on a QWERTY keyboard.

So here the shortcuts seem to be positioned as an equivalent to the NumPad and they should stay there, not move around arbitrarily based on the keyboard layout (AZERTY is a common issue, but test DVORAK...).

[akien-mga]

Thanks!