bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Allow certain components to be marked immutable

Open nakedible opened this issue 1 year ago • 8 comments

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

There are needs to enforce invariants, to build indexes or otherwise keep things consistent in the ECS between multiple entities or between multiple components in a single entity. The new "component hooks" system goes a long way towards making it easy to keep things in sync, as they can be triggered on component addition, removal and replace.

However, the hooks system does not catch mutation currently, and catching mutation has a lot of difficult problems to solve if it needs to be synchronously and with every way of mutating a component. Even if all the fields inside a component are marked private, that doesn't prevent the user from assigning a new instance of that component to it, overwriting the data for the old component - and no hooks will be fired, only asynchronous change events. This means that it is currently impossible to use hooks to maintain an invariant if the user mutates components directly.

What solution would you like?

There could be a flag on components that marks the component immutable. When set, it would mean that no API will return mutable references to the component, making it impossible to mutate the values in the component. This doesn't mean that there would be any change in how insert, remove, take etc. would work, so it would still be fully permitted to remove the component, or add a new one, or replace an existing with a new one. The current hooks system will fire on_replace and on_insert hooks when a component is replaced this way instead of mutated in-place.

How the mutation APIs should behave when called on a component that is marked immutable is still unspecified. Perhaps they should panic when trying to mutate a component that can't be mutated. Or maybe they act as if the component doesn't exist on the object if attempting to get a mutable reference. Or perhaps this can somehow be extended all the way to the type system so the calls which attempt to take a mutable reference to a component simply won't compile because those are not implemented for components which are immutable.

What alternative(s) have you considered?

The big alternative is that absolutely no change is needed. Preventing mutation is just a safeguard for certain kinds of components, and if the fields of the component are private, doing a mutable assignment on a component is not easy to accidentally do – so for most usages it's probably enough to just say in documentation "please don't mutate this component".

The second alternative is also that absolutely no change is needed. Such components may be made private, and exposed through QueryData and Bundle in interfaces such that the real component is never accessible, hence never mutated. It might still be accessible via reflection, save/load, etc. but all bets are off there anyway. This is not as nice as it requires a bunch of boilerplate, and the whole point of component hooks was to allow such invariants to be implemented without creating a shadow API to do all the ECS operations while keeping invariants.

The third alternative is to somehow make synchronous change detection hooks work. There is already change detection, so it's not so impossible to think that it could run a hook real time, possibly both before change and after a change. Then we could just have on_mutate and that would probably cover the vast majority of cases that would benefit from immutable components.

Additional context

I'm writing this issue mainly as a placeholder, and to see comments. It's one problem related to hooks that may or may not be common enough to warrant doing something about it. If there comes a strong use case for this that isn't easily solved by other means, and this problem is the primary motivation to make components private, then I will likely want to revisit this.

nakedible avatar Nov 01 '24 20:11 nakedible

The status-quo for this is to create a private Component and a public QueryData, but that doesn't allow replacing the value (instead you'd need to also add some commands). Supporting an immutable Component would eliminate this boilerplate.

Ideally, it would be a compiler error to ask for a mutable reference to an immutable component. Perhaps Component as a trait could be modified to include a parameter indicating mutability, and the mutable QueryData implementation would not be available for said parameter? Or Component could be split into a Component and ComponentMut pair of traits?

bushrat011899 avatar Nov 01 '24 21:11 bushrat011899

It could be as simple as the Component derive also adding an impl for a Mutable trait. You can have a #[immutable] attribute macro that opts out of the Mutable impl. Then just:

impl<T: Component + Mutable> QueryData for &'_ mut T {
    // ..
}

iiYese avatar Nov 01 '24 21:11 iiYese

Then just impl<T: Component + Mutable> QueryData for &'static mut T

I like this. Additionally we still need to store a mutability flag in the ComponentInfo and then check it when accessing components by id.

SpecificProtagonist avatar Nov 01 '24 22:11 SpecificProtagonist

Following some more discussion on Discord, there seems to be broad consensus that this feature is desirable and that we should use the design laid out in https://github.com/bevyengine/bevy/issues/16208#issuecomment-2452633963, which allows this to be easily extended to Resource as well.

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

Hey there! Just wanted to quickly drop in and say that this feels very useful. Please keep up the great work! :)


Otherwise, the design discussion on discord was pretty cool! I immediately thought of a scorched-earth solution, which is pretty much a secondary Component trait (like ComponentMut) that's opt-out through some kind of #[component::immutable] attr. Buuuuut... that seems somewhat hard to reason about, especially if anyone manually implements Component, then finds mysterious mutability errors everywhere. That's probably fixable through our god-given #[diagnostic::on_unimplemented(...)]. The discussion on reflection and a better implementation feels pretty solid, though, so I can't wait to see it materialize further!

Overall, I think most folks could get some decent use out of this. That's doubly so for large codebases

onkoe avatar Nov 30 '24 08:11 onkoe

Immutable components sound great !

I'm interested to know, although I can imagine: why an on_mutate hook is so tricky to implement ? 🤔

I guess that, when component hooks were first implemented, there were conversations about this, because on_mutate feels like a natural companion to on_insert, on remove, etc; but I missed them

maximetinu avatar Nov 30 '24 10:11 maximetinu

Immutable components sound great !

I'm interested to know, although I can imagine: why an on_mutate hook is so tricky to implement ? 🤔

I guess that, when component hooks were first implemented, there were conversations about this, because on_mutate feels like a natural companion to on_insert, on remove, etc; but I missed them

I think due to mem::swap but I could be wrong

BenjaminBrienen avatar Nov 30 '24 10:11 BenjaminBrienen

I'm interested to know, although I can imagine: why an on_mutate hook is so tricky to implement ? 🤔

Hooks, and to a lesser extent observers, want to run immediately after the change is made. Because of Bevy's access model, users can mutate data without having access to the full world, delaying the effect. Even if this wasn't true, there would be a serious performance risk due to the loss of cache locality during table iteration.

We can get around this by only implementing on_mutate observers, but there are still concerns around efficiently checking what has been changed. The current change detection impl relies on an O(n) scan every time, which could get expensive very fast, in hard to debug ways.

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