feat: ✨ Dynamic Script Components, register_new_component binding, remove_component no longer requires ReflectComponent data (#379)

# Summary
In order to support dynamic script components we need:
- to relax some requirements on having a `ReflectComponent` registration
to allow component operations
- add a script component registry, which stores metadata about each
dynamically registered component
- Add `ScriptValue` to `into_script_ref` and `from_script_ref`
implementations to allow any value to be inserted into the components
payload

The nice thing about this implementation is it *just* works with the
reference system.

The current problems are that the global type cache will not reflect
this script component, to fix that we'd need a mechanism for
"re-computing" certain globals for scripts. This also does not allow
setting a schema on these, which is handy but also footgunny

As a side effect I've also made `remove_component` no longer require the
`ReflectComponent` registration

Author: makspll

.github/workflows/mdbook.yml

@@ -9,13 +9,15 @@ on:
       - 'docs/**'
       - 'crates/xtask/**'
       - '.github/workflows/mdbook.yml'
+      - 'crates/bevy_mod_scripting_functions/**'
   pull_request:
     branches:
       - "**"
     paths:
       - 'docs/**'
       - 'crates/xtask/**'
       - '.github/workflows/mdbook.yml'
+      - 'crates/bevy_mod_scripting_functions/**'
 
 jobs:
 

assets/tests/has_component/dynamic_component.lua

@@ -0,0 +1,6 @@
+local NewComponent = world.register_new_component("ScriptComponentA")
+local entity = world.spawn()
+
+assert(world.has_component(entity, NewComponent) == false, "Entity should not have component")
+world.add_default_component(entity, NewComponent)
+assert(world.has_component(entity, NewComponent) == true, "Entity should have component")

assets/tests/has_component/dynamic_component.rhai

@@ -0,0 +1,6 @@
+let NewComponent = world.register_new_component.call("ScriptComponentA");
+let entity = world.spawn_.call();
+
+assert(world.has_component.call(entity, NewComponent) == false, "Entity should not have component");
+world.add_default_component.call(entity, NewComponent);
+assert(world.has_component.call(entity, NewComponent) == true, "Entity should have component");

assets/tests/register_new_component/new_component_can_be_retrieved.lua

@@ -0,0 +1,12 @@
+local NewComponent = world.register_new_component("ScriptComponentA")
+assert(NewComponent ~= nil, "Failed to register new component")
+assert(NewComponent:short_name() == "DynamicComponent", "Unexpected component type")
+
+
+local new_entity = world.spawn()
+
+world.add_default_component(new_entity, NewComponent)
+
+local component_intance = world.get_component(new_entity, NewComponent)
+
+assert(component_intance ~= nil, "Failed to get component instance")

assets/tests/register_new_component/new_component_can_be_set.lua

@@ -0,0 +1,17 @@
+function on_test()
+    local NewComponent = world.register_new_component("ScriptComponentA")
+
+    local new_entity = world.spawn()
+    world.insert_component(new_entity, NewComponent, construct(types.DynamicComponent, {
+        data = "Hello World"
+    }))
+
+    local component_instance = world.get_component(new_entity, NewComponent)
+    assert(component_instance.data == "Hello World", "unexpected value: " .. component_instance.data)
+
+    component_instance.data = {
+        foo = "bar"
+    }
+
+    assert(component_instance.data.foo == "bar", "unexpected value: " .. component_instance.data.foo)
+end

assets/tests/remove_component/can_remove_dynamic_component.lua

@@ -0,0 +1,11 @@
+local NewComponent = world.register_new_component("ScriptComponentA")
+local new_entity = world.spawn()
+world.add_default_component(new_entity, NewComponent)
+
+local component_instance = world.get_component(new_entity, NewComponent)
+assert(component_instance ~= nil, "unexpected value: " .. tostring(component_instance.data))
+
+world.remove_component(new_entity, NewComponent)
+local component_instance = world.get_component(new_entity, NewComponent)
+
+assert(component_instance == nil, "unexpected value: " .. tostring(component_instance))

assets/tests/remove_component/can_remove_dynamic_component.rhai

@@ -0,0 +1,11 @@
+let NewComponent = world.register_new_component.call("ScriptComponentA");
+let new_entity = world.spawn_.call();
+world.add_default_component.call(new_entity, NewComponent);
+
+let component_instance = world.get_component.call(new_entity, NewComponent);
+assert(type_of(component_instance) != "()", "unexpected value: " + component_instance.data);
+
+world.remove_component.call(new_entity, NewComponent);
+let component_instance_after = world.get_component.call(new_entity, NewComponent);
+
+assert(type_of(component_instance_after) == "()", "unexpected value: " + component_instance_after);

assets/tests/remove_component/no_component_data.lua

@@ -0,0 +1,5 @@
+
+local entity = world._get_entity_with_test_component("CompWithDefault")
+local component = world.get_type_by_name("CompWithDefault")
+world.remove_component(entity, component)
+assert(world.has_component(entity, component) == false, "Component was not removed")

assets/tests/remove_component/no_component_data_errors.rhai renamed to assets/tests/remove_component/no_component_data.rhai

@@ -2,6 +2,5 @@
 let entity = world._get_entity_with_test_component.call("CompWithDefault");
 let component = world.get_type_by_name.call("CompWithDefault");
 
-assert_throws(||{
-    world.remove_component.call(entity, component);
-}, "Missing type data ReflectComponent for type: .*CompWithDefault.*")
+world.remove_component.call(entity, component);
+assert(world.has_component.call(entity, component) == false, "Component was not removed");

assets/tests/remove_component/no_component_data_errors.lua

@@ -1,15 +1,15 @@
 //! Contains the [`FromScriptRef`] trait and its implementations.
 
-use std::{any::TypeId, ffi::OsString, path::PathBuf};
-use bevy::reflect::{
-    DynamicEnum, DynamicList, DynamicMap, DynamicTuple, DynamicVariant, Map, PartialReflect,
-};
 use crate::{
-    bindings::{match_by_type, WorldGuard, FromScript},
+    bindings::{match_by_type, FromScript, WorldGuard},
     error::InteropError,
     reflection_extensions::TypeInfoExtensions,
     ScriptValue,
 };
+use bevy::reflect::{
+    DynamicEnum, DynamicList, DynamicMap, DynamicTuple, DynamicVariant, Map, PartialReflect,
+};
+use std::{any::TypeId, ffi::OsString, path::PathBuf};
 
 /// Converts from a [`ScriptValue`] to a value equivalent to the given [`TypeId`].
 ///
@@ -56,6 +56,7 @@ impl FromScriptRef for Box<dyn PartialReflect> {
                 tq : String  => return <String>::from_script(value, world).map(|a| Box::new(a) as _),
                 tr : PathBuf => return <PathBuf>::from_script(value, world).map(|a| Box::new(a) as _),
                 ts : OsString=> return <OsString>::from_script(value, world).map(|a| Box::new(a) as _),
+                tsv: ScriptValue => return <ScriptValue>::from_script(value, world).map(|a| Box::new(a) as _),
                 tn : ()      => return <()>::from_script(value, world).map(|a| Box::new(a) as _)
             }
         );

crates/bevy_mod_scripting_core/src/bindings/function/from_ref.rs

@@ -102,6 +102,7 @@ fn into_script_ref(
             },
             tr : PathBuf => return downcast_into_value!(r, PathBuf).clone().into_script(world),
             ts : OsString=> return downcast_into_value!(r, OsString).clone().into_script(world),
+            tsv: ScriptValue=> return Ok(downcast_into_value!(r, ScriptValue).clone()),
             tn : ()      => return Ok(ScriptValue::Unit)
         }
     );

crates/bevy_mod_scripting_core/src/bindings/function/into_ref.rs

@@ -69,6 +69,8 @@ impl CoreGlobals {
     /// A cache of types normally available through the `world.get_type_by_name` function.
     ///
     /// You can use this to avoid having to store type references.
+    ///
+    /// Note that this cache will NOT contain types manually registered by scripts via `register_new_component`.
     fn types(
         guard: WorldGuard,
     ) -> Result<

crates/bevy_mod_scripting_core/src/bindings/globals/core.rs

@@ -12,5 +12,6 @@ crate::private::export_all_in_modules! {
     script_system,
     script_value,
     world,
+    script_component,
     type_data
 }

crates/bevy_mod_scripting_core/src/bindings/mod.rs

@@ -1,18 +1,20 @@
 //! Utilities for querying the world.
 
-use super::{with_global_access, ReflectReference, WorldAccessGuard};
+use super::{with_global_access, DynamicComponent, ReflectReference, WorldAccessGuard, WorldGuard};
 use crate::error::InteropError;
 use bevy::{
     ecs::{
         component::ComponentId,
         entity::Entity,
         query::{QueryData, QueryState},
+        reflect::ReflectComponent,
         world::World,
     },
     prelude::{EntityRef, QueryBuilder},
+    ptr::OwningPtr,
     reflect::{ParsedPath, Reflect, TypeRegistration},
 };
-use std::{any::TypeId, collections::VecDeque, sync::Arc};
+use std::{any::TypeId, collections::VecDeque, ptr::NonNull, sync::Arc};
 
 /// A reference to a type which is not a `Resource` or `Component`.
 ///
@@ -27,9 +29,13 @@ pub struct ScriptTypeRegistration {
 /// A reference to a component type's reflection registration.
 ///
 /// In general think of this as a handle to a type.
+///
+/// Not to be confused with script registered dynamic components, although this can point to a script registered component.
 pub struct ScriptComponentRegistration {
     pub(crate) registration: ScriptTypeRegistration,
     pub(crate) component_id: ComponentId,
+    /// whether this is a component registered BY a script
+    pub(crate) is_dynamic_script_component: bool,
 }
 
 #[derive(Clone, Reflect, Debug)]
@@ -100,6 +106,8 @@ impl ScriptComponentRegistration {
     /// Creates a new [`ScriptComponentRegistration`] from a [`ScriptTypeRegistration`] and a [`ComponentId`].
     pub fn new(registration: ScriptTypeRegistration, component_id: ComponentId) -> Self {
         Self {
+            is_dynamic_script_component: registration.type_id()
+                == std::any::TypeId::of::<DynamicComponent>(),
             registration,
             component_id,
         }
@@ -120,6 +128,85 @@ impl ScriptComponentRegistration {
     pub fn into_type_registration(self) -> ScriptTypeRegistration {
         self.registration
     }
+
+    /// Removes an instance of this component from the given entity
+    pub fn remove_from_entity(
+        &self,
+        world: WorldGuard,
+        entity: Entity,
+    ) -> Result<(), InteropError> {
+        world.with_global_access(|world| {
+            let mut entity = world
+                .get_entity_mut(entity)
+                .map_err(|_| InteropError::missing_entity(entity))?;
+            entity.remove_by_id(self.component_id);
+            Ok(())
+        })?
+    }
+
+    /// Inserts an instance of this component into the given entity
+    ///
+    /// Requires whole world access
+    pub fn insert_into_entity(
+        &self,
+        world: WorldGuard,
+        entity: Entity,
+        instance: Box<dyn Reflect>,
+    ) -> Result<(), InteropError> {
+        if self.is_dynamic_script_component {
+            // if dynamic we already know the type i.e. `ScriptComponent`
+            // so we can just insert it
+
+            world.with_global_access(|world| {
+                let mut entity = world
+                    .get_entity_mut(entity)
+                    .map_err(|_| InteropError::missing_entity(entity))?;
+                let cast = instance.downcast::<DynamicComponent>().map_err(|v| {
+                    InteropError::type_mismatch(TypeId::of::<DynamicComponent>(), Some(v.type_id()))
+                })?;
+                // the reason we leak the box, is because we don't want to double drop the owning ptr
+
+                let ptr = (Box::leak(cast) as *mut DynamicComponent).cast();
+                // Safety: cannot be null as we just created it from a valid reference
+                let non_null_ptr = unsafe { NonNull::new_unchecked(ptr) };
+                // Safety:
+                // - we know the type is ScriptComponent, as we just created the pointer
+                // - the box will stay valid for the life of this function, and we do not return the ptr
+                // - pointer is alligned correctly
+                // - nothing else will call drop on this
+                let owning_ptr = unsafe { OwningPtr::new(non_null_ptr) };
+                // Safety:
+                // - Owning Ptr is valid as we just created it
+                // - TODO: do we need to check if ComponentId is from this world? How?
+                unsafe { entity.insert_by_id(self.component_id, owning_ptr) };
+                Ok(())
+            })?
+        } else {
+            let component_data = self
+                .type_registration()
+                .type_registration()
+                .data::<ReflectComponent>()
+                .ok_or_else(|| {
+                    InteropError::missing_type_data(
+                        self.registration.type_id(),
+                        "ReflectComponent".to_owned(),
+                    )
+                })?;
+
+            //  TODO: this shouldn't need entire world access it feels
+            let type_registry = world.type_registry();
+            world.with_global_access(|world| {
+                let mut entity = world
+                    .get_entity_mut(entity)
+                    .map_err(|_| InteropError::missing_entity(entity))?;
+                {
+                    let registry = type_registry.read();
+                    component_data.insert(&mut entity, instance.as_partial_reflect(), &registry);
+                }
+                Ok(())
+            })?
+        }
+    }
 }
 
 impl std::fmt::Debug for ScriptTypeRegistration {