Description
What problem does this solve or what need does it fill?
avian3d, but the recent change to this crate exposes the underlying problem; so I will use it to provide examples.
Consider the following example:
use avian3d::prelude::*;
use bevy_ecs::prelude::*;
// From Avian:
#[require(
CollisionLayers,
/* ... */
)]
pub struct Collider { /* ... */ }
#[derive(Default, Component)]
#[require(CollisionLayers::NONE, Collider)]
struct A;
#[derive(Default, Component)]
#[require(A, Collider::default())]
struct B;
#[derive(Default, Component)]
#[require(A, Collider::default())]
struct C;
#[test]
fn collision_layers_a() {
let mut w = World::new();
let a = w.spawn(A);
assert_eq!(
a.get::<CollisionLayers>().unwrap().memberships,
LayerMask::NONE
);
}
#[test]
fn collision_layers_b1() {
let mut w = World::new();
let a = w.spawn(B);
assert_eq!(
a.get::<CollisionLayers>().unwrap().memberships,
LayerMask::NONE
);
}
#[test]
fn collision_layers_b2() {
let mut w = World::new();
let a = w.spawn((B, Collider::default()));
assert_eq!(
a.get::<CollisionLayers>().unwrap().memberships,
LayerMask::NONE
);
}In this example, test cases A and B1 Pass, while B2 Fails.
The only difference between B1 and B2 is the insertion of Collider::default() with B when spawning the entity.
None of these components explicitly require CollisionLayers::default(), yet that's what we end up with in case B2 despite specifying CollisionLayers::NONE as a requirement.
I understand that this is working as intended, based on the explanation from the docs:
Specifying a required component constructor for Foo directly on a spawned component Bar will result in that constructor being used (and overriding existing constructors lower in the inheritance tree). This is the classic “inheritance override” behavior people expect.
For cases where “multiple inheritance” results in constructor clashes, Components should be listed in “importance order”. List a component earlier in the requirement list to initialize its inheritance tree earlier.
However, I would argue the way this is implemented is counterintuitive and gives the user a very sneaky footgun.
It is very easy to lose track of required components, especially from external crates. So an external crate can just break crates downstream by adding a required component, which is exactly what happened to me.
I believe there is simple solution to this.
What solution would you like?
In my mind, the most intuitive approach is to ensure that a more specific required component always overrides a less specific required component, regardless of order in the declaration.
What is "More" or "Less" specific? Great question!
A Required Component declared with a constructor is more specific than one without. Period.
In our example, a Collider does not care what CollisionLayers it has. It just needs CollisionLayers.
However, A specifically requires a CollisionLayers::NONE (i.e. it requires an instance, not a type).
A's requirement should always precede Collider's requirement because it is more specific.
If there are two equally specific components within the requirement tree, then we can fallback to declaration order to resolve precedence.
For example:
#[derive(Component)]
#[require(Foo)] // <-- Type, less specific
struct A;
#[derive(Component)]
#[require(Foo::default())] // <-- Instance, more specific, doesn't matter if it's default ctor
struct B;
#[derive(Component)]
#[require(Foo::new(12))] // <-- Instance, more specific
struct C;In this case, B and Cs requirements would always override A's. But if B and C are declared together, then we resort to declaration order as usual.
What alternative(s) have you considered?
Currently, it seems like there is no way to override the Collider of a bundle without also overriding the CollisionLayers due to this implementation. This wasn't the case before Bevy 0.15, but this is only because Avian added CollisionLayers as a requirement of Collider in the latest update.
The only workaround I can think of is forcing the user to re-insert the required components in their bundles, like so:
#[test]
fn collision_layers_b3() {
let mut w = World::new();
let a = w.spawn((B, A, Collider::default())); // Note the addition of `A`
assert_eq!(
a.get::<CollisionLayers>().unwrap().memberships,
LayerMask::NONE
);
}This makes the test pass, but it feels very counterintuitive and error-prone to me.