Rework setting while active to be slightly less hacky.

src/dynamic_data/libraries/dynamic_library.rs

@@ -8,6 +8,7 @@ use crate::static_data::items::item::Item;
 use crate::static_data::libraries::static_data::StaticData;
 use crate::{PkmnResult, StringKey};
 use std::ops::Deref;
+use std::sync::Arc;
 
 #[derive(Debug)]
 pub struct DynamicLibrary {
@@ -49,7 +50,7 @@ impl DynamicLibrary {
         self.misc_library.deref()
     }
 
-    pub fn load_script(&self, _category: ScriptCategory, _key: &StringKey) -> PkmnResult<Option<Box<dyn Script>>> {
+    pub fn load_script(&self, _category: ScriptCategory, _key: &StringKey) -> PkmnResult<Option<Arc<dyn Script>>> {
         todo!()
     }
     pub fn load_item_script(&self, _key: &Item) -> PkmnResult<Option<Box<dyn ItemScript>>> {

src/dynamic_data/models/battle.rs

@@ -292,7 +292,7 @@ impl<'own, 'library> VolatileScripts<'own> for Battle<'own, 'library> {
         &self.volatile_scripts
     }
 
-    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Box<dyn Script>>> {
+    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Arc<dyn Script>>> {
         self.library.load_script(ScriptCategory::Battle, key)
     }
 }

src/dynamic_data/models/battle_side.rs

@@ -258,7 +258,7 @@ impl<'own, 'library> VolatileScripts<'own> for BattleSide<'own, 'library> {
         &self.volatile_scripts
     }
 
-    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Box<dyn Script>>> {
+    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Arc<dyn Script>>> {
         self.battle().library().load_script(crate::ScriptCategory::Side, key)
     }
 }

src/dynamic_data/models/pokemon.rs

@@ -395,15 +395,10 @@ impl<'own, 'library> Pokemon<'own, 'library> {
             .load_script(ScriptCategory::Ability, self.active_ability().name())
             .unwrap();
         if let Some(ability_script) = ability_script {
-            self.ability_script.set(ability_script);
+            self.ability_script
+                .set(ability_script)
+                .as_ref()
                 // Ensure the ability script gets initialized with the parameters for the ability.
-            self.ability_script()
-                .get()
-                .unwrap()
-                .read()
-                .as_ref()
-                .unwrap()
-                .as_ref()
                 .on_initialize(self.active_ability().parameters())
         } else {
             self.ability_script.clear();
@@ -581,7 +576,7 @@ impl<'own, 'library> VolatileScripts<'own> for Pokemon<'own, 'library> {
         &self.volatile
     }
 
-    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Box<dyn Script>>> {
+    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Arc<dyn Script>>> {
         self.library.load_script(ScriptCategory::Pokemon, key)
     }
 }

src/dynamic_data/script_handling/mod.rs

@@ -99,7 +99,7 @@ pub trait ScriptSource<'a> {
 
 #[derive(Debug)]
 pub enum ScriptWrapper {
-    Script(Weak<RwLock<Option<Box<dyn Script>>>>),
+    Script(Weak<RwLock<Option<Arc<dyn Script>>>>),
     Set(Weak<RwLock<ScriptSet>>),
 }
 
@@ -263,7 +263,7 @@ mod tests {
 
     #[test]
     fn script_aggregator_property_iterates_single_script() {
-        let script = ScriptContainer::new(Box::new(TestScript::new()));
+        let script = ScriptContainer::new(Arc::new(TestScript::new()));
         let scripts = vec![ScriptWrapper::from(&script)];
         let mut aggregator = ScriptAggregator::new(&scripts as *const Vec<ScriptWrapper>);
         while let Some(v) = aggregator.get_next() {
@@ -275,7 +275,7 @@ mod tests {
 
     #[test]
     fn script_aggregator_property_iterates_single_script_with_resets() {
-        let script = ScriptContainer::new(Box::new(TestScript::new()));
+        let script = ScriptContainer::new(Arc::new(TestScript::new()));
         let scripts = vec![ScriptWrapper::from(&script)];
         let mut aggregator = ScriptAggregator::new(&scripts as *const Vec<ScriptWrapper>);
         for i in 1..11 {
@@ -290,9 +290,9 @@ mod tests {
 
     #[test]
     fn script_aggregator_property_iterates_three_script() {
-        let script1 = ScriptContainer::new(Box::new(TestScript::new()));
+        let script1 = ScriptContainer::new(Arc::new(TestScript::new()));
-        let script2 = ScriptContainer::new(Box::new(TestScript::new()));
+        let script2 = ScriptContainer::new(Arc::new(TestScript::new()));
-        let script3 = ScriptContainer::new(Box::new(TestScript::new()));
+        let script3 = ScriptContainer::new(Arc::new(TestScript::new()));
         let scripts = vec![
             ScriptWrapper::from(&script1),
             ScriptWrapper::from(&script2),
@@ -312,9 +312,9 @@ mod tests {
 
     #[test]
     fn script_aggregator_property_iterates_three_script_with_resets() {
-        let script1 = ScriptContainer::new(Box::new(TestScript::new()));
+        let script1 = ScriptContainer::new(Arc::new(TestScript::new()));
-        let script2 = ScriptContainer::new(Box::new(TestScript::new()));
+        let script2 = ScriptContainer::new(Arc::new(TestScript::new()));
-        let script3 = ScriptContainer::new(Box::new(TestScript::new()));
+        let script3 = ScriptContainer::new(Arc::new(TestScript::new()));
         let scripts = vec![
             ScriptWrapper::from(&script1),
             ScriptWrapper::from(&script2),
@@ -339,9 +339,9 @@ mod tests {
     fn script_aggregator_property_iterates_script_set() {
         let set = Arc::new(RwLock::new(ScriptSet::default()));
         let mut mut_set = set.write();
-        mut_set.add(Box::new(TestScript::new_with_name("test_a")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_a")));
-        mut_set.add(Box::new(TestScript::new_with_name("test_b")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_b")));
-        mut_set.add(Box::new(TestScript::new_with_name("test_c")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_c")));
         // Drop so we don't have a lock on it anymore.
         drop(mut_set);
 
@@ -369,9 +369,9 @@ mod tests {
     fn script_aggregator_property_iterates_script_set_when_removing_last() {
         let set = Arc::new(RwLock::new(ScriptSet::default()));
         let mut mut_set = set.write();
-        mut_set.add(Box::new(TestScript::new_with_name("test_a")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_a")));
-        mut_set.add(Box::new(TestScript::new_with_name("test_b")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_b")));
-        mut_set.add(Box::new(TestScript::new_with_name("test_c")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_c")));
         // Drop so we don't have a lock on it anymore.
         drop(mut_set);
 
@@ -415,9 +415,9 @@ mod tests {
     fn script_aggregator_property_iterates_script_set_when_removing_middle() {
         let set = Arc::new(RwLock::new(ScriptSet::default()));
         let mut mut_set = set.write();
-        mut_set.add(Box::new(TestScript::new_with_name("test_a")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_a")));
-        mut_set.add(Box::new(TestScript::new_with_name("test_b")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_b")));
-        mut_set.add(Box::new(TestScript::new_with_name("test_c")));
+        mut_set.add(Arc::new(TestScript::new_with_name("test_c")));
         // Drop so we don't have a lock on it anymore.
         drop(mut_set);
 
@@ -491,7 +491,7 @@ mod tests {
         aggregator.reset();
         assert!(aggregator.get_next().is_none());
         aggregator.reset();
-        source.script.set(Box::new(TestScript::new()));
+        source.script.set(Arc::new(TestScript::new()));
         assert!(aggregator.get_next().is_some());
     }
 
@@ -503,7 +503,7 @@ mod tests {
         };
 
         let mut aggregator = source.get_script_iterator();
-        source.script.set(Box::new(TestScript::new()));
+        source.script.set(Arc::new(TestScript::new()));
         assert!(aggregator.get_next().is_some());
         aggregator.reset();
         source.script.clear();

src/dynamic_data/script_handling/script.rs

@@ -142,7 +142,7 @@ impl Debug for dyn Script {
     }
 }
 
-type ScriptHolder = Arc<RwLock<Option<Box<dyn Script>>>>;
+type ScriptHolder = Arc<RwLock<Option<Arc<dyn Script>>>>;
 
 #[derive(Default, Debug, Clone)]
 pub struct ScriptContainer {
@@ -150,7 +150,7 @@ pub struct ScriptContainer {
 }
 
 impl ScriptContainer {
-    pub fn new(script: Box<dyn Script>) -> ScriptContainer {
+    pub fn new(script: Arc<dyn Script>) -> ScriptContainer {
         Self {
             script: Arc::new(RwLock::new(Some(script))),
         }
@@ -167,21 +167,20 @@ impl ScriptContainer {
         }
     }
 
-    pub fn set(&self, script: Box<dyn Script>) {
+    pub fn set(&self, script: Arc<dyn Script>) -> Arc<dyn Script> {
         if let Some(v) = self.script.read().deref() {
             v.mark_for_deletion();
         }
         if self.script.is_locked() {
-            // This is unfortunate, but when we're replacing this script from itself, we can not delay
+            let holder = self.script.clone();
-            // it, and as the script is locked, we run into a deadlock here. As such, we force update
+            let new = script.clone();
-            // it, bypassing the RwLock entirely. A better solution might be possible, but I currently
+            std::thread::spawn(move || {
-            // cannot think of one.
+                holder.write().replace(new);
-            unsafe {
+            });
-                self.script.data_ptr().as_mut().unwrap().replace(script);
-            }
         } else {
-            self.script.write().replace(script);
+            self.script.write().replace(script.clone());
-        }
+        };
+        script
     }
 
     pub fn clear(&self) {
@@ -215,11 +214,13 @@ impl From<ScriptHolder> for ScriptContainer {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::sync::atomic::AtomicBool;
+    use std::sync::atomic::{AtomicBool, AtomicPtr};
+    use std::thread::sleep;
+    use std::time::Duration;
 
     pub struct TestScript {
         name: StringKey,
-        container: *mut ScriptContainer,
+        container: AtomicPtr<ScriptContainer>,
         suppressed_count: AtomicUsize,
         marked_for_deletion: AtomicBool,
     }
@@ -228,7 +229,7 @@ mod tests {
         fn new() -> Self {
             Self {
                 name: StringKey::new("test"),
-                container: std::ptr::null_mut::<ScriptContainer>(),
+                container: AtomicPtr::<ScriptContainer>::default(),
                 suppressed_count: AtomicUsize::new(0),
                 marked_for_deletion: Default::default(),
             }
@@ -253,7 +254,7 @@ mod tests {
         }
 
         fn on_initialize(&self, _pars: &[EffectParameter]) {
-            unsafe { self.container.as_mut().unwrap().clear() }
+            unsafe { self.container.load(Ordering::Relaxed).as_ref().unwrap().clear() }
         }
 
         fn as_any(&self) -> &dyn Any {
@@ -270,11 +271,13 @@ mod tests {
     // then dispose of the script.
     #[test]
     fn clear_self_while_active() {
-        let script = Box::new(TestScript::new());
+        let script = Arc::new(TestScript::new());
-        let container = ScriptContainer::new(script);
+        let mut container = ScriptContainer::new(script);
-        let mut w = container.script.write();
+        let c = &mut container as *mut ScriptContainer;
-        let script: &mut TestScript = w.as_mut().unwrap().as_any_mut().downcast_mut().unwrap();
+
-        script.container = &container as *const ScriptContainer as *mut ScriptContainer;
+        let w = container.script.read();
+        let script: &TestScript = w.as_ref().unwrap().as_any().downcast_ref().unwrap();
+        script.container.store(c, Ordering::SeqCst);
         drop(w);
         // Initialize with the script being taken as read lock. This prevents the script from actually
         // removing itself, as it's still doing things.
@@ -290,7 +293,7 @@ mod tests {
 
     pub struct ReplaceTestScript {
         name: StringKey,
-        container: *mut ScriptContainer,
+        container: AtomicPtr<ScriptContainer>,
         suppressed_count: AtomicUsize,
         marked_for_deletion: AtomicBool,
     }
@@ -299,15 +302,15 @@ mod tests {
         fn new(name: StringKey) -> Self {
             Self {
                 name,
-                container: std::ptr::null_mut::<ScriptContainer>(),
+                container: AtomicPtr::<ScriptContainer>::default(),
                 suppressed_count: AtomicUsize::new(0),
                 marked_for_deletion: Default::default(),
             }
         }
 
-        fn test(&self, script: Box<dyn Script>) {
+        fn test(&self, script: Arc<dyn Script>) {
             unsafe {
-                self.container.as_mut().unwrap().set(script);
+                self.container.load(Ordering::Relaxed).as_ref().unwrap().set(script);
             }
         }
     }
@@ -339,14 +342,16 @@ mod tests {
 
     #[test]
     fn replace_self_while_active() {
-        let script = Box::new(ReplaceTestScript::new("script1".into()));
+        let script = Arc::new(ReplaceTestScript::new("script1".into()));
-        let container = ScriptContainer::new(script);
+        let mut container = ScriptContainer::new(script);
-        let mut w = container.script.write();
+        let c = &mut container as *mut ScriptContainer;
-        let script: &mut ReplaceTestScript = w.as_mut().unwrap().as_any_mut().downcast_mut().unwrap();
+
-        script.container = &container as *const ScriptContainer as *mut ScriptContainer;
+        let w = container.script.read();
+        let script: &ReplaceTestScript = w.as_ref().unwrap().as_any().downcast_ref().unwrap();
+        script.container.store(c, Ordering::SeqCst);
         drop(w);
 
-        let script2 = Box::new(ReplaceTestScript::new("script2".into()));
+        let script2 = Arc::new(ReplaceTestScript::new("script2".into()));
         container
             .script
             .read()
@@ -356,6 +361,7 @@ mod tests {
             .downcast_ref::<ReplaceTestScript>()
             .unwrap()
             .test(script2);
+        sleep(Duration::from_micros(500));
         assert_eq!(
             container.script.read().as_ref().unwrap().name(),
             &StringKey::new("script2")

src/dynamic_data/script_handling/script_set.rs

@@ -1,6 +1,7 @@
 use crate::dynamic_data::script_handling::script::{Script, ScriptContainer};
 use crate::{PkmnResult, StringKey};
 use indexmap::IndexMap;
+use std::sync::Arc;
 
 #[derive(Debug, Default)]
 pub struct ScriptSet {
@@ -8,7 +9,7 @@ pub struct ScriptSet {
 }
 
 impl ScriptSet {
-    pub fn add(&mut self, script: Box<dyn Script>) -> ScriptContainer {
+    pub fn add(&mut self, script: Arc<dyn Script>) -> ScriptContainer {
         if let Some(lock) = self.scripts.get(script.name()) {
             if let Some(existing) = lock.get() {
                 let existing = existing.read();
@@ -24,7 +25,7 @@ impl ScriptSet {
 
     pub fn stack_or_add<'b, F>(&mut self, key: &StringKey, instantiation: &'b F) -> PkmnResult<Option<ScriptContainer>>
     where
-        F: Fn() -> PkmnResult<Option<Box<dyn Script>>>,
+        F: Fn() -> PkmnResult<Option<Arc<dyn Script>>>,
     {
         if let Some(lock) = self.scripts.get(key) {
             if let Some(existing) = lock.get() {

src/dynamic_data/script_handling/volatile_scripts.rs

@@ -6,7 +6,7 @@ use std::sync::Arc;
 
 pub trait VolatileScripts<'a> {
     fn volatile_scripts(&self) -> &Arc<RwLock<ScriptSet>>;
-    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Box<dyn Script>>>;
+    fn load_volatile_script(&self, key: &StringKey) -> PkmnResult<Option<Arc<dyn Script>>>;
 
     fn has_volatile_script(&self, key: &StringKey) -> bool {
         self.volatile_scripts().read().has(key)