feat: Use the Handles, Luke!

[shanecelis]

I was only planning to file issue #426 tonight, which states the problems I was having. I thought if I poked around I'd find the implementation reason for why what I suggest there wouldn't work. So I made a branch and rolled up my sleeves, and I was able to pretty quickly get exactly what I wanted.

Handles

In the issue I suggest doing away with ScriptId. As I worked with the implementation, it became clear I actually wanted to redefine it.

// OLD
// type ScriptId = Cow<'static, str>; 
// NEw
type ScriptId = AssetId<ScriptAsset>;

Beyond that I made it so ScriptComponent holds Handle<ScriptAsset>s. The nice thing about this is you can use this pattern:

ScriptComponent(vec![asset_server.load("foo.lua")])

No need to store a strong handle somewhere so your script isn't unloaded. However, if you rely on that behavior you can still do that like you do with any other asset-based item using weak handles.

let strong_handle = asset_server.load("foo.lua");
ScriptComponent(vec![strong_handle.clone_weak()])

This preserves the semantics of the current release.

ScriptAsset loading versus evaluation

In this PR the static scripts are handled essentially the same as before except they can retain handles (both strong and weak). EDIT: A script may be loaded without evaluation. If it is added as a static script, it will then be evaluated.

The non-static scripts are not evaluated until they are added to a ScriptComponent. Then they are evaluated in order, sequentially.

ScriptAsset change

I suggested a change to ScriptAsset and that survived this refactoring.

pub struct ScriptAsset {
    /// The body of the script
    pub content: Box<[u8]>,
    /// The language of the script
    pub language: Language,
}

I also provided a ScriptSettings where one can provide a definite language. None will use the file's extension.

asset_server.load_with_settings("hello.p8", |settings: &mut ScriptSettings| {
   settings.language = Some(Language::Lua);
});

One downside about ScriptSettings is it required adding "serde" with the "deriv" feature as a dependency.

Example

I updated the Game of Life to work with Lua and Rhai and all its console options.

Remaining work

I have not tried to exercise any other languages or even do much with the tests. But I'd be happy to move this forward to handle the rest with some consensus that that's the right direction.

[shanecelis]

I got the tests compiling and running. There are 9 failures with features 'lua' and 'rhai'.

[shanecelis]

There is a regression in that it doesn't support multiple scripts. I'm trying to get that ready in the 'use-entities' branch, which is compiling now but not running yet if you wanted to take a peek. But maybe this PR needs to cook a little more. I'm gonna take another pass on it tonight.

[shanecelis]

Ok, I have fixed the regression. Game of life works in all ways for Lua. For Rhai it doesn't seem to work for static scripts.

So as I was trying to make scripts work with a shared context and per entity/script, I realized the split of contexts is actually a big policy decision. I imagine in most cases it'll come down to a few qualities: entity, script, and another thing I'm calling domain. To that I end I wrote this trait:

/// A generic script context provider
pub trait ScriptContextProvider<P: IntoScriptPluginParams> {
    /// Get the context.
    fn get(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Domain) -> Option<&Arc<Mutex<P::C>>>;
    /// Insert a context.
    fn insert(&mut self, id: Option<Entity>, script_id: &ScriptId, domain: &Domain, context: P::C) -> Result<(), P::C>;
    /// Returns true if there is a context.
    fn contains(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Domain) -> bool;
}

My hope is this would allow us to provide the right criteria to choose contexts divisions.

SharedContext

So the shared context is implemented with this:

pub struct SharedContext<P: IntoScriptPluginParams>(pub Option<Arc<Mutex<P::C>>>);

and it ignores the arguments and just provides the same context to everyone.

EntityContext

A per entity context is implemented like this:

/// Stores the script context by entity.
pub struct EntityContext<P: IntoScriptPluginParams>(HashMap<Entity, Arc<Mutex<P::C>>>);

ScriptContext composition

And the resource we make available is a composition:

#[derive(Resource)]
/// Keeps track of contexts
pub enum ScriptContext<P: IntoScriptPluginParams> {
    /// One shared script context
    Shared(SharedContext<P>),
    /// One script context per entity
    ///
    /// Stores context by entity with a shared context as a last resort when no
    /// entity is provided.
    Entity(EntityContext<P>, SharedContext<P>)
}

And if we wanted to we could other provide other policies like by ScriptId or something.

Domain

But what's domain? It's the old ScriptId sort of, maybe?

/// A kind of catch all type for script context selection
///
/// I believe this is what the original ScriptId was intended to be.
pub type Domain = Option<Cow<'static, str>>;

I don't do anything with domain presently, and if you think it's extraneous we can toss it.

My vision for domain was to allow users to clump scripts together a little more haphazardly. Something like the following fantasy code would put scripts into one of two domains:

asset_server.load_with_settings("player.lua", |settings: &mut ScriptSettings| {
   settings.domain = Some("player".into());
});
asset_server.load_with_settings("monster.lua", |settings: &mut ScriptSettings| {
   settings.domain = Some("env".into());
});
asset_server.load_with_settings("effects.lua", |settings: &mut ScriptSettings| {
   settings.domain = Some("player".into());
});

Take away

This code is working and I think it's ready for your attention.

Having spent more time with this code base I'm in even more awe of what you've put together. Let me know if there's anything I can do to help. I'm sorry that this branch is probably going to conflict with 0.16 branch. :(

Further work

I'm going to try to make this branch work with Nano-9 to dog food it in the coming days.

[shanecelis]

It was quick and painless to switch over to this branch in Nano-9. See the commit.

[shanecelis]

Domains

I went ahead and fleshed out the Domain idea and dropped the Option from its type, but in almost every parameter it is an Option<Domain> now.

pub type Domain = Cow<'static, str>;

How to select a domain?

I thought about adding a 'domain' field to ScriptAsset, but I ultimately decided against it because a script could be used in multiple domains. Instead I just used a component; no component, no domain.

/// Defines the domain of a script
#[derive(Component)]
pub struct ScriptDomain(pub Domain);

Static scripts

I could allow for static scripts to have a domain but I have left them all as being in no domain. Are static scripts grouped into their own context? Are static scripts a kind of domain? Are they superseded by domains?

Add support for context per script

I added with_domains() and per_script().

impl<P: IntoScriptPluginParams> ScriptContext<P> {
    /// Use a shared script context
    pub fn shared() -> Self {
        Self::Shared(SharedContext::default())
    }
    /// Domain contexts or a shared context
    pub fn with_domains() -> Self {
        Self::Domain(DomainContext::default(), SharedContext::default())
    }
    /// Use one script context per entity
    pub fn per_entity() -> Self {
        Self::Entity(EntityContext::default(), SharedContext::default())
    }
    /// Use one script context per entity
    pub fn per_script() -> Self {
        Self::ScriptId(ScriptIdContext::default())
    }
}

Now that there are 4 choices, I'd suggest we change the enable_shared_context to instead pick a default. If the user wants to change it they do:

app.insert_resource(ScriptContext::shared());

Problem: N scripts produce N callback evaluations even with 1 shared context

I added a Recipients::Domain and patching everything up I dealt with an issue I came across as soon as I started using multiple scripts in BMS. If I have five scripts in a shared context, and I do a callback for _update() with Recipients::All, _update() is called FIVE TIMES! One can argue that's correct behavior, but I found it surprising. It's not surprising when every script is in its own context. I worked around this issue in Nano-9 by just taking note of one ScriptId and only calling that. However, updating the code I found an opportunity to implement a general solution.

Proposal: Evaluate Callbacks on a Per Context Basis

Here is my take for least-surprise: If I have five scripts all in one shared context, and I fire a call to Recipients::All, then I want that call to go to the shared context once.

If I have four scripts in three different contexts, and I fire a call to Recipients::All, then I want that call to be evaluated three times, once in each context.

To facilitate that, I added the following method:

/// A generic script context provider
pub trait ScriptContextProvider<P: IntoScriptPluginParams> {
    // ... 
    /// Hash for context.
    ///
    /// Useful for tracking what context will be returned by `get()` without
    /// requiring that `P::C` impl `Hash` and cheaper too.
    fn hash(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Option<Domain>) -> Option<u64>;
}

What I think is nice about this is it works the same as before if you're placing every script in its own context. But it will also work with scripts, entities, domains, and shared contexts.

Docs?

Would you like some help updating the docs/book for this PR?

[shanecelis]

I'm prepping this branch for a BMS 0.14.0.

Expecting some real-world use I updated the ScriptContext<P> to allow for the following kinds:

• ScriptContext::shared()
• ScriptContext::per_script()
• ScriptContext::per_entity()
• ScriptContext::domains()
• ScriptContext::shared().with_domains()
• ScriptContext::per_script().with_domains()
• ScriptContext::per_entity().with_domains()

And its default is ScriptContext::per_script() so that it follows the conventions of its preceding versions.

I found some tests failing (and some not even compiling--eek). I'll try to fix them tomorrow.

[shanecelis]

I keep thinking I'm close to being done, but then cargo unveils some new slew of errors. After much teeth gnashing, I have all 172 tests passing when I run "cargo test" from the root workspace. However, when I go into the core crate's directory and run "cargo test" I get compilation errors from the tests. Shouldn't cargo run all the workspace crates' tests?

Anyway, fixing the test harness unveiled this bug:

• If you added your ScriptAsset w/o AssetServer, it wouldn't work. (Now fixed.)

To fit better with ScriptContextProvider<P> which uses an Option<Entity>, I refactored the instances of Entity::from_raw(0) to be Nones.

I didn't seek out to do this, I think I was bewildered by the test harness not working, but as I was patching things up, I realized that we could load scripts without cloning their contents if they're given via a Handle<ScriptAsset>. So that was just a little performance commit as a treat.

Will continue to work on this until I get all tests to pass. Do you have a magic command you provide on the command line to run all the tests?

[makspll]

try cargo xtask test it's a rust-based makefile essentally!

[shanecelis]

Progress but cargo xtask test remains unfinished.

[shanecelis]

It's all compiling now. 3 tests are failing. Unfortunately the fix seems a little hairy. The called_on_right_recipients test in particular requires a change in the event handler to fix.

My workday has been interrupted quite a bit, so I put my partial commits into my "use-handles-dev" branch so it's not distracting in this PR. I changed the event handler and now 5 tests are failing. On it goes.

[shanecelis]

All tests pass! The game of life example starts, stops with Lua & Rhai with and without static argument.

Assumption Breakage

I had been adding Option<Entity> and Option<Domain> parameters and fields as necessary, but the assumption of one context per script handle was still embedded and becoming awkward. I decided to bite the bullet and break with that assumption with this struct:

pub struct ContextKey {
    /// Entity if there is one.
    pub entity: Option<Entity>,
    /// Script ID if there is one.
    pub script_id: Option<Handle<ScriptAsset>>,
    /// Domain if there is one.
    pub domain: Option<Domain>,
}

It's the new key to context.

/// A generic script context provider
pub trait ScriptContextProvider<P: IntoScriptPluginParams> {
    /// Get the context.
+    fn get(&self, context_key: &ContextKey) -> Option<&Arc<Mutex<P::C>>>;
-    fn get(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Option<Domain>) -> Option<&Arc<Mutex<P::C>>>;
    // ...
}

What's in a Domain?

With the inclusion of Domain in the ContextKey, I wanted it to be Copy if possible, so I altered its defintion to this:

/// A kind of catch all type for script context selection
#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
pub struct Domain(u64);

impl Domain {
    /// Create a domain handle.
    pub fn new(hashable: impl Hash) -> Self {
        Domain(DefaultHashBuilder::default().hash_one(hashable))
    }
}

New is Old

This is the new default ScriptContext variant that captures the prior version's behavior and then some.

    DomainEntityScriptId(Or<DomainContext<P>, Or<EntityScriptIdContext<P>, Or<ScriptIdContext<P>, SharedContext<P>>>>),

You can read the Ors as short-circuiting like this:

1. If there is a domain, DomainContext provides a context, or
2. if there is an entity and script, EntityScriptIdContext is used, or
3. if there is a script (which is the case for StaticScripts that have no entity), ScriptIdContext is used, or
4. if there is no domain, entity, or script, then SharedContext is used.

The prior version had behaviors 2 and 3 and 4 as a separate assignment mode. This adds domains without interfering with the rest if they're not used.

And its easy to design different behaviors within the library. For instance, if we wanted to have all static scripts live in the same context we can drop the ScriptIdContext.

    DomainX(Or<DomainContext<P>, Or<EntityScriptIdContext<P>, SharedContext<P>>>),

This means SharedContext will provide contexts for any context key that only has a script ID like static scripts. (We could also make static scripts work with domains as an alternative that's more flexible.)

Reify me not

I did a big refactor on the event_handler_inner<P: IntoScriptPluginParams> function to make it work with the ScriptContext. I chose some code duplication for hopefully better runtime performance. Is there any performance tests I could exercise to compare with the previous version?

It now has these benefits:

• Early exits
• Does not query and reify all scripts to a collection every call
• Queries one entity only when necessary
• Queries scripts only when necessary

Apologies

I am sorry that this PR has gotten so big. At the beginning it seemed like this could be a less invasive set of changes. Having failed to make this change set small, I have endeavored to make it good.

[shanecelis]

I updated the BMS book's chapter on contexts and managing scripts.

I realized through the original documentation that I had misunderstood BMS's original script context isolation. I thought it somehow isolated scripts on an entity-script pair basis. I wrote the entity-script pair context so it would be a drop-in substitute for the original system. Oops. Turns out isolating based on script would be the drop-in replacement.

Choose ScriptContext::default()

Now's the time to choose the default.

• Use ScriptContext::per_script().with_domains(). This is the closest to the preceding version's behavior.
• Use ScriptContext::per_entity_and_script().with_domain(). This is maximally isolated script execution, and perhaps less surprising since the scripts don't share a context between entities.

The default is currently set to the latter.

Migration Guide

I plan to also write a chapter on migrating from one version to another imitating Bevy's excellent migration guides.

[makspll]

Ello, apologies for not interacting much yet, rest assured I am observing the branch and trying to grok it (and I like this direction), I've now gotten the bevy migration branch to a final state, so now I am going to be reviewing the changes in this PR over the next week or so, do you mind merging the last bit of conflicts (as I need the xtask changes to load the workspace locally).

for the merge order, now I am thinking:

• release from this branch for a handles+bevy0.15 version via staging, perhaps as an alpha
• merge bevy 0.16 migration to main, release that as no-handles+bevy0.16
• rebase this branch against that, to get the handles+bevy0.16 version

thoughts?

[shanecelis]

Great to hear!

I think the versions sound fine. A handles+bevy0.15 as an alpha is good by me, better even than polluting the version space with use-handles and not.

rebase this branch against that [no-handles+bevy0.16], to get the handles+bevy0.16 version

I think the desired end is good, but I worry the process of rebasing this branch onto a 0.16 will be a chore. But maybe it won't. To update this branch to "main," I'm going to squash everything in this branch just to make the rebasing easier. Maybe it won't be as troublesome if the commits are fewer, but I was initially thinking I'd take the patch from the old handles+0.16 and apply that on top of this branch with whatever fix up was required.

[shanecelis]

I squashed this branch and rebased it to main. It's compiling. There are two tests which are failing.

failures:
    commands::test::test_commands_with_default_assigner
    commands::test::test_commands_with_global_assigner

And I'm not seeing the unload and reload work yet. Probably need to add a test for those.

[makspll]

I think the desired end is good, but I worry the process of rebasing this branch onto a 0.16 will be a chore. But maybe it won't. To update this branch to "main," I'm going to squash everything in this branch just to make the rebasing easier. Maybe it won't be as troublesome if the commits are fewer, but I was initially thinking I'd take the patch from the old handles+0.16 and apply that on top of this branch with whatever fix up was required.

Ah yeah sorry by rebase I mean rebase OR merge, don't mind which, squash and rebase is also good! Folks at work treat those words interchangably and it's infected me

[makspll]

Nice, I will pull this down locally and play with it

[shanecelis]

Fixed the failing assignment tests. All tests are passing!

I added an example called "run-script" to exercise the reload functionality. It's a mouthful to run reload.lua because of the needed features:

cargo run --features lua54,bevy/file_watcher,bevy/multi_threaded --example run-script -- reload.lua

I know you're looking to get more examples, so I figured this might be good to have for the rust part and maybe putting together quick pure lua or rhai examples as well.

Reload functionality is not working yet in this branch.

[shanecelis]

Reload functionality is now working!

Asset Handling Behavior Change

I forgot to mention this. But we've been using ScriptAsset handles as a proxy for their contexts. When we dropped all script handles, we could drop the context. Because of breaking the 1-to-1 relationship that doesn't seem workable anymore. Instead to specifically purge a context, we now treat both scripts and static scripts the same, in that, we call DeleteScript, which will delete the static script or delete a script's context. (Perhaps there should be a DeleteContext command?) I would suggest making a new system called auto_delete_script that will look for the script handle removals and call DeleteScript automatically to offer a transition path.

TODO

• Write system to automatically delete entity contexts when associated entity despawns.
• Write auto_delete_script system.
• Write migration-guide.md.