Page MenuHomeWildfire Games

Give an option to allow multiple controls groups share same entities.
Needs ReviewPublic

Authored by Freagarach on Aug 14 2019, 5:07 PM.

Details

Reviewers
wraitii
Test Plan
  1. Start a game.
  2. Group some units.
  3. Make a second group with a part of the first group.
    1. Verify that the part is not in the first group anymore.
  4. Flip the switch in the options menu.
  5. Try again and verify that units can be in multiple control groups.
  6. Check the replay and verify that the states are equal even when switching the option between successive replays.

Event Timeline

Freagarach created this revision.Aug 14 2019, 5:07 PM
Owners added a subscriber: Restricted Owners Package.Aug 14 2019, 5:07 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/selection.js
| 462| »   if·(!(Engine.ConfigDB_GetValue("user",·"gui.session.entitiesinmultiplecontrolgroups")·!=·"false"))
|    | [NORMAL] JSHintBear:
|    | Confusing use of '!'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/374/display/redirect

Stan awarded a token.Aug 14 2019, 5:16 PM
Angen added a subscriber: Angen.Aug 15 2019, 8:57 AM
Angen added inline comments.
binaries/data/mods/public/gui/session/selection.js
467–471

why do you negate negation to be false? Is not better to ask if it is equal to false? :)

Freagarach added inline comments.
binaries/data/mods/public/gui/session/selection.js
467–471

Ah, that is wat @elexis meant yesterday!

Freagarach retitled this revision from Give an option to allow multiple entities in the same control group. to Give an option to allow multiple controls groups share same entities..Aug 15 2019, 9:09 AM

Feels like this should be the default to me to be honest

The tooltip and name must be as transparent as possible, it's probably okay. I think I have to accept this unless you want to change it.

binaries/data/config/default.cfg
376

(That's the reason why aligning with spaces is discouraged) (Perhaps we can find a name that is 6 characters shorter without restoring to an abbreviation.)

binaries/data/mods/public/gui/session/selection.js
467–471

I don't remember pointing that out, but the double negation is indeed unnecessary indirection.
Was only wondering whether one needs an option to have it both ways rather than just deleting these 4 lines for everybody.
Since it's not obvious which behavior is better, we can probably assume that there are users for both values of the option.
(For instance I'd probably keep it the old way.)

Freagarach updated this revision to Diff 9359.EditedAug 16 2019, 5:32 PM
Freagarach marked 2 inline comments as done.

Negeted negation.

I have not yet found a better name, so if anyone has an idea, please share ;)

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/402/display/redirect

Freagarach added inline comments.Aug 28 2019, 5:15 PM
binaries/data/config/default.cfg
376

-control?

Stan added a subscriber: Stan.Aug 28 2019, 5:41 PM
Stan added inline comments.
binaries/data/config/default.cfg
376

what about entsinmultiplectrlgroups

binaries/data/mods/public/gui/session/selection.js
467–471

Wonder if one could use array.filter here. Also whether it's safe to remove entities in a group you are iterating in ...

Freagarach added inline comments.Aug 28 2019, 6:50 PM
binaries/data/config/default.cfg
376

Abb. [sic] but I'm okay with that.

binaries/data/mods/public/gui/session/selection.js
467–471

Wonder if one could use array.filter

In the previous situation that would indeed return only one group, however, when changing the option during a game, it could return an array now.

Also whether it's safe to remove entities in a group you are iterating in ...

That would indeed be a problem when you'd remove the last entity from a group, since the group would then be removed. Apparently there has been no issues with this before.
I would say out of scope to change here?

Stan added inline comments.Aug 28 2019, 10:49 PM
binaries/data/mods/public/gui/session/selection.js
467–471

In the previous situation that would indeed return only one group, however, when changing the option during a game, it could return an array now.

Well that's why I suggested filter, not find :)

That would indeed be a problem when you'd remove the last entity from a group, since the group would then be removed. Apparently there has been no issues with this before.

I would say out of scope to change here?

Yeah totally, just thought it was worth mentionning in case it could actually fail.

Freagarach added inline comments.Aug 29 2019, 7:51 AM
binaries/data/mods/public/gui/session/selection.js
467–471

Well that's why I suggested filter, not find :)

Like this?

usefulGroups = this.groups.filter(group  => ent in group.ent);
for (let group in usefulGroups)
    group.remove(ent);

This would also remove the problem with removing groups whilst iterating.

Stan added inline comments.Aug 29 2019, 10:32 AM
binaries/data/mods/public/gui/session/selection.js
467–471

I guess :) Adding a test would make sure of it :)

Freagarach updated this revision to Diff 9555.Aug 31 2019, 10:30 PM

disjointcontrolgroups (Wikipedia) as suggested by @elexis on IRC.

Freagarach marked 4 inline comments as done.Aug 31 2019, 10:31 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/19/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/528/display/redirect

wraitii edited reviewers, added: wraitii; removed: Restricted Owners Package.Sep 1 2019, 4:28 PM
Freagarach updated this revision to Diff 10135.Oct 12 2019, 9:39 PM
  • Improved text on option?
  • Reverted to old entity removal from groups.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/442/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/957/display/redirect

Freagarach updated this revision to Diff 10136.Oct 12 2019, 9:57 PM

More elaborate tooltip.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/443/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/958/display/redirect

Freagarach updated this revision to Diff 10171.Oct 17 2019, 9:54 PM

Tooltip yet again.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/466/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/981/display/redirect

Freagarach added inline comments.Oct 17 2019, 9:59 PM
binaries/data/config/default.cfg
376

Info is ambiguous.

binaries/data/mods/public/gui/options/options.json
547

-explicitely [sic]

Freagarach added inline comments.
binaries/data/mods/public/gui/options/options.json
546

Control Groups Behaviour

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/467/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/982/display/redirect