Skip to content

Add Required system param #2440

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

Closed
wants to merge 1 commit into from

Conversation

TheRawMeatball
Copy link
Member

Adds an alternative to Local without a FromWorld bound.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 6, 2021
@TheRawMeatball TheRawMeatball 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 6, 2021
@alice-i-cecile
Copy link
Member

Looks interesting. What do you see this being used for?

@Ratysz
Copy link
Contributor

Ratysz commented Jul 6, 2021

Any situation where the local resource cannot be initialized automatically, and anytime say a plugin author wants to draw attention to a local resource being configurable.

@NathanSWard
Copy link
Contributor

NathanSWard commented Jul 6, 2021

Looks pretty good, however, I'm not so sure about the name Required.
Perhaps:
Configured
(can't really think of any others haha)

Or, would it be possible to overload the usage of Local either via a second generic type or trait which can statically change behavior?

Maybe like
Configured<Local<T>> or something like that? 🤷

@alice-i-cecile
Copy link
Member

My preference is definitely to override or extend Local in some way; the usage of the two APIs is identical except at initialization.

I suppose there's no way to default to this behaviour if FromWorld is missing?

@cart
Copy link
Member

cart commented Jul 6, 2021

I suppose there's no way to default to this behaviour if FromWorld is missing?

Specialization could probably make this work, but without it i'm pretty this is impossible if we want FromWorld to be "automatic" behavior.

We could remove the FromWorld constraint and make Local always require manual configuration, but that would be a pretty huge step down in usability.

@mockersf
Copy link
Member

mockersf commented Jul 6, 2021

Not a fan of having both Local and Required, or the fact that Required would fail only at runtime...

But I would love to have something that fails to compile without a .config() call...

@cart
Copy link
Member

cart commented Jul 6, 2021

If we're going to have another type for this, I think Configured is the best name so far. It guides the user toward the "config()" api and semi-implies that if you don't "configure" it, something bad might happen.

@Nilirad
Copy link
Contributor

Nilirad commented Jul 7, 2021

@Ratysz
Copy link
Contributor

Ratysz commented Jul 7, 2021

+1 to Configured.

But I would love to have something that fails to compile without a .config() call...

I low-key assumed that was already the case when skimming it. Hm.

Could this help seeding multiple system local PRNGs from a single seed generator [...]

Not any more than Local itself could.

@mockersf
Copy link
Member

mockersf commented Jul 8, 2021

Would it be possible to have a const generic to specify if the Local need configuration?

struct Local<const DEFAULT: bool>;

fn init<const DEFAULT: bool>(test: Local<DEFAULT>) {
    if DEFAULT {
        println!("using default");
    } else {
        println!("need configuration");
    }
}

But I would love to have something that fails to compile without a .config() call...

I low-key assumed that was already the case when skimming it. Hm.

Didn't check, but it's an expect so that should fail at runtime

@inodentry
Copy link
Contributor

OK, so I just had a real-world use case for what this PR proposes. See: https://discord.com/channels/691052431525675048/742884593551802431/864264371106742312

@mockersf
Copy link
Member

I played with the const generic option in #2463, it works but I think it's not pretty in current Rust

@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
@TheRawMeatball
Copy link
Member Author

Closing because better patterns exist.

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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants