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

Improve collision generation usability in the new 3D scene import workflow. #52266

Merged
merged 2 commits into from
Sep 14, 2021

Conversation

AndreaCatania
Copy link
Contributor

@AndreaCatania AndreaCatania commented Aug 30, 2021

Mirror of: #51985 that was merged for mistake.


Implements the proposal: godotengine/godot-proposals#3155

With this PR it's possible to add a collision during the Mesh import, directly in editor.
To generate the shape is possible to chose between the following options:

  • Decompose Convex: The Mesh is decomposed in one or many Convex Shapes (Using the VHACD library).
  • Simple Convex: Is generated a convex shape that enclose the entire mesh.
  • Trimesh: Generate a trimesh shape using the Mesh faces.
  • Box: Add a primitive box shape, where you can tweak the size, position, rotation.
  • Sphere: Add a primitive sphere shape, where you can tweak the radius, position, rotation.
  • Cylinder: Add a primitive cylinder shape, where you can tweak the height, radius, position, rotation.
  • Capsule: Add a primitive capsule shape, where you can tweak the height, radius, position, rotation.

It's also possible to chose the generated body, so you can create:

  • Rigid Body.
  • Static Body.
  • Area.

See it in action here: https://youtu.be/I-N1IXgeb00

2021-08-27.09-36-34.mp4

The constructor was expecting a mutable pointer, while the passed pointer was
immutable (returns a const pointer), so the compilation was failing when iterating a constant.
// Decomposition
Mesh::ConvexDecompositionSettings decomposition_default;
r_options->push_back(ImportOption(PropertyInfo(Variant::BOOL, "decomposition/advanced", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), false));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "decomposition/precision", PROPERTY_HINT_RANGE, "1,10,1", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), 5));
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED modified for these. This flag is only for when changing the property results in other properties being added/removed/changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For advanced, I need it to make sure to show all the other properties, but for precision you right, I don't need it, nice catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -980,6 +1017,65 @@ bool ResourceImporterScene::get_internal_option_visibility(InternalImportCategor
case INTERNAL_IMPORT_CATEGORY_NODE: {
} break;
case INTERNAL_IMPORT_CATEGORY_MESH_3D_NODE: {
const bool generate_physics =
p_options.has("generate/physics") &&
Copy link
Member

Choose a reason for hiding this comment

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

If I am not mistaken, using .has() is not needed, it is warranted that you will always get all the properties filled in (and if not there is likely a bug).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling .has() is necessary, to avoid crash the engine while using the constant version of operator[]: https://github.com/godotengine/godot/blob/master/core/templates/map.h#L662

@@ -1016,6 +1112,33 @@ bool ResourceImporterScene::get_internal_option_visibility(InternalImportCategor
return true;
}

bool ResourceImporterScene::get_internal_option_update_view(InternalImportCategory p_category, const String &p_option, const Map<StringName, Variant> &p_options) const {
Copy link
Member

Choose a reason for hiding this comment

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

may use a different function like get_internal_option_update_view_required to make it a bit clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, make sense, I'll change that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

…kflow.

With this PR it's possible to add a collision during the Mesh import, directly in editor.
To generate the shape is possible to chose between the following options:
- Decompose Convex: The Mesh is decomposed in one or many Convex Shapes (Using the VHACD library).
- Simple Convex: Is generated a convex shape that enclose the entire mesh.
- Trimesh: Generate a trimesh shape using the Mesh faces.
- Box: Add a primitive box shape, where you can tweak the `size`, `position`, `rotation`.
- Sphere: Add a primitive sphere shape, where you can tweak the `radius`, `position`, `rotation`.
- Cylinder: Add a primitive cylinder shape, where you can tweak the `height`, `radius`, `position`, `rotation`.
- Capsule: Add a primitive capsule shape, where you can tweak the `height`, `radius`, `position`, `rotation`.

It's also possible to chose the generated body, so you can create:
- Rigid Body
- Static Body
- Area
@reduz
Copy link
Member

reduz commented Sep 9, 2021

Looks awesome, if someone else wants to give it a check to give feedback (@fire and @JFonS ?) please be welcome to, otherwise will merge in the coming days.

@fire
Copy link
Member

fire commented Sep 9, 2021

I checked the functionality of this previously. The changes to the default precision from 0 to 5+ and defaulting to convex decompose have been made.

We discussed how it was possible to make test cases for the matrix of cases.

Looks good to me.

params.m_resolution = p_settings.resolution;
params.m_maxNumVerticesPerCH = p_settings.max_num_vertices_per_convex_hull;
params.m_planeDownsampling = p_settings.plane_downsampling;
params.m_convexhullDownsampling = p_settings.convexhull_downsampling;
Copy link
Member

@fire fire Sep 9, 2021

Choose a reason for hiding this comment

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

wanted: spelling changes on convexhull vs convex_hull. I prefer "convex_hull". The usage is inconsistent.

This change should not block merge.

Copy link
Contributor

@JFonS JFonS left a comment

Choose a reason for hiding this comment

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

Looks good :)

@akien-mga akien-mga merged commit e13d8ed into godotengine:master Sep 14, 2021
@akien-mga
Copy link
Member

Thanks!

@aaronfranke
Copy link
Member

This doesn't work with scaled meshes. It scales the physics body instead of the mesh, which breaks physics (see the ⚠️):

scale

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.

7 participants