fix: script component not syncing correctly when modified

assets/tests/lifecycle/default/entity_script/loading/scenario.bmsscenario

@@ -1,3 +1,4 @@
+// #main_script lifecycle.lua
 SetCurrentLanguage language="@this_script_language"
 InstallPlugin emit_responses=true
 FinalizeApp
@@ -19,6 +20,22 @@ AssertCallbackSuccess attachment="EntityScript", entity="test_entity_a", label="
 AssertCallbackSuccess attachment="EntityScript", entity="test_entity_a", label="OnScriptReloaded", script="script_a", expect_string_value="reloaded with: unloaded!"
 AssertNoCallbackResponsesEmitted
 
+// insert second.lua into original entity
+LoadScriptAs as_name="script_b", path="second.lua"
+WaitForScriptAssetLoaded name="script_b"
+AddScriptToEntity name="test_entity_a", script="script_b"
+RunUpdateOnce
+AssertCallbackSuccess attachment="EntityScript", entity="test_entity_a", label="OnScriptLoaded", script="script_b", expect_string_value="2:loaded!"
+AssertNoCallbackResponsesEmitted
+AssertContextResidents attachment="EntityScript", script="script_a", entity="test_entity_a", residents_num=1
+AssertContextResidents attachment="EntityScript", script="script_b", entity="test_entity_a", residents_num=1
+
+// remove second.lua from original entity
+RemoveScriptFromEntity name="test_entity_a", script="script_b"
+RunUpdateOnce
+AssertCallbackSuccess attachment="EntityScript", entity="test_entity_a", label="OnScriptUnloaded", script="script_b", expect_string_value="2:unloaded!"
+AssertNoCallbackResponsesEmitted
+
 // now first drop the script asset, assert that does nothing yet 
 DropScriptAsset script="script_a"
 RunUpdateOnce

assets/tests/lifecycle/default/entity_script/loading/second.lua

@@ -0,0 +1,11 @@
+function on_script_loaded()
+    return "2:loaded!"
+end
+
+function on_script_unloaded()
+    return "2:unloaded!"
+end
+
+function on_script_reloaded(val)
+    return "2:reloaded with: " .. val
+end

crates/bevy_mod_scripting_core/src/lib.rs

@@ -9,8 +9,9 @@ use crate::{
     event::ScriptErrorEvent,
     handler::script_error_logger,
     pipeline::ScriptLoadingPipeline,
+    script::{ScriptComponentsChangeCache, script_component_changed_handler},
 };
-use bevy_app::{App, Plugin, PostUpdate};
+use bevy_app::{App, Plugin, PostUpdate, PreUpdate};
 use bevy_asset::{AssetApp, Handle};
 use bevy_ecs::schedule::IntoScheduleConfigs;
 use bevy_ecs::{
@@ -46,16 +47,11 @@ pub mod script_system;
 #[derive(SystemSet, Hash, Debug, Eq, PartialEq, Clone)]
 /// Labels for various BMS systems
 pub enum ScriptingSystemSet {
-    /// Systems which handle the processing of asset events for script assets, and dispatching internal script asset events
-    ScriptAssetDispatch,
-    /// Systems which read incoming internal script asset events and produce script lifecycle commands
-    ScriptCommandDispatch,
-    // /// Systems which read entity removal events and remove contexts associated with them
-    // EntityRemoval,
-    /// One time runtime initialization systems
-    RuntimeInitialization,
-    /// Systems which handle the garbage collection of allocated values
+    /// Systems in [`PostUpdate`] which handle the garbage collection of allocated values
     GarbageCollection,
+
+    /// Systems in [`PreUpdate`] dispatching pipeline events whenever handles are added or removed from [`ScriptComponent`]'s
+    SyncScriptingComponents,
 }
 
 /// Types which act like scripting plugins, by selecting a context and runtime
@@ -172,6 +168,7 @@ impl<P: IntoScriptPluginParams> Plugin for ScriptingPlugin<P> {
         P::set_world_local_config(app.world().id(), config);
 
         app.insert_resource(ScriptContexts::<P>::new(self.context_policy.clone()));
+        app.init_resource::<ScriptComponentsChangeCache>();
         app.register_asset_loader(ScriptAssetLoader::new(config.language_extensions));
 
         app.add_plugins((
@@ -355,6 +352,11 @@ impl Plugin for BMSScriptingInfrastructurePlugin {
             ((garbage_collector).in_set(ScriptingSystemSet::GarbageCollection),),
         );
 
+        app.add_systems(
+            PreUpdate,
+            script_component_changed_handler.in_set(ScriptingSystemSet::SyncScriptingComponents),
+        );
+
         if !self.dont_log_script_event_errors {
             app.add_systems(PostUpdate, script_error_logger);
         }

crates/bevy_mod_scripting_core/src/pipeline/mod.rs

@@ -18,7 +18,7 @@ use bevy_mod_scripting_display::DisplayProxy;
 use parking_lot::Mutex;
 
 use crate::{
-    IntoScriptPluginParams,
+    IntoScriptPluginParams, ScriptingSystemSet,
     context::ScriptingLoader,
     error::ScriptError,
     event::{
@@ -249,6 +249,7 @@ impl<P: IntoScriptPluginParams> Plugin for ScriptLoadingPipeline<P> {
             PreUpdate,
             PipelineSet::ListeningPhase
                 .after(bevy_asset::AssetTrackingSystems)
+                .after(ScriptingSystemSet::SyncScriptingComponents) // we want to pick up changes immediately
                 .before(PipelineSet::MachineStartPhase),
         );
 

crates/bevy_mod_scripting_core/src/script/mod.rs

@@ -17,7 +17,15 @@ use ::{
 
 mod context_key;
 mod script_context;
-use bevy_ecs::{component::Component, lifecycle::HookContext};
+use bevy_ecs::{
+    component::Component,
+    entity::EntityHashMap,
+    lifecycle::HookContext,
+    message::MessageWriter,
+    query::Changed,
+    system::{Query, ResMut},
+    world::Ref,
+};
 use bevy_log::trace;
 use bevy_mod_scripting_asset::ScriptAsset;
 use bevy_mod_scripting_script::ScriptAttachment;
@@ -29,7 +37,7 @@ pub use script_context::*;
 /// I.e. an asset with the path `path/to/asset.ext` will have the script id `path/to/asset.ext`
 pub type ScriptId = Handle<ScriptAsset>;
 
-#[derive(Component, Reflect, Clone, Default, Debug)]
+#[derive(Component, Reflect, Clone, Default, Debug, PartialEq, Eq)]
 #[reflect(Component)]
 #[component(on_remove=Self::on_remove, on_add=Self::on_add)]
 /// A component which identifies the scripts existing on an entity.
@@ -65,7 +73,13 @@ impl ScriptComponent {
     /// the removal of the script.
     pub fn on_remove(mut world: DeferredWorld, context: HookContext) {
         let context_keys = Self::get_context_keys_present(&world, context.entity);
+
         trace!("on remove hook for script components: {context_keys:?}");
+
+        if let Some(mut cache) = world.get_resource_mut::<ScriptComponentsChangeCache>() {
+            cache.last_values.remove(&context.entity);
+        }
+
         world.write_message_batch(context_keys.into_iter().map(ScriptDetachedEvent));
     }
 
@@ -74,10 +88,70 @@ impl ScriptComponent {
     pub fn on_add(mut world: DeferredWorld, context: HookContext) {
         let context_keys = Self::get_context_keys_present(&world, context.entity);
         trace!("on add hook for script components: {context_keys:?}");
+
+        if let Some(mut cache) = world.get_resource_mut::<ScriptComponentsChangeCache>() {
+            cache.last_values.insert(
+                context.entity,
+                context_keys.iter().map(|x| x.script().clone()).collect(),
+            );
+        }
+
         world.write_message_batch(context_keys.into_iter().map(ScriptAttachedEvent));
     }
 }
 
+/// Cache holding the last values of script components
+/// Allows the calculation of what handles have been added or removed since last frame.
+///
+/// Any handles in this cache are removed immediately when they are removed via the component
+#[derive(Resource, Default)]
+pub struct ScriptComponentsChangeCache {
+    last_values: EntityHashMap<HashSet<Handle<ScriptAsset>>>,
+}
+
+/// A system that handles pure modifications to a [`ScriptComponent`].
+///
+/// Other lifecycle events, such as addition and removal of these components are handled immediately via component hooks.
+pub fn script_component_changed_handler(
+    mut cache: ResMut<ScriptComponentsChangeCache>,
+    changed: Query<(Entity, Ref<ScriptComponent>), Changed<ScriptComponent>>,
+    mut attachment_messages: MessageWriter<ScriptAttachedEvent>,
+    mut detachment_messages: MessageWriter<ScriptDetachedEvent>,
+) {
+    for (entity, current_value) in changed {
+        if let Some(last_value) = cache.last_values.get_mut(&entity) {
+            let mut any_change = false;
+
+            // check removals
+            for old in last_value.iter() {
+                if !current_value.0.contains(old) {
+                    any_change = true;
+                    detachment_messages.write(ScriptDetachedEvent(ScriptAttachment::EntityScript(
+                        entity,
+                        old.clone(),
+                    )));
+                }
+            }
+
+            // check additions
+            for new in current_value.0.iter() {
+                if !last_value.contains(new) {
+                    any_change = true;
+                    attachment_messages.write(ScriptAttachedEvent(ScriptAttachment::EntityScript(
+                        entity,
+                        new.clone(),
+                    )));
+                }
+            }
+
+            if any_change {
+                last_value.clear();
+                last_value.extend(current_value.0.iter().cloned());
+            }
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use bevy_ecs::{message::Messages, world::World};

crates/testing_crates/bevy_mod_scripting_test_scenario_syntax/src/parse.rs

@@ -110,6 +110,20 @@ pub enum ScenarioStepSerialized {
         /// the script to spawn on the entity
         script: String,
     },
+    /// Pushes a script into the existing script component on an entity, or creates a new one and inserts the script
+    AddScriptToEntity {
+        /// the name of the entity to insert into
+        name: String,
+        /// the name of the script to insert
+        script: String,
+    },
+    /// Removes a script from the entity, asserting it exists
+    RemoveScriptFromEntity {
+        /// the name of the entity to remove from
+        name: String,
+        /// the name of the script to remove
+        script: String,
+    },
     /// Attaches a static script
     AttachStaticScript {
         /// the script to be attached

crates/testing_crates/bevy_mod_scripting_test_scenario_syntax/src/schema.rs

@@ -167,6 +167,28 @@ fn get_schema() -> ScenarioSchema {
                         .collect(),
                 },
             ),
+            (
+                "AddScriptToEntity".into(),
+                StepSchema {
+                    fields: vec![
+                        str_field("name", false, "the name of the script to insert"),
+                        str_field("script", false, "the name of the entity to insert into"),
+                    ]
+                    .into_iter()
+                    .collect(),
+                },
+            ),
+            (
+                "RemoveScriptFromEntity".into(),
+                StepSchema {
+                    fields: vec![
+                        str_field("name", false, "the name of the entity to remove from"),
+                        str_field("script", false, "the name of the script to remove"),
+                    ]
+                    .into_iter()
+                    .collect(),
+                },
+            ),
             (
                 "DetachStaticScript".into(),
                 StepSchema {

crates/testing_crates/script_integration_test_harness/src/scenario.rs

@@ -298,6 +298,18 @@ impl Scenario {
                     entity: name,
                 }
             }
+            ScenarioStepSerialized::AddScriptToEntity { name, script } => {
+                ScenarioStep::AddScriptToEntity {
+                    script: self.context.get_script_handle(&script)?,
+                    name: self.context.get_entity(&name)?,
+                }
+            }
+            ScenarioStepSerialized::RemoveScriptFromEntity { name, script } => {
+                ScenarioStep::RemoveScriptFromEntity {
+                    script: self.context.get_script_handle(&script)?,
+                    name: self.context.get_entity(&name)?,
+                }
+            }
             ScenarioStepSerialized::ReloadScriptFrom { script, path } => {
                 ScenarioStep::ReloadScriptFrom {
                     script: self.context.get_script_handle(&script)?,
@@ -569,6 +581,14 @@ pub enum ScenarioStep {
     SetNanosecondsBudget {
         nanoseconds_budget: Option<u64>,
     },
+    AddScriptToEntity {
+        script: Handle<ScriptAsset>,
+        name: Entity,
+    },
+    RemoveScriptFromEntity {
+        script: Handle<ScriptAsset>,
+        name: Entity,
+    },
 }
 
 /// Execution
@@ -1097,6 +1117,27 @@ impl ScenarioStep {
                     }
                 }
             }
+            ScenarioStep::AddScriptToEntity { script, name } => {
+                let world = app.world_mut();
+                world
+                    .entity_mut(name)
+                    .entry::<ScriptComponent>()
+                    .and_modify(|mut c| c.0.push(script.clone()))
+                    .or_insert_with(|| ScriptComponent(vec![script]));
+            }
+            ScenarioStep::RemoveScriptFromEntity { script, name } => {
+                let world = app.world_mut();
+                let mut entity = world.entity_mut(name);
+                let mut component = match entity.get_mut::<ScriptComponent>() {
+                    Some(component) => component,
+                    None => {
+                        return Err(anyhow!(
+                            "Expected script {script:?} to exist on entity {name}"
+                        ));
+                    }
+                };
+                component.0.retain(|c| c != &script);
+            }
         }
         Ok(())
     }