Support default layers and density for ColliderConstructorHierarchy
#386
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jondolf
force-pushed
the
more-default-config-for-hierarchies
branch
2 times, most recently
from
1c49621
to
9ee6f30
Compare
Jondolf
force-pushed
the
more-default-config-for-hierarchies
branch
from
9ee6f30
to
8cbe48c
Compare
janhohenheim
suggested changes
Jun 27, 2024
| #[cfg_attr(feature = "serialize", derive(serde::Serialize, serde::Deserialize))] | ||
| #[cfg_attr(feature = "serialize", reflect(Serialize, Deserialize))] | ||
| #[cfg_attr(all(feature = "3d", feature = "collider-from-mesh"), reflect(Default))] | ||
| pub struct ColliderConstructorHierarchyConfig { |
There was a problem hiding this comment.
It's weird how everything but the constructor is optional. With this PR, wouldn't it make sense to want to override only the layers and leave the default constructor? I'd suggest making the constructor an Option as well.
This will also remove the special-cased Default and the need for ColliderConstructorHierarchyConfig::from_constructor
There was a problem hiding this comment.
Yeah, I agree. I made the constructor an Option and reworked the related logic a bit
janhohenheim
suggested changes
Jun 29, 2024
janhohenheim
approved these changes
Jun 29, 2024
…com/Jondolf/bevy_xpbd into more-default-config-for-hierarchies
Jondolf
added
the
C-Breaking-Change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
When loading a (glTF) scene and generating colliders for it, it can be useful to be able to add all entities in that scene to some collision layer. However,
ColliderConstructorHierarchy(previouslyAsyncSceneCollider, see #378) doesn't currently allow specifying default collision layer or density configurations despite allowing configurations for individual entities.Solution
Add
default_layersanddefault_densityproperties forColliderConstructorHierarchy. Thelayersanddensityproperties ofColliderConstructorHierarchyDataare nowOptions, and if they areNone, the default values are used.I also moved the collider constructor types and tests to their own module, and updated some docs to remove mesh-specific wording.
For reviewing purposes, the first commit (8cbe48c) should be easier to read, the other commit just moves code to the
constructormodule.Migration Guide
If the default constructor for a
ColliderConstructorHierarchyisNone, meshes no longer default to triangle mesh colliders being generated even if they have a density or layers configured. The constructor must be specified either individually or through the default constructor.