Skip to content

Conversation

vladbat00
Copy link
Member

@vladbat00 vladbat00 commented Oct 21, 2020

Motivation

This PR introduces a new derive macro inspired by specs' #[derive(SystemData)], that can be used as follows:

pub struct TestA(usize);
pub struct TestB(usize);

#[derive(SystemResources)]
pub struct TestSystemResources<'a> {
    test_a: FetchMut<'a, TestA>,
    test_b: Fetch<'a, TestB>,
}

#[system]
fn basic(#[system_resources] test_resources: &mut TestSystemResources<'static>) {
    test_resources.test_a.0 += test_resources.test_b.0 * 2;
}

I see this feature useful in the following scenarious:

  1. Working around ResourceSet and ConsAppend limits of generic arguments number.
  2. Attaching an implementation to a set of resources. Some functionality may have sense only when there are several resources present at once. Here's an example of how I use this pattern in Grumpy Visitors to combine the functionality of Amethyst's Time and my custom GameTime resources: https://github.com/amethyst/grumpy_visitors/blob/f12b85d0dac07c101b43e83711b89777dc69dd58/libs/core/src/ecs/system_data/time.rs.
  3. Avoiding boilerplate, when there are a lot of common resources shared between other systems.

Notable changes

Apart from implementing the feature itself, I had to do some other changes to the codebase.

  1. Getting rid of for<'a> lifetime bounds for ResourceSet in favour of just 'static lifetime.
-        <R as ConsFlatten>::Output: for<'a> ResourceSet<'a>,
+        <R as ConsFlatten>::Output: ResourceSet<'static>,

"Any" lifetime doesn't work well with the fact that SystemResources has its own lifetime. I didn't figure out the way to implement ResourceSet for SystemResources, so it supports the HRTB magic. Without this change I was getting the following error:

error: implementation of `ResourceSet` is not general enough
   --> tests\system_resources.rs:23:10
    |
23  |           .build(|_, _, (test_resources), _| {
    |            ^^^^^ implementation of `ResourceSet` is not general enough
    | 
   ::: D:\Programming\legion\src\internals\systems\resources.rs:99:1
    |
99  | / pub trait ResourceSet<'a> {
100 | |     /// The resource reference returned during a fetch.
101 | |     type Result: 'a;
102 | |
...   |
121 | |     }
122 | | }
    | |_- trait `ResourceSet` defined here
    |
    = note: `legion::internals::query::view::system_resources_view::SystemResourcesView<TestSystemResources<'_>>` must implement `ResourceSet<'0>`, for any lifetime `'0`...
    = note: ...but `legion::internals::query::view::system_resources_view::SystemResourcesView<TestSystemResources<'_>>` actually implements `ResourceSet<'1>`, for some specific lifetime `'1`

I hope this change won't cause any troubles. I wasn't able to come up with an example when resources can be not static in Legion.

  1. Re-exporting FetchMut from the systems module. I've just noticed there's another PR for that too: Re-export FetchMut from systems #209

  2. ConsConcat

I've added ConsConcat to the codebase, but it turned out that I didn't need it for my implementation eventually... :) But it works, I was even able to flatten SystemResources to pass their members along with other resources. I figured ConsPrepend isn't used anywhere as well, probably they can make friends, lol.

  1. Re-exporting the cons magic

This might be useful to users who want to make their own wrappers around SystemBuilder. Right now they represent a separate module, though probably we should re-export them as a part of the systems module.

Status

This pull request is in a draft state. There are some obvious TODOs:

  • Documentation and examples
  • Improving error handling to match the codegen level of reporting errors to a user

I'm totally going to work on those if it's decided that this feature is something that would fit Legion and there are no major oversights in the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant