Skip to content

Allow components to be "explicitly required". #16209

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow components to be "explicitly required". #16209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions crates/bevy_app/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,18 +849,23 @@ impl App {
/// #[derive(Component, Default, PartialEq, Eq, Debug)]
/// struct C(u32);
///
/// #[derive(Component, Default, PartialEq, Eq, Debug)]
/// struct D(u32);
///
/// # let mut app = App::new();
/// # app.add_plugins(MinimalPlugins).add_systems(Startup, setup);
/// // Register B and C as required by A and C as required by B.
/// // A requiring C directly will overwrite the indirect requirement through B.
/// app.register_required_components::<A, B>();
/// app.register_required_components_with::<B, C>(|| C(1));
/// app.register_required_components_with::<A, C>(|| C(2));
/// app.register_required_components_with::<B, C>(Some(|| C(1)));
/// app.register_required_components_with::<A, C>(Some(|| C(2)));
/// // Register D as explicitly required by B.
/// app.register_required_components_with::<B, D>(None);
///
/// fn setup(mut commands: Commands) {
/// // This will implicitly also insert B with its Default constructor and C
/// // with the custom constructor defined by A.
/// commands.spawn(A);
/// // This will implicitly also insert B with its Default constructor and C with the custom
/// // constructor defined by A. D must also be inserted since B explicitly requires it.
/// commands.spawn((A, D(5)));
/// }
///
/// fn validate(query: Option<Single<(&A, &B, &C)>>) {
Expand All @@ -872,7 +877,7 @@ impl App {
/// ```
pub fn register_required_components_with<T: Component, R: Component>(
&mut self,
constructor: fn() -> R,
constructor: Option<fn() -> R>,
) -> &mut Self {
self.world_mut()
.register_required_components_with::<T, R>(constructor);
Expand Down Expand Up @@ -981,11 +986,11 @@ impl App {
/// // Register B and C as required by A and C as required by B.
/// // A requiring C directly will overwrite the indirect requirement through B.
/// app.register_required_components::<A, B>();
/// app.register_required_components_with::<B, C>(|| C(1));
/// app.register_required_components_with::<A, C>(|| C(2));
/// app.register_required_components_with::<B, C>(Some(|| C(1)));
/// app.register_required_components_with::<A, C>(Some(|| C(2)));
///
/// // Duplicate registration! Even if the constructors were different, this would fail.
/// assert!(app.try_register_required_components_with::<B, C>(|| C(1)).is_err());
/// assert!(app.try_register_required_components_with::<B, C>(Some(|| C(1))).is_err());
///
/// fn setup(mut commands: Commands) {
/// // This will implicitly also insert B with its Default constructor and C
Expand All @@ -1002,7 +1007,7 @@ impl App {
/// ```
pub fn try_register_required_components_with<T: Component, R: Component>(
&mut self,
constructor: fn() -> R,
constructor: Option<fn() -> R>,
) -> Result<(), RequiredComponentsError> {
self.world_mut()
.try_register_required_components_with::<T, R>(constructor)
Expand Down
11 changes: 8 additions & 3 deletions crates/bevy_ecs/macros/src/component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,16 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
});
match &require.func {
Some(RequireFunc::Path(func)) => {
let constructor = if func.is_ident("explicit") {
quote! { None }
} else {
quote! {Some(|| { let x: #ident = #func().into(); x })}
};
Comment on lines +95 to +99
Copy link
Member

Choose a reason for hiding this comment

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

This could be surprising behavior to users who name their constructor explicit (for whatever reason). Could we make sure this is at least documented?

Alternatively we could do #[requires(explicit = true)] and peeking for = could be used to disambiguate the two. Not sure that's totally worth the ergonomics hit though.

register_required.push(quote! {
components.register_required_components_manual::<Self, #ident>(
storages,
required_components,
|| { let x: #ident = #func().into(); x },
#constructor,
inheritance_depth
);
});
Expand All @@ -106,7 +111,7 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
components.register_required_components_manual::<Self, #ident>(
storages,
required_components,
|| { let x: #ident = (#func)().into(); x },
Some(|| { let x: #ident = (#func)().into(); x }),
inheritance_depth
);
});
Expand All @@ -116,7 +121,7 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
components.register_required_components_manual::<Self, #ident>(
storages,
required_components,
<#ident as Default>::default,
Some(<#ident as Default>::default),
inheritance_depth
);
});
Expand Down
45 changes: 42 additions & 3 deletions crates/bevy_ecs/src/bundle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ pub struct BundleInfo {
/// and the range (0..`explicit_components_len`) must be in the same order as the source bundle
/// type writes its components in.
component_ids: Vec<ComponentId>,
required_components: Vec<RequiredComponentConstructor>,
required_components: Vec<Option<RequiredComponentConstructor>>,
explicit_components_len: usize,
}

Expand Down Expand Up @@ -648,6 +648,35 @@ impl BundleInfo {
if let Some(add_bundle_id) = archetypes[archetype_id].edges().get_add_bundle(self.id) {
return add_bundle_id;
}

fn find_missing_explicit_required_components(
bundle_info: &BundleInfo,
archetype: &Archetype,
) -> Vec<ComponentId> {
bundle_info.component_ids[bundle_info.explicit_components_len..]
.iter()
.copied()
.enumerate()
.filter(|&(i, _)| bundle_info.required_components[i].is_none())
.map(|(_, component)| component)
.filter(|&component| !archetype.contains(component))
.collect()
}

let missing_required_components =
find_missing_explicit_required_components(self, &archetypes[archetype_id]);
if !missing_required_components.is_empty() {
let names = missing_required_components
.into_iter()
.map(|id| {
// SAFETY: the caller ensures component_id is valid.
unsafe { components.get_info_unchecked(id).name() }
})
.collect::<Vec<_>>()
.join(", ");
panic!("Bundle is missing explicitly required components: {names}");
}

let mut new_table_components = Vec::new();
let mut new_sparse_set_components = Vec::new();
let mut bundle_status = Vec::with_capacity(self.explicit_components_len);
Expand All @@ -674,7 +703,12 @@ impl BundleInfo {

for (index, component_id) in self.iter_required_components().enumerate() {
if !current_archetype.contains(component_id) {
added_required_components.push(self.required_components[index].clone());
added_required_components.push(
self.required_components[index]
.as_ref()
.expect("there are no explicitly required components missing from the archetype, but we are still trying to initialize this explicitly required component")
.clone(),
);
added.push(component_id);
// SAFETY: component_id exists
let component_info = unsafe { components.get_info_unchecked(component_id) };
Expand Down Expand Up @@ -1205,7 +1239,12 @@ impl<'w> BundleSpawner<'w> {
table,
sparse_sets,
&SpawnBundleStatus,
bundle_info.required_components.iter(),
bundle_info
.required_components
.iter()
// We already checked whether the archetype contains the explicitly required
// components, so filter them out.
.filter_map(|constructor| constructor.as_ref()),
entity,
table_row,
self.change_tick,
Expand Down
87 changes: 47 additions & 40 deletions crates/bevy_ecs/src/component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,19 @@ use derive_more::derive::{Display, Error};
/// #[derive(Component, PartialEq, Eq, Debug)]
/// struct C(u32);
///
/// #[derive(Component, PartialEq, Eq, Debug)]
/// struct D(u32);
///
/// # let mut world = World::default();
/// // Register B as required by A and C as required by B.
/// world.register_required_components::<A, B>();
/// world.register_required_components_with::<B, C>(|| C(2));
/// world.register_required_components_with::<B, C>(Some(|| C(2)));
/// // Register D as explicitly required by B.
/// world.register_required_components_with::<B, D>(None);
///
/// // This will implicitly also insert B with its Default constructor
/// // and C with the custom constructor defined by B.
/// let id = world.spawn(A).id();
/// // This will implicitly also insert B with its Default constructor and C with the custom
/// // constructor defined by B. D must also be inserted since B explicitly requires it.
/// let id = world.spawn((A, D(5))).id();
/// assert_eq!(&B(0), world.entity(id).get::<B>().unwrap());
/// assert_eq!(&C(2), world.entity(id).get::<C>().unwrap());
/// ```
Expand Down Expand Up @@ -1031,7 +1036,7 @@ impl Components {
&mut self,
required: ComponentId,
requiree: ComponentId,
constructor: fn() -> R,
constructor: Option<fn() -> R>,
) -> Result<(), RequiredComponentsError> {
// SAFETY: The caller ensures that the `requiree` is valid.
let required_components = unsafe {
Expand Down Expand Up @@ -1178,7 +1183,7 @@ impl Components {
&mut self,
storages: &mut Storages,
required_components: &mut RequiredComponents,
constructor: fn() -> R,
constructor: Option<fn() -> R>,
inheritance_depth: u16,
) {
let requiree = self.register_component::<T>(storages);
Expand Down Expand Up @@ -1216,7 +1221,7 @@ impl Components {
requiree: ComponentId,
required: ComponentId,
required_components: &mut RequiredComponents,
constructor: fn() -> R,
constructor: Option<fn() -> R>,
inheritance_depth: u16,
) {
// Components cannot require themselves.
Expand Down Expand Up @@ -1687,7 +1692,7 @@ impl RequiredComponentConstructor {
#[derive(Clone)]
pub struct RequiredComponent {
/// The constructor used for the required component.
pub constructor: RequiredComponentConstructor,
pub constructor: Option<RequiredComponentConstructor>,

/// The depth of the component requirement in the requirement hierarchy for this component.
/// This is used for determining which constructor is used in cases where there are duplicate requires.
Expand Down Expand Up @@ -1731,7 +1736,7 @@ impl RequiredComponents {
pub unsafe fn register_dynamic(
&mut self,
component_id: ComponentId,
constructor: RequiredComponentConstructor,
constructor: Option<RequiredComponentConstructor>,
inheritance_depth: u16,
) {
self.0
Expand All @@ -1757,7 +1762,7 @@ impl RequiredComponents {
&mut self,
components: &mut Components,
storages: &mut Storages,
constructor: fn() -> C,
constructor: Option<fn() -> C>,
inheritance_depth: u16,
) {
let component_id = components.register_component::<C>(storages);
Expand All @@ -1771,38 +1776,40 @@ impl RequiredComponents {
pub fn register_by_id<C: Component>(
&mut self,
component_id: ComponentId,
constructor: fn() -> C,
constructor: Option<fn() -> C>,
inheritance_depth: u16,
) {
let erased: RequiredComponentConstructor = RequiredComponentConstructor(Arc::new(
move |table,
sparse_sets,
change_tick,
table_row,
entity,
#[cfg(feature = "track_change_detection")] caller| {
OwningPtr::make(constructor(), |ptr| {
// SAFETY: This will only be called in the context of `BundleInfo::write_components`, which will
// pass in a valid table_row and entity requiring a C constructor
// C::STORAGE_TYPE is the storage type associated with `component_id` / `C`
// `ptr` points to valid `C` data, which matches the type associated with `component_id`
unsafe {
BundleInfo::initialize_required_component(
table,
sparse_sets,
change_tick,
table_row,
entity,
component_id,
C::STORAGE_TYPE,
ptr,
#[cfg(feature = "track_change_detection")]
caller,
);
}
});
},
));
let erased = constructor.map(|constructor| {
RequiredComponentConstructor(Arc::new(
move |table,
sparse_sets,
change_tick,
table_row,
entity,
#[cfg(feature = "track_change_detection")] caller| {
OwningPtr::make(constructor(), |ptr| {
// SAFETY: This will only be called in the context of `BundleInfo::write_components`, which will
// pass in a valid table_row and entity requiring a C constructor
// C::STORAGE_TYPE is the storage type associated with `component_id` / `C`
// `ptr` points to valid `C` data, which matches the type associated with `component_id`
unsafe {
BundleInfo::initialize_required_component(
table,
sparse_sets,
change_tick,
table_row,
entity,
component_id,
C::STORAGE_TYPE,
ptr,
#[cfg(feature = "track_change_detection")]
caller,
);
}
});
},
))
});
// SAFETY:
// `component_id` matches the type initialized by the `erased` constructor above.
// `erased` initializes a component for `component_id` in such a way that
Expand Down
Loading