Page MenuHomeWildfire Games

Use Garrison instead of PerformGarrison when autogarrisoning after/while spawning.

Authored by Freagarach on Jun 6 2020, 8:43 AM.


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
rP23743: Fix arrow count not being properly calculated when autogarrisoning.

rP23630 Moved the message sent that an entity garrisons from PerformGarrison to Garrison. However, when an entity is autogarrisoned from ProductionQueue, PerformGarrison is called thus not triggering the message. When ejecting the entity from the structure there is a message sent that the entity is removed, thus allowing for a negative amount of archers/arrows in BuildingAI (see rP23630#42654).
This diff replaces PerformGarrison in the ProductionQueue with Garrison.

Note that PerformGarrison was explicitly split in rP14144 when introducing autogarrisoning. It probably has something to do with the position, since that was split. But I couldn't find any reason why it cannot be used now.

A side effect of this is that when autogarrisoning an entity with visible garrison points those will be occupied as well now.

Since PerformGarrison is now unused, and rightly, the two functions are combined again.

Test Plan

Test that normal garrisoning works and autogarrisoning also.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Freagarach created this revision.Jun 6 2020, 8:43 AM
wraitii requested changes to this revision.Jun 6 2020, 8:55 AM
wraitii added a subscriber: wraitii.

This prevents auto-garrisoning of entities without position, which ought to be legal imo.

Realistically, Garrison & PerformGarrison seem like they should be merged back together. The cmpPosition early-return is a mistake, just check for it before calling MoveOutOfWorld.

This would force a rebase of D2367

This revision now requires changes to proceed.Jun 6 2020, 8:55 AM
Vulcan added a comment.Jun 6 2020, 9:09 AM

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

Link to build:

Freagarach updated this revision to Diff 12166.Jun 6 2020, 11:13 AM
  • Combine the two Garrison-functions.
  • Move some checks to IsAllowedToGarrison.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build:

Any chance you could add a test case?

This seems a very good change regardless

193 ↗(On Diff #12166)

I feel like this should return true if there is no this.template.List ?

The case is now (with the combination of the functions) covered by the tests introduced by rP23630. ProductionQueue was using the "wrong" function, to which no test can hold ;)

193 ↗(On Diff #12166)

To be done in a separate patch.

Freagarach edited the summary of this revision. (Show Details)Jun 6 2020, 11:52 AM
wraitii accepted this revision.Jun 6 2020, 12:14 PM

Bug reproduce, fixes as advertised + good cleanup => commit

232 ↗(On Diff #12166)

For D2367 this dependency should be removed, call MoveOutOfWorld inside the if instead. I think it's fine to leave as-is for now.

This revision is now accepted and ready to land.Jun 6 2020, 12:14 PM
This revision was landed with ongoing or failed builds.Jun 6 2020, 12:19 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review and commit @wraitii :)