refactor!: Merge ScriptContexts<T> into Scripts<T> + Remove Sync bound from Contexts (#350)

* feat!: Merge `ScriptContexts<T>` into `Scripts` and store type erased context pointers

* parameterize scripts by the plugin type

* improve logs

Author: makspll

crates/bevy_mod_scripting_core/src/bindings/schedule.rs

@@ -334,7 +334,7 @@ impl ScriptSystemBuilder {
                     bevy::log::debug_once!("First call to script system {}", name);
                     match handler_ctxt.call_dynamic_label(
                         &name,
-                        self.script_id.clone(),
+                        &self.script_id,
                         Entity::from_raw(0),
                         vec![],
                         guard.clone(),

crates/bevy_mod_scripting_core/src/commands.rs

@@ -3,73 +3,17 @@
 use crate::{
     bindings::{ThreadWorldContainer, WorldContainer, WorldGuard},
     error::{InteropError, ScriptError},
-    script::{Script, ScriptId},
+    script::ScriptId,
     IntoScriptPluginParams,
 };
 use bevy::ecs::{entity::Entity, system::Resource};
-use std::{collections::HashMap, sync::atomic::AtomicU32};
 
 /// A trait that all script contexts must implement.
-pub trait Context: 'static + Send + Sync {}
-impl<T: 'static + Send + Sync> Context for T {}
-
-/// The type of a context id
-pub type ContextId = u32;
-
-/// Stores script state for a scripting plugin. Scripts are identified by their `ScriptId`, while contexts are identified by their `ContextId`.
-#[derive(Resource)]
-pub struct ScriptContexts<P: IntoScriptPluginParams> {
-    /// The contexts of the scripts
-    pub contexts: HashMap<ContextId, P::C>,
-}
-
-impl<P: IntoScriptPluginParams> Default for ScriptContexts<P> {
-    fn default() -> Self {
-        Self {
-            contexts: Default::default(),
-        }
-    }
-}
-
-static CONTEXT_ID_COUNTER: AtomicU32 = AtomicU32::new(0);
-impl<P: IntoScriptPluginParams> ScriptContexts<P> {
-    /// Allocates a new ContextId and inserts the context into the map
-    pub fn insert(&mut self, ctxt: P::C) -> ContextId {
-        let id = CONTEXT_ID_COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
-        self.contexts.insert(id, ctxt);
-        id
-    }
-
-    /// Inserts a context with a specific id
-    pub fn insert_with_id(&mut self, id: ContextId, ctxt: P::C) -> Option<P::C> {
-        self.contexts.insert(id, ctxt)
-    }
-
-    /// Allocate new context id without inserting a context
-    pub fn allocate_id(&self) -> ContextId {
-        CONTEXT_ID_COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed)
-    }
-
-    /// Removes a context from the map
-    pub fn remove(&mut self, id: ContextId) -> Option<P::C> {
-        self.contexts.remove(&id)
-    }
-
-    /// Get a reference to a context
-    pub fn get(&self, id: ContextId) -> Option<&P::C> {
-        self.contexts.get(&id)
-    }
-
-    /// Get a mutable reference to a context
-    pub fn get_mut(&mut self, id: ContextId) -> Option<&mut P::C> {
-        self.contexts.get_mut(&id)
-    }
-
-    /// Check if a context exists
-    pub fn contains(&self, id: ContextId) -> bool {
-        self.contexts.contains_key(&id)
-    }
-}
+///
+/// Contexts are not required to be `Sync` as they are internally stored behind a `Mutex` but they must satisfy `Send` so they can be
+/// freely sent between threads.
+pub trait Context: 'static + Send {}
+impl<T: 'static + Send> Context for T {}
 
 /// Initializer run once after creating a context but before executing it for the first time as well as after re-loading the script
 pub type ContextInitializer<P> =
@@ -85,7 +29,7 @@ pub struct ContextLoadingSettings<P: IntoScriptPluginParams> {
     /// Defines the strategy used to load and reload contexts
     pub loader: ContextBuilder<P>,
     /// Defines the strategy used to assign contexts to scripts
-    pub assigner: ContextAssigner<P>,
+    pub assignment_strategy: ContextAssignmentStrategy,
     /// Initializers run once after creating a context but before executing it for the first time
     pub context_initializers: Vec<ContextInitializer<P>>,
     /// Initializers run every time before executing or loading a script
@@ -96,7 +40,7 @@ impl<P: IntoScriptPluginParams> Default for ContextLoadingSettings<P> {
     fn default() -> Self {
         Self {
             loader: ContextBuilder::default(),
-            assigner: ContextAssigner::default(),
+            assignment_strategy: Default::default(),
             context_initializers: Default::default(),
             context_pre_handling_initializers: Default::default(),
         }
@@ -107,7 +51,7 @@ impl<T: IntoScriptPluginParams> Clone for ContextLoadingSettings<T> {
     fn clone(&self) -> Self {
         Self {
             loader: self.loader.clone(),
-            assigner: self.assigner.clone(),
+            assignment_strategy: self.assignment_strategy,
             context_initializers: self.context_initializers.clone(),
             context_pre_handling_initializers: self.context_pre_handling_initializers.clone(),
         }
@@ -119,7 +63,7 @@ pub type ContextLoadFn<P> = fn(
     content: &[u8],
     context_initializers: &[ContextInitializer<P>],
     pre_handling_initializers: &[ContextPreHandlingInitializer<P>],
-    runtime: &mut <P as IntoScriptPluginParams>::R,
+    runtime: &<P as IntoScriptPluginParams>::R,
 ) -> Result<<P as IntoScriptPluginParams>::C, ScriptError>;
 
 /// A strategy for reloading contexts
@@ -129,7 +73,7 @@ pub type ContextReloadFn<P> = fn(
     previous_context: &mut <P as IntoScriptPluginParams>::C,
     context_initializers: &[ContextInitializer<P>],
     pre_handling_initializers: &[ContextPreHandlingInitializer<P>],
-    runtime: &mut <P as IntoScriptPluginParams>::R,
+    runtime: &<P as IntoScriptPluginParams>::R,
 ) -> Result<(), ScriptError>;
 
 /// A strategy for loading and reloading contexts
@@ -160,7 +104,7 @@ impl<P: IntoScriptPluginParams> ContextBuilder<P> {
         context_initializers: &[ContextInitializer<P>],
         pre_handling_initializers: &[ContextPreHandlingInitializer<P>],
         world: WorldGuard,
-        runtime: &mut P::R,
+        runtime: &P::R,
     ) -> Result<P::C, ScriptError> {
         WorldGuard::with_existing_static_guard(world.clone(), |world| {
             ThreadWorldContainer.set_world(world)?;
@@ -183,7 +127,7 @@ impl<P: IntoScriptPluginParams> ContextBuilder<P> {
         context_initializers: &[ContextInitializer<P>],
         pre_handling_initializers: &[ContextPreHandlingInitializer<P>],
         world: WorldGuard,
-        runtime: &mut P::R,
+        runtime: &P::R,
     ) -> Result<(), ScriptError> {
         WorldGuard::with_existing_static_guard(world, |world| {
             ThreadWorldContainer.set_world(world)?;
@@ -208,108 +152,12 @@ impl<P: IntoScriptPluginParams> Clone for ContextBuilder<P> {
     }
 }
 
-/// A strategy for assigning contexts to new and existing but re-loaded scripts as well as for managing old contexts
-pub struct ContextAssigner<P: IntoScriptPluginParams> {
-    /// Assign a context to the script.
-    /// The assigner can either return `Some(id)` or `None`.
-    /// Returning None will request the processor to assign a new context id to assign to this script.
-    ///
-    /// Regardless, whether a script gets a new context id or not, the processor will check if the given context exists.
-    /// If it does not exist, it will create a new context and assign it to the script.
-    /// If it does exist, it will NOT create a new context, but assign the existing one to the script, and re-load the context.
-    ///
-    /// This function is only called once for each script, when it is loaded for the first time.
-    pub assign: fn(
-        script_id: &ScriptId,
-        new_content: &[u8],
-        contexts: &ScriptContexts<P>,
-    ) -> Option<ContextId>,
-
-    /// Handle the removal of the script, if any clean up in contexts is necessary perform it here.
-    ///
-    /// If you do not clean up the context here, it will stay in the context map!
-    pub remove: fn(context_id: ContextId, script: &Script, contexts: &mut ScriptContexts<P>),
-}
-
-impl<P: IntoScriptPluginParams> ContextAssigner<P> {
-    /// Create an assigner which re-uses a single global context for all scripts, only use if you know what you're doing.
-    /// Will not perform any clean up on removal.
-    pub fn new_global_context_assigner() -> Self {
-        Self {
-            assign: |_, _, _| Some(0), // always use the same id in rotation
-            remove: |_, _, _| {},      // do nothing
-        }
-    }
-
-    /// Create an assigner which assigns a new context to each script. This is the default strategy.
-    pub fn new_individual_context_assigner() -> Self {
-        Self {
-            assign: |_, _, _| None,
-            remove: |id, _, c| _ = c.remove(id),
-        }
-    }
-}
-
-impl<P: IntoScriptPluginParams> Default for ContextAssigner<P> {
-    fn default() -> Self {
-        Self::new_individual_context_assigner()
-    }
-}
-
-impl<P: IntoScriptPluginParams> Clone for ContextAssigner<P> {
-    fn clone(&self) -> Self {
-        Self {
-            assign: self.assign,
-            remove: self.remove,
-        }
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use crate::asset::Language;
-
-    use super::*;
-
-    struct DummyParams;
-    impl IntoScriptPluginParams for DummyParams {
-        type C = String;
-        type R = ();
-
-        const LANGUAGE: Language = Language::Lua;
-
-        fn build_runtime() -> Self::R {}
-    }
-
-    #[test]
-    fn test_script_contexts_insert_get() {
-        let mut contexts: ScriptContexts<DummyParams> = ScriptContexts::default();
-        let id = contexts.insert("context1".to_string());
-        assert_eq!(contexts.contexts.get(&id), Some(&"context1".to_string()));
-        assert_eq!(
-            contexts.contexts.get_mut(&id),
-            Some(&mut "context1".to_string())
-        );
-    }
-
-    #[test]
-    fn test_script_contexts_allocate_id() {
-        let contexts: ScriptContexts<DummyParams> = ScriptContexts::default();
-        let id = contexts.allocate_id();
-        let next_id = contexts.allocate_id();
-        assert_eq!(next_id, id + 1);
-    }
-
-    #[test]
-    fn test_script_contexts_remove() {
-        let mut contexts: ScriptContexts<DummyParams> = ScriptContexts::default();
-        let id = contexts.insert("context1".to_string());
-        let removed = contexts.remove(id);
-        assert_eq!(removed, Some("context1".to_string()));
-        assert!(!contexts.contexts.contains_key(&id));
-
-        // assert next id is still incremented
-        let next_id = contexts.allocate_id();
-        assert_eq!(next_id, id + 1);
-    }
+/// The strategy used in assigning contexts to scripts
+#[derive(Default, Clone, Copy)]
+pub enum ContextAssignmentStrategy {
+    /// Assign a new context to each script
+    #[default]
+    Individual,
+    /// Share contexts with all other scripts
+    Global,
 }

crates/bevy_mod_scripting_core/src/context.rs

@@ -8,7 +8,6 @@ use crate::{
         script_value::ScriptValue,
         ReflectBaseType, ReflectReference,
     },
-    context::ContextId,
     script::ScriptId,
 };
 use bevy::{
@@ -599,9 +598,8 @@ impl InteropError {
     }
 
     /// Thrown if the required context for an operation is missing.
-    pub fn missing_context(context_id: ContextId, script_id: impl Into<ScriptId>) -> Self {
+    pub fn missing_context(script_id: impl Into<ScriptId>) -> Self {
         Self(Arc::new(InteropErrorInner::MissingContext {
-            context_id,
             script_id: script_id.into(),
         }))
     }
@@ -812,8 +810,6 @@ pub enum InteropErrorInner {
     },
     /// Thrown if the required context for an operation is missing.
     MissingContext {
-        /// The context that was missing
-        context_id: ContextId,
         /// The script that was attempting to access the context
         script_id: ScriptId,
     },
@@ -1053,15 +1049,9 @@ impl PartialEq for InteropErrorInner {
                 },
             ) => a == c && b == d,
             (
-                InteropErrorInner::MissingContext {
-                    context_id: a,
-                    script_id: b,
-                },
-                InteropErrorInner::MissingContext {
-                    context_id: c,
-                    script_id: d,
-                },
-            ) => a == c && b == d,
+                InteropErrorInner::MissingContext { script_id: b },
+                InteropErrorInner::MissingContext { script_id: d },
+            ) => b == d,
             (
                 InteropErrorInner::MissingSchedule { schedule_name: a },
                 InteropErrorInner::MissingSchedule { schedule_name: b },
@@ -1284,10 +1274,10 @@ macro_rules! argument_count_mismatch_msg {
 }
 
 macro_rules! missing_context_for_callback {
-    ($context_id:expr, $script_id:expr) => {
+    ($script_id:expr) => {
         format!(
-            "Missing context with id: {} for script with id: {}. Was the script loaded?.",
-            $context_id, $script_id
+            "Missing context for script with id: {}. Was the script loaded?.",
+            $script_id
         )
     };
 }
@@ -1433,9 +1423,8 @@ impl DisplayWithWorld for InteropErrorInner {
             InteropErrorInner::MissingScript { script_id } => {
                 missing_script_for_callback!(script_id)
             },
-            InteropErrorInner::MissingContext { context_id, script_id } => {
+            InteropErrorInner::MissingContext { script_id } => {
                 missing_context_for_callback!(
-                    context_id,
                     script_id
                 )
             },
@@ -1580,9 +1569,8 @@ impl DisplayWithWorld for InteropErrorInner {
             InteropErrorInner::MissingScript { script_id } => {
                 missing_script_for_callback!(script_id)
             },
-            InteropErrorInner::MissingContext { context_id, script_id } => {
+            InteropErrorInner::MissingContext { script_id } => {
                 missing_context_for_callback!(
-                    context_id,
                     script_id
                 )
             },