bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add Immutable `Component` Support

Open bushrat011899 opened this issue 1 year ago • 33 comments

Objective

  • Fixes #16208

Solution

  • Added an associated type to Component, Mutability, which flags whether a component is mutable, or immutable. If Mutability= Mutable, the component is mutable. If Mutability= Immutable, the component is immutable.
  • Updated derive_component to default to mutable unless an #[component(immutable)] attribute is added.
  • Updated ReflectComponent to check if a component is mutable and, if not, panic when attempting to mutate.

Testing

  • CI
  • immutable_components example.

Showcase

Users can now mark a component as #[component(immutable)] to prevent safe mutation of a component while it is attached to an entity:

#[derive(Component)]
#[component(immutable)]
struct Foo {
    // ...
}

This prevents creating an exclusive reference to the component while it is attached to an entity. This is particularly powerful when combined with component hooks, as you can now fully track a component's value, ensuring whatever invariants you desire are upheld. Before this would be done my making a component private, and manually creating a QueryData implementation which only permitted read access.

Using immutable components as an index
/// This is an example of a component like [`Name`](bevy::prelude::Name), but immutable.
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Component)]
#[component(
    immutable,
    on_insert = on_insert_name,
    on_replace = on_replace_name,
)]
pub struct Name(pub &'static str);

/// This index allows for O(1) lookups of an [`Entity`] by its [`Name`].
#[derive(Resource, Default)]
struct NameIndex {
    name_to_entity: HashMap<Name, Entity>,
}

impl NameIndex {
    fn get_entity(&self, name: &'static str) -> Option<Entity> {
        self.name_to_entity.get(&Name(name)).copied()
    }
}

fn on_insert_name(mut world: DeferredWorld<'_>, entity: Entity, _component: ComponentId) {
    let Some(&name) = world.entity(entity).get::<Name>() else {
        unreachable!()
    };
    let Some(mut index) = world.get_resource_mut::<NameIndex>() else {
        return;
    };

    index.name_to_entity.insert(name, entity);
}

fn on_replace_name(mut world: DeferredWorld<'_>, entity: Entity, _component: ComponentId) {
    let Some(&name) = world.entity(entity).get::<Name>() else {
        unreachable!()
    };
    let Some(mut index) = world.get_resource_mut::<NameIndex>() else {
        return;
    };

    index.name_to_entity.remove(&name);
}

// Setup our name index
world.init_resource::<NameIndex>();

// Spawn some entities!
let alyssa = world.spawn(Name("Alyssa")).id();
let javier = world.spawn(Name("Javier")).id();

// Check our index
let index = world.resource::<NameIndex>();

assert_eq!(index.get_entity("Alyssa"), Some(alyssa));
assert_eq!(index.get_entity("Javier"), Some(javier));

// Changing the name of an entity is also fully capture by our index
world.entity_mut(javier).insert(Name("Steven"));

// Javier changed their name to Steven
let steven = javier;

// Check our index
let index = world.resource::<NameIndex>();

assert_eq!(index.get_entity("Javier"), None);
assert_eq!(index.get_entity("Steven"), Some(steven));

Additionally, users can use Component<Mutability = ...> in trait bounds to enforce that a component is mutable or is immutable. When using Component as a trait bound without specifying Mutability, any component is applicable. However, methods which only work on mutable or immutable components are unavailable, since the compiler must be pessimistic about the type.

Migration Guide

  • When implementing Component manually, you must now provide a type for Mutability. The type Mutable provides equivalent behaviour to earlier versions of Component:
impl Component for Foo {
    type Mutability = Mutable;
    // ...
}
  • When working with generic components, you may need to specify that your generic parameter implements Component<Mutability = Mutable> rather than Component if you require mutable access to said component.
  • The entity entry API has had to have some changes made to minimise friction when working with immutable components. Methods which previously returned a Mut<T> will now typically return an OccupiedEntry<T> instead, requiring you to add an into_mut() to get the Mut<T> item again.

Draft Release Notes

Components can now be made immutable while stored within the ECS.

Components are the fundamental unit of data within an ECS, and Bevy provides a number of ways to work with them that align with Rust's rules around ownership and borrowing. One part of this is hooks, which allow for defining custom behavior at key points in a component's lifecycle, such as addition and removal. However, there is currently no way to respond to mutation of a component using hooks. The reasons for this are quite technical, but to summarize, their addition poses a significant challenge to Bevy's core promises around performance. Without mutation hooks, it's relatively trivial to modify a component in such a way that breaks invariants it intends to uphold. For example, you can use core::mem::swap to swap the components of two entities, bypassing the insertion and removal hooks.

This means the only way to react to this modification is via change detection in a system, which then begs the question of what happens between that alteration and the next run of that system? Alternatively, you could make your component private to prevent mutation, but now you need to provide commands and a custom QueryData implementation to allow users to interact with your component at all.

Immutable components solve this problem by preventing the creation of an exclusive reference to the component entirely. Without an exclusive reference, the only way to modify an immutable component is via removal or replacement, which is fully captured by component hooks. To make a component immutable, simply add #[component(immutable)]:

#[derive(Component)]
#[component(immutable)]
struct Foo {
    // ...
}

When implementing Component manually, there is an associated type Mutability which controls this behavior:

impl Component for Foo {
    type Mutability = Mutable;
    // ...
}

Note that this means when working with generic components, you may need to specify that a component is mutable to gain access to certain methods:

// Before
fn bar<C: Component>() {
    // ...
}

// After
fn bar<C: Component<Mutability = Mutable>>() {
    // ...
}

With this new tool, creating index components, or caching data on an entity should be more user friendly, allowing libraries to provide APIs relying on components and hooks to uphold their invariants.

Notes

  • ~~I've done my best to implement this feature, but I'm not happy with how reflection has turned out. If any reflection SMEs know a way to improve this situation I'd greatly appreciate it.~~ There is an outstanding issue around the fallibility of mutable methods on ReflectComponent, but the DX is largely unchanged from main now.
  • I've attempted to prevent all safe mutable access to a component that does not implement Component<Mutability = Mutable>, but there may still be some methods I have missed. Please indicate so and I will address them, as they are bugs.
  • Unsafe is an escape hatch I am not attempting to prevent. Whatever you do with unsafe is between you and your compiler.
  • I am marking this PR as ready, but I suspect it will undergo fairly major revisions based on SME feedback.
  • I've marked this PR as Uncontroversial based on the feature, not the implementation.

bushrat011899 avatar Nov 13 '24 06:11 bushrat011899

The behavior of implementing Component before this change and the behavior of implementing Component + ComponentMut after this change should be identical. Do I understand that correctly?

BenjaminBrienen avatar Nov 13 '24 06:11 BenjaminBrienen

Yes a pre-this-PR Component is identical to a this-PR Component + ComponentMut. Component contains all the implementation details it had previously, but now only implies an immutable type. Mutability is now explicitly stated by implementing ComponentMut. But for the derive macro, Component + ComponentMut are implemented by default (since that is the most typical use-case). To opt-out of mutability in the derive macro, you add #[immutable].

bushrat011899 avatar Nov 13 '24 06:11 bushrat011899

Small nit: I would prefer #[component(immutable)] to keep all component attributes together. It also follows #[world_query(mutable)].

ItsDoot avatar Nov 13 '24 07:11 ItsDoot

I've updated the macro to instead use #[component(immutable)]. It's much clearer what's happening and should be cleaner too. Good suggestion @ItsDoot.

bushrat011899 avatar Nov 13 '24 07:11 bushrat011899

Of note, FilteredEntityMut::get_mut_by_id is (so far) the only safe method I have found that can bypass immutable components. I did want to add the immutable flag to ComponentDescriptor, but propagating that information proved very challenging. If anyone has a suggestion for how to integrate ComponentMut and ComponentDescriptor in the least impactful way I would be greatly appreciative.

bushrat011899 avatar Nov 13 '24 08:11 bushrat011899

Why do you prefer the ComponentMut: Component design over a Mutable + Component design? I have a mild preference for the latter because I think it'll be easier to extend to resources. Broadly happy with this otherwise though, although I do think the reflection and dynamic component stories should probably be improved 🤔

alice-i-cecile avatar Nov 13 '24 14:11 alice-i-cecile

Don't have time to look over this fully, but I like this. I also prefer the version without the trait bound on component.

Just so I am sure this can be used as I want, if we make parent/children immutable, how do we preserve the existing hierarchy commands api? Will we use unsafe within the commands to get mutable access, or go properly immutable with only clones and inserts?

NthTensor avatar Nov 13 '24 14:11 NthTensor

Just so I am sure this can be used as I want, if we make parent/children immutable, how do we preserve the existing hierarchy commands api? Will we use unsafe within the commands to get mutable access, or go properly immutable with only clones and inserts?

I was wondering if it would make sense to have mutable component access require a key type. Then crates could keep that type private to simulate immutability while still being able to mutate the component themselves.

Not sure if that's possible and I don't know how well it fits with this approach, but possibly an option (though I’m going to guess far more complex and involved).

MrGVSV avatar Nov 13 '24 15:11 MrGVSV

or go properly immutable with only clones and inserts

It would be this. Either through Parent's on insert hook or a command.

iiYese avatar Nov 13 '24 15:11 iiYese

We'll have to make Children immutable too, won't we? That will mean some extra vec cloning, so might want to look at an immutable vector impl. But I'm content with that answer. We can deal with the costs if it turns out to be a problem.

NthTensor avatar Nov 13 '24 16:11 NthTensor

Why do you prefer the ComponentMut: Component design over a Mutable + Component design? I have a mild preference for the latter because I think it'll be easier to extend to resources.

The three reasons for me were:

  1. With reflection, we'd need a name for the trait that represents the mutable trait methods, and I'm not sure if we can do ReflectComponentMutable
  2. For updating where conditions it was slightly cleaner to write ComponentMut than Component + Mutable
  3. When we do extend this to resources, we'd have to use a separate trait anyway, because there are types that implement both Resource and Component, which would create a conflict on the common Mutable

bushrat011899 avatar Nov 13 '24 16:11 bushrat011899

It's probably fine to keep this component-only for now, considering that resources don't currently support hooks (so the applications are pretty minimal).

Eventually it seems like resources are going to become components anyway.

NthTensor avatar Nov 13 '24 16:11 NthTensor

We'll have to make Children immutable too, won't we? That will mean some extra vec cloning, so might want to look at an immutable vector impl. But I'm content with that answer. We can deal with the costs if it turns out to be a problem.

This shouldn't be required. The data is only immutable while it is stored on the entity, so remove/mutate/insert can be used to avoid any cloning. If that pattern is cumbersome we could add a component_scope method like resource_scope, except with ownership of the component.

bushrat011899 avatar Nov 13 '24 16:11 bushrat011899

This shouldn't be required. The data is only immutable while it is stored on the entity, so remove/mutate/insert can be used to avoid any cloning. If that pattern is cumbersome we could add a component_scope method like resource_scope, except with ownership of the component.

Wouldn't this create unnecessary archetype moves? I feel like cloning is probably faster (in this case at least). But that's getting off topic.

NthTensor avatar Nov 13 '24 17:11 NthTensor

Perhaps, then we'd do an Indiana Jones-style replacement where you temporarily place an empty/placeholder value while you're mutating the component. Basically std::mem::swap but ensuring hooks are still triggered.

But agreed, this is an optimisation question we can improve.

bushrat011899 avatar Nov 13 '24 17:11 bushrat011899

You could even do a mutate_via_reinsertion API, which could work on immutable components. Very clean, and no archetype moves. Easy followup though.

alice-i-cecile avatar Nov 13 '24 17:11 alice-i-cecile

What happens if you ask for a &mut C on an immutable C in a system? Is it the compile error where your function doesn't implement system?

hymm avatar Nov 13 '24 18:11 hymm

Compiler error. The QueryData implementation is not provided for immutable components.

bushrat011899 avatar Nov 13 '24 18:11 bushrat011899

There's some discussion on Discord around the name immutable. These components are mutable, but only when outside of the ECS. Once stored on an entity they become immutable. To clarify this, an alternative name is being bikeshedded. My personal preference is for frozen, as that has the same implications as immutable, with the added possibility of "thawing" (remove to mutate)

bushrat011899 avatar Nov 13 '24 18:11 bushrat011899

For posterity, frozen gets my vote as well, but "restricted components" is a safe fallback.

alice-i-cecile avatar Nov 13 '24 18:11 alice-i-cecile

I'm a strong proponent of "immutable". It's the right level of technical precision for the users who will know they want to use this, and those who are unfamiliar will mostly hit error messages which we can make more friendly. But I'm happy to be overruled.

NthTensor avatar Nov 13 '24 18:11 NthTensor

My objection to "immutable" is that many of the most useful operations involve indirectly mutating the data involved. Which is... confusing. It's also a lot harder to Google. I do appreciate the crisp technical correctness from FP here though. We'll see what the other SMEs want; I don't think either choice is bad.

alice-i-cecile avatar Nov 13 '24 19:11 alice-i-cecile

I'd love if there was a trait like ComponentNonMut. bevy_mod_index would want to use that as a bound for components that can be reliability tracked with hooks now that mutations can be prevented.

chrisjuchem avatar Nov 13 '24 20:11 chrisjuchem

We could maybe add an unsafe trait ComponentNonMut: Component { } to cover that usecase. We would then have to make ComponentMut unsafe too, since they're mutually exclusive. The derive macro could hide these details tho.

bushrat011899 avatar Nov 13 '24 20:11 bushrat011899

Unfortunately Rust's trait system doesn't play very nice with that sort of negative bound. If we had both MutableComponent and ImmutableComponent as traits, there's nothing stopping users from implementing both. I agree that both forms would be useful though.

Would an associated type work here?

impl <C: Component<Mutable=true>> Index {} seems pretty workable, and I don't think it's much worse / less clear than the current design. It would also bypass the nasty reflection problems of the current approach.

alice-i-cecile avatar Nov 13 '24 20:11 alice-i-cecile

Using an associated type on Component allows resolving the reflection DX issues previously introduced by this PR. Additionally, we can now offer a marker trait ComponentImmutable which is effectively !ComponentMut.

bushrat011899 avatar Nov 13 '24 23:11 bushrat011899

After further iteration, the ComponentMut and ComponentImmutable traits have been removed, opting instead for matching against the associated type Mutability.

/// Work with a component, regardless of its mutability
fn get_component<C: Component>(/* ... */) { /* ... */ }

/// _Only_ allow mutable components
fn get_component_mut<C: Component<Mutability = Mutable>>(/* ... */) { /* ... */ }

/// _Only_ allow immutable components
fn get_component_immutable<C: Component<Mutability = Immutable>>(/* ... */) { /* ... */ }

If you find this cumbersome, you can easily create your own blanket-impl trait(s):

pub trait ComponentMut: Component<Mutability = Mutable> {}
impl<C: Component<Mutability = Mutable>> ComponentMut for C {}

pub trait ComponentNonMut: Component<Mutability = Immutable> {}
impl<C: Component<Mutability = Immutable>> ComponentNonMut for C {}

bushrat011899 avatar Nov 14 '24 01:11 bushrat011899

I think that my only remaining major request is that this has a test suite for dynamic components. Once that's done I'll do a final polish pass on this at the start of 0.16.

alice-i-cecile avatar Nov 14 '24 02:11 alice-i-cecile

I really like how this PR is turning out. I'm a fan of the associated type idea.

BenjaminBrienen avatar Nov 14 '24 07:11 BenjaminBrienen

I have added a second example, immutable_components_dynamic, which demonstrates creating dynamic immutable components at runtime. In particular, the example shows that while get_by_id(...) will succeed for an immutable component, get_mut_by_id(...) will return an Err, since that component cannot be mutably accessed.

There is definitely room for better documentation, and there's probably some additional methods/changes to existing methods we'd want to do before merging, but I think I have sufficiently covered the bulk of the work for this feature. I look forward to more detailed feedback in the coming weeks!

bushrat011899 avatar Nov 14 '24 10:11 bushrat011899