-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Allow bundle to be inserted without overwriting duplicate components #14397
Comments
I would accept an additional method that did this (on World, EntityWorldMut and Commands). In the long-term, I expect that required components (see #14437) will have large implications for this space. |
Is this the same as #2054? |
Yes, it looks basically the same. (I did search for duplicates before filing this, honest!) |
Often there are reasons to insert some components (e.g. Transform) separately from the rest of a bundle (e.g. PbrBundle). However `insert` overwrites existing components, making this difficult. This PR adds the method `insert_if_new` to EntityMut and Commands, which is the same as `insert` except that the old component is kept in case of conflicts. It also renames some internal enums (from `ComponentStatus::Mutated` to `Existing`), to reflect the possible change in meaning. - Did you test these changes? If so, how? Added basic unit tests; used the new behavior in my project. - Are there any parts that need more testing? There should be a test that the change time isn't set if a component is not overwritten; I wasn't sure how to write a test for that case.
@alice-i-cecile I have a PR for this (#14646), if you're still ok accepting it. |
Often there are reasons to insert some components (e.g. Transform) separately from the rest of a bundle (e.g. PbrBundle). However `insert` overwrites existing components, making this difficult. This PR adds the method `insert_if_new` to EntityMut and Commands, which is the same as `insert` except that the old component is kept in case of conflicts. It also renames some internal enums (from `ComponentStatus::Mutated` to `Existing`), to reflect the possible change in meaning. - Did you test these changes? If so, how? Added basic unit tests; used the new behavior in my project. - Are there any parts that need more testing? There should be a test that the change time isn't set if a component is not overwritten; I wasn't sure how to write a test for that case.
# Objective Often there are reasons to insert some components (e.g. Transform) separately from the rest of a bundle (e.g. PbrBundle). However `insert` overwrites existing components, making this difficult. See also issue #14397 Fixes #2054. ## Solution This PR adds the method `insert_if_new` to EntityMut and Commands, which is the same as `insert` except that the old component is kept in case of conflicts. It also renames some internal enums (from `ComponentStatus::Mutated` to `Existing`), to reflect the possible change in meaning. ## Testing *Did you test these changes? If so, how?* Added basic unit tests; used the new behavior in my project. *Are there any parts that need more testing?* There should be a test that the change time isn't set if a component is not overwritten; I wasn't sure how to write a test for that case. *How can other people (reviewers) test your changes? Is there anything specific they need to know?* `cargo test` in the bevy_ecs project. *If relevant, what platforms did you test these changes on, and are there any important ones you can't test?* Only tested on Windows, but it doesn't touch anything platform-specific. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>
What problem does this solve or what need does it fill?
Say you have a simulation that can run headless. In headless mode, entities still get a
TransformBundle
, but don't need any rendering info. In normal mode, they get a fullPbrBundle
.Or maybe you don't have headless, but just want the simulation-related code to not have to depend on rendering stuff like materials and meshes.
A nice way to implement this is to have the simulation code add the TransformBundle with positions etc., and have a rendering system with
Query<Entity, Added<MyThing>>
which inserts aPbrBundle
when active.But
PbrBundle
containsTransform
, so if you do this, it will clobber the existing transform.[Note that
Transform
is just an example here, albeit the most common one. The issue applies to Bundles in general.]What solution would you like?
It would be helpful to have a variant of
insert()
that gives precedence to the existing components instead of the new ones. E.g. maybeinsert_if_new()
.What alternative(s) have you considered?
The name could be bikeshedded of course.
insert()
could be changed to just never overwrite an existing component. Personally I've never encountered a situation where the "replace" behavior is what I want, but changinginsert
's behavior now would certainly be a large and disruptive change and probably isn't feasible even if everyone agreed it was desirable.Instead of a new method name, the "noclobber" behavior could come from some magic wrapper in the bundle arguments. E.g.
One advantage of this approach is you could have three choices: clobber, no_clobber, and error. But it's more complex (e.g. you would have to define what happens in case of nested combinations of different clobber choices. Yuk.)
Instead of changing
insert
, bundles could gain a "subtract" operation. E.g.PbrBundle { ... }.without::<TransformBundle>()
.A workaround for now is to have your
add_rendering_components
system from the example above also query forTransform
and any similar components, and explicitly copy them into the newly inserted bundle. The downside of this is that you have to know about a bunch of specific fields in the bundle, and if the simulation starts using other components you have to update the renderer to manually avoid clobbering those too. It also makes a spurious change to theTransform
(or whatever components are involved), which the simulation has to be careful not to be affected by.An alternative workaround is to not insert e.g.
PbrBundle
, but instead the various components it uses. But this is fragile since the bundle components tend to change between Bevy versions.Additional context
#6622 - related discussion of documenting the existing behavior.
I think the implementation would be pretty straightforward:
BundleInserter
, add a fieldif_exists
for whether keep/replace behavior is desired.insert_if_new
methods, which setBundleInserter.if_exists = Keep
BundleInfo::write_components
, in theComponentStatus::Mutated
case, ifif_exists==Keep
, do nothing instead of callingcolumn.replace
.I'd be happy to make this change if there's a chance it'll be accepted.
The text was updated successfully, but these errors were encountered: