Add support for collision handlers

[FinnG]

I'm a big fan of chipmunkpp, but as far as I can see, there is no support for collision handlers.

I think this would be a very useful feature - is there any plan to add this?

[jhasse]

Hi! I've always wanted to add them, but in the end I ended up using cpSpaceAddCollisionHandler directly in my own game.

But this would definitely be a good idea, I will think of a solution in the next day and see if I can implement this.

[jhasse]

Okay here's a first (untested) draft. The problem is that all 4 functions need to be passed to addCollisionHandler. Any ideas how to solve this?

[FinnG]

Thanks for this. Ive not had much time to play with it yet though. I'll hopefully get a chance in the next day or so.

As far as setting all four callbacks at once goes, isn't it possible to set a std::function equal to std::nullptr? If so a simple check in the callback lambda could be used to cause the callback to return immediately - this would mimic the normal behaviour of the C library.

On Sunday, 29 December 2013, Jan Niklas Hasse wrote:

Okay here's a first (untested) draft. The problem is that all 4 functions need to be passed to addCollisionHandler. Any ideas how to solve this?

— Reply to this email directly or view it on GitHubhttps://github.com/jhasse/chipmunkpp/issues/2#issuecomment-31309377 .

[jhasse]

Ah didn't now that std::function can be set to nullptr. Thanks!

When I was actually testing addCollisionHandler though, I noticed that lambdas apparently can't be passed as C functions? Or maybe I did something else wrong, check out the new commit ;)

[FinnG]

I've never tried to pass a lambda as a C function, but I had a quick play yesterday evening and it seems like it's quite a lot of hassle - it's a shame because it was a nice solution!

I guess another option is to create a simple static handler function that uses the context pointer as a lookup to find the correct C++ function to call?

[jhasse]

Yes sounds like a good solution. Btw: Do you think the template is worth it? I'm thinking about just using cp::DataPointer (aka void*) as the last argument.

[FinnG]

I think the template solution you've come up with is cleaner for the end user, so I'd stick with that over a generic void*. Better still, I'd try to remove the context pointer from the callback entirely because it could prove useful to chipmunkpp to look up which std::function to call. Shouldn't extra data the user wants to pass through to the collision handler could be encapsulted in a class/struct containing the handler function.

I'm not sure if I've explained myself very well. I could put together a patch if you want?

[jhasse]

I think I know what you mean:

template<class T>
CallbackContext {
    std::function<void(Arbiter, Space&, T> callback,
    T data
};

A pointer to this struct gets then passed to the static function.

[FinnG]

Yep, pretty much, the only extra thing was that I don't see what the point of the T data; is in a C++ program. You can use std::bind to make sure your callback function calls a specific instance of a class, then encapsulate and additional data required in that class.

It seems to me that the void* data often used in C libraries is for the benefit of C programmers mostly and that C++ libraries can get away without having to have one of these - although I'm happy to be proved wrong :-)

[jhasse]

You're right! That makes things a little bit easier. The lambda problem doesn't though, but I implemented the way with static functions now. Tell me what you think :)

I've tried it in my own game and it's working fine. cp::Arbiter could need some more methods though.

[FinnG]

I gave it a go just now and it works a treat! It only took me about 2 minutes to convert my game from cpSpaceAddCollisionHandler. Fantastic! :-)

It is a shame about the lambda problem because that was quite a nice solution, but what really matters is the interface that is presented to the user, which is now very C++ like and very nice I think.

That said, there is a possible memory leak in that the callbackDatas vector will just grow in size forever and never shrink (even if you replace an old collision handler with the a new one - the old one just sits in the vector unused) I don't think this is a massive problem as I don't generally change collision handlers very often at all but I'm sure someone out there does.

[jhasse]

Yes, the memory leak needs to be fixed. Do you know if

cpSpaceAddCollisionHandler(space, a, b, somefunc, somefunc2, NULL, NULL, NULL);
cpSpaceAddCollisionHandler(space, a, b, somefunc3, NULL, NULL, NULL, NULL);

removes somefunc and somefunc2 as the collision handler? If so, then callbacksDatas could have the following type

std::map<std::pair<CollisionType, CollisionType>, std::unique_ptr<CallbackData>>

which would fix the memory leak.

[FinnG]

I just put together a quick test and every time cpSpaceAddCollisionHandler is called, provided the CollisionType parameters are the same, all the handlers are updated so it sounds like a map would be ideal in this case!

On Friday, 3 January 2014, Jan Niklas Hasse wrote:

Yes, the memory leak needs to be fixed. Do you know if

cpSpaceAddCollisionHandler(space, a, b, somefunc, somefunc2, NULL, NULL, NULL);cpSpaceAddCollisionHandler(space, a, b, somefunc3, NULL, NULL, NULL, NULL);

removes somefunc and somefunc2 as the collision handler? If so, then callbacksDatas could have the following type

std::map<std::pair<CollisionType, CollisionType>, std::unique_ptr>

which would fix the memory leak.

— Reply to this email directly or view it on GitHubhttps://github.com/jhasse/chipmunkpp/issues/2#issuecomment-31521925 .

[jhasse]

Thanks for testing! I implemented it with the map (untested, I should have added a unit test ...).