Skip to content

Generic system config #17962

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 34 commits into from
Mar 12, 2025
Merged

Generic system config #17962

merged 34 commits into from
Mar 12, 2025

Conversation

@newclarityex
Copy link
Contributor

@newclarityex newclarityex commented Feb 21, 2025 •
edited
Loading

Objective

Prevents duplicate implementation between IntoSystemConfigs and IntoSystemSetConfigs using a generic, adds a NodeType trait for more config flexibility (opening the door to implement #14195?).

Solution

Followed writeup by @ItsDoot:
https://hackmd.io/@doot/rJeefFHc1x

Removes IntoSystemConfigs and IntoSystemSetConfigs, instead using IntoNodeConfigs with generics.

Testing

Pending

Showcase

N/A

Migration Guide

SystemSetConfigs -> ScheduleConfigs
SystemConfigs -> ScheduleConfigs
IntoSystemSetConfigs -> IntoScheduleConfigs<InternedSystemSet, M>
IntoSystemConfigs -> IntoScheduleConfigs<ScheduleSystem, M>

@newclarityex newclarityex marked this pull request as draft February 21, 2025 08:54
@ItsDoot ItsDoot self-requested a review February 21, 2025 08:57
@ItsDoot ItsDoot added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 21, 2025
@newclarityex newclarityex marked this pull request as ready for review February 23, 2025 08:11
@ItsDoot ItsDoot added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 23, 2025
ItsDoot
ItsDoot suggested changes Feb 23, 2025
View reviewed changes
Copy link
Contributor

@ItsDoot ItsDoot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass done! Good work so far!

However, one thing I don't mention below is that the impl_system_collection and impl_system_set_collection macros that provide implementations for tuples should be merged into a single impl_node_type_collection macro that uses a T: NodeType bound instead of hardcoding to ScheduleSystem or InternedSystemSet. It should be as simple as taking one of the macros, renaming it, adding the bounds, and using T in place of either of the aforementioned types.

newclarityex and others added 11 commits February 23, 2025 15:31
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/config.rs
f9390cb 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/config.rs
ec81bce 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/config.rs
976b6a2 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/config.rs
a7509cd 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/config.rs
58d125c 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/config.rs
e46050c 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/config.rs
f98b9a3 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/schedule.rs
d5dd05f 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex @ItsDoot
Update crates/bevy_ecs/src/schedule/schedule.rs
052986a 
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@newclarityex
move nodetype bound to trait definition
ad06313
@newclarityex
fix macro and into_config
d42eb93
ItsDoot
ItsDoot approved these changes Mar 7, 2025
View reviewed changes
Copy link
Contributor

@ItsDoot ItsDoot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ItsDoot ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 7, 2025
@hymm
Copy link
Contributor

hymm commented Mar 7, 2025

NodeConfigs feels a little too generic. Would it make sense to call it ScheduleConfigs?

@newclarityex
Copy link
Contributor Author

newclarityex commented Mar 8, 2025

NodeConfigs feels a little too generic. Would it make sense to call it ScheduleConfigs?

Yup, updated NodeConfigs and NodeConfig to ScheduleConfigs, although kept NodeType as is.

alice-i-cecile
alice-i-cecile reviewed Mar 10, 2025
View reviewed changes
crates/bevy_ecs/src/schedule/config.rs Outdated
}
}

/// Stores data to differentiate different Node types
Copy link
Member

@alice-i-cecile alice-i-cecile Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs doc links to Node.

alice-i-cecile
alice-i-cecile reviewed Mar 10, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile requested changes Mar 10, 2025
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the net effect here, but IMO the NodeType name is quite confusing, and not a good fit for a trait. How would Schedulable work for you?

Some little doc nits too :)

@newclarityex
Copy link
Contributor Author

newclarityex commented Mar 10, 2025

I really like the net effect here, but IMO the NodeType name is quite confusing, and not a good fit for a trait. How would Schedulable work for you?

Some little doc nits too :)

Renamed and updated some docs :>

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 10, 2025
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 10, 2025

Manually resolved merge conflicts. You might need a cargo fmt in there. Ping me to re-attempt the merge if needed.

auto-merge was automatically disabled March 10, 2025 22:09

Head branch was pushed to by a user without write access

@newclarityex
Copy link
Contributor Author

newclarityex commented Mar 11, 2025

Manually resolved merge conflicts. You might need a cargo fmt in there. Ping me to re-attempt the merge if needed.

i think we can reattempt it now

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 12, 2025
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 12, 2025

Awesome, thanks :) I appreciate it: merge conflicts can be super annoying on a project this big.

Merged via the queue into bevyengine:main with commit ecccd57 Mar 12, 2025
31 checks passed
@mnmaita
Copy link
Member

mnmaita commented Mar 13, 2025

FYI the migration guide doesn't seem to be up to date based on some naming changes made later.

@newclarityex
Copy link
Contributor Author

newclarityex commented Mar 13, 2025

FYI the migration guide doesn't seem to be up to date based on some naming changes made later.

Updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@ItsDoot ItsDoot ItsDoot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@newclarityex @hymm @alice-i-cecile @mnmaita @ItsDoot