Fix multiple issues with test_body_motion() and test_body_ray_separation(). #35945
Conversation
|
No, I don't think this is a good idea, you may go into corners that apply opposing forces (like this _), so you need the recovery to be iterative to always work. The jitter is probably due to something else, I would suggest bisecting. |
|
The jittering is caused by that, but I agree that's not safe remove it. The best solution would be change how it works entirely. |
|
In the past I worked on another approach to solve this issue, if you want I can make a proposal @reduz. |
|
Before I address @reduz's concern, I think I need to clarify a few things. This PR resolves the issue of the jitter being noticeable when a The jitter reported in #34596, #35713 and #35780 is noticeable, because the oscillations are greater than 0.01. After this PR the remaining jitter is not noticeable, because the oscillations are less than 0.0001. The jitter (> 0.01) first became noticeable in Godot Physics with 3.1-stable after f2e5405, which "moved The jitter and the other related issues occur, because the calculation of the safe movement is failing, and the body is then moved into an unsafe position. In Godot Physics, the calculation is failing, because it is not correctly incorporating the collision margin. In Bullet Physics, the calculation is failing, because it's not detecting collisions. Note: when Bullet Physics does detect a collision, it doesn't record the collision details, which causes #26550. A complete solution requires safe movement to be calculated correctly. Unfortunately, I've not yet been able to find a successful solution to this problem. Moving the body into an unsafe position is not a major problem, because the body is quickly moved into a safe position through the recovery step. In Godot Physics, the recovery step is performed at the start of the next call to move_and_collide(). In Bullet Physics, the recovery step is called at the end of every call to move_and_collide(). However, the calculation of the recovery step is also flawed. Currently, the recovery step sums the penetration vectors (distance x normal) from each collision shape the body is colliding with. This works when collision shapes are orthogonal e.g. a corner, but not with collision shapes that have normals with components in the same direction e.g. aligned boxes. The extreme case being a This PR does two things:
The first change resolves #34596, #35713 and #35780, by using the maximum distance of each component, similar to creating an inverted With regards @reduz's concern. There is a possibility that the recovery step can push the body into another body that it's not currently colliding with. This problem exists whether a single step or iterative partial steps are used. This is the reason, the current implementation needs to limit the number of partial iterations to four. Since the recovery step is exactly what is needed to move the body into a safe place, if it cannot be moved into a safe place, it is, by definition, stuck. In the situation where the other body's collision surface is not parallel e.g. an inverted wedge, a second step could nudge the body away from the tight corner. However, in most circumstances, the In summary, until a solution is found for correctly calculating the safe movement, this patch ensures that the recovery motion is not excessive, which resolves the serious issues of #34596, #35713 and #35780 that will stop users migrating to 3.2, and it works in corners (rincones) too. |
|
@madmiraal I will check the code again soon when I have a bit of time, but I have the feeling you are not interpreting what is going on correctly. Even if you have recovery in the same direction, recovery happens only as a fraction of the penetration depth, so it never goes beyond that. This is exactly how all discrete physics engines work. The jitter is most likely just having to adjust the minimum penetration dept in Godot physics, but I am not sure about what is going on in Bullet, I would need to check it again. |
|
@reduz Just to clarify my understanding of the purpose of the first step in I believe this is based on two assumptions:
The first assumption is not valid, because this function is currently needed every time a previous call to The second assumption is not valid when the body is colliding with multiple aligned surfaces e.g. a |
|
Noting that #18433 (mentioned in a previous comment here) is technically not about jitter but its particular behavior will still probably change as a result of this patch. The problem in #18433 applies even when colliding against a single planar surface and manifests in different ways depending on exactly how |
|
Any update on this? For now I have compiled Godot using @madmiraal branch |
|
Rebased following NULL to nullptr change. |
|
I tested this and it has no effect at all on the weird collisions of kinematicbody2d with slopes (#20593) |
madmiraal
changed the title
|
The underlying problem in #18433 is not linked to jitter and can't be truly fixed without entirely removing overlap rectification from the code paths used by move_and_collide (i.e. breaking backwards compatibility or adding more optional arguments). |
|
Added a fix for the sticking identified by @MBZSoftProds. |
Actually, all this is done on purpose, and the reason it works this way is due to numerical precision. Assuming stuck by checking margin then move safely without margin. If it did not work this way, your body would approach the collision point infinitely and start causing floating point precision problems. Again, all physics engines work like this to avoid running into this problem. I will give an overhaul to the physics engine when I am done with rendering and will eventually check issues there, so from my side I will not do any work or changes on physics until I can spend time on it in some months.
Shapes can be any amount in any direction, and you have to always properly free from them as a first step using incremental biases. This is unavoidable. This algorithm is expensive, and considerably more expensive than just using a rigid body because its proper CCD. |
|
Setting aside the enticing discussion on how a physics engine should work and why collision margins are necessary, I think it’s important to focus on what is causing the issues and how this PR addresses them. Also, even if the approach in this PR is flawed, it should at least be considered as a workaround until the the physics engine is overhauled, because the issues it addresses are cited as one of the main reasons people are not moving to 3.2. The 3.2 version of this PR is #37498. To be specific, the issues addressed are caused by the current implementation of In the current implementation
In Bullet physics these are:
In the current implementation of both Godot and Bullet physics, the move to a safe position with margin is calculated by summing the vectors of the shortest penetration distance between each collision shape. When using 4 incremental steps this distance is multiplied by 0.4 and repeated 4 times. The jitter described in the issues is caused by the move to a safe position being greater than the attempted motion. To simplify things, I’m only going to cover The current Godot implementation works for a body (pink) colliding with a single surface (blue) as follows:
Since the distance between the body and the surface in (a) and (g) is roughly the same, no jitter is seen. However, when the body collides with four parallel surfaces, for example on a
This PR proposes modifying the calculation of the recovery motion by using the maximum penetration distance of each surface instead of summing them together, and only performing a single iteration instead of four. This results in the following when the body collides with a single surface:
Since the distance between the body and the surface in (a) and (d) is roughly the same, no jitter is seen. When the body collides with four parallel surfaces, for example on a
The result is exactly the same as for a single surface, hence no jitter is seen. For Bullet physics the situation is slightly different, when the body is colliding with a single surface it works as follows:
Again, since the distance between the body and the surface in (a) and (d) is roughly the same, no jitter is seen. However, when the body collides with four parallel surfaces, for example on a
There is a lot of jitter, because the distance between the body and the surfaces at the end of each from: (a), (d), (e), (f) and (g) vary a lot. This PR proposes modifying the calculation of the recovery motion and the motion in Bullet physics. The calculation for the recovery motion is the same as the proposed calculation for Godot physics. The calculation of the motion is different, because the tolerance is not used and both the safe and unsafe motion distances are calculated. Note, in Bullet physics the collision margin is included in the
Since the distance between the body and the surface in (a) and (c) is roughly the same, no jitter is seen. When the body collides with four parallel surfaces, for example on a
The result is exactly the same as for a single surface, hence no jitter is seen. If required I can do a similar expalantion for multiple surfaces in differing directions, but hopefully this is sufficient. |
|
Rebased following merge of #37314. |
|
I have tested and analysed the circle/sphere shape in a wedge and I have not found a problem with the approach used in this PR. In fact, I have found that the approach used in this PR actually improves the situation. First, for the benefit of others reading this, I want to clarify the situation presented. Since, we're assuming that "things either fit or not" the circle shape would not overlap the shapes creating the wedge. Therefore, for a 30° wedge, the worst case scenario would look like this: Despite using different methods to calculate the green arrow, the direction of the resultant movement (the green arrow) is the same for both the current approach and the approach used in this PR. The difference is the length of the green arrow and the number of times the green arrow is calculated and the With the current default
This shows that this PR does a better job i.e. moves it further – in one step – of "freeing" the |
Extracted from godotengine#35945 Using min/max from different axes instead of adding recovery vectors together in order to avoid cases where the recovery is too high when colliding with several shapes at once along the same direction. Co-authored-by: Marcel Admiraal <madmiraal@users.noreply.github.com>
Extracted from godotengine#35945 Using min/max from different axes instead of adding recovery vectors together in order to avoid cases where the recovery is too high when colliding with several shapes at once along the same direction. Co-authored-by: Marcel Admiraal <madmiraal@users.noreply.github.com>
|
@madmiraal al Yeah I guess in this case yours will give a similar result because it's vertical, and because the original approach is not actually moving the object, but the recovery vector, so in this very specific case it's not fundamentally that different. That said, I still think it's not a great idea. Again, as I mentioned in my previous post, the way I see things are:
Again, the way I see it, I really believe all this is time mis-spent because to me the problem is solved already. I am open to looking for ways to improving the approximation to the "correct" solver, but not to cut corners like this. |
|
I do not know if any such tests exist or not, but if not, perhaps it would be worth exploring creating some tests to verify results of depenetration methods? The results could be used to confirm whether or not @madmiraal 's approach could work, and also be useful for identifying regressions for any later changes to physics, which could be important in the near future given the significant amount of work related to physics in Godot that is planned. |
|
@mindinsomnia The problem is that his approach cuts too many corners, as well as being anisotropic (diagonals wont work as well), when you do these kind of things, in my experience, you are not really certain what will fail (because all examples you can think of will work) until it fails (in a situation you did not think about). It's a sort of experience about not trusting certain types of solutions to problems that you get you grow up doing these things over and over again. In any case, I think the problem @madmiraal is trying to solve with this PR can be solved more correctly (in a non-anisotropic way and closer to how an actual full solver would work) in a different way with a minimal modification to the current code. Will hope to try this out in the coming days. |
|
Also, I had to think a bit more, but here is a better example that will not work with this PR. Because the recovery is anisotropic, this will produce no recovery at all (which should be towards the upper right diagonal), and most likely cause the sphere to fall through the bottom. I am pretty sure that there are a lot more cases using diagonals that will break this PR because of its assumed orthogonality. |
|
Sooo... Will this be PR closed, or will it stay open until all the good bits are taken out? |
|
This is tangential, but I will note that there are a lot of situations in which it is preferable to depenetrate in a way that prefers some directions over others. For example, if you have a shooter character walking up and down shallow slopes, it makes a lot more sense to try to depenetrate them vertically before trying to depenetrate them along the collision normal. Additionally, getting accurate collision normal data between two overlapping objects is hard, especially if one of them is a mesh or an AABB. So sometimes, a depenetration algorithm that looks like it prefers some directions over others will actually give more accurate results than one that tries to prefer directions derived from collision normals. (I happen to know that bullet in particular can be fairly poor at giving direction data for contact pairs; no idea how godot physics works when it comes to this.) Considering both of these two things together, it makes sense to think that, at the very least, the presence of anisotropic depenetration code is not necessarily a mark against a depenetration system, as long as there is also isotropic depenetration code. |
|
@wareya That is not really the problem that this PR is attempting to solve. (TBH the real problem here is that this should work for users in Godot Physics right now and nobody ported the same fix to Bullet). @Riteo I want to experiment a bit more with this in the coming days, but I need to finish my upcoming PR. |
|
Here are some extra thoughts on this. You guys got me involved, so now bear with me :) So, here are my two proposed solutions, one for a more robust one-step solver (non-iterative) and another for a more robust iterative one. For a one-step solver, this is the best I can come up with. I tried this approach in the new particle collider and it seems to work very well. Its kind of similar to what this PR attempts to do, but does not have the anisotropy limitation. Vector2 recover_motion;
for (int i = 0; i < cbk.amount; i++) {
Vector2 a = sr[i * 2 + 0]; //own point
Vector2 b = sr[i * 2 + 1]; //foreign point
Vector2 n = b-n;
real_t l = n.length(); //collision length
n.normalize(); //collision normal
l -=MAX(0, n.dot(recover_motion)); //maximum amount you can move in this direction, without accumulating
if (l > 0.0) {
recover_motion += n * l;
}
}What this does Is just push away the contact, but ensure it never pushes again more than the maximum depth in a given direction. In my view, this is what, in my mind, the correct version of this PR would be (in practice does more or less the same, likely not as good with collision on all sides, could be worked around, but does not fail on the test case I posted). I am still not sure if I prefer this over an iterative solution, given the iterative solution is a lot more robust. So I tried to come up with a more correct iterative solution and this is what I have so far. I think it would be worth testing as it requires minimal change to what there is now: Vector2 recover_motion;
for (int i = 0; i < cbk.amount; i++) {
Vector2 a = sr[i * 2 + 0]; //own point
Vector2 b = sr[i * 2 + 1]; //foreign point
//compute plane on b towards a
Vector2 n = (a-b).normalized(); //normal
float d = n.dot(b); //distance
//compute depth on recovered motion
real_t depth = n.dot(a + recover_motion) - d;
if (depth > 0.0) { //only recover if there is penetration
recover_motion -= n * depth * 0.4;
}
}The above is more close to how a correct iterative solver would work, as it considers motion (something the the current one does not) and I think it should also solve the jittering problem. Of course I may be wrong so all this need to be tested, so If you guys want to give a check to this or discuss these ideas, let me know. |
|
Rebased following merge of #45564. |
…ion(). The test_body_motion() and test_body_ray_separation() functions perform an initial recovery step to attempt to move the body to a safe place before testing whether the body can safely move. Currently this is done in four partial steps, and during each step the detected penetrations are reversed by summing them together. This patch changes this approach to a single step and uses the maximum penetration instead of the sum. The same approach is applied to Godot Physics 2D, 3D and Bullet Physics, and to both CollisionShapes and RayShapes. Also, fixes Bullet physics not detecting collisions and relying on the recovery step. Furthermore, ensures that Bullet physics only moves a safe amount by using a binary search as done in Godot physics.
|
Rebased following merge of #43952. |
akien-mga
changed the title
|
I am closing this pull request for the following reasons:
I think this is a very interesting and clever idea, and it is entirely possible that in practice it solves all possible cases that may arise, besides being far more efficient. The reason it was not adopted is that, given how unconventional it is, It is not possible for us to establish with 100% confidence that it will not have future side effects or fail in very specific situations. Given this is such a critical and often used piece of the engine, a more conservative approach using an iterative solver was preferred. I am still thankful and appreciative of all the effort that went into this, which contributed to making Godot a much better piece of free and open source technology for everyone. |
Fixes most of the jitter seen when using either
move_and_collide(),move_and_slide()ormove_and_slide_with_snap()and colliding with multiple surfaces. This is especially noticeable when twoBoxShapeCollisionShapes collide, for example a box shaped character on aGridMap.Fixes #33833.
Fixes #34081.
Fixes #34098.
Fixes #34242.
Fixes #34436.
Fixes #34596.
Fixes #35713.
Fixes #35780.
Fixes #42175.
Update:
Now fixes remaining jitter, sliding in move_and_collide() and inaccurate collision information in Bullet physics:
Fixes #17893
Fixes #18433
Fixes #26550
Fixes #28717
Fixes #32182
Godot Physics v 3.1
Godot Physics v 3.2
Godot Physics v3.2 with #35945
Bullet Physics v 3.1
Bullet Physics v 3.2
Bullet Physics v3.2 with #35945