Add scene-wide collision settings for imported scenes.#98018
Add scene-wide collision settings for imported scenes.#98018SaracenOne wants to merge 2 commits intogodotengine:masterfrom
Conversation
193f8f7 to
4007aec
Compare
4007aec to
406dfb4
Compare
There was a problem hiding this comment.
Tested locally, it works as expected.
I'm not sure if there's a lot of use to the primitive box/sphere/cylinder/capsule colliders as a default though, given scenes made of multiple meshes will most likely have different shapes for each part of the scene. The trimesh and convex options are more suited here. I guess it's not a big problem to expose anyway.
5e1adca to
a67a0ff
Compare
|
This has been rebased and now also includes full documentation. |
4618e6d to
82d0276
Compare
Mickeon
left a comment
There was a problem hiding this comment.
I am pleasantly surprised in the way these descriptions are subtly written to guide the reader. For example, meshes/physics/shape_type shares which shapes are primitive, as a point of reference for the next descriptions.
As future reference, a lot of these settings are also present in MeshConvexDecompositionSettings, and they have entirely different descriptions. The ones in this PR are somewhat more informative. Perhaps it would be a good idea to standardize them in the future.
This setting allow you to choose between Disabled and Static. By choosing Static, all Meshes of the 3D scene will be imported with a Static Trimesh collider. Then each individual Mesh can override the setting. To accommodate for this, the Physics option from a Mesh was changed from a bool to an Enum with the options: Default, Enabled, Disabled. All meshes are Default by default, which will use the new "Generate Colliders" global value. With Enabled, the Mesh can override physics options and Disabled is self descriptive. Implements and closes godotengine/godot-proposals#5955
82d0276 to
5ffd7b5
Compare
|
Rebased and implement @Mickeon's recommendation for re-wording of certain documentation. |
| Node *_pre_fix_node(Node *p_node, Node *p_root, HashMap<Ref<ImporterMesh>, Vector<Ref<Shape3D>>> &r_collision_map, Pair<PackedVector3Array, PackedInt32Array> *r_occluder_arrays, List<Pair<NodePath, Node *>> &r_node_renames, const HashMap<StringName, Variant> &p_options); | ||
| Node *_pre_fix_animations(Node *p_node, Node *p_root, const Dictionary &p_node_data, const Dictionary &p_animation_data, float p_animation_fps); | ||
| Node *_post_fix_node(Node *p_node, Node *p_root, HashMap<Ref<ImporterMesh>, Vector<Ref<Shape3D>>> &collision_map, Pair<PackedVector3Array, PackedInt32Array> &r_occluder_arrays, HashSet<Ref<ImporterMesh>> &r_scanned_meshes, const Dictionary &p_node_data, const Dictionary &p_material_data, const Dictionary &p_animation_data, float p_animation_fps, float p_applied_root_scale, const String &p_source_file, const HashMap<StringName, Variant> &p_options); | ||
| Node *_post_fix_node(Node *p_node, Node *p_root, HashMap<Ref<ImporterMesh>, Vector<Ref<Shape3D>>> &collision_map, Pair<PackedVector3Array, PackedInt32Array> &r_occluder_arrays, HashSet<Ref<ImporterMesh>> &r_scanned_meshes, const Dictionary &p_node_data, const Dictionary &p_material_data, const Dictionary &p_animation_data, float p_animation_fps, float p_applied_root_scale, const String &p_source_file, const HashMap<StringName, Variant> &p_options, int p_default_collision); |
There was a problem hiding this comment.
Use the enum type instead of int. You need to bind the enum with VARIANT_ENUM_CAST and BIND_ENUM_CONSTANT.
There was a problem hiding this comment.
I don't disagree, though I might want to investigate why we haven't done this for any of the other enum types in the import system either. It might simply be because the .import files don't really have any concept of enums yet and will simply serialize them as raw integers (this is very bad and fragile, but I think fixing this should probably be part of a broader improvement to the import system which may go a bit beyond the scope of this PR). Either way, let me know if you feel like this a sticking point; I've at least gone and change the internal handling of this value to work with the enum type, but I'm a bit hesitant to expose this enum to the scripting layer since there no-consistency precedent in the context of the import system, no practical use of this value for the end-user, and I wonder if this should be part of a broader set of improvements to the system.
There was a problem hiding this comment.
Is the issue that this enum would look very similar to another enum but off by 1 and it would be confusing to users which enum constants to use?
Is there a way to choose which subset of enum values are visible to the user (hide "Default" from the advanced importer settings, but show it on the global import dock)?
I agree with Saracen that what is suggested is just not trivial from the advanced importer (and anyway the properties won't be properly exposed to scripting, so people will be manipulating int values in a dictionary either way).
@aaronfranke Would you be open to perhaps doing this enum change as a separate PR so we can see what is required to make typed enums happen for import settings without mixing it with the rest of the complexity of this change.
There was a problem hiding this comment.
Changing the public API later breaks compatibility, and using an enum is clearer in the code. If we think it's fine to use non-enum integers then maybe we can go ahead, but I would prefer to "get the API right the first time". I don't think we should merge this in with the intent to change it in a follow-up PR. Unless you mean opening a separate PR before this one is merged, which would change existing numbers to enums and explore if that can be done without compatibility breakage or not, in which case I am in favor of that.
There was a problem hiding this comment.
Oh, I see the problem. I didn't notice the extra int function argument to the virtual function.
I think we should avoid modifying the API and instead use the existing p_options argument to lookup the setting from inside _post_fix_node:
CollisionMode default_collisions = COLLISION_DISABLED;
if (p_options.has(SNAME("meshes/generate_collisions"))) {
default_collisions = (CollisionMode)p_options["meshes/generate_collisions"];
}
I understand there is some inconsistency between this and other settings such as fps or root_scale, but this is just for historical reasons. The argument cannot be removed since we've had fps in the API for a long time, so it stays there. But for new things I think the code is generally reading the options from p_options instead of making an argument for each new setting.
| enum CollisionMode { | ||
| COLLISION_DISABLED, | ||
| COLLISION_STATIC, | ||
| }; |
There was a problem hiding this comment.
Why not the other motion types? Also, why doesn't this also have "default"?
There was a problem hiding this comment.
The other motion types I believe were discussed, and I think there was some hesitation over broadening the useful scope of the feature (your most-likely application for this is for people wanting to bring in full kitbashed 3D scenes without manually having to set up collision on every node). As for why disabled is the default, that's simply because the default behaviour for importing individual nodes is to simply not create collision for node unless explicitly stated otherwise. The basic idea for the flow is you have scene-wide collision generation settings, and each individual node is now instead set up with 'default' instead which will match the scene-wide settings. This also ensures this feature is implemented with backwards-compatibility of existing setups.
There was a problem hiding this comment.
I think the other types could make sense when Godot finally gets support for indirect collision shape children of rigid bodies.
Basically, I think it is a bit unusual to have an option to automatically convert all mesh instances in the scene to rigid bodies or other types, once per mesh (the scene is not one object, but instead exactly one physical object per mesh). This is very awkward and probably not what the user wants.
I think the user might want to import the whole scene as a single rigid body, but this is where we get the problem with indirect children.
For static bodies, we don't have this problem because you can safely create as many or as few StaticBody3D nodes in the scene without impacting the physical nature.
There was a problem hiding this comment.
See also godotengine/godot-proposals#542 and #106048
Honestly, I'd really like to get these merged in first. Having the importer generate CollisionShape3D nodes would be a much better workflow for most cases. so I'd like to see that be both available and given priority, and the scene import code can work around that. So I'd like to consolidate the plan for this PR together with #106048, but that requires merging #77937 first, which is still awaiting approval.
* All shape types can be chosen. * Physics materials, masks and layers can be chosen. * Decomposition and primitive settings. * Visualization of collision appears immediately.
5ffd7b5 to
f74105c
Compare
| decomposition_default.instantiate(); | ||
| r_options->push_back(ImportOption(PropertyInfo(Variant::BOOL, "meshes/decomposition/advanced", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), false)); | ||
| r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "meshes/decomposition/precision", PROPERTY_HINT_RANGE, "1,10,1"), 5)); | ||
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/max_concavity", PROPERTY_HINT_RANGE, "0.0,1.0,0.001", PROPERTY_USAGE_DEFAULT), decomposition_default->get_max_concavity())); |
There was a problem hiding this comment.
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/max_concavity", PROPERTY_HINT_RANGE, "0.0,1.0,0.001", PROPERTY_USAGE_DEFAULT), decomposition_default->get_max_concavity())); | |
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/max_concavity", PROPERTY_HINT_RANGE, "0.0,1.0,0.001"), decomposition_default->get_max_concavity())); |
Redundant, for all these
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/max_concavity", PROPERTY_HINT_RANGE, "0.0,1.0,0.001", PROPERTY_USAGE_DEFAULT), decomposition_default->get_max_concavity())); | ||
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/symmetry_planes_clipping_bias", PROPERTY_HINT_RANGE, "0.0,1.0,0.001", PROPERTY_USAGE_DEFAULT), decomposition_default->get_symmetry_planes_clipping_bias())); | ||
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/revolution_axes_clipping_bias", PROPERTY_HINT_RANGE, "0.0,1.0,0.001", PROPERTY_USAGE_DEFAULT), decomposition_default->get_revolution_axes_clipping_bias())); | ||
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/min_volume_per_convex_hull", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT), decomposition_default->get_min_volume_per_convex_hull())); |
There was a problem hiding this comment.
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/min_volume_per_convex_hull", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT), decomposition_default->get_min_volume_per_convex_hull())); | |
| r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "meshes/decomposition/min_volume_per_convex_hull"), decomposition_default->get_min_volume_per_convex_hull())); |
Same for all these
| r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "meshes/physics/mask", PROPERTY_HINT_LAYERS_3D_PHYSICS), 1)); | ||
|
|
||
| // Decomposition | ||
| Ref<MeshConvexDecompositionSettings> decomposition_default = Ref<MeshConvexDecompositionSettings>(); |
There was a problem hiding this comment.
| Ref<MeshConvexDecompositionSettings> decomposition_default = Ref<MeshConvexDecompositionSettings>(); | |
| Ref<MeshConvexDecompositionSettings> decomposition_default; |
| Local position offset for generated primitive shapes. Only used when [member meshes/physics/shape_type] is set to a primitive type. | ||
| </member> | ||
| <member name="meshes/primitive/radius" type="float" setter="" getter="" default="1.0"> | ||
| The radius of generated primitive shapes. Only used when [member meshes/physics/shape_type] is set to [b]Spheres[/b], [b]Cylinders[/b], and [b]Capsules[/b]. |
There was a problem hiding this comment.
| The radius of generated primitive shapes. Only used when [member meshes/physics/shape_type] is set to [b]Spheres[/b], [b]Cylinders[/b], and [b]Capsules[/b]. | |
| The radius of generated primitive shapes. Only used when [member meshes/physics/shape_type] is set to [b]Spheres[/b], [b]Cylinders[/b], or [b]Capsules[/b]. |
Supersedes #70256
Long overdue taking over of this PR which I agreed to do but kind of forgot about. 👀
Reopening it now due to recently realizing the extent of what a massive usability slog it is for users to manually have to set individual collision settings for each individual mesh, which is pretty bad if you're, say, wanting to rapidly prototype a complex character controller in a variety of 3D scenes. As far as I can tell, the way that the enums are set up should mean there is no backwards compatibility breakage with existing projects; imported scenes which have physics settings set to off should now be set to default, which uses the scene settings which is also off, so it should all line up fine.
This PR makes the following changes to the initial implementation: