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

Insert custom local system resources #745

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

Jerald
Copy link
Contributor

@Jerald Jerald commented Oct 29, 2020

This modifies some of the traits on Local<T> causing it to only initialize T using FromResources if the resource T doesn't yet exist on the system. This allows having a Local<T> on a system which you then initialize later using whatever value of T you'd like.

In addition, this adds some convenience functions to AppBuilder that allow easily adding a custom T value to a given system.

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Oct 29, 2020
@cart
Copy link
Member

cart commented Oct 29, 2020

I think the "only set Local if it hasn't been initialized" part of this is good to go, but I think the convenience functions are a bit too "hard coded" as a system could have more than one local resource.

I think I'd rather have an api like this:

app.add_system(something.system())
  .add_system_local(something.name(), SomeResource)

Retrieving a system id/name without a system instance is something thats still being discussed. We'll want whatever api we use for "adding system locals" to line up with other apis like "adding explicit manual system dependencies".

Can we scope this pr to just the "only set Local if it hasn't been initialized" for now? Then we can spec out the "address system by name and/or id without re-initializing the system" design elsewhere.

@Jerald
Copy link
Contributor Author

Jerald commented Oct 29, 2020

I specifically made sure to do those two in separate commits so the convenience functions could be shelved if they aren't as agreed upon. That said, I'm not entirely sure what you mean by the "Retrieving a system id/name without a system instance" part. As far as I understand what you mean, this doesn't require that?

Personally, I know that if these conveniences aren't implemented, I'll be making my own convenience trait that effectively does this:

app.init_system(|resources| {
    let system = system.system();
    resources.insert_local(system.id(), local_resource_to_init_with);
    system
})

Did you mean that it's presently not possible (or in debate for how it should be possible) to call Resources::insert_local with a system that's not been "initialized" in some sense yet? In that case, the above convenience function wouldn't work either.

@cart
Copy link
Member

cart commented Oct 30, 2020

I mean that right now if you dont have a direct reference to Box<dyn System> then you cannot get the name or the id. You're right to say that a builder like the above would work. But rather than adding "yet another" set of AppBuilder system registration variants, I think id rather build a separate api that can stand alone / compose with all of the other existing app builder system functions.

If in the short term you can build your own helpers, I think thats preferable while we work out what the final api looks like.

@Jerald
Copy link
Contributor Author

Jerald commented Oct 30, 2020

I understand, makes sense. I'm going to get to removing that commit soon and then this can hopefully get merged

Local<T> will no longer insert the inner resource if it already exists.
@Jerald Jerald force-pushed the local-resource-custom-insert branch from 73cea50 to bc5cd93 Compare October 30, 2020 19:06
@Jerald
Copy link
Contributor Author

Jerald commented Oct 30, 2020

Rebased and removed the appbuilder changes. Should be all ready to merge now!

@cart cart merged commit b6004e4 into bevyengine:master Oct 30, 2020
Jerald added a commit to Jerald/bevy that referenced this pull request Nov 3, 2020
cart added a commit that referenced this pull request Nov 3, 2020
* Added #745 to changelog

* Update CHANGELOG.md

Co-authored-by: karroffel <[email protected]>

Co-authored-by: Carter Anderson <[email protected]>
Co-authored-by: karroffel <[email protected]>
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.

3 participants