Page MenuHomeWildfire Games

Easier double-clicking on moving units and remove doubleclick hack input.js
ClosedPublic

Authored by causative on Apr 14 2017, 3:32 AM.

Details

Reviewers
elexis
Commits
rP19444
Trac Tickets
#4414
Summary

If you have a moving unit, especially a fast one such as spear cavalry, it's difficult to double click on it to select all units of that type, because both clicks have to be on the same unit, but if the unit has moved, you will often click empty ground the second time.

For instance, this happens often when cavalry raiding; each time you make new cavalry you want to select all cavalry and bind them to a hotkey, but if all cavalry are moving it's tricky to alt-double click on one.

This patch modifies input.js so that a double or triple click operates on the first unit to be clicked as part of the double or triple click. This makes the above use case much easier. Now that the requirement that all clicks be on the same unit has been removed, we need a different criterion for distinguishing a double click from rapid successive single clicks, and this patch chooses that the squared pixel distance between successive clicks must be no more than 5.

See also: trac ticket 4414 which may move all this double click code to C++. However, since this is a simple patch that solves a UI issue, I think it can serve as a temporary fix for the time being, until 4414 is addressed.

Test Plan

Open a scenario with a bunch of cavalry (such as the Rome sandbox), move one of them, and double click on it. As long as your first click is on the moving unit, all units in view should be selected even if the unit has moved past your second click. Verify that all combinations of clicking on units still work:

  • with a single, double, or triple click
  • with or without the alt hotkey
  • with or without the ctrl hotkey, to deselect units
  • with or without the shift hotkey, to add units to selection

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 1230
Build 1942: Vulcan BuildJenkins

Event Timeline

causative created this revision.Apr 14 2017, 3:32 AM
causative added a reviewer: elexis.
causative edited the summary of this revision. (Show Details)Apr 14 2017, 3:38 AM

Good change in my opinion, experienced this problem myself even with slower units.
Might need to check that we don't move th mouse to follow the unit and still accept that as a second click though.

binaries/data/mods/public/gui/session/input.js
49

We usually go with doubleClickDistSquared for this.

causative updated this revision to Diff 1233.Apr 14 2017, 11:22 AM

Changed sqDoubleClickDist to doubleClickDistSquared, and increased the value from 5 to 10 for greater tolerance of mouse motions.

When I need to select a moving unit and all with his type, I just press F and double click on it.

I think, the suggested way is less obvious for players, because mouse is clicked outside of unit, but still selects something (looks like a bug/hack, no?). So I could suggest to not modify double click, but use something like Alt+LMB on any unit to select all of this type, then it'll be obvious for players.

elexis edited edge metadata.Apr 14 2017, 2:23 PM

When I need to select a moving unit and all with his type, I just press F and double click on it.

I think, the suggested way is less obvious for players, because mouse is clicked outside of unit, but still selects something (looks like a bug/hack, no?). So I could suggest to not modify double click, but use something like Alt+LMB on any unit to select all of this type, then it'll be obvious for players.

In particular I'm not fond of that new pixel distance constant. We already have three different way to detect doubleclicks. If they were merged as proposed in #4414 and if this feature request as proposed by causative was accepted, then it could still be implemented by saving the entity type on the first click and then selecting all of them on the second click.
Do have to say though that doubleclicking and alt+doubleclicknig on units to select all of that type seems quite intuitive, while having to use the follow unit function for fast moving units seems to be a workaround too.

There could also be a hotkey to select all units of the current type, so we could have a selection of a single unit (or a lasso selection for extremely fast moving cavalry) and then pressing a hotkey an arbitrary time afterwards to solve the issue.

Alt+LMB might work too, but must be checked whether it conflicts with something (doesn't iirc).

causative updated this revision to Diff 1251.EditedApr 14 2017, 9:23 PM

Got rid of the ugly js detection of double clicks. Instead pass up the clicks SDL attribute from C++ and use that.

elexis requested changes to this revision.Apr 17 2017, 4:07 AM

The best test case is upgraded horizontally moving cavalry.

There is a catch with this patch.

  1. Select a player owned unit
  2. Move the mouse somewhere else, optionally click once to select nothing
  3. Doubleclick

It will not select all units on screen of that type, but that action is not expected as neither the first nor the second click were focused on that unit, so you will have to reset at some point.

This revision now requires changes to proceed.Apr 17 2017, 4:07 AM
causative updated this revision to Diff 1305.Apr 17 2017, 5:47 PM
causative edited edge metadata.

I decided it was better to set the prevClickedUnit unit on the mousebuttondown event, not the mousebuttonup event. That way it's more responsive and easier to click a moving unit, because the delay is less between pressing the button and choosing the unit.

The prevClickedUnit is now set on every single-click (in normal input mode), so it gets reset if you click again.

elexis added inline comments.Apr 17 2017, 6:04 PM
binaries/data/mods/public/gui/session/input.js
970

Appears wrong to have the input handler change camera controls. If the user wants to follow an entity, why not let him do so?

causative added a comment.EditedApr 17 2017, 8:18 PM

That was an already existing feature that I just preserved. It looks like disabling camera follow when a new unit is selected, was added in revision 9764.

causative updated this revision to Diff 1325.Apr 18 2017, 6:41 AM

Set prevClickedUnit on any mousedown (normal input mode) or mouseup (select mode) whenever it's currently INVALID_ENTITY. This makes it easier to click on a moving unit, since if you miss the unit on one mouse button event you can get it on another one.

On a single-click mousedown, as before it's reset regardless of whether it's INVALID_ENTITY, so it won't persist between unrelated clicks.

elexis retitled this revision from Easier double-clicking on moving units to Easier double-clicking on moving units and remove doubleclick hack input.js.Apr 20 2017, 1:33 PM
causative updated this revision to Diff 1416.Apr 22 2017, 3:10 AM
  • Added/changed some comments per IRC discussion with elexis
  • renamed prevClickedEntity to just clickedEntity, for clarity
  • moved the Engine.CameraFollow(0) block down
elexis accepted this revision.Apr 22 2017, 3:19 AM

(rP9764 actually)

Patch works great and I'm really glad to see this ugly custom double clicker implementation finally being removed!
Also thanks for discussing and explaining it on multiple days and updating it following the feedback.

This revision is now accepted and ready to land.Apr 22 2017, 3:19 AM
Vulcan added a subscriber: Vulcan.Apr 22 2017, 3:55 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/850/ for more details.

elexis added inline comments.May 15 2017, 2:18 PM
binaries/data/mods/public/gui/session/input.js
46

Should have called this g_ClickedEntity even if the other names in this file don't abide the http://trac.wildfiregames.com/wiki/Coding_Conventions yet

temple added a subscriber: temple.Aug 18 2017, 4:32 PM

I've had a lot of problems with double clicking to select units in a22. I double click but it doesn't select all the units, so then I double click again, but still no luck. So I triple click or quadruple click and then it selects all the units for a moment before going back to one because I've clicked once too many and cycled through. It's very frustrating.
I'll try to experiment in the coming days, see if I can consistently reproduce the issue, and maybe revert this patch and see if that helps. (I guess it's possible that this was an issue in a21 too and I just don't remember.)

Possible hint: I use a pen rather than a mouse, so often (or always?) there's some tiny jitter between clicks.

It is a problem with jitter. For example, in an earlier version causative used a squared distance threshold of 10, which meant offsets of (0,3), (1,3), and (2,2) were acceptable. But to increment "clicks" it seems the mousebuttondown offset can only be (0,1) or (1,1). With my pen on a small tablet and large monitor, I miss that all the time.

Failure, mousebuttondown offset (0,2):

clicked entity = 1788, ev = ({type:"mousebuttondown", button:1, state:1, x:341, y:562, clicks:1})
clicked entity = 1761, ev = ({type:"mousebuttonup", button:1, state:0, x:341, y:563, clicks:1})
clicked entity = 1761, ev = ({type:"mousebuttondown", button:1, state:1, x:341, y:564, clicks:1})
clicked entity = 1761, ev = ({type:"mousebuttonup", button:1, state:0, x:341, y:564, clicks:1})

Successful double-click, mousebuttondown offset (1,1):

clicked entity = 1776, ev = ({type:"mousebuttondown", button:1, state:1, x:332, y:675, clicks:1})
clicked entity = 1771, ev = ({type:"mousebuttonup", button:1, state:0, x:332, y:674, clicks:1})
clicked entity = 1771, ev = ({type:"mousebuttondown", button:1, state:1, x:331, y:674, clicks:2})
clicked entity = 1771, ev = ({type:"mousebuttonup", button:1, state:0, x:331, y:674, clicks:2})

I didn't revert the changes, but I'm guessing the "doubleclick hack" that was removed was making things work okay back in a21.

But to increment "clicks" it seems the mousebuttondown offset can only be (0,1) or (1,1). With my pen on a small tablet and large monitor, I miss that all the time.

The idea was that we shouldn't reinvent the wheel (i.e. not hardcode timeouts and mouse distances between clicks), but use SDL to parse input events.
I assume that your unintentional-doubleclick issue might therefore also apply to other places that use SDL to detect doubleclicks (if the multiplayer lobby doubleclick join bugs, the issue might just be hidden by not enough things using doubleclicks).
So the generic solution to me seems to add a new config option to change the interval with SDL.

However I couldn't find any way to modulate the mouse settings with SDL anywhere. Apparently there are undocumented environment variables, like SDL_VIDEO_X11_MOUSEACCEL.
Apparently there is or has been (SDL 1.2 instead of SDL2?) this function SDL_GetClickInterval(). But I couldn't find a setter.
https://forums.libsdl.org/viewtopic.php?p=44357
https://discourse.libsdl.org/t/click-count-and-events/20826/8

It's hardcoded by SDL in SDL_mouse.c: SDL_double_click_radius = 1. https://fossies.org/linux/SDL2/src/events/SDL_mouse.c There was some discussion on changing the value here: https://forums.libsdl.org/viewtopic.php?p=51852. Perhaps someone could submit a patch to SDL to increase the default radius and add a setter.

It's hardcoded by SDL in SDL_mouse.c: SDL_double_click_radius = 1. https://fossies.org/linux/SDL2/src/events/SDL_mouse.c There was some discussion on changing the value here: https://forums.libsdl.org/viewtopic.php?p=51852. Perhaps someone could submit a patch to SDL to increase the default radius and add a setter.

Good research causative! In that file we can also find SDL_SetDoubleClickTime, it should be exposed as well.
Ubuntu 16.04 still ships with SDL 2.0.4, while 2.0.5 is released since a considerable time. So either we'd have to make use of the new function optional or give the distribution an incentive to update SDL.
Someone could write a ticket to keep track of this.

causative added a comment.EditedAug 19 2017, 9:39 PM

It's not fixed in SDL 2.0.5, we'd need to submit a patch to a future SDL version. Ubuntu 17.04 does come with SDL 2.0.5.

Double clicking has been working much better now that I know I need to click in the same spot twice. It's not perfect (because of the small radius), but it's not frustrating anymore.