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

NonUniqueResource #10793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Nov 29, 2023

WIP implementation of #10789.

Objective

Consider generic code connecting two systems:

// Pseudocode
fn connect<V>(a: impl System<Out = V>, b: impl System<In = V>) -> (impl System, impl System) {
  let resource = ???
  let a = a.pipe(|v| resource.write(v));
  let b = (|| resource.read()).pipe(b);
  (a, b)
}

Currently it is not trivial to implement, because as far as I understand there's no API to allocate such resource. Such resource can be allocated outside of bevy, and some mutex can be used to make access safe.

But it would be nicer if there was some API in Bevy to implement it.

What solution would you like?

Public API:

#[derive(Clone, Copy)]
struct NonUniqueResourceRef<T> { ... }

impl World {
  fn new_non_unique_resource<T>(&mut self) -> NonUniqueResourceRef<T> { ... }
}

impl NonUniqueResourceRef<T> {
  fn write_system(&self) -> impl System<In = T, Out = ()> { ... }
  fn read_system(&self) -> impl System<In = (), Out = T> { ... }
}

TODO

  • implement more operations
    • .read_shared_system() (return &T) (Edit: I think it is not possible)
    • .read_opt_system() (return Option<T>, None if resource was not written)
    • .read_opt_shared_system() (return Option<&T> if resource was not written)
    • .remove_system() (reset to None)
    • .read_copy_opt_system()
    • .read_copy_system()
  • implement tracking of changes
  • test that writing the same instance of resource is not allowed concurrently
  • test that writing two different instances of the same resource is allowed concurrently

Help

  • Is idea good?
  • Implementation approach?
  • How to write tests for concurrency?
  • Naming?

CC @ZBemo #8857

@stepancheg stepancheg force-pushed the non-uniq-res branch 8 times, most recently from 4990dcf to 568d7b0 Compare November 29, 2023 03:13
@alice-i-cecile
Copy link
Member

I don't understand how this could be used, or why it's a useful addition.

Questions:

  1. In what way is it non-unique?
  2. What motivated your code snippet?
  3. How else might this be used?
  4. How does the code added here help resolve the problem added in your snippet?
  5. In your code snippet, how would the resource defined by resource be accessed? It's accesses weren't declared in the function parameters at all.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Nov 29, 2023
@stepancheg
Copy link
Contributor Author

stepancheg commented Nov 29, 2023

In what way is it non-unique?

Non-unique by T. Multiple resources of T are allowed.

What motivated your code snippet?

Exploring various options for implementing issue described in #8857.

First code snippet in this PR description. It cannot use resource, because multiple pair of systems can use the same passed value type.

How else might this be used?

For example, for binding Bevy to dynamically typed language like Python. All "resources" will be the same PyObject or something like that.

How does the code added here help resolve the problem added in your snippet?

fn connect<V>(
  world: &mut World,
  a: impl System<(), V>,
  b: impl System<V, ()>
) -> (impl System<(), (), impl System<(), ()> {
  let res = world.new_non_unique_resource::<V>();
  let a = a.pipe(res.write_system());
  let b = res.read_system().pipe(b);
  (a, b)
}

In your code snippet, how would the resource defined by resource be accessed? It's accesses weren't declared in the function parameters at all.

Resource can mainly be accessed through systems exposed as functions on the NonUniqueResource.

(We can also add direct accessors from World to initialize from plugins for example, but this is not needed for initial version.)

@bevyengine bevyengine deleted a comment from stepancheg Nov 29, 2023
@stepancheg
Copy link
Contributor Author

stepancheg commented Nov 29, 2023

I completely forgot why I wanted to add it in the first place. Another use case.

This utility can also be used as replacement for conditions.

Currently conditions can be configured only when SystemSet is provided, which is fixed (can only be initialized from tuples, and generally inflexible, which does not allow fully implement automatic system linking as described in #8857 (comment)).

While implementing the approach from linkd comment I found current condition API does not allow this automatic system linking with conditions, because run_if requires <(), ()> system and returns SystemSet. I need a variant of run_if which gives me back the system.

So the idea is

Each new condition (or non-builtin condition) is a system which writes to a bool resource.

Each run_if implementation takes that bool condition resource as input, depends on the condition system, and executes the system only if condition is true.

Using proposed API, these custom conditions can be implemented like this:

let mut app = ...

// Set up condition
let condition_res = app.new_non_unique_resource::<bool>();
let condition_set = AnonymousSet::new();
app.add_systems(
  Update,
  (|| true).pipe(condition_res.write_system()).in_set(condition_set)
);

// Set up some systems depending on condition
for i in 0..10 {
  // My system I want to run conditionally
  let my_system = |local: Local<u32>, query: Query<&mut Something>| { }
  // Setup dynamic condition and dependency using builtin condition
  app.add_systems(
    Update,
    condition_res.read_system().and_then(my_system).after(condition_set)
  );
}

(CC #10757 for AnonymousSet::new)

Ideally I'd want to convert conditions to normal systems. Remove conditions builtin from bevy_ecs scheduler and make conditions normal systems writing to non-unique resources. But that's not important right now. Non-unique resources provides more flexibility how Bevy can be configured.

@cart
Copy link
Member

cart commented Nov 29, 2023

First: I think should have been presented with some clearer motivation, expressed up front (either in the issue or in the pr, ideally both). I was in the process of writing this off in my head when it finally clicked.

The motivation here: is "being able express value producer and consumer graphs in the schedule, where the 'value' is a unique piece of data that the scheduler can reason about".

This does seem like an interesting (and likely useful concept), but before adding a new "foundational building block", I think we need a bit more holistic discussion about:

  1. What are actual "real world" motivating use cases? Do we actually want this? When would users want to reach for this over the existing tools we provide?
  2. What is the right way to present this to the user? I'm personally not yet convinced that the API as exposed is the correct interface, especially given that it requires doing operations that are (1) outside of the add_systems / SystemConfig api and (2) possible to infer based on input/output types.

I think this is a good candidate for an RFC.

I also have some concerns about this implementation, such as storing the table row directly in the NonUniqueResourceRef, as that would mean we could never clean up NonUniqueResources. But that something to discuss after (1) and (2) are answered / we have buy-in from ECS SMEs.

@stepancheg
Copy link
Contributor Author

I think should have been presented with some clearer motivation, expressed up front

My bad.

I'm thinking about it this way. Resource is analogous to rwlock, basic building block in many libraries working with threads. NonUniqueResource lift the restriction of having single Resource instance per T.

It is lower level API not meant to be used directly, but can be used to implement higher level primitives:

I think this is a good candidate for an RFC.

I'll try to write one if that's what you suggest.

I'm personally not yet convinced that the API as exposed is the correct interface

This is meant to be low level API, higher level utilities can be built upon.

@alice-i-cecile
Copy link
Member

Thanks :) There's a good template; please submit it to https://github.com/bevyengine/rfcs

@stepancheg
Copy link
Contributor Author

bevyengine/rfcs#73

@stepancheg
Copy link
Contributor Author

One more potential application of non-unique resources: fix concurrency of EventWriter (or implement similar higher level primitives).

All EventWriter<T> must hold ResMut of something, currently it is Events<T>, the same for every resource instance.

Instead, the design could be this:

  • each EventWriter<T> gets own shard of events
    • writes only to own shard
    • requires exclusive access only to own shard
  • EventReader<T> stores all the shards
    • EventReader<T> requests shared access to all the shards
    • iterates over all the shards at read

For simplicity let's assume that Events<T> stores Vec<T>.

// current "Events"
struct EventsShard<T> {
  events: Vec<T>,
}

// New events: just stores the list of shards.
pub struct Events<T> {
  shards: Vec<NonUniqueResourceRef<EventsShard<T>>>,
}

pub struct EventWriter<T> {
  shard: NonUniqueResourceRef<EventsShard<T>>,
}

impl SystemParam for EventWriter<T> {
  type State = NonUniqueResourceRef<EventsShard<T>>;
  fn init_state(world) -> Self::State {
    let shard = world.new_non_unique_resource::<EventsShard<T>>();
    let events = world.init_resource::<Events<T>>();
    events.shards.push(shard);
    shard
  }

  fn new_archetype(state) {
    // I want exclusive access to my shard
  }  
}

pub struct EventWriter<T> {
  shard: Vec<NonUniqueResourceRef<EventsShard<T>>>,
}

...

It is a bit more complicated than this (all the readers must be initialized after all the writers).

But the issue is that currently Bevy lacks basic building blocks to solve this issue (and other similar issues). This is not something can be worked around in third-party library or some undocumented API.

The problem is: shared/exclusive access to anything can be only controlled by types in current Bevy. NonUniqueResource makes it more flexible.

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-Enhancement A new feature X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants