*
Details
Details
- Reviewers
mimo - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP20137: Fix broken Autogarrison in rP19927
Check everything works as expected.
Diff Detail
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
Comment Actions
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! Checking XML files...
http://jenkins-master:8080/job/phabricator/1992/ for more details.
Comment Actions
Yes, that the first solution which come to mind, but i'm not sure this is the best one: when units are not spawned, they will send two consecutives messages for ownership changed, possibly being accounted in summaries counts, updating entityLimits (hopefully increasing then decreasing it), ... This should be quite rare, but still.
I wonder if it would not be better to add a force argument to PerformGarrison, true when called from productionQueue, and don't do the ownership test in that case? Let's get opinions from other devs.
Comment Actions
In ICmpOwnership there is a setownerquiet that doesn't end message, maybe that'll be better ?
Comment Actions
No because when everything is ok (spawning succeeded or garrison succeeded), we want the messages to be sent.
Comment Actions
If fact, that could work with a temporary affectation using SetOwnerQuiet, something like (not tested)
Index: binaries/data/mods/public/simulation/components/ProductionQueue.js =================================================================== --- binaries/data/mods/public/simulation/components/ProductionQueue.js (révision 20135) +++ binaries/data/mods/public/simulation/components/ProductionQueue.js (copie de travail) @@ -608,12 +608,23 @@ { var ent = this.entityCache[0]; - if (cmpAutoGarrison && cmpAutoGarrison.PerformGarrison(ent)) + let garrisoned; + if (cmpAutoGarrison) { + // Temporary owner affectation needed for Garrison checks let cmpNewOwnership = Engine.QueryInterface(ent, IID_Ownership); + cmpNewOwnership.SetOwnerQuiet(cmpOwnership.GetOwner()); + garrisoned = cmpAutoGarrison.PerformGarrison(ent)) + cmpNewOwnership.SetOwnerQuiet(-1); + } + + if (garrisoned) + { + let cmpNewOwnership = Engine.QueryInterface(ent, IID_Ownership); cmpNewOwnership.SetOwner(cmpOwnership.GetOwner()); let cmpUnitAI = Engine.QueryInterface(ent, IID_UnitAI); - cmpUnitAI.Autogarrison(this.entity); + if (cmpUnitAI) + cmpUnitAI.Autogarrison(this.entity); } else {
Comment Actions
Test and use @mimo 's approach. Only query for new owner once, move the owner setting outside of the if else block as there is a break and it won't be executed anyway. That cleans it up a bit. Remove useless comments, and remove an else.
Add the check for UnitAI not sure it's needed.
Comment Actions
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! Checking XML files...
http://jenkins-master:8080/job/phabricator/1993/ for more details.