Page MenuHomeWildfire Games

Enable garrisoning on gates / fix movement of units with visible garrison points
Needs ReviewPublic

Authored by wraitii on Mar 27 2018, 2:43 AM.

Details

Reviewers
temple
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#2679
#5151
Summary

This addresses two related issues:

  • Units visibly garrisoned on gates keep the gate open.
  • Units visibly garrisoned on entities keep their pathfinding blocker flags.

The second is fixed by switching gates to look for "block movement" flag, instead of block-construction, and by removing the "block-movement" flag of visibly garrisoned entities.
This also fixes #5151 .

The first is fixed by changing some logic to consider the ability of units to move.
Add a "SetImmobile" function to UnitAI, to mark a unit as incapable of moving (though it theoretically could).
Change from IsTurret checks to use that function instead.
Packing logic is kept separate as it's trickier.

With this, gates can subscribe to those events and can choose to ignore units that can't move (since it makes no sense to open the gate for those), fixing the issue.

Attached is a small replay of some cases that wouldn't work properly on SVN.

Test Plan

Review code.

Event Timeline

temple created this revision.Mar 27 2018, 2:43 AM
temple edited the summary of this revision. (Show Details)
Stan added a comment.EditedMar 27 2018, 7:18 AM

Thanks so much for working on this.

Stan awarded a token.Mar 27 2018, 7:19 AM
temple added inline comments.May 1 2018, 5:32 PM
binaries/data/mods/public/simulation/components/Gate.js
69–70
wraitii added a reviewer: Restricted Owners Package.May 14 2018, 11:53 AM
Imarok added a subscriber: Imarok.May 14 2018, 1:04 PM
Imarok added inline comments.
binaries/data/mods/public/simulation/components/Gate.js
69–70

There is a ticket now: #5151

wraitii requested changes to this revision.May 14 2018, 1:37 PM
wraitii added a subscriber: wraitii.

Nice bug.

My input:

  • This happens because we don't move visible-garrisoned units out of the world. This makes sense if these are somewhat independent, for example in the case of autonomous drones and so on.
  • It should probably be a GarrisonHolder property whether garrisoning removes your "collision" or not.

Thus the correct way to fix this, imo, is to add a parameter to GarrisonHolder (and other components that have the same behaviour), instead of changing Gate. We may want buildings where visible garrison points are obstructions.

On the precise mechanism, calling the pathfinder seems reasonable enough.

This revision now requires changes to proceed.May 14 2018, 1:37 PM
wraitii added inline comments.May 14 2018, 1:52 PM
binaries/data/mods/public/simulation/components/Gate.js
174

This seems unrelated?

Read this better, there's two things here. On the "remove obstruction" part, see above.

These units should indeed not be counted towards opening the gate. I think however your current method won't work: say we have two garrisoned gates very close to each other, I think they'll get counted. The distinctive criteria here is that garrisoned units cannot move, they're static. I'm not entirely sure that's easily transatable in code though, maybe we need to add a "CutMyLegsOff" to CmpUnitMotion like we have "moveOutOfWorld" for CmpPosition.

binaries/data/mods/public/simulation/components/Gate.js
174

Ah right no I understand now.

wraitii commandeered this revision.Jul 13 2019, 7:40 PM
wraitii edited reviewers, added: temple; removed: wraitii.
wraitii updated this revision to Diff 8862.Jul 13 2019, 7:43 PM

More involved fix involving four things:

  • Gates open for units blocking movement, not construction (my opinion is that this overall makes more sense too).
  • Gates open only if units nearby 'can move', a new unitAI state
  • UnitAI keeps track of whether it can move or not. Packing moves the entity to Immobile, garrisoning too.
  • Garrison holders remove the obstruction of garrisoned entities (this is the incidental fix for #5151)

Also use the opportunity to use this "AbleToMove" function in UnitAI elsewhere.
Regarding my above comment - if we want buildings where visible obstruction points _are_ obstructions, we'll handle that then.

This correctly handles the case of two gates close to each other with visible garrison units inside each of them, unlike the previous patch.

wraitii updated the Trac tickets for this revision.Jul 13 2019, 7:44 PM

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

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

Nescio added a subscriber: Nescio.Thu, May 21, 7:12 PM

@wraitii, this looks promising; any idea when it'll be finished? Being able to garrison units on gates without interfering with opening and closing would certainly be a great improvement.

wraitii updated this revision to Diff 11976.Fri, May 22, 5:23 PM
wraitii retitled this revision from Enable garrisoning on gates to Enable garrisoning on gates / fix movement of units with visible garrison points.
wraitii edited the summary of this revision. (Show Details)
wraitii edited the test plan for this revision. (Show Details)

Rebase and slight rework.

See summary.

This patches only adds the ability to Iberia (cause I forgot to do it for the others), I'll do that before committing.

Owners added a subscriber: Restricted Owners Package.Fri, May 22, 5:23 PM

Mh, the turret code is actually newer than my "AbleToMove" changes and technically it's useless right now, but I guess I'll leave it in since it could be useful to freeze a unit or something.

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/792/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1674/display/redirect

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

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

This patches only adds the ability to Iberia (cause I forgot to do it for the others), I'll do that before committing.

Perhaps it's better if you focus this patch on the necessary mechanics changes. The template changes can be done in a separate patch afterwards, also because gates can have different heights than wall segments.