Skip to content

Get names of queued components #18451

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

Merged
merged 16 commits into from
Mar 31, 2025
Merged

Get names of queued components #18451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c9509fe
include descriptor on queued components
ElliottjPierce Mar 20, 2025
d7bc388
remove indirection in register_resource_unchecked_with
ElliottjPierce Mar 20, 2025
788ed67
change backing descriptors to arcs
ElliottjPierce Mar 20, 2025
b6aa68e
allow getting the descriptor and name of queued ids
ElliottjPierce Mar 20, 2025
f9f3264
cleanup
ElliottjPierce Mar 20, 2025
ddc4a6f
superficial changes while I'm here
ElliottjPierce Mar 20, 2025
5f8e208
fixed doc
ElliottjPierce Mar 21, 2025
90ad95f
use a Cow to prevent clones
ElliottjPierce Mar 21, 2025
2f01fa2
doc and usability improvements
ElliottjPierce Mar 21, 2025
b7b785f
clean up some related issues
ElliottjPierce Mar 21, 2025
5fedc19
use atomicow and only arc if needed
ElliottjPierce Mar 21, 2025
dcb9dd3
Chain iterators
ElliottjPierce Mar 31, 2025
4d90ac5
remove atomic cow and simplify
ElliottjPierce Mar 31, 2025
3e632b6
fix doc
ElliottjPierce Mar 31, 2025
470dc37
Update crates/bevy_ecs/src/component.rs
ElliottjPierce Mar 31, 2025
47b203c
remove unused import
ElliottjPierce Mar 31, 2025
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
4 changes: 2 additions & 2 deletions crates/bevy_ecs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ bevy_reflect = ["dep:bevy_reflect"]
reflect_functions = ["bevy_reflect", "bevy_reflect/functions"]

## Use the configurable global error handler as the default error handler.
##
##
## This is typically used to turn panics from the ECS into loggable errors.
## This may be useful for production builds,
## but can result in a measurable performance impact, especially for commands.
Expand Down Expand Up @@ -110,7 +110,6 @@ bevy_platform_support = { path = "../bevy_platform_support", version = "0.16.0-d
] }

bitflags = { version = "2.3", default-features = false }
concurrent-queue = { version = "2.5.0", default-features = false }
disqualified = { version = "1.0", default-features = false }
fixedbitset = { version = "0.5", default-features = false }
serde = { version = "1", default-features = false, features = [
Expand All @@ -133,6 +132,7 @@ tracing = { version = "0.1", default-features = false, optional = true }
log = { version = "0.4", default-features = false }
bumpalo = "3"

concurrent-queue = { version = "2.5.0", default-features = false }
[target.'cfg(not(all(target_has_atomic = "8", target_has_atomic = "16", target_has_atomic = "32", target_has_atomic = "64", target_has_atomic = "ptr")))'.dependencies]
concurrent-queue = { version = "2.5.0", default-features = false, features = [
"portable-atomic",
Expand Down
159 changes: 110 additions & 49 deletions crates/bevy_ecs/src/component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1175,8 +1175,9 @@ impl ComponentCloneBehavior {

/// A queued component registration.
struct QueuedRegistration {
registrator: Box<dyn FnOnce(&mut ComponentsRegistrator, ComponentId)>,
registrator: Box<dyn FnOnce(&mut ComponentsRegistrator, ComponentId, ComponentDescriptor)>,
id: ComponentId,
descriptor: ComponentDescriptor,
}

impl QueuedRegistration {
Expand All @@ -1187,17 +1188,19 @@ impl QueuedRegistration {
/// [`ComponentId`] must be unique.
unsafe fn new(
id: ComponentId,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId) + 'static,
descriptor: ComponentDescriptor,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId, ComponentDescriptor) + 'static,
) -> Self {
Self {
registrator: Box::new(func),
id,
descriptor,
}
}

/// Performs the registration, returning the now valid [`ComponentId`].
fn register(self, registrator: &mut ComponentsRegistrator) -> ComponentId {
(self.registrator)(registrator, self.id);
(self.registrator)(registrator, self.id, self.descriptor);
self.id
}
}
Expand Down Expand Up @@ -1294,6 +1297,7 @@ impl ComponentIds {
///
/// As a rule of thumb, if you have mutable access to [`ComponentsRegistrator`], prefer to use that instead.
/// Use this only if you need to know the id of a component but do not need to modify the contents of the world based on that id.
#[derive(Clone, Copy)]
pub struct ComponentsQueuedRegistrator<'w> {
components: &'w Components,
ids: &'w ComponentIds,
Expand Down Expand Up @@ -1326,7 +1330,8 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
unsafe fn force_register_arbitrary_component(
&self,
type_id: TypeId,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId) + 'static,
descriptor: ComponentDescriptor,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId, ComponentDescriptor) + 'static,
) -> ComponentId {
let id = self.ids.next();
self.components
Expand All @@ -1337,7 +1342,7 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
.insert(
type_id,
// SAFETY: The id was just generated.
unsafe { QueuedRegistration::new(id, func) },
unsafe { QueuedRegistration::new(id, descriptor, func) },
);
id
}
Expand All @@ -1350,7 +1355,8 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
unsafe fn force_register_arbitrary_resource(
&self,
type_id: TypeId,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId) + 'static,
descriptor: ComponentDescriptor,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId, ComponentDescriptor) + 'static,
) -> ComponentId {
let id = self.ids.next();
self.components
Expand All @@ -1361,15 +1367,16 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
.insert(
type_id,
// SAFETY: The id was just generated.
unsafe { QueuedRegistration::new(id, func) },
unsafe { QueuedRegistration::new(id, descriptor, func) },
);
id
}

/// Queues this function to run as a dynamic registrator.
fn force_register_arbitrary_dynamic(
&self,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId) + 'static,
descriptor: ComponentDescriptor,
func: impl FnOnce(&mut ComponentsRegistrator, ComponentId, ComponentDescriptor) + 'static,
) -> ComponentId {
let id = self.ids.next();
self.components
Expand All @@ -1379,7 +1386,7 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
.dynamic_registrations
.push(
// SAFETY: The id was just generated.
unsafe { QueuedRegistration::new(id, func) },
unsafe { QueuedRegistration::new(id, descriptor, func) },
);
id
}
Expand All @@ -1388,6 +1395,8 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
/// This will reserve an id and queue the registration.
/// These registrations will be carried out at the next opportunity.
///
/// If this has already been registered or queued, this returns the previous [`ComponentId`].
///
/// # Note
///
/// Technically speaking, the returned [`ComponentId`] is not valid, but it will become valid later.
Expand All @@ -1397,13 +1406,17 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
self.component_id::<T>().unwrap_or_else(|| {
// SAFETY: We just checked that this type was not in the queue.
unsafe {
self.force_register_arbitrary_component(TypeId::of::<T>(), |registrator, id| {
// SAFETY: We just checked that this is not currently registered or queued, and if it was registered since, this would have been dropped from the queue.
#[expect(unused_unsafe, reason = "More precise to specify.")]
unsafe {
registrator.register_component_unchecked::<T>(&mut Vec::new(), id);
}
})
self.force_register_arbitrary_component(
TypeId::of::<T>(),
ComponentDescriptor::new::<T>(),
|registrator, id, _descriptor| {
// SAFETY: We just checked that this is not currently registered or queued, and if it was registered since, this would have been dropped from the queue.
#[expect(unused_unsafe, reason = "More precise to specify.")]
unsafe {
registrator.register_component_unchecked::<T>(&mut Vec::new(), id);
}
},
)
}
})
}
Expand All @@ -1421,7 +1434,7 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
&self,
descriptor: ComponentDescriptor,
) -> ComponentId {
self.force_register_arbitrary_dynamic(|registrator, id| {
self.force_register_arbitrary_dynamic(descriptor, |registrator, id, descriptor| {
// SAFETY: Id uniqueness handled by caller.
unsafe {
registrator.register_component_inner(id, descriptor);
Expand All @@ -1433,6 +1446,8 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
/// This will reserve an id and queue the registration.
/// These registrations will be carried out at the next opportunity.
///
/// If this has already been registered or queued, this returns the previous [`ComponentId`].
///
/// # Note
///
/// Technically speaking, the returned [`ComponentId`] is not valid, but it will become valid later.
Expand All @@ -1443,16 +1458,18 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
self.get_resource_id(type_id).unwrap_or_else(|| {
// SAFETY: We just checked that this type was not in the queue.
unsafe {
self.force_register_arbitrary_resource(type_id, move |registrator, id| {
// SAFETY: We just checked that this is not currently registered or queued, and if it was registered since, this would have been dropped from the queue.
// SAFETY: Id uniqueness handled by caller, and the type_id matches descriptor.
#[expect(unused_unsafe, reason = "More precise to specify.")]
unsafe {
registrator.register_resource_unchecked_with(type_id, id, || {
ComponentDescriptor::new_resource::<T>()
});
}
})
self.force_register_arbitrary_resource(
type_id,
ComponentDescriptor::new_resource::<T>(),
move |registrator, id, descriptor| {
// SAFETY: We just checked that this is not currently registered or queued, and if it was registered since, this would have been dropped from the queue.
// SAFETY: Id uniqueness handled by caller, and the type_id matches descriptor.
#[expect(unused_unsafe, reason = "More precise to specify.")]
unsafe {
registrator.register_resource_unchecked(type_id, id, descriptor);
}
},
)
}
})
}
Expand All @@ -1461,6 +1478,8 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
/// This will reserve an id and queue the registration.
/// These registrations will be carried out at the next opportunity.
///
/// If this has already been registered or queued, this returns the previous [`ComponentId`].
///
/// # Note
///
/// Technically speaking, the returned [`ComponentId`] is not valid, but it will become valid later.
Expand All @@ -1471,16 +1490,18 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
self.get_resource_id(type_id).unwrap_or_else(|| {
// SAFETY: We just checked that this type was not in the queue.
unsafe {
self.force_register_arbitrary_resource(type_id, move |registrator, id| {
// SAFETY: We just checked that this is not currently registered or queued, and if it was registered since, this would have been dropped from the queue.
// SAFETY: Id uniqueness handled by caller, and the type_id matches descriptor.
#[expect(unused_unsafe, reason = "More precise to specify.")]
unsafe {
registrator.register_resource_unchecked_with(type_id, id, || {
ComponentDescriptor::new_non_send::<T>(StorageType::default())
});
}
})
self.force_register_arbitrary_resource(
type_id,
ComponentDescriptor::new_non_send::<T>(StorageType::default()),
move |registrator, id, descriptor| {
// SAFETY: We just checked that this is not currently registered or queued, and if it was registered since, this would have been dropped from the queue.
// SAFETY: Id uniqueness handled by caller, and the type_id matches descriptor.
#[expect(unused_unsafe, reason = "More precise to specify.")]
unsafe {
registrator.register_resource_unchecked(type_id, id, descriptor);
}
},
)
}
})
}
Expand All @@ -1498,7 +1519,7 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
&self,
descriptor: ComponentDescriptor,
) -> ComponentId {
self.force_register_arbitrary_dynamic(|registrator, id| {
self.force_register_arbitrary_dynamic(descriptor, |registrator, id, descriptor| {
// SAFETY: Id uniqueness handled by caller.
unsafe {
registrator.register_component_inner(id, descriptor);
Expand Down Expand Up @@ -1802,7 +1823,7 @@ impl<'w> ComponentsRegistrator<'w> {
}
}

/// Same as [`Components::register_resource_unchecked_with`] but handles safety.
/// Same as [`Components::register_resource_unchecked`] but handles safety.
///
/// # Safety
///
Expand Down Expand Up @@ -1833,7 +1854,7 @@ impl<'w> ComponentsRegistrator<'w> {
let id = self.ids.next_mut();
// SAFETY: The resource is not currently registered, the id is fresh, and the [`ComponentDescriptor`] matches the [`TypeId`]
unsafe {
self.register_resource_unchecked_with(type_id, id, descriptor);
self.register_resource_unchecked(type_id, id, descriptor());
}
id
}
Expand Down Expand Up @@ -1959,13 +1980,53 @@ impl Components {
self.components.get(id.0).and_then(|info| info.as_ref())
}

/// Returns the name associated with the given component, if it is registered.
/// This will return `None` if the id is not regiserted or is queued.
/// Gets the [`ComponentDescriptor`] of the component with this [`ComponentId`] if it is present.
/// This will return `None` only if the id is neither regisered nor queued to be registered.
///
/// Currently, the [`Cow`] will be [`Cow::Owned`] if and only if the component is queued. It will be [`Cow::Borrowed`] otherwise.
///
/// This will return an incorrect result if `id` did not come from the same world as `self`. It may return `None` or a garbage value.
#[inline]
pub fn get_name(&self, id: ComponentId) -> Option<&str> {
self.get_info(id).map(ComponentInfo::name)
pub fn get_descriptor<'a>(&'a self, id: ComponentId) -> Option<Cow<'a, ComponentDescriptor>> {
self.components
.get(id.0)
.and_then(|info| info.as_ref().map(|info| Cow::Borrowed(&info.descriptor)))
.or_else(|| {
let queued = self.queued.read().unwrap_or_else(PoisonError::into_inner);
// first check components, then resources, then dynamic
queued
.components
.values()
.chain(queued.resources.values())
.chain(queued.dynamic_registrations.iter())
.find(|queued| queued.id == id)
Copy link
Contributor

Choose a reason for hiding this comment

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

The linear search here looks expensive. Instead of putting the descriptors in the individual queues, would it make sense to have a separate HashMap<ComponentId, Arc<ComponentDescriptor>>? For that matter, the ComponentIds are dense, so I think it could be Vec<Arc<ComponentDescriptor>>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the linear search is not ideal, but this is a cold path. Ids are almost always correct unless this is in a inter-world context, and queued components aren't queued for long. I genuinely think it would be slower to cache it's location somewhere or change the backing data structure because that slows everything down by a tiny bit, where as this slows almost nothing down (but by a lot).

That's why I did it this way (even though it hurts my eyes too). But if the consensus is different, I can absolutely do otherwise.

.map(|queued| Cow::Owned(queued.descriptor.clone()))
})
}

/// Gets the name of the component with this [`ComponentId`] if it is present.
/// This will return `None` only if the id is neither regisered nor queued to be registered.
///
/// This will return an incorrect result if `id` did not come from the same world as `self`. It may return `None` or a garbage value.
#[inline]
pub fn get_name<'a>(&'a self, id: ComponentId) -> Option<Cow<'a, str>> {
self.components
.get(id.0)
.and_then(|info| {
info.as_ref()
.map(|info| Cow::Borrowed(info.descriptor.name()))
})
.or_else(|| {
let queued = self.queued.read().unwrap_or_else(PoisonError::into_inner);
// first check components, then resources, then dynamic
queued
.components
.values()
.chain(queued.resources.values())
.chain(queued.dynamic_registrations.iter())
.find(|queued| queued.id == id)
.map(|queued| queued.descriptor.name.clone())
})
}

/// Gets the metadata associated with the given component.
Expand Down Expand Up @@ -2388,15 +2449,15 @@ impl Components {
/// The [`ComponentId`] must be unique.
/// The [`TypeId`] and [`ComponentId`] must not be registered or queued.
#[inline]
unsafe fn register_resource_unchecked_with(
unsafe fn register_resource_unchecked(
&mut self,
type_id: TypeId,
component_id: ComponentId,
func: impl FnOnce() -> ComponentDescriptor,
descriptor: ComponentDescriptor,
) {
// SAFETY: ensured by caller
unsafe {
self.register_component_inner(component_id, func());
self.register_component_inner(component_id, descriptor);
}
let prev = self.resource_indices.insert(type_id, component_id);
debug_assert!(prev.is_none());
Expand Down Expand Up @@ -2872,13 +2933,13 @@ pub fn enforce_no_required_components_recursion(
"Recursive required components detected: {}\nhelp: {}",
recursion_check_stack
.iter()
.map(|id| format!("{}", ShortName(components.get_name(*id).unwrap())))
.map(|id| format!("{}", ShortName(&components.get_name(*id).unwrap())))
.collect::<Vec<_>>()
.join(" → "),
if direct_recursion {
format!(
"Remove require({}).",
ShortName(components.get_name(requiree).unwrap())
ShortName(&components.get_name(requiree).unwrap())
)
} else {
"If this is intentional, consider merging the components.".into()
Expand Down
5 changes: 2 additions & 3 deletions crates/bevy_ecs/src/query/access.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,11 +968,10 @@ impl AccessConflicts {
format!(
"{}",
ShortName(
world
&world
.components
.get_info(ComponentId::get_sparse_set_index(index))
.get_name(ComponentId::get_sparse_set_index(index))
.unwrap()
.name()
)
)
})
Expand Down
Loading