bugfix: GLA Bomb Truck now consistently plays detonation weapon effects

[Stubbjax]

This change fixes an inconsistency with the Bomb Truck detonation effect, which does not often play for opponents / allies / observers.

This inconsistency occurs because the Bomb Truck loses its OBJECT_STATUS_DISGUISED status as soon as it begins revealing itself as a result of the RevealDistanceFromTarget property on the StealthUpdate module, however it does not lose its OBJECT_STATUS_STEALTHED status until the DisguiseRevealTransitionTime has elapsed. (This is necessary or the stealth update will not run and update the transitions.)

This means there is a brief transitional period where the Bomb Truck has a status combination of OBJECT_STATUS_STEALTHED (1), OBJECT_STATUS_DETECTED (0) and OBJECT_STATUS_DISGUISED (0), which bypasses the retail weapon effect conditions as the object is considered invisible.

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp

Lines 935 to 944 in 0a05454

	if(!sourceObj->isLocallyControlled() // if user watching is not controller and
	&& sourceObj->testStatus(OBJECT_STATUS_STEALTHED) // if unit is stealthed (like a Pathfinder)
	&& !sourceObj->testStatus(OBJECT_STATUS_DETECTED) // but not detected...
	&& !sourceObj->testStatus(OBJECT_STATUS_DISGUISED) // and not disguised...
	&& !sourceObj->isKindOf(KINDOF_MINE) // and not a mine (which always do the FX, even if hidden)...
	&& !isPlayFXWhenStealthed() // and not a weapon marked to playwhenstealthed
	)
	{
	handled = TRUE; // then let's just pretend like we did the fx by returning true
	}

There seems to be an assumption with most conditions throughout the codebase that OBJECT_STATUS_STEALTHED is always kept in alignment with OBJECT_STATUS_DISGUISED, i.e. if a KINDOF_DISGUISER object is disguised, it is also stealthed, and vice versa. This is evident when viewing references to OBJECT_STATUS_DISGUISED, where almost every usage follows an if stealthed + not detected + not disguised pattern (which returns a different result during the Bomb Truck's reveal transition). Several behaviours break during this transition, though it's only for a second so it's relatively inconspicuous.

A viable (and somewhat temporary) solution is to alter the OBJECT_STATUS_DISGUISED condition to KINDOF_DISGUISER as it then also extends the coverage to the disguise and reveal transitions on either side of the disguised state while otherwise behaving the same.

The ultimate long-term solution would likely involve decoupling the disguise behaviour from the stealth update and introducing a DisguiseUpdate module. This would simplify the logic and resolve the need to check the disguise status in tandem with the stealth status condition in almost all cases. It would also allow the stealth condition to be properly applied to KINDOF_DISGUISER units as a standalone status, such as via the GPS Scrambler. The KINDOF_DISGUISER type wouldn't be needed either.

Another safe and easy solution to fix the detonation effects is with a data change to the Bomb Truck weapons by applying PlayFXWhenStealthed = Yes.

Before

The Bomb Truck explodes during its reveal transition, resulting in no detonation effect for other players

RETAIL.mp4

After

Other players can always see the detonation effect when the Bomb Truck explodes during its reveal transition

IDEAL.mp4

Note: The fix is also applied to Generals for posterity / consistency (disguised units do not exist there).

[xezon]

Several behaviours break during this transition, though it's only for a second so it's relatively inconspicuous.

Do we need a bug report for that? Reads like there is more to do now or later.

[xezon]

This is now the forth time this condition is touched? It implies we don't get it right and it is too error prone. How about we implement a new flag in Drawable, DrawModule that can actually accurately tell whether an object is visible or not, without any special side conditions? Reason being, it should not be this finicky. There should be one state that tells "you can see this geometry in the world".

In RenderObjClass we have 3 functions that could be of interest and perhaps trickle up.

	virtual int						Is_Really_Visible(void)														{ return ((Bits & IS_REALLY_VISIBLE) == IS_REALLY_VISIBLE); }
	virtual int						Is_Not_Hidden_At_All(void)													{ return ((Bits & IS_NOT_HIDDEN_AT_ALL) == IS_NOT_HIDDEN_AT_ALL); }
	virtual int						Is_Visible(void) const														{ return (Bits & IS_VISIBLE); }

If you search code for "public DrawModule" you will find all Draw Modules. They contain Render Objects. Try to work with these.

[Stubbjax]

That is ideally the case, but that should already be the expected responsibility and purpose of Drawable::isVisible. In this particular case, it is unreliable as the Bomb Truck is destroyed, and isVisible (Is_Really_Visible) changes to false during the loop of detonation weapons.

This is also a fix that should be considered for nearly all cases where stealth is checked. I've applied it to the weapon effect condition because it's conspicuous, safe, retail-compatible, and related to recent changes / investigation.

[xezon]

If changing the old isVisible() is too risky to change for now, then we can add a new one beside it and slowly deprecate the current one.

[Stubbjax]

I'm not sure reimplementing isVisible is going to solve the unit being destroyed - and thus no longer being visible - at the time the detonation weapon is fired.

[xezon]

Hmm ok, but wouldn't that also mean that particle effects of other stealthed units are broken then, for example suiciding a GPS scrambled GLA unit?

I would like us to come up with a solution that works genericly and does not need all these special conditions. The EA conditions were already wrong. And ours are too brittle as well.

[xezon]

After investigating, I think we must not use Drawable::isVisible status in GameLogic update.

It concerns the following things:

GeneralsMD\Code\GameEngine\Source\GameLogic\Object\Object.cpp(3159):&& outerDrawable->isVisible();
GeneralsMD\Code\GameEngine\Source\GameLogic\Object\Update\AIUpdate\HackInternetAIUpdate.cpp(547):if (outerDrawable && outerDrawable->isVisible())
GeneralsMD\Code\GameEngine\Source\GameLogic\Object\Update\AutoDepositUpdate.cpp(176):if (moneyAmount > 0 && outerDrawable && outerDrawable->isVisible())
GeneralsMD\Code\GameEngine\Source\GameLogic\Object\Update\DockUpdate\SupplyCenterDockUpdate.cpp(133):if (value > 0 && outerDrawable && outerDrawable->isVisible())
GeneralsMD\Code\GameEngine\Source\GameLogic\Object\Weapon.cpp(921):const Bool isVisible = outerDrawable && outerDrawable->isVisible();

The problem is, when a new Drawable is spawned from a GameLogic update event, then the RenderObjectClass in Drawable is created with DEFAULT_BITS = COLL_TYPE_ALL | IS_NOT_HIDDEN | IS_NOT_ANIMATION_HIDDEN, which does not include IS_VISIBLE.

IS_VISIBLE will only be updated in GameClient update with RTS3DScene::Visibility_Check. GameLogic cannot rely on it before that. I suspect Fast Forwarding is another rabbit hole where RenderObject visibility will produce inconsistencies, because the GameClient update is (or will) be skipped in favor of more GameLogic updates.

Can you try to find ways to make it work without render object information, like EA tried to do it originally with getObject()->testStatus? Otherwise, we need to think of a new strategy here.

[xezon]

Another reason to not use Render Object visibility is:

If Object is not inside View Frustum, then Particle Effect is not spawned. But maybe Player will scroll Object into View after Particle Spawn event, and Particle Effect could have been visible now, but since it was never spawned, we cannot see it then.

[Stubbjax]

Do we need a bug report for that? Reads like there is more to do now or later.

Probably. I expect it'll be a long-term endeavour if it is to involve a rewrite of the disguise logic. I'll write up a list of broken or unexpected behaviour when I have time.