Fix some keys triggering their actions twice in GridMap

[geowarin]

This is a bug introduced by #79529.

Some keys (not all) trigger the associated action twice: one is triggered by the forward_spatial_input_event method as intended in the PR and another time from the menu event handler (not intended, we were supposed to prevent further inputs).

The original code contained the accept_event() statement but it was removed after the code review.
Apologies for not testing this thoroughly... 😞

@KoBeWi Can I have your input on this, I'm not sure if it's the right way but it does fix the bug.

[geowarin]

So this is a pretty big regression, I really hope we can find a solution to this in time for 2.2!

Regarding the implementation:

It is impossible for the gridmap plugin to know which of its keybindings are used by other plugins so the solution I found is everytime a key is pressed in the grimap, if it's mapped to a GridMap action, trigger this action from forward_spatial_input_event and stop the propagation.

I'm not familiar enough with the event propagation in Godot but it seems to me that both calling accept_event() and returning EditorPlugin::AFTER_GUI_INPUT_STOP from this method are required to really prevent propagation.

I'm not sure why, for instance, accepting the event in the menu option handler does NOT do the same thing:

void GridMapEditor::_menu_option(int p_option) {
       // this does not work
	options->accept_event();
...
}

---

This implementation does have a side effect though.
If we take the delete key as an example, while having the gridmap selected, the user could use this key for two different actions:

• deleting a selection within the GridMap (made by holding shift while dragging in the editor)
• deleting the GridMap node in the scene tree

Without the code from #79529, while having the editor focused, both actions would be triggered, which is very annoying.
Some other conflicting actions would randomly do something else not related to the gridmap, which, I guess would come down to how different inputs are handled within different editor plugins.

With the code from #79529, while having the editor focused, the gridmap action will always take precedence.
It's still possible to delete the node by refocusing the scene tree.

I think this approach, while not perfect is a massive improvement from the previous state but I'd really like some maintainer's opinion about the matter!

It also feels a bit like a hack if I'm being totally honest...

[KoBeWi]

I think the main issue here is that 2D and 3D editors handle events differently. 2D editor will accept events automatically:

godot/editor/plugins/canvas_item_editor_plugin.cpp

Lines 2594 to 2596 in 3ed4497

	if (accepted) {
	accept_event();
	}

While 3D doesn't:

godot/editor/plugins/node_3d_editor_plugin.cpp

Lines 1614 to 1616 in 3ed4497

	if (discard == EditorPlugin::AFTER_GUI_INPUT_STOP) {
	return;
	}

I wasn't able to find an example of using accept_event() event in another plugin, but seems like only GridMap is using keyboard shortcuts (at least the way you implemented it).

I do think the current behavior is correct, as users expect that shortcuts work in the context they are currently using (as proven by countless issues about deleting nodes while editing some unrelated thing). TileMap also uses similar code since #80317

Given the aforementioned inconsistency and lack of keyboard examples in other plugins, I think this fix is fine.

[geowarin]

I think the main issue here is that 2D and 3D editors handle events differently. 2D editor will accept events automatically:

Oh woah. This is a bit unexpected.
Does the nature of the things we do in the 3D editor explains this difference in behavior?

I'm guessing changing it would break everyone's workflow at this point.

accept_event is very seldom used in Godot code but it seems to have a lot more popularity in the wild:
https://github.com/search?utf8=%E2%9C%93&q=+accept_event+language%3AGDScript+&type=code

However, the real problem here is that ED_SHORTCUTs do not stop event propagation...
I can't really think of a case where that would be a desirable behavior.

[KoBeWi]

I think the event first goes to the editor and then to the menu; in opposite order it would be fine, because the menu accepts events properly.

[akien-mga]

Thanks!