bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.46k stars 3.6k forks source link

Add Immutable `Component` Support #16372

Open bushrat011899 opened 2 weeks ago

bushrat011899 commented 2 weeks ago

Objective

Solution

Testing


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 ```rust /// 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, } impl NameIndex { fn get_entity(&self, name: &'static str) -> Option { 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::() else { unreachable!() }; let Some(mut index) = world.get_resource_mut::() 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::() else { unreachable!() }; let Some(mut index) = world.get_resource_mut::() else { return; }; index.name_to_entity.remove(&name); } // Setup our name index world.init_resource::(); // Spawn some entities! let alyssa = world.spawn(Name("Alyssa")).id(); let javier = world.spawn(Name("Javier")).id(); // Check our index let index = world.resource::(); 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::(); 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

Notes

BenjaminBrienen commented 2 weeks ago

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?

bushrat011899 commented 2 weeks ago

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].

ItsDoot commented 2 weeks ago

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

bushrat011899 commented 2 weeks ago

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 commented 2 weeks ago

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.

alice-i-cecile commented 2 weeks ago

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 🤔

NthTensor commented 2 weeks ago

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?

MrGVSV commented 2 weeks ago

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).

iiYese commented 2 weeks ago

or go properly immutable with only clones and inserts

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

NthTensor commented 2 weeks ago

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.

bushrat011899 commented 2 weeks ago

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
NthTensor commented 2 weeks ago

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.

bushrat011899 commented 2 weeks ago

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.

NthTensor commented 2 weeks ago

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.

bushrat011899 commented 2 weeks ago

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.

alice-i-cecile commented 2 weeks ago

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

hymm commented 2 weeks ago

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?

bushrat011899 commented 2 weeks ago

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

bushrat011899 commented 2 weeks ago

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)

alice-i-cecile commented 2 weeks ago

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

NthTensor commented 2 weeks ago

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.

alice-i-cecile commented 2 weeks ago

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.

chrisjuchem commented 2 weeks ago

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.

bushrat011899 commented 2 weeks ago

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.

alice-i-cecile commented 2 weeks ago

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.

bushrat011899 commented 2 weeks ago

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 commented 2 weeks ago

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 {}
alice-i-cecile commented 2 weeks ago

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.

BenjaminBrienen commented 2 weeks ago

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

bushrat011899 commented 2 weeks ago

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 commented 2 weeks ago

As an aside, I added #[component(immutable)] to Name to see if anything broke, and to my pleasant surprise it just worked without any knock-on effects. Bevy already treats Name as some immutable data, so making it "official" is a 1-line PR once this is merged. I haven't included that in this PR just to keep the scope as small as it can be.

NthTensor commented 2 weeks ago

The associated trait magic with sealed logic types is very cute, I like it. Is the issue with FilteredEntityMut::get_mut_by_id resolved or still outstanding?

bushrat011899 commented 2 weeks ago

The associated trait magic with sealed logic types is very cute, I like it. Is the issue with FilteredEntityMut::get_mut_by_id resolved or still outstanding?

Thanks! And yes that issue is resolved. The move to an associated type made enough information available on Component for me to have immutability be a bool on ComponentDescriptor.