Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 7, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
86f2be8
feat!: Merge `ScriptContexts<T>` into `Scripts` and store type erased…
makspll Mar 6, 2025
c7799a6
parameterize scripts by the plugin type
makspll Mar 7, 2025
69e5584
improve logs
makspll Mar 7, 2025
cc2e23a
Merge branch 'main' into refactor/inline-contexts-into-scripts
makspll Mar 7, 2025
370fe52
format
makspll Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_mod_scripting_core/src/bindings/schedule.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
398 changes: 184 additions & 214 deletions crates/bevy_mod_scripting_core/src/commands.rs
View file
Open in desktop

Large diffs are not rendered by default.

194 changes: 21 additions & 173 deletions crates/bevy_mod_scripting_core/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> =
Expand All @@ -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
Expand All @@ -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(),
}
Expand All @@ -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(),
}
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)?;
Expand All @@ -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)?;
Expand All @@ -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,
}
30 changes: 9 additions & 21 deletions crates/bevy_mod_scripting_core/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use crate::{
script_value::ScriptValue,
ReflectBaseType, ReflectReference,
},
context::ContextId,
script::ScriptId,
};
use bevy::{
Expand Down Expand Up @@ -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(),
}))
}
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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
)
};
}
Expand Down Expand Up @@ -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
)
},
Expand Down Expand Up @@ -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
)
},
Expand Down
Loading
Loading