-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 missing schedules in add_systems
doc (#11814)
#11815
base: main
Are you sure you want to change the base?
Add missing schedules in add_systems
doc (#11814)
#11815
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
I'm not sure I understand why, when I try to run the rustdoc, I get: error[E0425]: cannot find value `Update` in this scope
--> crates/bevy_ecs/src/system/combinator.rs:57:5
|
40 | Update,
| ^^^^^^ not found in this scope
error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> crates/bevy_ecs/src/system/combinator.rs:56:5
|
39 | app.add_systems(
| ^^^^^^^^^^^
40 | Update,
| ____________-
41 | |/ my_system.run_if(Xor::new(
42 | || IntoSystem::into_system(resource_equals(A(1))),
43 | || IntoSystem::into_system(resource_equals(B(1))),
44 | || // The name of the combined system.
45 | || std::borrow::Cow::Borrowed("a ^ b"),
46 | || )));
| || -
| ||__|
| |__help: remove the extra argument
| unexpected argument of type `NodeConfigs<Box<(dyn bevy_ecs::system::System<In = (), Out = ()> + 'static)>>`
|
note: method defined here
--> /home/purfakt/dev/perso/bevy/crates/bevy_ecs/src/schedule/schedule.rs:249:12
|
249 | pub fn add_systems<M>(&mut self, systems: impl IntoSystemConfigs<M>) -> &mut Self {
| ^^^^^^^^^^^
error: aborting due to 2 previous errors |
This is because app doesn't actually exist in bevy_ecs. It's just mocked in the doc comments as a schedule. Not sure how to fix this. |
I think this can't be solved unless we import bevy_app into bevy_ecs (that would probably defeat the purpose of bevy_ecs being independent) Here we have the definition of the var: /// # let mut app = Schedule::default();
We would first remove the # to clear that we are not talking about the App here and rename app to schedule /// let mut schedule = Schedule::default();
That would clear up the problem, I think |
We could move the add_systems impl on App onto world and just use that on the impl on App. More code duplication, but might be worth it. |
Oh I see. I'm not sure if I'm capable or entitled to make this change. I didn't expect my first PR to be tagged controversial. Especially, when it was just a documentation update! |
Well, this means that the issue is rather good, as this shows that there is a misunderstood going about what is the app that the documentation is talking about, a third opinion might be good here in regarding of what to do for sure. |
I wouldn't mind having this pr change the examples in |
Objective
Fixes (#11814)
Solution
I added
Update
where it was missing in the rustdoc.Uncovered problem
The previous doc was mocking the app as @hymm said:
Proposed solutions
@pablo-lua suggested to just rename
app
inschedule
and let the example be correct without the changes of the PR as it is, in fact, aschedule
@hymm suggested to move the
add_systems
impl onApp
ontoWorld
and just use that on the impl onApp