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

[Merged by Bors] - Remove the config api #3633

Closed
wants to merge 7 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Jan 10, 2022

Objective

Solution

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 10, 2022
@DJMcNab DJMcNab added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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 10, 2022
@mockersf
Copy link
Member

mockersf commented Jan 11, 2022

After using config a lot during my latest experiment with Bevy, I grew to dislike it quite a lot...

As it's a useful feature, and config was somewhat discoverable through docs, could you add an example on how to do it without?

/// move |mut val| val.0 = value.0
/// }
///
/// // .add_system(reset_to(my_config))
Copy link
Member

Choose a reason for hiding this comment

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

could this example be more complete? you missed a rename here as it's commented

Copy link
Member Author

Choose a reason for hiding this comment

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

We're stuck in bevy_ecs here, so I can't create an App, without making a stub implementation. That seemed like quite a lot of effort for the example.

There's not really a great way to demonstrate adding systems within bevy_ecs imo, since every consumer should also be using bevy_app.

The rename does need to happen though, thanks.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

After trying to use this way to configure my system, I'm not completely a fan:

fn configured_animate_scale(
    speed: f32,
) -> impl FnMut(Res<Time>, Query<&mut Transform, (With<Text>, With<AnimateScale>)>) {
    move |time, mut query| {
        for mut transform in query.iter_mut() {
            transform.translation = Vec3::new(400.0, 0.0, 0.0);
            transform.scale =
                Vec3::splat((time.seconds_since_startup().sin() as f32 + 1.1) * speed);
        }
    }
}

// Adding this system
app.add_system(configured_animate_scale(5.0))

But still better than the current config api so... 🤷

@cart
Copy link
Member

cart commented Feb 25, 2022

This is also an option. Slightly more typing, but allows you to home the variable type next to the variable name.

fn prepare_system(mut state: LocalFixedTimestepState) -> impl System<In = (), Out = ShouldRun> {
    IntoSystem::into_system(
        move |time: Res<Time>, mut fixed_timesteps: ResMut<FixedTimesteps>| {
            let should_run = state.update(&time);
            if let Some(ref label) = state.label {
                let res_state = fixed_timesteps.fixed_timesteps.get_mut(label).unwrap();
                res_state.step = state.step;
                res_state.accumulator = state.accumulator;
            }

            should_run
        },
    )
}

Or this if you're willing to use deprecated methods 😄

fn prepare_system(mut state: LocalFixedTimestepState) -> impl System<In = (), Out = ShouldRun> {
    (move |time: Res<Time>, mut fixed_timesteps: ResMut<FixedTimesteps>| {
        let should_run = state.update(&time);
        if let Some(ref label) = state.label {
            let res_state = fixed_timesteps.fixed_timesteps.get_mut(label).unwrap();
            res_state.step = state.step;
            res_state.accumulator = state.accumulator;
        }

        should_run
    })
    .system()
}

@cart
Copy link
Member

cart commented Feb 25, 2022

bors r+

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
@bors bors bot changed the title Remove the config api [Merged by Bors] - Remove the config api Feb 25, 2022
@bors bors bot closed this Feb 25, 2022
@DJMcNab DJMcNab deleted the cleanup-config branch February 25, 2022 07:14
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-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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants