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

Support Removing Required Components at Runtime #14927

Closed
omaskery opened this issue Aug 26, 2024 · 43 comments
Closed

Support Removing Required Components at Runtime #14927

omaskery opened this issue Aug 26, 2024 · 43 comments
Labels
A-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales C-Feature A new feature, making something new possible S-Wontfix This issue is the result of a deliberate design decision, and will not be fixed X-Controversial There is active debate or serious implications around merging this PR

Comments

@omaskery
Copy link

omaskery commented Aug 26, 2024

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

We are adding the ability to require components in Bevy, this feature request asks for the ability to override this behaviour when it prevents you from customizing third party code.

For example, suppose you're using some hypothetical Bevy provided components & systems:

Suppose there are components:

  • SpecialComponent (which requires Transform),
  • Transform (which requires GlobalTransform), and
  • GlobalTransform.

Suppose there is the system: update_globaltransform_for_specialcomponents_with_transforms.

You're having a great time, until you reach an edge case where your requirements deviate from Bevy's assumptions, so you try to swap out some types:
You create a new component: BetterTransform.
You create a new system: update_globaltransform_for_specialcomponents_with_bettertransforms.

Pre-"Required Components": you would be unable to use bundles that assumed SpecialComponent should use Transform, potentially defining your own new bundles for convenience, but you'd be able to move on and achieve your goals.

In a world with "Required Components": you cannot currently "un-require" Transform from SpecialComponent in 3rd party code (without forking), resulting in this situation:

Your types end up with all of the components: SpecialComponent pulls in Transform, you add BetterTransform, Transform and BetterTransform pull in GlobalTransform.
You end up with two systems trying to compute the GlobalTransform for SpecialComponents:
update_globaltransform_for_specialcomponents_with_transforms, and
update_globaltransform_for_specialcomponents_with_bettertransforms.

What solution would you like?

Currently the required components are authored using an annotation. This feature request proposes that methods be added to Bevy's App type for customizing them dynamically, a hypothetical set of methods:

  • app.add_requires::<DependantComponent, RequiredComponent>();
  • app.remove_requires::<DependantComponent, RequiredComponent>();

(Names bike-sheddable as always!).

These would, as the names hopefully suggest, dynamically (un)register the "required components" relationship between those two types. The annotations in the current implementation would just be a short-hand for automatically registering these relationships.

What alternative(s) have you considered?

Extensive discussion in Discord (the Next Generation Scene/UI Working Group channel), including but not limited to:

  • Not having required components (because BSN might add good enough Nested Scenes to address the need)
  • A full-fledged "provides" system where instead of requiring a component you require some contract that other components provide
  • ???

Effectively, Required Components are going in (soon? today? already?), so we're committed to experimenting with those and learning from it. This feature request effectively acts as a short-term solution to ensure the addition of Required Components doesn't cause a surge in use that prevents customizability.

Additional context

N/A

@omaskery omaskery added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 26, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Aug 26, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events P-High This is particularly urgent, and deserves immediate attention and removed S-Needs-Triage This issue needs to be labelled labels Aug 26, 2024
@alice-i-cecile
Copy link
Member

Great writeup, and I think this is an essential escape hatch. I think that the relatively straightforward "just register / unregister required components" is the right call for now.

@NiseVoid
Copy link
Contributor

NiseVoid commented Aug 26, 2024

Since @alice-i-cecile asked me to put my more detailed description of provides here:

Components can register theirselves as providing another component. If we want to add Transform, but the entity already has a component providing Transform, it does not need to be inserted. Alternatively this could be extended to a tuple of components providing another component, for example (Position, Rotation) could provide Transform.

Some interesting usecases for this would be replacing things like Transform that still have a well-defined edge to write back to (GlobalTransform in this case), or systems working with trait queries that might require some default but allow you to overwrite it.

This feature could of course be extended in many ways (like making things mutually exclusive if we ever get archetype invariants), but for this usecase having it as a way to avoid required components inserting unwanted components would be enough.

(This idea was originally brought up by @NthTensor, which in turn was inspired by another idea from Joy relating to plugin dependencies)

@alice-i-cecile
Copy link
Member

Long-term, I think we're going to want both: provides is a good mechanism for plugin authors, but the simple override is more suitable for end users.

@cart
Copy link
Member

cart commented Aug 26, 2024

Required Component Overrides

In general I think this is a pattern we should not be encouraging. If some third party SpecialComponent requires Transform, then that third party code (and other third parties that depend on it) can and should be written under the assumption that it will have a Transform.

A developer choosing to break that assumption risks breaking third party code in unexpected and arbitrary ways. And as these dependencies update, they risk adding new cases that also break.

Encouraging the "remove arbitrary upstream components" pattern is abstraction-breaking and "ecosystem stability"-hostile.

The ability to remove a required component constraint is an "unsafe escape hatch". If we were to include it, I think we'd want to heavily discourage its use in 3rd party plugins.

I also think we should heavily reconsider adding a global app.remove_requires::<SpecialComponent, Transform>(), as that is a blunt-force tool that could break things that rely on that behavior. I strongly suspect that anyone trying to do this wants to do it in specific contexts. Therefore, I think we should consider implementing this as an extension of the per-component require API:

#[derive(Component)]
#[require(SpecialComponent)]
#[remove_require(SpecialComponent, Transform)]
struct MySpecialComponent;

This would mean that existing uses of SpecialComponent do not break. This comes at the cost of needing to specify MySpecialComponent instead of (or in addition to) SpecialComponent during spawn. But it also means we aren't reaching in and invalidating existing upstream assumptions (which again, is pretty much never a good idea).

"Provides" API

In the context of a provides API, given that the computation has to happen somewhere, how is #[provides((Position, Rotation), Transform)] any different from this? Why do we need a new abstraction?

#[derive(Component)]
#[require(Transform)]
struct Position {
  x: f32,
  y: f32,
  z: f32,
}

#[derive(Component, Default)]
#[require(Transform)]
struct Rotation(Quat);

#[derive(Component)]
#[require(Transform)]
struct MyThing;

commands.spawn((
  MyThing,
  Position { x: 1.0, y: 0.0, z: 0.0 },
  Rotation::default(),
));

fn sync_position_rotation(query: Query<(&Position, &Rotation, &mut Transform)>) {
  // sync code here
}

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed P-High This is particularly urgent, and deserves immediate attention labels Aug 26, 2024
@alice-i-cecile
Copy link
Member

For more context, Transform is the only component that I've ever seen this sort of replacement requested for. Nothing else is similarly deeply integrated, and Transform itself has a number of limitations / frustrations that aren't ideal for all users. In particular:

  • no f64 support
  • no interpolation
  • difficulty controlling what properties get inherited
  • wasteful and unergonomic for 2D

It may be that the best solution here is to close this as Won't Fix (following Cart's points about abstraction breaking), but work harder to make sure that Transform can meet the needs of all of our users by solving some of these problems (or at least making more behavior configurable).

@alice-i-cecile alice-i-cecile added the A-Transform Translations, rotations and scales label Aug 26, 2024
@NthTensor
Copy link
Contributor

NthTensor commented Aug 26, 2024

This is the general idea of Provides, which seems really different from your example.

#[derive(Component)]
struct A;

#[derive(Component)]
#[provides(A)]
struct B;

#[derive(Component)]
#[requires(A)]
struct C;

commands.spawn((B, C)); // does not insert A

The idea is that inserting (Position, Rotation) would opt-you-out of getting a Transform.

Without a mechanism like this it will quite difficult to alter the existing semantics of a component imported from a dependency. That dosn't seem very "Modular: Use only what you need. Replace what you don't like" to me.

@cart
Copy link
Member

cart commented Aug 26, 2024

The only way I can see this working in a way that doesn't break arbitrary upstream code is using dynamic dispatch, which for the case of per-frame-per-entity Transform code would not be ideal.

@cart
Copy link
Member

cart commented Aug 26, 2024

And if the goal is to remove a stored computed Transform, that would mean that every time a Transform is needed for an entity with (Position, Rotation), we'd need to compute it on the spot / on demand. Each point of consumption would have a high cost. That, combined with dynamic dispatch makes the wins here pretty dubious relative to just making Position and Rotation drive the Transform component.

@NthTensor
Copy link
Contributor

NthTensor commented Aug 26, 2024

The only way I can see this working in a way that doesn't break arbitrary upstream code is using dynamic dispatch, which for the case of per-frame-per-entity Transform code would not be ideal.

Because now we can output multiple archetypes? I can see how that might be an issue.

And if the goal is to remove a stored computed Transform, that would mean that every time a Transform is needed for an entity with (Position, Rotation), we'd need to compute it on the spot / on demand. Each point of consumption would have a high cost. That, combined with dynamic dispatch makes the wins here pretty dubious relative to just making Position and Rotation drive the Transform component.

The point is not to remove a stored computed Transform. The point is rather to, for example, allow users to replace the bevy built-in transform with their own f64 bit version or what have you. Position and Rotation just happened to appear in your example, i don't know why they were chosen.

Provides lets you say 'actually, I have my own Transform-like component, with it's own systems that update it. Everywhere that would usually take a bevy::Transform can use mine instead.' without still having unused bevy transforms floating around and complicating things.

@cart
Copy link
Member

cart commented Aug 26, 2024

Provides lets you say 'actually, I have my own Transform-like component, with it's own systems that update it. Everywhere that would usually take a bevy::Transform can use mine instead.' without still having unused bevy transforms floating around and complicating things.

Can you describe how this would be implemented functionally, if not in a way that:

  1. Computes a common Transform / Mat4 / whatever our chosen "interface" ends up being (not a component, just a rust type) at each point of consumption using some form of dynamic dispatch
  2. Computes a common Transform / Mat4 / whatever our chosen "interface" ends up being at a given point in the schedule, using a driver system.

I'm having trouble understanding what your ideal system is.

@cart
Copy link
Member

cart commented Aug 26, 2024

Additionally, the "common interface" is going to need to select things like layout + precision, which will be opinionated in the same way that Transform is currently opinionated.

@NthTensor
Copy link
Contributor

NthTensor commented Aug 26, 2024

We're not talking about having these share traits, or have one component type actually provide an interface for another. I do not know where you are getting "common interface" from. The rule, as I originally framed it, is as follows: if X provides Y then bevy acts exactly as if Y is provided for the purpose of required component resolution when spawning a bundle containing X. Thats it.

#[require(Transform)]
struct SpecialComponent;

#[require(SpecialComponent)]
#[remove_require(SpecialComponent, Transform)]
struct MySpecialComponent;

// vs

#[require(SpecialComponent)]
#[provides(Transform)]
struct MySpecialComponent;

The last two defs do the same thing.

@omaskery
Copy link
Author

I need time to digest Carts original response, but wanted to flag that this issue isn't intended to be about Transform replacement specifically, that was just the best thing I could come up with as an example for the original issue. I'd like people not to get too focussed on the specifics of Transforms.

@cart
Copy link
Member

cart commented Aug 26, 2024

I'd like people not to get too focussed on the specifics of Transforms.

I think its good to explore this usecase because:

  1. Its the only real-world use case we're currently aware of
  2. It could easily inform the design of this system

@cart
Copy link
Member

cart commented Aug 26, 2024

if X provides Y then bevy acts exactly as if Y is provided for the purpose of required component resolution when spawning a bundle containing X. That it.

Gotcha. I was playing off of this "contract" phrasing:

A full-fledged "provides" system where instead of requiring a component you require some contract that other components provide

In combination with stated desire to have something less flimsy than reaching in to replace a component with another component.

I'm not really a fan of provides without static guarantees that it:

  1. Actually performs the same function
  2. Doesn't break arbitrary upstream code

I don't see it as meaningfully better than in-place remove_require and require.

@NthTensor
Copy link
Contributor

Fair enough, I can agree with that. I do have thoughts about making transforms more modular, and you did ask some interesting questions I'd like to answer. But I view them as separate from this issue, and think they deserve a proper proposal to lay everything out.

@NiseVoid
Copy link
Contributor

I see a lot of mentions of "breaking upstream assumptions", but I don't think that's fully accurate. There's plenty of cases where a component might be registered as required but not actually necessary, many bundles in bevy currently include Transform despite only really needing GlobalTransform. We also aren't introducing a new problem, it's already possible to remove these components, it's just slower and annoying to do so, while if we add add_require (which has a lot more uses) also adding remove_require is a relatively "cheap" escape hatch until we have a more substanial solution for such replacements.

I also think we should move away from insisting things (especially user code) shouldn't be able to break plugin assumptions, especially if those assumptions are completely unfounded because we do not in fact have anything like archetype invariants. I've seen people use the same argument to resist entity disabling, while in reality not having good solutions for such problems often causes far more problems than having them ever could.

@NiseVoid
Copy link
Contributor

NiseVoid commented Aug 26, 2024

I also don't see how provides and #[remove_require] are equivalent. Provides would allow B to replace A on any T that requires A, while remove_require only works if you replace every T with your own MyT, which would result in a situation where ultimately you miss one and end up having both A and B.

@ricky26
Copy link
Contributor

ricky26 commented Aug 26, 2024

In general I think this is a pattern we should not be encouraging. If some third party SpecialComponent requires Transform, then that third party code (and other third parties that depend on it) can and should be written under the assumption that it will have a Transform.

I have two problems with this assertion:

  1. Requires are already being considered for convenience in discussion, rather than as part of the invariants of the library. The example given was that adding Button to an entity should create a working button (for it to work, it would need a Transform, even though the code that implements Button likely would only need GlobalTransform). This is a conflict that cannot be resolved (we have no way to prevent requirements being used for convenience, and it seems like it's desired), and based on how people are discussing it, is a mistake that library authors will make. We already have an ecosystem of crates which have similar ergonomics issues for simple oversights (such as the inability to disable systems which are not public).
  2. Pretty much every way of accessing components on entities returns Option<> or Result<>. This is required anyway, since you might do any number of dynamic modifications to entities, and library code absolutely should not panic over issues such as missing components.

From my point of view, required components are a standardised replacement for systems which would add required components, which avoid common developer pitfalls and improve performance. As such, in the same way that one can remove a system, one should be able to remove a requirement.

Encouraging the "remove arbitrary upstream components" pattern is abstraction-breaking and "ecosystem stability"-hostile.

I would argue that not including the escape hatch is a big risk to the ecosystem also. My personal experience with bevy is that I constantly have to fork crates because they haven't followed good practice in a way which allows me to prevent them from misbehaving.

The ability to remove a required component constraint is an "unsafe escape hatch". If we were to include it, I think we'd want to heavily discourage its use in 3rd party plugins.

This I broadly agree with - I would really only expect this to be used at the root application level. I fully expect it to be a common occurrence though.

@alice-i-cecile
Copy link
Member

As such, in the same way that one can remove a system, one should be able to remove a requirement.

Notably, we can't currently remove systems (although you can permanently disable them), and doing so requires exposing a public API for the system: either a system set or a public fn type. Cart's been consistent with the stance against reaching in and messing with plugin internals over the years, so the pushback here doesn't surprise me.

@NthTensor
Copy link
Contributor

NthTensor commented Aug 26, 2024

I can see the argument that if you don't want Transform semantics then don't add the transform plugin (in retrospect maybe thats a bad example). I guess my question would be, what does that look like with required components? Required components seem to 'harden' dependencies between plugins, where before you could just swap out the bundles now it's probably going to crash.

Both of these sides seem pretty reasonable to me, I'd be surprised if we can't come to some kind of common understanding here.

@NiseVoid
Copy link
Contributor

Of course when bevy makes real guarantees to end users, with something like archetype invariants, we can't let other crates or end users mess with those, because unsafe code later might be built on those guarantees. It is however extremely unlikely we'd see something like "Every archetype with Handle<Mesh> MUST also have Transform".

@omaskery
Copy link
Author

I haven't thought this through, but just to throw ideas at the wall: instead of adding and removing requirements, you could have variants of the insert() methods of the form insert_without_required() (name unimportant right now) to opt out when you know what you're doing? 🤔

@NiseVoid
Copy link
Contributor

NiseVoid commented Aug 26, 2024

insert_without_required()

I'd imagine this would be very much possible, but I doubt it would very practically solve this problem. And at the same time you'd have to manually list out all the other requires, which could get quite annoying. I imagine there's basically two cases for removing a require:

  1. One specific component has a require you really don't want it to have, either globally or under specific conditions. This could be because of an upstream oversight or a "convenience require" that ends up being not so convenient.
  2. You have a component that can fully replace another component and don't want the old version to waste memory and do useless calculations only for it's output to be overwritten anyway

Meanwhile adding a require could be the counterpart of reason 2, looser coupling to optional plugins in the same crate ("RigidBody requires SleepTimer only if SleepPlugin is enabled", convenience for the end user ("I need every entity with X to also have Y for this system")

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Aug 26, 2024

FYI, there's a pretty ergonomic (if inefficient) workaround for this using observers: listen for whenever a component of the undesired type is added, and then replace it with your own. Definitely wasteful, but foolproof and very easy to set up. You can even check within that observer if the entity meets some other condition.

@omaskery
Copy link
Author

omaskery commented Aug 26, 2024

From my point of view, the claimed pros/cons of required components are:

Pros:

  1. Dev experience: it's easier to remember what components to add
  2. WG Dependency: it is listed as a requirement for the new scenes/UI WG work
  3. Invariants: it helps with preserving invariants

Cons:

  1. People will have to fork 3rd party crates more often because people will overuse required components.

I have concerns about the pros, though:

  1. Dev experience:

    • We currently have bundles for that (although there are debates about how good those are)
    • We are planning on adding BSN (etc.) to improve our ability to ergonomically abstract over entity [hierarchy + component] authoring
  2. WG Dependency:

    • It still isn't clear to me what the justification for this is:
    • It seems to me like it's just a "nice to have" in order to reduce boilerplate in hand-written BSN assets.
    • I presume that it doesn't have any real impact on boilerplate in bsn! macros because presumably we can use Rust to reduce boilerplate there.
  3. Invariants:

    • All systems currently have to be written robustly against those invariants not holding already because of the ECS API
    • In an ECS you should be robust to components being added and removed, as that's the primary way users interact with your API

Since Required Components are going ahead regardless we're going to have to just get on with it and learn together how this will go. This issue request is effectively just suggesting a mitigation for a downside of this new feature.


Got a bit over excited there, now having read Alice's comment: this means there is a known workaround to mitigate this. With that in place, it's now only really a question of what our stance is on the issue, this work isn't required because a workaround exists.

This issue only really serves a purpose if we want a better workaround, or use this opportunity to brainstorm how we communicate the new required components feature. Are we going to encourage people to use it? For what? Are we going to discourage people from using it? Are we going to document the pros/cons of using it?

@NiseVoid
Copy link
Contributor

NiseVoid commented Aug 26, 2024

I think the pros of required components are actually:

  1. It's impossible to forget the components and end up in a "why does it not work?" moment
  2. All required components can be added in a single archetype move, this is massively more efficient than existing "manual required component" implementations
  3. It makes things a lot less verbose, even compared to bundles

What it doesn't do is:

  1. Give any insurances about invariants

Alice's workaround would work, but it feels counter to advantage 2. The #[remove_require] seems counter to 3, since you could end up with dozens of MyT variants just to avoid one component. Meanwhile not having a way to add/remove required components only harms the non-goal of required components being archetype invariants.

Also I think it's important to note that required components isn't a BSN requirement, BSN could probably work with bundles, it would just be clunky. The community however decided they'd rather see required components first

@omaskery

This comment was marked as off-topic.

@alice-i-cecile

This comment was marked as off-topic.

@cart
Copy link
Member

cart commented Aug 26, 2024

@NiseVoid

many bundles in bevy currently include Transform despite only really needing GlobalTransform

Yeah we've discussed only requiring GlobalTransform in these contexts. However in cases where users are expected to position entities, we do really want to include a Transform by default to ensure that systems like this work as expected:

fn x(query: Query<&mut Transform, With<X>>) {
}

For a given X (ex: Sprite) we essentially always want exactly one specific and predictable driver component to:

  1. Avoid every consuming system from needing to manually handle every possible permutation of driver component
  2. To ensure that plugins developed to interact with the transform of X will be able to
  3. To avoid logic breaking when a user fails to insert Transform manually

I think the solution is to prefer GlobalTransform -only for "intermediate" Components that aren't really intended to be used on their own. Then require whatever driver makes sense for "top level" Components like Sprite.

We also aren't introducing a new problem, it's already possible to remove these components,

Definitely true, although I'll still assert that removing a component like Transform is unadvisable generally, given the unpredictable upstream effects. This argument applies to all upstream components outside of the users control. Don't remove something unless it was designed to be removed.

I also think we should move away from insisting things (especially user code) shouldn't be able to break plugin assumptions

Allowing people to build solid abstractions is a critical piece of the "modular ecosystem" puzzle. See my reply below for thoughts on this.

@ricky26

Requires are already being considered for convenience in discussion, rather than as part of the invariants of the library.

Requires are an actual, reliable invariant in that they guarantee that the components will exist at insertion time. Supporting remove_require allows people to reach in and break that guarantee. Ensuring components are never removed is an additional invariant we could add, but its not a desirable default and the existence of that potential invariant doesn't invalidate the value and use of "required at spawn".

Requires are not just a convenience from my perspective. They exist as a piece of the puzzle to build reliable abstractions. In this case, they exist to define a predictable / functional initial state of an entity.

I would argue that not including the escape hatch is a big risk to the ecosystem also.

The risk that providing an escape hatch poses is as high if not higher imo (see my next reply).

From my point of view, both should be possible or we increase the risk of having a cardboard cut-out of modular engine design. An example of the failure in the ecosystem at the moment is when systems aren't exposed, and you need to ensure ordering in your app.

Being able to build private, unaddressable abstractions that downstream users cannot depend on is an important tool for enabling modularity actually. Making everything public/addressable/usable creates a world where people build on the details of an implementation that were not intentionally designed to be built on. An ecosystem where people are building on internal implementation details will chaotically break, making it hard to trust. An important part of making Bevy "modular" is intentional design of public APIs and intentional hiding of implementation details.

@NiseVoid
Copy link
Contributor

Definitely true, although I'll still assert that removing a component like Transform is unadvisable generally, given the unpredictable upstream effects.

I definitely agree that doing this arbitrarily would be unadvisable. This is however an API you have to go out of your way to use, and it could have plenty of warnings included so people understand that if things break it's their fault.

Requires are an actual, reliable invariant in that they guarantee that the components will exist at insertion time.

I guess that is indeed a guarantee, it is a bit limited in its scope but things like hooks and observers would still allow for this guarantee to be used. One possibility would be for "add this for convenience" and "add this because I need the guarantee" separate, but I'd imagine it would be hard to get right.

I think the solution is to prefer GlobalTransform -only for "intermediate" Components that aren't really intended to be used on their own. Then require whatever driver makes sense for "top level" Components like Sprite.

One option could be to have add_require in a plugin so GlobalTransform can require Transform, but only if the TransformPlugin is added to the app. This way if conevience requires ever pose a problem they could be thrown into an optional plugin or config, while the plugin developer still has control over what people can and can't unrequire. This gives us both a fully efficient solution that plugin developers can account for during development, with alice's solution (using observers to remove the offending component) as a fallback

@cart
Copy link
Member

cart commented Aug 26, 2024

One option could be to have add_require in a plugin so GlobalTransform can require Transform

Not really an option on its own, as we're already living in a world where we don't want Transform for every GlobalTransform (ex: we're discussing Transform2d and removing Transform for UI Nodes)

@NiseVoid
Copy link
Contributor

we don't want Transform for every GlobalTransform

Yea I guess this specific case wouldn't work out once we split up Transform. You'd either have to express this as "GlobalTransform requires a transform source" (which creates a whole set of new problems), or have each transform register itself as requirements for relevant types if that plugin is added (which sounds like a coupling nightmare)

@ricky26
Copy link
Contributor

ricky26 commented Aug 27, 2024

Definitely true, although I'll still assert that removing a component like Transform is unadvisable generally, given the unpredictable upstream effects. This argument applies to all upstream components outside of the users control. Don't remove something unless it was designed to be removed.

I don't think I agree with this. To take it slightly ad absurdum, does this mean we should never destroy entities with components that shouldn't be removed (as destruction implies removal of all components)? Is this to protect against bugs in cleanup?

Allowing people to build solid abstractions is a critical piece of the "modular ecosystem" puzzle. See my reply below for thoughts on this.

I absolutely agree with this, I think we're just considering the boundary of the abstractions to be in different places.

Requires are an actual, reliable invariant in that they guarantee that the components will exist at insertion time.

Is this a desirable invariant? Is it even observable? If you remove the component with a hook as soon as it is added, technically the invariant has been upheld (the component was added at insertion time), but no library author could rely on this for anything.

Requires are not just a convenience from my perspective. They exist as a piece of the puzzle to build reliable abstractions. In this case, they exist to define a predictable / functional initial state of an entity.

I don't think that requires are at the right level to be useful for building abstractions. Since the components on an entity are inherently publicly exposed, any abstraction over which components are associated with an entity is going to be leaky. There's no way for a library to maintain its invariants between calls.

You can't have a functional initial state and multiple ways to structure an entity. If you have a Text component that could exist with either a Transform or UiTransform, what do you do?

Being able to build private, unaddressable abstractions that downstream users cannot depend on is an important tool for enabling modularity actually. Making everything public/addressable/usable creates a world where people build on the details of an implementation that were not intentionally designed to be built on. An ecosystem where people are building on internal implementation details will chaotically break, making it hard to trust. An important part of making Bevy "modular" is intentional design of public APIs and intentional hiding of implementation details.

I think that it's already hard to reason about what needs to be exposed as part of the interface of a plugin crate. Systems being addressable (by system or exposing sets) so that ordering is part of the interface is a common oversight.

At the moment the reality is that Bevy plugins aren't all successful abstractions. If they were, we wouldn't have the integration issues we do.

If requires are going to be used to provide reasonable defaults, I definitely think that an escape hatch is justified (since that's well outside the remit of invariants), but even otherwise, I think it's far preferable to forking crates.

@BenjaminBrienen BenjaminBrienen added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Sep 26, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.15 milestone Sep 30, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Sep 30, 2024
@ricky26
Copy link
Contributor

ricky26 commented Oct 6, 2024

Is it possible to get some form of clarity on this issue before the 0.15 RC?

If I understand the current status-quo, once 0.15 is released, plugin authors will be encouraged to use requires, and in particular should be able to treat required component co-existence as invariant.

This says to me that using unwrap in systems on dependent components, or writing unsafe code which relies on this invariant should be fair game, which in turn means that removing components is effectively unsafe.

Additionally, requires will be recommended for sensible initial state and plugin authors will have to choose between making their plugins more flexible, or easy to use.

This situation makes me pretty uncomfortable.

@alice-i-cecile
Copy link
Member

This says to me that using unwrap in systems on dependent components, or writing unsafe code which relies on this invariant should be fair game, which in turn means that removing components is effectively unsafe.

This is not the case: please don't rely on dependent components existing! These aren't archetype invariants: it's only a guarantee at insertion time, not for the whole lifetime. Your plugin code should still try to be reasonably robust to these failing, especially if you expose public systems or other backdoors.

@Jondolf
Copy link
Contributor

Jondolf commented Oct 6, 2024

Alice already responded while I was writing my own message :p but leaving it here anyway:

This says to me that using unwrap in systems on dependent components, or writing unsafe code which relies on this invariant should be fair game, which in turn means that removing components is effectively unsafe.

Required components are primarily an insertion mechanism that ensures that components are initialized with all the other components that they need to function as expected. This can be thought of as a kind of invariant, but not one that is enforced at runtime. As a plugin author, I don't expect to be able to treat this as as an absolute, and to be able to use unwrap or unsafe code that just assumes the invariant to be held. That would need archetype invariants. Perhaps this should be stated more explicitly in the documentation however.

Also, this is already kind of the case with bundles. You could assume that users will always use PbrBundle instead of adding every component manually, but if users don't do that, or remove components at runtime, you'd end up with the same issues. Required components are essentially a more ergonomic and reliable replacement for bundles.

As for one of the other things raised in this issue, adding requirements at runtime, that was addressed by #15458. It also makes it technically possible to have optional requirements if they are added through plugins (just don't add the plugin). Arbitrarily removing requirements on the other hand I consider to be a non-goal for several reasons already discussed here.

@alice-i-cecile
Copy link
Member

I think we've exhausted this conversation. We can see how this plays out in practice and reopen this if there are ecosystem problems, but I'm going to mark "removing required component constraints" as won't fix.

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale Oct 6, 2024
@alice-i-cecile alice-i-cecile added S-Wontfix This issue is the result of a deliberate design decision, and will not be fixed and removed S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged labels Oct 6, 2024
@alice-i-cecile alice-i-cecile changed the title Support Overriding Required Components at Runtime Support Removing Required Components at Runtime Oct 6, 2024
@ricky26
Copy link
Contributor

ricky26 commented Oct 6, 2024

@alice-i-cecile,

This is not the case: please don't rely on dependent components existing! These aren't archetype invariants: it's only a guarantee at insertion time, not for the whole lifetime. Your plugin code should still try to be reasonably robust to these failing, especially if you expose public systems or other backdoors.

I'm not planning to do this, or recommending it, I was just trying to summarise what the current stance is (though it looks like I'm still missing something). To quote @cart:

If some third party SpecialComponent requires Transform, then that third party code (and other third parties that depend on it) can and should be written under the assumption that it will have a Transform.

This is what lead me to the conclusion I made above. The further comment about not removing components that are not designed to be removed seems in-line too.

@Jondolf, I hope your interpretation is correct, but it seems to me that there's not a consensus in this particular issue.

Anyway, my goal here wasn't to create more noise, as @alice-i-cecile says, we can see how this plays out. 🙂

@cart
Copy link
Member

cart commented Oct 9, 2024

This is what lead me to the conclusion I made above. The further comment about not removing components that are not designed to be removed seems in-line too.

I said this in the context of removing requires defined by components you depend on (not removing components brought in by a require). With requires, the one invariant you should be able to rely on is "this will exist at insertion time". All other constraints about what you can remove are defined by the context of each component.

@github-staff github-staff deleted a comment from Lxx-c Oct 23, 2024
@github-staff github-staff deleted a comment from Lxx-c Oct 23, 2024
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 A-Transform Translations, rotations and scales C-Feature A new feature, making something new possible S-Wontfix This issue is the result of a deliberate design decision, and will not be fixed X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

10 participants