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.

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

Make run_if_inner public and rename to run_if_dyn #9576

Merged
merged 7 commits into from
Aug 27, 2023
Merged

Make run_if_inner public and rename to run_if_dyn #9576

merged 7 commits into from
Aug 27, 2023

Conversation

st0rmbtw
Copy link
Contributor

@st0rmbtw st0rmbtw commented Aug 25, 2023 •
edited
Loading

Objective

Sometimes you want to create a plugin with a custom run condition. In a function, you take the Condition trait and then make a BoxedCondition from it to store it. And then you want to add that condition to a system, but you can't, because there is only the run_if function available which takes impl Condition<M> instead of BoxedCondition. So you have to create a wrapper type for the BoxedCondition and implement the System and ReadOnlySystem traits for the wrapper (Like it's done in the picture below). It's very inconvenient and boilerplate. But there is an easy solution for that: make the run_if_inner system that takes a BoxedCondition public. Also, it makes sense to make in_set_inner function public as well with the same motivation.

image
A chunk of the source code of the bevy-inspector-egui crate.

Solution

Make run_if_inner function public.
Rename run_if_inner to run_if_dyn.

Make in_set_inner function public.
Rename in_set_inner to in_set_dyn.

Changelog

Changed visibility of run_if_inner from pub(crate) to pub.
Renamed run_if_inner to run_if_dyn.

Changed visibility of in_set_inner from pub(crate) to pub.
Renamed in_set_inner to in_set_dyn.

Migration Guide

@ItsDoot
Copy link
Contributor

ItsDoot commented Aug 25, 2023

I would recommend a rename as well while we're at it. Perhaps run_if_dyn or run_if_ref since those seem like relatively common suffixes in Bevy for this type of thing.

@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 labels Aug 26, 2023
alice-i-cecile
alice-i-cecile approved these changes Aug 26, 2023
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Helpful motivation in the PR description, thanks :)

I'd prefer a more descriptive name and doc strings for this newly public type, but won't block on it. run_if_dyn gets my vote.

@st0rmbtw
Copy link
Contributor Author

@alice-i-cecile I'm not good at commenting things, so I'm not sure about docs. Could you help me, please? :)

@st0rmbtw
Copy link
Contributor Author

Does it make sense to also make the in_set_inner public?

@alice-i-cecile
Copy link
Member

Does it make sense to also make the in_set_inner public?

Sure, I think the same logic applies for that.

I'll leave a suggestion for docs :)

JoJoJet
JoJoJet reviewed Aug 26, 2023
View reviewed changes
crates/bevy_ecs/src/schedule/config.rs
Comment on lines +86 to +87
/// Adds a new boxed system set to the systems.
pub fn in_set_dyn(&mut self, set: BoxedSystemSet) {
Copy link
Member

Choose a reason for hiding this comment

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

Note that this change will be unnecessary if #7762 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I revert that change?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, that PR isn't guaranteed to get merged. Just thought I'd mention that this function might get removed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, okay

JoJoJet
JoJoJet approved these changes Aug 26, 2023
View reviewed changes
crates/bevy_ecs/src/schedule/config.rs Outdated Show resolved Hide resolved
@st0rmbtw @JoJoJet
Update crates/bevy_ecs/src/schedule/config.rs
0b62152 
Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com>
@JoJoJet JoJoJet 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 26, 2023
@alice-i-cecile
Copy link
Member

Can you update your PR description to match the changes?

@st0rmbtw
Copy link
Contributor Author

@alice-i-cecile Updated the description

@alice-i-cecile alice-i-cecile changed the title Make run_if_inner public Make run_if_inner public and rename to run_if_dyn Aug 27, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 27, 2023
Merged via the queue into bevyengine:main with commit 9d804a2 Aug 27, 2023
@st0rmbtw st0rmbtw mentioned this pull request Nov 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoJoJet JoJoJet JoJoJet approved these changes

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

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@st0rmbtw @ItsDoot @alice-i-cecile @JoJoJet