Skip to content

Allow components to be "explicitly required". #16209

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

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Add an "explicit" tag to required components to allow them to check (at runtime) that a component is present (or inserted at the same time).

Testing

  • Unit tests are sufficient.

Showcase

#[derive(Component)]
#[require(EnemyTarget(explicit))]
struct Enemy {
  health: f32,
}

#[derive(Component)]
struct EnemyTarget(Entity);

fn main() {
  App::new()
    .add_plugins(DefaultPlugins)
    .run()
}

fn setup(mut commands: Commands) {
  let target = commands.spawn(Transform::default()).id();
  // Panics if we don't add the EnemyTarget.
  commands.spawn((Enemy{ health: 10.0 }, EnemyTarget(target)));
}

Migration Guide

  • Required component constructors named explicit must be renamed to anything else.

Risks

Required components so far are safe - they should not cause a panic. Explicitly required components can cause panics now whenever you spawn or insert a component that names an explicitly required component. For example, in the example above, if you spawn the Enemy + EnemyTarget components, then remove the EnemyTarget component, then insert the Enemy component again, you get a panic! This can make the panics more non-deterministic.

Now, users can specify that components must be inserted (or at least present) when inserting or spawning another component.
@alice-i-cecile alice-i-cecile requested a review from cart November 2, 2024 00:34
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Nov 2, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 2, 2024
Comment on lines +95 to +99
let constructor = if func.is_ident("explicit") {
quote! { None }
} else {
quote! {Some(|| { let x: #ident = #func().into(); x })}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be surprising behavior to users who name their constructor explicit (for whatever reason). Could we make sure this is at least documented?

Alternatively we could do #[requires(explicit = true)] and peeking for = could be used to disambiguate the two. Not sure that's totally worth the ergonomics hit though.

@mockersf
Copy link
Member

mockersf commented Nov 3, 2024

Would it be possible to find different words for require as in "automatically added if not present when adding this component" and require as in "must be present when adding this component"

Maybe "dependent" for the current require?

@andriyDev
Copy link
Contributor Author

Closed as per the discussion in #16194.

@andriyDev andriyDev closed this Nov 5, 2024
@andriyDev andriyDev deleted the explicit-required branch November 5, 2024 18:59
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 S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require components that can't be defaulted
4 participants