Add constraint between two static bodies which are later changed to dynamic does not work

[viblo]

A user of Pymunk reported that when he added a constraint between two static bodies, did a couple of simulation steps and finally changed the bodies to dynamic the bodies got NaN position. I did a little bit research and it seems like its because the constraints will try to move the static bodies with infinite force, which is then cached and not cleared when the bodies become dynamic. Issue is tracked here on Pymunk issue tracker as well: viblo/pymunk#237

This code illustrates the issue (sorry for python, but I think the main issue is clear)

>>> import pymunk
>>> s = pymunk.Space()
>>> b1 = pymunk.Body(body_type=pymunk.Body.STATIC)
>>> b2 = pymunk.Body(body_type=pymunk.Body.STATIC)
>>> c = pymunk.PinJoint(b1, b2, (0, 0), (0, 0))
>>> s.add(b1,b2,c)
>>> s.step(1)
>>> b1.position
Vec2d(0.0, 0.0)
>>> c.impulse
inf
>>> b1.body_type = pymunk.Body.DYNAMIC
>>> b1.mass = 1
>>> b1.moment = 10
>>> s.step(1)
>>> b1.position
Vec2d(nan, nan)

One way to fix it would be to reset any constraints that have a body that changes type. Looks fairly easy to do, I might do it myself.

[viblo]

Some more research and I see that the bodies have their velocities updated by the constraint as well.. So it should be reset. On the other hand, it is valid to give a kinematic body a velocity, so maybe its not good to reset it. I could see a case when a kinematic body becomes dynamic, and in those cases it feels like the proper thing to do is to keep any velocity values..

Now I wonder if the correct fix would be to only apply the constraint impulse to dynamic bodies. That would fix all issues I think. The downside I see with this is that its an additional check in the 'hot path' of the constraints...

[slembcke]

Hrm. So it was certainly never intended that static and/or kinematic bodies would be constrained together. They already represent edge cases of objects that cannot be moved, so adding constraints that try to move them doesn't make a lot of sense. As you say, detecting such a condition adds a (probably not statistically significant?) conditional to the hot-path. If this was a more common issue I might be more amenable to making the change, but this is the first time I've heard of someone wanting that use case in 15+ years.

[viblo]

I think he did it because just because it was natural and he didnt know it would break. His code had two "balls" attached together with a Pinjoint drawn on screen with debug draw. Then when user clicked, the balls became dynamic.

For this kind of quick physics simulations I think it can make sense to be able to "arrange" the scene first, then activate (make dynamic) part of it. If nothing else its quite helpful to be able to see what it looks like before its actually simulated as dynamic. If its not supported you would have to duplicate the constraint drawing code separately (or maybe some other workaround)

I should add that this was a beginner both in coding and using Pymunk, took some time before I figured out his problem.. While I think the use case is reasonable, I can agree that its also reasonable to not allow this.. its easy to explain that constraints will make a mess when they interact with bodies with infinite mass etc.

[slembcke]

Hrm. I'll think about this a bit. It is 2023 after all... Computers are plenty fast to handle a few runtime sanity checks, especially when the code has other, larger performance gaps anyway...

[viblo]

Im not sure if you checked the code I added. If not: What I did was reset the body v and w, and jAcc on the constraints if body changes from non-dynamic to dynamic, and its attached to another non-dynamic body with constraint. I do not think this has any noticable effect on performance since its only run when body type is changed. Of course it still wastes some resources while everything is static, but I think those that have code where this waste is relevant also are not using debug draw and should be able to optimize this by not adding the constraint between non-dynamic bodies in the first place.

The other option in my mind would be to just forbid this case with an assert. That would at least make it clear that its not supported with static-static constraints. I think this is also a reasonable solution. This could be checked when the constraint is created (and to make it tight also when body type is changed)

So far I havent actually included this whole fix in a released version, so the assert option is totally valid. (I want to do something to help the user, but if thats this fix or a assert that prevents it Im not sure. Only that NaN values are difficult to troubleshoot, especially since they often show up by breaking drawing but not actually at the place that created them in the first place. )