-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add cached run_system
API
#14920
Add cached run_system
API
#14920
Conversation
Note: Now |
Great addition! Code looks good to me, but can we make the docs a bit more approachable? I understand them, but I doubt the average user will. Why is this useful? When should I prefer the uncached vs the cached variant? Maybe mention Local, and EventReader in particular. Further, can we link this from more places? I recall some part of the docs saying "running a system like this will not cache anything", maybe that can link to these? |
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.
Seconding the doc changes, and also requesting that this be a full replacement for the existing run_system API. The ergonomics of this are so much nicer, and we don't need yet another variant.
I think making this the default API (aka shortest name) would be reasonable, but I'm not sure about fully removing the existing API. Sometimes you do want to store and pass around a type-erased identifier for the system, like in an Removing |
Let's swap it to the default API for now then. I wouldn't mind removing |
Completely agree with @benfrankel here based on my experience, as |
I find it strange that pub struct CachedSystemId<S: System> {
/// Cached `SystemId`.
pub id: SystemId<S::In, S::Out>,
} This will also allow the type level to more clearly define what the resource does and get rid of unnecessary generic parameters. |
Wrt making
This PR is blocked on these questions. If there's no good way forward to make |
My two cents:
These are not strong opinions, feel free to disregard. |
Okay, here's what the full rename would look like along those lines:
Notes:
You can't exactly use Also, if |
Ready for review again. Changes:
|
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.
The PR looks in a pretty good shape to me.
The only maybe issue I can see is that the ZST check won't work for systems created by pipe
and map
, even when the systems and closures involved are ZSTs. This is due to pipe
and map
eagerly calling into_system
on their receiver, which will result in non-ZST types even when called on ZST types. The solution would be changing them to be lazy and call into_system
on their receiver only when into_system
is directly called on them.
I'm happy enough to call this "fix in follow-up", but this is a good find. |
|
Made an issue: #15373 |
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.
I'm happy with the API now. Thanks for exploring this so thoroughly.
Really lovely work; thanks for seeing this through. |
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.
Sick! I do think as follow up we should consider flipping this around to be the "default" oneshot api.
Hello, I am testing the latest main branch of bevy now with this feature merged, but I am running into the compile time assert check for closures and function pointers even though my system should be normal? My system definition looks like this: pub fn start_road_building_system(
gr: Res<GlobalResources>,
gs: Res<GameState>,
mut players: Query<(&mut PlayerStateECS, &HouseECS)>,
junctions: Query<(&JunctionECS)>,
road_paths: Query<(&NewRoadPieceECS)>,
roads: Query<(&NewRoadECS)>,
) { ... } And I am first using Is there something I am missing, should not this work? |
I think this is a bug in how the |
# Objective - Fixes #15373 - Fixes #14920 (comment) ## Solution - Make `IntoSystem::pipe` and `IntoSystem::map` return two new (possibly-ZST) types that implement `IntoSystem` and whose `into_system` method return the systems that were previously being returned by `IntoSystem::pipe` and `IntoSystem::map` - Don't eagerly call `IntoSystem::into_system` on the argument given to `RunSystemCachedWith::new` to avoid losing its ZST-ness ## Testing - Added a regression test for each issue ## Migration Guide - `IntoSystem::pipe` and `IntoSystem::map` now return `IntoPipeSystem` and `IntoAdapterSystem` instead of `PipeSystem` and `AdapterSystem`. Most notably these types don't implement `System` but rather only `IntoSystem`.
Objective
Working with
World
is painful due to lifetime issues and a lack of ergonomics, so you may want to delegate to the system API. Your current options are:world.run_system_once
, which initializes the system each time it's called (performance cost) and doesn't supportLocal
. The docs recommend users not use this method outside of diagnostic use cases like unit tests.world.run_system
, which requires you to register the system and store theSystemId
somewhere (made easier by implementingFromWorld
for a newtypedLocal
, unless you're in e.g. a customCommand
impl).These options work, but you're choosing between a performance cost and an ergonomic challenge.
Solution
Provide a cached
run_system
API that accepts anS: IntoSystem
and checks for aCachedSystemId<S::System>(SystemId)
resource. If it doesn't exist, it will register the system and save itsSystemId
in that resource.In other words, it hides the "save the
SystemId
in aLocal
orResource
" pattern as an implementation detail.Prior work: #10469.
Testing
This approach worked in a proof-of-concept: https://github.com/benfrankel/bevy_jam_template/blob/b34ee29531e3ceae287a9cc44ec6c520e83a4cdd/src/util/patch/run_system_cached.rs#L35.
A new unit test was added and it passes in CI.