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

Runtime required components #15458

Merged
merged 23 commits into from
Sep 30, 2024
Merged

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Sep 26, 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 Retain Rendering World (adopted) #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.

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.
  2. Register Bar as a required component for Foo, and add Foo to the required_by list for Bar.
  3. Find and register all indirect requirements inherited from Bar, adding Foo to the required_by list for each component.
  4. 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?

@Jondolf Jondolf added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 26, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 26, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Sep 26, 2024
@alice-i-cecile
Copy link
Member

This is going to want to get rolled into the existing required components release notes.

@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 26, 2024

Removing C-Breaking-Change actually, since required components are a new feature and the method mentioned in the migration guide didn't even exist in prior releases.

@Jondolf Jondolf removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 26, 2024
crates/bevy_ecs/src/component.rs Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Show resolved Hide resolved
crates/bevy_app/src/app.rs Show resolved Hide resolved
crates/bevy_app/src/app.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers label Sep 26, 2024
@SkiFire13
Copy link
Contributor

I gave a quick glance, but it seems this is not accounting for archetype edges that were created before the required component was registered. Could you add a test that:

  • creates an entity with a component A
  • registers B as required for A
  • creates a new entity with A
  • checks that B has been added to new entity

I would expect this to either pass the check or at least panic when B is registered as required for A after its list of required component has been used in some archetype transition.

@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 27, 2024

it seems this is not accounting for archetype edges that were created before the required component was registered

You're right, this is an issue...

For now, I took the easy route of panicking when the requiring component (A in your example) exists in an archetype before the requirement is registered. I don't think I'm confident enough in the ECS internals to make it update the archetype edges and everything correctly, although I'd love to see it implemented in a follow-up (or if someone wants to make a PR on my branch, or guide me through how it should work).

This does mean that these won't be fully "runtime" requirements in the sense that you can't register requirements at arbitrary moments in time, and must do it before the requiree is inserted into the world for the first time. But I think that would be a pretty niche use case anyway; in the vast majority of cases, I expect users to be doing these registrations in plugins in build, and that should still work, unless you're directly spawning entities there for whatever reason.

@alice-i-cecile
Copy link
Member

Yep, I agree with that as a compromise for now :) Plugin building finishing before anything is spawned is a reasonable assumption to get this shipped.

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

I love the documentation and tests especially

crates/bevy_ecs/src/component.rs Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Jondolf Jondolf added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Sep 30, 2024
@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 30, 2024

@alice-i-cecile I fixed merge conflicts, so this should be good to go again

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 30, 2024
Merged via the queue into bevyengine:main with commit f3e8ae0 Sep 30, 2024
26 checks passed
@Jondolf Jondolf deleted the runtime-requires branch October 3, 2024 00:04
robtfm pushed a commit to robtfm/bevy that referenced this pull request 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?
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1707 if you'd like to help out.

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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow plugins to define component requirements
4 participants