Skip to content

Conversation

@Koyper
Copy link
Contributor

@Koyper Koyper commented Sep 4, 2025

Fixes #77246, Fixes #102081 and Fixes #70361.

This PR fixes the issue where a submenu would not open on hover until the mouse was jiggled. It also adds an additional delay to close a submenu when the mouse is moving to the right and goes across another submenu item. This provides time to reach a long submenu without it being closed by touching another submenu item.

[Edit: The issue below went away after the push eliminating minimum_lifetime_timer. See comments below regarding probable source of the issue.]

There were several things confounding the issue, one of which is mysterious. In _activate_submenu() calling submenu_pum->popup() would intermittently fail, leaving the submenu invisible, even when submenu_pum->is_visible() shows as true. This is fixed by a process_frame, so I added a hack to do this, but I'm sure there is a better way. There should be a fix maybe in Window that would detect the incomplete rendering?

Here's the line that can be improved:

submenu_pum->popup();
// Unpredictably, some submenus won't show even after popup without a process_frame.
get_tree()->connect("process_frame", callable_mp(this, &PopupMenu::_activate_submenu).bind(p_over, p_by_keyboard), CONNECT_ONE_SHOT);
```</s>

@Koyper Koyper requested a review from a team as a code owner September 4, 2025 15:56
@AThousandShips AThousandShips added bug topic:gui cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release labels Sep 4, 2025
@AThousandShips AThousandShips added this to the 4.6 milestone Sep 4, 2025
@bruvzg bruvzg self-requested a review September 4, 2025 16:04
@Koyper
Copy link
Contributor Author

Koyper commented Sep 4, 2025

This const doesn't seem to be used anywhere - can it be removed from popup_menu.h? It also shows up in option_button.h.

	// ATTENTION: This is used by the POT generator's scene parser. If the number of properties returned by `_get_items()` ever changes,
	// this value should be updated to reflect the new size.
	static const int ITEM_PROPERTY_SIZE = 10;

@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch 2 times, most recently from 88a0428 to a251137 Compare September 5, 2025 23:30
@KoBeWi
Copy link
Member

KoBeWi commented Sep 20, 2025

Doesn't seem to fix the issue

godot.windows.editor.dev.x86_64_Kdv2xEbSfA.mp4

@WhalesState
Copy link
Contributor

WhalesState commented Sep 20, 2025

It can be fixed by storing the next submenu that should show and once the timer timeouts we show it here after we hide the previous submenu.

void PopupMenu::_minimum_lifetime_timeout() {
	close_allowed = true;
	// If the mouse still isn't in this popup after timer expires, close.
	if (!activated_by_keyboard && !get_visible_rect().has_point(get_mouse_position())) {
		_close_pressed();
	}
}

Or by having smaller wait time for both timers, I have tried 0.01 and it's more user friendly than 0.3.

	submenu_timer = memnew(Timer);
	submenu_timer->set_wait_time(0.3);
	submenu_timer->set_one_shot(true);
	submenu_timer->connect("timeout", callable_mp(this, &PopupMenu::_submenu_timeout));
	add_child(submenu_timer, false, INTERNAL_MODE_FRONT);

	minimum_lifetime_timer = memnew(Timer);
	minimum_lifetime_timer->set_wait_time(0.3);
	minimum_lifetime_timer->set_one_shot(true);
	minimum_lifetime_timer->connect("timeout", callable_mp(this, &PopupMenu::_minimum_lifetime_timeout));
	add_child(minimum_lifetime_timer, false, INTERNAL_MODE_FRONT);

Edit: minimum_lifetime_timer is the issue, but submenu_timer can be changed to 0.1 instead of 0.01 to avoid showing it while moving the mouse over the items fast.

@WhalesState
Copy link
Contributor

WhalesState commented Sep 20, 2025

Also this may be related to why the popup menu didn't receive the motion event when the mouse entered.

@Koyper
Copy link
Contributor Author

Koyper commented Sep 20, 2025

Doesn't seem to fix the issue

The whack-a-mole continues - thanks for finding that. If you set the submenu timer to 0.1 does it still act the same per @WhalesState?

Is there a movement sequence you can replicate this on an Editor menu? I will set this up for myself and go back on the hunt.

Edit: minimum_lifetime_timer is the issue, but submenu_timer can be changed to 0.1 instead of 0.01 to avoid showing it while moving the mouse over the items fast.

I agree that 0.1 feels better - any reason we can't set the default to 0.1 instead of 0.3?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 20, 2025

thanks for finding that

I just tested with the MRP from the issue you linked 🙃

If you set the submenu timer to 0.1 does it still act the same

Yes.

@Koyper
Copy link
Contributor Author

Koyper commented Sep 20, 2025

I just tested with the MRP from the issue you linked 🙃

That's pretty funny! My notoriety must be in reinventing the wheel. I've been testing with a much fancier menu that doesn't exhibit the issues as obviously for an as yet unknown reason.

@Koyper
Copy link
Contributor Author

Koyper commented Sep 20, 2025

I just tested with the MRP from the issue you linked 🙃

Okay, go to project settings and disable embed_subwindows and everything works perfectly. I should have tested this properly with embedded subwindows. This might indeed be related to something like #110093.

Also this may be related to why the popup menu didn't receive the motion event when the mouse entered.

There seems to be an issue regarding incorrect determination of the mouse leaving the safe area.

@WhalesState
Copy link
Contributor

There seems to be an issue regarding incorrect determination of the mouse leaving the safe area.

Yes, The issue is that the parent window only process one event while another window has focus, but when you disable embedding sub-windows they become just fake windows extending Control node so the mouse events works fine.

@WhalesState
Copy link
Contributor

WhalesState commented Sep 20, 2025

Okay, go to project settings and disable embed_subwindows and everything works perfectly. I should have tested this properly with embedded subwindows. This might indeed be related to something like #110093.

Also this issue is in both embedded and non-embedded windows so maybe you forgot to revert back the timer changes while testing or you test with the changes from this PR, i have tested with editor submenus in master and the issue also exists there by default and the editor is not embedding subwindows.

Screencast.From.2025-09-20.23-54-36.mp4

@WhalesState
Copy link
Contributor

WhalesState commented Sep 20, 2025

Also check what happens here, the root window receives one mouse event when another window is focused but when you focus it no other window receives any input, the settings window is exclusive so the parent window shouldn't receive any input at all.

Screencast.From.2025-09-20.23-45-59.mp4

@Koyper
Copy link
Contributor Author

Koyper commented Sep 20, 2025

i have tested with editor submenus in master and the issue also exists there by default and the editor is not embedding subwindows.

The PR fixed the issue with non-embedded windows and the Editor, though I found a couple of bugs there that I'm taking care of. What I've got working now for embedded widows is to emit the popup_hide signal when a submenu closes, which then creates an InputEventMouseMotion event that forces the parent menu to process a mouse movement over the currently hovered item. This seems to offer a solution for the time being. I will set the default submenu wait time to 0.1 and we can test it out to see if it's satisfactory. I think the ergonomics are better with the faster response.

What you demonstrated regarded no window receiving input, might certainly be related to the closed submenu blocking input to the parent menu, which is the source of the blocked submenu issue. This is a deeper problem with Window - fixing that might also take care of the "hung" activation of submenus that requires that callable loop to fix.

@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch from a251137 to b51c6d2 Compare September 24, 2025 16:56
@Koyper Koyper requested a review from a team as a code owner September 24, 2025 16:56
@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch from b51c6d2 to 49b8f45 Compare September 24, 2025 17:12
@Koyper
Copy link
Contributor Author

Koyper commented Sep 24, 2025

After a lot of frustration, I finally had to get rid of the minimum_lifetime_timer as it was the source of many of the issues. I replaced it with close_suspended_timer, which is only set in the parent menu, whereas minimum_lifetime_timer was being set in each submenu. The code now enforces submenus only being activated after any open submenus are closed. This eliminated the need for the hack double-calling _activate_submenu(), which I believe was being caused by a race condition where a prior submenu was in a closing state while the next was being activated.

The other major problem was the fixes for issues with embedded vs. non-embedded subwindows were completely different. The only way to get embedded subwindows to work was connecting popup_hide to spoof a mouse input on the hovered menu item that's triggered by the submenu closing.

When the mouse moves out of a submenu item, it's last relative motion state inside its menu item is preserved, and if it's moving into the lower right quadrant (or lower left if right-to-left layout), it will start the close_suspended_timer which runs for 0.5 seconds to allow the mouse to sweep into the open submenu across other menu items without triggering them to open or close the open submenu. This timing is modeled after the behavior of MacOS system menus, which seems reasonable.

Finally, I reduced the default submenu delay time from 0.3 to 0.1 seconds, as that feels more natural and responsive, and the fixes above make it unnecessary to prolong the opening of the submenu.

@akien-mga akien-mga removed the cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release label Oct 7, 2025
@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch from 49b8f45 to fd98bf6 Compare October 7, 2025 14:13
@Koyper
Copy link
Contributor Author

Koyper commented Oct 7, 2025

This push adds support for right-to-left layouts for the direction testing of mouse movements toward an open submenu.

@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch from fd98bf6 to 39e2f25 Compare October 7, 2025 17:05
@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch from 39e2f25 to c8aae8e Compare October 10, 2025 14:22
@Koyper
Copy link
Contributor Author

Koyper commented Oct 10, 2025

This push handles submenu positions where the right-to-left layout setting is overwritten because of inadequate screen space for the submenu, so the submenu position is left vs. right or the converse.

@kitbdev
Copy link
Contributor

kitbdev commented Oct 11, 2025

I tried visualizing the dot products and different menus. Submenus can be above the cursor when tall near the bottom, and the calculated range doesn't match:

image

Moving toward the top of this submenu closes it.

I found references of how this is done elsewhere https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown , https://codepen.io/popov654/pen/XyGXzb
It shows using a triangle between the mouse and the relevant corners of the submenu, and is much more accurate.

@Koyper
Copy link
Contributor Author

Koyper commented Oct 11, 2025

It shows using a triangle between the mouse and the relevant corners of the submenu, and is much more accurate.

I like this idea! I had not considered that case of the menu originating near the bottom of the screen. I had assumed the submenu would always project below the menu item. Shouldn't be too much work to calculate those vectors aiming at the corners. Nice also to see the quadrant orientation - thanks.

This also future-proofs having submenus that pop with the selected submenu choice aligned with the parent menu item. It's something we don't do now, but would be nice to implement someday.

@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch 3 times, most recently from a6617fb to 71228b8 Compare October 25, 2025 20:01
@Koyper
Copy link
Contributor Author

Koyper commented Oct 25, 2025

This push improves the mouse movement direction detection so the detection angles are based on the position and height of the submenu.

I included debug draw lines to visualize the detection angles from the mouse position, which you can see in the screen recordings below. The detection now works correctly for both left and right side submenus, and submenus shifted by any amount vertically.

Screen.Recording.2025-10-25.at.1.42.55.PM.mov
Screen.Recording.2025-10-25.at.1.39.15.PM.mov

@YeldhamDev
Copy link
Member

The PR makes submenus close instantly if the cursor leaves the item.

@Koyper
Copy link
Contributor Author

Koyper commented Oct 27, 2025

The PR makes submenus close instantly if the cursor leaves the item.

The intended behavior is that the submenu stays open for 0.5 seconds if you are moving the mouse toward the submenu upon exiting the parent menu item. Otherwise, the submenu should close instantly when exiting the parent item, because you are moving downward or otherwise away from the submenu. Are you observing something else like a bug?

Do you see some other behavior other than what's shown in the last screen recordings above?

@YeldhamDev
Copy link
Member

YeldhamDev commented Oct 27, 2025

The intended behavior is that the submenu stays open for 0.5 seconds if you are moving the mouse toward the submenu upon exiting the parent menu item. Otherwise, the submenu should close instantly when exiting the parent item, because you are moving downward or otherwise away from the submenu. Are you observing something else like a bug?

The actual feeling is that the submenu closes faster than the current master. It also feels that the submenu opens faster as well.

@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch 4 times, most recently from eead1dc to 770c23e Compare November 2, 2025 19:35
@Koyper Koyper force-pushed the fix_popup_menu_item_wont_open branch from 770c23e to 6107c5c Compare November 2, 2025 19:47
@YeldhamDev
Copy link
Member

YeldhamDev commented Nov 2, 2025

  • Moving the cursor to the submenu makes the hover in the parent item blink, because now when the cursor is in the border it removes the hover.
  • Moving the cursor around the border while a submenu is open causes a crash.

@Koyper
Copy link
Contributor Author

Koyper commented Nov 2, 2025

  • Moving the cursor to the submenu makes the hover in the parent item blink, because now when the cursor is in the border it removes the hover.

Thanks for comments. I'm working on a fix for this - the parent item also can blink when the mouse moves across another item on the way to the submenu.

  • Moving the cursor around the border while a submenu is open causes a crash.

I can't reproduce this - what platform is it happening on? Is this on an Editor menu?

@YeldhamDev
Copy link
Member

I can't reproduce this - what platform is it happening on? Is this on an Editor menu?

Just go up-and-down with the cursor in the border while the submenu is open, should crash eventually.

image

@Koyper
Copy link
Contributor Author

Koyper commented Nov 3, 2025

Will do.

By-the-way, the new default popup total delay is 0.23 seconds, which is the delay setting of 0.2 seconds, plus the 30 msec delay added before determining the mouse movement vector away from the parent item. This made the vector much more reliable. I more carefully measured the delays on MacOS and now the popup behaves just like it.

@Koyper
Copy link
Contributor Author

Koyper commented Nov 3, 2025

Just go up-and-down with the cursor in the border while the submenu is open, should crash eventually.

Do you get any log output on crash? This might be OS-dependent. I'll see if I can reproduce it on Windows.

@YeldhamDev
Copy link
Member

ERROR: FATAL: Index p_index = -1 is out of bounds (size() = 14).
   at: get (./core/templates/cowdata.h:193)

================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.6.dev.custom_build (e0cb03a3710e97d4f8b9201ceb1a36c7adac8eeb)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[1] /lib64/libc.so.6(+0x1a070) [0x7f90e1cb6070] (??:0)
[2] CowData<PopupMenu::Item>::get(long) const (/var/home/michael/Development/Projects/godot/core/templates/cowdata.h:193 (discriminator 13))
[3] Vector<PopupMenu::Item>::operator[](long) const (/var/home/michael/Development/Projects/godot/core/templates/vector.h:134)
[4] PopupMenu::_activate_submenu(int, bool) (/var/home/michael/Development/Projects/godot/scene/gui/popup_menu.cpp:348 (discriminator 1))
[5] PopupMenu::_submenu_timeout() (/var/home/michael/Development/Projects/godot/scene/gui/popup_menu.cpp:467)
[6] void call_with_variant_args_helper<PopupMenu>(PopupMenu*, void (PopupMenu::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (/var/home/michael/Development/Projects/godot/core/variant/binder_common.h:228)
[7] void call_with_variant_args<PopupMenu>(PopupMenu*, void (PopupMenu::*)(), Variant const**, int, Callable::CallError&) (/var/home/michael/Development/Projects/godot/core/variant/binder_common.h:338)
[8] CallableCustomMethodPointer<PopupMenu, void>::call(Variant const**, int, Variant&, Callable::CallError&) const (/var/home/michael/Development/Projects/godot/core/object/callable_method_pointer.h:107)
[9] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/var/home/michael/Development/Projects/godot/core/variant/callable.cpp:57)
[10] Object::emit_signalp(StringName const&, Variant const**, int) (/var/home/michael/Development/Projects/godot/core/object/object.cpp:1342)
[11] Node::emit_signalp(StringName const&, Variant const**, int) (/var/home/michael/Development/Projects/godot/scene/main/node.cpp:4166)
[12] Error Object::emit_signal<>(StringName const&) (/var/home/michael/Development/Projects/godot/core/object/object.h:967)
[13] Timer::_notification(int) (/var/home/michael/Development/Projects/godot/scene/main/timer.cpp:68)
[14] Timer::_notification_forwardv(int) (/var/home/michael/Development/Projects/godot/scene/main/timer.h:36)
[15] Object::_notification_forward(int) (/var/home/michael/Development/Projects/godot/core/object/object.cpp:995)
[16] Object::notification(int, bool) (/var/home/michael/Development/Projects/godot/core/object/object.h:921)
[17] SceneTree::_process_group(SceneTree::ProcessGroup*, bool) (/var/home/michael/Development/Projects/godot/scene/main/scene_tree.cpp:1201)
[18] SceneTree::_process(bool) (/var/home/michael/Development/Projects/godot/scene/main/scene_tree.cpp:1274 (discriminator 2))
[19] SceneTree::process(double) (/var/home/michael/Development/Projects/godot/scene/main/scene_tree.cpp:708)
[20] Main::iteration() (/var/home/michael/Development/Projects/godot/main/main.cpp:4768 (discriminator 3))
[21] OS_LinuxBSD::run() (/var/home/michael/Development/Projects/godot/platform/linuxbsd/os_linuxbsd.cpp:991 (discriminator 1))
[22] ./bin/godot.linuxbsd.editor.dev.x86_64(main+0x1bd) [0x4232f5] (/var/home/michael/Development/Projects/godot/platform/linuxbsd/godot_linuxbsd.cpp:118)
[23] /lib64/libc.so.6(+0x3575) [0x7f90e1c9f575] (??:0)
[24] /lib64/libc.so.6(__libc_start_main+0x88) [0x7f90e1c9f628] (??:0)
[25] ./bin/godot.linuxbsd.editor.dev.x86_64(_start+0x25) [0x406ab5] (??:?)
-- END OF C++ BACKTRACE --
================================================================

@Koyper
Copy link
Contributor Author

Koyper commented Nov 3, 2025

Excellent - this means it's not a platform issue, and I know where to look...

@Koyper
Copy link
Contributor Author

Koyper commented Nov 3, 2025

I've fixed the crashing and other issues, and added several error checks to prevent any similar crashes. The crash culprit was the submenu timer was running as another submenu was closed. I'm trying to wrap things up regarding the parent item highlight flickering, and ran into this bug - #112358. I can work around it, but it would be nice to sort out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release topic:gui

Projects

None yet

7 participants