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

[Merged by Bors] - Obviate the need for RunSystem, and remove it #3817

Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - Obviate the need for RunSystem, and remove it #3817

wants to merge 6 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Jan 30, 2022

Objective

Solution

P.S. I also want this for an experimental refactoring of Assets, to remove the duplication of Events<AssetEvent<T>>

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 30, 2022
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Jan 30, 2022
@DJMcNab DJMcNab added S-Needs-Triage This issue needs to be labelled C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed C-Code-Quality A section of code that is hard to understand or change S-Needs-Triage This issue needs to be labelled labels Jan 30, 2022
alice-i-cecile
alice-i-cecile reviewed Jan 30, 2022
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.

Very nice. It's good to simplify this, and the code itself makes sense. Minor docs nit, then this looks good to me.

DJMcNab
DJMcNab commented Feb 1, 2022
View reviewed changes
crates/bevy_ecs/src/system/system_param.rs
Comment on lines +1285 to +1289
/// For example, using this would allow a type to be generic over
/// whether a resource is accessed mutably or not, with
/// impls being bounded on [`P: Deref<Target=MyType>`](Deref), and
/// [`P: DerefMut<Target=MyType>`](DerefMut) depending on whether the
/// method requires mutable access or not.
Copy link
Member Author

Choose a reason for hiding this comment

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

I plan on trying to use this trick for Assets (to remove the fact that the events are seperate in an ergonomic way)

That will be a seperate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done some more thinking about this, and I realise that none of the reading only Assets methods access the events.
I don't actually think that PR would need this struct/this trick. Still, this is much nicer than RunSystem

@alice-i-cecile alice-i-cecile added the A-Assets Load files from disk to use for things like images, models, and sounds label Feb 1, 2022
@alice-i-cecile
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Feb 4, 2022

try

Merge conflict.

@DJMcNab
Copy link
Member Author

DJMcNab commented Feb 4, 2022

bors retry

bors bot added a commit that referenced this pull request Feb 4, 2022
@bors
Try #3817:
a52962f
@bors
Copy link
Contributor

bors bot commented Feb 4, 2022

try

Build succeeded:

DJMcNab
DJMcNab commented Feb 10, 2022
View reviewed changes
crates/bevy_ecs/src/system/system_param.rs
/// This type is a [`SystemParam`] adapter which always has
/// `Self::Fetch::Item == Self` (ignoring lifetimes for brevity),
/// no matter the argument [`SystemParam`] (`P`) (other than
/// that `P` must be `'static`)
Copy link
Member Author

Choose a reason for hiding this comment

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

This restriction is unfortunate, but pretty fundamental as far as I can tell.
This is because we need be able to refer to P in SystemParamState, which reasonably needs to be 'static

TheRawMeatball
TheRawMeatball reviewed Feb 11, 2022
View reviewed changes
Copy link
Member

@TheRawMeatball TheRawMeatball left a comment

Choose a reason for hiding this comment

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

Looks good other than this one nit :P

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Feb 14, 2022
@mockersf mockersf mentioned this pull request Mar 5, 2022
Closed
@DJMcNab DJMcNab mentioned this pull request Mar 7, 2022
Closed
cart
cart approved these changes Mar 15, 2022
View reviewed changes
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I like these changes. It keeps the hackery scoped and allows us to use the "same" system paradigm everywhere. It would be nice if we didn't need the "wrapper" StaticSystemParamState or StaticSystemParam types, but I understand why they're there.

@cart
Copy link
Member

cart commented Mar 15, 2022

(I just removed the old Config api usage, which would have failed during bors validation)

@cart
Copy link
Member

cart commented Mar 15, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 15, 2022
@DJMcNab @cart
Obviate the need for RunSystem, and remove it (#3817)
6e61fef 
# Objective

- Fixes #3300
- `RunSystem` is messy

## Solution

- Adds the trick theorised in #3300 (comment)

P.S. I also want this for an experimental refactoring of `Assets`, to remove the duplication of `Events<AssetEvent<T>>`


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 15, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Obviate the need for RunSystem, and remove it [Merged by Bors] - Obviate the need for RunSystem, and remove it Mar 15, 2022
@bors bors bot closed this Mar 15, 2022
@DJMcNab DJMcNab deleted the static_param branch March 15, 2022 07:15
@cart cart mentioned this pull request Mar 19, 2022
Closed
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@DJMcNab @cart @aevyrie
Obviate the need for RunSystem, and remove it (bevyengine#3817)
3cc6c56 
# Objective

- Fixes bevyengine#3300
- `RunSystem` is messy

## Solution

- Adds the trick theorised in bevyengine#3300 (comment)

P.S. I also want this for an experimental refactoring of `Assets`, to remove the duplication of `Events<AssetEvent<T>>`


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@DJMcNab @cart @ItsDoot
Obviate the need for RunSystem, and remove it (bevyengine#3817)
d6ff47d 
# Objective

- Fixes bevyengine#3300
- `RunSystem` is messy

## Solution

- Adds the trick theorised in bevyengine#3300 (comment)

P.S. I also want this for an experimental refactoring of `Assets`, to remove the duplication of `Events<AssetEvent<T>>`


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheRawMeatball TheRawMeatball TheRawMeatball approved these changes

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

@cart cart cart approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change M-Needs-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
@cart's attention
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible to define a system as generic over SystemParam and provide concrete type?
4 participants
@DJMcNab @alice-i-cecile @cart @TheRawMeatball