rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.79k stars 177 forks source link

ECS? #95

Closed cheako closed 4 years ago

cheako commented 4 years ago

Embedded scripting languages are ripe for implementing systems for an ECS(Entity component system). Integration with an ECS would be ideal and I believe that Specs is the current front runner.

Perhaps all that's needed is documentation?

Goals:

  1. Passing components to/from script code
    • Think generics over components
  2. Systems calling conventions
schungx commented 4 years ago

Can you elaborate on how you'd like to use Rhai with Specs?

cheako commented 4 years ago

That would be a good topic for the documentation to cover!

schungx commented 4 years ago

A few examples or proof of concept stuff will be nice, as I am not quite sure what you're really trying to do.

Raz-Hemo commented 4 years ago

Currently facing this problem for integration with Specs. The main issue is that Specs requires component members to be Send + Sync, which prevents a rhai::Scope from being stored in the component. A way to bypass this would be to convert the scope to a HashMap<String, String> when storing it and back when loading, but i'm not sure how useful (or performant) that would be.

schungx commented 4 years ago

OK, I see. The problem with Scope not being Send is because the values are all Any, meaning that they can be anything, including something that is not Send. However, most primitive types are Send, so if you're only using standard types, this restriction shouldn't really be a problem...

So, maybe one way is to modify the Dynamic type alias to be Any + Send, which then restricts all values that the scripting engine can use to be Send.

An alternative is to find some way to unwrap/rewrap a Scope'd value into basic types (similar to your idea of mapping to a HashMap<String, String>.

schungx commented 4 years ago

Tried it and it is extremely simple to make Scope Send+Sync.

pub trait Any: StdAny + Send + Sync { ... }

I may put this restriction under a new feature flag.

schungx commented 4 years ago

The new PR https://github.com/jonathandturner/rhai/pull/123 adds a sync feature.

Turn it on, and Dynamic, Variant, Engine, Scope and AST become Send + Sync.

Pull from the PR to try.

Raz-Hemo commented 4 years ago

Tried it out. Works beautifully, but there is one additional suggestion I have for the future. Scripts for games are usually a collection of functions for a single entity:

// entity1.rhai
fn on_spawn() {
...
}

fn tick() {
...
}

fn on_use() {
...
}

fn on_die() {
...
}
// more functions...

Then in the engine you would do something like: rhai_engine.eval_with_scope(&mut component.scope, component.script) But right now there is no way to call these functions, as they must be loaded into the rhai_engine itself. Which means a whole rhai::Engine instance for each component.

Consider this: There are 100 scripted entities on the map. That means there will be 100 engines as well, each of them having its own HashMap of engine defined functions (for example, send_message_to_entity, start_animation, set_resolution, etc.). This means two things:

  1. very expensive to create a new entity, as the entire engine needs to be replicated
  2. a memory scaling of num_entities * size_engine_libs, which would be horribly slow for anything beyond a tiny map with barely any scripts.

I can think of a few ways to solve this. One would be from the developer side, to split the script into its functions. so for the above example, you would have 4 script files for this entity. This is acceptable, but a bit inconvenient. The better solution IMO is to create the following function: Engine::eval_ast_call_with_scope(&self, ast: &AST, scope: &mut Scope, fn_name: &str) where fn_name is the function to call, for example "on_use".

What do you think?

Edit: This is actually less painful than I thought, because the Engine per entity is unnecessary - only one per entity type (like car, demon_enemy, etc). That could still be a problem for some games, but generally that's acceptable.

schungx commented 4 years ago

Have you tried out call_fn? It pretty much does what your eval_ast_call_with_scope does, which is to take an AST, a Scope, and then evaluate one function out of it.

I can't see how you can avoid having multiple Engine's because you'll probably want to execute scripts in parallel. An Engine by itself is not really large... all the hassles being with registering functions. If you have lots of custom functions to register, then the registration process is expensive. Otherwise, creating an Engine should be relatively quick.

I'll add a new_raw function that creates a raw Engine - one that doesn't have on_print, pretty-print type names etc. That would make the creation step extremely minimal.

EDIT: This PR https://github.com/jonathandturner/rhai/pull/123 has new_raw creation function for Engine.

schungx commented 4 years ago

BTW, there is an outstanding issue https://github.com/jonathandturner/rhai/issues/101 to benchmark Rhai, and I think a games engine is a great environment to do so.

I am really curious to know how it performs when scripts are being run extremely frequently. Right now the AST is stored simply as a tree with Box nodes. That's not very cache-friendly. Short of going into full-blown byte-codes, we can further optimize the speed by packing AST nodes into a dense array with offset indices instead of pointers. But that would require quite a bit of modifications to the existing code, and I am really interested to know if it'd be worth doing.

Raz-Hemo commented 4 years ago

The problem with call_fn is that it doesn't take a scope, which makes it impossible for scripts to keep any state using it.

Raz-Hemo commented 4 years ago

2 more problems that have come up:

  1. call_fn cannot call functions with 0 arguments (the FuncArgs cannot be created with ())
  2. Tried using engine.eval_with_scope, and to my surprise I couldnt call functions defined in the scripts i consumed into the engine, while engine.call_fn could. i.e. the commented out line doesn't work (could not find the function) and the other one does.
    //println!("{:?}", engine.eval_with_scope::<i32>(&mut script.scope, "on_lclick()"));
    println!("{:?}", engine.call_fn::<_, i32>("on_lclick", (1337 as i32)));

    Is this intentional? If so, why?

schungx commented 4 years ago

Yes, the eval... methods do not retain previous functions (clear them before executing), only consume... did. I never liked the way I did it... feels like a hack.

And call_fn has a problem with one or no arguments (since tuples, by default, need >= 2 elements), which you found.

In the process of streamlining Engine to be as light-weight as possible for repeated callings, I've changed the API as follows:

  1. None of the methods now retain functions across runs. Everything is now encapsulated into AST. This makes the Engine relatively stateless.

  2. Use AST::merge to collect different groups of functions for evaluation.

  3. call_fn now takes an AST directly. There are alternate versions call_fn1 and call_fn0 for one/no arguments respectively. Yes, I know this is ugly...

PR https://github.com/jonathandturner/rhai/pull/123 now has these new changes. You can see if it works better.

The way to use it is:

let ast = engine.compile(script)?;
let result: i32 = engine.call_fn1(&ast, "on_lclick", 1337_i32)?;

However, since script functions do not capture their environment, all data must be passed in (you notice that it doesn't take a Scope). So in order to achieve any side effect or do anything interesting, you probably have to pass the object in as a parameter, or register an external function to return a handle or something to all your scripts. Something like:

fn on_lclick(event) {
    let object = get_current_object(event.obj_id);
    object.spin();
}

Is this how you're doing it?

It might be better to allow a function to access the global Scope, but that means either implementing a scope chain or having to copy all scope values across each function call. Quite messy.

Raz-Hemo commented 4 years ago
schungx commented 4 years ago

About call_fn1 and call_fn0, why not just pass arguments as a vector of trait objects instead of a tuple?

Because that would require an allocation per call... not terribly efficient. A tuple is passed by value.

I toyed with the idea of using fixed-length arrays and supporting through arrays of up to 20 elements. However, can't figure out a way to avoid having to cast all array elements to Dynamic first.

* TBH it's not a big deal, but I think the ideal would be that `call_fn` would get a list of ASTs instead of one

That seems like a good idea, because I assume that it is just as easy to keep a Vec<AST> as to keep an AST. What if I make all the ..._ast and call_fn calls take a &[&AST]? Would that help?

* `call_fn` use the scope it was given instead of using `Scope::new`

call_fn doesn't take a scope right now, because it links directly to the script-evaluation mechanism which always creates a new Scope for function calls. It'll be simple to modify to take an external Scope though...

Raz-Hemo commented 4 years ago

What if I make all the ..._ast and call_fn calls take a &[&AST]? Would that help?

That would be awesome. Though I think the ...ast ones wouldn't really benefit from this, as call_fn is always the one you use in a multiple AST scenario, unless i'm missing something. If breaking API is an issue, I assume the only change necessary would just be a new call_fn_ast(&[&AST], fn_name: &str, args)

Also, seeing as I successfully called call_fn with a single argument, it's weird to me that another version is necessary, even in the 0 argument case. Even further - I looked into call_fn in api.rs and the tuple was converted to a vector anyway:

 pub fn call_fn<A: FuncArgs, T: Any + Clone>(
        &mut self,
        name: &str,
        args: A,
    ) -> Result<T, EvalAltResult> {
       // !!
        let mut values = args.into_vec();

doesn't that mean using a vector is even preferable?

schungx commented 4 years ago

You succeeded in calling call_fn with a single argument because I manually register versions of all standard support types. It will not work for any type, while the tuple trick allows all types. Try calling call_fn with one argument which is a custom type, it doesn't work. So it is inconsistent.

All the arguments ended up turning into a Vec, even for one or no argument cases. However, it is the API that's hard to formulate without the specialization feature which is only available in nightly and has not been stabilized. That's because a tuple also implements Any and there will be implementation conflicts if I put in a generic version with only one element that implements Any.

Making call_fn take a Vec parameter means one new allocation for each call. Not terribly efficient.

schungx commented 4 years ago

As for making call_fn take &[&AST], it may not be as easy as I originally thought. That's because the functions resolution mechanism goes very deep, and it is essential for Engine to keep a reference to this structure during the duration of this call (or pass this structure all the way through to the end). But there is no way to make sure this structure lives as long as the Engine (in fact, it usually doesn't).

Making call_fn take a custom Scope should be easy though...

EDIT: Not easy. Right now function calls are quite efficient - they use references to avoid copying the names of variables during every call to setup the scope, and the scope holds on to variable name references instead of copying them. I can't avoid copying variable names during every call_fn call if I pass in an external Scope -- although the parameter variables are deleted at the end of the call, Rust doesn't know that...

At this point, I think merging the AST's will be the best solution - it is quite fast and inexpensive if you have no code statements, just functions. That's because function definitions are immutable objects being held by Rc/Arc wrappers, and they are simply passed along while incrementing the count.

schungx commented 4 years ago

PR https://github.com/jonathandturner/rhai/pull/123 now takes a Scope in call_fn. Please test it.

Right now it clones the function variable names during each call when the external Scope is non-empty, otherwise it uses references. This needs to do for now, until I find a way to satisfy the borrow checker...

You can avoid this cost by making all functions take no parameters, and pass parameters as variables inside the Scope instead. This way, no strings need to be cloned. You can use the set_value method to update the scope.

Raz-Hemo commented 4 years ago

Sorry for the delay :)

First - if functions don't capture their environment, isn't passing the scope through call_fn kinda pointless?

And in other news - your new changes pretty much solved my problem, except the scope obviously. Something that occurred to me is maybe to pass a sort of self to the functions. I might just make it a HashMap of string to something. Any ideas on how to make this as seamless as possible to scripts?

schungx commented 4 years ago

First - if functions don't capture their environment, isn't passing the scope through call_fn kinda pointless?

It is exactly because functions are not closure (i.e. not capture environment) that you must have some means of passing external data into the function without resorting to a godzillion arguments. You put the external inside a Scope and pass it in. That's essentially the same as capturing them from the environment.

Any ideas on how to make this as seamless as possible to scripts?

Create some form of an object (can be a HashMap) holding all the stuff your functions need to access. It can also be a custom type with getters/setters/methods. This is usually the best, as you can hook the script up directly with the external mechanisms.

Check out the side_effects test for an example.

schungx commented 4 years ago
3\. There are alternate versions `call_fn1` and `call_fn0` for one/no arguments respectively.

https://github.com/jonathandturner/rhai/pull/128

OK, I found a way to get rid of the call_fn1 and call_fn0 methods. There is now only one call_fn version, which takes tuples all the way down to one and zero.

(xxx,) = tuple of one (learnt a new trick today) () = tuple of zero