Page MenuHomeWildfire Games
Differential D192

CanPatrol to selectively disable patrol ability and fix GUI
ClosedPublic
Actions

Authored by elexis on Mar 5 2017, 8:25 AM.

Details

Reviewers
vladislavbelov
Commits
rP19455: Add UnitAI CanPatrol boolean property to selectively disable patroling for some…
Summary

Since the introduction of the patrol feature, the patrol icon has been visible for sheep, even though animals can't patrol. The button should be hidden without hardcoded animal checks.

However we might want to disable patroling of some templates selectively and some mods or maps might want to add patroling animals, f.e.:
From https://en.wikipedia.org/wiki/Lion

The males associated with a pride tend to stay on the fringes, patrolling their territory

Changing the ANIMAL UnitAI state to work with patroling is not in the scope of this diff.

The new CanPatrol boolean is used to disable patroling of traders, fishing ships the hawk and to fix the GUI button of domestic animals without hardcoded checks in the simulation or GUI.

Test Plan

Test the patrol hotkey, patrol rallypoint of buildings, domestic animals, fishing ships, merchant ships and whatever

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

elexis created this revision.Mar 5 2017, 8:25 AM
Harbormaster completed remote builds in B686: Diff 719.Mar 5 2017, 8:25 AM
Sandarac added a subscriber: Sandarac.Mar 5 2017, 8:29 AM

Surely the bugs introduced in UnitAI in rP19254 should be fixed before any more changes to UnitAI are proposed?

elexis mentioned this in D176: Fix UnitAI issues in rP19254.Mar 5 2017, 9:25 AM
Harbormaster completed remote builds in B686: Diff 719.Mar 5 2017, 10:29 AM
Vulcan added a subscriber: Vulcan.Mar 5 2017, 10:29 AM

Build is green

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

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

elexis added a comment.Mar 6 2017, 3:23 AM
In D192#7315, @Sandarac wrote:

Surely the bugs introduced in UnitAI in rP19254 should be fixed before any more changes to UnitAI are proposed?

Since sheep won't be able to patrol either way and the diff doesn't patch the same code, it can be fixed before or after

vladislavbelov requested changes to this revision.Mar 30 2017, 6:00 PM
vladislavbelov added a subscriber: vladislavbelov.
vladislavbelov added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
235 ↗(On Diff #719)

If elexis likes to minimize code if possible, then it should be like next:

return entState.unitAI && entState.unitAI.canPatrol && { "possible": true };
binaries/data/mods/public/simulation/components/UnitAI.js
5051 ↗(On Diff #719)

The same here:

return this.IsFormationController() || this.template.CanPatrol == "true";
This revision now requires changes to proceed.Mar 30 2017, 6:00 PM
elexis marked 2 inline comments as done.EditedMar 31 2017, 5:00 AM

Does this proposal sound useful at all?
Is a boolean better than an Identity class?
Are the template changes complete?
Does it break the GUI in any way?
Can I break the commit guidelines by not adding a test as usual?

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

Against it would speak that this is the first paragraph where we don't have early returns and that people don't understand that && conjunctions return the last value if all previous ones are true and false otherwise.

On third thought, I agree, elexis does like to minimize code if possible.

binaries/data/mods/public/simulation/components/UnitAI.js
5049 ↗(On Diff #719)

(Usually we want to avoid copy n pasted comments and have them only in one place of the code.)

5595 ↗(On Diff #719)

should keep this empty line I guess

elexis updated this revision to Diff 1024.Mar 31 2017, 5:02 AM
elexis edited edge metadata.
elexis marked an inline comment as done.

Minify two statements

Vulcan added a comment.Mar 31 2017, 5:47 AM

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

Harbormaster completed remote builds in B961: Diff 1024.Mar 31 2017, 5:47 AM
vladislavbelov requested changes to this revision.Apr 1 2017, 11:51 PM
vladislavbelov added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
243 ↗(On Diff #1024)

Here we could short too, but it'll be too long line.

1279 ↗(On Diff #1024)

I think we could use the same logic about minizing here too.

binaries/data/mods/public/simulation/components/GuiInterface.js
385 ↗(On Diff #1024)

Just wondering, may it be that we will need to use "isPatroling"?

This revision now requires changes to proceed.Apr 1 2017, 11:51 PM
vladislavbelov added a comment.EditedApr 2 2017, 12:06 AM
In D192#10821, @elexis wrote:

Does this proposal sound useful at all?
Is a boolean better than an Identity class?
Are the template changes complete?
Does it break the GUI in any way?
Can I break the commit guidelines by not adding a test as usual?

  1. Yes, at least it's useful for mods, where we could allow/deny patrol for some units.
  2. Yes, why it should be an Identity class?
  3. What's about template_unit_mechanical_siege?
  4. I don't see cases while.
  5. Test plan or usual tests?
elexis requested review of this revision.Apr 21 2017, 6:56 PM
elexis edited edge metadata.
elexis marked 5 inline comments as done.
In D192#11113, @vladislavbelov wrote:
In D192#10821, @elexis wrote:

Does this proposal sound useful at all?
Is a boolean better than an Identity class?
Are the template changes complete?
Does it break the GUI in any way?
Can I break the commit guidelines by not adding a test as usual?

  1. Yes, at least it's useful for mods, where we could allow/deny patrol for some units.
  2. Yes, why it should be an Identity class?
  3. What's about template_unit_mechanical_siege?
  4. I don't see cases while.
  5. Test plan or usual tests?
  1. Ok (I guess it's also nice to remove patrol ability from those few units)
  2. Agree :P We could also have a new Patrollable component, but that isn't right either since we need UnitAI in order to patrol. So go along, there's nothing to see here.
  3. Siege engines should be able to patrol too. D204 uses it for example.
  4. Me neither, good.
  5. (Well the coding conventions say we must add a test for everything)

Thanks for making that clear. (We want our reviews to be complete, otherwise we might end up having to add a "fix the previous commit" and a reviewer should state everything he checked so we can lookit up in months later to see if there was a reason for having things the way they were done)

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

Meh, changed my mind again, since it's not widely known how && works actually x)

243 ↗(On Diff #1024)

Yep, better keep as is, in particular since every property ought to be on a separate line

1279 ↗(On Diff #1024)

see above

binaries/data/mods/public/simulation/components/GuiInterface.js
385 ↗(On Diff #1024)

isGuarding is used for the GUI to toggle the icon. So we will need isPatroling once we change the GUI somehow if patorling is active.

elexis updated this revision to Diff 1412.Apr 21 2017, 6:57 PM
elexis marked 4 inline comments as done.

Revert that one code minimization

Vulcan added a comment.Apr 22 2017, 12:10 AM

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

Harbormaster completed remote builds in B1228: Diff 1412.Apr 22 2017, 12:10 AM
vladislavbelov accepted this revision.Apr 24 2017, 7:33 PM
This revision is now accepted and ready to land.Apr 24 2017, 7:33 PM
elexis added a comment.Apr 25 2017, 1:21 PM

Well, we did forget some units:

  • Besides Spartans, female citizens can't attack most units, so they shouldn't be expected to be able to patrol either
  • The mauryan worker elephant

Also tested with the Petra bot since that can break when changing the simulation and templates.

Thanks for the review!

Closed by commit rP19455: Add UnitAI CanPatrol boolean property to selectively disable patroling for some… (authored by elexis). · Explain WhyApr 25 2017, 1:22 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1482

View Options

ps/trunk/binaries/data/mods/public/gui/session/unit_actions.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_formation.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_fauna.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_fauna_decorative.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_mechanical_ship_fishing.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_mechanical_ship_merchant.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_support_female_citizen.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_support_trader.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/units/maur_support_elephant.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/templates/units/spart_support_female_citizen.xml

Loading...
Wildfire Games · Phabricator