-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add Immutable Component
Support
#16372
base: main
Are you sure you want to change the base?
Conversation
The `Component` trait now implies an immutable, readonly component. To add mutability, you must implement `ComponentMut`, which is a simple marker. In the derive macro, `ComponentMut` will be implemented with `Component` unless you add an `#[immutable]` attribute.
The behavior of implementing |
Yes a pre-this-PR |
Small nit: I would prefer |
I've updated the macro to instead use |
Of note, |
Why do you prefer the |
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? |
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). |
It would be this. Either through |
Co-Authored-By: Alice Cecile <[email protected]>
After further iteration, the /// 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- 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 {} |
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. |
I really like how this PR is turning out. I'm a fan of the associated type idea. |
Demonstrates using the immutable components feature with dynamic components. We go through the process of creating dynamic immutable components, adding them to an entity, and then testing retrieval methods.
I have added a second example, 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! |
As an aside, I added |
The associated trait magic with sealed logic types is very cute, I like it. Is the issue with |
Thanks! And yes that issue is resolved. The move to an associated type made enough information available on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some thinking about what this API will let us do over my lunch-break, and I'm very pleased. An immutable component with an insert hook can cache data on a private required mutable component with extremely little overhead (at most one archetype move, and some non-structural mutations). This seems to align perfectly with the direction the engine is going with required components and hooks (and, eventually, archetype invariants).
Looked over the code and saw no issues, as far as I am aware this is good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below is some notes to assist reviewers understand the decisions I've made for this PR.
/// # Safety | ||
/// - the `drop` fn must be usable on a pointer with a value of the layout `layout` | ||
/// - the component type must be safe to access from any thread (Send + Sync in rust terms) | ||
pub unsafe fn new_immutable_with_layout( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered just adding an immutable: bool
parameter to the existing new_with_layout
, but this avoids another breaking change in the API.
crates/bevy_ecs/src/component.rs
Outdated
@@ -841,6 +930,7 @@ impl ComponentDescriptor { | |||
type_id: Some(TypeId::of::<T>()), | |||
layout: Layout::new::<T>(), | |||
drop: needs_drop::<T>().then_some(Self::drop_ptr::<T> as _), | |||
immutable: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't added immutability to Resource
s in this PR to keep the scope contained. I suspect it would basically just be a duplication of some of the infrastructure added for immutable Component
s anyway, so once this PR is merged a followup should be straight-forward. However, without Resource
-hooks, I'm not sure how useful it'd be.
@@ -46,7 +46,7 @@ pub mod prelude { | |||
pub use crate::{ | |||
bundle::Bundle, | |||
change_detection::{DetectChanges, DetectChangesMut, Mut, Ref}, | |||
component::Component, | |||
component::{Component, Immutable, Mutable}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little concerned about putting these in the prelude, since they're such generic terms. If any other area of Bevy needs to use similar terms, I think it'd make sense to move these mutability markers into their own module, independent of component
(perhaps change_detection
?).
Happy to remove them from the prelude if anyone has concerns. They're still publicly accessible regardless.
let mut observed_by = entity_mut.entry::<ObservedBy>().or_default(); | ||
let mut observed_by = entity_mut.entry::<ObservedBy>().or_default().into_mut(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the consequence of changes made to the entry API for components. Ideally, we would return Mut<T>
when calling or_default()
on a mutable component, and &T
on an immutable component (and likewise for other entry methods). The problem is that would require specialisation, since the Rust compiler has no way of knowing that Component<Mutability = Mutable>
and Component<Mutability = Immutable>
are mutually exclusive traits.
Since we already have the OccupiedEntry
type, I decided to return that for all relevant operations instead, since it already has methods to either get a im/mutable reference to the underlying component. The alternatives would either be to not allow the entry API for immutable components (bad), or duplicate all the methods (or_default_immutable()
, etc.)
/// Inserts the component into the `VacantEntry` and returns a mutable reference to it. | ||
/// | ||
/// # Examples | ||
/// | ||
/// ``` | ||
/// # use bevy_ecs::{prelude::*, world::Entry}; | ||
/// #[derive(Component, Default, Clone, Copy, Debug, PartialEq)] | ||
/// struct Comp(u32); | ||
/// | ||
/// # let mut world = World::new(); | ||
/// let mut entity = world.spawn_empty(); | ||
/// | ||
/// if let Entry::Vacant(v) = entity.entry::<Comp>() { | ||
/// v.insert(Comp(10)); | ||
/// } | ||
/// | ||
/// assert_eq!(world.query::<&Comp>().single(&world).0, 10); | ||
/// ``` | ||
#[inline] | ||
pub fn insert(self, component: T) -> Mut<'a, T> { | ||
self.entity_world.insert(component); | ||
// This shouldn't panic because we just added this component | ||
self.entity_world.get_mut::<T>().unwrap() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed because insert_entry
and insert
would be identical with insert
returning an OccupiedEntry
. As previously mentioned, if we had specialisation we wouldn't need to make this change.
/// | ||
/// - `T` must be a mutable component | ||
#[inline] | ||
pub unsafe fn into_mut_assume_mutable<T: Component>(self) -> Option<Mut<'w, T>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These x_assume_mutable
methods are the escape-hatch to let you work with a possibly mutable component. I don't like expanding the unsafe API (especially the public unsafe API), but there isn't really a clean alternative. As mentioned in earlier reviews, this is at least a very small safety invariant to uphold.
// - No drop command is required | ||
// - The component will store [u8; size], which is Send + Sync | ||
let descriptor = unsafe { | ||
ComponentDescriptor::new_immutable_with_layout( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all that really changes when working with immutable dynamic components; the ComponentDescriptor
just needs to include the immutable flag and then everything else works as it did previously.
// ...but we cannot gain a mutable reference. | ||
assert!(entity.get_mut_by_id(*component_id).is_err()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A runtime-error is as-good-as it gets for dynamic immutable components. Not ideal but there's no compile-time options for hopefully pretty obvious reasons.
This is an alternative to storing `immutable`, which is strictly speaking a negation on `mutable`.
Co-authored-by: Benjamin Brienen <[email protected]>
@@ -265,6 +266,8 @@ impl ReflectComponent { | |||
|
|||
impl<C: Component + Reflect + TypePath> FromType<C> for ReflectComponent { | |||
fn from_type() -> Self { | |||
// TODO: Currently we panic of a component is immutable and you use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: Currently we panic of a component is immutable and you use | |
// TODO: Currently we panic if a component is immutable and you use |
component.apply(reflected_component.as_partial_reflect()); | ||
entity.insert(component); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this intentional?
entity.insert(component); |
@@ -273,12 +276,22 @@ impl<C: Component + Reflect + TypePath> FromType<C> for ReflectComponent { | |||
entity.insert(component); | |||
}, | |||
apply: |mut entity, reflected_component| { | |||
let mut component = entity.get_mut::<C>().unwrap(); | |||
if !C::Mutability::MUTABLE { | |||
panic!("This component is immutable, you cannot modify it through reflection"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: For debuggability, could we maybe have these panic messages indicate the method that caused the error? Even better would be to provide the name of the component.
Maybe something like: {component_name} is immutable, it cannot be modified via `ReflectComponent::apply`
?
Objective
Solution
Component
,Mutability
, which flags whether a component is mutable, or immutable. IfMutability= Mutable
, the component is mutable. IfMutability= Immutable
, the component is immutable.derive_component
to default to mutable unless an#[component(immutable)]
attribute is added.ReflectComponent
to check if a component is mutable and, if not, panic when attempting to mutate.Testing
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: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
Additionally, users can use
Component<Mutability = ...>
in trait bounds to enforce that a component is mutable or is immutable. When usingComponent
as a trait bound without specifyingMutability
, 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
Component
manually, you must now provide a type forMutability
. The typeMutable
provides equivalent behaviour to earlier versions ofComponent
:Component<Mutability = Mutable>
rather thanComponent
if you require mutable access to said component.Mut<T>
will now typically return anOccupiedEntry<T>
instead, requiring you to add aninto_mut()
to get theMut<T>
item again.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 onReflectComponent
, but the DX is largely unchanged frommain
now.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.