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

SystemResources: port of specs' SystemData for resources #211

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

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