Skip to content

bugfix: GLA Bomb Truck now consistently plays detonation weapon effects #1919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Stubbjax wants to merge 1 commit into
base: main
Choose a base branch
from
Open

bugfix: GLA Bomb Truck now consistently plays detonation weapon effects #1919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,7 @@ UnsignedInt WeaponTemplate::fireWeaponTemplate

if (!isVisible // if user watching cannot see us
&& sourceObj->testStatus(OBJECT_STATUS_STEALTHED) // if unit is stealthed (like a Pathfinder)
&& !sourceObj->isKindOf(KINDOF_DISGUISER) // and not a disguiser (which should show FX while stealthed)...
Copy link

@xezon xezon Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Stubbjax Stubbjax Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@xezon xezon Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Stubbjax Stubbjax Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@xezon xezon Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Stubbjax Stubbjax Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GameLogic cannot rely on it before that.

I appreciate the investigation. Of course, the game logic should not rely on anything to do with drawables/rendering. But in this case, all respective instances have no effect on game logic and are only used for client-side animations and effects. The original logic checks isLocallyControlled, to which the same rules apply.

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.

This is a good point that I had not considered. Perhaps a Bomb Truck might explode just off-screen, or the user pauses a replay and scrolls around the map. We really need a way to determine if the object (effect) should be visible to the user, rather than whether it is currently visible.

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.

Sure. The condition looks like this if we avoid drawable-related logic altogether:

Bool Object::canPlayFX() const
{
	if (isLocallyViewed())
		return true;

	if (TheControlBar->isObserverControlBarOn())
	{
		const Player* observedPlayer = TheControlBar->getObserverLookAtPlayer();
		if (!observedPlayer || !observedPlayer->isPlayerActive())
			return true;
	}

	const Object* obj = getOuterObject();

	if (obj->isKindOf(KINDOF_DISGUISER))
		return true;

	if (obj->testStatus(OBJECT_STATUS_STEALTHED) && !obj->testStatus(OBJECT_STATUS_DETECTED))
	{
		const Player* player = rts::getObservedOrLocalPlayer();
		const Relationship relationship = player->getRelationship(getTeam());

		if (relationship != ALLIES)
			return false;
	}

	return true;
}

Copy link

@xezon xezon Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a lot :o

	if (TheControlBar->isObserverControlBarOn())
	{
		const Player* observedPlayer = TheControlBar->getObserverLookAtPlayer();
		if (!observedPlayer || !observedPlayer->isPlayerActive())
			return true;
	}

would that by synonymous for rts::localPlayerIsObserving()?

Copy link
Author

@Stubbjax Stubbjax Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of TheControlBar->isObserverControlBarOn()? I originally tried rts::localPlayerIsObserving(), but it doesn't currently work (it should check !isPlayerActive rather than isPlayerObserver).

Copy link

@xezon xezon Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ok, maybe localPlayerIsObserving needs fixing then.

Copy link

@xezon xezon Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest naming the function Object::isLogicallyVisible, because it does not necessarily need to be used for FX related logic only.

&& !sourceObj->isKindOf(KINDOF_MINE) // and not a mine (which always do the FX, even if hidden)...
&& !isPlayFXWhenStealthed() // and not a weapon marked to playwhenstealthed
)
Expand Down
1 change: 1 addition & 0 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,7 @@ UnsignedInt WeaponTemplate::fireWeaponTemplate

if (!isVisible // if user watching cannot see us
&& sourceObj->testStatus(OBJECT_STATUS_STEALTHED) // if unit is stealthed (like a Pathfinder)
&& !sourceObj->isKindOf(KINDOF_DISGUISER) // and not a disguiser (which should show FX while stealthed)...
&& !sourceObj->isKindOf(KINDOF_MINE) // and not a mine (which always do the FX, even if hidden)...
&& !isPlayFXWhenStealthed() // and not a weapon marked to playwhenstealthed
)
Expand Down
Loading