Script Management Exclusive System Refactor (NEEDS REVIEW)

[ConnorBP]

Tentative initial work for exclusive system refactor. Still in todo:

• [ ] quality check some of the changes (spent most of the time fighting the borrow checker).
• [ ] make sure the new interface is correctly added to each script host implementation
• [x] figure out why even with this, trying to get world from an attach api bound function is returning nil
• [ ] implement usage of world access to rune and rhai (maybe this should be its own PR)
• [ ] the script reload system makes a clone of all loaded script handles due to lifetime reasons. Lessening this would be ideal.
• [ ] update relevant documentation about script load system
• [ ] maybe increment semvar?

example of what is trying to be fixed

fn attach_api(&mut self, ctx: &mut Self::APITarget) -> Result<(), ScriptError> {
        // callbacks can receive any `ToLuaMulti` arguments, here '()' and
        // return any `FromLuaMulti` arguments, here a `usize`
        // check the Rlua documentation for more details

        println!("ATTACHED API");

        let ctx = ctx.get_mut().unwrap();

        ctx.globals()
            .set(
                "print2",
                ctx.create_function(|ctx, msg: String| {
                    println!("RUNNING PRINT COMMAND");
                    // retrieve the world pointer
                    let world = match (ctx.get_world()) { // <---------------------------------- THIS FAILS when called during script load
                        Ok(world) => world,
                        Err(e) => {
                            error!("print() failed to get world from ctx: {e}");
                            return Ok(());
                        }
                    };
                    println!("GOT WORLD");
                    let mut world = world.write();
                    println!("GOT WRITE ACCESS TO WORLD");

                    let mut events: Mut<Events<PrintConsoleLine>> =
                        world.get_resource_mut().unwrap();
                    println!("GOT EVENT WRITER");
                    events.send(PrintConsoleLine { line: msg.into() });
                    println!("SENT EVENT");

                    // return something
                    Ok(())
                })
                .map_err(|e| {
                    println!("print was called and errored. {e}");
                    ScriptError::new_other(e)
                })?,
            )
            .map_err(|e| {
                println!("failed to attach print command. {e}");
                ScriptError::new_other(e)
            })?;

        // equivalent to ctx.globals().set() but for multiple items
        tealr::mlu::set_global_env(Export, ctx).map_err(ScriptError::new_other)?;

        Ok(())
    }

List of changes to api from this PR:

• Made script load and reload api and systems intake a global varialbe
• replaced event_writer: &mut EventWriter<ScriptLoaded>, argument to reload and insert new functions with a resource_scope on the world pointer.
• Both api bindings and "world" global are now available during initial script load execution and api initialization
• Script handles are now cloneable and so are script collections
• more script errors are now passed to the event writer (such as script load failure)
• added CachedScriptLoadState system state resource for event writers and queries used by the exclusive system script loading systems

[ConnorBP]

The console integration examples print command still fails to get LuaWorld from "world" on script load for some reason here. Not quite sure why....

I'm gonna come back to it with fresh eyes and maybe realize that I forgot to call a function somewhere, but please do let me know if you spot it.

[ConnorBP]

I should mention, one thing the changes so far have made possible which was not possible previously is this use case:

#[derive(Default)]
struct Export;
impl tealr::mlu::ExportInstances for Export {
    fn add_instances<'lua, T: tealr::mlu::InstanceCollector<'lua>>(
        self,
        instance_collector: &mut T,
    ) -> mlua::Result<()> {
        instance_collector
            // .document_instance("The root collection of API modules making up the Lychee API")
            // .add_instance("lychee", |ctx| {
            //     Ok(ctx.create_table_from(vec![("test", LycheeAPIModule)])?)
            // })?
            .document_instance("Utility functions for interacting with the game memory directly.")
            .add_instance("memory", |ctx| {
                if let Some(processr) = ctx
                    .get_world()
                    .map_err(|e| {
                        mlua::Error::RuntimeError(format!(
                            "Failed getting world in tealr lua api init: {e}"
                        ))
                    })?
                    .read()
                    .get_resource::<ProcessR<IntoProcessInstance<CBox<c_void>, CArc<c_void>>>>()
                {
                    Ok(MemoryAPIModule {
                        process: processr.0.clone(),
                    })
                } else {
                    return Err(mlua::Error::RuntimeError(
                        "failed to get memflow process resource".to_string(),
                    ));
                }
            })
            .map_err(|e| mlua::Error::RuntimeError(format!("{e}")))?;

        Ok(())
    }
}

Before the changes in this PR the globals variable was not set so it was not possible to initialize modules content in the instance collector with any meaningful data.

The fact that this is working now means that the globals are in-fact set, so I am not quite sure why they are still inaccessible during script load execution. Something weird with the stack is going on I think?

[ConnorBP]

Found the problem I believe. Its a cyclic dependency. The world cannot be injected the first time and fails because its type has no yet been defined by tealr in the add_instances. But if you call attach api first then the world passed to it is not yet set.

set("world", crate::lua::bevy::LuaWorld::new(world_ptr))

fails if

 instances.add_instance(
    "world",
    crate::lua::util::DummyTypeName::<crate::lua::bevy::LuaWorld>::new,
)?;

was not called first.

Not sure what the best solution is. The quickest fix would be to call the initialization twice, but some way of splitting off the dependent sections into a third seperate stage would be ideal I think.

• Register Types by attaching bevy_api
• then setup runtime which causes world to be set
• then attach any apis dependent on world access

[ConnorBP]

Found the problem I believe. Its a cyclic dependency. The world cannot be injected the first time and fails because its type has no yet been defined by tealr in the add_instances. But if you call attach api first then the world passed to it is not yet set.

set("world", crate::lua::bevy::LuaWorld::new(world_ptr))

fails if

 instances.add_instance(
    "world",
    crate::lua::util::DummyTypeName::<crate::lua::bevy::LuaWorld>::new,
)?;

was not called first.

Not sure what the best solution is. The quickest fix would be to call the initialization twice, but some way of splitting off the dependent sections into a third seperate stage would be ideal I think.

• Register Types by attaching bevy_api
• then setup runtime which causes world to be set
• then attach any apis dependent on world access

I was wrong. I don't think this was relevant at all. I now suspect it is because contexts.insert_context(script_data.to_owned(), Some(lua)); is called after the load script function.

edit: kill me please, I have no idea what is wrong with it.

[ConnorBP]

I have been debugging this for days. I am completely at a loss as to why this function fails when called in a rust callback function that is bound to lua:

impl GetWorld for Lua {
    type Error = mlua::Error;
    fn get_world(&self) -> Result<WorldPointer, Self::Error> {
        self.globals().get::<_, LuaWorld>("world").map(Into::into)
    }
}

.get::<_, LuaWorld>("world") here returns a nil. The craziest thing is that the world global is accessible just fine from the tealr add_instances() function which is called shortly before this one. Either something is funky with the way the lua callbacks are being created, or world is being popped without being put back. I am very close to giving up but I might be close........ I have been close for days. :'(

[makspll]

Hi @ConnorBP apologies I haven't had much time for this project recently, I'll try have a look and help ASAP! This is all great work and I really appreciate it!

[makspll]

So i've had a quick look, Looks like the issues begin with this code:

        // {
        //     providers.attach_all(&mut lua)?;
        // }

commenting this out and modifying the console example script to:

Gives me this output:

so somehow the API attachment step resets the globals maybe?

[makspll]

Ahh, I believe the BevyAPIProvider uses this:

        instances.add_instance(
            "world",
            crate::lua::util::DummyTypeName::<crate::lua::bevy::LuaWorld>::new,
        )?;

To expose world in the docs, but this might be overwriting the world global in this case

[makspll]

Okay, here is the easy solution then, setting up the script runtime is very light, it's just setting up a world pointer, let's do it twice, once before attaching hte API so it's available there, then afterwards so it's available when the script loads, this shouldn't mess with docs generation and won't drag performance too much (plus only once per script). Otherwise we'd have to figure out another way of documenting these things but the tealr part of the system needs an overhaul later anyway so that can be addressed at the same time.

EDIT: Yes doing this fixed the problem

[ConnorBP]

Okay, here is the easy solution then, setting up the script runtime is very light, it's just setting up a world pointer, let's do it twice, once before attaching hte API so it's available there, then afterwards so it's available when the script loads, this shouldn't mess with docs generation and won't drag performance too much (plus only once per script). Otherwise we'd have to figure out another way of documenting these things but the tealr part of the system needs an overhaul later anyway so that can be addressed at the same time.

EDIT: Yes doing this fixed the problem

Oh amazing! Nice work, thanks for the assist!

[makspll]

Hey @ConnorBP sorry for not reviewing in a while, I am doing some serious re-factoring and I have this issue in mind. I think soon this use case will be supported through a much more modular structure!

[ConnorBP]

Hey @ConnorBP sorry for not reviewing in a while, I am doing some serious re-factoring and I have this issue in mind. I think soon this use case will be supported through a much more modular structure!

Excellent! :) should be much easier on the latest bevy as well. The branch for this PR works pretty well if you wanna try it out

[shanecelis]

If this PR makes world available at script load time, I'm all about that! Any chance it could be resurrected? I see it's a little dated now. If it was rebased and working with main, would it be likely to be merged?

[makspll]

Hi @shanecelis, if I remember correctly the PR needed some more work before merging (testing, and I think porting this to the other scripting langs), if the demand is high I don't mind merging this work assuming somebody else does this work. However worth keeping in mind is these features will be supported in the big upcoming refactor over at: https://github.com/makspll/bevy_mod_scripting/pull/112 (but I cannot tell you when that will be ready)