garrysmod-issues icon indicating copy to clipboard operation
garrysmod-issues copied to clipboard

The engine incorrectly assumes IPhysicsObject::GetGameData is a valid CBaseEntity

Open Kefta opened this issue 4 years ago • 0 comments

The gamedata can be any void* including NULL. There is nothing in the vphysics interface nor internal implementation that prevents this, nor does vphysics even know about entities/CBaseEntity at all. Fixing this will allow for physobjs to exist independently of entities so that we may write our own entity systems in Lua while still utilising vphysics. In this case, the gamedata would just be NULL, and physobj/Lua entity relationships would be stored in a hashtable. Here are all the places in the engine where this needs to be fixed (I found no additional blunders in the 2018 branches, but GMod may have some additional ones in its custom code):

Shared

physics_shared.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/shared/physics_shared.cpp#L740 ->
void *pGameData0 = pObject0->GetGameData();

if (pGameData0 == NULL)
	return;

void *pGameData1 = pObject1->GetGameData();

if (pGameData1 == NULL)
	return;

g_EntityCollisionHash->RemoveObjectPair( pGameData0, pGameData1 );
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/shared/physics_shared.cpp#L750 - same fix as above
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/shared/physics_shared.cpp#L877 -> if ( pGameData == NULL || pOther->GetGameData() != pGameData )

props_shared.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/shared/props_shared.cpp#L1203 -> if ( pOwnerEntity != NULL && pOwnerEntity->IsEffectActive( EF_NOSHADOW ) )
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/shared/props_shared.cpp#L1603 - same fix as above

Server

physics.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L706 -> else if (pOther != NULL) - it would actually be preferable to create an alternate physics solver entity that instead functions with an anonymous physobj and entity instead of two entities, but that would require a lot of extra code and refactoring
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L726 (_DEBUG only) - check for NULL
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L760 (this is not actually used anywhere but is an external utility) -> return true;
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L2429 (at the top of the method) ->
if (pEntity == NULL)
	return 0;
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L2852-L2854 ->
CBaseEntity *pEntity0 = static_cast<CBaseEntity *>(pOther->GetGameData());

if (pEntity0 != NULL)
{
	CFmtStr str("%s (%s): %s [%0.2f]", pEntity0->GetClassname(), STRING(pEntity0->GetModelName()), pEntity0->GetDebugName(), pSnapshot->GetFrictionCoefficient() );
	NDebugOverlay::Text( pt, str.Access(), false, 0 );
}

physconstraint.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physconstraint.cpp#L1900 ->
if (pChildEntity == NULL)
	return;

trigger_portal.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/src_main/game/server/trigger_portal.cpp#L309 (at the top of the method) ->
if (pEntity == NULL)
	return false;

trains.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/trains.cpp#L1556 -> if ( pOtherEntity != NULL && pOtherEntity->GetMoveType() == MOVETYPE_VPHYSICS )

baseentity.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/baseentity.cpp#L2015 ->
if (pOtherEntity != NULL)
{
	for ( int i = 0; i < list.pushedCount; i++ )
	{
		if ( pOtherEntity == list.pushedEnts[i] )
		{
			inList = true;
			break;
		}
	}
}

episodic/vehicle_jeep_episodic.cpp

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/episodic/vehicle_jeep_episodic.cpp#L1180 - remove assertion

A note about CalculateObjectStress in physics_impact_damage.cpp:

This method assumes that NULL physobjs originate from the world, thus the force they exert on the target object contributes to static forces. Not only is this an incorrect assumption, but the entire method is pretty poorly and inefficiently programmed, and also returns very inaccurate results. I am working on rewriting it but I first need to test how it affects the few use cases it has (airboats and weight buttons). As it doesn't cause any crashes, however, it's outside the scope of this PR.


Additionally, there are quite a few places where reinterpret_cast/C-styled casts are incorrectly used where static_cast should be instead:

  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L1256
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L1561
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L1702
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L1815
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L1849
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physics.cpp#L1925
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/client/physics.cpp#L313
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/client/physics.cpp#L456
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/client/physics.cpp#L670
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physconstraint.cpp#L1708
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physobj.cpp#L278
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/physobj.cpp#L284
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/src_main/game/server/trigger_portal.cpp#L332
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/server/hl2/hl2_player.cpp#L1343
  • https://github.com/VSES/SourceEngine2007/blob/43a5c90a5ada1e69ca044595383be67f40b33c61/se2007/game/shared/props_shared.cpp#L946 - should be pOwnerEntity->GetBaseAnimating() instead of a dynamic_cast

Kefta avatar Jul 24 '20 17:07 Kefta