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

ShouldCollide causes physics engine to mess up when certain things are done.

Open crazycarpet opened this issue 11 years ago • 17 comments

This is just an example, I'm aware there is a setting for collision with teammates. In the example:

function GM:ShouldCollide(enta, entb) if enta:Team() ~= entb:Team() then return false end end

if I were to change my team well standing in another player there is a high chance source engine's physics will totally fail and objects will start floating around.

I am not calling return true anywhere in ShouldCollide.

crazycarpet avatar Dec 17 '13 05:12 crazycarpet

Source does not respond well to collision rules changing on an entity mid-game. I seem to remember Garry was going to add logic for custom collision groups to solve this issue. Does anyone know what happened to that?

On 17 December 2013 06:58, crazycarpet [email protected] wrote:

This is just an example, I'm aware there is a setting for collision with teammates. In the example:

function GM:ShouldCollide(enta, entb) if enta:Team() ~= entb:Team() then return false end end

if I were to change my team well standing in another player there is a high chance source engine's physics will totally fail and objects will start floating around.

I am not calling return true anywhere in ShouldCollide.

— Reply to this email directly or view it on GitHubhttps://github.com/Facepunch/garrysmod-issues/issues/642 .

Lexicality avatar Dec 17 '13 07:12 Lexicality

Probably the same thing that happened to everything else - Rust.

robotboy655 avatar Dec 17 '13 07:12 robotboy655

Lol ^ so true, you can enable custom collision on the entity, but it doesn't help. I mean it seems to happen less often now.

Garry - Please fix this!

crazycarpet avatar Dec 17 '13 17:12 crazycarpet

Could we get an update on this? I have some collision checks that simply can't be done with the current collision groups and it gets really annoying. I also have the same code running on two different maps and it only happens on one. (rp_evocity2_v2p)

I haven't been able to figure out exactly what causes it, but I cache collisions every tick so it can't be returning a different value in the same tick for the two entities.

if I were to change my team well standing in another player there is a high chance source engine's physics will totally fail and objects will start floating around.

Hmm, I will try caching the first collision state until it stops colliding to see if that'll help with anything.

xaviergmail avatar Mar 06 '15 19:03 xaviergmail

Hmm, I will try caching the first collision state until it stops colliding to see if that'll help with anything.

A year later, this issue still persists and I can't figure out for the life of me a reliable way to make it crash (oy any reproducible test scenario at all), it only happens in production at the most unfortunate times

xaviergmail avatar Jan 09 '17 23:01 xaviergmail

It's probably best to avoid it. Physical collisions aren't the only thing that call it e.g. traces (i think), so finding the problem is probably impossible.

thegrb93 avatar Jan 10 '17 00:01 thegrb93

I'm not entirely sure how to avoid it seeing as I need dynamic collision rules for certain things in my gamemode and I can't resort to collision groups :S

xaviergmail avatar Jan 10 '17 22:01 xaviergmail

@xaviergmail Have you given this thread a read? It contains some useful information that might help. https://facepunch.com/showthread.php?t=1540390

Mista-Tea avatar Jan 10 '17 23:01 Mista-Tea

Is there any error message?

mcNuggets1 avatar Oct 30 '18 14:10 mcNuggets1

Any updates here ?

Pdzly avatar May 07 '23 12:05 Pdzly

There will never be.

Kefta avatar May 07 '23 16:05 Kefta

sadge

Pdzly avatar May 08 '23 05:05 Pdzly

when

31east avatar Nov 24 '23 16:11 31east

Why not store the return value of ShoudCollide for both entities and if it changes without CollisionRulesChanged getting called it will return the old value and throw a warning, or it calls CollisionRulesChanged in the next tick to update it. It would at least stop the Physics engine from breaking

RaphaelIT7 avatar Mar 05 '24 18:03 RaphaelIT7

@RaphaelIT7 I believe it already does that, but it’s not the cause of this issue; even if you do it properly and call CollisionRulesChanged it still happens.

WilliamVenner avatar Mar 06 '24 10:03 WilliamVenner

@RaphaelIT7 I believe it already does that, but it’s not the cause of this issue; even if you do it properly and call CollisionRulesChanged it still happens.

Calling CollisionRulesChanged properly works fine for me. I have an entity that changes the collision rules with every player every 20 minutes, and it can also change randomly for one player. After I called CollisionRulesChanged for every possible change I never had this issue again. So I would think that everything works fine if you call it for all possible cases.

But to be sure, I tested a bit around with ShouldCollide, and it seems like CollisionRulesChanged really works completely fine. It should also be noted that if you don't call CollisionRulesChanged it would only break the physics engine when something collides with the Entity/a Collision happens. It doesn't seem to break randomly.

This is btw how I tested it with CollisionRulesChanged. I'm randomly changing if it should collide each frame and I call CollisionRulesChanged on all Entities that have a PhysicsObject. This works completely fine, but If I were to remove the CollisionRulesChanged call it can break Physics in a few seconds (You would need to collide with the Entity or cause a collision with a prop to break the Physics).

local ret = math.random(0, 1) == 1
hook.Add("ShouldCollide", "Test", function(ent1, ent2)
	return ret
end)

hook.Add("Think", "Test", function()
	local last = ret
	ret = math.random(0, 1) == 1
	if last != ret then
		for _, ent in ipairs(ents.GetAll()) do
			if !IsValid(ent:GetPhysicsObject()) then continue end
			ent:CollisionRulesChanged()
		end
	end
end)

concommand.Add("enable_customcollison", function(ply) -- Spawn a prop, look at it and run this command.
	local ent = ply:GetEyeTrace().Entity
	if IsValid(ent) then
		ent:SetCustomCollisionCheck(true)
	end
end)

Would you maybe have a case where CollisionRulesChanged would fail?

RaphaelIT7 avatar Mar 06 '24 16:03 RaphaelIT7

This issue is actually caused by not calling Entity:CollisionRulesChanged on the entity and Entity:CollisionRulesChanged being called. In a debug build, IVP would crash but in a release build it just calls delete this; without any warning or error. This seems to cause this undefined behavior.

I think the best solution would be to just cache the return value of ShouldCollide and return it. This way we would reduce the amount of Lua calls, fix the issue (By requiring everyone to call CollisionRulesChanged) and probably improve performance. The code is probably not the fastest, but it should work.

class CGarrysMod
{
public:
	[ ur other stuff ]
	bool ShouldCollide(CBaseEntity*, CBaseEntity*);
	void ClearCollideCache(CBaseEntity*);
};

std::unordered_map<CBaseEntity*, std::unordered_map<CBaseEntity*, bool>> cache;
bool CGarrysMod::ShouldCollide(CBaseEntity* ent1, CBaseEntity* ent2) // It seems like ent1 is always the Entity with CustomCollisions enabled.
{
	std::unordered_map<CBaseEntity*, bool> entires;
	if (cache.find(ent1) == cache.end()) {
		cache[ent1] = entires;
	} else {
		entires = cache[ent1];
	}

	if (entires.find(ent2) != entires.end()) {
		return entires[ent2];
	}

	// Do the Lua call

	bool val = true; /*LUA->GetBool(1)*/
	entires[ent2] = val;

	return val;
}

void CGarrysMod::ClearCollideCache(CBaseEntity* ent) // Call it from CBaseEntity::CollisionRulesChanged
{
	cache.erase(ent);
}

And at least a Warning should be readded to print that somehow it failed again, just to be sure (If it should happen)

Edit: Removing the delete this; also solves this bug, but it could cause other issues, but I couldn't find any.

Edit 2: Replacing the delete this; with

for (int i = 0;i<2;i++){
	objects[i]->recheck_collision_filter();
}

Seems to fix this, and the IVP_Mindist will also be deleted without it breaking the physics engine. So this should actually be the real fix. NOTE: You also need to move the if statement to the end of the function like here

RaphaelIT7 avatar Mar 08 '24 03:03 RaphaelIT7