Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cone primitive shape for 3D #96288

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

LRJiao
Copy link

@LRJiao LRJiao commented Aug 29, 2024

Attempt at a 3D cone collision shape (mesh added for posterity, same as cylinder mesh but with top radius at 0 default). Demo attached with all the primitive shapes - some rough edges.

cone_demo.zip

@@ -1609,22 +1614,25 @@
<constant name="SHAPE_CAPSULE" value="4" enum="ShapeType">
The [Shape3D] is a [CapsuleShape3D].
</constant>
<constant name="SHAPE_CYLINDER" value="5" enum="ShapeType">
<constant name="SHAPE_CONE" value="5" enum="ShapeType">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an enum value in the middle like this breaks compatibility, I can say this for certain right off the bat.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is, this enum constant should be added at the very end of the list. It's fine if it's out of order, compatibility is more important.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I was trying to follow an alphabetical trend, thank you for letting me know!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the checks want the documentation to be in alphabetical? The check is failing and prefers it in position 5 rather than at the end of the list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not enough to change only the documentation, you also need to change servers/physics_server_3d.h.

@Mickeon
Copy link
Contributor

Mickeon commented Aug 29, 2024

Could you be more detailed and specific?

At first, I assumed this was going to be a default CSGMesh _and that's it, which I would be against, but there can instead be some merit in this 3D physics shape.

I would recommend looking for, or opening a proposal before notable contributions like these, to see if there is interest.

@LRJiao
Copy link
Author

LRJiao commented Aug 29, 2024

Ah sorry, I'm not familiar with how these are meant to be formatted, I wanted this to be a part of godotengine/godot-proposals#610 this feature request - its a 3D Primitive CollisionShape (I added in the dropdowns for the cones)

@AThousandShips AThousandShips changed the title Cone primitive shape for 3D Add cone primitive shape for 3D Aug 29, 2024
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if you've considered having a split bottom radius and top radius, to allow tapered cones (cylinder with different top and bottom).

also curious if it would be easier to add this functionality to the existing cylinder type rather than make a whole new type.

(Note for context that I had a discussion last year with some industry people e.g. Pixar who mentioned that their application requires capsules with different top and bottom radius, and this language exists in the KHR_physics_shape draft. Though cylinder/cone are not considered there due to ease of emulation with a convex hull, it might make sense to model cylinder in the same way.)

@@ -212,6 +213,7 @@ class ResourceImporterScene : public ResourceImporter {
SHAPE_TYPE_SPHERE,
SHAPE_TYPE_CYLINDER,
SHAPE_TYPE_CAPSULE,
SHAPE_TYPE_CONE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not allowed to insert values in the middle of an enum, because it will break compatibility with any content using the value after (automatic)

@@ -1982,7 +1982,7 @@ void ResourceImporterScene::get_internal_import_options(InternalImportCategory p
r_options->push_back(ImportOption(PropertyInfo(Variant::BOOL, "generate/physics", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), false));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "generate/navmesh", PROPERTY_HINT_ENUM, "Disabled,Mesh + NavMesh,NavMesh Only"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "physics/body_type", PROPERTY_HINT_ENUM, "Static,Dynamic,Area"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "physics/shape_type", PROPERTY_HINT_ENUM, "Decompose Convex,Simple Convex,Trimesh,Box,Sphere,Cylinder,Capsule,Automatic", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), 7));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "physics/shape_type", PROPERTY_HINT_ENUM, "Decompose Convex,Simple Convex,Trimesh,Box,Sphere,Cylinder,Capsule,Cone,Automatic", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), 7));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cone should be added to the end of the list to avoid compatibility break (if it is determined that numeric compatibility is not necessary due to automatic being the default value, make sure to change that 7 to 8)

@LRJiao
Copy link
Author

LRJiao commented Aug 30, 2024

Curious if you've considered having a split bottom radius and top radius, to allow tapered cones (cylinder with different top and bottom).

also curious if it would be easier to add this functionality to the existing cylinder type rather than make a whole new type.

(Note for context that I had a discussion last year with some industry people e.g. Pixar who mentioned that their application requires capsules with different top and bottom radius, and this language exists in the KHR_physics_shape draft. Though cylinder/cone are not considered there due to ease of emulation with a convex hull, it might make sense to model cylinder in the same way.)

I can definitely look into trying to add a top and bottom radius, but I didn't want to alter anything in the other shapes' code so I stayed away from trying to change cylinder.

@aaronfranke
Copy link
Member

See also godotengine/godot-proposals#610 (comment) and jrouwe/JoltPhysics#1241 - we will likely go with a tapered cylinder instead of a cone, which includes cones and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a ConeShape primitive 3D collision shape
4 participants