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
472–473

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
472–473

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
377

(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
472–473

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.Wed, Aug 28, 5:15 PM
binaries/data/config/default.cfg
377

-control?

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

what about entsinmultiplectrlgroups

binaries/data/mods/public/gui/session/selection.js
472–473

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.Wed, Aug 28, 6:50 PM
binaries/data/config/default.cfg
377

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

binaries/data/mods/public/gui/session/selection.js
472–473

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.Wed, Aug 28, 10:49 PM
binaries/data/mods/public/gui/session/selection.js
472–473

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.Thu, Aug 29, 7:51 AM
binaries/data/mods/public/gui/session/selection.js
472–473

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.Thu, Aug 29, 10:32 AM
binaries/data/mods/public/gui/session/selection.js
472–473

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

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

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

Freagarach marked 4 inline comments as done.Sat, Aug 31, 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.Sun, Sep 1, 4:28 PM