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

change not implemation to custom system struct #8105

Merged
merged 12 commits into from
Mar 17, 2023

Conversation

B-Reif
Copy link
Contributor

@B-Reif B-Reif commented Mar 16, 2023

Objective

The function common_conditions::not can't be used with the system configuration distributive_run_if, because the returned closure is typed as impl Condition<()>. Fixes another part of #8058.

Solution

Changes to not:

  • Add a custom NotSystem struct which wraps a Condition and inverts its output whenever it's run. Implements Clone.
  • Return the concrete NotSystem<_, T> type from not<_, T>, so that the return type is Clone when the input system is Clone.

Changes to FunctionSystem:

  • Add a Clone implementation for FunctionSystem<Marker, F> where F is Clone. Note: this implementation does not clone the param_state field, so the cloned system instance doesn't have the original's state. Let me know if this is a problem.
  • #[derive(Clone)] for the IsFunctionSystem marker. The compiler didn't like it when I tried to remove this. Is it really necessary to implement Clone for the marker types?

Changelog

  • Added NotSystem system type
  • Changed common_conditions::not to return a concrete type
  • Changed FunctionSystem to implement Clone for Clone functions

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 16, 2023

I accidentally removed some of the common_conditions tests while resolving conflicts in #8060, so this PR also re-adds them.

@JoJoJet JoJoJet self-requested a review March 16, 2023 04:01
@JoJoJet
Copy link
Member

JoJoJet commented Mar 16, 2023

  • #[derive(Clone)] for the IsFunctionSystem marker. The compiler didn't like it when I tried to remove this. Is it really necessary to implement Clone for the marker types?

The reason for this is right here:

#[derive(Clone)]
pub struct ConditionInverter<Marker, T>

When using #[derive(Clone)], the compiler (incorrectly) adds a Marker: Clone bound. To fix this you'd have to do a manual implementation of Clone.

I will do a full review later.

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

It's a bit unfortunate to have to add another manual System impl, but I think it's absolutely worth it to make run conditions just work in more situations.

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved
@JoJoJet
Copy link
Member

JoJoJet commented Mar 16, 2023

  • Add a Clone implementation for FunctionSystem<Marker, F> where F is Clone. Note: this implementation does not clone the param_state field, so the cloned system instance doesn't have the original's state. Let me know if this is a problem.

Essentially, this just means that cloned function systems will have their initialization undone. I think that's acceptable -- however the current implementation does not fully de-initialize the system. You'd need to make it reset the world_id, archetype_generation, and probably the system_meta.

Also, it might be worth adding a SystemClone trait instead of using std's Clone. It would make it easier to document the de-initialization behavior, and make it more difficult to misuse.

Alternatively, we could retain the initialization when cloning, but require the system's state to be Clone.

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Thank you for taking my suggestions. Generally I'm on board with this PR, but I'd like more opinions on how to handle the initialization issue.

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

I'd definitely prefer if this reset the system initialization completely. I don't care about performance here, and I think that not doing so is a correctness footgun.

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 17, 2023

Thank you for taking my suggestions.

Thank you very much for your guidance!

I'd definitely prefer if this reset the system initialization completely.

I've changed the Clone implementation to only clone the func; other fields now match the IntoSystem implementation.

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 think this safety comment is clearer, but I won't block on it. Other changes look good.

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved
Comment on lines +383 to +386
// De-initializes the cloned system.
impl<Marker, F> Clone for FunctionSystem<Marker, F>
where
F: SystemParamFunction<Marker> + Clone,
Copy link
Member

@JoJoJet JoJoJet Mar 17, 2023

Choose a reason for hiding this comment

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

This behavior, while probably correct, is very subtle. It needs to be documented somewhere visible -- perhaps on the System trait? Also, maybe it should be documented on distributive_run_if, since I think that's the only place we're cloning systems right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now Clone is implemented specifically for FunctionSystem, and not necessarily for other System implementers. I think it makes more sense to call it out on FunctionSystem. I also added a note on distributive_run_if.

Copy link
Member

@JoJoJet JoJoJet Mar 17, 2023

Choose a reason for hiding this comment

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

Right now Clone is implemented specifically for FunctionSystem, and not necessarily for other System implementers.

Right, but users will rarely if ever interact with the FunctionSystem type directly. I think it's more likely to be seen if the System trait mentions that systems defined using a fn have this property.

Copy link
Member

Choose a reason for hiding this comment

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

Why is this resolved?

Copy link
Member

Choose a reason for hiding this comment

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

This was addressed below in 3a98270.

Copy link
Member

Choose a reason for hiding this comment

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

That documented it on FunctionSystem, but users have little reason to ever interact with that type or read its documentation. Function systems get automagically created through type magic, so many users are probably only vaguely aware of that type's existence because of compiler errors.

Comment on lines +383 to +386
// De-initializes the cloned system.
impl<Marker, F> Clone for FunctionSystem<Marker, F>
where
F: SystemParamFunction<Marker> + Clone,
Copy link
Member

@JoJoJet JoJoJet Mar 17, 2023

Choose a reason for hiding this comment

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

Right now Clone is implemented specifically for FunctionSystem, and not necessarily for other System implementers.

Right, but users will rarely if ever interact with the FunctionSystem type directly. I think it's more likely to be seen if the System trait mentions that systems defined using a fn have this property.

@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 A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 17, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10.1 milestone Mar 17, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2023
@alice-i-cecile alice-i-cecile merged commit bca4b36 into bevyengine:main Mar 17, 2023
@B-Reif B-Reif deleted the condition-inverter branch March 17, 2023 22:40
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
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.

3 participants