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

Local with required config as a type parameter #2491

Closed
wants to merge 4 commits into from
Closed

Local with required config as a type parameter #2491

wants to merge 4 commits into from

Conversation

mockersf
Copy link
Member

Objective

Solution

  • Added three unit struct to configure a Local: Default, FromWorld and NeedConfig
  • By default, it is using the Default configuration. This is a breaking change but it's what is making the most sense
  • If the Local is initialised by Default, nothing change
  • For the other, need to add an extra type parameter to local, either local_value::FromWorld or local_value::NeedConfig

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 16, 2021
@mockersf mockersf added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jul 16, 2021
@alice-i-cecile
Copy link
Member

By default, it is using the Default configuration. This is a breaking change but it's what is making the most sense

Definitely agree with this choice. I would expect the large majority of current uses to be using Default.

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile alice-i-cecile mentioned this pull request Jul 26, 2021
Closed
@alice-i-cecile alice-i-cecile mentioned this pull request Jul 28, 2021
Closed
NiklasEi
NiklasEi reviewed Oct 29, 2021
View reviewed changes
crates/bevy_ecs/src/system/system_param.rs
/// system.initialize(world);
/// system.run((), world);
/// ```
pub struct Local<'a, T: Resource, ValueFrom: local_value::LocalValue = local_value::Default> {
Copy link
Member

Choose a reason for hiding this comment

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

How about ValueFrom -> InitializeWith?

crates/bevy_ecs/src/system/system_param.rs

/// [`Local`](crate::system::Local) should be initialized by calling
/// [`FunctionSystem::config()`](crate::system::FunctionSystem::config) on the system.
pub struct NeedConfig;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe SystemConfig would fit better?

@DJMcNab DJMcNab mentioned this pull request Jan 10, 2022
Closed
@alice-i-cecile alice-i-cecile added C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Migration-Guide labels Jan 20, 2022
bors bot pushed a commit that referenced this pull request Feb 25, 2022
@DJMcNab
Remove the config api (#3633)
c1a4a2f 
# Objective

- Fix the ugliness of the `config` api. 
- Supercedes #2440, #2463, #2491

## Solution

- Since #2398, capturing closure systems have worked.
- Use those instead where we needed config before
- Remove the rest of the config api. 
- Related: #2777
@DJMcNab
Copy link
Member

DJMcNab commented Feb 28, 2022

As of #3633, this no longer makes sense.

@mockersf mockersf closed this Feb 28, 2022
kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
@DJMcNab
Remove the config api (bevyengine#3633)
55ac6c3 
# Objective

- Fix the ugliness of the `config` api. 
- Supercedes bevyengine#2440, bevyengine#2463, bevyengine#2491

## Solution

- Since bevyengine#2398, capturing closure systems have worked.
- Use those instead where we needed config before
- Remove the rest of the config api. 
- Related: bevyengine#2777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiklasEi NiklasEi NiklasEi left review comments

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Feature A new feature, making something new possible
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mockersf @alice-i-cecile @DJMcNab @NiklasEi @cart