Page MenuHomeWildfire Games

Town bell - remove alert status on units
ClosedPublic

Authored by temple on Jun 26 2017, 9:22 PM.

Details

Reviewers
causative
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP21074: Remove the alert status on units
Trac Tickets
#4185
#4656
Summary

There are a lot of issues with the town bell. For example:

  • If an alert raiser is destroyed, there's no way to unring the bell, so units don't respond to new bells.
  • #4656: "When a building is captured the alert is not reset. So the new owner still have its building under alert."
  • D690: "When an alertRaiser is captured, its units under alert should not be controllable by the enemy."
  • If units are garrisoned before ringing the bell, they aren't included in the alert so they have to be tasked back to work manually. (Often when being raided you'll garrison some women in houses at the edge of your base before ringing the bell later when the attack becomes larger. It's very easy to forget about those women for the rest of the game, and it would be nice if they returned to work at the same time as the other garrisoned units.)
  • #4185: "Unit does not react to alert when ungarrisoned while alerted". (Often when the danger has passed in one area you'll ungarrison some women to go back to work, but then the next time you hit the town bell they won't respond.)
  • If more units come into the area (or units are promoted), to put them under alert you have to unring the bell then ring it again.
  • If units are redirected before they garrison, say to build something, when the bell is unrung they'll resume their previous orders rather than continuing to build.

Basically, it's very messy.

causative had a nice idea:

No alert status on units!  This would greatly simplify the implementation and prevent bugs.  Just two bells, "alert" and "safe," that you can ring at any time.  When the "alert" bell is rung, all women in range try to garrison, and remember their previous task.  If a woman is working and you select her and tell her to garrison or move (without using the bell), she will also remember her previous task.  When the "safe" bell is rung, all women in range that are garrisoned (or garrisoning, walking, or idle) will ungarrison and return to their previous task, like the back to work command.

In this patch I've removed the alert status on units and production buildings. When you ring the bell, units will try to garrison in a building (or move to the cc if they're all full or will be full). When you unring the bell, units will ungarrison and go back to work, or stop and go back to work if they haven't garrisoned yet.

In another patch (D937) I'll do the following:

  • Allow garrisoning in allied buildings
  • Replace the alert level with matching classes
  • Add a bell at the market for trade carts
  • Change the GUI to always display two buttons: alert (garrison) and safe (back to work)
  • Possibly hotkeys

I've also incorporated Stan's cleanup from D403.

Test Plan

Test that everything works as described.

Do we lose anything by removing the alert status?

Production buildings don't have the alert status, meaning newly created units won't auto-garrison. This was a closed ticket, #2342, so the change counts as a regression, but I don't think it's a big deal. In the second patch I play on removing levels, which counts as another regression, but I don't think they're necessary. (We could do both levels and classes, although the GUI might get complicated.)

The alert status on units was used when finding new garrison targets when the current target was full, but now we use causative's idea of keeping track of the reserved spots so that's unnecessary. I've implemented this very simply, with units closest to the alert raiser having the first choice of garrison holders. Because units might garrison in a building outside the alert range, when we unring the bell we have to use a larger search radius for garrison holders.

There might be performance issues with redoing the calculations repeatedly if people keep hitting the alert button (I haven't changed the gui yet), so if that seems like a problem we might need to keep track of the time and not redo the calculation if it's on the same turn or whatever.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4100
Build 7207: Vulcan BuildJenkins
Build 7206: arc lint + arc unit

Event Timeline

temple created this revision.Jun 26 2017, 9:22 PM
fatherbushido added a subscriber: fatherbushido.EditedJun 26 2017, 9:43 PM

(should come after D403 ideally)
(and those - possibly broken things - should be caught in D403 tests btw)

Imarok added a subscriber: Imarok.Jun 26 2017, 9:47 PM

Just quickly tested one thing:
garrison some dudes in a tower, then later raised alert and ended it again: my dudes also were ungarrisoned.
I don't think this is a good behaviour, because when I put my soldiers into a tower and let's say 20 mins later raise and end an alert, I want them stay in the tower.

If a building is captured or destroyed, ejected units will try to find a new building to garrison inside.

That may not be a good thing. I often will set a rally point for a building before it's destroyed, so that the units inside will run away to the desired location when the building is destroyed.

By the way, don't know if you are planning to do this, but there's one change to the bell that could make it a lot smarter: don't have 30 units run to a building that only has 20 garrison limit. I might do this at some point if you don't. It would work like this: Each building should track the number of "reserved spots" for units with a garrison order at the building who have not yet reached the building. Currently, the bell will direct units to the building if the building has available garrison space, but with this change the bell would direct units to the building only when current garrison + reserved spots < max garrison.

The net effect of this change would be to reduce the time women mill about vulnerably when the bell is rung, which would reduce the impact of cavalry raids a bit.

temple planned changes to this revision.EditedJun 28 2017, 10:05 PM

I was thinking of adding something like UnitAI.prototype.GetGarrisonHolder() used in D685 but didn't know exactly how to do it (and didn't want to mess too much with UnitAI if I could help it). That should simplify this code a lot, if AlertRaiser doesn't need to keep track of where units are garrisoned.

The philosophy of the town bell (to me) is that everyone should drop what they're doing and follow the emergency orders: run and hide. When the alert is lifted everyone should come out and return to what they were doing previously. I have responses to Imarok and causative but since even my modest changes might be controversial, I think it's probably better to have a discussion in the forums (after a22).

In D681#27554, @temple wrote:

I was thinking of adding something like UnitAI.prototype.GetGarrisonHolder() used in D685 but didn't know exactly how to do it (and didn't want to mess too much with UnitAI if I could help it). That should simplify this code a lot, if AlertRaiser doesn't need to keep track of where units are garrisoned.

The philosophy of the town bell (to me) is that everyone should drop what they're doing and follow the emergency orders: run and hide. When the alert is lifted everyone should come out and return to what they were doing previously. I have responses to Imarok and causative but since even my modest changes might be controversial, I think it's probably better to have a discussion in the forums (after a22).

I think only those who where garrisoned because of the town bell, should come out and return to work when the alert is lifted.
(Maybe a trac ticket would be a better place to discuss this)

Stan added a subscriber: Stan.Aug 8 2017, 10:52 AM

This probably needs rebasing now that D90 is commited and some other tickets touched UnitAI.js since then.

temple added a comment.Aug 8 2017, 2:46 PM

I posted in the forums and liked causative's ideas here.

temple updated this revision to Diff 3787.Sep 29 2017, 4:01 AM
temple retitled this revision from Town bell (alert) fixes and improvements to Town bell - remove alert status on units.
temple edited the summary of this revision. (Show Details)
temple edited the test plan for this revision. (Show Details)
temple updated the Trac tickets for this revision.

See summary.

temple updated this revision to Diff 4645.Dec 8 2017, 5:00 AM
temple set the repository for this revision to rP 0 A.D. Public Repository.

rebase

Vulcan added a subscriber: Vulcan.Dec 8 2017, 5:16 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
elexis added a reviewer: Restricted Owners Package.Dec 8 2017, 11:31 AM
temple edited the summary of this revision. (Show Details)

D937 is the follow-up patch.

temple updated this revision to Diff 4673.Dec 9 2017, 7:56 PM

rebase, some tests

Vulcan added a comment.Dec 9 2017, 7:58 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
temple added inline comments.Dec 9 2017, 7:59 PM
binaries/data/mods/public/simulation/components/AlertRaiser.js
90

inline

causative requested changes to this revision.EditedDec 12 2017, 12:49 PM

Looks good except for one thing: if you raise an alert and a unit tries to garrison beyond the alert range, and then you cancel the alert before the unit finishes garrisoning but after the unit has walked beyond the alert range, it will continue to garrison. My suggestion would be to filter the list of buildings a unit may garrison in, to exclude buildings located beyond the alert range from the alert raiser. With that change, when you end the alert, you only need to check buildings out to the alert range instead of alert range + search range.

This revision now requires changes to proceed.Dec 12 2017, 12:49 PM

Looks good except for one thing: if you raise an alert and a unit tries to garrison beyond the alert range, and then you cancel the alert before the unit finishes garrisoning but after the unit has walked beyond the alert range, it will continue to garrison. My suggestion would be to filter the list of buildings a unit may garrison in, to exclude buildings located beyond the alert range from the alert raiser. With that change, when you end the alert, you only need to check buildings out to the alert range instead of alert range + search range.

This is where an alert status would've helped. The problem I saw was that we want units to garrison in the nearest building, even if it's outside the alert range. Maybe we can have a "buffer range" (maybe not the best name) to deal with that, rather than using the search range.

temple updated this revision to Diff 4748.Dec 12 2017, 6:51 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
causative accepted this revision.Dec 13 2017, 7:10 AM

Looks good. Although I don't think it's necessary to have a separate buffer range. If the alert range (140m) doesn't include all the desired garrison buildings, you could just increase the alert range a bit. I think 140m should be enough for all alerted units and buildings.

I'm not 100% certain about our Javascript floating point math (as used here for filtering buildings based on distance from the alert raiser). It doesn't seem like it would always be deterministic on all platforms, and I don't know why it doesn't cause desyncs. Do you know? Or maybe it does cause desyncs. If Javascript math doesn't cause desyncs, maybe we can get away with using floating point math in C++ as well. Anyway, it's used in multiple places so if there's a problem with it, it's beyond the scope of D681.

This revision is now accepted and ready to land.Dec 13 2017, 7:10 AM

Looks good. Although I don't think it's necessary to have a separate buffer range. If the alert range (140m) doesn't include all the desired garrison buildings, you could just increase the alert range a bit. I think 140m should be enough for all alerted units and buildings.

The question is what to do about units that are just inside the range that are next to buildings that are just outside the range. There's always that possibility, whatever the alert range. I think it's more natural for the units to garrison inside the nearby buildings rather than force them to find buildings strictly inside the alert range.

I'm not 100% certain about our Javascript floating point math (as used here for filtering buildings based on distance from the alert raiser). It doesn't seem like it would always be deterministic on all platforms, and I don't know why it doesn't cause desyncs. Do you know? Or maybe it does cause desyncs. If Javascript math doesn't cause desyncs, maybe we can get away with using floating point math in C++ as well. Anyway, it's used in multiple places so if there's a problem with it, it's beyond the scope of D681.

I don't know. There's this comment in Math.js:

/**
 * Safe, platform consistent implementations of some Math functions
 *
 * These functions are implemented in JS to avoid observed differences
 * between results of different floating point libraries, see
 * https://bugzilla.mozilla.org/show_bug.cgi?id=531915
 *
 * They mostly meet the ECMAScript Edition 5 spec, see
 * http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf
 *
 * See simulation/components/tests/test_Math.js for tests.
 */
temple updated this revision to Diff 4765.Dec 13 2017, 4:00 PM

Only ungarrison the owner's units.

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
temple added inline comments.Jan 30 2018, 2:45 AM
binaries/data/mods/public/simulation/components/AlertRaiser.js
89

I'll replace this with WalkToTarget for now.

binaries/data/mods/public/simulation/helpers/Entity.js
20

This optimization didn't seem to have much of an effect on performance, so I'll skip it.

This revision was automatically updated to reflect the committed changes.