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

feat: add insert_if_new (#14397) #14646

Merged
merged 16 commits into from
Aug 15, 2024
Merged

Conversation

jpetkau
Copy link
Contributor

@jpetkau jpetkau commented Aug 7, 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.

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.
Copy link
Contributor

github-actions bot commented Aug 7, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 7, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some cleanup to do, but I like the feature. The name is fine, and I'm not super concerned about introducing a branch here: there's already one, we're just adding another arm.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Aug 7, 2024
@jpetkau
Copy link
Contributor Author

jpetkau commented Aug 8, 2024

Apparently I messed something up when fixing conflicts with "Track source location in change detection (#14034)", unit tests are failing now. Looking into it.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. There are a few more things that should be added:

  • EntityWorldMut::try_insert_if_new
  • EntityCommands::try_insert_if_new

Those can easily be done in follow-up though, so you have my approval.

Copy link
Contributor

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me after CI is fixed.

crates/bevy_ecs/src/bundle.rs Show resolved Hide resolved
crates/bevy_ecs/src/storage/blob_vec.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/table.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
@SIGSTACKFAULT
Copy link
Contributor

Could you add an example which demonstrates it? not a blocker.

@jpetkau
Copy link
Contributor Author

jpetkau commented Aug 8, 2024

@alice-i-cecile currently I have the on_insert trigger not running if you call insert_if_new and the component already exists. (The logic being that this is a no-op, the component didn't change so nothing should happen). Does this seems like the right behavior?

@NEON725
Copy link

NEON725 commented Aug 8, 2024

Wish I'd known about this before trying to adopt #9231. Regardless, that PR should probably be closed now as this PR deprecates it.

@alice-i-cecile alice-i-cecile removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Aug 9, 2024
@alice-i-cecile
Copy link
Member

@alice-i-cecile currently I have the on_insert trigger not running if you call insert_if_new and the component already exists. (The logic being that this is a no-op, the component didn't change so nothing should happen). Does this seems like the right behavior?

This is correct.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 9, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Aug 12, 2024
@alice-i-cecile
Copy link
Member

Sorry, I swear I tried to merge this! We'll see if my merge conflict resolution worked out.

@alice-i-cecile
Copy link
Member

@jpetkau once CI is green please ping me and I'll get this in.

auto-merge was automatically disabled August 15, 2024 20:14

Head branch was pushed to by a user without write access

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 15, 2024
@jpetkau
Copy link
Contributor Author

jpetkau commented Aug 15, 2024

@jpetkau once CI is green please ping me and I'll get this in.

@alice-i-cecile Ok it's green again

Merged via the queue into bevyengine:main with commit b2529bf Aug 15, 2024
26 checks passed
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1675 if you'd like to help out.

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-overwriting insert commands
5 participants