[PATCH] Fix gate closing range for enemy units

[historic_bruno]

Current behavior is that gates open once a player unit is within passRange of the gate and they close when no units are within that same range. This creates a problem with enemy units as they can be relatively far away yet keep the gate from closing or locking. They should have a much closer distance check than ally/player-owned units.

See #619 for related comments.

[Deiz]

Building upon what was described on IRC, the patch behaviour is as follows:

Gate closing only fails if a unit is colliding with the gate. If this occurs, a timer will run every 500ms to check the collision status and will close as soon as there are no more colliding units.

All gates have this sanity check. Unlocked gates additionally do the usual check for allies in range and open if there are any.

I modified the range query accordingly; it now only cares about allied units.

As for the new engine function, GetStructureUnitCollisions(): The construction-specific GetConstructionCollisions() cares about collisions with both units and structures. This isn't normally a problem with player-built walls (presumably due to control groups?), but results in gates being held open constantly on the Fortress random map. It's a problem that could be solved there, but I opted to be safe and filter to only check for collisions with units.

[Deiz]

Reworked the engine side of the patch somewhat. I noticed that GetConstructionCollisions() only had one call site, the same as my newly-added GetStructureUnitCollisions() and opted to combine the two into a generic function that handles both cases (and potentially others).

The function assumes (as does the engine in several other places) that structures block everything, while units only block movement and construction.

[historic_bruno]

Works great, let's get this in A11 :)

The only problem I noticed is that a timer could still be running when the Gate is destroyed, so make sure to clean that up in the OnDestroy handler.

Things that could be clarified with comments:

• In Gate.js, for CloseGate() you might mention the condition for closing in its comment, since the collision detection logic is in there
• the logic in CCmpObstruction.cpp between lines 576-589 is weird and unintuitive, you might explain more clearly why those flags, which apparently don't correspond directly to units or structures (so it's not obvious), are used and also what exactly invertMatch does.
• similarly add to the comment for SkipControlGroupsRequireFlagObstructionFilter to explain the new "exclude" parameter

Thinking about it more, could/should we add a new flag to avoid that extra "exclude" parameter? Or maybe add a different set of flags that semantically reflect the unit/structure distinction. Not a major concern other than reducing the effort required to understand how the code works, if it's likely to be reused :) (I'd even be content with a TODO or Trac ticket for that, if what I'm saying makes sense)

[Deiz]

I don't believe it's possible to avoid the exclude parameter without modifying lower-level functions, specifically TestStaticShape and TestUnitShape, as both iterate over units and structure obstructions, and so it's necessary to rely on mapping the four obstruction flags to units and structures.

The type of an obstruction is known (via m_Type) within CCmpObstruction. It seems this logic is deliberately not exposed to the obstruction manager, though I don't know the intent behind that.

Anyhow, in the new version of the patch, I've documented things a bit more thoroughly and rewrote SkipControlGroupsRequireFlagObstructionFilter to be a bit more intuitive (to my eyes). I confirmed that it still passes all the tests.

[historic_bruno]

Good enough for me! (Though I'm curious why you changed the timer to instant?)

[Deiz]

I changed it for behavioural consistency, mostly. 500ms is the turn length in multiplayer, so chances are an obstructed gate would close on the next simulation turn. In singleplayer the turn length is 200ms, so 500ms doesn't fit terribly well into that, and chances are the gate will be attempting to close every 3 turns.

[historic_bruno]

Ah, that almost seems like a flaw in the CmpTimer logic, shouldn't it be interpolated between turns or is that not possible / too inefficient? Not that it's directly relevant to your patch.

[Deiz]

I suspect that's not possible due to the simulation architecture. I.e. the real action happens on each simulation turn and everything else is extrapolated from the last turn.

The recurring timer (SetInterval) has a provision for being 'late' which is then passed to the called function (for things that are actually time-sensitive), but I think my little hack is okay.

[historic_bruno]

Is this still being tested or not ready to commit?

[Deiz]

Ah, was waiting to see if you had any further concerns. I'm happy with it, so I'm going to commit it.

[Deiz]

In 12337:

Improve gate closing behaviour when locked or with no allies nearby. Fixes #1551.