Conversation
5e37c0f to
f777430
Compare
ChristopherBiscardi
left a comment
There was a problem hiding this comment.
Code seems fine generally. The std::mem::take and loop of the hashmap happens every update regardless of if there are hooks defined or not.
I'm not aware of why this is x-contentious though, so this is just a review of the code as implemented, not a review of the exclusive hook feature.
I'd love to see an example for this in examples/
|
I've marked this as Contentious because I don't immediately understand the use case, and am not convinced that this is the best way to accomplish these goals. Better documentation may be the right fix, but this PR also needs end to end tests verifying that this works as intended. |
1e9704a to
e482770
Compare
|
Any meetings or places to discuss it? I'm currently trying to find out why im getting completely unrelated errors to this code even though i've already tested it. This code aims to allow better modding support, maybe we can discuss with core team to implement a DynamicPlugin system to better utilize this and allow for dynamic plugin building? My vision of bevy is a game engine with lots of redundancy and modularity, while allowing to dynamically modify the game engine at runtime without a restart being required, unlike winit's event handler. This would allow us to have a lot more modding support, maybe even make it built into bevy directly. |
|
Also i have no idea how to fix the CI issues considering its not related to my changes. |
CI was broken with the release of Rust 1.90. Just updated your branch to incorporate the fix..
Is this not possible with our existing schedule APIs? Schedules already live in the World, and can be mutated at runtime from within an exclusive system. The only thing it's missing is the ability to change out the top level app runner while it is currently running. At the same time, I don't think this is particularly ergonomic right now, butthis does seem like an easy path to crashes/violated logical invariants, and thus shouldn't be explicitly encouraged unless there is a very compelling use case for it. |
Not only that, it also doesnt allow you to access other subapps which is a shame. This not only allows access to other subapps but also allows modifying and creating new ones, giving further modding possibilities.
That issue was resolved by making app hooks unaccessible during app runtime, resulting in no violation. What is being done fully respects rust's borrow semantics, with no extra unsafe usage, meaning there should be no issues caused by this. The only thing i have to add is to turn the hook hashmap into an option and make it panic when you attempt to access it during runtime. |
f6ca681 to
88f72b6
Compare
|
Added a new example |
88f72b6 to
48e1a04
Compare
Co-authored-by: Chris Biscardi <chris@christopherbiscardi.com>
05737af to
9ed4a43
Compare
This PR depends on another PR #21239
Objective
Solution
Testing
Ran cargo check
Tested to ensure no runtime errors.
Showcase
cargo run --example exclusive_hook