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

Non-overwriting insert commands #2054

Closed
alice-i-cecile opened this issue Apr 29, 2021 · 8 comments · Fixed by #14646
Closed

Non-overwriting insert commands #2054

alice-i-cecile opened this issue Apr 29, 2021 · 8 comments · Fixed by #14646
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@alice-i-cecile
Copy link
Member

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

When doing more complex work with bundles, users often end up combining two related bundles, overwriting all values set when they add the second bundle.

What solution would you like?

Add try_insert_bundle(bundle: impl Bundle), try_insert::<C: Component>(component: Component) commands.

These act like their non-try versions, but do not overwrite existing components of the same type.

As part of the same PR, include try_insert_resource and try_insert_local_resource for consistency (and related uses).

What alternative(s) have you considered?

Better bundle merging tools.

Make this the default behaviour, and instead change the current behavior to something like force_insert.

Additional context

Handling the results of these commands relates to #2004, and should probably be left as part of that.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Trivial Nice and easy! A great choice to get started with Bevy labels Apr 29, 2021
@alice-i-cecile
Copy link
Member Author

I'm not convinced on the names; bikeshed away.

@minecrawler
Copy link

I'm not sure this is a good solution to the initial problem. Bundles using the same identifiers, in the worst case for different things, sounds like a nightmare (reminds me of Skyrim mods). Bevy should set standards imho. Things like best-practices bundle authors can follow to create bundles which do not collide with other bundles, or create bundles which can enhance other bundles in a clear way.

@alice-i-cecile
Copy link
Member Author

Bundles using the same identifiers, in the worst case for different things

Identifiers will always work on the type level, so I don't think we'll get collisions in the way you're worried about. Rust's name-spacing strategies will work for us already.

Reuse of components between plugins is both inevitable and desirable IMO. Transform is the obvious example: you can easily imagine multiple compatible bundles that want a transform, and I'd prefer to have the ability to toggle the overwriting behavior than needing to manually check or recomposite all of the provided bundles.

@ahmedcharles
Copy link
Contributor

When I originally read this, I thought the issue was that components were getting silently overwritten and therefore, the developer wasn't even aware of the issue. The linked PR and the proposed feature of having a bundle builder api doesn't actually address the question of whether the developer even notices that two bundles insert a conflicting component.

I.e. there are two issues:

  1. Are two conflicting components being inserted for the same entity?
  2. Knowing that two bundles insert conflicting components for the same entity, how does the developer ergonomically end up inserting the right one?

Is there an existing solution for 1? And if not, how are non-experts supposed to know that the problem even exists?

@ahmedcharles
Copy link
Contributor

Note, std::collections::HashMap uses the word insert to name an operation which doesn't change existing data and replace to name an operation which does. Granted, in those cases, the values are returned and perhaps that isn't as appropriate here (for performance reasons) but being consistent with the terminology might be helpful before stability is a bigger consideration.

@alice-i-cecile alice-i-cecile added S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Dec 12, 2021
@alice-i-cecile
Copy link
Member Author

Note, std::collections::HashMap uses the word insert to name an operation which doesn't change existing data and replace to name an operation which does

I like this nomenclature a lot. I personally would like to move towards this.

Overall I think this issue should probably be tackled after #2241.

@CMorrison82z
Copy link

Is this still on the table?

@alice-i-cecile
Copy link
Member Author

Yep: should be straightforward enough, just hasn't been a priority. Feel free to adopt the linked PR or start fresh and ping me for review.

@alice-i-cecile alice-i-cecile added S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Aug 4, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 15, 2024
# 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 <[email protected]>
Co-authored-by: Giacomo Stevanato <[email protected]>
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 C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants