Skip to content

feat: Complete Rewrite #183

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.

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 39 commits into from
Feb 1, 2025
Merged

feat: Complete Rewrite #183

merged 39 commits into from
Feb 1, 2025

Conversation

makspll
Copy link
Owner

@makspll makspll commented Dec 31, 2024

Todo:

  • Global namespace support
  • Removal/Overwriting of script functions
  • Re-check soundness
  • Increase coverage in BMS core
  • Add insert component/resource methods
  • Decouple code gen away from lua
  • Get rid of panicky errors on access violations and in general reduce unwrap usage
  • Get rid of script allocator in favour of an actual allocator not possible at the moment
  • RHAI
  • Optional args
  • Documentation registration scaffolding
  • Utilities for getters/setters
  • HashMap support
  • final docs adjustments

@makspll
Copy link
Owner Author

makspll commented Jan 4, 2025

There is potential to make the book both a reference and source of integration tests for the various languages

github-actions bot and others added 3 commits January 5, 2025 22:22
* chore: release v0.9.0-alpha.2

* Update CHANGELOG.md

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Maksymilian Mozolewski <[email protected]>
@makspll
Copy link
Owner Author

makspll commented Jan 5, 2025

Now released as 0.9.0-alpha.2

makspll and others added 2 commits January 8, 2025 22:32
* fix: bugs to do with multiple languages being initialized

* rhai re-implementation WIP

* feat: switch vscode to xtask

* re-write lua tests to use common integration test framework

* setup rhai tests

* add assertions

* start writing marshalling code for rhai

* move world pointers to thread locals

* move namespace stuff into core

* chore(codegen): update bevy bindings (#195)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* refactor function interface, reduce amount of string cloning, happy cows

* remove extra import, fix lua to_string

* clean up comments

* remove rhai and rune support for now

* correct for changed features in xtasks

* Add note in readme about removal of features

* start adding documentation for new languages

* format

* fix failing test utils test

* generate matrix from xtask

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
makspll and others added 15 commits January 13, 2025 15:58
* chore: make CI/CD compile

* update commit parsers for release plz

* print the output

* fix line issues

* Print a bit more

* fix jq command

* fix command

* Improve display names for actions and fix short name for subcommand

* fix invalid default feature, and remove short option for subcommand

* don't print matrix anymore

* remove short options

* try build docker image

* fix tag

* leave docker for now

* fix into command not taking into account features

* fix various problems

* fix luau compile error

* fix luau and try docker again

* typo

* try without windows platform

* move docker image creation to its own action
…afe_lua_modules into a conditional default feature
* chore: sort functions in bevy_api_gen

* chore(codegen): update bevy bindings (#201)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* chore: trigger ci

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* feat: add more functionality to the script registry

* add global namespace alias

* register global functions in lua
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
fix: functions referencing test time dependencies
* Add more information to docs

* add information on world containers

* add edit url to mdbook
* feat: tighten testing, and frame hygiene

* remove unnecessary imports in bindings

* update release-plz config

* update pr-title filter

* trigger bindings ci

* chore(codegen): update bevy bindings (#209)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Improve UI for adding intiializers and add docs, correct initialization of lua plugins

* remove some unnecessary optionals

* report coverage

* remove failing todo

* set error settings

* fix workflow syntax

* run after check and bump versions

* actually include coverage in the run

* install genhtml

* actually use the right path and only upload coverage from ubuntu

* change profile for coverage runs

* move coverage gen into the matrix

* output coverage in the matrix

* improve comments slightly, and make global contexts work

* refactor asset systems, make use of internal script asset events

* add simple error test

* remove inactive docs code

* improve docs

* add reflect reference tests

* update report command

* include coverage badge as test

* increase badge width and update badge on commit

* force badge re-creation

* add more tests to printing and conversions

* update workflow permissions

* update coverage badge on main only

* fix test and stop ignoring things

* ignore bevy bindings from coverage

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* make the codegen crate work with vscode finally

* update CI to use xtask for bindings

* refactor slightly

* add nightly toolchain to setup

* update PR command

* control vscode settings.json from init

* add docs mention

* tighten codegen ordering

* clean bevy target dir before codegen

* add more context to errors

* tighten the codegen ordering again, for collection

* print faulty json

* escape json

* chore(codegen): update bevy bindings (#214)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* typo

* ignore less paths now that everything is one workflow

* fix bug causing incomplete codegen

* fix clippy

* inject BMS into codegen :D

* remove 'dependencies' key from tera

* remove mlua references from codegen

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@shanecelis
Copy link
Contributor

shanecelis commented Jan 19, 2025

Important: Keep Going!

This is looking really good! I went through the book. It looks a lot simpler for the user and more controllable too. I think I'm going to try to build with the alpha.5 for my project nano-9 soon.

Unimportant: A Question

One question that's not clear to me is, what is shared between scripts? Do they each have their own context? Is Runtime the facilitator for that?

One thing I'd like to be able to do is load some Lua code for every script. Playing with the example, I got this to work:

    let mut plugin = LuaScriptingPlugin::default();
    plugin.scripting_plugin.add_context_initializer(|script_id: &str, context: &mut bevy_mod_scripting::lua::mlua::Lua| {
        let _ = context.load(r#"
my_global = "BLAH"
"#).exec()?;
        Ok(())
    });

Is this what you'd suggest for loading static Lua code? Your documentation suggested this was good when you had dynamic needs.

I tried simply loading a "script.lua" but the two namespaces appear to be isolated.

I apologize for adding some tech support questions in this PR. I'm not blocked; I don't need any answers. Just thought I'd share what my sticking point is from a 0.8 user.

@makspll
Copy link
Owner Author

makspll commented Jan 19, 2025

Hi! All good!

Yes if you have something you'd like to inject into every script that cant be served well with script functions (the dynamic script registry), I'd reccomend using context initializers!

As for sharing contexts, I've made it possible to change context loading settings, so that all scripts actually do end up living in a single Lua context.

I'll add a bit to the docs about that.

As for runtimes, they are not really relevant to Lua, since mlua doesnt have a concept of this separation. For Lua the runtime type is (). For rhai however you can share some stuff with all scripts through the runtime, as well as by interacting with contexts

* add information on context sharing

* expand docs slightly

* apply heavy clippy lints to remove all panics

* remove event handler panics

* remove panics from commands

* DONT PANIC

* fix fmt

* fix failing tests and update docs

* remove unnecessary thing

* add badges

* update badge

* Update readme.md

* change badge

* change badge again
@makspll
Copy link
Owner Author

makspll commented Jan 22, 2025

I found my issue. My cart's lua code had this path: "cart.p8#lua" and that wasn't recognized as a Language::Lua. Got it fixed up with ScriptAssetSettings.

Nice! I will add this to the docs as well

One new issue is, how do you handle optional arguments? I was hoping I could use Option like so:

        NamespaceBuilder::<GlobalNamespace>::new_unregistered(&mut world)
            .register(
                "btn",
                |ctx: FunctionCallContext, b: Option<u8>| {
                    with_pico8(&ctx, |pico8| pico8.btn(b))
                },
            );

But I get, "Error in function btn : Error in function call: expected 1 arguments but received 0." On that note, how do you handle varargs?

Ah yes, this will expect a unit argument, i.e. Nil instead of a lack of arguments. Hmm, i could make the calling mechanism recognize "optional" arguments.

As for varargs, there isn't support for this yet, I think I could however add a variant to the script value which marshalls appropriately 🤔

@shanecelis
Copy link
Contributor

Thank you!

I puzzled this one out myself. Haven't figured out how to exercise it. Looks like FunctionCallContext must come first.

    NamespaceBuilder::<N9Entity>::new(app.world_mut())
        .register("name", |ctx: FunctionCallContext, this: Val<N9Entity>| {
            let world = ctx.world()?;
            // let id: Entity = Entity::PLACEHOLDER; // How do I get the N9Entity's entity field? Oh, I see.
            world.with_component(this.entity, |name: Option<&Name>| {
                name.map(|s| s.as_str().to_owned())
            })
        })

Is this going to do what I want? My hope would be in Lua that I could do some_n9entity:name() and get nil or its name if any.

Is this what we do now instead of implementing UserData?

@shanecelis
Copy link
Contributor

Oh, one thing I thought I'd pass on. You've got some functions with this signature:

pub fn register_script_functions(app: &mut App) -> &mut App;

Perhaps they might be good candidates for the function as Plugin:

pub fn register_script_functions(app: &mut App) {
    NamespaceBuilder::<World>::new_unregistered(app.world_mut()).register("info", |s: String| {
        bevy::log::info!(s);
    });
}
/// ...
app.add_plugins(register_script_functions);

@makspll
Copy link
Owner Author

makspll commented Jan 22, 2025

Thank you!

I puzzled this one out myself. Haven't figured out how to exercise it. Looks like FunctionCallContext must come first.

    NamespaceBuilder::<N9Entity>::new(app.world_mut())
        .register("name", |ctx: FunctionCallContext, this: Val<N9Entity>| {
            let world = ctx.world()?;
            // let id: Entity = Entity::PLACEHOLDER; // How do I get the N9Entity's entity field? Oh, I see.
            world.with_component(this.entity, |name: Option<&Name>| {
                name.map(|s| s.as_str().to_owned())
            })
        })

Is this going to do what I want? My hope would be in Lua that I could do some_n9entity:name() and get nil or its name if any.

Is this what we do now instead of implementing UserData?

Yup thats exactly right! Function context comes first, then any args, the method call syntax just desuggars to fn(entity, arg1) and the function context is sort of a virtual argument

@makspll
Copy link
Owner Author

makspll commented Jan 22, 2025

Oh, one thing I thought I'd pass on. You've got some functions with this signature:

pub fn register_script_functions(app: &mut App) -> &mut App;

Perhaps they might be good candidates for the function as Plugin:

pub fn register_script_functions(app: &mut App) {
    NamespaceBuilder::<World>::new_unregistered(app.world_mut()).register("info", |s: String| {
        bevy::log::info!(s);
    });
}
/// ...
app.add_plugins(register_script_functions);

I could but i didnt really intend these to be re-usable, mostly a convenient syntax for feature flagging some of these

* feature: Add with_or_insert_component_mut().

* doc: Remove comment on panic.

Accepts Option<&T>. Should not panic if component is not available.
@shanecelis
Copy link
Contributor

Everything is working on my end with the new BMS. I guess optional arguments is my only want but I don't need it if I use a Lua facade for all my functions.

function rectfill(x0,y0,x1,y1,c)
    return __rectfill(x0,y0,x1,y1,c)
end

function spr(n,x,y,w,h,fx,fy)
    return __spr(n,x,y,w,h,fx,fy)
end

function map(cx,cy,sx,sy,cw,ch,l)
    return __map(cx or 0,cy or 0,sx or 0,sy or 0,cw or 128,ch or 64,l)
end

function print(s,x,y,c)
    return __print(s,x,y,c)
end

* initial rhai work

* LETS GOO

* convert more tests

* pass the whole scripting suite

* some cleanup

* add rhai to ci matrix

* fix fmt

* re-arrange dependencies, and don't run exclusive deps in ci by themselves

* rearrange

* finalize game of life for both languages

* fix resource issues

* clippy

* correct script unloading handling
@makspll
Copy link
Owner Author

makspll commented Jan 24, 2025

I'll have a crack at optionals next, rhai is now back :D

@shanecelis
Copy link
Contributor

What am I doing wrong with this? I can call __map() and return a N9Entity but when I call retain() I get the below error.

#[derive(Clone, Reflect)]
pub struct N9Entity {
    pub entity: Entity,
    pub drop: DropPolicy,
}

pub(crate) fn register_script_functions(app: &mut App) {
    NamespaceBuilder::<N9Entity>::new(app.world_mut())
        .register("retain", |ctx: FunctionCallContext, this: Val<N9Entity>| {
            let world = ctx.world()?;
            world.with_global_access(|world| {
                let mut commands = world.commands();
                commands.entity(this.entity).remove::<Clearable>();
            })?;
            Ok(this)
        })
        ;
}
// ...
.register(
    "__map",
    |ctx: FunctionCallContext| {
        let id: Entity = // ...
            let entity = N9Entity { entity: id, drop: DropPolicy::Nothing };
            let world = ctx.world()?;
            let reference = {
                let allocator = world.allocator();
                let mut allocator = allocator.write();
                ReflectReference::new_allocated(entity, &mut allocator)
            };
            Ok(ReflectReference::into_script_ref(reference, world)?)
    },
)
    if my_map == nil then
        my_map = map()
        my_map:retain()
    end
ERROR bevy_mod_scripting_core::handler: error in script `../carts/nano9/cardboard-toad.p8#lua`: Could not find function: get for type: bevy_mod_scripting_core::bindings::reference::ReflectReference.
Context:
stack traceback:
        [C]: in metamethod 'index'
        [string "/Users/shane/Projects/bevy_mod_scripting/crat..."]:1693: in field 'draw'
        [string "/Users/shane/Projects/bevy_mod_scripting/crat..."]:1735: in method 'draw'
        [string "/Users/shane/Projects/bevy_mod_scripting/crat..."]:1837: in function '_draw'
Event handling for: Language: Lua

@makspll
Copy link
Owner Author

makspll commented Jan 25, 2025

That's strange, the error suggests the fallback for the index metamethod is failing, it normally looks for a "get" function in the registry to index into the type with, I.e. to get the properties.

Do you manipulate the script function registry? It may be possible you remove it at some point and forget to insert it, which causes the guard to create a default empty one

@shanecelis
Copy link
Contributor

shanecelis commented Jan 25, 2025

I have an old UserData impl for it. I'll try taking that out.

EDIT: I removed that. No change. My struct isn't a Component, Resource, or anything. That shouldn't matter, right?

* implement optional arguments

* bump rhai version

* fmt
@shanecelis
Copy link
Contributor

Confirm option arguments are working. Woohoo!

@makspll
Copy link
Owner Author

makspll commented Jan 25, 2025

EDIT: I removed that. No change. My struct isn't a Component, Resource, or anything. That shouldn't matter, right?

Yeah it should not matter. It definitely looks to me like a symptom of a removed AppScriptFunctionRegistry

#226)

* make sure to call custom get and set functions first

* shorten CI name
@shanecelis
Copy link
Contributor

I got a nice new error with a type id for the missing "get" or "index" function, so I went to confirm that it was my type.

pub(crate) fn plugin(app: &mut App) {
    info!("N9Entity type id {:?}", TypeId::of::<N9Entity>());
    NamespaceBuilder::<N9Entity>::new(app.world_mut()).register(
        "retain",
        |ctx: FunctionCallContext, this: Val<N9Entity>| {
            let world = ctx.world()?;
            world.with_global_access(|world| {
                let mut commands = world.commands();
                commands.entity(this.entity).remove::<Clearable>();
            })?;
            Ok(this)
        },
    );
}

Guess what didn't get logged? Yep, that info! line because it wasn't being called. [Slaps forehead.] Oy vey! Sorry for troubling you about it.

@makspll
Copy link
Owner Author

makspll commented Jan 26, 2025

It's all good, this is useful to me for improving the docs!

makspll and others added 5 commits January 27, 2025 23:11
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* fix: bug where ID ranges were overlapping for components and allocations

* correct release plz

* add rhai changes to changelogs
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
gallexme and others added 3 commits January 31, 2025 16:17
* feat: profiling first draft

* run `cargo fmt` to trigger CI

---------

Co-authored-by: makspll <[email protected]>
* update docs

* change asset path mappers to use actual asset path

* run on pr events too

* fix up more docs

* add badges to docs.io and deny missing docs lint

* only run PR check on opened

* fix for more workflows

* enable rhai for doc gen

* get rid of dupplicate CI jobs

* fix missing docs lints
* add more info about sending reflect types as payloads, streamline interface

* add arg info to callback
@makspll makspll merged commit 8b309a6 into main Feb 1, 2025
32 of 33 checks passed
@makspll makspll deleted the staging branch February 1, 2025 15:34
@makspll makspll mentioned this pull request Feb 2, 2025
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants