-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Small cleaning up + Fix for crash when looping through empty Vec #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catches, just a few comments
I suggest we move the existing Wrapper example into lua, as it only really shows how to work with Lua. |
btw while working with the Wrappers in Rhai, i noticed that i always have to manually implement RhaiCopy, even though it's completely empty. #[derive(Resource, Reflect, Default, Clone, Debug)]
#[reflect(Resource, RhaiProxyable)]
pub struct MyThing {
usize: usize,
string: String,
array: Vec<usize>,
}
impl RhaiCopy for MyThing {} Is this wanted or an oversight? |
This reverts commit c282ada.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more discussion, and minor changes
So the discussion above might shed some more light on this but let me know if you are still confused. Basically if you don't register RhaiProxyable yourself, you will receive a Registering it yourself is needed when you need a custom proxy type which has custom methods and behaviours attached. But because Rhai doesn't have support for generating those nicely it's not easilly available here yet as seen in lua. The need for RhaiCopy arises from the automatic implementation for RhaiProxyable which assumes your type is its own proxy and that it is converted into this proxy by copying, hence the name of the trait. This lets you register all Rhai goodies on your type in your APIProvider and use them that way. But of course since you are not using ReflectedValue anymore you lose the automatically generated getters for reflected fields. Hope this makes sense |
Co-authored-by: Maksymilian Mozolewski <makspl17@gmail.com>
Co-authored-by: Maksymilian Mozolewski <makspl17@gmail.com>
So this pull request should be good now, applied all your suggestions. And am looking forward for the wrapper rewrite ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks again, great work!
This pull request cleans the Rhai side of the project up a bit.
get_children
that didn't even workget_parent
, before it wasn't found by Scripts, presumably because not the correct World was asked for (&ScriptWorld
instead ofScriptWorld
). Also made it return a Entity if a parent was found, and a i32 of value -1 if not, instead of Option, as that isnt really supported by Rhai.reload_script
as the game often crashed there. This adds on check if entity exists before accessing #68