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.