Skip to content

Local with required config as a type parameter #2491

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
Closed
mockersf wants to merge 4 commits into from
Closed

Local with required config as a type parameter #2491

mockersf wants to merge 4 commits into from

Conversation

@mockersf
Copy link
Member

@mockersf mockersf commented Jul 16, 2021

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

alice-i-cecile commented Jul 16, 2021

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
@mockersf mockersf force-pushed the configstruct-local branch from 2f954dc to 43846c0 Compare July 27, 2021 21:29
@alice-i-cecile alice-i-cecile mentioned this pull request Jul 28, 2021
Closed
@mockersf mockersf force-pushed the configstruct-local branch from 43846c0 to fc69097 Compare August 18, 2021 22:36
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

@NiklasEi NiklasEi Oct 29, 2021

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

@NiklasEi NiklasEi Oct 29, 2021

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 M-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
@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

1 more reviewer

@NiklasEi NiklasEi NiklasEi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide

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