[PATCH] Game cursor state is not consistently reset

[Sergio Fabian Vier]

When you are playing, select a soldier and click over a tree, appear an wood icon. If you press ESC key and quit of the game to main menu, this icon persist in this place

Edit: The same bug can occur when pressing F10 to open the in-game menu

If you want simply reproduce the problem : select one citizen, hovering the mover over a tree, delete this citizen

[(none)]

Milestone Unclassified deleted

[Kieran P]

No longer valid. In the next release, ESC closes menus, it doesn't open them.

[Philip Taylor]

The same bug can occur when pressing F10 to open the in-game menu (the cursor doesn't change back to default unless you move the mouse, and if you somehow exit to the main menu without moving the mouse then I believe it'll be stuck on the wrong cursor since nothing will try to reset it).

I'm not certain what's the best solution. Maybe the cursor changes should be scoped to the current GUI page, so session cursors can't leak out, and/or maybe we should send mouse movement events when showing/hiding pages (even if the mouse didn't really move), so the session code doesn't forget to set its cursor after changes.

[historic_bruno]

Technically this could happen anytime you switch GUI object focus without moving the mouse. Hard to do now, but if we add something like tab focus switching between GUI elements, then you'd potentially see this more often. Maybe scoping the cursor somehow is most correct, but it seems like just as good or better of a solution to send an extra message when GUI focus changes (assume the mouse should move to cause this, even when it doesn't).

I've not looked in great depth at the code responsible for this, but perhaps key presses in addition to mouse movement, could trigger a cursor update.

[Philip Taylor]

With focus changing, e.g. when opening the chat box under the mouse, the cursor doesn't change to the correct (default) cursor until the next input event (e.g. the key release event). I think input.js handleInputBeforeGui is setting mouseIsOverObject which determines whether to show the default cursor or the world-interaction cursor; the problem is that the mouse might become over an object (e.g. when the chat box pops up) without handleInputBeforeGui being called so it doesn't notice.

Maybe we should add some new engine function so updateCursor can tell if the mouse is currently over an object, rather than relying on the value from the last call to handleInputBeforeGui.

I don't think the same solution would work for the issue when exiting to the main menu, though - in that case the session GUI code stops running entirely so there's nothing to revert the cursor.

[historic_bruno]

#1904 was a duplicate of this.

[Yves]

I can't reproduce it anymore as described in this ticket, but I can still reproduce it this way:

1. Start a singleplayer scenario match against the AI
2. Enable the developer overlay (menu->settings->developer overlay)and select "reveal map" and "control all units"
3. Delete all enemy units except one
4. Send your units to kill the remaining enemy while selecting a female citizen and hovering the mouse over a tree
5. Ehen the enemy resigns, the icon won't change

[Nicolas]

If you want simply reproduce the problem : select one citizen, hovering the mover over a tree, delete this citizen

[Yves]

Thanks for working on this ticket!

I think you should use another approach to solve this problem. Resetting the cursor when the number of GUI pages is greater than 1 means we cannot change the cursor in any ingame menus. I haven't found a case where this is currently needed but we could want to change the cursor over tooltips in the manual for example. In addition to that I think there's an underlying issue you didn't cover with this patch. Also the condition is quite arbitrary because the the number of active GUI pages isn't logically related to which cursor should be used.

Btw. I noticed a similar issue that could be related. If you select a unit, drag a box while holding the left mouse button and hit delete before releasing the left mouse button, the selection box will also stay. Part of the problem here is that apparently the "LMB release" event doesn't reach the correct GUI Page.

I also got this when compiling (it should just be SizePages instead of CGUIManager::SizePages):

../../../source/gui/GUIManager.h|62|error: extra qualification 'CGUIManager::' on member 'SizePages' [-fpermissive]|

[Yves]

It's not quite what I meant yet. You have successfully addressed the first problem I mentioned but the condition is still quite arbitrary or too specific when there should be a more general solution. We could want another GUI page that can be opened with a shortcut and there's the same problem again. We currently only get this problem with the messagebox, but the problem does not happen because the page is a messagebox, it happens because there's any other page on top of the session GUI page but the session GUI page still updates the cursor in its tick event. I hope you get what I mean. :)

You could check for "IsTopPage" and then ONLY update the cursor if it's true instead of setting it to "default" if it's false. Then you would still have to care about resetting the cursor to default state when the messagebox opens. In this case it would probably make sense to reset the cursor to default state each time a GUI page changes instead of expecting from each page to care about the cursor even if it doesn't need any other cursor than the default one. Implementing the IsTopPage check could be a bit tricky. If possible it should not require a hardcoded page name parameter.

Another approach that could work is another event. For example there could be an "onActiveTick" event in addition to the onTick event. It would be triggered only if the current page is the top page.

[leper]

Replying to nick2paris: Any updates?

[sanderd17]

Backlogged due to lack of progress

[Jia Henry]

Modifies the patch and makes it compatible with the current revision

[Yves]

This seems to be pretty much the same patch as nick2paris suggested and the same comments I've posted 9 months ago still apply.

[Jia Henry]

Yea, it is, I just modified it to work on the current alpha

[elexis]

Reproduce 1 (Exit game / switch gui page):

1. Get a modified cursor (f.e. by selecting a unit and then hovering a resource)
2. Hit F9 to open the developer console
3. Type leaveGame();

A simple Engine.SetCursor("arrow-default"); before the SwitchGuiPage

Reproduce 2 (Message box / Push GUI page):

1. Select a unit
2. Hover a resource in the center of the screen
3. Press the delete button

Could not reproduce the chat window bug, the server is corrected once the gui object is revealed. Also couldn't produce any other bugs with GUI objects that are not pages (like the diplomacy dialog f.e.).

How to fix: Indeed we should fix this in the engine as the bug could easily reappear when doing something else with cursors and opening pages without requiring keyboard or mouse events.

Notice this requires the hardcoding of the default cursor in the C++ code, defining it in JS or some random file and loading it via C++.

Currently Config.cpp defines the default cursor as test. I suggest to move the default-arrow.png to the test mod and rename the test to default.png. We should probably have a constant for the default cursor name in C++, so that we have to specify the name only once.

To cover as many cases as possible CGUIManager::PushPage of GUIManager.cpp should reset the cursor. As that method is also called from SwitchPage, both cases are handled.

[bb]

Patch made by danger89 here: ​https://gist.github.com/danger89/4972b54d12cbe1ccc2af1878db59ef77

[Melroy van den Berg]

Why is the patch not include in 21??

[elexis]

It was too short before the release for my taste to rewrite the engine part. If we would have discovered any bug with it, it would have delayed the release even further. Since the described bug occurs rather rarely, I didn't want to take that risk.

That being said I think we can commit the patch in the coming days.

[elexis]

In 18933:

Reset the cursor when opening new GUI pages. Patch by danger89, refs #539.
Remove duplicate cursor image and duplicate hardcoded default paths.

[elexis]

Thanks for the patch danger89! Not many people are able and willing to fix bugs in source/gui/ (as you can see from the revision log of that folder). Further patches are always welcome even if it may take months in many cases.

Problems fixed in your patch:

• The default cursor should reside in mod, not public so that the engine can be used without the public mod.
• The default cursor is specified twice in the code, once in Config.cpp, once in GUIManager.cpp, unifying that with a constant
• Reproduced the unexplained crashes you had - we can't use the name default as that is a reserved sprite name, hence default-arrow

Style:

• Changed spaces to tabs.
• Removed pointless comments like // Reset Cursor above ResetCursor();
• GUIManager.h: 2015 -> 2016

Also found that there are 2 places doing cursor="" to hide it (screenshots and loading screen) - that could be abstracted (but who actually cares).

---

TODO: The patch did not address the message box bug. The line in input.js is still called while the message box is opened, setting the cursor onTick with the information from before the box was opened.

[Imarok]

Move tickets to In-game UI as UI & Simulation got some sub components.

[elexis]

In 23149:

Fix rP18933 resetting the cursor after calling the GUI pages script "init" function, refs #539.
Reset the cursor everytime the GUI page is opened, therefore also when hotloading.

The init function shall be able to determine a cursor, such as already present in the loading screen.

Differential Revision: ​https://code.wildfiregames.com/D2417
Tested on: clang 9.0.0, Jenkins/gcc6, Jenkins/vs2015

[Freagarach]

So this is fixed now, elexis? (Asking because I can't reproduce the bugs anymore, but you probably had a reason to ref, not close.)

[elexis]

The switch-page bug is fixed.

The input.js message box TODO above isn't: (1) select unit, (2) hover tree, (3) press delete. Result: wood-gather cursor, Expected result: default cursor.

[Freagarach]

Ah, thanks for clarifying! I read over that one ;(

(I tried the chat-box, which is not a message-box...)