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 const generic parameter #2463

Closed
wants to merge 3 commits into from

Conversation

mockersf
Copy link
Member

Objective

Solution

Either:

  • parameter should use the default value or a FromWorld impl, use Local<T, true>
  • parameter must be configured using config, use Local<T, false>

If the parameter is false, the bound for T on FromWorld is lifted.

#[derive(Default)]
struct DefaultableStruct(u32);

struct NotDefaultableStruct(u32);

fn my_system(a: Local<DefaultableStruct, true>, b: Local<NotDefaultableStruct, false>) {}

Ideally, I would want the Local type to be:

enum LocalType {
    FromWorld,
    NeedConfig
}

pub struct Local<'a, T: Component, const LOCAL_TYPE: LocalType = LocalType::FromWorld>(&'a mut T);

Then my system would be:

fn my_system(a: Local<DefaultableStruct>, b: Local<NotDefaultableStruct, LocalType::NeedConfig>) {}

But this is not possible in Rust for now 😞 :

Those are the two top issues in "what's next", so hopefully they should come soonish...

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 13, 2021
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 13, 2021

I prefer this solution in general, but would like to hold off for a prettier version. I suppose the churn isn't too bad; this is not a particularly prominent feature and the refactor would be easy.

Overall, making sure that the configured and defaulted versions of Local are the same type makes communicating about the API much simpler and reduces code duplication.

@alice-i-cecile alice-i-cecile 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 13, 2021
@mockersf
Copy link
Member Author

mockersf commented Jul 13, 2021

once we can use an enum, we could have more control on how it's initialized:

enum LocalType {
    FromWorld,
    Default, // now separate from FromWorld
    NeedConfig
}

@cart
Copy link
Member

cart commented Jul 13, 2021

Ooh this is a clever take / a pattern that wasn't on my radar. I'd also prefer to hold off until const generic defaults are an option (and ideally, enums).

@mockersf
Copy link
Member Author

it seems feature const_generics_defaults is on a good way to stabilisation 🎉

@TheRawMeatball
Copy link
Member

We could already use something like this design, if we substitute a module with 2 unit structs instead of an enum:

mod local_mode {
    struct FromWorld;
    struct NeedsConfig;
}

@NathanSWard
Copy link
Contributor

it seems feature const_generics_defaults is on a good way to stabilisation 🎉

Just upgrade the compiler we use to nightly and all our problems are solved 😉

@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 added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Sep 22, 2021
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
@mockersf mockersf reopened this Feb 28, 2022
@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 S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants