feat: read only script system parameters (+ access declaration fixes)

assets/tests/add_system/read_only_parameters_cannot_be_written.lua

@@ -0,0 +1,52 @@
+-- parameters declared with `resource_read` / `component_read` are declared as reads to bevy,
+-- so the system may read but must not write them
+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::TestResourceWithVariousFields")
+local ComponentA = world.get_type_by_name("test_utils::test_data::CompWithFromWorldAndComponentData")
+
+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 entity = world.spawn()
+    world.add_default_component(entity, ComponentA)
+
+    world.add_system(
+        post_update_schedule,
+        bms.system_builder("my_reading_system", script_attachment)
+        :resource_read(ResourceType)
+        :query(world.query():component_read(ComponentA))
+    )
+
+    return true
+end
+
+function my_reading_system(resource, query)
+    runs[#runs + 1] = "my_reading_system"
+    local ok, err = pcall(check_accesses, resource, query)
+    runs[#runs + 1] = ok and "finished" or tostring(err)
+end
+
+function check_accesses(resource, query)
+    assert(resource.string == "Initial Value", "Expected 'Initial Value', got: " .. resource.string)
+    assert(#query >= 1, "Expected results, got: " .. #query)
+    local component = query[1]:components()[1]
+    assert(type(component[1]) == "string", "Expected to read the component, got: " .. tostring(component[1]))
+
+    local ok, err = pcall(function() resource.string = "changed" end)
+    assert(not ok and tostring(err):find("annot claim access"), "resource write: " .. tostring(err))
+    ok, err = pcall(function() component[1] = "changed" end)
+    assert(not ok and tostring(err):find("annot claim access"), "component write: " .. tostring(err))
+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

crates/bevy_mod_scripting_bindings/src/query.rs

@@ -407,6 +407,22 @@ impl std::fmt::Display for ScriptTypeRegistration {
     }
 }
 
+/// How [`ScriptQueryBuilder::as_query_state`] declares the accesses of the query.
+///
+/// Read and written components match the same entities, the difference is only in the access the query reports.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum QueryAccess {
+    /// Every component is declared as read, for finding the matching entities, i.e. with
+    /// [`bevy_ecs::query::QueryState::iter`] on a `&World`, while the actual component accesses are claimed through
+    /// the world guard.
+    ///
+    /// The query state must not be used to write components, or have its access reported to the scheduler.
+    MatchOnly,
+    /// The written components are declared as written, for queries whose access is reported to the scheduler,
+    /// i.e. the queries of script systems.
+    Declared,
+}
+
 #[derive(Clone, Default, Reflect)]
 #[reflect(opaque)]
 /// The query builder is used to build ECS queries which retrieve spefific components filtered by specific conditions.
@@ -431,6 +447,8 @@ impl std::fmt::Display for ScriptTypeRegistration {
 pub struct ScriptQueryBuilder {
     /// The components to query for.
     pub components: Vec<ScriptComponentRegistration>,
+    /// Whether each of the components is written, components without an entry are written.
+    writes: Vec<bool>,
     /// Components that must be present.
     pub with: Vec<ScriptComponentRegistration>,
     /// Components that must not be present.
@@ -444,17 +462,38 @@ impl ScriptQueryBuilder {
         Default::default()
     }
 
-    /// Adds components to the query.
+    /// Adds components to the query, which can be read and written.
     pub fn components(&mut self, components: Vec<ScriptComponentRegistration>) -> &mut Self {
-        self.components.extend(components);
+        for component in components {
+            self.component(component);
+        }
         self
     }
-    /// Adds a component to the query.
+    /// Adds a component to the query, which can be read and written.
     pub fn component(&mut self, component: ScriptComponentRegistration) -> &mut Self {
+        self.push_component(component, true)
+    }
+
+    /// Adds a component to the query, which can only be read.
+    ///
+    /// Systems reading a component can run in parallel with each other.
+    pub fn component_read(&mut self, component: ScriptComponentRegistration) -> &mut Self {
+        self.push_component(component, false)
+    }
+
+    fn push_component(&mut self, component: ScriptComponentRegistration, write: bool) -> &mut Self {
+        // components pushed to the public field directly are written
+        self.writes.resize(self.components.len(), true);
         self.components.push(component);
+        self.writes.push(write);
         self
     }
 
+    /// Returns true if the component at the given index of [`Self::components`] is written
+    pub fn is_written(&self, index: usize) -> bool {
+        self.writes.get(index).copied().unwrap_or(true)
+    }
+
     /// Adds components to the query that must be present.
     pub fn with_components(&mut self, with: Vec<ScriptComponentRegistration>) -> &mut Self {
         self.with.extend(with);
@@ -479,14 +518,21 @@ impl ScriptQueryBuilder {
         self
     }
 
-    /// Builds the query into a query state as used in systems.
-    pub fn as_query_state<Q: QueryData>(&self, world: &mut World) -> QueryState<Q> {
+    /// Builds the query into a query state, declaring its component accesses as given by `access`.
+    pub fn as_query_state<Q: QueryData>(
+        &self,
+        world: &mut World,
+        access: QueryAccess,
+    ) -> QueryState<Q> {
         let mut dynamic_query = QueryBuilder::<Q>::new(world);
         // we don't actually want to fetch the data for components now, only figure out
-        // which entities match the query
-        // so we might be being slightly overkill
-        for c in &self.components {
-            dynamic_query.ref_id(c.component_id());
+        // which entities match the query, and declare the access
+        for (index, c) in self.components.iter().enumerate() {
+            if access == QueryAccess::Declared && self.is_written(index) {
+                dynamic_query.mut_id(c.component_id());
+            } else {
+                dynamic_query.ref_id(c.component_id());
+            }
         }
 
         for w in &self.with {

crates/bevy_mod_scripting_bindings/src/world_extensions.rs

@@ -509,7 +509,8 @@ impl<'w> WorldExtensions for WorldAccessGuard<'w> {
         query: crate::ScriptQueryBuilder,
     ) -> Result<VecDeque<ScriptQueryResult>, InteropError> {
         self.with_world_mut(|world| {
-            let mut built_query = query.as_query_state::<EntityRef>(world);
+            let mut built_query =
+                query.as_query_state::<EntityRef>(world, crate::QueryAccess::MatchOnly);
             let query_result = built_query.iter(world);
 
             Ok(query_result

crates/bevy_mod_scripting_core/src/extractors.rs

@@ -143,12 +143,12 @@ pub(crate) fn get_all_access_ids(access: &Access) -> Vec<(ComponentId, bool)> {
     let mut access_all_write = Access::default();
     access_all_write.write_all();
 
-    // read conflicts with each set to figure out the necessary locks
+    // read conflicts with each set to figure out the necessary locks:
+    // only writes conflict with reading everything, reads and writes conflict with writing everything
+    let written = individual_conflicts(access.get_conflicts(&access_all_read));
+    let mut read = individual_conflicts(access.get_conflicts(&access_all_write));
 
-    let mut read = individual_conflicts(access.get_conflicts(&access_all_read));
-    let written = individual_conflicts(access.get_conflicts(&access_all_write));
-
-    // remove reads from writes
+    // remove writes from reads
     read.difference_with(&written);
 
     let mut result = Vec::new();

crates/bevy_mod_scripting_core/src/script_system.rs

@@ -14,7 +14,7 @@ use ::{
     bevy_ecs::{
         component::ComponentId,
         entity::Entity,
-        query::{FilteredAccess, FilteredAccessSet, QueryState},
+        query::{FilteredAccessSet, QueryState},
         reflect::AppTypeRegistry,
         schedule::SystemSet,
         system::{System, SystemParamValidationError},
@@ -38,7 +38,7 @@ use bevy_log::{debug, warn, warn_once};
 use bevy_mod_scripting_bindings::{
     AppScheduleRegistry, AppScriptCallbackRunners, AppScriptComponentRegistry,
     AppScriptFunctionRegistry, AppScriptTypeCatalogue, CurrentScriptAttachment, InteropError,
-    IntoScript, ReflectReference, ScriptQueryBuilder, ScriptQueryResult,
+    IntoScript, QueryAccess, ReflectReference, ScriptQueryBuilder, ScriptQueryResult,
     ScriptResourceRegistration, V, WorldExtensions,
 };
 use bevy_mod_scripting_display::DisplayProxy;
@@ -89,7 +89,10 @@ impl SystemSet for ScriptSystemSet {
 
 #[derive(Clone)]
 enum ScriptSystemParamDescriptor {
-    Res(ScriptResourceRegistration),
+    Res {
+        resource: ScriptResourceRegistration,
+        write: bool,
+    },
     EntityQuery(ScriptQueryBuilder),
 }
 
@@ -126,10 +129,21 @@ impl ScriptSystemBuilder {
         self
     }
 
-    /// Adds a resource access to the system
+    /// Adds a resource access to the system, the resource can be read and written
     pub fn resource(&mut self, resource: ScriptResourceRegistration) -> &mut Self {
-        self.system_params
-            .push(ScriptSystemParamDescriptor::Res(resource));
+        self.system_params.push(ScriptSystemParamDescriptor::Res {
+            resource,
+            write: true,
+        });
+        self
+    }
+
+    /// Adds a read only resource access to the system, systems reading a resource can run in parallel with each other
+    pub fn resource_read(&mut self, resource: ScriptResourceRegistration) -> &mut Self {
+        self.system_params.push(ScriptSystemParamDescriptor::Res {
+            resource,
+            write: false,
+        });
         self
     }
 
@@ -847,12 +861,17 @@ impl<P: IntoScriptPluginParams> System for DynamicScriptSystem<P> {
 
         // start with resources
         let mut subset = HashSet::<ComponentId>::new();
+        // the accesses in `subset` which can be written, the others are read only
+        let mut written = HashSet::<ComponentId>::new();
         let mut system_params = Vec::with_capacity(self.system_param_descriptors.len());
         let mut component_access_set = FilteredAccessSet::new();
         let mut initialization_errors = Vec::new();
         for param in &self.system_param_descriptors {
             match param {
-                ScriptSystemParamDescriptor::Res(res) => {
+                ScriptSystemParamDescriptor::Res {
+                    resource: res,
+                    write,
+                } => {
                     let component_id = res.resource_id;
                     let type_id = res.type_registration().type_id();
 
@@ -862,10 +881,13 @@ impl<P: IntoScriptPluginParams> System for DynamicScriptSystem<P> {
                     };
                     system_params.push(system_param);
 
-                    let mut access = FilteredAccess::matches_nothing();
-
-                    access.add_write(component_id);
-                    component_access_set.add(access);
+                    // like `Res`/`ResMut`, an access matching nothing would be compatible with any other access
+                    if *write {
+                        component_access_set.add_resource_write(component_id);
+                        written.insert(component_id);
+                    } else {
+                        component_access_set.add_resource_read(component_id);
+                    }
                     if subset.contains(&component_id) {
                         initialization_errors.push(format!(
                             "Duplicate resource access in system: {component_id:?}."
@@ -879,18 +901,22 @@ impl<P: IntoScriptPluginParams> System for DynamicScriptSystem<P> {
                         .iter()
                         .map(|c| (c.component_id, c.type_registration().type_id()))
                         .collect();
-                    let query = query.as_query_state::<Entity>(world);
+                    let query = query.as_query_state::<Entity>(world, QueryAccess::Declared);
 
                     // 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<A>` vs `Without<A>`).
                     // 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())
-                        .into_iter()
-                        .map(|(a, _)| a)
-                        .collect::<HashSet<_>>();
+                    let accesses = get_all_access_ids(query.component_access().access());
+                    written.extend(
+                        accesses
+                            .iter()
+                            .filter(|(_, write)| *write)
+                            .map(|(id, _)| *id),
+                    );
+                    let new_raids = accesses.into_iter().map(|(a, _)| a).collect::<HashSet<_>>();
 
                     if !subset.is_disjoint(&new_raids) {
                         initialization_errors
@@ -906,8 +932,15 @@ impl<P: IntoScriptPluginParams> System for DynamicScriptSystem<P> {
             }
         }
 
-        let final_subset =
-            AccessByteSet::from_allowed_list(&subset.iter().map(|c| c.index()).collect::<Vec<_>>());
+        // the guard must not allow writes which are declared to bevy as reads
+        let (read_write, read_only): (Vec<_>, Vec<_>) = subset
+            .iter()
+            .map(|c| (c.index(), written.contains(c)))
+            .partition(|(_, write)| *write);
+        let final_subset = AccessByteSet::from_allowed_lists(
+            &read_write.into_iter().map(|(c, _)| c).collect::<Vec<_>>(),
+            &read_only.into_iter().map(|(c, _)| c).collect::<Vec<_>>(),
+        );
 
         self.state = Some(ScriptSystemState {
             type_registry: world.get_resource_or_init::<AppTypeRegistry>().clone().0,
@@ -1077,7 +1110,9 @@ mod test {
             schedule::{ScheduleLabel, Schedules},
         },
     };
-    use bevy_mod_scripting_bindings::ScriptValue;
+    use bevy_mod_scripting_bindings::{
+        ScriptComponentRegistration, ScriptTypeRegistration, ScriptValue,
+    };
     use test_utils::make_test_plugin;
 
     use crate::{
@@ -1291,4 +1326,61 @@ mod test {
             1
         );
     }
+
+    #[derive(bevy_ecs::resource::Resource, Reflect, Default)]
+    struct AccessTestResource;
+
+    #[derive(bevy_ecs::component::Component, Reflect)]
+    struct AccessTestComponent;
+
+    fn access_of(world: &mut World, f: impl FnOnce(&mut ScriptSystemBuilder)) -> FilteredAccessSet {
+        let mut builder = ScriptSystemBuilder::new("test".to_owned().into(), attachment());
+        f(&mut builder);
+        let mut system = DynamicScriptSystem::<TestPlugin>::new(
+            builder,
+            ScriptSystemSet(0),
+            Arc::new(AtomicBool::new(true)),
+            ScriptSystems::default(),
+        );
+        match system.initialize(world) {
+            SystemAccess::Shared(access) => access,
+            _ => panic!("expected a non-exclusive system"),
+        }
+    }
+
+    #[test]
+    fn read_only_parameters_are_declared_as_reads() {
+        let mut world = World::new();
+        let resource = ScriptResourceRegistration::new(
+            ScriptTypeRegistration::new(Arc::new(bevy_reflect::TypeRegistration::of::<
+                AccessTestResource,
+            >())),
+            world.init_resource::<AccessTestResource>(),
+        );
+        let component = ScriptComponentRegistration::new(
+            ScriptTypeRegistration::new(Arc::new(bevy_reflect::TypeRegistration::of::<
+                AccessTestComponent,
+            >())),
+            world.register_component::<AccessTestComponent>(),
+        );
+        let reader = |builder: &mut ScriptSystemBuilder| {
+            let mut query = ScriptQueryBuilder::new();
+            query.component_read(component.clone());
+            builder.resource_read(resource.clone()).query(query);
+        };
+        let resource_writer = |builder: &mut ScriptSystemBuilder| {
+            builder.resource(resource.clone());
+        };
+        let component_writer = |builder: &mut ScriptSystemBuilder| {
+            let mut query = ScriptQueryBuilder::new();
+            query.component(component.clone());
+            builder.query(query);
+        };
+
+        let a = access_of(&mut world, reader);
+        let b = access_of(&mut world, reader);
+        assert!(a.is_compatible(&b), "readers should run in parallel");
+        assert!(!a.is_compatible(&access_of(&mut world, resource_writer)));
+        assert!(!a.is_compatible(&access_of(&mut world, component_writer)));
+    }
 }