godotengine / godot-proposals

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

Add support for exposing properties only available when using a certain physics server #10438

Open Ughuuu opened 4 weeks ago

Ughuuu commented 4 weeks ago

Describe the project you are working on

Rapier Physics Server - a physics server written with rapier which adds stability and new features.

Describe the problem or limitation you are having in your project

Some physics servers have some properties which Godot currently supports that are very much implemented for a single physics server and not generic enough:

Rapier has also some extra properties that Godot Physics doesn't. Mainly it has contact_skin (which in this example godot calls margin). But this is just an example. Joints may expose more or less properties in Rapier (eg pin joints don't have softness). Just having those options makes users be confused about what it should do and what it does.

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

A Physics Server API with a bunch of extra methods used only by certain Physics Servers that are big enough and have interest in Godot (eg. Jolt, Rapier, etc.). These would be prefixed with physics engine name, and would not be implemented by Godot Physics:

Then, the nodes inspector would expose settings based on Physics Engine selected:

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

Allow the adding of extra physics server functions to api without a need to implement them everywhere (right now if I want to extend the Physics Server API, I need to implement in Godot Physics too some things, which I do not want to do, as the scope of my change really only will handle Rapier Physics). If needed by Godot Physics to also add these methods, a generic one can be added that is documented that it does nothing right now for it, but it might in the future.

Then, inside the RigidBody node, there will be extra properties that are only visible in the editor if the right physics server is selected:

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

It's currently not possible to add extra properties to existing nodes. Another option is to extend from RigidBody2D -> RapierRigidBody2D, but that is a lot of effort and godot doesn't have to do this: eg GodotRigidBody2D (well, it does in code, but in editor it doesn't). Also it will create more confusion for users using Rapier Physics.

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

The reason is if Godot does indeed want to offer more support to Physics Engines or keep the API as fixed as possible and not extend it.

mihe commented 4 weeks ago

Allow the adding of extra physics server functions to api without a need to implement them everywhere (right now if I want to extend the Physics Server API, I need to implement in Godot Physics too some things, which I do not want to do, as the scope of my change really only will handle Rapier Physics).

I realize this doesn't entirely solve the issue at hand here, but since I don't see it mentioned anywhere, keep in mind that there's nothing stopping you from exposing your derived physics server class as just another singleton, using Engine.register_singleton. With that anyone using a dynamic language (like GDScript) can access whatever methods you expose there (e.g. RapierPhysicsServer2D.body_get_whatever(...)) along with the default physics server methods.

Things get a bit more complicated when accessing it from statically compiled languages (C#, C++, Rust, etc.) but that would likely be an issue with your proposed solution as well I guess.

Another option is to extend from RigidBody2D -> RapierRigidBody2D, but that is a lot of effort

If you're only wanting to add a property/method or two then it really shouldn't be that many lines of code to do it this way (at least with GDScript or godot-cpp). The only real gotcha I can think of is if you want the new nodes to "degrade" gracefully when you switch back over to Godot Physics or something, but that can usually be solved with some downcasting.

Ughuuu commented 4 weeks ago

Thank you for the extra workaround option, indeed yes there is also the case of exposing from singleton the option and the user using it. The point I am trying to make is that there is already an option that is not used by Godot at all, margin (for 3D Collision Shape) and was used in the past by Bullet Physics System.

What I am asking by this is if other Physics Engines can get the same treatment. I do want users to see all properties that rapier has to offer directly in the inspector. However this is not possible with Godot right now, unless either:

  1. Modifying the API for all Physics Servers
  2. Creating a new node that inherits RigidBody(for eg.)
  3. I will add what you proposed too, even though it's not really a solution. Adding a bunch of functions to the RapierPhysics Server singleton that the user has to manually call.

I just made this issue/proposal to see if there would be any interest in Godot team to help the custom Physics Plugins to develop/mature and offer more help to users. Right now it feels like users have to go around doing extra things to get what they want, while Godot Physics, while sure being maybe less efficient, less stable, just works. I want that for custom Physics Servers too, to just work and be just as good as others.

Ughuuu commented 4 weeks ago

Adding here the property I am thinking about so it's more clear:

image

This reads:

Property: margin The collision margin for the shape. This is not used in Godot Physics.

This is exactly what I want, support for properties not used in Godot Physics (which could be hidden for all I know inside the inspector if the engine is not the right one, or not).

Ughuuu commented 4 weeks ago

If this is something that just cannot be done, then so be it, this is mostly a question about if such properties would ever be considered to be added, considering that there have already been some added in the past. If not I get by with the current way I do right now, exposing properties from a singleton. Not great but definitely not as good as how Godot Physics has.

Ughuuu commented 2 weeks ago

Let me reiterate here my thoughts on this, since it might seem like not much of an issue.

This issue only affects directly people who want to develop Custom Physics Engine as extensions to Godot Physics. Some examples are Godot Jolt and Godot Rapier.

Joints:

Possible options

What options would I have here? If I do want to have these custom joints, the option is to make a new base node, RapierJoint, and have nodes extend these, RapierPinJoint. Sure, but at that point, why even use the physics server?

My argument is that if the Physics Server is too rigid, there is no point to extend it at all, but instead make a new Physics Server and not even try to integrate with Godot Physics Nodes. Sadly as I go more and more to implement this, it seems this is the stance of Godot for this. Combine this with little to not much official help/word or a person in charge of the physics server makes it seem like I'm using a tool that I have no idea who made it and how to change it. Something rigid and fixed that doesn't want to change.

Compare this for example to the gdextension. An active place, where there are meetings regularly, problems are discussed and it seems like there is interest there for change.

Is it even worth extending Physics Server?

In the current state, probably not. Unless a Physics Library exists that exactly implements what Physics Server implements, and nothing more, then yes. If not, if there is less implemented, then for the users they will try this addon, be like, oh, but this is missing feature A that Godot had. Then they switch back. If it has more, it doesn't really matter, since it can't be exposed in any meaningful way that the user can directly use it (only for really advanced users).

So, in it's current state, using a Physics Library that extends Physics Server is a losing battle. It's not worth doing it for the community, since Godot Physics will always have the advantage. So just not using it at all makes way more sense, and just doing new nodes, new physics server, etc.

dsnopek commented 2 weeks ago

These would be prefixed with physics engine name, and would not be implemented by Godot Physics:

  • PhysicsServer2D -> rapier_pin_joint_extra_setting_A

Then, the nodes inspector would expose settings based on Physics Engine selected:

  • eg. if Godot Physics, display some settings
  • eg. if Rapier Physics, display some other settings

It could certainly be possible to allow the physics server to add new properties to Godot's nodes. We do a thing like that on OpenXRCompositionLayer, which allows GDExtensions to add new properties to those nodes that can be used to extend their capabilities.

See OpenXRCompositionLayer's _property_list, _get and _set functions here.

We'd need all the physics nodes to implement similar methods which call into the current physics server to get the list of extra properties, and set and get them on the underlying resource.

This would only require adding new generic methods to physics server's API - they wouldn't be specific to particular physics engines, unlike the example of adding something like rapier_pin_joint_extra_setting_A().

UPDATE: This is very, very incomplete, but just adding this patch as a rough sketch illustrating what I mean above, in case it wasn't clear from the description:

Patch with rough sketch ``` diff --git a/scene/3d/physics/joints/pin_joint_3d.cpp b/scene/3d/physics/joints/pin_joint_3d.cpp index 8e367c5ef6e..da095ce1a91 100644 --- a/scene/3d/physics/joints/pin_joint_3d.cpp +++ b/scene/3d/physics/joints/pin_joint_3d.cpp @@ -43,6 +43,18 @@ void PinJoint3D::_bind_methods() { BIND_ENUM_CONSTANT(PARAM_IMPULSE_CLAMP); } +void PinJoint3D::_get_property_list(List *p_property_list) const { + PhysicsServer3D::get_singleton()->pin_joint_get_extra_property_list(get_rid(), p_property_list); +} + +bool PinJoint3D::_get(const StringName &p_property, Variant &r_value) const { + PhysicsServer3D::get_singleton()->pin_joint_get_extra_property(get_rid(), p_property, r_value); +} + +bool PinJoint3D::_set(const StringName &p_property, const Variant &p_value) { + PhysicsServer3D::get_singleton()->pin_joint_set_extra_property(get_rid(), p_property, p_value); +} + void PinJoint3D::set_param(Param p_param, real_t p_value) { ERR_FAIL_INDEX(p_param, 3); params[p_param] = p_value; diff --git a/scene/3d/physics/joints/pin_joint_3d.h b/scene/3d/physics/joints/pin_joint_3d.h index 34c8db2890c..daeb37e8388 100644 --- a/scene/3d/physics/joints/pin_joint_3d.h +++ b/scene/3d/physics/joints/pin_joint_3d.h @@ -48,6 +48,10 @@ protected: virtual void _configure_joint(RID p_joint, PhysicsBody3D *body_a, PhysicsBody3D *body_b) override; static void _bind_methods(); + void _get_property_list(List *p_property_list) const; + bool _get(const StringName &p_property, Variant &r_value) const; + bool _set(const StringName &p_property, const Variant &p_value); + public: void set_param(Param p_param, real_t p_value); real_t get_param(Param p_param) const; diff --git a/servers/physics_server_3d.h b/servers/physics_server_3d.h index ea785fa03f1..a4fc9fd49e7 100644 --- a/servers/physics_server_3d.h +++ b/servers/physics_server_3d.h @@ -665,6 +665,10 @@ public: virtual void pin_joint_set_local_b(RID p_joint, const Vector3 &p_B) = 0; virtual Vector3 pin_joint_get_local_b(RID p_joint) const = 0; + virtual void pin_joint_get_extra_property_list(RID p_joint, List *p_property_list) const = 0; + virtual void pin_joint_set_extra_property(RID p_joint, const StringName &p_name, const Variant &p_value) = 0; + virtual void pin_joint_get_extra_property(RID p_joint, const StringName &p_name, Variant &r_value) const = 0; + enum HingeJointParam { HINGE_JOINT_BIAS, HINGE_JOINT_LIMIT_UPPER, ```
Ughuuu commented 2 weeks ago

Would this actually solve all the issues I presented?

To me it still seems the Physics Server API is designed in a way that doesn't allow for much changes. This solution you propose would only work if I were able to change all things from physics nodes with new implementation, otherwise it would still be rigid and fixed.

dsnopek commented 2 weeks ago
  • What about hiding properties (eg. pin joint softness, or joint bias).

If the _validate_property() virtual method was also used and the physics server was allowed to do something there, then, yes, you could also hide properties.

  • What about if I inherit from Joint and try to make a node of my own?

I think this should work fine? What's the challenge you're encountering here? If you make your own child class, then it can have extra properties, and call methods on a singleton/server provided by your extension.

To me it still seems the Physics Server API is designed in a way that doesn't allow for much changes. This solution you propose would only work if I were able to change all things from physics nodes with new implementation, otherwise it would still be rigid and fixed.

For the rest of your notes, I think it's inevitable that there will be some amount of left-over cruft, as we're talking about evolving an API that we don't want to break in the process. We can always add new things and deprecate old things, but the old things will probably need to stay for compatibility.

So, you can't necessarily change the meaning of an old property, but you could ignore its value and add a new custom property to replace it. We can't change the arguments of an existing virtual function on the physics server, but we can add a new function with more flexible arguments that could be used if the physics server supports it.

I think this type of evolution is possible, but it will lead to some cruft and not necessarily the prettiest looking API, which will end up creating some amount of extra work for those folks who implement physics extensions.

mihe commented 2 weeks ago

What about things that are called from nodes onto physics server, eg. PhysicsServer2D.make_pin_joint(.. anchor_a,.. ) This already sends just one anchor, not 2. If I would just add a new anchor property to pin joint, then I would need to recreate the pin joint that the node already creates somehow.

I'm curious as to what the practical problem is with this exactly? Obviously the interface of joint_make_pin (and presumably joint_make_groove) is somewhat inconsistent with all the other joints (3D included), so there might be some argument for making a PR that adds joint_make_pin2/joint_make_groove2 that addresses this, but given that you can't modify the anchors (aka reference frames in 3D) through the editor in Godot anyway (since they're hardcoded to always be the joint's position/transform) wouldn't you just end up with the same result as if you did what Godot Physics does already?

Ughuuu commented 2 weeks ago

What about things that are called from nodes onto physics server, eg. PhysicsServer2D.make_pin_joint(.. anchor_a,.. ) This already sends just one anchor, not 2. If I would just add a new anchor property to pin joint, then I would need to recreate the pin joint that the node already creates somehow.

I'm curious as to what the practical problem is with this exactly? Obviously the interface of joint_make_pin (and presumably joint_make_groove) is somewhat inconsistent with all the other joints (3D included), so there might be some argument for making a PR that adds joint_make_pin2/joint_make_groove2 that addresses this, but given that you can't modify the anchors (aka reference frames in 3D) through the editor in Godot anyway (since they're hardcoded to always be the joint's position/transform) wouldn't you just end up with the same result as if you did what Godot Physics does already?

Right now you can't edit anchors, but with the exposing of new properties that @dsnopek suggests, this might be possible (eg. extending and adding anchor_a in editor and then being able to change that). My thinking here was we expose anchor_a and anchor_b through editor using the custom properties, and then I can if this anchor is set in the custom Physics Server set the anchor.

But I don't think it will be enough, that's why I think there needs to be some changes as well in the Physics API and in the nodes that call these API's (For some of API's) such that they use a modern and complete API function (at least for what is standard/implemented widely enough in other Physics Engines).

mihe commented 2 weeks ago

Ah, I missed the part about the new properties (despite quoting it), my bad.

but with the exposing of new properties that @dsnopek suggests, this might be possible (eg. extending and adding anchor_a in editor and then being able to change that)

There will be some problems with extending any Joint*D, even with this proposed _property_list solution, which is that the physics server joints are currently cleared (through PhysicsServer*D::joint_clear) whenever the corresponding node exits the scene tree, and whenever certain properties are changed. The physics server joint then gets completely re-initialized from the virtual method Joint*D::_configure_joint. This means that any extended property you set the value of will (or at least should) have its value discarded when the joint is cleared, but won't be set again as part of _configure_joint, since it has no idea about these new properties.

This could be easily fixed for the case of inheriting from Joint*D specifically, by just exposing _configure_joint as a GDVIRTUAL method in the bindings. I honestly can't remember why I didn't make a PR for this myself when I first implemented the joints in Godot Jolt, seeing as how I added a bunch of other missing pieces in the bindings, but frankly I never had a problem with the JoltJoint3D approach, and haven't seen any complaints about it. The default Joint3D joints work well enough with Jolt that most people probably don't feel the need to use them though, and there are Jolt substitutes for every joint, making the case of mixing both Joint3D and JoltJoint3D unlikely I guess.

Fixing this problem for the case of _property_list might perhaps be as simple as just caching all the extension property values (much like what seems to be done in OpenXRCompositionLayer already) in the joint's scene node, and push them along with the rest of the values in _configure_joint, but I haven't pondered every edge-case.

Frankly I'm tempted to try adding this property extension stuff myself. I knew about _property_list and all that, but I wasn't aware of the precedence in OpenXRCompositionLayer, which looks pretty neat, and surprisingly non-invasive.

The one place where my JoltJoint3D approach does become a bit of a showstopper is with PhysicalBone3D, which only supports the default joints for connecting the bones, so if you want to use soft limits there you're somewhat screwed when using Godot Jolt, since you also don't have access to the underlying joint RID anywhere. So if only just to solve that problem (in a good way) I'd be tempted to take a crack at this.

I should have some time next week that I can spend testing this out, if no one else beats me to the punch.

[^1]: The default joints do work well enough in Godot Jolt though, which probably helps a lot with this, and which might not be the case with every physics engine I guess.

dsnopek commented 2 weeks ago

Fixing this problem for the case of _property_list might perhaps be as simple as just caching all the extension property values (much like what seems to be done in OpenXRCompositionLayer already) in the joint's scene node, and push them along with the rest of the values in _configure_joint, but I haven't pondered every edge-case.

I don't know the physics APIs terribly well, but this sounds like a good option to me.

Ughuuu commented 2 weeks ago

I should have some time next week that I can spend testing this out, if no one else beats me to the punch.

I'm not sure I understand how in great detail, but if you want to do it I would be very happy indeed. I'm not sure how complicated it would be do to, and where godot-cpp would come into play in all this (I would assume we need to wait until 4.4 for it to be officially integrated anyway), and how fast that would come along. But if needed I can take a look also.

And about other things, eg. RigidBodies, StaticBodies, etc. Could those follow same practice for extension? Anyway, if yes first lets see how the joint one goes, if you can get it working, then we would at least have a clear and proven way of doing this.

In any case, I say we should think about this good and well, so we don't make a new API that is just as rigid. If it means making a new API that extends a bunch of unused properties, but most physics engine has them, thats not too bad (in my view, but lets see).

My only fear is that there is so much that needs to change, and I doubt it will get fixed by in time for 4.4. But lets see.

mihe commented 2 weeks ago

RigidBodies, StaticBodies, etc. Could those follow same practice for extension?

Yes, that would/should be the plan. Having this only for joints seems questionable.

In any case, I say we should think about this good and well, so we don't make a new API that is just as rigid.

The thing I'm unsure about is how this would play out with the base classes, like CollisionObject*D and PhysicsBody*D. Ideally those would also get these property extensions, but the physics server API has no idea about any of this class hierarchy (and probably shouldn't in my opinion) so what would something like a PhysicsServer*D::body_get_extra_property_list return, and where would it be called?

I guess the answer could be "only on the leaf classes", but that isn't very clear-cut either, since you have AnimatableBody*D inheriting from StaticBody*D, and distinguishing between something like a CharacterBody*D and an AnimatableBody*D from the physics server alone is likely impossible. This would also mean that you could never add properties to something like PhysicsObject*D, which seems unfortunate.

The kneejerk fix to that is of course to instead just have the names of the actual nodes in the physics server API, so you'd have PhysicsServer*D::animatable_body_get_property_list and whatnot, but that does bum me out a little bit, since I quite like the node-agnostic design of the server API. Perhaps some sort of StringName identifier could be passed along with PhysicsServer*D::body_get_extra_property_list instead, containing the class name of the thing you're extending.

I also see two other limitations with this approach:

  1. You won't have access to any of the scene-side state of this Object in the physics server's _get/_set implementations, so you would be forced to rely solely on whatever state you have on the physics server side of things, which might not be enough, or might not be up-to-date. I don't think this is a huge problem in practice, nor do I see a good solution to it, but it might be a gotcha with some advanced use-cases.
  2. You still won't be able to add new methods to the objects, since this is centered strictly around properties. I'm not sure if there's some equivalent of _property_list for methods in Godot already, in which case that might be something for the future, assuming it even makes sense to begin with.

Anyway, I think it's worth exploring despite all that.

mihe commented 1 week ago

I only have a minor update here for now, but I've at least got the general flow working of a physics server being able to provide the implementation for all the property-related methods of a particular object. That was never really much of a concern though, since it's essentially just about proxying those calls, but I guess it does prove that it can be achieved with the existing capabilities of the bindings (since you're limited in what types you can send across the script/extension boundary) which I guess is a good thing.

I only have a basic Joint3D property working for now. I still need to wire up the caching of values for _configure_joint that I mentioned above, as well as the rest of the physics classes. I'll try to have a draft PR up some time later this week.

Ughuuu commented 1 week ago

Nice, thanks again for taking a look into this.

Another use case I thought of using this for is for loading/unloading of physics world state with exposing a new unique id for each object. Right now all objects have RID's and if i want to reload the objects, I won't have a good way to match them(imagine rb 1 has rid 1, but when I do scene reload and then PhysicsServer2D.load() the RID's won't match anymore). With this I could expose a new property, unique id or something similar, and have it both internally in physics server and also be able to serialize it on the node side. Making it thus invisible to the user.