Page MenuHomeWildfire Games

Add SelectionGroupNames for siege engines.
Needs ReviewPublic

Authored by causative on Apr 27 2018, 12:05 AM.

Details

Reviewers
Stan
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Gives all catapults a SelectionGroupName, gives all bolt shooters a SelectionGroupName and gives all siege towers and rams a SelectionGroupName. This means that if you have catapults from different civs because you captured an enemy catapult, double clicking on a catapult will select all of them regardless of civ.

Note: This patch previously was about how ptol catapults unpacked into mace catapults. Because discussion on D1473 has become controversial regarding SelectionGroupNames, the fix for the ptol/mace catapult issue has been removed from D1473 and split off into D1474.

Test Plan

Set up a scenario where player 1 has catapults, siege towers, and bolt shooters from multiple civs. Verify the selection behavior works as intended.

Event Timeline

causative created this revision.Apr 27 2018, 12:05 AM
Vulcan added a subscriber: Vulcan.Apr 27 2018, 12:09 AM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/441/display/redirect

elexis added a subscriber: elexis.Apr 27 2018, 12:10 AM

Nice find.
The foremost question would be if it's complete, i.e. if the same mistake is present in similar templates.
(A not really relevant question would be which commit introduced the issue)

If we want different civs catapults to be selected as the same entity, they should have the same selection group name (unless there is a good reason to keep them distinguished, but probably there isnt).

causative updated this revision to Diff 6480.Apr 27 2018, 12:37 AM

Also added a SelectionGroupName to catapults and bolt shooters

temple added a subscriber: temple.Apr 27 2018, 12:38 AM

Lol, fixed in D1032/rP20445, then unfixed in rP20483. (I don't know why I didn't change the mace parent.)

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

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

causative edited the summary of this revision. (Show Details)Apr 27 2018, 12:40 AM
causative edited the test plan for this revision. (Show Details)

The other catapult and bolt shooter templates don't have this problem.

elexis added a reviewer: Stan.Apr 27 2018, 9:15 AM
Stan requested changes to this revision.Apr 27 2018, 9:28 AM

Nice patch ! There are a few things like this that should be fixed at some point.

I'm not sure we need the template prefix at the beginning of the selection group name. Look at how it is done for units.

Otherwise patch looks good.

This revision now requires changes to proceed.Apr 27 2018, 9:28 AM

Requesting changes implies being sure, the units changed here are the only ones capturable, besides the relic that already has a selectiongroupname of the template and one can't unify the selections of the same unit type of different civs without refering to a template outside of unit/ because unit contains the civ specific templates.
If you want only one instead of two patterns, the other 350 occurrences can be changed to also reference the parent templates.
More importantly buildings have no selection group names, so if you captured towers of 5 civs and have all of them selected, you get 5 different panel buttons. (Some years ago I wrote a ticket about that but there were some meh's iirc)

Patch is correct as far as I see.
I think the mobile tower is needed for completeness, right?
(The ram could be added, then we have all siege engines (i.e. beyond all capturable units), but some day we can change all selectables if we want)

Stan added a comment.Apr 27 2018, 12:06 PM

Yeah sounds like a good idea to unify the other siege engines.

causative updated this revision to Diff 6481.Apr 27 2018, 12:48 PM

Added a SelectionGroupName for siege towers as well

causative edited the summary of this revision. (Show Details)Apr 27 2018, 12:50 PM
causative edited the test plan for this revision. (Show Details)

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

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

Nescio added a subscriber: Nescio.Apr 27 2018, 1:12 PM

Also, gives all catapults a SelectionGroupName, gives all bolt shooters a SelectionGroupName, and gives all siege towers a SelectionGroupName.

Doesn't this mean packed and unpacked will be grouped together as a single entity?

Also, introducing selection group names at a generic template level is inconsistent with all other existing templates, as far as I'm aware.

In D1473#60293, @Nescio wrote:

Also, gives all catapults a SelectionGroupName, gives all bolt shooters a SelectionGroupName, and gives all siege towers a SelectionGroupName.

Doesn't this mean packed and unpacked will be grouped together as a single entity?

That should be avoided indeed. Could be achieved by using something like "template*_packed".
The schema doesn't define that the template must exist (although some code already wanted to assume that), so we could either use template*_packed without that exact template name existing,
or in alpha 24 change the schema to state that it must be a template and consider adding such a template.
But the revert should be reverted for this release, i.e this weekend.

Also, introducing selection group names at a generic template level is inconsistent with all other existing templates, as far as I'm aware.

That's why I mentioned the relic example (and why capturable ones are exactly the ones with the thing surfacing and the existance of the ticket).

I think that grouping packed and unpacked catapults together is better than the current behavior. The player can triple click if they want the old behavior. I've noticed in many of my games that I frequently want to retreat *all* catapults because some anti-catapult thing is approaching, and it's a minor annoyance that double clicking doesn't select them all. I don't think wanting to select all packed or all unpacked is as common a use case. If I only wanted to move certain catapults I would drag to select them.

elexis added subscribers: bb, mimo.Apr 27 2018, 3:47 PM

It was #3502

it's a minor annoyance that double clicking doesn't select them al

Agree on doubleclicking,
for the selection panel it could be useful in at least some occasions to distinguish packed and unpacked ones (so that the unpacked continue to attack and the packed ones can move),
if tripleclick addresses that, maybe, although it still can't cover the case where you only want to select the packed ones on one area of the screen (to optimize their position without accidentally packing the attacking, unpacked siege engines).

Adding hotkeys to select only siege engines and similar classes is an idea I had once.
Grouping siege engine types but splitting packed and unpacked ones seems more versatile until we had hotkeys?
Although nearly empty, adding the template_*_packed ones seems easy to achieve and changing the selectiongroupname cost-free?

@temple @mimo @bb any opinion on wheather to group all packagable catapults or all catapults using SelectionGroupName? (and structures in #3502?)

mimo added a comment.Apr 27 2018, 6:46 PM

I've noticed that all 3 kush walls also have a wrong SelectionGroupName (either ptol or mace). That could be added to that patch.
Otherwise, I've no real preference on elexis' question,

temple added a comment.EditedApr 28 2018, 1:35 AM

I don't have a strong opinion on SelectionGroupName stuff.

In D1473#60314, @mimo wrote:

I've noticed that all 3 kush walls also have a wrong SelectionGroupName (either ptol or mace). That could be added to that patch.

I did a similar thing for ptol walls at D1033/rP20451. Wonder if there's more issues with kush?
Yeah that reminds me, they don't get all the formations. (Fixed in rP21793.)

causative retitled this revision from Ptol catapults should unpack to ptol catapults, not mace catapults to Add SelectionGroupNames for siege engines.
causative edited the summary of this revision. (Show Details)
causative edited the test plan for this revision. (Show Details)
causative updated this revision to Diff 6486.Apr 28 2018, 10:40 AM

remove the mace/ptol template fix from the patch

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

Link to build: https://jenkins.wildfiregames.com/job/differential/445/display/redirect

wraitii added a reviewer: Restricted Owners Package.May 14 2018, 11:54 AM
wraitii accepted this revision.Jan 23 2019, 12:19 PM

This seems obviously good to me?
Unless I'm misunderstanding things this just affects the double-clicking behaviour?

Imarok added a subscriber: Imarok.Jan 23 2019, 12:31 PM

This seems obviously good to me?
Unless I'm misunderstanding things this just affects the double-clicking behaviour?

I think this should differentiate packed and unpacked siege engines. (Correct me if it already does)

I think this should differentiate packed and unpacked siege engines. (Correct me if it already does)

It doesn't as-is but I actually disagree here, so :p . Is the triple-clicking behaviour described above in-game already?

I think this should differentiate packed and unpacked siege engines. (Correct me if it already does)

It doesn't as-is but I actually disagree here, so :p . Is the triple-clicking behaviour described above in-game already?

It is.
But usually I just use the selection drag box to select a bunch of units and then remove those from selection I don't want. This won't be possible after this patch. And triple clicking on a unit isn't easy if the unit is part of a big fighting crowd.
Also after this patch you have no possibility to see if any of your catas packed itself when it's part of a big fighting crowd.

But usually I just use the selection drag box to select a bunch of units and then remove those from selection I don't want. This won't be possible after this patch. And triple clicking on a unit isn't easy if the unit is part of a big fighting crowd.
Also after this patch you have no possibility to see if any of your catas packed itself when it's part of a big fighting crowd.

Ok that's a bigger issue that wasn't obvious from just reading the diff today.

In that situation I agree that we need to split packed/unpacked variants, at least in the selection panel. IMO the fact that the click-select behaviour and the selection panels are tied is wrong design. The problem does not lie with this patch, but this patch cannot be merged until then.

I believe packed/unpacked variants inherit from the unit template so we are stuck.

Packed / unpacked entities have different templates, so they can have custom selectiongroupnames, no?

("This seems obviously good" = The flaw is really hard to find. Every diff is bugged. The ones where noone discovered a bug yet are the ones where the bug was not discovered by anyone yet. So one can only spend five lifetimes looking at a diff and trying to construct bugs to rule out all the other bugs except the perfectly hidden one. And after that there will still be a concern. And with that level of verification, one feels relief for finally receiving the information what one missed. Then one will be able to learn form the bug, abstract an entire class of bugs and that bug will never happen again. Which in turn means the next apparently perfectly hidden bug will even be more difficult to predict.)

Stan requested changes to this revision.Jan 24 2019, 8:32 AM

Can you add a different selection group name for packed entities ?

This revision now requires changes to proceed.Jan 24 2019, 8:32 AM
Freagarach updated this revision to Diff 10012.Sep 29 2019, 3:58 PM
Freagarach retitled this revision from Add SelectionGroupNames for siege engines to Add SelectionGroupNames for siege engines..
Freagarach edited the summary of this revision. (Show Details)
Freagarach added a subscriber: Freagarach.

Different selection group names based on packing status.

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

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

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

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

The

binaries/data/mods/public/simulation/templates/units/spart_siege_oxybeles_unpacked.xml
5

Don't start inventing non-existent file names, those are misleading.

The last time I looked at this it grouped packed and unpacked siege engines together. That no longer seems to be the case.
Two other things to consider:

  • Roman artillery inflicts more damage; should those be grouped separately?
  • While there is only one generic parent template for bolt shooters and one for stone throwers, in the units folder (and workshop production queue) there are three each: scorpio, polybolos, oxybeles and onager, lithobolos, ballista. Perhaps those ought to be the selection groups?

I don't know, I don't have strong feelings either way.