-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added add_scripting_api function to AppBuilder #23
Added add_scripting_api function to AppBuilder #23
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.
Hi! First of all thank you for your contribution ❤️! I agree with the benefits of supporting your use-case.
I think it would be fine to just add this as another method in BuildScriptingRuntime
trait as you said.
It would be really great if we also added a test and/or example and/or a documentation paragraph that would allow keeping track of supported features and allow users to easily discover them. If you are not interested in contributing those that's fine - I can do that later :)
- Merged add_scripting_api into the BuildScriptingRuntime - Added an example - Working on documentation
… feature/add_plugin_builder
Gladly! I moved the code to the BuildScriptingRuntime. Because of that there is also still one use of the callbacks_resource code you mentioned, so extraction of that code is not required and with that I resolved that comment. I added a working example for lua and will work on the documentation a bit tomorrow. Honestly I tried implementing mlua myself and failing miserably at that before I found this crate. It was all a bit above my skill level, saying that I looked into the test script and have to admit I'm not sure how that's working. So ideally I work on the documentation some more, add a rhai example, work on any comments you may have and then leave writing the test(s) to you 😉. And thank you for this crate! |
Hi, I added a short description in the book and the Rust docs. One thing which maybe is a good improvement for the future is to remove the and make it like this: App::new()
.add_plugins(DefaultPlugins)
.init_scripting::<LuaRuntime>()
.add_scripting_api::<LuaRuntime>(|runtime| {
..
|); It will make it more clear that Also it is maybe a bit more inline with Bevy naming conventions like I understand it may be a big change for now, but maybe in a future version. Anyway for I think this pull-request is complete and I'm glad I could contribute to this crate, don't hesitate to comment on the code though and let me fix stuff up 👍. |
Merged. Thank you for your work on this! :) |
Proposed Changes
The possibility to add lua functions from different plugins after running
.add_scripting::<LuaRuntime>(|b| { // not needed })
At the moment all the lua functions have to be defined in the
add_scripting
call this becomes messy quickly, especially when working with big projects with different parts of the game put in separate plugins.With the proposed change it it possible to do the following:
I think this change promotes separation of concern, improves readability in big projects and makes defining lua system-functions more flexible.
p.s.
I made this change without wanting to touch the original code. I guess this code can easily be merged into the
BuildScriptingRuntime
.