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

Allow plugins to define component requirements #15367

Closed
Jondolf opened this issue Sep 22, 2024 · 7 comments · Fixed by #15458
Closed

Allow plugins to define component requirements #15367

Jondolf opened this issue Sep 22, 2024 · 7 comments · Fixed by #15458
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers X-Contentious There are nontrivial implications that should be thought through

Comments

@Jondolf
Copy link
Contributor

Jondolf commented Sep 22, 2024

This is similar to #14927, but more scoped, and with a slightly more flexible API.

What problem does this solve or what need does it fill?

Currently, required components work only through the require attribute on component types. For example:

#[derive(Component)]
#[require(Mass, SleepTimer, ...)]
pub struct RigidBody;

#[derive(Component, Default)]
pub struct Mass(pub f32);

#[derive(Component, Default)]
pub struct SleepTimer(pub Timer);

The above works, nice! However, there's a problem. We only want mass properties for rigid bodies with the Dynamic marker component, as static and kinematic bodies shouldn't have any mass components. Additionally, SleepTimer is only relevant when the SleepPlugin is enabled, and only for dynamic rigid bodies. If sleeping is disabled, there's no reason to insert sleeping components for everything!

We could use component lifecycle hooks to add the components conditionally, but this is also problematic. It causes additional archetype moves for each insert command, and you can also only have one instance of each lifecycle hook per component, which could cause conflicts if the component already has a hook registered, either by itself or by another plugin. The next option is to use observers, but that feels less semantically correct, runs after hooks, and incurs a larger overhead.

Not being able to define component requirements externally and conditionally harms composability and usability. The natural way of organizing and encapsulating functionality in Bevy is a plugin, but they currently have no way of defining these relationships. Instead, all the requirements are forced to be on the type definition directly, which leaks implementation details they shouldn't always need to care about. This also hurts third party plugins, as they might want to make their own components required by external components, or even define their own versions of components that are required by some shared component.

Note

For the earlier example specifically, we could technically get around some of the issues with separate DynamicBody, KinematicBody, and StaticBody components that maybe require some shared RigidBodyMarker component. This is a broader issue though, and a lot of the same arguments still apply, especially around composability and third party requirements.

What solution would you like?

Add an add_require method to App. It takes two type arguments: a bundle defining the components that must be present, and the required components that should be added when they are present.

The earlier Mass case could be handled like this:

struct MassPropertyPlugin;

impl Plugin for MassPropertyPlugin {
    fn plugin(app: &mut App) {
        // Add `Mass` only for dynamic rigid bodies.
        // Both components must be present.
        app.add_require::<(RigidBody, Dynamic), Mass>();
    }
}

and the sleeping case like this:

struct SleepingPlugin;

impl Plugin for SleepingPlugin {
    fn plugin(app: &mut App) {
        // Add `SleepTimer` only for dynamic rigid bodies.
        app.add_require::<(RigidBody, Dynamic), SleepTimer>();
    }
}

The second type parameter could even be a bundle, letting you insert multiple components with a single requirement relationship.

Custom constructors can be provided using add_require_with:

app.add_require_with::<RigidBody, SleepTimer>(my_constructor);

These methods have a few nice benefits over the type-level require attribute:

  • In addition to "component requirements", we also have "bundle requirements". All components in a bundle must be present for the component to be inserted, allowing basic conditional insertion.
  • Requirements are optional and composable through plugins. The core RigidBody shouldn't need to care about sleeping unless the plugin for it is enabled.
  • Third party crates can define their own requirements for first party types. For example, "each Handle<Mesh> should require my custom rendering data components". This also gets around the orphan rule.
  • Users can replace built-in components with their own versions. Let's say GlobalTransform required Transform, but we instead wanted to use our own CustomTransform type. This isn't possible when the requirement is encoded at the type level. Instead, we might want to have a TransformPlugin that defines a relationship like app.add_require::<GlobalTransform, Transform>. This would still allow us to replace it with our own plugin that defines app.add_require::<GlobalTransform, CustomTransform>.
    • Note: This likely isn't how we should actually handle custom transforms. It's mainly just a simple example of how this could be useful in general.

Discussion

Breaking Assumptions? Removing Requirements?

Is this abstraction breaking, and does it invalidate assumptions made by other code? The suggestion to remove requirements was heavily controversial in #14927. Quoting Cart:

In general I think this [Required Component Overrides] is a pattern we should not be encouraging. If some third party SpecialComponent requires Transform, then that third party code (and other third parties that depend on it) can and should be written under the assumption that it will have a Transform.

A developer choosing to break that assumption risks breaking third party code in unexpected and arbitrary ways. And as these dependencies update, they risk adding new cases that also break.

I 100% agree with this sentiment. Being able to arbitrarily remove constraints like this from any plugin is risky and can easily break things.

However, requirements being additive and composable through plugins is not as big of an issue in my opinion. That requirement is semantically tied to the plugin, not the component, and you need to explicitly disable a plugin to remove its requirement constraints. This could indeed break logic that depends on that constraint, but the same is also true for systems and resources. Disabling a plugin just means that you are disabling some piece of functionality, and you can always reimplement it with your own version.

That's a big part of what makes Bevy great in my opinion. Plugins are organizational units that encapsulate and compose functionality in a clean and structured manner, and you can freely replace them with your own versions if you want to. I think this should extend to components to some extent.

To be clear, this shouldn't be the default way to define requirement constraints. The type-level require attribute should still be used in the vast majority of cases, especially for true requirements. For example, a RigidBody should always require a PhysicsTransform. However, for components that only make sense in the context of optional plugins (like SleepPlugin) or are defined in third party code, there should be an external way to define requirements.

QueryFilter

Instead of add_require taking a bundle, it could take a full QueryFilter. This would add the specified components only when the filter is satisfied.

However, I think this would quickly become confusing and technically challenging (impossible?), especially for things like Added<T> and Changed<T>.

The only useful functionality other than taking a bundle would be a way to define Without<T> filters, so you could do e.g. app.add_require::<(RigidBody, Without<Static>), Mass>, meaning that Mass is required by all rigid bodies that are not static.

What alternative(s) have you considered?

Keep inserting components manually using lifecycle hooks where possible, and using observers where it isn't.

I just want some form of:

  • conditional requirements, i.e. only requiring components if some other components are/aren't also present
  • allowing plugins to define component requirements.

What this looks like isn't as important to me, as long as it's possible. I don't think forcing the use of hooks or observers for this is a good solution, unless doing it through required components is impossible from a technical standpoint.

@Jondolf Jondolf 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 Sep 22, 2024
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Sep 22, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Sep 22, 2024

IMO adding requirements like this is a reasonable extension, and poses almost none of the risks from the linked issue. I'm in favor of this work, and I expect @cart is as well.

I like the ideas around conditional requirements as well, although I think that those are distinct enough that they can and should be split out into their own issue and/or PR.

@MalekiRe
Copy link
Contributor

I am heavily in favor of the allowing plugins to define component requirements, and think it should be a separate PR.

@SkiFire13
Copy link
Contributor

It's not clear to me how the Mass example would work in practice. Mass is not default, and even if it was that would seem a footgun for anyone forgetting to set its value.

@UkoeHB
Copy link
Contributor

UkoeHB commented Sep 22, 2024

Another use-case: I want all replicated entities in a networked game to get StateScoped when the Replicated component is inserted on a client entity. Adding a component requirement at the plugin level would avoid race conditions between spawns and adding StateScoped.

@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 22, 2024

It's not clear to me how the Mass example would work in practice. Mass is not default, and even if it was that would seem a footgun for anyone forgetting to set its value.

That example was purely for demonstration purposes (although Default was indeed missing, added it now) and because mass happens to be relevant to me since I'm currently reworking how it works in Avian. The example wasn't intended to represent how mass should actually work in physics engines, since that is not relevant to what this issue is about.

If we do want to go into it though...

Mass needs to be required by dynamic rigid bodies, otherwise they wouldn't be dynamic or make much sense as simulated physics objects. It's a property of the RigidBody type/object in practically all physics engines. The only difference here is that it's stored in a separate component since we're using an ECS.

Physics engines need to assume some default value for this, commonly either 0.0 or 1.0. Many engines (like Rapier) treat zero mass as infinite mass. However, at least in Rapier, Box2D, and Avian, mass properties are computed automatically from the attached colliders at runtime, unless specified otherwise. Especially for angular inertia and the center of mass, this is incredibly important for any non-trivial shapes to get realistic behavior, since they are very difficult to tune correctly by hand. This is also what for example Unity and Godot do for angular inertia and the center of mass by default.

As for the internal representation, almost no engine stores just a raw scalar mass. Instead, they store the inverse mass and inverse angular inertia, since that's what most of the solver uses. The mass exposed to users is just an API-level abstraction.

So yes, you do need to require mass for dynamic rigid bodies, but you're not expected to set it manually everywhere unless you need to do so to get the exact desired behavior. You can always tune it as needed, but generally, the defaults should be pretty reasonable.

@cart
Copy link
Member

cart commented Sep 23, 2024

Adding Runtime Requires

I'm on board for adding requires at runtime. I also don't see any major downsides. The problems I can think of are minor /
almost certainly don't outweigh the usefulness of the feature:

  • This will allow upstream developers to change the performance characteristics of a component insertion. Ex: if you add a require(MyExpensiveComponent) to Transform, suddenly spawning gets way more expensive across the board.
  • This does kind of allow "piercing" abstractions because you can load new semantics onto existing 3rd party concepts (not generally how inheritance should work). In practice I think adding components isn't a big deal here though.

Imo we should add this, but with the general recommendation that this is a "break glass" feature for specific use cases that really need it, not a pattern most people should be employing regularly.

Conditional Requirements

I'm pretty strongly against this unless we add auto-removes to required components. In the current form, this would immediately break down in the context of multiple inserts. And even if we added support for auto-removes, this might still be too complicated. Not shutting this down entirely, but imo this should be considered highly controversial, and even if we get a lot of support for it, I think we should defer this until after the initial rollout of BSN / we see more usage in the real world.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! label Sep 23, 2024
@ricky26
Copy link
Contributor

ricky26 commented Sep 25, 2024

IME with other engines, adding a component based on a query is more commonly applicable than single component requires. One additional case which comes up a lot is automatic removal of implementation detail / state tracking components (i.e. when (A + B + !AState) add AState, when (AState & (!A || !B)) remove AState). (There is a third common pattern where state cleanup is required, but that can't be done transparently for the user.)

I have definitely used queries with excluded components for this too, so I think that "bundle requirements" are too limited.

@BenjaminBrienen BenjaminBrienen added the X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers label Sep 25, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 30, 2024
# Objective

Fixes #15367.

Currently, required components can only be defined through the `require`
macro attribute. While this should be used in most cases, there are also
several instances where you may want to define requirements at runtime,
commonly in plugins.

Example use cases:

- Require components only if the relevant optional plugins are enabled.
For example, a `SleepTimer` component (for physics) is only relevant if
the `SleepPlugin` is enabled.
- Third party crates can define their own requirements for first party
types. For example, "each `Handle<Mesh>` should require my custom
rendering data components". This also gets around the orphan rule.
- Generic plugins that add marker components based on the existence of
other components, like a generic `ColliderPlugin<C: AnyCollider>` that
wants to add a `ColliderMarker` component for all types of colliders.
- This is currently relevant for the retained render world in #15320.
The `ExtractComponentPlugin<C>` should add `SyncToRenderWorld` to all
components that should be extracted. This is currently done with
observers, which is more expensive than required components, and causes
archetype moves.
- Replace some built-in components with custom versions. For example, if
`GlobalTransform` required `Transform` through `TransformPlugin`, but we
wanted to use a `CustomTransform` type, we could replace
`TransformPlugin` with our own plugin. (This specific example isn't
good, but there are likely better use cases where this may be useful)

See #15367 for more in-depth reasoning.

## Solution

Add `register_required_components::<T, R>` and
`register_required_components_with::<T, R>` methods for `Default` and
custom constructors respectively. These methods exist on `App` and
`World`.

```rust
struct BirdPlugin;

impl Plugin for BirdPlugin {
    fn plugin(app: &mut App) {
        // Make `Bird` require `Wings` with a `Default` constructor.
        app.register_required_components::<Bird, Wings>();

        // Make `Wings` require `FlapSpeed` with a custom constructor.
        // Fun fact: Some hummingbirds can flutter their wings 80 times per second!
        app.register_required_components_with::<Wings, FlapSpeed>(|| FlapSpeed::from_duration(1.0 / 80.0));
    }
}
```

The custom constructor is a function pointer to match the `require` API,
though it could take a raw value too.

Requirement inheritance works similarly as with the `require` attribute.
If `Bird` required `FlapSpeed` directly, it would take precedence over
indirectly requiring it through `Wings`. The same logic applies to all
levels of the inheritance tree.

Note that registering the same component requirement more than once will
panic, similarly to trying to add multiple component hooks of the same
type to the same component. This avoids constructor conflicts and
confusing ordering issues.

### Implementation

Runtime requirements have two additional challenges in comparison to the
`require` attribute.

1. The `require` attribute uses recursion and macros with clever
ordering to populate hash maps of required components for each component
type. The expected semantics are that "more specific" requirements
override ones deeper in the inheritance tree. However, at runtime, there
is no representation of how "specific" each requirement is.
2. If you first register the requirement `X -> Y`, and later register `Y
-> Z`, then `X` should also indirectly require `Z`. However, `Y` itself
doesn't know that it is required by `X`, so it's not aware that it
should update the list of required components for `X`.

My solutions to these problems are:

1. Store the depth in the inheritance tree for each entry of a given
component's `RequiredComponents`. This is used to determine how
"specific" each requirement is. For `require`-based registration, these
depths are computed as part of the recursion.
2. Store and maintain a `required_by` list in each component's
`ComponentInfo`, next to `required_components`. For `require`-based
registration, these are also added after each registration, as part of
the recursion.

When calling `register_required_components`, it works as follows:

1. Get the required components of `Foo`, and check that `Bar` isn't
already a *direct* requirement.
3. Register `Bar` as a required component for `Foo`, and add `Foo` to
the `required_by` list for `Bar`.
4. Find and register all indirect requirements inherited from `Bar`,
adding `Foo` to the `required_by` list for each component.
5. Iterate through components that require `Foo`, registering the new
inherited requires for them as indirect requirements.

The runtime registration is likely slightly more expensive than the
`require` version, but it is a one-time cost, and quite negligible in
practice, unless projects have hundreds or thousands of runtime
requirements. I have not benchmarked this however.

This does also add a small amount of extra cost to the `require`
attribute for updating `required_by` lists, but I expect it to be very
minor.

## Testing

I added some tests that are copies of the `require` versions, as well as
some tests that are more specific to the runtime implementation. I might
add a few more tests though.

## Discussion

- Is `register_required_components` a good name? Originally I went for
`register_component_requirement` to be consistent with
`register_component_hooks`, but the general feature is often referred to
as "required components", which is why I changed it to
`register_required_components`.
- Should we *not* panic for duplicate requirements? If so, should they
just be ignored, or should the latest registration overwrite earlier
ones?
- If we do want to panic for duplicate, conflicting registrations,
should we at least not panic if the registrations are *exactly* the
same, i.e. same component and same constructor? The current
implementation panics for all duplicate direct registrations regardless
of the constructor.

## Next Steps

- Allow `register_required_components` to take a `Bundle` instead of a
single required component.
    - I could also try to do it in this PR if that would be preferable.
- Not directly related, but archetype invariants?
robtfm pushed a commit to robtfm/bevy that referenced this issue Oct 4, 2024
# Objective

Fixes bevyengine#15367.

Currently, required components can only be defined through the `require`
macro attribute. While this should be used in most cases, there are also
several instances where you may want to define requirements at runtime,
commonly in plugins.

Example use cases:

- Require components only if the relevant optional plugins are enabled.
For example, a `SleepTimer` component (for physics) is only relevant if
the `SleepPlugin` is enabled.
- Third party crates can define their own requirements for first party
types. For example, "each `Handle<Mesh>` should require my custom
rendering data components". This also gets around the orphan rule.
- Generic plugins that add marker components based on the existence of
other components, like a generic `ColliderPlugin<C: AnyCollider>` that
wants to add a `ColliderMarker` component for all types of colliders.
- This is currently relevant for the retained render world in bevyengine#15320.
The `ExtractComponentPlugin<C>` should add `SyncToRenderWorld` to all
components that should be extracted. This is currently done with
observers, which is more expensive than required components, and causes
archetype moves.
- Replace some built-in components with custom versions. For example, if
`GlobalTransform` required `Transform` through `TransformPlugin`, but we
wanted to use a `CustomTransform` type, we could replace
`TransformPlugin` with our own plugin. (This specific example isn't
good, but there are likely better use cases where this may be useful)

See bevyengine#15367 for more in-depth reasoning.

## Solution

Add `register_required_components::<T, R>` and
`register_required_components_with::<T, R>` methods for `Default` and
custom constructors respectively. These methods exist on `App` and
`World`.

```rust
struct BirdPlugin;

impl Plugin for BirdPlugin {
    fn plugin(app: &mut App) {
        // Make `Bird` require `Wings` with a `Default` constructor.
        app.register_required_components::<Bird, Wings>();

        // Make `Wings` require `FlapSpeed` with a custom constructor.
        // Fun fact: Some hummingbirds can flutter their wings 80 times per second!
        app.register_required_components_with::<Wings, FlapSpeed>(|| FlapSpeed::from_duration(1.0 / 80.0));
    }
}
```

The custom constructor is a function pointer to match the `require` API,
though it could take a raw value too.

Requirement inheritance works similarly as with the `require` attribute.
If `Bird` required `FlapSpeed` directly, it would take precedence over
indirectly requiring it through `Wings`. The same logic applies to all
levels of the inheritance tree.

Note that registering the same component requirement more than once will
panic, similarly to trying to add multiple component hooks of the same
type to the same component. This avoids constructor conflicts and
confusing ordering issues.

### Implementation

Runtime requirements have two additional challenges in comparison to the
`require` attribute.

1. The `require` attribute uses recursion and macros with clever
ordering to populate hash maps of required components for each component
type. The expected semantics are that "more specific" requirements
override ones deeper in the inheritance tree. However, at runtime, there
is no representation of how "specific" each requirement is.
2. If you first register the requirement `X -> Y`, and later register `Y
-> Z`, then `X` should also indirectly require `Z`. However, `Y` itself
doesn't know that it is required by `X`, so it's not aware that it
should update the list of required components for `X`.

My solutions to these problems are:

1. Store the depth in the inheritance tree for each entry of a given
component's `RequiredComponents`. This is used to determine how
"specific" each requirement is. For `require`-based registration, these
depths are computed as part of the recursion.
2. Store and maintain a `required_by` list in each component's
`ComponentInfo`, next to `required_components`. For `require`-based
registration, these are also added after each registration, as part of
the recursion.

When calling `register_required_components`, it works as follows:

1. Get the required components of `Foo`, and check that `Bar` isn't
already a *direct* requirement.
3. Register `Bar` as a required component for `Foo`, and add `Foo` to
the `required_by` list for `Bar`.
4. Find and register all indirect requirements inherited from `Bar`,
adding `Foo` to the `required_by` list for each component.
5. Iterate through components that require `Foo`, registering the new
inherited requires for them as indirect requirements.

The runtime registration is likely slightly more expensive than the
`require` version, but it is a one-time cost, and quite negligible in
practice, unless projects have hundreds or thousands of runtime
requirements. I have not benchmarked this however.

This does also add a small amount of extra cost to the `require`
attribute for updating `required_by` lists, but I expect it to be very
minor.

## Testing

I added some tests that are copies of the `require` versions, as well as
some tests that are more specific to the runtime implementation. I might
add a few more tests though.

## Discussion

- Is `register_required_components` a good name? Originally I went for
`register_component_requirement` to be consistent with
`register_component_hooks`, but the general feature is often referred to
as "required components", which is why I changed it to
`register_required_components`.
- Should we *not* panic for duplicate requirements? If so, should they
just be ignored, or should the latest registration overwrite earlier
ones?
- If we do want to panic for duplicate, conflicting registrations,
should we at least not panic if the registrations are *exactly* the
same, i.e. same component and same constructor? The current
implementation panics for all duplicate direct registrations regardless
of the constructor.

## Next Steps

- Allow `register_required_components` to take a `Bundle` instead of a
single required component.
    - I could also try to do it in this PR if that would be preferable.
- Not directly related, but archetype invariants?
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 S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants