forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Only get valid component ids (#19510)

Browse Source 
# Objective

- #19504 showed a 11x regression in getting component values for
unregistered components. This pr should fix that and improve others a
little too.
- This is some cleanup work from #18173 .

## Solution

- Whenever we expect a component value to exist, we only care about
fully registered components, not queued to be registered components
since, for the value to exist, it must be registered.
- So we can use the faster `get_valid_*` instead of `get_*` in a lot of
places.
- Also found a bug where `valid_*` did not forward to `get_valid_*`
properly. That's fixed.

## Testing

CI
This commit is contained in:
Eagster 2025-06-06 16:59:57 -04:00 committed by GitHub
parent 7ac2ae5713
commit 8ad7118443
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 70 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
crates/bevy_ecs/src/component.rs
View File

@ -2400,7 +2400,7 @@ impl Components {
/// * [`World::component_id()`]
#[inline]
pub fn valid_component_id<T: Component>(&self) -> Option<ComponentId> {
self.get_id(TypeId::of::<T>())
self.get_valid_id(TypeId::of::<T>())
}
/// Type-erased equivalent of [`Components::valid_resource_id()`].
@ -2431,7 +2431,7 @@ impl Components {
/// * [`Components::get_resource_id()`]
#[inline]
pub fn valid_resource_id<T: Resource>(&self) -> Option<ComponentId> {
self.get_resource_id(TypeId::of::<T>())
self.get_valid_resource_id(TypeId::of::<T>())
}
/// Type-erased equivalent of [`Components::component_id()`].

8
crates/bevy_ecs/src/entity/clone_entities.rs
View File

@ -705,7 +705,7 @@ impl<'w> EntityClonerBuilder<'w> {
/// [`deny_all`](`Self::deny_all`) before calling any of the `allow` methods.
pub fn allow_by_type_ids(&mut self, ids: impl IntoIterator<Item = TypeId>) -> &mut Self {
for type_id in ids {
if let Some(id) = self.world.components().get_id(type_id) {
if let Some(id) = self.world.components().get_valid_id(type_id) {
self.filter_allow(id);
}
}
@ -740,7 +740,7 @@ impl<'w> EntityClonerBuilder<'w> {
/// Extends the list of components that shouldn't be cloned by type ids.
pub fn deny_by_type_ids(&mut self, ids: impl IntoIterator<Item = TypeId>) -> &mut Self {
for type_id in ids {
if let Some(id) = self.world.components().get_id(type_id) {
if let Some(id) = self.world.components().get_valid_id(type_id) {
self.filter_deny(id);
}
}
@ -762,7 +762,7 @@ impl<'w> EntityClonerBuilder<'w> {
&mut self,
clone_behavior: ComponentCloneBehavior,
) -> &mut Self {
if let Some(id) = self.world.components().component_id::<T>() {
if let Some(id) = self.world.components().valid_component_id::<T>() {
self.entity_cloner
.clone_behavior_overrides
.insert(id, clone_behavior);
@ -787,7 +787,7 @@ impl<'w> EntityClonerBuilder<'w> {
/// Removes a previously set override of [`ComponentCloneBehavior`] for a component in this builder.
pub fn remove_clone_behavior_override<T: Component>(&mut self) -> &mut Self {
if let Some(id) = self.world.components().component_id::<T>() {
if let Some(id) = self.world.components().valid_component_id::<T>() {
self.entity_cloner.clone_behavior_overrides.remove(&id);
}
self

46
crates/bevy_ecs/src/world/entity_ref.rs
View File

@ -3279,7 +3279,11 @@ impl<'w> FilteredEntityRef<'w> {
/// Returns `None` if the entity does not have a component of type `T`.
#[inline]
pub fn get<T: Component>(&self) -> Option<&'w T> {
let id = self.entity.world().components().get_id(TypeId::of::<T>())?;
let id = self
.entity
.world()
.components()
.get_valid_id(TypeId::of::<T>())?;
self.access
.has_component_read(id)
// SAFETY: We have read access
@ -3293,7 +3297,11 @@ impl<'w> FilteredEntityRef<'w> {
/// Returns `None` if the entity does not have a component of type `T`.
#[inline]
pub fn get_ref<T: Component>(&self) -> Option<Ref<'w, T>> {
let id = self.entity.world().components().get_id(TypeId::of::<T>())?;
let id = self
.entity
.world()
.components()
.get_valid_id(TypeId::of::<T>())?;
self.access
.has_component_read(id)
// SAFETY: We have read access
@ -3305,7 +3313,11 @@ impl<'w> FilteredEntityRef<'w> {
/// detection in custom runtimes.
#[inline]
pub fn get_change_ticks<T: Component>(&self) -> Option<ComponentTicks> {
let id = self.entity.world().components().get_id(TypeId::of::<T>())?;
let id = self
.entity
.world()
.components()
.get_valid_id(TypeId::of::<T>())?;
self.access
.has_component_read(id)
// SAFETY: We have read access
@ -3637,7 +3649,11 @@ impl<'w> FilteredEntityMut<'w> {
/// Returns `None` if the entity does not have a component of type `T`.
#[inline]
pub fn get_mut<T: Component<Mutability = Mutable>>(&mut self) -> Option<Mut<'_, T>> {
let id = self.entity.world().components().get_id(TypeId::of::<T>())?;
let id = self
.entity
.world()
.components()
.get_valid_id(TypeId::of::<T>())?;
self.access
.has_component_write(id)
// SAFETY: We have write access
@ -3665,7 +3681,11 @@ impl<'w> FilteredEntityMut<'w> {
/// - `T` must be a mutable component
#[inline]
pub unsafe fn into_mut_assume_mutable<T: Component>(self) -> Option<Mut<'w, T>> {
let id = self.entity.world().components().get_id(TypeId::of::<T>())?;
let id = self
.entity
.world()
.components()
.get_valid_id(TypeId::of::<T>())?;
self.access
.has_component_write(id)
// SAFETY:
@ -3895,7 +3915,7 @@ where
C: Component,
{
let components = self.entity.world().components();
let id = components.component_id::<C>()?;
let id = components.valid_component_id::<C>()?;
if bundle_contains_component::<B>(components, id) {
None
} else {
@ -3915,7 +3935,7 @@ where
C: Component,
{
let components = self.entity.world().components();
let id = components.component_id::<C>()?;
let id = components.valid_component_id::<C>()?;
if bundle_contains_component::<B>(components, id) {
None
} else {
@ -3995,7 +4015,11 @@ where
/// detection in custom runtimes.
#[inline]
pub fn get_change_ticks<T: Component>(&self) -> Option<ComponentTicks> {
let component_id = self.entity.world().components().get_id(TypeId::of::<T>())?;
let component_id = self
.entity
.world()
.components()
.get_valid_id(TypeId::of::<T>())?;
let components = self.entity.world().components();
(!bundle_contains_component::<B>(components, component_id))
.then(|| {
@ -4164,7 +4188,7 @@ where
C: Component<Mutability = Mutable>,
{
let components = self.entity.world().components();
let id = components.component_id::<C>()?;
let id = components.valid_component_id::<C>()?;
if bundle_contains_component::<B>(components, id) {
None
} else {
@ -4747,7 +4771,7 @@ mod tests {
let entity = world.spawn(TestComponent(42)).id();
let component_id = world
.components()
.get_id(core::any::TypeId::of::<TestComponent>())
.get_valid_id(core::any::TypeId::of::<TestComponent>())
.unwrap();
let entity = world.entity(entity);
@ -4764,7 +4788,7 @@ mod tests {
let entity = world.spawn(TestComponent(42)).id();
let component_id = world
.components()
.get_id(core::any::TypeId::of::<TestComponent>())
.get_valid_id(core::any::TypeId::of::<TestComponent>())
.unwrap();
let mut entity_mut = world.entity_mut(entity);

4
crates/bevy_ecs/src/world/filtered_resource.rs
View File

@ -157,7 +157,7 @@ impl<'w, 's> FilteredResources<'w, 's> {
let component_id = self
.world
.components()
.resource_id::<R>()
.valid_resource_id::<R>()
.ok_or(ResourceFetchError::NotRegistered)?;
if !self.access.has_resource_read(component_id) {
return Err(ResourceFetchError::NoResourceAccess(component_id));
@ -474,7 +474,7 @@ impl<'w, 's> FilteredResourcesMut<'w, 's> {
let component_id = self
.world
.components()
.resource_id::<R>()
.valid_resource_id::<R>()
.ok_or(ResourceFetchError::NotRegistered)?;
// SAFETY: THe caller ensures that there are no conflicting borrows.
unsafe { self.get_mut_by_id_unchecked(component_id) }

24
crates/bevy_ecs/src/world/mod.rs
View File

@ -531,7 +531,7 @@ impl World {
/// Retrieves the [required components](RequiredComponents) for the given component type, if it exists.
pub fn get_required_components<C: Component>(&self) -> Option<&RequiredComponents> {
let id = self.components().component_id::<C>()?;
let id = self.components().valid_component_id::<C>()?;
let component_info = self.components().get_info(id)?;
Some(component_info.required_components())
}
@ -1623,7 +1623,7 @@ impl World {
/// since the last call to [`World::clear_trackers`].
pub fn removed<T: Component>(&self) -> impl Iterator<Item = Entity> + '_ {
self.components
.get_id(TypeId::of::<T>())
.get_valid_id(TypeId::of::<T>())
.map(|component_id| self.removed_with_id(component_id))
.into_iter()
.flatten()
@ -1772,7 +1772,7 @@ impl World {
/// Removes the resource of a given type and returns it, if it exists. Otherwise returns `None`.
#[inline]
pub fn remove_resource<R: Resource>(&mut self) -> Option<R> {
let component_id = self.components.get_resource_id(TypeId::of::<R>())?;
let component_id = self.components.get_valid_resource_id(TypeId::of::<R>())?;
let (ptr, _, _) = self.storages.resources.get_mut(component_id)?.remove()?;
// SAFETY: `component_id` was gotten via looking up the `R` type
unsafe { Some(ptr.read::<R>()) }
@ -1791,7 +1791,7 @@ impl World {
/// thread than where the value was inserted from.
#[inline]
pub fn remove_non_send_resource<R: 'static>(&mut self) -> Option<R> {
let component_id = self.components.get_resource_id(TypeId::of::<R>())?;
let component_id = self.components.get_valid_resource_id(TypeId::of::<R>())?;
let (ptr, _, _) = self
.storages
.non_send_resources
@ -1805,7 +1805,7 @@ impl World {
#[inline]
pub fn contains_resource<R: Resource>(&self) -> bool {
self.components
.get_resource_id(TypeId::of::<R>())
.get_valid_resource_id(TypeId::of::<R>())
.and_then(|component_id| self.storages.resources.get(component_id))
.is_some_and(ResourceData::is_present)
}
@ -1823,7 +1823,7 @@ impl World {
#[inline]
pub fn contains_non_send<R: 'static>(&self) -> bool {
self.components
.get_resource_id(TypeId::of::<R>())
.get_valid_resource_id(TypeId::of::<R>())
.and_then(|component_id| self.storages.non_send_resources.get(component_id))
.is_some_and(ResourceData::is_present)
}
@ -1846,7 +1846,7 @@ impl World {
/// was called.
pub fn is_resource_added<R: Resource>(&self) -> bool {
self.components
.get_resource_id(TypeId::of::<R>())
.get_valid_resource_id(TypeId::of::<R>())
.is_some_and(|component_id| self.is_resource_added_by_id(component_id))
}
@ -1877,7 +1877,7 @@ impl World {
/// was called.
pub fn is_resource_changed<R: Resource>(&self) -> bool {
self.components
.get_resource_id(TypeId::of::<R>())
.get_valid_resource_id(TypeId::of::<R>())
.is_some_and(|component_id| self.is_resource_changed_by_id(component_id))
}
@ -1902,7 +1902,7 @@ impl World {
/// Retrieves the change ticks for the given resource.
pub fn get_resource_change_ticks<R: Resource>(&self) -> Option<ComponentTicks> {
self.components
.get_resource_id(TypeId::of::<R>())
.get_valid_resource_id(TypeId::of::<R>())
.and_then(|component_id| self.get_resource_change_ticks_by_id(component_id))
}
@ -2558,7 +2558,7 @@ impl World {
let last_change_tick = self.last_change_tick();
let change_tick = self.change_tick();
let component_id = self.components.get_resource_id(TypeId::of::<R>())?;
let component_id = self.components.get_valid_resource_id(TypeId::of::<R>())?;
let (ptr, mut ticks, mut caller) = self
.storages
.resources
@ -3750,7 +3750,7 @@ mod tests {
world.insert_resource(TestResource(42));
let component_id = world
.components()
.get_resource_id(TypeId::of::<TestResource>())
.get_valid_resource_id(TypeId::of::<TestResource>())
.unwrap();
let resource = world.get_resource_by_id(component_id).unwrap();
@ -3766,7 +3766,7 @@ mod tests {
world.insert_resource(TestResource(42));
let component_id = world
.components()
.get_resource_id(TypeId::of::<TestResource>())
.get_valid_resource_id(TypeId::of::<TestResource>())
.unwrap();
{

4
crates/bevy_ecs/src/world/reflect.rs
View File

@ -70,7 +70,7 @@ impl World {
entity: Entity,
type_id: TypeId,
) -> Result<&dyn Reflect, GetComponentReflectError> {
let Some(component_id) = self.components().get_id(type_id) else {
let Some(component_id) = self.components().get_valid_id(type_id) else {
return Err(GetComponentReflectError::NoCorrespondingComponentId(
type_id,
));
@ -158,7 +158,7 @@ impl World {
));
};
let Some(component_id) = self.components().get_id(type_id) else {
let Some(component_id) = self.components().get_valid_id(type_id) else {
return Err(GetComponentReflectError::NoCorrespondingComponentId(
type_id,
));

18
crates/bevy_ecs/src/world/unsafe_world_cell.rs
View File

@ -401,7 +401,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// - no mutable reference to the resource exists at the same time
#[inline]
pub unsafe fn get_resource<R: Resource>(self) -> Option<&'w R> {
let component_id = self.components().get_resource_id(TypeId::of::<R>())?;
let component_id = self.components().get_valid_resource_id(TypeId::of::<R>())?;
// SAFETY: caller ensures `self` has permission to access the resource
// caller also ensure that no mutable reference to the resource exists
unsafe {
@ -419,7 +419,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// - no mutable reference to the resource exists at the same time
#[inline]
pub unsafe fn get_resource_ref<R: Resource>(self) -> Option<Ref<'w, R>> {
let component_id = self.components().get_resource_id(TypeId::of::<R>())?;
let component_id = self.components().get_valid_resource_id(TypeId::of::<R>())?;
// SAFETY: caller ensures `self` has permission to access the resource
// caller also ensures that no mutable reference to the resource exists
@ -471,7 +471,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// - no mutable reference to the resource exists at the same time
#[inline]
pub unsafe fn get_non_send_resource<R: 'static>(self) -> Option<&'w R> {
let component_id = self.components().get_resource_id(TypeId::of::<R>())?;
let component_id = self.components().get_valid_resource_id(TypeId::of::<R>())?;
// SAFETY: caller ensures that `self` has permission to access `R`
// caller ensures that no mutable reference exists to `R`
unsafe {
@ -514,7 +514,7 @@ impl<'w> UnsafeWorldCell<'w> {
#[inline]
pub unsafe fn get_resource_mut<R: Resource>(self) -> Option<Mut<'w, R>> {
self.assert_allows_mutable_access();
let component_id = self.components().get_resource_id(TypeId::of::<R>())?;
let component_id = self.components().get_valid_resource_id(TypeId::of::<R>())?;
// SAFETY:
// - caller ensures `self` has permission to access the resource mutably
// - caller ensures no other references to the resource exist
@ -578,7 +578,7 @@ impl<'w> UnsafeWorldCell<'w> {
#[inline]
pub unsafe fn get_non_send_resource_mut<R: 'static>(self) -> Option<Mut<'w, R>> {
self.assert_allows_mutable_access();
let component_id = self.components().get_resource_id(TypeId::of::<R>())?;
let component_id = self.components().get_valid_resource_id(TypeId::of::<R>())?;
// SAFETY:
// - caller ensures that `self` has permission to access the resource
// - caller ensures that the resource is unaliased
@ -826,7 +826,7 @@ impl<'w> UnsafeEntityCell<'w> {
/// - no other mutable references to the component exist at the same time
#[inline]
pub unsafe fn get<T: Component>(self) -> Option<&'w T> {
let component_id = self.world.components().get_id(TypeId::of::<T>())?;
let component_id = self.world.components().get_valid_id(TypeId::of::<T>())?;
// SAFETY:
// - `storage_type` is correct (T component_id + T::STORAGE_TYPE)
// - `location` is valid
@ -852,7 +852,7 @@ impl<'w> UnsafeEntityCell<'w> {
pub unsafe fn get_ref<T: Component>(self) -> Option<Ref<'w, T>> {
let last_change_tick = self.last_run;
let change_tick = self.this_run;
let component_id = self.world.components().get_id(TypeId::of::<T>())?;
let component_id = self.world.components().get_valid_id(TypeId::of::<T>())?;
// SAFETY:
// - `storage_type` is correct (T component_id + T::STORAGE_TYPE)
@ -884,7 +884,7 @@ impl<'w> UnsafeEntityCell<'w> {
/// - no other mutable references to the component exist at the same time
#[inline]
pub unsafe fn get_change_ticks<T: Component>(self) -> Option<ComponentTicks> {
let component_id = self.world.components().get_id(TypeId::of::<T>())?;
let component_id = self.world.components().get_valid_id(TypeId::of::<T>())?;
// SAFETY:
// - entity location is valid
@ -968,7 +968,7 @@ impl<'w> UnsafeEntityCell<'w> {
) -> Option<Mut<'w, T>> {
self.world.assert_allows_mutable_access();
let component_id = self.world.components().get_id(TypeId::of::<T>())?;
let component_id = self.world.components().get_valid_id(TypeId::of::<T>())?;
// SAFETY:
// - `storage_type` is correct

5
crates/bevy_remote/src/builtin_methods.rs
View File

@ -570,7 +570,8 @@ pub fn process_remote_get_watching_request(
);
continue;
};
let Some(component_id) = world.components().get_id(type_registration.type_id()) else {
let Some(component_id) = world.components().get_valid_id(type_registration.type_id())
else {
let err = BrpError::component_error(format!("Unknown component: `{component_path}`"));
if strict {
return Err(err);
@ -1304,7 +1305,7 @@ fn get_component_ids(
let type_id = type_registration.type_id();
world
.components()
.get_id(type_id)
.get_valid_id(type_id)
.map(|component_id| (type_id, component_id))
});
if let Some((type_id, component_id)) = maybe_component_tuple {

2
crates/bevy_scene/src/dynamic_scene_builder.rs
View File

@ -350,7 +350,7 @@ impl<'w> DynamicSceneBuilder<'w> {
let original_world_dqf_id = self
.original_world
.components()
.get_resource_id(TypeId::of::<DefaultQueryFilters>());
.get_valid_resource_id(TypeId::of::<DefaultQueryFilters>());
let type_registry = self.original_world.resource::<AppTypeRegistry>().read();