Page MenuHomeWildfire Games

Properly sort selected entities by owner
ClosedPublic

Authored by Imarok on Feb 3 2017, 11:11 PM.

Details

Summary

Instead of using this hack with the modified template name (e.g. p3&foo) everywhere, now EntityGroups uses this "extended" template name only internally as key to sort by owner and template. So nothing besides EntityGroups needs to know about how this key is constructed.
Fixes #4425. refs #1807, #4167.
Fixes rP18979

Test Plan

Tested entity selection groups, adding and removing enitities from a group.
Tested control groups.
Set a rallypoint on a huntable animal, train a unit. The unit should move to the animal and kill/gather it.
Alt+doubleclick on some units like women, traders and military units (since that was broken in one of the predecessors of rP18979)

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
http://svn.wildfiregames.com/public/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 445
Build 718: Vulcan BuildJenkins
Build 717: arc lint + arc unit

Event Timeline

Imarok created this revision.Feb 3 2017, 11:11 PM
Imarok added a reviewer: mimo.
Imarok edited the summary of this revision. (Show Details)Feb 3 2017, 11:14 PM
Vulcan added a subscriber: Vulcan.Feb 4 2017, 12:05 AM

Build is green

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

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

mimo requested changes to this revision.Feb 4 2017, 12:42 PM

The selectionGroupName is lost with this patch. That is used when we want units with different templates to be grouped together (typically to group basic, advanced and elite units). I've noticed two places where changes are needed, but there may be more.

binaries/data/mods/public/gui/session/selection.js
66

shouldn't it be key = "p" + ... + "&" + key; as we want to keep the selectionGroupName if any (so that basic, advanced and elite are grouped together).

binaries/data/mods/public/gui/session/selection_panels.js
459

I think you should do add
templateName = GetTemplateData(entState.template).selectionGroupName || entState.template;

and use that in 461

This revision now requires changes to proceed.Feb 4 2017, 12:42 PM

Will update soon

binaries/data/mods/public/gui/session/selection.js
66

True. I wanted to do that, but seems like I didn't ^^

binaries/data/mods/public/gui/session/selection_panels.js
459

You are right.

Imarok updated this revision to Diff 448.Feb 4 2017, 1:19 PM
Imarok edited edge metadata.

Use selectionGroupName

mimo added a comment.Feb 4 2017, 2:01 PM

Looks good to me now. Thanks, that will solve a longstanding issue!
I'll do some tests and accept it later.

binaries/data/mods/public/gui/session/selection.js
51

not necessary for the patch, but as you modify this function, we could have "if (this.ents[ent]) continue;" here

binaries/data/mods/public/gui/session/selection_panels.js
940–941

missing semicolon

Vulcan added a comment.Feb 4 2017, 2:03 PM

Build is green

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

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

mimo accepted this revision.Feb 4 2017, 8:59 PM

Tests are ok, so revision accepted after fixing at least the semicolon.

This revision is now accepted and ready to land.Feb 4 2017, 8:59 PM
elexis edited the test plan for this revision. (Show Details)Feb 4 2017, 9:08 PM
Imarok updated this revision to Diff 466.Feb 6 2017, 5:38 PM

Add forgotten semicolon and some style fix

Vulcan added a comment.Feb 6 2017, 6:22 PM

Build is green

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

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

mimo added a comment.Feb 6 2017, 8:18 PM

Thanks for the changes; The acceptation still holds :-)

This revision was automatically updated to reflect the committed changes.