diff --git a/assets/tests/add_system/adding_system_again_replaces_it.lua b/assets/tests/add_system/adding_system_again_replaces_it.lua new file mode 100644 index 0000000000..ae5f1e432e --- /dev/null +++ b/assets/tests/add_system/adding_system_again_replaces_it.lua @@ -0,0 +1,37 @@ +-- adding a system with the same callback label for the same script replaces the old one +local ScriptAttachment = import_namespace("bms").ScriptAttachment +local bms = import_namespace("bms") +local world = bms.world +runs = {} +local ResourceType = world.get_type_by_name("test_utils::test_data::TestResource") + +function on_test() + local post_update_schedule = world.get_schedule_by_name("PostUpdate") + local script_attachment = ScriptAttachment.new_entity_script(bms.current_entity(), bms.current_script()) + + world.add_system( + post_update_schedule, + bms.system_builder("my_replaced_system", script_attachment) + ) + world.add_system( + post_update_schedule, + bms.system_builder("my_replaced_system", script_attachment) + :resource(ResourceType) + ) + + return true +end + +function my_replaced_system(resource) + runs[#runs + 1] = resource +end + +function on_test_post_update() + return true +end + +function on_test_last() + assert(#runs == 1, "Expected 1 run, got: " .. #runs) + assert(runs[1] ~= nil, "Expected the replacement with the resource parameter to run") + return true +end diff --git a/assets/tests/add_system/system_cannot_access_component_outside_query_archetypes.lua b/assets/tests/add_system/system_cannot_access_component_outside_query_archetypes.lua new file mode 100644 index 0000000000..0ce3229e9b --- /dev/null +++ b/assets/tests/add_system/system_cannot_access_component_outside_query_archetypes.lua @@ -0,0 +1,64 @@ +-- bevy runs a system querying `A` with `B` in parallel with systems accessing `A` without `B`, +-- so the script system must not reach `A` on entities without `B`, even though it has access to `A` +local ScriptAttachment = import_namespace("bms").ScriptAttachment +local bms = import_namespace("bms") +local world = bms.world +runs = {} +local ComponentA = world.get_type_by_name("test_utils::test_data::CompWithFromWorldAndComponentData") +local ComponentB = world.get_type_by_name("test_utils::test_data::CompWithDefaultAndComponentData") + +function on_test() + local post_update_schedule = world.get_schedule_by_name("PostUpdate") + local script_attachment = ScriptAttachment.new_entity_script(bms.current_entity(), bms.current_script()) + + local matched = world.spawn() + world.add_default_component(matched, ComponentA) + world.add_default_component(matched, ComponentB) + + outsider = world.spawn() + world.add_default_component(outsider, ComponentA) + assert(world._archetype_of(matched) ~= world._archetype_of(outsider), + "the entities should live in different archetypes") + + world.add_system( + post_update_schedule, + bms.system_builder("my_filtered_system", script_attachment) + :query(world.query():component(ComponentA):with(ComponentB)) + ) + + return true +end + +function my_filtered_system(query) + runs[#runs + 1] = "my_filtered_system" + local ok, err = pcall(check_accesses, query) + runs[#runs + 1] = ok and "finished" or tostring(err) +end + +function check_accesses(query) + assert(#query == 1, "Expected 1 result, got: " .. #query) + local matched = query[1] + assert(matched:components()[1][1] == "Default", "the matched component should be accessible") + assert(world.get_component(matched:entity(), ComponentA)[1] == "Default", + "the matched component should be accessible through the world too") + + assert_throws(function() + local component = world.get_component(outsider, ComponentA) + local _ = component[1] + end, ".*not matched by any query.*") + + -- `with` filters by `B` without accessing it + assert_throws(function() + local component = world.get_component(matched:entity(), ComponentB) + local _ = component[1] + end, ".*annot claim access.*") +end + +function on_test_post_update() + return true +end + +function on_test_last() + assert(#runs == 2 and runs[2] == "finished", "the system did not finish, runs: " .. table.concat(runs, ",")) + return true +end diff --git a/assets/tests/add_system/system_with_parameters.lua b/assets/tests/add_system/system_with_parameters.lua index cc64871415..8bd83e9064 100644 --- a/assets/tests/add_system/system_with_parameters.lua +++ b/assets/tests/add_system/system_with_parameters.lua @@ -49,9 +49,11 @@ function my_parameterised_system(resourceA, query, resourceB) assert(#components == 2, "Expected 2 components, got " .. #components) local componentA = components[1] local componentB = components[2] - assert(componentA._1 == "Default", "Expected 'Default', got: " .. componentA._1) - assert(componentB._1 == "Default", "Expected 'Default', got: " .. componentA._1) + assert(componentA[1] == "Default", "Expected 'Default', got: " .. componentA[1]) + assert(componentB[1] == "Default", "Expected 'Default', got: " .. componentB[1]) end + -- errors inside systems are only logged, so record that we got to the end + runs[#runs + 1] = "finished" end function on_test_post_update() @@ -59,6 +61,6 @@ function on_test_post_update() end function on_test_last() - assert(#runs == 1, "Expected 1 runs, got: " .. #runs) + assert(#runs == 2 and runs[2] == "finished", "the system did not finish, runs: " .. table.concat(runs, ",")) return true end diff --git a/crates/bevy_mod_scripting_bindings/src/reference.rs b/crates/bevy_mod_scripting_bindings/src/reference.rs index 5e434e6895..27636856f7 100644 --- a/crates/bevy_mod_scripting_bindings/src/reference.rs +++ b/crates/bevy_mod_scripting_bindings/src/reference.rs @@ -484,6 +484,10 @@ impl ReflectReference { .get_type_data(self.base.type_id) .ok_or_else(|| InteropError::unregistered_base(self.base.clone()))?; + if let ReflectBase::Component(entity, component_id) = &self.base.base_id { + world.check_archetype_access(*entity, *component_id)?; + } + let ptr = unsafe { self.base .base_id @@ -556,6 +560,10 @@ impl ReflectReference { .get_type_data(self.base.type_id) .ok_or_else(|| InteropError::unregistered_base(self.base.clone()))?; + if let ReflectBase::Component(entity, component_id) = &self.base.base_id { + world.check_archetype_access(*entity, *component_id)?; + } + let ptr = unsafe { self.base .base_id diff --git a/crates/bevy_mod_scripting_core/src/handler.rs b/crates/bevy_mod_scripting_core/src/handler.rs index f22fee6909..63be3b3b6e 100644 --- a/crates/bevy_mod_scripting_core/src/handler.rs +++ b/crates/bevy_mod_scripting_core/src/handler.rs @@ -1126,6 +1126,7 @@ mod test { WorldAccessGuard::new_non_exclusive( world.as_unsafe_world_cell(), AccessByteSet::from_allowed_list(&[]), + None, type_registry, cache, ) diff --git a/crates/bevy_mod_scripting_core/src/lib.rs b/crates/bevy_mod_scripting_core/src/lib.rs index 9749218ba2..7d78ad9a3e 100644 --- a/crates/bevy_mod_scripting_core/src/lib.rs +++ b/crates/bevy_mod_scripting_core/src/lib.rs @@ -16,8 +16,9 @@ use crate::{ }, pipeline::ScriptLoadingPipeline, script::{ScriptComponentsChangeCache, script_component_changed_handler}, + script_system::{ScriptSystems, apply_script_system_changes}, }; -use bevy_app::{App, Last, Plugin, PostUpdate, PreUpdate}; +use bevy_app::{App, First, Last, Plugin, PostUpdate, PreUpdate}; use bevy_asset::{AssetApp, Handle}; use bevy_ecs::schedule::IntoScheduleConfigs; use bevy_ecs::{ @@ -392,6 +393,15 @@ impl Plugin for BMSScriptingInfrastructurePlugin { run_queued_callbacks_system.in_set(ScriptingSystemSet::RunQueuedCallbacks), ); + // changes to script systems in running schedules are applied in between schedules, twice so that + // changes to `Last` itself are applied too + app.init_resource::() + .add_systems(First, apply_script_system_changes) + .add_systems( + Last, + apply_script_system_changes.after(ScriptingSystemSet::RunQueuedCallbacks), + ); + if !self.dont_log_script_event_errors { app.add_systems(PostUpdate, script_error_logger); } diff --git a/crates/bevy_mod_scripting_core/src/pipeline/hooks.rs b/crates/bevy_mod_scripting_core/src/pipeline/hooks.rs index 2d4fb4e45d..2b4751f1e6 100644 --- a/crates/bevy_mod_scripting_core/src/pipeline/hooks.rs +++ b/crates/bevy_mod_scripting_core/src/pipeline/hooks.rs @@ -9,6 +9,7 @@ use bevy_mod_scripting_script::ScriptAttachment; use crate::{ commands::RunScriptCallback, event::{IntoCallbackLabel, OnScriptLoaded, OnScriptReloaded, OnScriptUnloaded}, + script_system::remove_script_systems, }; use super::*; @@ -56,78 +57,89 @@ pub fn on_script_unloaded_for_unload_pipeline_handler trigger: On>, mut commands: Commands, world_id: WorldId, + run_callback: bool, ) { let event = trigger.event(); - let emit_responses = P::readonly_configuration(world_id).emit_responses; - // let guard = WorldGuard::new_exclusive(world); - bevy_log::debug!( - "Running on_script_unloaded hook for script: {}, due to unload", - event.attachment - ); - commands.queue( - RunScriptCallback::

::new( - event.attachment.clone(), - OnScriptUnloaded::into_callback_label(), - vec![], - emit_responses, - ) - .with_context_override(trigger.existing_context.clone()) - .with_post_callback_handler(|world, attachment, response| { - if let Ok(v) = response.as_ref() { - bevy_log::debug!( - "on_script_unloaded hook for script: {} setting reload_state: {:?}", - attachment, - v - ); - let mut data = world.get_resource_or_init::(); - let data = data.0.entry(attachment).or_default(); - - data.reload_state = v.clone(); - } - }), - ); + if run_callback { + let emit_responses = P::readonly_configuration(world_id).emit_responses; + // let guard = WorldGuard::new_exclusive(world); + bevy_log::debug!( + "Running on_script_unloaded hook for script: {}, due to unload", + event.attachment + ); + commands.queue( + RunScriptCallback::

::new( + event.attachment.clone(), + OnScriptUnloaded::into_callback_label(), + vec![], + emit_responses, + ) + .with_context_override(trigger.existing_context.clone()) + .with_post_callback_handler(|world, attachment, response| { + if let Ok(v) = response.as_ref() { + bevy_log::debug!( + "on_script_unloaded hook for script: {} setting reload_state: {:?}", + attachment, + v + ); + let mut data = world.get_resource_or_init::(); + let data = data.0.entry(attachment).or_default(); + + data.reload_state = v.clone(); + } + }), + ); + } remove_registrations_when_unloaded(&mut commands, event.attachment.clone()); } -/// Once the unload callback ran, removes what the script registered which only makes sense while it runs, i.e. its exports and observers. +/// Removes the registrations and script systems of a script which is unloaded, or reloaded (it adds them again once loaded). +/// +/// Runs whether or not the unload callback is enabled, after the callback. fn remove_registrations_when_unloaded(commands: &mut Commands, attachment: ScriptAttachment) { - commands.queue(move |world: &mut World| remove_script_registrations(world, &attachment)); + commands.queue(move |world: &mut World| { + remove_script_registrations(world, &attachment); + remove_script_systems(world, &attachment); + }); } -// todo unify the two, they just need a context extractor trait + pub fn on_script_unloaded_for_reload_pipeline_handler( trigger: On>, mut commands: Commands, world_id: WorldId, + run_callback: bool, ) { let event = trigger.event(); - let emit_responses = P::readonly_configuration(world_id).emit_responses; - bevy_log::debug!( - "Running on_script_unloaded hook for script: {}, due to reload", - event.attachment - ); - commands.queue( - RunScriptCallback::

::new( - event.attachment.clone(), - OnScriptUnloaded::into_callback_label(), - vec![], - emit_responses, - ) - .with_context_override(trigger.existing_context.clone()) - .with_post_callback_handler(|world, attachment, response| { - if let Ok(v) = response.as_ref() { - bevy_log::debug!( - "on_script_unloaded hook for script: {} setting reload_state: {:?}", - attachment, - v - ); - let mut datas = world.get_resource_or_init::(); - let data = datas.0.entry(attachment).or_default(); - - data.reload_state = v.clone(); - bevy_log::debug!("Datas: {}", datas.0.len()); - } - }), - ); + if run_callback { + let emit_responses = P::readonly_configuration(world_id).emit_responses; + bevy_log::debug!( + "Running on_script_unloaded hook for script: {}, due to reload", + event.attachment + ); + commands.queue( + RunScriptCallback::

::new( + event.attachment.clone(), + OnScriptUnloaded::into_callback_label(), + vec![], + emit_responses, + ) + .with_context_override(trigger.existing_context.clone()) + .with_post_callback_handler(|world, attachment, response| { + if let Ok(v) = response.as_ref() { + bevy_log::debug!( + "on_script_unloaded hook for script: {} setting reload_state: {:?}", + attachment, + v + ); + let mut datas = world.get_resource_or_init::(); + let data = datas.0.entry(attachment).or_default(); + + data.reload_state = v.clone(); + bevy_log::debug!("Datas: {}", datas.0.len()); + } + }), + ); + } remove_registrations_when_unloaded(&mut commands, event.attachment.clone()); } diff --git a/crates/bevy_mod_scripting_core/src/pipeline/mod.rs b/crates/bevy_mod_scripting_core/src/pipeline/mod.rs index 07f7e44ca6..19c253a75f 100644 --- a/crates/bevy_mod_scripting_core/src/pipeline/mod.rs +++ b/crates/bevy_mod_scripting_core/src/pipeline/mod.rs @@ -240,11 +240,23 @@ impl Plugin for ScriptLoadingPipeline

{ ); } - // these are the only triggers on their events - if self.on_script_unloaded_callback { - app.add_observer(on_script_unloaded_for_unload_pipeline_handler::

); - app.add_observer(on_script_unloaded_for_reload_pipeline_handler::

); - } + // these are the only triggers on their events, so the unload callback runs before the script's + // registrations and systems are removed + let unloaded = self.on_script_unloaded_callback; + app.add_observer( + move |trigger: On>, commands: Commands, world_id: WorldId| { + on_script_unloaded_for_unload_pipeline_handler( + trigger, commands, world_id, unloaded, + ) + }, + ); + app.add_observer( + move |trigger: On>, commands: Commands, world_id: WorldId| { + on_script_unloaded_for_reload_pipeline_handler( + trigger, commands, world_id, unloaded, + ) + }, + ); // clear hanging machine data app.add_observer( diff --git a/crates/bevy_mod_scripting_core/src/script_system.rs b/crates/bevy_mod_scripting_core/src/script_system.rs index 5082d17779..6d473ddff9 100644 --- a/crates/bevy_mod_scripting_core/src/script_system.rs +++ b/crates/bevy_mod_scripting_core/src/script_system.rs @@ -1,8 +1,13 @@ //! everything to do with dynamically added script systems use crate::{ - IntoScriptPluginParams, callbacks::ScriptCallbacks, event::CallbackLabel, - extractors::get_all_access_ids, handler::ScriptingHandler, script::ScriptContexts, + IntoScriptPluginParams, + callbacks::ScriptCallbacks, + error::ScriptError, + event::{CallbackLabel, ScriptErrorMessage}, + extractors::get_all_access_ids, + handler::ScriptingHandler, + script::ScriptContexts, }; use ::{ @@ -18,49 +23,67 @@ use ::{ bevy_reflect::Reflect, }; use bevy_ecs::{ + archetype::ArchetypeGeneration, change_detection::{CheckChangeTicks, Tick}, - schedule::{InternedSystemSet, IntoScheduleConfigs, Schedule, Schedules}, - system::{RunSystemError, SystemAccess, SystemIn, SystemStateFlags}, - world::DeferredWorld, + message::Messages, + resource::Resource, + schedule::{ + InternedScheduleLabel, InternedSystemSet, IntoScheduleConfigs, Schedule, + ScheduleCleanupPolicy, ScheduleConfigs, ScheduleError, Schedules, + }, + system::{RunSystemError, ScheduleSystem, SystemAccess, SystemIn, SystemStateFlags}, + world::{DeferredWorld, WorldId}, }; -use bevy_log::{debug, error, warn_once}; +use bevy_log::{debug, warn, warn_once}; use bevy_mod_scripting_bindings::{ AppScheduleRegistry, AppScriptCallbackRunners, AppScriptComponentRegistry, AppScriptFunctionRegistry, AppScriptTypeCatalogue, CurrentScriptAttachment, InteropError, IntoScript, ReflectReference, ScriptQueryBuilder, ScriptQueryResult, ScriptResourceRegistration, V, WorldExtensions, }; +use bevy_mod_scripting_display::DisplayProxy; use bevy_mod_scripting_script::ScriptAttachment; -use bevy_mod_scripting_world::{AccessByteSet, WorldAccessGuard, WorldGuard}; +use bevy_mod_scripting_world::{AccessByteSet, ArchetypeFilter, WorldAccessGuard, WorldGuard}; +use bevy_platform::collections::HashMap; use bevy_reflect::TypeRegistryArc; use bevy_system_reflection::{ReflectSchedule, ReflectSystem}; use bevy_utils::prelude::DebugName; -use std::{any::TypeId, borrow::Cow, collections::HashSet, hash::Hash, marker::PhantomData}; -#[derive(Clone, Hash, PartialEq, Eq)] -/// a system set for script systems. -pub struct ScriptSystemSet(Cow<'static, str>); +use parking_lot::Mutex; +use std::{ + any::TypeId, + borrow::Cow, + collections::HashSet, + hash::Hash, + sync::{ + Arc, + atomic::{AtomicBool, Ordering}, + }, +}; +/// A system set containing a single script system, identified by its slot in [`ScriptSystems`]. +/// +/// System sets are interned, which leaks every distinct set value for the lifetime of the program, +/// so slots are recycled once their system is removed, bounding the leak by the peak number of live script systems. +#[derive(Clone, Copy, Hash, PartialEq, Eq)] +pub struct ScriptSystemSet(u32); impl std::fmt::Debug for ScriptSystemSet { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str("ScriptSystem(")?; - f.write_str(self.0.as_ref())?; - f.write_str(")")?; - Ok(()) + write!(f, "ScriptSystem({})", self.0) } } #[profiling::all_functions] impl ScriptSystemSet { - /// Creates a new script system set - pub fn new(id: impl Into>) -> Self { - Self(id.into()) + /// The slot of the set in [`ScriptSystems`] + pub fn slot(&self) -> u32 { + self.0 } } #[profiling::all_functions] impl SystemSet for ScriptSystemSet { fn dyn_clone(&self) -> Box { - Box::new(self.clone()) + Box::new(*self) } } @@ -128,57 +151,425 @@ impl ScriptSystemBuilder { self } - /// Builds the system and inserts it into the given schedule + /// Builds the system and inserts it into the given schedule. + /// + /// Replaces the system with the same attachment and callback label if one exists, see [`ScriptSystems`]. + /// If the schedule is running, the system is added once it finishes, and the returned system has no node id. #[allow(deprecated)] pub fn build( self, world: WorldGuard, schedule: &ReflectSchedule, ) -> Result { - world.scope_schedule(schedule, |world, schedule| { - // this is different to a normal event handler - // the system doesn't listen to events - // it immediately calls a singular script with a predefined payload - let before_systems = self.before.clone(); - let after_systems = self.after.clone(); - - // this is quite important, by default systems are placed in a set defined by their TYPE, i.e. in this case - // all script systems would be the same - - let system: DynamicScriptSystem

= bevy_ecs::system::IntoSystem::into_system(self); - let mut system_config = system.into_configs(); - // let mut system_config = > + 'static)>, (Infallible, IsDynamicScriptSystem

)>>::into_configs(self); // apply ordering - for (other, is_before) in before_systems - .into_iter() - .map(|b| (b, true)) - .chain(after_systems.into_iter().map(|a| (a, false))) - { - for default_set in other.default_system_sets() { - if is_before { - system_config = system_config.before(*default_set); - } else { - system_config = system_config.after(*default_set); + world.with_world_mut(|world| { + let systems = world.get_resource_or_init::().clone(); + let label = *schedule.label(); + let alive = Arc::new(AtomicBool::new(true)); + let mut changes = Vec::with_capacity(2); + + let set = { + let mut inner = systems.0.lock(); + let key = (self.attachment.clone(), self.name.clone()); + let set = match inner.systems.get(&key) { + Some(old) => { + // a dead one is a tombstone of a removed system, see `remove_systems_of` + if old.alive.swap(false, Ordering::Relaxed) { + warn!( + "Script system `{}` of script {} already exists, replacing it", + self.name, self.attachment + ); + } + // keep the set, so systems ordered relative to the old system stay ordered relative to the new one + changes.push(PendingScheduleChange::new( + old.schedule, + ScheduleChange::Remove { + set: old.set, + free_slot: false, + }, + )); + old.set } - } + None => inner.allocate_set(), + }; + inner.systems.insert( + key, + ScriptSystemEntry { + set, + schedule: label, + alive: alive.clone(), + orders_against: self + .before + .iter() + .chain(&self.after) + .flat_map(|system| system.default_system_sets().iter().copied()) + .collect(), + }, + ); + set + }; + + let unscheduled = ReflectSystem::from_unscheduled_system( + &DynamicScriptSystem::

::new(self.clone(), set, alive.clone(), systems.clone()), + ); + changes.push(PendingScheduleChange::new( + label, + ScheduleChange::Add { + builder: self.clone(), + set, + alive, + // schedule configs are not `Send`, so the system is created once it is added + into_configs: script_system_configs::

, + }, + )); + + if !systems.apply_or_queue(world, changes) { + debug!( + "Schedule `{}` is running, script system `{}` will be added once it finishes", + schedule.identifier(), + self.name + ); + return Ok(unscheduled); } - schedule.add_systems(system_config); - // TODO: the node id seems to always be system.len() - // if this is slow, we can always just get the node id that way - // and let the schedule initialize itself right before it gets run - // for now I want to avoid not having the right ID as that'd be a pain - schedule.initialize(world).map_err(InteropError::external)?; - // now find the system - let (node_id, system) = schedule - .systems() - .map_err(InteropError::external)? - .max_by_key(|(n, _)| *n) - .ok_or_else(|| InteropError::invariant("After adding the system, it was not found in the schedule, could not return a reference to it"))?; - Ok(ReflectSystem::from_system(system.as_ref(), node_id)) + // initialize the schedule to find the node id of the new system + let Some(found) = with_schedule(world, label, |world, schedule| { + schedule.initialize(world).map_err(InteropError::external)?; + let set = set.intern(); + Ok::<_, InteropError>( + schedule + .systems() + .map_err(InteropError::external)? + .find(|(_, system)| system.default_system_sets().contains(&set)) + .map(|(node_id, system)| ReflectSystem::from_system(system.as_ref(), node_id)), + ) + }) else { + return Ok(unscheduled); + }; + found?.ok_or_else(|| InteropError::invariant("After adding the system, it was not found in the schedule, could not return a reference to it")) })? } } +/// Keeps track of the systems added by scripts. +/// +/// A script system is identified by its attachment and callback label: adding a system with the same identity replaces +/// the old one, and the systems of an attachment are removed when its script unloads or reloads. +/// +/// Schedules cannot be changed while they run (they are taken out of [`Schedules`]), i.e. `PreUpdate`, where scripts +/// are loaded. Changes to running schedules (and later changes to the same schedule) are queued and applied by +/// [`apply_script_system_changes`] in `First` and `Last`. +/// Removed systems stop running right away, even if their removal from the schedule is queued. +#[derive(Resource, Clone, Default)] +pub struct ScriptSystems(Arc>); + +#[derive(Default)] +struct ScriptSystemsInner { + systems: HashMap<(ScriptAttachment, CallbackLabel), ScriptSystemEntry>, + free_slots: Vec, + next_slot: u32, + /// changes to schedules which were running when requested, in order of request + pending: Vec, + /// errors of script systems, which cannot send messages themselves as they might not have access to them + errors: Vec, +} + +struct ScriptSystemEntry { + set: ScriptSystemSet, + schedule: InternedScheduleLabel, + alive: Arc, + /// the sets this system is ordered against, a removed set must not be reused while systems are ordered against it + orders_against: Vec, +} + +/// Script system sets are never removed from schedules, only their systems. +/// +/// Bevy 0.20 does not forget removed sets (`SystemSets::remove` keeps the set in its id map), adding a system to a +/// set which was removed panics. Empty sets are reused instead, when their slot is recycled. +/// Breakages are allowed, i.e. the systems ordered before and after a removed system are not ordered against +/// each other, as that ordering only existed because of the removed system. +const SCRIPT_SYSTEM_CLEANUP_POLICY: ScheduleCleanupPolicy = + ScheduleCleanupPolicy::RemoveSystemsOnlyAllowBreakages; + +struct PendingScheduleChange { + schedule: InternedScheduleLabel, + change: ScheduleChange, + attempts: u8, +} + +type IntoScriptSystemConfigs = fn( + ScriptSystemBuilder, + ScriptSystemSet, + Arc, + ScriptSystems, +) -> ScheduleConfigs; + +/// Creates the system with its ordering +fn script_system_configs( + builder: ScriptSystemBuilder, + set: ScriptSystemSet, + alive: Arc, + systems: ScriptSystems, +) -> ScheduleConfigs { + let (before, after) = (builder.before.clone(), builder.after.clone()); + let mut config = DynamicScriptSystem::

::new(builder, set, alive, systems).into_configs(); + for other in before { + for default_set in other.default_system_sets() { + config = config.before(*default_set); + } + } + for other in after { + for default_set in other.default_system_sets() { + config = config.after(*default_set); + } + } + config +} + +enum ScheduleChange { + Add { + builder: ScriptSystemBuilder, + set: ScriptSystemSet, + alive: Arc, + into_configs: IntoScriptSystemConfigs, + }, + /// Removes the systems in the set, and frees its slot afterwards if `free_slot` is set + Remove { + set: ScriptSystemSet, + free_slot: bool, + }, +} + +/// How many times a queued change is retried before giving up, schedules which don't exist at all would never succeed. +const MAX_SCHEDULE_CHANGE_ATTEMPTS: u8 = 8; + +impl PendingScheduleChange { + fn new(schedule: InternedScheduleLabel, change: ScheduleChange) -> Self { + Self { + schedule, + change, + attempts: 0, + } + } +} + +impl ScriptSystemsInner { + fn allocate_set(&mut self) -> ScriptSystemSet { + ScriptSystemSet(self.free_slots.pop().unwrap_or_else(|| { + self.next_slot += 1; + self.next_slot - 1 + })) + } +} + +#[profiling::all_functions] +impl ScriptSystems { + /// Applies the changes in order, queueing changes to running schedules and changes behind queued ones. + /// + /// Returns true if the last change was applied. + fn apply_or_queue(&self, world: &mut World, changes: Vec) -> bool { + let mut last_applied = true; + for change in changes { + // Keep the order of changes to a schedule across calls, the loop only keeps it within one call. + // A change to a schedule which is back in the world must wait for the ones queued while it was running: + // Update: a script adds S to Update, which is running -> Add(S) is queued + // PostUpdate: the script unloads -> Remove(S) to Update, which is back in the world + // applied right away, Remove(S) finds no set and frees the slot, then `Last` applies the queued Add(S): + // a dead system stays in the schedule, in the set of whichever system reuses the slot + let queued_before = self + .0 + .lock() + .pending + .iter() + .any(|pending| pending.schedule == change.schedule); + let not_applied = if queued_before { + Some(change) + } else { + self.try_apply(world, change) + }; + last_applied = not_applied.is_none(); + self.0.lock().pending.extend(not_applied); + } + last_applied + } + + /// Applies the change, returns it back if its schedule is not in the world, i.e. because it is running. + fn try_apply( + &self, + world: &mut World, + change: PendingScheduleChange, + ) -> Option { + let PendingScheduleChange { + schedule: label, + change, + attempts, + } = change; + let mut change = Some(change); + let result = with_schedule(world, label, |world, schedule| match change.take() { + Some(ScheduleChange::Add { + builder, + set, + alive, + into_configs, + }) => { + schedule.add_systems(into_configs(builder, set, alive, self.clone())); + Ok(()) + } + Some(ScheduleChange::Remove { set, free_slot }) => { + let result = match schedule.remove_systems_in_set( + set, + world, + SCRIPT_SYSTEM_CLEANUP_POLICY, + ) { + Ok(_) | Err(ScheduleError::SetNotFound) => Ok(()), + Err(err) => Err(InteropError::external(err)), + }; + if free_slot && result.is_ok() { + let mut inner = self.0.lock(); + // systems ordered against the set would be ordered against whatever reuses it + let interned = set.intern(); + if !inner + .systems + .values() + .any(|entry| entry.orders_against.contains(&interned)) + { + inner.free_slots.push(set.0); + } + } + result + } + None => Ok(()), + }); + + match (result, change) { + (Some(Ok(())), _) => None, + (Some(Err(err)), _) => { + self.0.lock().errors.push( + ScriptError::from(err) + .with_context(format!("changing script systems in schedule: {label:?}")), + ); + None + } + (None, Some(change)) if attempts < MAX_SCHEDULE_CHANGE_ATTEMPTS => { + Some(PendingScheduleChange { + schedule: label, + change, + attempts: attempts + 1, + }) + } + (None, _) => { + self.0.lock().errors.push( + ScriptError::from(InteropError::string(format!( + "schedule {label:?} was not found in the world after {MAX_SCHEDULE_CHANGE_ATTEMPTS} attempts" + ))) + .with_context("changing script systems"), + ); + None + } + } + } + + /// Marks the systems of the attachment as removed, and removes them from their schedules. + /// + /// A removed system other systems are ordered against stays as a tombstone keeping its set, so a reload re-adding + /// it is ordered as before. Tombstones nothing is ordered against anymore are freed. + pub fn remove_systems_of(&self, world: &mut World, attachment: &ScriptAttachment) { + let changes = { + let mut inner = self.0.lock(); + let mut removed = HashSet::new(); + for ((owner, _), entry) in inner.systems.iter_mut() { + if owner == attachment && entry.alive.swap(false, Ordering::Relaxed) { + // a tombstone holds no other sets + entry.orders_against.clear(); + removed.insert(entry.set); + } + } + let ordered_against = inner + .systems + .values() + .flat_map(|entry| entry.orders_against.iter().copied()) + .collect::>(); + let mut changes = Vec::new(); + inner.systems.retain(|_, entry| { + if entry.alive.load(Ordering::Relaxed) { + return true; + } + let free_slot = !ordered_against.contains(&entry.set.intern()); + if free_slot || removed.contains(&entry.set) { + changes.push(PendingScheduleChange::new( + entry.schedule, + ScheduleChange::Remove { + set: entry.set, + free_slot, + }, + )); + } + !free_slot + }); + changes + }; + if !changes.is_empty() { + self.apply_or_queue(world, changes); + } + } + + fn report(&self, error: ScriptError) { + self.0.lock().errors.push(error); + } +} + +/// Removes the script systems of the given attachment from their schedules, see [`ScriptSystems`]. +pub fn remove_script_systems(world: &mut World, attachment: &ScriptAttachment) { + if let Some(systems) = world.get_resource::().cloned() { + systems.remove_systems_of(world, attachment); + } +} + +/// Applies queued changes to script systems, and sends the errors of script systems as [`ScriptErrorMessage`]s. +pub fn apply_script_system_changes(world: &mut World) { + let Some(systems) = world.get_resource::().cloned() else { + return; + }; + let pending = std::mem::take(&mut systems.0.lock().pending); + let mut still_pending = Vec::new(); + for change in pending { + // keep the order of changes to the same schedule + if still_pending + .iter() + .any(|p: &PendingScheduleChange| p.schedule == change.schedule) + { + still_pending.push(change); + continue; + } + still_pending.extend(systems.try_apply(world, change)); + } + let errors = { + let mut inner = systems.0.lock(); + // changes requested while applying go after the ones which are still pending + still_pending.append(&mut inner.pending); + inner.pending = still_pending; + std::mem::take(&mut inner.errors) + }; + if !errors.is_empty() + && let Some(mut messages) = world.get_resource_mut::>() + { + messages.write_batch(errors.into_iter().map(|error| ScriptErrorMessage { error })); + } +} + +/// Takes the schedule out of the world, runs the function on it and puts it back. +/// +/// Returns `None` if the schedule is not in the world, i.e. because it is running. +fn with_schedule( + world: &mut World, + label: InternedScheduleLabel, + f: impl FnOnce(&mut World, &mut Schedule) -> O, +) -> Option { + let mut schedule = world.get_resource_mut::()?.remove(label)?; + let result = f(world, &mut schedule); + world.get_resource_or_init::().insert(schedule); + Some(result) +} + /// TODO: inline world guard into the system state, we should be able to re-use it struct ScriptSystemState { type_registry: TypeRegistryArc, @@ -192,7 +583,15 @@ struct ScriptSystemState { system_params: Vec, script_contexts: ScriptContexts

, script_callbacks: ScriptCallbacks

, - initialization_errors: Result<(), Vec>, + /// set if the system parameters are invalid, the system then never runs, the errors are reported once on initialization + invalid: bool, + /// the world the system was initialized in, its queries cannot be used in other worlds + world_id: WorldId, + /// restricts component accesses to the archetypes matched by the queries, see [`ArchetypeFilter`]. + /// Rebuilt whenever new archetypes appear, as the queries might match them. + archetype_filter: Option>, + /// the archetype generation the filter was built for + archetype_generation: ArchetypeGeneration, } /// Equivalent of [`bevy_ecs::system::SystemParam`] but for dynamic systems, these are the kinds of things @@ -223,29 +622,61 @@ pub struct DynamicScriptSystem { target_attachment: ScriptAttachment, system_param_descriptors: Vec, state: Option>, + /// the set containing just this system, used for ordering against it and removing it + set: ScriptSystemSet, + /// cleared when the system is replaced or its script is unloaded, the system then stops running + alive: Arc, + systems: ScriptSystems, _marker: std::marker::PhantomData P>, } -/// A marker type distinguishing between vanilla and script system types -pub struct IsDynamicScriptSystem

(PhantomData P>); - -#[profiling::all_functions] -impl bevy_ecs::system::IntoSystem<(), (), IsDynamicScriptSystem

> - for ScriptSystemBuilder -{ - type System = DynamicScriptSystem

; - - fn into_system(builder: Self) -> Self::System { - Self::System { +impl DynamicScriptSystem

{ + fn new( + builder: ScriptSystemBuilder, + set: ScriptSystemSet, + alive: Arc, + systems: ScriptSystems, + ) -> Self { + Self { name: builder.name.to_string().into(), exclusive: builder.is_exclusive, system_param_descriptors: builder.system_params, last_run: Default::default(), target_attachment: builder.attachment, state: None, + set, + alive, + systems, _marker: Default::default(), } } + + fn report(&self, error: InteropError) { + self.systems.report( + ScriptError::from(error) + .with_script(self.target_attachment.script().display()) + .with_context(format!("script system: {}", self.name)) + .with_language(P::LANGUAGE), + ); + } +} + +/// Builds the [`ArchetypeFilter`] restricting each component declared by a query to the archetypes the queries +/// declaring it matched. Must be called after the queries were updated with the world's archetypes. +fn build_archetype_filter(params: &[ScriptSystemParam]) -> ArchetypeFilter { + let mut filter = ArchetypeFilter::default(); + for param in params { + if let ScriptSystemParam::EntityQuery { query, components } = param { + // only components the query reads or writes, components used just as filters (`with`) are not accessible + for (component_id, _) in components { + filter.restrict(*component_id); + for archetype_id in query.matched_archetypes() { + filter.allow(*component_id, archetype_id); + } + } + } + } + filter } // #[profiling::all_functions] @@ -271,15 +702,51 @@ impl System for DynamicScriptSystem

{ _input: SystemIn<'_, Self>, world: UnsafeWorldCell, ) -> Result { + // replaced or unloaded, the system might still be in the schedule until the removal is applied + if !self.alive.load(Ordering::Relaxed) { + return Ok(()); + } + let _change_tick = world.increment_change_tick(); - #[allow( - clippy::panic, - reason = "cannot avoid panicking inside run_unsafe due to Bevy API structure" - )] - let state = match &mut self.state { - Some(state) => state, - None => panic!("System state not initialized!"), + let Some(state) = &mut self.state else { + return Err(RunSystemError::Skipped( + SystemParamValidationError::skipped::<()>( + "Script system was run before it was initialized", + ), + )); + }; + + // reported on initialization + if state.invalid { + return Ok(()); + } + + if world.id() != state.world_id { + return Err(RunSystemError::Skipped( + SystemParamValidationError::skipped::<()>( + "Script system was run in a different world than it was initialized in", + ), + )); + } + + let script_context = { + // the lock must not be held while the callback runs, which might load or unload scripts + let contexts = state.script_contexts.read(); + match contexts.get_context(&self.target_attachment) { + Some(context) => context.as_loaded().cloned(), + None => { + warn_once!( + "Dynamic script system `{}` could not find script for attachment: {}. It will not run until it's loaded.", + self.name, + self.target_attachment + ); + None + } + } + }; + let Some(script_context) = script_context else { + return Ok(()); }; let mut payload = Vec::with_capacity(state.system_params.len()); @@ -296,20 +763,31 @@ impl System for DynamicScriptSystem

{ let world = unsafe { world.world_mut() }; WorldAccessGuard::new_exclusive(world, cache) } else { + // the queries match newly created archetypes when updated, the filter has to include them + if state.archetype_filter.is_none() + || world.archetypes().generation() != state.archetype_generation + { + for param in &mut state.system_params { + if let ScriptSystemParam::EntityQuery { query, .. } = param { + query.update_archetypes_unsafe_world_cell(world); + } + } + state.archetype_filter = + Some(Arc::new(build_archetype_filter(&state.system_params))); + state.archetype_generation = world.archetypes().generation(); + } + // Safety: the subset and archetype filter are exactly the accesses declared to bevy in `initialize` unsafe { WorldAccessGuard::new_non_exclusive( world, state.subset.clone(), + state.archetype_filter.clone(), state.type_registry.clone(), cache, ) } }; - if let Err(Some(first)) = state.initialization_errors.as_mut().map_err(|e| e.pop()) { - return Err(RunSystemError::Skipped(first)); - } - // TODO: cache references which don't change once we have benchmarks for param in &mut state.system_params { match param { @@ -347,41 +825,16 @@ impl System for DynamicScriptSystem

{ } } - // Now that we have everything ready, we need to run the callback on the - // targetted scripts. Let's start with just calling the one targetted - // script. - - let script_context = &state.script_contexts.read(); - - if let Some(context) = script_context.get_context(&self.target_attachment) { - let context = if let Some(context) = context.as_loaded() { - context - } else { - return Ok(()); - }; - - let result = P::handle( - payload, - &self.target_attachment, - &state.callback_label, - context, - state.script_callbacks.clone(), - guard.clone(), - ); - // TODO: Emit error events via commands, maybe accumulate in state - // instead and use apply. - match result { - Ok(_) => {} - Err(err) => { - error!("Error in dynamic script system `{}`: {:#?}", self.name, err) - } - } - } else { - warn_once!( - "Dynamic script system `{}` could not find script for attachment: {}. It will not run until it's loaded.", - self.name, - self.target_attachment - ); + let result = P::handle( + payload, + &self.target_attachment, + &state.callback_label, + &script_context, + state.script_callbacks.clone(), + guard.clone(), + ); + if let Err(err) = result { + self.report(err); } Ok(()) @@ -396,7 +849,7 @@ impl System for DynamicScriptSystem

{ let mut subset = HashSet::::new(); let mut system_params = Vec::with_capacity(self.system_param_descriptors.len()); let mut component_access_set = FilteredAccessSet::new(); - let mut initialization_errors: Result<(), Vec> = Ok(()); + let mut initialization_errors = Vec::new(); for param in &self.system_param_descriptors { match param { ScriptSystemParamDescriptor::Res(res) => { @@ -414,14 +867,9 @@ impl System for DynamicScriptSystem

{ access.add_write(component_id); component_access_set.add(access); if subset.contains(&component_id) { - initialization_errors = Err(initialization_errors - .err() - .unwrap_or_default() - .into_iter() - .chain([SystemParamValidationError::skipped::<()>(format!( - "Duplicate resource access in system: {component_id:?}." - ))]) - .collect()); + initialization_errors.push(format!( + "Duplicate resource access in system: {component_id:?}." + )); } subset.insert(component_id); } @@ -433,7 +881,10 @@ impl System for DynamicScriptSystem

{ .collect(); let query = query.as_query_state::(world); - // Safety: we are not removing + // The access carries the query filters, so bevy runs this system in parallel with systems accessing + // the same components on other archetypes (i.e. `With` vs `Without`). + // The guard only allows component accesses per component id, so it is narrowed to the archetypes + // matched by this query in `run_unsafe`, see `ArchetypeFilter`. component_access_set.add(query.component_access().clone()); let new_raids = get_all_access_ids(query.component_access().access()) @@ -442,14 +893,8 @@ impl System for DynamicScriptSystem

{ .collect::>(); if !subset.is_disjoint(&new_raids) { - initialization_errors = Err(initialization_errors - .err() - .unwrap_or_default() - .into_iter() - .chain([SystemParamValidationError::skipped::<()>( - "Non-disjoint query in dynamic system parameters.".to_string(), - )]) - .collect()); + initialization_errors + .push("Non-disjoint query in dynamic system parameters.".to_owned()); } system_params.push(ScriptSystemParam::EntityQuery { @@ -484,9 +929,18 @@ impl System for DynamicScriptSystem

{ system_params, script_contexts: world.get_resource_or_init::>().clone(), script_callbacks: world.get_resource_or_init::>().clone(), - initialization_errors, + invalid: !initialization_errors.is_empty(), + world_id: world.id(), + archetype_filter: None, + archetype_generation: ArchetypeGeneration::initial(), }); + for error in initialization_errors { + self.report(InteropError::string(format!( + "{error} The system will not run." + ))); + } + if self.exclusive { SystemAccess::Exclusive } else { @@ -511,7 +965,7 @@ impl System for DynamicScriptSystem

{ fn queue_deferred(&mut self, _world: DeferredWorld) {} fn default_system_sets(&self) -> Vec { - vec![ScriptSystemSet::new(self.name.clone()).intern()] + vec![self.set.intern()] } fn system_type(&self) -> TypeId { @@ -565,19 +1019,16 @@ impl ManageScriptSystems for WorldGuard<'_> { let result = f(world, &mut removed_schedule); - let mut schedules = world.get_resource_mut::().ok_or_else(|| { - InteropError::unsupported_operation( - None, - None, - "removing `Schedules` resource within a schedule scope", - ) - })?; - - assert!( - removed_schedule.label() == *label.label(), - "removed schedule label doesn't match the original" - ); - schedules.insert(removed_schedule); + // put the schedule back even if `Schedules` was removed in the meantime, so it isn't lost + let matches = removed_schedule.label() == *label.label(); + world + .get_resource_or_init::() + .insert(removed_schedule); + if !matches { + return Err(InteropError::invariant( + "removed schedule label doesn't match the original", + )); + } Ok(result) })? @@ -695,4 +1146,149 @@ mod test { // now re-run app, expect no panicks app.update(); } + + #[derive(ScheduleLabel, Clone, Debug, Hash, PartialEq, Eq)] + struct SystemsTestSchedule; + + fn systems_test_app() -> App { + let mut app = App::new(); + app.add_plugins(( + AssetPlugin::default(), + DiagnosticsPlugin, + TestPlugin::default(), + BMSScriptingInfrastructurePlugin::default(), + )); + app.init_schedule(SystemsTestSchedule); + app.finish(); + app.cleanup(); + app + } + + fn attachment() -> ScriptAttachment { + ScriptAttachment::StaticScript(Handle::default()) + } + + fn add(app: &mut App, label: &str) -> ReflectSystem { + build( + app, + ScriptSystemBuilder::new(label.to_owned().into(), attachment()), + ) + } + + fn build(app: &mut App, builder: ScriptSystemBuilder) -> ReflectSystem { + let world = app.world_mut(); + let cache = WorldAccessGuard::setup_cache(world, CurrentScriptAttachment::empty()); + builder + .build::( + WorldAccessGuard::new_exclusive(world, cache), + &ReflectSchedule::from_label(SystemsTestSchedule), + ) + .unwrap() + } + + fn count(app: &mut App, label: &str) -> usize { + // `systems` lists the systems of the last schedule build + with_schedule( + app.world_mut(), + SystemsTestSchedule.intern(), + |world, schedule| { + schedule.initialize(world).unwrap(); + schedule + .systems() + .unwrap() + .filter(|(_, system)| system.name().to_string() == label) + .count() + }, + ) + .unwrap() + } + + /// takes the schedule out of the world, as bevy does while running it + fn while_running(app: &mut App, f: impl FnOnce(&mut App)) { + let schedule = app + .world_mut() + .resource_mut::() + .remove(SystemsTestSchedule) + .unwrap(); + f(app); + app.world_mut().resource_mut::().insert(schedule); + } + + #[test] + fn adding_a_system_again_replaces_it() { + let mut app = systems_test_app(); + let first = add(&mut app, "test"); + let second = add(&mut app, "test"); + assert_eq!(count(&mut app, "test"), 1); + assert_eq!(first.default_system_sets(), second.default_system_sets()); + // sets are interned by value, so each slot is its own set + assert_ne!(ScriptSystemSet(0).intern(), ScriptSystemSet(1).intern()); + } + + #[test] + fn systems_are_removed_with_their_script_and_slots_are_recycled() { + let mut app = systems_test_app(); + let first = add(&mut app, "test"); + remove_script_systems(app.world_mut(), &attachment()); + assert_eq!(count(&mut app, "test"), 0); + + let second = add(&mut app, "other"); + assert_eq!(first.default_system_sets(), second.default_system_sets()); + + // a system of another script is ordered against keeps its set when its script reloads + let mut dependent = ScriptSystemBuilder::new( + "dependent".to_owned().into(), + ScriptAttachment::EntityScript(Entity::PLACEHOLDER, Handle::default()), + ); + dependent.after_system(second.clone()); + build(&mut app, dependent); + remove_script_systems(app.world_mut(), &attachment()); + let reloaded = add(&mut app, "other"); + assert_eq!(second.default_system_sets(), reloaded.default_system_sets()); + let schedules = app.world().resource::(); + let graph = bevy_system_reflection::schedule_to_reflect_graph( + schedules.get(SystemsTestSchedule).unwrap(), + ); + assert!(graph.system_runs_before("other", "dependent").unwrap()); + } + + #[test] + fn changes_to_running_schedules_are_applied_later() { + let mut app = systems_test_app(); + while_running(&mut app, |app| { + add(app, "test"); + }); + assert_eq!(count(&mut app, "test"), 0); + apply_script_system_changes(app.world_mut()); + assert_eq!(count(&mut app, "test"), 1); + + while_running(&mut app, |app| { + remove_script_systems(app.world_mut(), &attachment()); + }); + // stops running right away, but stays in the schedule until the change is applied + assert_eq!(count(&mut app, "test"), 1); + assert!( + app.world() + .resource::() + .0 + .lock() + .systems + .is_empty() + ); + apply_script_system_changes(app.world_mut()); + assert_eq!(count(&mut app, "test"), 0); + } + + #[test] + fn script_system_errors_are_sent_as_messages() { + let mut app = systems_test_app(); + app.world() + .resource::() + .report(ScriptError::from(InteropError::string("oops".to_owned()))); + apply_script_system_changes(app.world_mut()); + assert_eq!( + app.world().resource::>().len(), + 1 + ); + } } diff --git a/crates/bevy_mod_scripting_world/src/archetype_filter.rs b/crates/bevy_mod_scripting_world/src/archetype_filter.rs new file mode 100644 index 0000000000..a9ff970beb --- /dev/null +++ b/crates/bevy_mod_scripting_world/src/archetype_filter.rs @@ -0,0 +1,112 @@ +//! Per-entity restrictions on component access, for guards of non-exclusive systems with filtered queries. +//! +//! # Why this exists +//! +//! An [`crate::AccessByteSet`] grants access per [`ComponentId`], i.e. to the component on *every* entity. +//! Bevy schedules systems by their [`bevy_ecs::query::FilteredAccess`] instead, which also carries the query +//! filters: a system writing `C` with `With` and a system writing `C` with `Without` are considered disjoint +//! and run in parallel. If a script in the first system could reach `C` on an entity *without* `A` +//! (i.e. via `world.get_component`), both systems would hold mutable references to the same value. +//! +//! # Why archetypes +//! +//! An archetype is the exact set of component types an entity has, every entity lives in exactly one. +//! `With`/`Without`/`Or` only depend on which components an entity has, so they match whole archetypes, +//! and a query records the archetypes it matched. Tables are NOT the right unit: entities with and without a +//! sparse set component `A` share a table (and its `C` column) while living in different archetypes. +//! +//! Non-archetypal filters (`Added`/`Changed`) match every archetype containing the component, so the +//! restriction over-approximates them, which is fine since Bevy also assumes the whole component is accessed. +//! +//! # Invariant +//! +//! The (component, archetype) pairs allowed here must be a subset of the pairs Bevy believes the system accesses. +//! Building the filter from the matched archetypes of the same `QueryState`s whose accesses are reported to Bevy +//! guarantees this for every kind of filter. +//! +//! Archetypes cannot change while a non-exclusive system runs (structural changes need exclusive world access), +//! so the filter stays valid for the whole run of the system. + +use bevy_ecs::{archetype::ArchetypeId, component::ComponentId}; + +/// Restricts access to components to entities in a set of archetypes, per component. +/// +/// Components without a restriction are not affected, i.e. resources and components accessed by exclusive guards. +#[derive(Clone, Debug, Default)] +pub struct ArchetypeFilter { + /// Restricted components, each with a bitset indexed by [`ArchetypeId::index`]. + /// Systems restrict a handful of components, so a linear scan beats indexing by component id. + restricted: Vec<(ComponentId, Vec)>, +} + +impl ArchetypeFilter { + fn bits_mut(&mut self, component_id: ComponentId) -> &mut Vec { + let index = match self + .restricted + .iter() + .position(|(id, _)| *id == component_id) + { + Some(index) => index, + None => { + self.restricted.push((component_id, Vec::new())); + self.restricted.len() - 1 + } + }; + &mut self.restricted[index].1 + } + + /// Allows access to the given component on entities in the given archetype. + /// + /// Once called for a component, the component is restricted to the archetypes allowed this way. + pub fn allow(&mut self, component_id: ComponentId, archetype_id: ArchetypeId) { + let bits = self.bits_mut(component_id); + let (word, bit) = (archetype_id.index() / 64, archetype_id.index() % 64); + if word >= bits.len() { + bits.resize(word + 1, 0); + } + bits[word] |= 1 << bit; + } + + /// Restricts the given component to no archetypes, until [`Self::allow`] is called for it. + pub fn restrict(&mut self, component_id: ComponentId) { + self.bits_mut(component_id); + } + + /// Returns true if the component may be accessed on an entity in the given archetype. + #[inline] + pub fn is_allowed(&self, component_id: ComponentId, archetype_id: ArchetypeId) -> bool { + match self.restricted.iter().find(|(id, _)| *id == component_id) { + Some((_, bits)) => bits + .get(archetype_id.index() / 64) + .is_some_and(|word| word & (1 << (archetype_id.index() % 64)) != 0), + None => true, + } + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn unrestricted_components_are_allowed_everywhere() { + let filter = ArchetypeFilter::default(); + assert!(filter.is_allowed(ComponentId::new(3), ArchetypeId::new(70))); + } + + #[test] + fn restricted_components_are_only_allowed_in_allowed_archetypes() { + let mut filter = ArchetypeFilter::default(); + let (component, other) = (ComponentId::new(2), ComponentId::new(5)); + filter.restrict(component); + assert!(!filter.is_allowed(component, ArchetypeId::new(1))); + + filter.allow(component, ArchetypeId::new(1)); + filter.allow(component, ArchetypeId::new(130)); + assert!(filter.is_allowed(component, ArchetypeId::new(1))); + assert!(filter.is_allowed(component, ArchetypeId::new(130))); + assert!(!filter.is_allowed(component, ArchetypeId::new(0))); + assert!(!filter.is_allowed(component, ArchetypeId::new(64))); + assert!(filter.is_allowed(other, ArchetypeId::new(0))); + } +} diff --git a/crates/bevy_mod_scripting_world/src/lib.rs b/crates/bevy_mod_scripting_world/src/lib.rs index 23d6bb6294..db6948ef59 100644 --- a/crates/bevy_mod_scripting_world/src/lib.rs +++ b/crates/bevy_mod_scripting_world/src/lib.rs @@ -1,7 +1,9 @@ //! Abstractions for interacting with the bevy world without knowing compile time type information safely. mod access_map; +mod archetype_filter; mod world; pub use access_map::*; +pub use archetype_filter::*; pub use world::*; diff --git a/crates/bevy_mod_scripting_world/src/world.rs b/crates/bevy_mod_scripting_world/src/world.rs index 1d6e6a3fbf..64d6bf28f5 100644 --- a/crates/bevy_mod_scripting_world/src/world.rs +++ b/crates/bevy_mod_scripting_world/src/world.rs @@ -5,11 +5,12 @@ //! Scripting languages only really support `Clone` objects so if we want to support references, //! we need wrapper types which have owned and ref variants. -use crate::{AccessByteSet, AccessMap, WorldAccessRange}; +use crate::{AccessByteSet, AccessMap, ArchetypeFilter, WorldAccessRange}; use super::access_map::DynamicSystemMeta; use ::bevy_ecs::{ component::ComponentId, + entity::Entity, world::{World, unsafe_world_cell::UnsafeWorldCell}, }; use bevy_ecs::{ @@ -22,7 +23,7 @@ use std::{ cell::{Ref, RefCell}, fmt::Debug, rc::Rc, - sync::atomic::AtomicBool, + sync::{Arc, atomic::AtomicBool}, }; /// Prefer to directly using [`WorldAccessGuard`]. If the underlying type changes, this alias will be updated. @@ -91,6 +92,8 @@ pub(crate) struct WorldAccessGuardInner<'w> { cell: UnsafeWorldCell<'w>, // TODO: this is fairly hefty, explore sparse sets, bit fields etc pub(crate) accesses: AccessMap, + /// Per-entity restrictions on top of `accesses`, set for non-exclusive systems with filtered queries + archetype_filter: Option>, /// Cached for convenience, since we need it for most operations, means we don't need to lock the type registry every time type_registry: TypeRegistryArc, @@ -361,12 +364,15 @@ impl<'w> WorldAccessGuard<'w> { /// It requires specyfing the exact accesses that are allowed to be given out by the guard. /// Those accesses need to be safe to be given out to the script, as the guard will assume that it is safe to give them out in any way. /// + /// The `archetype_filter` further restricts component accesses to entities in the given archetypes, see [`ArchetypeFilter`]. + /// /// # Safety - /// - The caller must ensure that the accesses in subset are not aliased by any other access + /// - The caller must ensure that the accesses in subset (narrowed by the archetype filter) are not aliased by any other access /// - If an access is allowed in this subset, but alised by someone else, pub unsafe fn new_non_exclusive( world: UnsafeWorldCell<'w>, subset: AccessByteSet, + archetype_filter: Option>, type_registry: TypeRegistryArc, registry_cache: RegistryCache, ) -> Self { @@ -374,6 +380,7 @@ impl<'w> WorldAccessGuard<'w> { inner: Rc::new(WorldAccessGuardInner { cell: world, accesses: AccessMap::new_subset(subset), + archetype_filter, type_registry, cached_slots: registry_cache, }), @@ -381,6 +388,35 @@ impl<'w> WorldAccessGuard<'w> { } } + /// Checks the guard's [`ArchetypeFilter`] allows accessing the component on the given entity. + /// + /// Must be checked before handing out references to components, on top of claiming the component's access. + /// Missing entities pass, as there is nothing to alias. + #[inline] + pub fn check_archetype_access( + &self, + entity: Entity, + component_id: ComponentId, + ) -> Result<(), DynWorldAccessError> { + let Some(filter) = &self.inner.archetype_filter else { + return Ok(()); + }; + // only reads entity metadata, which cannot change while a non-exclusive system runs + let Ok(entity_cell) = self.as_unsafe_world_cell()?.get_entity(entity) else { + return Ok(()); + }; + if filter.is_allowed(component_id, entity_cell.archetype().id()) { + Ok(()) + } else { + Err(DynWorldAccessError::cannot_claim_access( + component_id.into(), + format!( + "entity {entity} is not matched by any query of this system accessing the component" + ), + )) + } + } + /// Creates a new [`WorldAccessGuard`] for the given mutable borrow of the world. /// /// If these resources do not exist, they will be initialized. @@ -391,6 +427,7 @@ impl<'w> WorldAccessGuard<'w> { inner: Rc::new(WorldAccessGuardInner { cell: world.as_unsafe_world_cell(), accesses: map, + archetype_filter: None, cached_slots: registry_cache, type_registry, }), diff --git a/crates/bevy_system_reflection/src/lib.rs b/crates/bevy_system_reflection/src/lib.rs index cff937e760..2b1eb7d138 100644 --- a/crates/bevy_system_reflection/src/lib.rs +++ b/crates/bevy_system_reflection/src/lib.rs @@ -58,6 +58,18 @@ impl ReflectSystem { } } + /// Creates a reflect system from a system which is not in a schedule yet, so it has no node id + pub fn from_unscheduled_system( + system: &dyn System, + ) -> Self { + ReflectSystem { + name: system.name().into(), + type_id: system.system_type(), + node_id: ReflectNodeId(NodeId::System(SystemKey::default())), + default_system_sets: system.default_system_sets(), + } + } + /// gets the short identifier of the system, i.e. just the function name pub fn identifier(&self) -> &str { // if it contains generics it might contain more than diff --git a/crates/testing_crates/script_integration_test_harness/src/test_functions.rs b/crates/testing_crates/script_integration_test_harness/src/test_functions.rs index dd13d66654..2bc9a9fdd4 100644 --- a/crates/testing_crates/script_integration_test_harness/src/test_functions.rs +++ b/crates/testing_crates/script_integration_test_harness/src/test_functions.rs @@ -76,6 +76,20 @@ pub fn register_test_functions(world: &mut App) { }) }, ) + .register( + "_archetype_of", + |s: FunctionCallContext, entity: V| { + let world = s.world()?; + world + .with_world_access(|world| { + world + .get_entity(*entity) + .map(|entity| entity.archetype().id().index()) + .ok() + })? + .ok_or_else(|| InteropError::string(format!("missing entity: {}", *entity))) + }, + ) .register( "_assert_system_runs_before", |s: FunctionCallContext, diff --git a/docs/src/ReleaseNotes/0.23-to-0.24.md b/docs/src/ReleaseNotes/0.23-to-0.24.md index d55f08dd7a..d2be51fb19 100644 --- a/docs/src/ReleaseNotes/0.23-to-0.24.md +++ b/docs/src/ReleaseNotes/0.23-to-0.24.md @@ -62,6 +62,7 @@ Other changes: - `ScriptLoadingPipeline::time_budget: Option` is replaced by `tick_budget: Option`, the maximum amount of pipeline steps per frame, unlimited by default. `ActiveMachines::budget` and `RunProcessingPipelineOnce::new` take the same step count. - `ScriptAttachedMessage(a)` / `ScriptDetachedMessage(a)` are one message, `ScriptLifecycleMessage::Attached(a)` / `::Detached(a)`, so attachments and detachments are processed in the order they were written. `GetScriptHandle::get_script_handle` returns an `Option` (`None`: the event doesn't wait for its script to load) and `ScriptLifecycleQueue::get_ready`, formerly known as `LoadedWithHandles` yields `(event, Option<(StrongScriptHandle, Language)>)`. - An attachment whose script is still loading holds back the attachments and detachments after it, and callbacks reach scripts in a deterministic order (attachment order, until scripts are removed), see [Determinism](../ScriptPipeline/pipeline.md#determinism). Lua's `math.random` is seeded with `0` in new contexts. +- Script systems are identified by attachment and callback label (adding one again replaces it) and are removed with their script. `ScriptSystemSet` holds a recycled slot instead of the callback label, `ScriptSystemBuilder` no longer implements `IntoSystem` (use `ScriptSystemBuilder::build`), and their errors are sent as `ScriptErrorMessage`s instead of being logged. `WorldAccessGuard::new_non_exclusive` takes an `Option>`, restricting component accesses to the given archetypes (`None` for no restriction). - `WorldAccessRange::External` holds the value's owner (`AccessOwner`) instead of an id, and the access map keeps it alive until the claim is released. Build it from the value (`(&allocation_id).into()`); `WorldAccessRange` is no longer `Copy`. ### Language implementations diff --git a/docs/src/ScriptSystems/introduction.md b/docs/src/ScriptSystems/introduction.md index ee7a62272e..19136e2958 100644 --- a/docs/src/ScriptSystems/introduction.md +++ b/docs/src/ScriptSystems/introduction.md @@ -44,11 +44,11 @@ local world = import_namespace("bms").world world.add_system(update_schedule,system) ``` -

+A script system is identified by its attachment and callback label: adding it again replaces the old one (with a warning). Script systems are removed when their script unloads or reloads, a reloading script adds them again in `on_script_loaded`. -If your event handler running the script is running in a certain schedule, that schedule will be temporarilly removed by Bevy. Meaning you won't be able to modify it from within the script in-flight. +Bevy cannot change a schedule while it runs, i.e. `PreUpdate` where scripts are loaded. Systems added to (or removed from) a running schedule are added (or removed) once it finishes, removed systems stop running right away. `world.add_system` then returns a system which can be used for ordering, but has no node in the schedule yet. -
+Errors in script systems are sent as `ScriptErrorMessage`s, like those of other callbacks. ## Parameters @@ -83,6 +83,8 @@ This allows the system to access everything as in a normal event handler. Non-exclusive systems, will only be able to access the set of components and resources as parameterized when building the system. This is why we can run the system in parallel to other non-overlapping systems. +Components are only accessible on the entities matched by the queries declaring them, i.e. with `world.query():component(A):with(B)` the system can't access `A` on entities without `B`, since Bevy runs it in parallel with systems accessing `A` on those. Components only used as filters (`with`, `without`) can't be accessed. + Exclusive systems on the other hand, cannot run in parallel.