Skip to content
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.

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

Use UnsafeWorldCell to increase code quality for SystemParam #8174

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
ParamSet {
Expand Down Expand Up @@ -455,7 +455,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &#path::system::SystemMeta,
world: &'w #path::world::World,
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
change_tick: #path::component::Tick,
) -> Self::Item<'w, 's> {
let (#(#tuple_patterns,)*) = <
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/removal_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
prelude::Local,
storage::SparseSet,
system::{ReadOnlySystemParam, SystemMeta, SystemParam},
world::World,
world::{unsafe_world_cell::UnsafeWorldCell, World},
};

use std::{
Expand Down Expand Up @@ -264,9 +264,9 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentEvents {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.removed_components()
world.world_metadata().removed_components()
}
}
9 changes: 7 additions & 2 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,12 @@ impl<Param: SystemParam> SystemState<Param> {
world: &'w World,
change_tick: Tick,
) -> SystemParamItem<'w, 's, Param> {
let param = Param::get_param(&mut self.param_state, &self.meta, world, change_tick);
let param = Param::get_param(
&mut self.param_state,
&self.meta,
world.as_unsafe_world_cell_migration_internal(),
change_tick,
);
self.meta.last_run = change_tick;
param
}
Expand Down Expand Up @@ -481,7 +486,7 @@ where
let params = F::Param::get_param(
self.param_state.as_mut().expect(Self::PARAM_MESSAGE),
&self.system_meta,
world,
world.as_unsafe_world_cell_migration_internal(),
change_tick,
);
let out = self.func.run(input, params);
Expand Down
75 changes: 38 additions & 37 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
Access, FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyWorldQuery, WorldQuery,
},
system::{Query, SystemMeta},
world::{FromWorld, World},
world::{unsafe_world_cell::UnsafeWorldCell, FromWorld, World},
};
use bevy_ecs_macros::impl_param_set;
pub use bevy_ecs_macros::Resource;
Expand Down Expand Up @@ -135,14 +135,13 @@ pub unsafe trait SystemParam: Sized {

/// # Safety
///
/// This call might use any of the [`World`] accesses that were registered in [`Self::init_state`].
/// - None of those accesses may conflict with any other [`SystemParam`]s
/// that exist at the same time, including those on other threads.
/// - The passed [`UnsafeWorldCell`] must have access to any world data
/// registered in [`init_state`](SystemParam::init_state).
/// - `world` must be the same `World` that was used to initialize [`state`](SystemParam::init_state).
unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State,
system_meta: &SystemMeta,
world: &'world World,
world: UnsafeWorldCell<'world>,
change_tick: Tick,
) -> Self::Item<'world, 'state>;
}
Expand Down Expand Up @@ -200,10 +199,19 @@ unsafe impl<Q: WorldQuery + 'static, F: ReadOnlyWorldQuery + 'static> SystemPara
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
Query::new(world, state, system_meta.last_run, change_tick, false)
Query::new(
// SAFETY: We have registered all of the query's world accesses,
// so the caller ensures that `world` has permission to access any
// world data that the query needs.
world.unsafe_world(),
state,
system_meta.last_run,
change_tick,
false,
)
}
}

Expand Down Expand Up @@ -337,7 +345,7 @@ fn assert_component_access_compatibility(
/// ```
pub struct ParamSet<'w, 's, T: SystemParam> {
param_states: &'s mut T::State,
world: &'w World,
world: UnsafeWorldCell<'w>,
system_meta: SystemMeta,
change_tick: Tick,
}
Expand Down Expand Up @@ -443,11 +451,10 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let (ptr, ticks) = world
.as_unsafe_world_cell_migration_internal()
.get_resource_with_ticks(component_id)
.unwrap_or_else(|| {
panic!(
Expand Down Expand Up @@ -484,11 +491,10 @@ unsafe impl<'a, T: Resource> SystemParam for Option<Res<'a, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_resource_with_ticks(component_id)
.map(|(ptr, ticks)| Res {
value: ptr.deref(),
Expand Down Expand Up @@ -538,11 +544,10 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let value = world
.as_unsafe_world_cell_migration_internal()
.get_resource_mut_by_id(component_id)
.unwrap_or_else(|| {
panic!(
Expand Down Expand Up @@ -576,11 +581,10 @@ unsafe impl<'a, T: Resource> SystemParam for Option<ResMut<'a, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_resource_mut_by_id(component_id)
.map(|value| ResMut {
value: value.value.deref_mut::<T>(),
Expand Down Expand Up @@ -629,10 +633,11 @@ unsafe impl SystemParam for &'_ World {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world
// SAFETY: Read-only access to the entire world was registerd in `init_state`.
world.world()
}
}

Expand Down Expand Up @@ -750,7 +755,7 @@ unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
Local(state.get())
Expand Down Expand Up @@ -924,7 +929,7 @@ unsafe impl<T: SystemBuffer> SystemParam for Deferred<'_, T> {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
Deferred(state.get())
Expand Down Expand Up @@ -1030,11 +1035,10 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let (ptr, ticks) = world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.unwrap_or_else(|| {
panic!(
Expand Down Expand Up @@ -1069,11 +1073,10 @@ unsafe impl<T: 'static> SystemParam for Option<NonSend<'_, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.map(|(ptr, ticks)| NonSend {
value: ptr.deref(),
Expand Down Expand Up @@ -1122,11 +1125,10 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let (ptr, ticks) = world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.unwrap_or_else(|| {
panic!(
Expand Down Expand Up @@ -1155,11 +1157,10 @@ unsafe impl<'a, T: 'static> SystemParam for Option<NonSendMut<'a, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.map(|(ptr, ticks)| NonSendMut {
value: ptr.assert_unique().deref_mut(),
Expand All @@ -1182,7 +1183,7 @@ unsafe impl<'a> SystemParam for &'a Archetypes {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.archetypes()
Expand All @@ -1203,7 +1204,7 @@ unsafe impl<'a> SystemParam for &'a Components {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.components()
Expand All @@ -1224,7 +1225,7 @@ unsafe impl<'a> SystemParam for &'a Entities {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.entities()
Expand All @@ -1245,7 +1246,7 @@ unsafe impl<'a> SystemParam for &'a Bundles {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.bundles()
Expand Down Expand Up @@ -1294,7 +1295,7 @@ unsafe impl SystemParam for SystemChangeTick {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
SystemChangeTick {
Expand Down Expand Up @@ -1364,7 +1365,7 @@ unsafe impl SystemParam for SystemName<'_> {
unsafe fn get_param<'w, 's>(
name: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
SystemName { name }
Expand Down Expand Up @@ -1406,7 +1407,7 @@ macro_rules! impl_system_param_tuple {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {

Expand Down Expand Up @@ -1529,7 +1530,7 @@ unsafe impl<P: SystemParam + 'static> SystemParam for StaticSystemParam<'_, '_,
unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State,
system_meta: &SystemMeta,
world: &'world World,
world: UnsafeWorldCell<'world>,
change_tick: Tick,
) -> Self::Item<'world, 'state> {
// SAFETY: Defer to the safety of P::SystemParam
Expand All @@ -1547,7 +1548,7 @@ unsafe impl<T: ?Sized> SystemParam for PhantomData<T> {
unsafe fn get_param<'world, 'state>(
_state: &'state mut Self::State,
_system_meta: &SystemMeta,
_world: &'world World,
_world: UnsafeWorldCell<'world>,
_change_tick: Tick,
) -> Self::Item<'world, 'state> {
PhantomData
Expand Down
6 changes: 4 additions & 2 deletions crates/bevy_ecs/src/world/identifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ use crate::{
};
use std::sync::atomic::{AtomicUsize, Ordering};

use super::unsafe_world_cell::UnsafeWorldCell;

#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
// We use usize here because that is the largest `Atomic` we want to require
/// A unique identifier for a [`World`].
Expand Down Expand Up @@ -56,10 +58,10 @@ unsafe impl SystemParam for WorldId {
unsafe fn get_param<'world, 'state>(
_: &'state mut Self::State,
_: &crate::system::SystemMeta,
world: &'world super::World,
world: UnsafeWorldCell<'world>,
_: Tick,
) -> Self::Item<'world, 'state> {
world.id
world.world_metadata().id()
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/world/unsafe_world_cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// # Safety
/// - must not be used in a way that would conflict with any
/// live exclusive borrows on world data
unsafe fn unsafe_world(self) -> &'w World {
pub(crate) unsafe fn unsafe_world(self) -> &'w World {
// SAFETY:
// - caller ensures that the returned `&World` is not used in a way that would conflict
// with any existing mutable borrows of world data
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_render/src/extract_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use bevy_ecs::{
component::Tick,
prelude::*,
system::{ReadOnlySystemParam, SystemMeta, SystemParam, SystemParamItem, SystemState},
world::unsafe_world_cell::UnsafeWorldCell,
};
use std::ops::{Deref, DerefMut};

Expand Down Expand Up @@ -76,7 +77,7 @@ where
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
// SAFETY:
Expand Down