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

Implemented Resource for () #15510

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pablo-lua
Copy link
Contributor

@pablo-lua pablo-lua commented Sep 28, 2024

Objective

Solution

  • Implement Resource for (), like the first version of Draft: Implement Resource for () #14877
    Note that this PR doesn't implement ReflectResource for Unit type: There isn't much a reason to do so.
    This can only affect you if you need to use () more than a resource, using it as a Reflected Resource too.

Testing

  • Tested locally, with a system querying a Option<Res<()>>, as expected, returned None.

Showcase

// Let's say I have a system or plugin that I want the user to specify the Type of the Resource
fn my_system<T: MyTrait + Resource = ()>(resource: Option<Res<T>>) {
      // This will do nothing if the user defines so.
      // if None, not found or the dummy () type
      if let Some(res) = resource {
           res.my_trait();
      }
}

Notes

Comments in #14877 suggests adding a MaybeResource type query.
Even so, its simpler and more ergonomic IMO to just implement () as a Resource, so you can use default types without having to create ZST to do this work for you. But have the drawback of you having to use Option to intermediate between a type that exists in the world, or the dummy () type.

World::resource::<()> will not work: () isn't really inserted in the World, instead you should use Option or get_resource() to help you here.

@pablo-lua pablo-lua added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 28, 2024
@pablo-lua pablo-lua marked this pull request as draft September 28, 2024 22:55
@pablo-lua
Copy link
Contributor Author

Nevermind: Never noticed that this approach was banned basically because of SceneSpawner notices that there is a type not registered as ReflectResource, which is a problem with this approach. Making implementing Reflect for unit is possible, but isn't good. In all worlds
Two solutions possible are the MaybeResource type query, or implement Resource for unit type, but not inserting in all Worlds.
Its possible to relax the requirements by using another approach.

@pablo-lua pablo-lua marked this pull request as ready for review September 28, 2024 23:50
@alice-i-cecile alice-i-cecile added S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 29, 2024
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

Implement Resource for ()
2 participants