godotengine / godot-proposals

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

Expose PhysicalBone's skeleton parameter to allow physical bones outside the tree of the skeleton node #4664

Open Riordan-DC opened 2 years ago

Riordan-DC commented 2 years ago

Describe the project you are working on

A FPS with many NPCs based on the same generic NPC.

Describe the problem or limitation you are having in your project

Physical bones have to be children of a skeleton. This is difficult because the hierarchy of the skeleton is kept as external because we want it to automatically update if we reimport the model. With the current method we force uses to mix external the external hierarchy with the scenes. This means if I change the external model I can risk breaking all the work I did on my ragdoll (creating its joints / limits / shapes etc).

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

We expose the skeleton parameter of Physical Bone to the user in the editor and in GDScript. This allows a user to keep their Physical bones anywhere in their trees. Imagine a generic NPC character which can have its model swapped but maintain the same ragdoll setup, speeding up development time.

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

We remove the code that automatically finds the parent skeleton and replace it with code that provides an editor export of a skeleton NodePath which can be set in the Physical Bone node in the editor or in code.

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

There is no workaround as the current Physical Bone does not work unless its parent is a skeleton which requires it to be a child of the skeleton node. The only workaround I see is building a GDNative addon that implements a new node called the GlobalPhysicalBone which has all the same Physical Bone code + this feature.

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

This edit is to the 3D node Physical Bone in Godot's C++ code. An asset library could work but we would be reimplementing all the physical bone functionality in Godot already. I imagine this works and will try it but I think for such a small change and such a large quality of life improvement it should be incorporated into Godot's codebase.

fire commented 2 years ago

@TokageItLab Can you evaluate this?

TokageItLab commented 2 years ago

For example, is it meant to place a CollisionShape in a location that is not a child of a RigidBody? I don't see much worth in that.

If the problem is that the ragdolls and settings are broken, then removing the cause of it is the right way to go.

I think the shown problem that in the case of moving the PhysicalBone to another model, I think it can be solved by preparing a generic PhysicalBone and instantiating it then assigning it to each model, rather than moving/removing the PhysicalBone of the original model.

I don't think it is necessary to add the ability to change the hierarchy.

Riordan-DC commented 2 years ago

For example, is it meant to place a CollisionShape in a location that is not a child of a RigidBody? I don't see much worth in that.

If the problem is that the ragdolls and settings are broken, then removing the cause of it is the right way to go.

I think the shown problem that in the case of moving the PhysicalBone to another model, I think it can be solved by preparing a generic PhysicalBone and instantiating it then assigning it to each model, rather than moving/removing the PhysicalBone of the original model.

I don't think it is necessary to add the ability to change the hierarchy.

The idea isn't to move collision shapes from being children of rigid bodies. The collision shape inherits transforms from its parent physics body. A physical bone isn't dependent on the skeleton node in the same way. It is dependent on a specific bone (which doesn't exist in the tree). So why not allow physical bones anywhere in the tree? Why must they be children of the target skeleton? I'd like to keep all my physical bones grouped together under a node called "ragdoll bones" or "hit boxes". Then I can save and reuse these groups at once. I also have the same issue with skeleton IK. It MUST be a child of its skeleton instead of also allowing a skeleton to be assigned in editor. Why? Is skeleton IK inheriting transforms from the skeleton? No. It is just for convenience so that it will automatically detect the target skeleton (being its parent). I propose we don't remove this "find skeleton parent" feature but just add a way to set the target skeleton if a designer desires.

I'm short: Having a node automatically find its user is convenient for some and for others it limits the way they want to structure their scene. So we should provide both options. :)

TokageItLab commented 2 years ago

I think it is because at least PhysicalBone's Collisions also needs to follow the transformation of the parent target skeleton and bone pose.

SkeletonIK is deprecated. It will be replaced by SkeletonModifireStack.

It would be ideal if PhysicalBone could also be ported to SkeletonModifireStack, but even then the Collisions would need to exist as a separate Node since collisions would affect external objects. Also, these Collisions still need to be children of Skeleton in order to inherit Skeleton transformation.

If the collision shape of the PhysicalBone does not inherit the transformation of the parent Skeleton, then it might be a bug.