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

Refactor hot reload to allow custom dynamic plugins besides dylib-based #683

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kkolyan
Copy link
Contributor

@kkolyan kkolyan commented Oct 9, 2024

AbstractDynamicPlugin trait introduced as a contract between engine and dynamic plugins. All dylib-specific code is hidden inside DyLibPlugin, the only built-in implementation of the AbstractDynamicPlugin.

@kkolyan
Copy link
Contributor Author

kkolyan commented Oct 9, 2024

I haven't tested it yet. Please confirm that approach is fine, then I'll test it.

@kkolyan
Copy link
Contributor Author

kkolyan commented Oct 9, 2024

My only concern (that's code based assumption, not checked manually) is that if Plugin unloaded successfully, but failed to load, it stucks forever in this state. Though, it seems that's original behavior. My proposal is to clear "need to reload" flag before reloading, so next change will force reloading again, allowing to recover. (note that there is no code in this MR regarding this)

@mrDIMAS
Copy link
Member

mrDIMAS commented Oct 10, 2024

Looks good, please test your changes thoroughly, these dynamic plugins sucked a lot of blood from me and I don't want to debug them yet again.

@kkolyan
Copy link
Contributor Author

kkolyan commented Oct 10, 2024

What about to perform following renamings?

  1. AbstractDynamicPlugin to DynamicPlugin
  2. DyLibPlugin to DyLybDynamicPlugin
  3. DynamicPlugin to DyLibHandle or something like.

@mrDIMAS
Copy link
Member

mrDIMAS commented Oct 10, 2024

I'm ok with that, feel free to rename it if you think new names are better.

@kkolyan
Copy link
Contributor Author

kkolyan commented Oct 12, 2024

During testing, it's occurred that hot reload was broken in e9ff227a (reproduced on macos Ventura 13.4.1 (22F82), rustc 1.81.0 (eeb90cda1 2024-09-04), by setting this version and cargo clean && RUSTFLAGS="-C prefer-dynamic=yes" cargo build --package game_dylib --no-default-features --features="dylib-engine" --profile dev-hot-reload && RUSTFLAGS="-C prefer-dynamic=yes" cargo run --package editor --no-default-features --features="dylib" --profile dev-hot-reload). Unfortunately I haven't access to Windows next two weeks, but two days ago I had experience on Windows that indirectly indicates that this problem affects Windows too.

fixed

@kkolyan
Copy link
Contributor Author

kkolyan commented Oct 13, 2024

merge after #687, because contains commits from.

@kkolyan
Copy link
Contributor Author

kkolyan commented Oct 13, 2024

tested on MacOS

@kkolyan kkolyan changed the title Refactor hot reload to allow custom dynamic plugins besides dylib-based WIP: Refactor hot reload to allow custom dynamic plugins besides dylib-based Oct 13, 2024
@kkolyan kkolyan marked this pull request as draft October 13, 2024 23:07
@kkolyan kkolyan changed the title WIP: Refactor hot reload to allow custom dynamic plugins besides dylib-based Refactor hot reload to allow custom dynamic plugins besides dylib-based Oct 13, 2024
@kkolyan kkolyan requested a review from mrDIMAS October 17, 2024 23:32
@kkolyan kkolyan marked this pull request as ready for review October 17, 2024 23:33
@kkolyan kkolyan marked this pull request as draft October 17, 2024 23:58
@kkolyan kkolyan marked this pull request as ready for review October 18, 2024 01:26
@mrDIMAS
Copy link
Member

mrDIMAS commented Oct 18, 2024

Could you please fix CI errors? It seems that there's one warning treated like an error and it should be easy to fix.



/// Implementation of DynamicPluginTrait that [re]loads Rust code from Rust dylib .
pub struct DyLybDynamicPlugin {
Copy link
Member

Choose a reason for hiding this comment

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

Typo DyLyb..., should be DyLib...

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