Simplify world schedule methods

[JoJoJet]

Objective

Methods for interacting with world schedules currently have two variants: one that takes impl ScheduleLabel and one that takes &dyn ScheduleLabel. Operations such as run_schedule or schedule_scope only use the label by reference, so there is little reason to have an owned variant of these functions.

Solution

Decrease maintenance burden by merging the ref variants of these functions with the owned variants.

---

Changelog

• Deprecated World::run_schedule_ref. It is now redundant, since World::run_schedule can take values by reference.

Migration Guide

The method World::run_schedule_ref has been deprecated, and will be removed in the next version of Bevy. Use run_schedule instead.

[hymm]

So with the methods that take &dyn ScheduleLabel, it's very easy to end up with a boxed label that you need to remember to dereference or things will fail silently. You can see this if you go to https://github.com/bevyengine/bevy/blob/main/crates/bevy_app/src/main_schedule.rs#L146 and change the line to let _ = world.try_run_schedule_ref(label);. In this case the label is in fact double boxed.

We should remove this footgun before removing the owned variants. One potential solution is this PR #7762. But I'm unsure if that PR is the right direction.

[james7132]

Roping in @cart for this since this does have some major UX implications for the API.

[JoJoJet]

So with the methods that take &dyn ScheduleLabel, it's very easy to end up with a boxed label that you need to remember to dereference or things will fail silently.

Is that not also true for impl ScheduleLabel?

[JoJoJet]

So the footgun does exist for impl ScheduleLabel as well; the following test fails on main:

#[test]
fn test_boxed_label() {
    use crate::{self as bevy_ecs, world::World};

    #[derive(ScheduleLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
    struct A;

    let mut world = World::new();
    world.add_schedule(Default::default(), A);
    world
        .try_run_schedule(Box::new(A) as BoxedScheduleLabel) 
        .unwrap(); // This returns `Err` since `Box::new(A) != A`
}

I agree that we should fix this footgun but I don't think this PR is affected by it.

[JoJoJet]

I have a fix for the aforementioned footgun in #8436.

[alice-i-cecile]

I think this is a net improvement, but I think in this case it may be worth it to make an API that can accept either the owned form or the reference using impl Deref.

I would like to avoid the silly duplication though.

[cart]

Operations such as run_schedule or schedule_scope only use the label by reference, so there is little reason to have an owned variant of these functions.

Having both was a UX decision. As a rule of thumb, we try to make as many user-facing label apis by-value as possible. It reads better, one less character to type, and the consistency across all label apis is nice. If someone is passing a label into a function, they can generally expect by value.

The by-ref calls are almost always going to be "bevy internals code" (ex: retrieving an existing stored label and doing something with it). Users trying to run a schedule manually should not need to worry about references. I think it is worth it to make this by-value and then have the _ref variant for the less common (generally internal) cases. Pretty much everything user-facing can (and i would assert should) use by-value.

[JoJoJet]

I took @alice-i-cecile's idea and made these methods take AsRef<dyn ScheduleLabel>, so now they accept labels either by value or by reference. Also, I made this change less breaking by deprecating run_schedule_ref, which was the only one of these methods that existed in 0.10.

[alice-i-cecile]

Great, I think that's a nice balance of pretty end user API and no code duplication.

[cart]

Best of both worlds. Love it!