Page MenuHomeWildfire Games

Regicide game - Gamesetup Option to Disable Garrisoning of Heroes
ClosedPublic

Authored by Sandarac on Jan 29 2017, 2:08 AM.

Details

Summary

At this point, this one gamesetup option is planned for the regicide victory condition.

The trac ticket was originally created with the goal of introducing a "regicide" victory condition of some kind, and it has a lengthy history; once this option is implemented, it should be closed.

This patch uses a dynamic template in order to prevent heroes from garrisoning, and so it requires the patch at #2951 and cannot be implemented until dynamic templates are added.

One of the largest issues with this patch is handling cases when players start with all of their units garrisoned on a ship, and (currently) this is only an issue with the skirmish map "Sicilia Nomad". In this patch, this is handled by spawning the hero using the closest possible entity on land, instead of spawning the hero on the ship.

Light AI support is included.

I haven't added component tests yet, as some things will likely need to be changed.

Test Plan

With the patch at #2951 applied, when the ability to garrison heroes is not selected for a regicide game, the heroes cannot garrison.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

Only the description needs an update?

@leper Why do we have FriendlyFire Undeletable Unhealable Pickup DiesInWater Preview Corpse AlwaysVisible RetainInFog KillBeforeGather CanGuard FormationController Sharable Bribable as boolean template options that change simulation behavior if we could make all of them Identity classes?

The list of classes is Animal, Apadana, ArmyCamp, Ashoka, BarterMarket, Celt, CitizenSoldier, CivCentre, Colony, ConquestCritical, Defensive, Domestic, DropsiteFood, DropsiteMetal, DropsiteStone, DropsiteWood, Elephant, Female, FishingBoat, ForestPlant, Fortress, GarrisonFortress, GarrisonTower, Gates, Human, Iberian, Immortal, Italian, Juggernaut, Kennel, Lighthouse, LongWall, MercenaryCamp, Naval, NavalMarket, Organic, Palace, Palisade, Player, PtolemyIV, SeaCreature, SiegeWall, SpecialBuilding, StoneWall, Structure, Syssiton, Theater, Tower, Unit, Archer, Barracks, Blacksmith, BoltShooter, Camel, Catapult, Cavalry, Champion, Chariot, Citizen, City, Civic, CivilCentre, Corral, DefenseTower, Dock, Dog, Economic, Embassy, Farmstead, Field, Fireship, Healer, Hero, House, Infantry, Javelin, Market, Mechanical, Melee, Mercenary, Military, Outpost, Pike, Ram, Ranged, Resource, SentryTower, Ship, Shipyard, Siege, SiegeTower, Slave, Sling, Soldier, Spear, Stables, Storehouse, Support, Sword, Temple, Town, Trader, Village, Warship, Wonder, Worker

The only legitimate use case for me to have classes is so that auras and techs can query units. But whenever the simulation should change its behavior, there should be a bit in the template being able to switch the specific behavior on and off.

  • It doesn't seem like we have verification that the identity classes given are actually correct, i.e. one can have typos in the strings, whereas trying to refer to undefined template proeprties yields a reference error
  • Classes are a bag of random properties, not ordered, every entity can have every classs, whereas boolean properties can only be defined if the entity has that specific component

Using classes for logic checks is a maxim I don't like seeing followed.

Adding CanGarrison to UnitAI might not be ideal in case we have entities without UnitAI that can be garrisoned.
But instead of adding the property as a string to Identity.Class, adding it as a boolean in the form of Identity.CanGarrison seems better, given the above arguments.

and once we used ungarrisonable, we can't get rid of the class anymore with a garrisonable special filter anymore, right?

In D104#8604, @elexis wrote:

@leper Why do we have FriendlyFire Undeletable Unhealable Pickup DiesInWater Preview Corpse AlwaysVisible RetainInFog KillBeforeGather CanGuard FormationController Sharable Bribable as boolean template options that change simulation behavior if we could make all of them Identity classes?

Because those things were added at different points in time, partially before we had nice identity class matching functions. (I'm also responsible for a fair share of entries in both lists... go figure.)

The only legitimate use case for me to have classes is so that auras and techs can query units. But whenever the simulation should change its behavior, there should be a bit in the template being able to switch the specific behavior on and off.

Classes are also easy to add without having to extend components a lot, and we can push the whole matching thing to technologies or auras or just plain data files, instead of hard-coding it in JS components.

  • It doesn't seem like we have verification that the identity classes given are actually correct, i.e. one can have typos in the strings, whereas trying to refer to undefined template proeprties yields a reference error

That is one of the things where properties do win.

  • Classes are a bag of random properties, not ordered, every entity can have every classs, whereas boolean properties can only be defined if the entity has that specific component

Well, they are, but that is part of what they should be, since you can change quite a few things without having to think about a lot (yes, I am very well aware that this is a mess, but dropping either in favour of the other seems just as bad as the current mess).

Using classes for logic checks is a maxim I don't like seeing followed.

Adding tons of properties is also slightly ugly (especially given that properties should best be non-optional.

Adding CanGarrison to UnitAI might not be ideal in case we have entities without UnitAI that can be garrisoned.

That does not sound like it belongs into UnitAI (at least not as the place that decides upon whether that is the case, once range checks complete). (Take a look at the other CanFoo functions there, most of which do call into the relevant component to do the actual checks.)

But instead of adding the property as a string to Identity.Class, adding it as a boolean in the form of Identity.CanGarrison seems better, given the above arguments.

Well, there are multiple ways to do this, I'm not sure which one of those is the better approach.

In D104#8827, @elexis wrote:

and once we used ungarrisonable, we can't get rid of the class anymore with a garrisonable special filter anymore, right?

Why? We can remove tokens when using inheritance, and dynamic templates are just the same. (Admittedly it might yield a warning in case you are removing something that isn't there, not entirely sure, it has been some time since I've touched that code.)

It doesn't seem like we have verification that the identity classes given are actually correct, i.e. one can have typos in the strings, whereas trying to refer to undefined template proeprties yields a reference error

See rP11043 (and rP18616 :/)

We could add all those classes in a json file and merging them in the Identity schema like resources for the resource agnostic patch?

Sandarac updated this revision to Diff 1267.Apr 15 2017, 10:48 PM

Rebase and use Identity.CanGarrison.

Sandarac marked 2 inline comments as done.Apr 15 2017, 11:01 PM
Sandarac added inline comments.
binaries/data/mods/public/gui/common/gamedescription.js
213 ↗(On Diff #1267)

There is a space here mainly because bb recommended to do so with the else if above in D152.

218 ↗(On Diff #1267)

Maybe this string could be worded a bit better - or be more descriptive.

binaries/data/mods/public/gui/session/unit_actions.js
645 ↗(On Diff #1267)

This is needed so that the tooltip is handled properly.

binaries/data/mods/public/simulation/helpers/Setup.js
50 ↗(On Diff #1267)

There has to be a check for settings.GameType otherwise warnings get thrown in Atlas.

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/776/ for more details.

Sandarac updated this revision to Diff 1270.Apr 16 2017, 1:07 AM

entity.js was missing.

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/779/ for more details.

leper added inline comments.Apr 17 2017, 10:26 PM
binaries/data/mods/public/simulation/components/Identity.js
171 ↗(On Diff #1270)

I guess adding a Garrisonable component would be nicer and not put us in the slightly awkward position of needing an Identity component just for garrisoning.

elexis added inline comments.Apr 18 2017, 5:02 PM
binaries/data/mods/public/simulation/components/Identity.js
171 ↗(On Diff #1270)

Adding a component that doesn't do anything?
How likely is it that we or a modder will add an entity that doesn't have an Identity (therefore most likely can't be owned and selected either)?
Couldn't we say that about most other Identity properties too?

leper added inline comments.Apr 18 2017, 7:13 PM
binaries/data/mods/public/simulation/components/Identity.js
171 ↗(On Diff #1270)

cmpWonder.

It can be owned (cmpOwnership), and it can be selected (cmpSelectable).

Most of the other properties aren't used for a single thing only, or are grouped with others for what is mostly GUI purposes here.

Sandarac updated this revision to Diff 1697.May 6 2017, 11:53 PM

Use a Garrisonable component based on the way that cmpWonder is used, and update after the gamesetup changes in rP19504.

Vulcan added a comment.May 7 2017, 1:07 AM

Build has FAILED

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests).ERROR: JavaScript error: simulation/components/GuiInterface.js line 379
ReferenceError: IID_Garrisonable is not defined
  GuiInterface.prototype.GetEntityState@simulation/components/GuiInterface.js:379:21
  @simulation/components/tests/test_GuiInterface.js:597:25

In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_GuiInterface.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
................................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 306 tests
Success rate: 99%

Link to build: http://jw:8080/job/phabricator/1041/
See console output for more information: http://jw:8080/job/phabricator/1041/console

Sandarac updated this revision to Diff 1748.May 8 2017, 2:05 AM

Update after the changes made in the commits in the last day. The failing test is expected, as this patch adds a new component.

Update after the changes made in the commits in the last day. The failing test is expected, as this patch adds a new component.

You might want to load the corresponding interface file in the failing test.

Sandarac updated this revision to Diff 1752.May 8 2017, 5:26 AM
Vulcan added a comment.May 8 2017, 5:49 PM

Build has FAILED

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests).ERROR: JavaScript error: simulation/components/GuiInterface.js line 379
ReferenceError: IID_Garrisonable is not defined
  GuiInterface.prototype.GetEntityState@simulation/components/GuiInterface.js:379:21
  @simulation/components/tests/test_GuiInterface.js:597:25

In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_GuiInterface.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
................................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 306 tests
Success rate: 99%

Link to build: http://jw:8080/job/phabricator/1078/
See console output for more information: http://jw:8080/job/phabricator/1078/console

Vulcan added a comment.May 8 2017, 6:38 PM

Build is green

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

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

Build is green

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

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

Sandarac added a comment.EditedMay 14 2017, 6:11 AM

Petra is fairly easy to defeat when regicide garrison is disabled, as it will only pick one location to "retreat" the hero to when its health decreases (the closest base/CC), but I am working on some improvements for another diff similar to the regicide AI additions in rP18731 and rP19085.

When it comes to gamedescription.js, I am rather fond of the "Exposed Heroes" label when garrisoning is disabled, but this label doesn't match up exactly to the name of the actual gamesetup option (it's a similar case with Last Man Standing/Allied Victory). So the other option would be to use "Hero Garrison: Enabled" and "Hero Garrison: Disabled" for the label strings so that players know exactly what gamesetup option it refers to when checking the objectives menu in-game.

Sandarac updated this revision to Diff 1940.May 15 2017, 8:32 AM

Use "Hero Garrison: Enabled" and "Hero Garrison: Disabled" for the label strings in gamedescription.js.

Build is green

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

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

elexis added inline comments.May 15 2017, 3:45 PM
binaries/data/mods/public/gui/common/gamedescription.js
237 ↗(On Diff #1940)

I am rather fond of the "Exposed Heroes" label

Me too. That colon isn't too nice, since we will have Setting: Boolean: Description, i.e. two colons. The Exposed Heroes and the gamesetup controller should still be capable of figuring out which option determines this label.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
652 ↗(On Diff #1940)

(This will keep the setting saved even if the victory condition is changed again, but fixing this is out-of-scope as it isn't trivial.)

1525 ↗(On Diff #1940)

(Correct, must be in this check because it's a user setting, as opposed to the properties deleted in the loop above.)

binaries/data/mods/public/gui/session/unit_actions.js
645 ↗(On Diff #1267)

The first clause is obsolete now right? Thus the hardcoding can be removed.

binaries/data/mods/public/maps/scripts/Regicide.js
73 ↗(On Diff #1940)

(Same as relics - what if all territory is owned)

76 ↗(On Diff #1940)

(Why do we get the 3D position, never use the third coordinate and then convert it to 2D? horizDistanceToSquared works too (bit faster than horizDistanceTo and comparing the square one works just as well).)

75 ↗(On Diff #378)

Lets just imagine there is a better map than sicilia nomad demonstrating naval nomad. One where you actually had a choice which island you would settle on. (Would be fun on most naval maps we have actually).
Wouldn't It be preferable to force-garrison the hero on start and prevent garrisoning it again as leper proposed?

binaries/data/mods/public/simulation/ai/common-api/shared.js
117 ↗(On Diff #1940)

(Stacking hardcodings is rarely joyful)

126 ↗(On Diff #1940)

(ent[key] = key == "Garrisonable" ? "false" : base[key] or not)

binaries/data/mods/public/simulation/templates/template_unit.xml
30 ↗(On Diff #1940)

(We now have the choice to keep or remove the Garrisonable component from some units like domestic and non-domestic animals.
mimo mentioned, I think it was in the context of #4030, that we should avoid having two places where to save the information where a unit can be garrisoned for exactly that reason. But since this is kept trivial, you made an offer I can't refuse.)

Sandarac added inline comments.May 16 2017, 8:21 AM
binaries/data/mods/public/gui/common/gamedescription.js
237 ↗(On Diff #1940)

Exposed Heroes it is then.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
652 ↗(On Diff #1940)

(Yes, also an issue for RelicCount and VictoryDuration, and it's not so nice when these are written to the commands.txt for no reason.)

binaries/data/mods/public/gui/session/unit_actions.js
645 ↗(On Diff #1267)

Okay.

binaries/data/mods/public/maps/scripts/Regicide.js
73 ↗(On Diff #1940)

Okay.

76 ↗(On Diff #1940)

Okay.

75 ↗(On Diff #378)

But that would involve doing the exact opposite of what this gamesetup option is supposed to do, and that is simply the strongest argument against doing that. Your hero (maybe heroes at some point) is supposed to be vulnerable (read: not garrisoned (hence "Exposed" heroes)). Not only that, but in order to allow heroes to start garrisoned when garrisoning is disabled would likely involve all sorts of hackery somewhere, and so I think it just isn't a good idea - especially at this point when we only have this one Skirmish map where units start on ships. Maybe at some point if other people propose new maps where units start garrisoned (and that's a big if), and they are accepted and included in the game, and then if it is deemed really important to have heroes start on ships (when they logically shouldn't be able to if one reads what this option actually toggles), then I guess it could be done in a new diff.

binaries/data/mods/public/simulation/ai/common-api/shared.js
117 ↗(On Diff #1940)

(Yes, and there are plans to remove all these hardcoded checks for the AI.)

The Athenian hero Themistocles is intended to be garrisoned in a ship; it's his main feature.

Sandarac updated this revision to Diff 2046.May 20 2017, 5:24 PM

Update after rP19614, remove unneeded check in unit_actions.js, and handle maps with no neutral territory.

Build is green

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

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

Sandarac updated this revision to Diff 2070.May 21 2017, 3:37 AM

Regicide.js cleanup.

Sandarac updated this revision to Diff 2071.May 21 2017, 3:54 AM

Check for UnitMotionFlying.

Sandarac updated this revision to Diff 2072.May 21 2017, 4:25 AM

Move a function to TriggerHelper and use it for regicide and capture the relic to remove duplication.

Build is green

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

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

Build is green

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

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

Build is green

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

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

Sandarac updated this revision to Diff 2088.May 21 2017, 6:41 PM

More style fixes after discussion with @elexis. Move the regicide garrison option below the name of the victory condition in gamedescription, check for capture_the_relic in Setup.js, rename potentialSpawnPoints to potentialNeutralSpawnPoints in TriggerHelper.js

elexis accepted this revision.May 22 2017, 12:40 AM

Thanks for the patch (and having it rewritten from scratch a couple of times) and addressing all remarks (mostly on irc). Not certain about those AI files, but in my tests it worked without problems.

Build is green

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

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

This revision was automatically updated to reflect the committed changes.
elexis added inline comments.May 22 2017, 3:19 AM
binaries/data/mods/public/maps/scripts/Regicide.js
10 ↗(On Diff #2088)

can be a local variable

binaries/data/mods/public/maps/scripts/TriggerHelper.js
172 ↗(On Diff #2088)

removing the potential

194 ↗(On Diff #2088)

inverting, so that it's more straight forward to read