Move schedule name into `Schedule` #9600

hymm · 2023-08-27T22:01:51Z

Objective

Move schedule name into Schedule to allow the schedule name to be used for errors and tracing in Schedule methods
Fixes Schedule build errors should include the schedule in which the problem occurred when reporting problems #9510

Solution

Move label onto Schedule and adjust api's on World and Schedule to not pass explicit label where it makes sense to.
add name to errors and tracing.
Schedule::new now takes a label so either add the label or use Schedule::default which uses a default label. default is mostly used in doc examples and tests.

Changelog

move label onto Schedule to improve error message and logging for schedules.

Migration Guide

Schedule::new and App::add_schedule

// old
let schedule = Schedule::new();
app.add_schedule(MyLabel, schedule);

// new
let schedule = Schedule::new(MyLabel);
app.add_schedule(schedule);

if you aren't inserting the schedule into the world and are using the schedule directly you can use the default constructor which reuses a default label.

// old
let schedule = Schedule::new();
schedule.run(world);

// new
let schedule = Schedule::default();
schedule.run(world);

Schedules:insert

// old
let schedule = Schedule::new();
schedules.insert(MyLabel, schedule);

// new
let schedule = Schedule::new(MyLabel);
schedules.insert(schedule);

World::add_schedule

// old
let schedule = Schedule::new();
world.add_schedule(MyLabel, schedule);

// new
let schedule = Schedule::new(MyLabel);
world.add_schedule(schedule);

alice-i-cecile

Very straightforward, but extremely useful for diagnostics.

JoJoJet · 2023-08-27T23:05:39Z

crates/bevy_ecs/src/world/mod.rs

+        let Some((_, mut schedule)) = self
            .get_resource_mut::<Schedules>()
            .and_then(|mut s| s.remove_entry(label))


Should remove_entry still return the label?

I'm not sure if remove_entry has any use. We already have remove, so we should just delete remove_entry if it doesn't. It's slightly useful in that you can get the owned Boxed label for free, but not sure that's worth the added api.

I should switch to using remove here, since we're not using the label.

Fair enough. If #7762 gets merged then we should definitely get rid of remove_entry, since there'd be no benefit to keeping the schedule label.

JoJoJet

I have one concern about docs, but otherwise LGTM.

JoJoJet · 2023-08-28T12:46:32Z

crates/bevy_ecs/src/schedule/schedule.rs

+/// Creates a schedule with a default label. Only use in situations where
+/// where you don't care about the [`ScheduleLabel`]. Inserting a default schedule
+/// into the world risks overwriting another schedule. For most situations you should use
+/// [`Schedule::new`].
 impl Default for Schedule {
    fn default() -> Self {


Will this documentation be visible to users?

It should be yes. I'll verify before merging though.

alice-i-cecile · 2023-08-28T18:03:22Z

The docs on the trait impl show up fine :)

alice-i-cecile · 2023-08-28T18:22:49Z

Looks like merge conflict-y issues: can you rebase and fix this up?

alice-i-cecile · 2023-08-28T18:55:22Z

@hymm CI is still failing: looks like another use of Schedule was added.

JoJoJet · 2023-08-28T21:39:09Z

The docs on the trait impl show up fine :)

They show up, but I don't think they'll show up where anyone will see them. It'd have to be on the default fn to be useful, I think.

# Objective - Make the default docs more useful like suggested in #9600 (comment) ## Solution - Move the documentation to the `fn default()` method instead of the `impl Default`. Allows you to view the docs directly on the function without having to go to the implementation. ### Before ![Screenshot 2023-08-29 at 18 21 03](https://github.com/bevyengine/bevy/assets/104745335/6d31591e-f190-4b8e-8bc3-a570ada294f0) ### After ![Screenshot 2023-08-29 at 18 19 54](https://github.com/bevyengine/bevy/assets/104745335/e2442ec1-593d-47f3-b539-8c77a170f0b6)

# Objective - Move schedule name into `Schedule` to allow the schedule name to be used for errors and tracing in Schedule methods - Fixes bevyengine#9510 ## Solution - Move label onto `Schedule` and adjust api's on `World` and `Schedule` to not pass explicit label where it makes sense to. - add name to errors and tracing. - `Schedule::new` now takes a label so either add the label or use `Schedule::default` which uses a default label. `default` is mostly used in doc examples and tests. --- ## Changelog - move label onto `Schedule` to improve error message and logging for schedules. ## Migration Guide `Schedule::new` and `App::add_schedule` ```rust // old let schedule = Schedule::new(); app.add_schedule(MyLabel, schedule); // new let schedule = Schedule::new(MyLabel); app.add_schedule(schedule); ``` if you aren't using a label and are using the schedule struct directly you can use the default constructor. ```rust // old let schedule = Schedule::new(); schedule.run(world); // new let schedule = Schedule::default(); schedule.run(world); ``` `Schedules:insert` ```rust // old let schedule = Schedule::new(); schedules.insert(MyLabel, schedule); // new let schedule = Schedule::new(MyLabel); schedules.insert(schedule); ``` `World::add_schedule` ```rust // old let schedule = Schedule::new(); world.add_schedule(MyLabel, schedule); // new let schedule = Schedule::new(MyLabel); world.add_schedule(schedule); ```

# Objective - Make the default docs more useful like suggested in bevyengine#9600 (comment) ## Solution - Move the documentation to the `fn default()` method instead of the `impl Default`. Allows you to view the docs directly on the function without having to go to the implementation. ### Before ![Screenshot 2023-08-29 at 18 21 03](https://github.com/bevyengine/bevy/assets/104745335/6d31591e-f190-4b8e-8bc3-a570ada294f0) ### After ![Screenshot 2023-08-29 at 18 19 54](https://github.com/bevyengine/bevy/assets/104745335/e2442ec1-593d-47f3-b539-8c77a170f0b6)

# Objective Fixes #11411 ## Solution - Added a simple example how to create and configure custom schedules that are run by the `Main` schedule. - Spot checked some of the API docs used, fixed `App::add_schedule` docs that referred to a function argument that was removed by #9600. ## Open Questions - While spot checking the docs, I noticed that the `Schedule` label is stored in a field called `name` instead of `label`. This seems unintuitive since the term label is used everywhere else. Should we change that field name? It was introduced in #9600. If so, I do think this change would be out of scope for this PR that mainly adds the example.

# Objective Fixes bevyengine#11411 ## Solution - Added a simple example how to create and configure custom schedules that are run by the `Main` schedule. - Spot checked some of the API docs used, fixed `App::add_schedule` docs that referred to a function argument that was removed by bevyengine#9600. ## Open Questions - While spot checking the docs, I noticed that the `Schedule` label is stored in a field called `name` instead of `label`. This seems unintuitive since the term label is used everywhere else. Should we change that field name? It was introduced in bevyengine#9600. If so, I do think this change would be out of scope for this PR that mainly adds the example.

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Aug 27, 2023

alice-i-cecile approved these changes Aug 27, 2023

View reviewed changes

alice-i-cecile added this to the 0.12 milestone Aug 27, 2023

alice-i-cecile requested a review from JoJoJet August 27, 2023 22:07

JoJoJet reviewed Aug 27, 2023

View reviewed changes

alice-i-cecile requested a review from JoJoJet August 28, 2023 12:45

JoJoJet approved these changes Aug 28, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 28, 2023

alice-i-cecile added this pull request to the merge queue Aug 28, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 28, 2023

hymm force-pushed the schedule-name-2 branch from 217bdc8 to 930380a Compare August 28, 2023 18:37

alice-i-cecile enabled auto-merge August 28, 2023 18:40

auto-merge was automatically disabled August 28, 2023 19:13
Head branch was pushed to by a user without write access

hymm force-pushed the schedule-name-2 branch from 930380a to 5d1dca8 Compare August 28, 2023 19:13

hymm added 11 commits August 28, 2023 12:50

move label to be stored on Schedule

74d6b9c

fix docs

50bd793

clarify doc comment

c3ca8a6

add schedule name to initialization error

0d678f5

add schedule name to build errors and warnings.

9886af7

fix world doc

fb29996

fix rebase issues

1f8e6fd

fix some accidental changes

6892f6b

run cargo fmt

66a569a

clippy

ec59d1c

fix doc example

c5d808f

hymm added 2 commits August 28, 2023 12:50

use remove instead of remove_entry

009dee3

fix new errors

8e022a9

hymm force-pushed the schedule-name-2 branch from 5d1dca8 to 8e022a9 Compare August 28, 2023 19:50

alice-i-cecile added this pull request to the merge queue Aug 28, 2023

Merged via the queue into bevyengine:main with commit 33fdc5f Aug 28, 2023
20 checks passed

hymm deleted the schedule-name-2 branch August 28, 2023 21:35

ameknite mentioned this pull request Aug 30, 2023

Move default docs #9638

Merged

dmlary mentioned this pull request Sep 15, 2023

System Stepping implemented as Resource #8453

Merged

cart mentioned this pull request Oct 13, 2023

News: Release 0.12 bevyengine/bevy-website#754

Merged

43 tasks

Malax mentioned this pull request Jan 25, 2024

Add custom schedule example #11527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move schedule name into `Schedule` #9600

Move schedule name into `Schedule` #9600

hymm commented Aug 27, 2023 •

edited

Loading

alice-i-cecile left a comment

JoJoJet Aug 27, 2023

hymm Aug 28, 2023

JoJoJet Aug 28, 2023

JoJoJet left a comment

JoJoJet Aug 28, 2023

alice-i-cecile Aug 28, 2023

alice-i-cecile commented Aug 28, 2023

alice-i-cecile commented Aug 28, 2023

alice-i-cecile commented Aug 28, 2023

JoJoJet commented Aug 28, 2023

Move schedule name into Schedule #9600

Move schedule name into Schedule #9600

Conversation

hymm commented Aug 27, 2023 • edited Loading

Objective

Solution

Changelog

Migration Guide

alice-i-cecile left a comment

Choose a reason for hiding this comment

JoJoJet Aug 27, 2023

Choose a reason for hiding this comment

hymm Aug 28, 2023

Choose a reason for hiding this comment

JoJoJet Aug 28, 2023

Choose a reason for hiding this comment

JoJoJet left a comment

Choose a reason for hiding this comment

JoJoJet Aug 28, 2023

Choose a reason for hiding this comment

alice-i-cecile Aug 28, 2023

Choose a reason for hiding this comment

alice-i-cecile commented Aug 28, 2023

alice-i-cecile commented Aug 28, 2023

alice-i-cecile commented Aug 28, 2023

JoJoJet commented Aug 28, 2023

Move schedule name into `Schedule` #9600

Move schedule name into `Schedule` #9600

hymm commented Aug 27, 2023 •

edited

Loading