Skip to content
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

Adding ScheduleGraph::contains_set #16206

Merged

Conversation

urben1680
Copy link
Contributor

@urben1680 urben1680 commented Nov 1, 2024

Objective

The schedule graph can easily confirm whether a set is contained or not.

This helps me in my personal project where I write an extension trait for Schedule and I want to configure a specific set in its methods. The set in question has a run condition though and I don't want to add that condition to the same schedule as many times as the trait methods are called. Since the non-pub set is unknown to the schedule until then, a contains_set is sufficient.

It is probably trivial to add a method that returns an Option<NodeId> as well but as I personally don't need it I did not add that. If it is desired I can do so here though. It might be unneeded to have a contains_set then because one could check is_some on the returned id in that case.

An argument against that is that future changes may be easier if only a contains_set needs to be ported.

Solution

Added ScheduleGraph::contains_set.

Testing

I put the below showcase code into a temporary unit test and it worked. If wanted I add it as a test too but I did not see that other more somewhat complicated methods have tests


Showcase

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

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

let mut schedule = Schedule::new(MySchedule);
assert_eq!(schedule.graph().contains_set(MySet), false);
schedule.configure_sets(MySet);
assert_eq!(schedule.graph().contains_set(MySet), true);

@urben1680
Copy link
Contributor Author

urben1680 commented Nov 1, 2024

For funsies, this is my current workaround:

fn my_set_previously_unknown(schedule: &mut Schedule) -> bool {
    let (lower_bound_before, upper_bound_before) = schedule.graph().system_sets().size_hint();
    schedule.configure_sets(MySet);
    let (lower_bound_after, upper_bound_after) = schedule.graph().system_sets().size_hint();

    const EXPECT: &'static str = "ScheduleGraph::system_sets() expected to impl ExactSizeIterator";
    debug_assert_eq!(Some(lower_bound_before), upper_bound_before, "{EXPECT}");
    debug_assert_eq!(Some(lower_bound_after), upper_bound_after, "{EXPECT}");

    lower_bound_before < lower_bound_after
}

crates/bevy_ecs/src/schedule/schedule.rs Outdated Show resolved Hide resolved
@alice-i-cecile 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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Nov 3, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 3, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Nov 3, 2024
Merged via the queue into bevyengine:main with commit 1e47604 Nov 3, 2024
32 checks passed
@urben1680 urben1680 deleted the schedule-graph-contains-set branch November 3, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants