Skip to content
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

Apply LUA api providers before loading script. #98

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Conversation

ConnorBP
Copy link
Contributor

Fixes issue where api is not available during script initialization. #97

PR is not yet complete. There is still a callback error caused by: error converting Lua nil to userdata to investigate which happens when the api function is called.

Fixes issue where api is not available during script initialization.
@ConnorBP
Copy link
Contributor Author

Found what is causing the NIL error. It happened upon fetching the bevy "world" from globals. It is not set on script load execution, only after event execution is fired. This makes it so that any API function which accesses world state cannot be used during script load. To fix this requires a refactor involving changing the script_add_synchronizer into an exclusive system with a SystemState

@ConnorBP
Copy link
Contributor Author

Update: the other issues have their own concerns that should be in another PR. But the api registration issue is solved by this PR and should be considered Complete.

I will open a different PR for potential api refactoring for connecting callback functions with event signals.

Copy link
Owner

@makspll makspll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one small nit

languages/bevy_mod_scripting_lua/src/lib.rs Show resolved Hide resolved
@ConnorBP
Copy link
Contributor Author

ConnorBP commented Jan 23, 2024

Looks good, just one small nit

`providers.attach_all()' requires a mutex as it's argument.

Also get_mut is describes as follows:
`
Returns a mutable reference to the underlying data.

Since this call borrows the Mutex mutably, no actual locking needs to take place -- the mutable borrow statically guarantees no locks exist.
`
So it should not be an issue

@makspll
Copy link
Owner

makspll commented Jan 24, 2024

Sounds good, let's just fix CI and happy to merge (rustfmt not aarch64, that should fail)

@ConnorBP
Copy link
Contributor Author

It worked it looks like... but then failed to cache?

@ConnorBP ConnorBP requested a review from makspll January 24, 2024 21:59
@makspll makspll merged commit 3362815 into makspll:main Jan 25, 2024
11 of 13 checks passed
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.

2 participants