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

Local with required config as a type parameter #2491

Closed
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
/// 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?


/// [`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?

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide 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
# 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
# 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
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants