Can persistant, shared globals state be enabled?

[LaserWitch]

I'm not sure if this is a FR or something else, but here goes.

In my testing I've found global state isn't behaving how I expect Lua state in a game to act. It appears that each script runs inside it's own, enviroment/context. A global defined by one script is nil when another script tries to access it. I can see some cases for this, but it also seems to inhibit many of the approaches to scripting that I want to enable unless I go through a lot of effort to essentially reimplement parts of Lua as resources.

Perhaps relatedly, when a script is reloaded the original global state seems to be discarded and a fresh one replaces it. for instance this hook:

function on_level()
    if type(_G.i) == 'nil' then _G.i = 0 end
    if(i%200 == 0) then
        print("\n******** " .. i )
    end
    i=i+1
end

In theory the count should persist between reloads, if there was a single context and the hook function was simply replaced by executing the changed file. It appears that it's a clean new context though. Any stateful gameplay scripting live development is going to be really awkward this way. If I make use of require then it may be viable to code around this to a limited degree inside lua itself, but I'm not sure that can cover everything.

Do you think it'd be feasible for a game to get around either of those with custom ScriptHost implementations? If not, is it conceivable to make those alternate behaviors available somehow, likely optionally? I've only got a loose grasp of the internals of the crate so far, so I'm not able to make a confident guess myself.

[makspll]

Hi again! all great questions!

In short, yes I believe all of this could be handled by a custom ScriptHost. The interface is very flexible and in fact register_with_app of each ScriptHost is the sole point of contact with the app for the entire crate.

Discounting this itty bitty fragment of shared code:
bevy_mod_scripting_core/src/lib.rs

    45 | fn build(&self, app: &mut bevy::prelude::App) {
    46 |  app.add_event::<ScriptErrorEvent>();
    47 | }

The removal of old contexts happens in the reload_script method of a script which is used by the script_hot_reload_handler system

bevy_mod_scripting_core/src/hosts.rs

     366 |  // remove old context
     367 |  contexts.remove_context(script.id());

This system is registerd with the app in each ScriptHost like this

languages/bevy_mod_scripting_lua/src/lib.rs, from line 81

    fn register_with_app_in_set(app: &mut App, schedule: impl ScheduleLabel, set: impl SystemSet) {
        app.add_priority_event::<Self::ScriptEvent>()
            .add_asset::<LuaFile>()
            .init_asset_loader::<LuaLoader>()
            .init_resource::<CachedScriptState<Self>>()
            .init_resource::<ScriptContexts<Self::ScriptContext>>()
            .init_resource::<APIProviders<Self>>()
            .register_type::<ScriptCollection<Self::ScriptAsset>>()
            .register_type::<Script<Self::ScriptAsset>>()
            .register_type::<Handle<LuaFile>>()
            // handle script insertions removal first
            // then update their contexts later on script asset changes
            .add_systems(
                schedule,
                (
                    script_add_synchronizer::<Self>, 
                    script_remove_synchronizer::<Self>,
                    script_hot_reload_handler::<Self>, \\ HERE
                )
                    .chain()
                    .in_set(set),
            );
    }

If all scripts are to share a context then instead of using:

    type ScriptContext = Mutex<Lua>;

you'd use something more appropriate, note I am not sure exactly how mlua would handle this exact use case mlua::chunk maybe? but you'd likely have to use a better suited type for the actual script context. This type is mainly used by APIProviders callback's so you'd need to think what would be appropriate. However do note that LuaBevyAPIProvider expects a Mutex<Lua> context type to work, so you could simply keep this the same and make changes elsewhere.

pub trait APIProvider: 'static + Send + Sync {
    /// the type of script engine/context the API is attached to, this must be the same as the APITarget of the ScriptHost meant to receive it.
    type APITarget: Send + Sync + 'static;
    /// The type of script context the APIProvider works with, must be the same as the ScriptContext of the target ScriptHost.
    type ScriptContext: Send + Sync + 'static;
    /// The type of documentation fragment produced by the APIProvider, must be the same as the DocTarget of the target ScriptHost.
    type DocTarget: DocFragment;

    /// provide the given script context with the API permamently.
    /// Depending on the host, API's may be attached on a per-script basis
    /// or on a per-engine basis. Rhai for example allows you to decouple the State of each script from the
    /// engine. For one-time setup use `Self::setup_script`
    fn attach_api(&mut self, api: &mut Self::APITarget) -> Result<(), ScriptError>;

    /// Hook executed every time a script is about to handle events, most notably used to "refresh" world pointers
    fn setup_script_runtime(
        &mut self,
        _world_ptr: WorldPointer,
        _script_data: &ScriptData,
        _ctx: &mut Self::ScriptContext,
    ) -> Result<(), ScriptError> {
        Ok(())
    }

    /// Setup meant to be executed once for every single script. Use this if you need to consistently setup scripts.
    /// For API's use `Self::attach_api` instead.
    fn setup_script(
        &mut self,
        _script_data: &ScriptData,
        _ctx: &mut Self::ScriptContext,
    ) -> Result<(), ScriptError> {
        Ok(())
    }

    /// Generate a piece of documentation to be merged with the other documentation fragments
    /// provided by other API providers
    fn get_doc_fragment(&self) -> Option<Self::DocTarget> {
        None
    }

    /// Some providers might provide additional types which need to be registered
    /// with the reflection API to work.
    fn register_with_app(&self, _app: &mut App) {}
}

What you'd need to be careful about is things like LuaBevyAPIProvider which implements setup_script as:

    fn setup_script(
        &mut self,
        script_data: &ScriptData,
        ctx: &mut Self::ScriptContext,
    ) -> Result<(), ScriptError> {
        let ctx = ctx.get_mut().expect("Could not get context");
        let globals = ctx.globals();
        globals
            .set(
                "entity",
                crate::lua::bevy::LuaEntity::new(script_data.entity),
            )
            .map_err(ScriptError::new_other)?;
        globals
            .set::<_, crate::lua::bevy::LuaScriptData>("script", script_data.into())
            .map_err(ScriptError::new_other)?;

        Ok(())
    }

Note this is run ONCE at the start of the lifetime of each script, (as dictated by the current Script Host implementation), and assuming all the other systems use a shared script context instead, this would mean entity and script globals would be incorrect. Easy fix however by defining a custom APIProvider which performs the same logic in setup_script_runtime instead.

Again this is all assuming mlua has a nice way of handling something like this (I just haven't properly looked!)

[makspll]

If this is something that the crate should handle in your opinion, then we could enable this behavior behind some feature flags in the next release!

[LaserWitch]

It's my expected mode of scripting operation, so to me at least it seems like it'd be reasonable for the crate to offer it if it's feasible to make optional.

But it also seems reasonable to let me beat on the implementation first and iron out any details before having to roll it back into the mainline. The modularity is an asset for doing that kind of thing, I think. I'll definitely try to put your suggestions into practice and let you know if I have any difficulties!

Mod/scriptability is pretty central to my goals in my current project and I'm in the thick of ironing out the basics of it, so I'm likely to have more ideas and/or questions soon! Would it be helpful to you to take a look at my usage at some point? My demo project for it is pretty minimal at the moment so I can focus on getting these parts right.

[makspll]

I am more than happy to incorporate this as a feature, this would also have some performance benefits. Please do let me know how this goes, especially from the mlua side! I'd love new ideas and happy to answer any questions, just shoot them as issues my way.

I can definitely have a look and especially interested in thoughts on the Bevy API as well as the proxy wrapper macros, currently working on a much more coherent and user friendly macro over at: https://github.com/makspll/bevy_mod_scripting/tree/feature/derive_macros

[LaserWitch]

I've chewed on this one on and off, the way everything interconnects is pretty easy to get lost one and I'm not super familiar all these pieces yet. I've mainly attacked it from the Host side so far, but perhaps I should look again at how to do it from the provider side. Though so far I've been looking more at the shared-between-scripts part rather than the persistant-through-reloadspart.

I've tried creating my own derivative of the LuaScriptHost for this. The wall I seem to be hitting consistently is that I need to store a common Lua object, either as a Bevy resource or a field in my script host, and I need to be able to access that in the host's load_script to run the chunk in it. load script not having world access can't really get a resource, and storing it as a field is giving me all sorts of access headaches...

Without wrapping it in boxes I understand very poorly, the most viable option I've not yet tried is having load_script run the chunk on a self.lua, and create a dummy lua mutex just to pass back in the return value and immediately discard, or to just eliminate all use of the load_script function in the host entirely. I'm not sure if I can do that while still playing well with the load_script calls in insert_new_script_context, but maybe if I do both... creating and discarding a lua instance every load seems wasteful, but not actually more expensive than just creating and storing one for each which is already happening, so I guess I'll explore that a bit more.

[LaserWitch]

alright, I did get it working! the two problems are that the events model doesn't work this way(each script with an event hook function overwrites the last. That's basically to be expected, and can even be addressed through Lua code probably. The second problem is that it requires one tiiiiny upstream change.
The current implementation just stores a Mutex in the host struct, and passes that to anywhere that previously would have used the normally stored contexts. This actually accidentally achieved both goals, because the reload wiping out the stored context doesn't touch this overall context.
The one upstream change it needed was taking ScriptHost.handle_events and changing it from

    fn handle_events<'a>(
        &self,
        world_ptr: &mut World,
        events: &[Self::ScriptEvent],
        ctxs: impl Iterator<Item = (ScriptData<'a>, &'a mut Self::ScriptContext)>,
        providers: &mut APIProviders<Self>,
    );

to be a &mut self instead. Otherwise I couldn't properly pass the master context to the providers.

[makspll]

That's great to hear! If this is the only upstream change required AFAIK there shouldn't be any issues including it in the next release

[LaserWitch]

With that merged this seems closable. Once I've reached the conclusion of my efforts in #69 I'll see about tidying up and documenting my little demo project and posting it publicly for review.