Page MenuHomeWildfire Games

Introduce a new hotkey to order one unit from current selection.
ClosedPublic

Authored by wraitii on Apr 9 2017, 3:14 PM.

Details

Reviewers
vladislavbelov
Imarok
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20441: Introduce a new hotkey to order only one unit from the current selection.
Summary

This is an idea I've had some days back. Turns out it was super simple to implement, so here's a patch.
This will mostly be about the design discussion of whether we want this hotkey or not.

To test it, select a few units, then press this hotkey, then give an order. You will see one unit (the first) will go and do that order, and get removed from the selection.

This allows you, for example, to select all your units at the start and easily give them each a different order.

Test Plan

Test the hotkey. Report whether you like this or not.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.Apr 9 2017, 3:14 PM
Vulcan added a subscriber: Vulcan.Apr 9 2017, 3:59 PM

Build is green

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

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

vladislavbelov requested changes to this revision.Apr 10 2017, 2:32 PM
vladislavbelov added a subscriber: vladislavbelov.

I've tested it, it works for me, except the hotkey.

binaries/data/config/default.cfg
286 ↗(On Diff #1169)

The "Super" hotkey isn't good for Windows users, because when you release Super (Windows) key, it opens Start menu, even in fullscreen, it'll annoying for players. So I suggest to add other key.

This revision now requires changes to proceed.Apr 10 2017, 2:32 PM

Also what if selection[0] can not do an order, shouldn't we deselect first unit who can do it?

I think it will be really useful feature.

Ah, I was wondering this about Super... We'd have to find something else then, on OSX it's quite convenient 📦

Also what if selection[0] can not do an order, shouldn't we deselect first unit who can do it?

Well, right now, we don't handle this either you give an order to a group of units. Those that can't do it just walk there. So I did not feel like it was worth handling specifically. I could be convinced otherwise though.

vladislavbelov added a comment.EditedApr 10 2017, 3:03 PM
In D308#12255, @wraitii wrote:

Well, right now, we don't handle this either you give an order to a group of units. Those that can't do it just walk there. So I did not feel like it was worth handling specifically. I could be convinced otherwise though.

In any case selection has a random order (if select many units), so why do not select right unit? If a player want to just walk to some point, he may just click to this point, but if he want gather something, then it'd better to order unit who can do this.

wraitii updated this revision to Diff 1196.Apr 10 2017, 4:21 PM
wraitii edited edge metadata.

Change the hotkey to alt and select the first unit that can do this order instead (had to purify the code a bit which we probably should do anyhow).

vladislavbelov requested changes to this revision.Apr 10 2017, 4:37 PM
vladislavbelov added inline comments.
binaries/data/mods/public/gui/session/input.js
1165 ↗(On Diff #1196)

It'll be shorter, and I think, has the same readability or even more, if write like:

let methods = ["preSelectedActionCheck", "hotkeyActionCheck", "actionCheck"];
for (let i = 0; i < selection.length; ++i)
{
	let actionTarget = action.target;
        if (!methods.find(method => unitActions[action.type][method] && unitActions[action.type][method](actionTarget, [selection[i]])))
		continue;
	selection = [selection[i]];
	g_Selection.removeList(selection);
}
This revision now requires changes to proceed.Apr 10 2017, 4:37 PM
wraitii updated this revision to Diff 1201.Apr 10 2017, 4:57 PM
wraitii edited edge metadata.

Good point, I think this is slightly more readable still.

Build is green

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

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

vladislavbelov accepted this revision.Apr 10 2017, 5:59 PM

I've tested it, it works as expected, it's good when you have many selected units and you need to complete a small job.

This revision is now accepted and ready to land.Apr 10 2017, 5:59 PM

Just going to ping @Itms @elexis @fatherbushido << are you OK with this? If Yes I will commit

Itms added a reviewer: Imarok.EditedApr 10 2017, 6:24 PM

I'm not sure in which kind of situation it would be useful...

I suggest a review by Imarok who worked hard so that the first unit of the selection would not be special at all. The "firstness" of that unit is subject to a lot of irrelevant things, for instance the way you selected units.

EDIT: I hadn't understood how it would behave, after clarification I do see when it is useful. 🙂

This revision now requires review to proceed.Apr 10 2017, 6:24 PM

Build is green

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

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

Why did you choose meta as name for the hotkey?

Don't know, if it's because of this patch, but I got this error:

WARNING: JavaScript warning: simulation/components/GuiInterface.js line 233 Script value conversion check failed: v.isNumber() (got type undefined)

ERROR: JavaScript error: gui/session/selection_panels.js line 875 TypeError: entState is null g_SelectionPanels.Selection.setupButton@gui/session/selection_panels.js:875:7 setupUnitPanel@gui/session/unit_commands.js:96:8 updateUnitCommands@gui/session/unit_commands.js:147:4 updateSelectionDetails@gui/session/selection_details.js:474:2 updateGUIObjects@gui/session/session.js:845:2 onSimulationUpdate@gui/session/session.js:788:2 __eventhandler176 (simulationupdate)@sn simulationupdate:0:1
binaries/data/mods/public/gui/session/input.js
1485 ↗(On Diff #1201)

Don't think you should use tab instead of space here..
(Same for the next changed line)

Mh, that does seem related. Any chance it happens on a replay?

Re the name, honestly because I had no better idea and it does something vaguely "meta".

Imarok added a comment.EditedApr 18 2017, 3:35 PM

Doesn't work for guard and repair hotkey.

Edit: repair hotkey works, when fixed with D358

binaries/data/mods/public/gui/session/input.js
1155 ↗(On Diff #1201)

Why do you check for 'preSelectedActionCheck'?

In D308#13920, @wraitii wrote:

Mh, that does seem related. Any chance it happens on a replay?

Oh, surprisingly it does, but you need to select and deselect the right things at the right moment, and I could reproduce only once. :/ (F119745)

Re the name, honestly because I had no better idea and it does something vaguely "meta".

What about orderone?

Imarok requested changes to this revision.May 4 2017, 12:32 PM

See comments above.

This revision now requires changes to proceed.May 4 2017, 12:32 PM
Imarok added a comment.EditedOct 1 2017, 12:08 AM

repair works now (rP20250)
guard is probably still broken.
(Also needs a rebase)

wraitii updated this revision to Diff 4060.Nov 1 2017, 5:06 PM
wraitii added reviewers: Restricted Owners Package, Restricted Owners Package.

Address comments.

Vulcan added a comment.Nov 1 2017, 6:48 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Nov 1 2017, 6:49 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/input.js
| 231| »   »   var·entState·=·GetExtendedEntityState(entityID);
|    | [NORMAL] JSHintBear:
|    | 'entState' is already defined.

binaries/data/mods/public/gui/session/input.js
| 273| »   var·target·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'target' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 287| »   var·actionInfo·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actionInfo' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 301| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 304| »   »   »   var·r·=·unitActions[action].hotkeyActionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 309| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 312| »   »   »   var·r·=·unitActions[action].actionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 509| »   »   &&·(ev.button·==·SDL_BUTTON_LEFT·||·ev.button·==·SDL_BUTTON_RIGHT))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/gui/session/input.js
| 539| »   »   »   »   var·rect·=·updateBandbox(bandbox,·ev,·true);
|    | [NORMAL] JSHintBear:
|    | 'rect' is already defined.

binaries/data/mods/public/gui/session/input.js
| 542| »   »   »   »   var·ents·=·getPreferredEntities(Engine.PickPlayerEntitiesInRect(rect[0],·rect[1],·rect[2],·rect[3],·g_ViewedPlayer));
|    | [NORMAL] JSHintBear:
|    | 'ents' is already defined.

binaries/data/mods/public/gui/session/input.js
| 691| »   »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 731| »   »   »   var·dragDeltaX·=·ev.x·-·dragStart[0];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaX' is already defined.

binaries/data/mods/public/gui/session/input.js
| 732| »   »   »   var·dragDeltaY·=·ev.y·-·dragStart[1];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaY' is already defined.

binaries/data/mods/public/gui/session/input.js
| 733| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] JSHintBear:
|    | 'maxDragDelta' is already defined.

binaries/data/mods/public/gui/session/input.js
| 765| »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 897| »   »   »   »   »   »   var·sptr·=·ev.hotkey.split(".");
|    | [NORMAL] JSHintBear:
|    | 'sptr' is already defined.

binaries/data/mods/public/gui/session/input.js
| 913| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
| 924| »   »   »   »   var·action·=·determineAction(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 939| »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/session/input.js
| 966| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
|1406| »   »   »   »   »   »   multiplyEntityCosts(template,·g_BatchTrainingCount·+·batchIncrementSize)·}))
|    | [NORMAL] JSHintBear:
|    | 'template' used out of scope.

bi
Imarok requested changes to this revision.Nov 5 2017, 4:00 PM

Works good.
Looks good.
Things needed:
Add the hotkey to ingame and trac manual.

binaries/data/mods/public/gui/session/input.js
1153 ↗(On Diff #4060)

I guess that looks nicer:

let selection = g_Selection.toList();
if (orderone)
{
    // pick the first unit that can do this order.
    let unit = selection.find(entity =>
        ["preSelectedActionCheck", "hotkeyActionCheck", "actionCheck"].some(method =>
            unitActions[action.type][method] &&
            unitActions[action.type][method](action.target || undefined, [entity])
        ));
    if (unit)
    {
        selection = [unit];
        g_Selection.removeList(selection);
    }
}
1491 ↗(On Diff #4060)

Still

This revision now requires changes to proceed.Nov 5 2017, 4:00 PM
wraitii updated this revision to Diff 4140.Nov 11 2017, 11:23 AM

Should fix Imarok's comments.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/input.js
| 231| »   »   var·entState·=·GetExtendedEntityState(entityID);
|    | [NORMAL] JSHintBear:
|    | 'entState' is already defined.

binaries/data/mods/public/gui/session/input.js
| 273| »   var·target·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'target' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 287| »   var·actionInfo·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actionInfo' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 301| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 304| »   »   »   var·r·=·unitActions[action].hotkeyActionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 309| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 312| »   »   »   var·r·=·unitActions[action].actionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 509| »   »   &&·(ev.button·==·SDL_BUTTON_LEFT·||·ev.button·==·SDL_BUTTON_RIGHT))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/gui/session/input.js
| 539| »   »   »   »   var·rect·=·updateBandbox(bandbox,·ev,·true);
|    | [NORMAL] JSHintBear:
|    | 'rect' is already defined.

binaries/data/mods/public/gui/session/input.js
| 542| »   »   »   »   var·ents·=·getPreferredEntities(Engine.PickPlayerEntitiesInRect(rect[0],·rect[1],·rect[2],·rect[3],·g_ViewedPlayer));
|    | [NORMAL] JSHintBear:
|    | 'ents' is already defined.

binaries/data/mods/public/gui/session/input.js
| 691| »   »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 731| »   »   »   var·dragDeltaX·=·ev.x·-·dragStart[0];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaX' is already defined.

binaries/data/mods/public/gui/session/input.js
| 732| »   »   »   var·dragDeltaY·=·ev.y·-·dragStart[1];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaY' is already defined.

binaries/data/mods/public/gui/session/input.js
| 733| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] JSHintBear:
|    | 'maxDragDelta' is already defined.

binaries/data/mods/public/gui/session/input.js
| 765| »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 897| »   »   »   »   »   »   var·sptr·=·ev.hotkey.split(".");
|    | [NORMAL] JSHintBear:
|    | 'sptr' is already defined.

binaries/data/mods/public/gui/session/input.js
| 913| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
| 924| »   »   »   »   var·action·=·determineAction(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 939| »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/session/input.js
| 966| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
|1406| »   »   »   »   »   »   multiplyEntityCosts(template,·g_BatchTrainingCount·+·batchIncrementSize)·}))
|    | [NORMAL] JSHintBear:
|    | 'template' used out of scope.

binaries/data/mods/public/gui/session/input.js
|1424| »   »   »   multiplyEntityCosts(template,·batchIncrementSize)·}))
|    | [NORMAL] JSHintBear:
|    | 'template' used out of scope.
Imarok accepted this revision.Nov 11 2017, 5:57 PM

Please fix these things when committing:
(And also don't forget to update the trac manual after committing ;))

binaries/data/mods/public/gui/session/input.js
1163 ↗(On Diff #4140)

Looks like trailing whitespaces

1491 ↗(On Diff #4140)

To state it clearly: this change should be reverted

1520 ↗(On Diff #4140)

Same here

This revision is now accepted and ready to land.Nov 11 2017, 5:57 PM
wraitii added inline comments.Nov 11 2017, 6:08 PM
binaries/data/mods/public/gui/session/input.js
1491 ↗(On Diff #4140)

I don't understand how this happens, I've actually tried reverting it three times already :p

Imarok added inline comments.Nov 11 2017, 6:10 PM
binaries/data/mods/public/gui/session/input.js
1491 ↗(On Diff #4140)

I guess your editor is doing s**t

This revision was automatically updated to reflect the committed changes.