Skip to content

RFC: SystemParam::user_meta #10678

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

Closed
wants to merge 1 commit into from
Closed

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Nov 21, 2023

Objective

The objective of this RFC is the same as in #10618: how to declare one system depends on the other while keeping such configuration robust to the changes.

Solution

Previous RFC suggested making it native to Bevy: each system param could declare before/after system sets.

Some considered that RFC too controversial and not fitting well into Bevy ESC model.

API: user metadata

This RFC proposes alternative solution to the problem. Each SystemParam can supply arbitrary metadata. Metadata is a Rust value of arbitrary type.

trait SystemParam {
  // Implemented by a user.
  fn user_meta(request: &mut SystemParamUserMetaRequest);
}

// This type cannot be instantiated by a user (no need).
impl SystemParamUserMetaRequest {
  // Collect the value of type `T` if requested, ignore otherwise
  fn provide_value<T>(&mut self, value: T) { ... }
}

impl dyn System {
  // User facing API to fetch metadata.
  fn param_user_meta_dyn<T>(&self) -> Vec<T> {
    // Initialize SystemParamUserMetaRequest
    // collect meta from all the params
  }
}

API is similar to Error::provide API from rust stdlib.

How it can be used

A user outside of Bevy can declare two special parameter types: Before<T: SystemSet> and After<T: SystemSet>. These parameter types would emit metadata like BeforeItem(InternedSystemSet) and AfterItem(InternedSystemSet).

Bevy would not automatically do anything with this knowledge (it is just opaque metadata for the scheduler).

But a user can implement utilities over App to account for this metadata to adjust the scheduler.

In pseudocode:

fn my_add_system(app: &mut App, schedule: ScheduleLabel, system: IntoSystem) {
  let system = system.into_system();
  let before = system.param_user_meta_dyn::<BeforeItem>();
  let after = system.param_user_meta_dyn::<AfterItem>();
  let mut system = system.into_system_configs();
  for before in before {
    system = system.before(before);
  }
  for after in after {
    system = system.after(after);
  }
  app.add_systems(schedule, system)
}

Meta vs builtin

Frankly I don't see any applications of param meta outside of these scheduling labels, and this meta will not be very ergonomic (although it would solve the problems I described in the previous RFC). So I would prefer scheduling instructions to be built in Bevy.

But I'd be fine with any solution which would solve the problem, even if user code would be verbose.

And, last words, I'm not going to die on this hill. If this idea is not good enough either, I think I can live with existing API which requires looking very hard to verify schedule correctness.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Nov 21, 2023
@alice-i-cecile
Copy link
Member

I like that this is less intrusive.

The notion of being able to add arbitrary user-defined meta-data is an interesting hook for extension. And plumbing it up from system params makes sense.

I'm unsure if this would be useful for anything else, but perhaps others will have ideas.

In the absence of that, I don't think this is a good use of developer effort or internal complexity.

@stepancheg
Copy link
Contributor Author

Now I think dependencies should be based on in-out parameters like described here: #8857 (comment). And nobody is interested in this. So closing this one.

@stepancheg stepancheg closed this Nov 27, 2023
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 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.

2 participants