bugfix: Propaganda effects no longer persist for contained units with direct propaganda tower behaviour modules

[Stubbjax]

This change fixes an issue where propaganda effects persist for contained units that have a direct PropagandaTowerBehavior module (e.g. Attack Outpost, Assault Troop Crawler).

The contained tower logic was only ever anticipated to be used for the Overlord's tower attachment, which is technically considered as a 'contained' passenger by the Overlord - hence the need for the two container checks. However, as the PropagandaTowerBehavior module can be added directly to units themselves, the container check does not function correctly for singular containers.

In the 1.04 example below, an Assault Troop Crawler enters a tunnel and its propaganda effect persists for units around the tunnel:

[xezon]

This change correctly identifies the cause of the issue, but the fix is not properly implemented.

The KindOf inside the Contain module is configurable in INI:

  Behavior = HelixContain ModuleTag_09
    Slots                     = 1
    DamagePercentToUnits      = 100%
    AllowInsideKindOf         = PORTABLE_STRUCTURE
    PassengersAllowedToFire   = Yes
  End

Therefore, hardcoding this KindOf is not right.

The correct way to test is in the contain module via ContainModuleInterface::isSpecialOverlordStyleContainer. An example of how this is used can be seen in TransportContain::isPassengerAllowedToFire.

Therefore, I have created new change #1609.

[Stubbjax]

Oh right, isSpecialOverlordStyleContainer is what I was looking for, though it hardly feels any more generic.

[Stubbjax]

I believe I have figured out a more comprehensive solution that covers all cases.

PROP.mp4

[xezon]

I suggest

// TheSuperHackers @bugfix If our container or any parent containers are enclosing, we turn the heck off.

and move comment below #else ?

[xezon]

How about we simplify this code here by creating a Object::getEnclosingContain()? which only returns ContainModuleInterface* if it is enclosing?

[xezon]

I see you moved the whole loop into new function. What I meant was:

ContainModuleInterface* Object::getEnclosingContain(Object* containee)
{
  ContainModuleInterface* containModule = getContain();
  if (containModule && containModule->isEnclosingContainerFor(containee))
  {
    return containModule;
  }
  return NULL;
}

This way it is more in line with what getContain is doing.

And then can loop like so:

Object* Object::getFirstEnclosingContainedBy()
{
  for (Object* child = this, Object *container = getContainedBy(); container; child = container, container = container->getContainedBy())
  {
    if (container->getEnclosingContain(child))
      return container;
  }
  return NULL;
}

Basically keep
getEnclosingContain in line with getContain
getFirstEnclosingContainedBy in line with getContainedBy

I did not test it. Just draft.

[Stubbjax]

Is the current approach unsatisfactory?

Basically keep
getEnclosingContain in line with getContain
getFirstEnclosingContainedBy in line with getContainedBy

Functionally, getEnclosingContain is more in line with getContainedBy as both functions look up the containment chain for an external container object, whereas getContain is an instance accessor akin to getAI, getPhysics, getStealth, etc.

[xezon]

It is a bit confusing because the original getContain returns the direct contain module of the object, whereas your proposed getEnclosingContain returns Null or the first parent contain module that is enclosing. Perhaps name this differently to more clearly conceptually distinguish from getContain?

Other than that, my proposed getFirstEnclosingContainedBy allows to get hands on the Object first, which could be useful for future uses, because ContainModuleInterface currently does not expose the owning object. There simply is much more we can do with an Object than just ContainModuleInterface, which makes this function much more versatile.

[xezon]

I think I now understand your thinking behind getEnclosingContain, from natural language we can assume that it is a parent contain. My monkey brain thinks more of a "get the enclosing contain module that is owned by this object".

[xezon]

I think my function would be clear with this name:
ContainModuleInterface* Object::getContainIfEnclosing(Object* containee)

[Stubbjax]

It is a bit confusing because the original getContain returns the direct contain module of the object, whereas your proposed getEnclosingContain returns Null or the first parent contain module that is enclosing. Perhaps name this differently to more clearly conceptually distinguish from getContain?

It was meant to be comparable to getContainedBy rather than getContain, but I see how the naming can be misleading.

How about changing the return type to Object* and the name to getEnclosingContainedBy to better align with getContainedBy?

- ContainModuleInterface* getEnclosingContain();
+ Object* getEnclosingContainedBy();

[xezon]

Yes. I suggest implement it roughly the way I suggested above. Can optimize the names where applicable.