TileSet Collision Editor - buggy behaviour when releasing key combinations in different order

d0mk commented 3 years ago

Bugsquad note: This issue has been confirmed several times already. No need to confirm it further.

Godot version

3.3.4.stable.mono.official.faf3f883d

System information

Windows 10

Issue description

I'm following a YT tutorial on making a 2D game in Godot, and currently I'm adding collision rectangles to my TileSet in the TileSet editor, Collision tab.

I've noticed that the "New Rectangle" button can be activated using "Shift+R" shortcut, so I started using it to make the process faster, but then I've encountered some bugs:

if the selection tool (mouse pointer icon) is active and "Shift+R" is pressed too fast, both icons stay highlighted and clicking on the tile grid has no effect (it acts as if only the selection tool was active)
if the "New Rectangle" tool is active and "Shift+P" (New Polygon) is clicked too fast, both icons stay highlighted and clicking on the grid has the effect of "New Rectangle" tool
if the "New Polygon" tool is active and "Shift+R" is clicked too fast, both icons stay highlighted and clicking on the grid has the effect of "New Polygon" tool

Actually, I think "clicking too fast" just means releasing the keys in different order, for example: if I have the selection tool active and I press Shift, then press R, then release R, then release Shift, the tool is properly changed to "New Rectangle". But if I press Shift, then press R, then release Shift, then release R, the bug appears. So it seems like releasing the keys in certain order matters, that's why doing it too fast may be causing issues.

Steps to reproduce

Open the TileSet editor in the attached project.
Navigate to Collision tab.
Select the "Selection" tool.
Click on any cell on the grid.
Press and hold left Shift.
Press and hold R.
Release left Shift.
Release R.

After these steps, "Selection" and "New Rectangle" icons stay highlighted, clicking on the grid has the effect of "Selection" tool. Same stays true if you try "Shift+P" or start on different tool than "Selection".

Minimal reproduction project

minimal-example.zip

EricEzaM commented 3 years ago

Duplicate of #45033 I think. You did a good job identifying that the order matters - I believe that is the correct diagnosis. Technically the issues are different but the underlying problem is the same between the two.

melodysium commented 2 years ago

Seconding this issue. Note that this behavior also occurs with the "Delete Selected Shape" shortcut (Shift+BackSpace).

Demo of bug attached, using AutoHotKey's OSAK. This script unfortunately doesn't show the sequence "Shift down, R down, Shift up, R up" explicitly, instead highlighting Shift until R is released as well.

https://user-images.githubusercontent.com/6174468/155889014-f3393851-4cf2-48a3-a43e-8528e5ee24fc.mp4

coffe789 commented 2 years ago

I've done some investigating on this issue and I've found it isn't exclusive to the tileset editor, and it applies to every button with ACTION_MODE_BUTTON_RELEASE. If you press the shortcut key for a given button, then press either shift, alt, meta, or ctrl, and then let go of the buttons (letting go of the shortcut key last), letting go of the shortcut is not detected. This explains why the button stays highlighted but doesn't get pressed. Should also apply to #45033

melodysium commented 2 years ago

(heavily edited as of 2hrs after post) I've done some digging myself, and I believe the problem is the following:

when releasing a "condition" key (like Shift),
while also holding a non-"condition" key (like R),
the resulting InputEventKey will be unaware of the non-"conditon" key, only showing the condition key being released.

Thus the solution would seem to be holding a set of all currently pressed keys, and checking through that set when generating InputEventKeys. I'd suspect this is already implemented somewhere (InputDefault::keys_pressed?), but perhaps it isn't working correctly?

I'm out of time for tonight, but I'll (try to) be back later in the week to figure this out some more. I'd love to know, does this bug occur on other OS's? I also believe this is editor-wide, not just 2D.

I'm also a new contributor, so I figure I'll give a bit more context & explain my logic for the above:

I've identified that the "wrong" shortcut produces flawed InputEventKeys from the OS_Windows::KeyEvents processed:

Keypress	OS_Windows::KeyEvent		InputEventKey
	wParam (name)	uMsg	as_text()	event.pressed ? "Down" : "Up"
↓ Shift	0x10 (Shift)	WM_KEYDOWN	Shift	Down
↓ R	0x52 (R)	WM_KEYDOWN	Shift+R	Down
↑ Shift	0x52 (R)	WM_CHAR	(ignored)
	0x10 (Shift)	WM_KEYUP	Shift	Up
↑ R	0x52 (R)	WM_KEYUP	R	Up

whereas the "correct" shortcut produces correct ones:

Keypress	OS_Windows::KeyEvent		InputEventKey
	wParam	uMsg	as_text()	event.pressed ? "Down" : "Up"
↓ Shift	0x10 (Shift)	WM_KEYDOWN	Shift	Down
↓ R	0x52 (R)	WM_KEYDOWN	Shift+R	Down
↑ R	0x52 (R)	WM_CHAR	(ignored)
	0x52 (R)	WM_KEYUP	Shift+R	Up
↑ Shift	0x10 (Shift)	WM_KEYUP	Shift	Up

This flawed InputEventKey is then sent through InputDefault::_parse_input_event_impl and eventually to BaseButton::_unhandled_input, where the shortcut is finally not being triggered.

Zireael07 commented 2 years ago

Yes, I'm 100% sure this is editor-wide, not just tileset collision editor. Can't find the other report atm.

jhaakma commented 2 years ago

This is particularly annoying in the TileSet editor because of how cumbersome it is to add collision to individual tiles.

I've confirmed the issue exists on both Windows 10 and on MacOS.

coffe789 commented 2 years ago

I can confirm this also occurs on Linux (tested using Debian 11)

godotengine / godot