godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.44k stars 21.07k forks source link

Gridmap selection hotkeys no longer working since 4.2 #87107

Closed TiagoKuribara closed 3 months ago

TiagoKuribara commented 9 months ago

Tested versions

Reproducible in 4.2 onwards. it was working in Godot 3, and it was working up until 4.1.3

System information

Godot v4.2.stable - Windows 10.0.22621 - Vulkan (Forward+) - integrated AMD Radeon(TM) Graphics (Advanced Micro Devices, Inc.; 31.0.12044.56000) - AMD Ryzen 7 5700U with Radeon Graphics (16 Threads)

Issue description

You can hold shift and hold the left mouse button to select cells in a gridmap. While holding shift, you used to be able to be able to select multiple floors by also pressing the "e" or "q" keys, as seem in the video below:

https://www.youtube.com/watch?v=xond8D3mf2w&feature=youtu.be

But since 4.2, this stopped working, and now nothing happens, as seem in the video below:

https://www.youtube.com/watch?v=S2RFTIe3XoQ&feature=youtu.be

I am pressing "q" and "e" in the video, but nothing happens

Steps to reproduce

Minimal reproduction project (MRP)

N/A

akien-mga commented 9 months ago

We keep getting conflicting reports for this in #67497, where for some it worked before and no longer works in 4.2, and for others it didn't work in 4.1 and it works now. Needs investigating.

Which keyboard layout are you using?

Calinou commented 9 months ago

I can confirm this in 4.2.stable (Linux, fr-oss AZERTY keyboard layout).

In comparison, you could do this in 4.1.2.stable:

https://github.com/godotengine/godot/assets/180032/53980955-2fbd-46ef-b584-e6dcb67d380c

The issue appeared between 4.2.dev3 and 4.2.dev4. I'll look into bisecting this.

Edit: I bisected the regression to f80f4eb390e22b3c20b2697ca15432d91b6f1de4. cc @geowarin

KoBeWi commented 9 months ago

Might be caused by #79529

TiagoKuribara commented 9 months ago

We keep getting conflicting reports for this in #67497, where for some it worked before and no longer works in 4.2, and for others it didn't work in 4.1 and it works now. Needs investigating.

Which keyboard layout are you using?

Im using a brazilian qwerty keyboard

geowarin commented 9 months ago

Edit: I bisected the regression to f80f4eb. cc @geowarin

Yeah, I never knew that was a feature! My bad.

Seems the code responsible for this feature is here: https://github.com/godotengine/godot/blob/1a1c54283653fa021ab604dbe29396b26c58b5ad/modules/gridmap/editor/grid_map_editor_plugin.cpp#L759-L770

But it can't be executed because of the block that I added a few lines above: https://github.com/godotengine/godot/blob/1a1c54283653fa021ab604dbe29396b26c58b5ad/modules/gridmap/editor/grid_map_editor_plugin.cpp#L747-L757

(this will return if "q" or "e" are pressed)

The two blocks of code probably have to be merged now (or swapped?), I'll take a stab at it tomorrow

geowarin commented 9 months ago

Mmm.... turns out that adding ED_SHORTCUT to the options menu makes this dead code.

(Key)options->get_popup()->get_item_accelerator(options->get_popup()->get_item_index(MENU_OPTION_PREV_LEVEL))

return KEY::NONE

My first idea was to use get_item_shortcut() and then match it with shortcut->matches_event(p_event) but they don't match.

My guess here is since, the shift key also is pressed when the event is triggered, it is not a match for the shortcut (that does not include the shift key).

full code that I tried for clarity:

if (k->is_shift_pressed() && selection.active && input_action != INPUT_PASTE) {
    const Ref<Shortcut> &shortcut1 = options->get_popup()->get_item_shortcut(options->get_popup()->get_item_index(MENU_OPTION_PREV_LEVEL));
    if (shortcut1.is_valid() && shortcut1->matches_event(p_event)) {
        // NOPE
    }
}

Any idea? @Calinou @KoBeWi

KoBeWi commented 9 months ago

I investigated and you don't need to hold Shift all the time, only to start selecting. While selection is active, you can release Shift and use the shortcuts normally. Although it doesn't work correctly, the code quoted above needs to be moved somewhere else.