Opened 13 years ago

Last modified 8 years ago

#850 new defect

[PATCH] GUI - Improve mouse button 'release' event handling

Reported by: historic_bruno Owned by:
Priority: Nice to Have Milestone: Backlog
Component: Core engine Keywords:
Cc: Yves Patch:

Description

The GUI engine is subject to a number of bugs related to the handling of mouse button releases. Currently mouse events are only sent to the GUI object that has focus, which is adequate for mouse button presses. The problem comes from not handling the subsequent release:

In the minimap if you left-click-drag, the camera moves but if you go outside the minimap area and release the button, the minimap is never notified since it lost focus and will continue to expect mouse movement (in fact you must re-click and release inside the minimap to correct this.) A similar problem occurs for bandboxing selection in windowed mode and in fact anywhere that mouse button release is expected.

At a guess, we may want to send button release events to all GUI objects instead of only the one with focus. Trying to reset the button state locally upon losing focus is a hack that doesn't really work (imagine left-click-dragging in the minimap and then accidentally going 1 pixel outside the minimap area, well you've lost focus and so even though you're still holding down the button, it would lose function).

Attachments (3)

minimap_ticket850.patch (3.5 KB ) - added by Yves 13 years ago.
Ticket_850_V2.patch (26.4 KB ) - added by Yves 12 years ago.
Changelog.txt (1.2 KB ) - added by Yves 12 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 by Philip Taylor, 13 years ago

Maybe some kind of 'mouse capture' functionality would help? e.g. when you click on the minimap it enables mouse capture, which means all mouse events go that object instead of whatever object is underneath the cursor, until you release the button and the minimap uncaptures the mouse. (Plus some suitable handling of edge cases like the minimap disappearing (switching to cinematics mode, or exiting the game via hotkeys, etc) while it has the mouse captured.)

comment:2 by Yves, 13 years ago

Owner: set to Yves

Let's give it a try.

comment:3 by Yves, 13 years ago

Keywords: review added
Milestone: BacklogAlpha 7

by Yves, 13 years ago

Attachment: minimap_ticket850.patch added

comment:4 by Yves, 13 years ago

Summary: GUI - Improve mouse button 'release' event handling[PATCH] GUI - Improve mouse button 'release' event handling

Each GUIObject can now define if it should still receive mouse input after left-click-dragging and leaving the object. The GUI resets the object capturing the mouse after releasing the left mouse button. This fixes the minimap issue.

comment:5 by Yves, 13 years ago

Cc: Yves added

comment:6 by Kieran P, 13 years ago

Milestone: Alpha 7Alpha 8

comment:7 by historic_bruno, 12 years ago

Keywords: review removed

Thanks for the patch.

Will we only want mouse capturing on left mouse button press? The logic is a bit confusing in the patch, because the minimap sets m_CaptureMouseAfterLMBClick and the GUI engine would always release mouse capture on a left mouse button release, but the check for mouse capture is called DoesCaptureMouse, implying a more general solution. We might want right-click-and-drag behavior at some point, so I think mouse capture should be more flexible, if it doesn't introduce too many problems.

One issue introduced by the patch is that after the minimap has captured input, it still calculates the mouse position relative to the minimap bounds when moving the camera, but the mouse position can easily exceed the bounds. Instead the minimap should not adjust the camera position if the mouse is outside those bounds (to prevent the camera moving very far off-world).

The patch solves the focus loss problem in fullscreen mode, but not windowed mode if the mouse goes outside the game window (during a dragging action with a button release, because the engine never receives that event). Maybe it's possible to somehow simulate the mouse release event in that case, once the game window regains focus. That's probably outside the context of your patch, but I do want to mention it for completeness.

in reply to:  7 comment:8 by Philip Taylor, 12 years ago

Replying to historic_bruno:

One issue introduced by the patch is that after the minimap has captured input, it still calculates the mouse position relative to the minimap bounds when moving the camera, but the mouse position can easily exceed the bounds. Instead the minimap should not adjust the camera position if the mouse is outside those bounds (to prevent the camera moving very far off-world).

I think it should probably adjust the camera position to the point on the edge of the map that is nearest the mouse, since that seems more intuitive behaviour. (Since our maps are all circular now, that effectively just involves clamping the length of the vector from the center of the minimap to the mouse, so it can't exceed the radius of the minimap.)

The patch solves the focus loss problem in fullscreen mode, but not windowed mode if the mouse goes outside the game window (during a dragging action with a button release, because the engine never receives that event).

There's also the case of a new GUI page (e.g. an options screen) becoming active while the user is dragging, so the mouse-up event goes to the new GUI page instead of the old one. Is there some SDL function that reports the current status of the mouse button, so the GUI could check it every frame (after processing all the queued mouse events) and break out of capture mode in these edge cases? Also probably ought to check the hiddenness of the capturing object, so the mouse gets released if that object disappears.

comment:9 by historic_bruno, 12 years ago

Milestone: Alpha 8Backlog

comment:10 by Kieran P, 12 years ago

Milestone: BacklogAlpha 9

comment:11 by Yves, 12 years ago

Keywords: review added
Status: newassigned

I now have a much more generic solution for capturing input and the minimap-problems is solved much better than with the last patch... but I had to change quite a lot. See the changelog and the patch for details.

by Yves, 12 years ago

Attachment: Ticket_850_V2.patch added

by Yves, 12 years ago

Attachment: Changelog.txt added

comment:12 by historic_bruno, 12 years ago

Keywords: review removed

Mostly the patch solves the problems as described, but here's a few things I noticed during testing:

  • Got this warning when compiling:
    ps\source\gui\IGUIScrollBar.h(161): warning C4505: 'IGUIScrollBar::Draw'
    : unreferenced local function has been removed
    
  • The patch is slightly outdated since [11033], it doesn't take much to fix it, just replacing a few pNearest references with pActive (have to be careful though because some of the code is #ifdef'd out now and therefor won't compile to notify you of errors. I also got a rejected hunk on the changes to Camera.cpp.
  • Map bounds are broken for square maps, I think because of the code deleted from Camera.cpp, was that changed to fix something else? With square maps it's possible to be within the minimap circle but outside the map bounds - that's the only reason I can think of for the clamping that was there. You can test this on the "Massacre of Delphi" map.
  • I also noticed map bounds are broken in other cases not tied to your patch. Moving the camera "upward" allows it to go far off world for some reason, but not if you move it downward or to the sides. Square maps are even worse, I guess because they don't get tested as often. This probably needs a new ticket if it's not an obvious fix.
  • In MiniMap.cpp, isn't m_Clicking a redundant variable now? Notice it mirrors the value of m_IsCapturing. As for the GUIM_MOUSE_DBLCLICK_LEFT case, it does set only m_Clicking, but I think that's a mistake and it should set m_IsCapturing (since it's in response to a mouse up event). Though it's hard to test because double clicking doesn't do anything yet. Any reason not to replace m_Clicking with m_IsCapturing? We should decide if it's possible and sensible to be clicking but not capturing or vice versa.
  • For the minimap camera clamping, I think you should only have to recalculate CornerToMouse if the mouse position exceeded the bounds. Even though it works currently because two variables cancel out, it's a bit confusing. Similarly, CenterToMouse and CenterToCorner values are only needed in the clamping logic, moving them would clean things up a bit.

Minor issues:

  • Typo in IGUIButtonBehavior.cpp line 57 "caputre" > capture
  • Same type in IGUIObject.h line 461. I think the comment is slightly confusing because it says the object "requires" mouse input to be captured, but really you're checking the state of the object, it might be more accurate to say: "Check if the object is currently capturing mouse events"
  • The member variable indentation and comments in IGUIObject.h are pretty weird and unconventional compared to the rest of the code base, but that's not your fault :)
  • In CGUI.h, you might as well give full comments for the getter functions you added, so that it shows up correctly in Doxygen, or if it's painfully obvious what they do, they could have no comment. Still I think it's usually worth taking up a few lines with proper comments :)
  • In MiniMap.cpp, local variables should start with lower case letters, otherwise they get slightly confused with other things like class names.
  • It also says on line 162 that it's recalculating CenterToMouse but the value gets stored in CornerToMouse, I'm assuming the comment is the typo.
  • Would "origin" be a better word than "corner"? I don't care too much either way, but people might wonder which corner is referred to. For that matter they might also wonder where the origin is :)
  • In mainmenu.js:EnableUserReport(), function parameters should also start with lower case letters.

comment:13 by ben, 12 years ago

In 11044:

Fixes camera constraints by clamping the camera focus point to within the map bounds instead of outside, since the constraints are partially based on terrain height. Should improve scrolling behavior on both circular and square maps. Refs #656, #850

comment:14 by Kieran P, 12 years ago

Milestone: Alpha 9Alpha 10

comment:15 by Kieran P, 12 years ago

Priority: Should HaveNice to Have

comment:16 by Kieran P, 12 years ago

@Yves Hey. How's it going with implementing the suggestions from Ben (see http://trac.wildfiregames.com/ticket/850#comment:12). It's be nice to commit your patch once the changes have been made.

comment:17 by Yves, 12 years ago

Thank you for the review. I'm now using the existing code for camera restrictions on circular maps and square maps. After removing the code from camera.cpp it works quite well (also with square maps). It calculated the camera position based on a wrong angle and produced other errors when the focus point was cut at the map's size on one or two axes.

There are still some problem when the minimap is combined with keyboard commands for camera rotation an movement and I've noticed there's a new patch which has some influence on my changes too. http://trac.wildfiregames.com/changeset/11556

I'm still working on that patch. the progress is slow but steady. :)

comment:18 by Kieran P, 12 years ago

Milestone: Alpha 10Alpha 11

comment:19 by Kieran P, 12 years ago

Keywords: gui button mouse removed
Milestone: Alpha 11Backlog

comment:20 by historic_bruno, 11 years ago

Yves are you still working on this?

comment:21 by Josh, 10 years ago

Is this ticket still valid? I cannot reproduce the issue with the minimap unless I release the mouse outside the window and as I understand that was not the original issue this ticket was addressing.

comment:22 by historic_bruno, 10 years ago

It still seems broken to me, perhaps broken in a slightly different way than the ticket describes, Now moving the mouse outside the minimap seems to "freeze" it's last position, which isn't really much of an improvement over the old behavior (expected: minimap will continue to track the button being held down as it's moved and especially when it returns within the minimap).

comment:23 by Yves, 8 years ago

Owner: Yves removed
Status: assignednew

I'm not working on this anymore and the patch is probably terribly outdated.

Note: See TracTickets for help on using tickets.