Rejecting collisions from begin() callback considered a bug?

[slembcke]

I got an email from somebody this morning that made me think a bit about how rejecting collisions from the begin callback works.

As it is currently, if you reject a collision from begin(), preSolve() and postSolve() will not be called in the same step. However, the next step, begin() will not be called again and Chipmunk will go and call preSolve() and postSolve(). The default version of preSolve() simply accepts all collisions. This means that rejecting a collision from begin() only rejects the collision for the first step, and if you wanted to permanently reject a collision, you would have to do so by storing the value somewhere else and return false from ever call to preSolve(). Pretty much makes the return value of the begin() callback useless...

Having thought about it a little more now, it makes immensely more sense to make the rejection by begin() be permanent. Rejecting a collision from begin() means that preSolve() and postSolve() will never get called for as long as the collision collision and separate() would still be called when the shapes stop touching. Rejecting a collision from preSolve() should continue to be done for an individual frame, and a new function should be introduced to allow you to permanently reject a collision like begin() would.

Does this make sense, and should the current behavior be considered a bug needing to be fixed? Otherwise I will just add the function to permanently reject a collision and let people call that from begin() if that is what they want to happen.

[viblo]

I think the best would be to make begin reject permanently.. An extra function to call feels like it just complicate things.

[majicDave]

I would probably consider that a bug, though as long as the behavior is clearly documented I wouldn't mind either way.

I would also assume that a rejected collision would not call separate either, though again, I can see some cases where getting the separate callback of a rejected collision may be useful. So though my assumption would be that rejecting the collision in begin would cause no further callbacks to be made at all for that collision, it might still be better to have at least some way to still get the separate callback.

I definitely like the extra information in the arbiter, and that patch alone solves all of my problems with it. Overall I think the default behavior should be for pre/post solve to not be called if rejected in begin, but that separate should still be called, but documented as doing so, and with that extra info in the arbiter.

I think I just repeated myself about 3 times, sorry... was thinking aloud!

I'll give the patch a try, Thanks!

[majicDave]

Hmm after applying the patch I no longer get any collisions at all. Will investigate. Update - It looks like the patch is a bit dodgy with what it does around line 580 in cpSpace.c. Don't know enough about what it's trying to do to be sure. Will revert the patch for now I think.

[slembcke]

Ugh. No collisions at all. I probably did something stupid when creating the patch... I hand reverted one of the changes in the patch. When I reapplied it it worked though... I swear...

edit: Yep. ID10T error for sure. I should have just deleted the file and remade it. I re-uploaded the file above with a working version.

[dieterweb]

+1 for adding this to the trunk

[slembcke]

Well, that is 3 votes for and none against. I'll wait until the end of the day, but it seems like people think this is a good change.

[slembcke]

It certainly makes one way platforms easier.

Code: Select all

static int
preSolve(cpArbiter *arb, cpSpace *space, void *ignore)
{
	CP_ARBITER_GET_SHAPES(arb, a, b);
	OneWayPlatform *platform = (OneWayPlatform *)a->data;
		
	if(cpvdot(cpArbiterGetNormal(arb, 0), platform->n) < 0){
		cpArbiterIgnore(arb);
		return 0; // still need an explicit return from preSolve()
	}
	
	return 1;
}

No need to keep my own array of collisions to ignore and no need to have a separate() callback to remove them when they expire.