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

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

Details

Reviewers

wraitii

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP23743: Fix arrow count not being properly calculated when autogarrisoning.

Summary

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

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

Freagarach created this revision.Jun 6 2020, 8:43 AM

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

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

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

Harbormaster completed remote builds in B11964: Diff 12162.Jun 6 2020, 9:09 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: https://jenkins.wildfiregames.com/job/docker-differential/2335/display/redirect

Harbormaster failed remote builds in B11968: Diff 12166!Jun 6 2020, 11:14 AM

Any chance you could add a test case?

This seems a very good change regardless

binaries/data/mods/public/simulation/components/GarrisonHolder.js
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 ;)

binaries/data/mods/public/simulation/components/GarrisonHolder.js
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

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

binaries/data/mods/public/simulation/components/GarrisonHolder.js
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.