godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.07k stars 69 forks source link

Expose Surface Normals as part of Contact Data 3D #8324

Open Gamepro5 opened 8 months ago

Gamepro5 commented 8 months ago

Describe the project you are working on

A multiplayer FPS with quake style movement that uses Godot Jolt to handle collisions.

Describe the problem or limitation you are having in your project

After several months of debugging with the generous help of @mihe and @jrouwe, I believe that my project and many other projects would benefit from being able to use surface normals. The Jolt physics engine is able to give surface normals, not just collision normals. Godot Physics is not able to do this, and as such, the Godot Jolt addon is unable to allow users to use this normal.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Too many to list here. The belief is that this problem for move_and_collide() would not be an issue if I could just use surface normals. But it would mean that character controllers made could, if the third party physics engine supports it, use surface normals instead of collision normals. Jolt uses GJK-EPA, which from what I understand struggles with reporting accurate collision normals on sharp edges. Thus, being able to access the surface normals it already provides through godot's physics server api would (presumably) be a solution to the problem. Even if it doesn't, having access to surface normals would still be beneficial for other uses, as right now the only way to get this info is by using raycasts.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I'm not qualified to say how this would work, so I talked to @rburing about it. Here's what I gathered from that:

Quote from @rburing:

there needs to be some uniform API. i think adding it to Godot Physics might be the easiest route, similar to https://github.com/godotengine/godot/pull/81610 which added some API to Godot Physics (which wasn't too much work to implement) because they wanted to expose the same in their Box2D GDExtension

This isn't a request to add surface normals to Godot Physics. It would be odd to have a property in the default KinematicCollision3D that isn't used unless a third party physics engine uses it, so I think that a more flexible api for this would be required. I am not sure though and am open to discussion.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No.

Is there a reason why this should be core and not an add-on in the asset library?

n/a

mihe commented 8 months ago

The belief is that this problem for move_and_collide() would not be an issue if I could just use surface normals.

To give a much needed summary of the issue linked there, the problem is that with collision/contact normals being what they are (and correct me if I'm wrong) meaning the best (?) direction by which to separate the two bodies/shapes, combined with the fact that things like move_and_collide will always sink into the ground a bit (through safe_margin), you expose yourself to the possibility of colliding with the edges of shapes as you're walking over those edges. This can result in a collision/contact normal that points towards-ish the direction of movement (even in Godot Physics sometimes), which is then typically (erroneously) treated as an obstacle. This article does a good job of explaining the problem, as well as some mitigation strategies.

In some cases though, you simply just want the normal of the surface at that contact point, without having to consider all the ins-and-outs of how the underlying collision detection works, like for example the shape margins used by GJK-EPA. I would even go so far as to argue that it's probably what most users think they're actually getting when they're making use of the current collision/contact normal in Godot.

This isn't a request to add surface normals to Godot Physics.

I think it should be. Whichever way you slice it you're going to have surface normals as part of the shared API, which definitely should be populated by Godot Physics as well. If nothing else, it's (as you say) a useful piece of contact information that can likely benefit many other use-cases, including move_and_slide.

Actually calculating the surface normals for all the shape types in Godot Physics should be fairly straight-forward, if it's not readily available already. It's more just about someone sitting down and actually doing it and then running the PR gauntlet with it.

Gamepro5 commented 8 months ago

I think it should be. Whichever way you slice it you're going to have surface normals as part of the shared API, which definitely should be populated by Godot Physics as well. If nothing else, it's (as you say) a useful piece of contact information that can likely benefit many other use-cases, including move_and_slide.

The reason why I thought it didn't need to be was because I thought there were plans to replace Godot Physics 3D with your addon. I may have misunderstood though. In my mind, it didn't make sense to make satisfying this request more complicated by requiring more work on Godot Physics if it plans to be deprecated soon anyway. I don't know if the deprecation is a 4.x thing or a 5.0+ idea though. If it does plan to be a 5.0+ deprecation, I hope there is another way to satisfy this proposal before then.

Also, thanks for providing more context, and thanks for that article!

rburing commented 8 months ago

This isn't a request to add surface normals to Godot Physics.

I think it should be. Whichever way you slice it you're going to have surface normals as part of the shared API, which definitely should be populated by Godot Physics as well.

I agree, as expressed in the quote in the OP.

In Godot Physics I think it could be done by calculating the surface normal in the get_supports function for each shape (usually it's already available in some form, so it's more a matter of filling in than calculating), and then passing that info along to the callback in SeparatorAxisTest::generate_contacts. This is just based on a quick look, maybe someone can comment on that proposed approach, and/or I could give it a shot.

Gamepro5 commented 8 months ago

@rburing @mihe Should I change the proposal? I don't know the convention for these kinds of "issues".

yosoyfreeman commented 8 months ago

In some cases though, you simply just want the normal of the surface at that contact point, without having to consider all the ins-and-outs of how the underlying collision detection works, like for example the shape margins used by GJK-EPA. I would even go so far as to argue that it's probably what most users think they're actually getting when they're making use of the current collision/contact normal in Godot.

Yeah. We I been too trying to dig into this to see if i could make a better integration of sliding a collision handling myself to being able to keep testing Godot Jolt without the current implementation problems. This combined with meaning the best (?) direction by which to separate the two bodies/shapes has show to be a real problem. I was unable to find nobody able to do this kind of complex handling in Godot. It is my sentiment that indeed there is confusion between what the engine gives you and what you would expect. I'll recognize that i'm more a tester that a programmer in this scenario, so take this with a grain of salt, but digging into the source code i found that a couple of functions, such as get_rest_info(), who do not take motion parameters into account, have some motion related code on it. There is also this comment here in test_body_motion which reads:

    /give me back regular physics engine logic
    //this is madness
    //and most people using this function will think
    //what it does is simpler than using physics
    //this took about a week to get right..
    //but is it right? who knows at this point..

With this, my intention is just to point that maybe the adoption of Godot Jolt and the physics systems is being slowed down because of clarity about what the engine is exactly doing. I think it may be worth it to check and clarify this things to help future contributors understand with what they are working and get a better picture of the whole thing.

mihe commented 8 months ago

I'll recognize that i'm more a tester that a programmer in this scenario, so take this with a grain of salt, but digging into the source code i found that a couple of functions, such as get_rest_info(), who do not take motion parameters into account, have some motion related code on it. There is also this comment here in test_body_motion which reads: [...]

With this, my intention is just to point that maybe the adoption of Godot Jolt and the physics systems is being slowed down because of clarity about what the engine is exactly doing.

While the physics server interface certainly has some quirks, I don't think there's anything particularly odd about the type of normals you currently get in the contact data. If you're looking to solve the collision between the two bodies/shapes then that's what you'd need to use, along with the contact depth. It's what pretty much all physics engines provide as the normal in their contact data and seems like a reasonable thing for Godot to do as well. The problem is that they're often treated as surface normals.

Frankly, there might even be a reasonably strong argument against this proposal for that exact reason, as other third-party physics engine integrations (PhysX, Rapier, etc.) might not be able to provide surface normals as easily. This becomes especially problematic if first-party scene nodes in Godot would ever start relying on them, like CharacterBody3D. There's not much point in having the ability to extend Godot with other physics engines (which is a somewhat killer feature in my opinion) if the interface will only ever fit one or two engines.

Gamepro5 commented 8 months ago

Frankly, there might even be a reasonably strong argument against this proposal for that exact reason, as other third-party physics engine integrations (PhysX, Rapier, etc.) might not be able to provide surface normals as easily.

I personally dont think that we should concern ourselves with what future engines might be able to do with ease. I only really agree with

This becomes especially problematic if first-party scene nodes in Godot would ever start relying on them, like CharacterBody3D.

I could see this being a problem if move_and_slide() relied on surface normals. Especially since move_and_slide() seems to be "good enough" for a lot of people using capsule shapes. Perhaps an easy solution to the argument against this proposal would be to just add a toggle where, if the surface normal value is constantly null, move_and_slide() will execute older code that doesn't rely on this theoretical future move_and_slide() that relies on surface normals.

TechnikEmpire commented 3 months ago

@Gamepro5 I found your post here while trying to port quake 3 player physics 1:1 over. Is there any reason why this doesn't work?

Gamepro5 commented 3 months ago

@TechnikEmpire yes, because it returns the collision normal of something you consider a floor. It does not actually return the surface normal.

TechnikEmpire commented 3 months ago

@Gamepro5 hmmm ok, I'll have to dig into the source code for it because if that's the case, then the docs need their language updated as well. "the surface normal of the floor at the last collision point" - to me would mean that you're getting the surface normal of the surface last collided with.