Skip to content

Add insert_batch and variations#15702

Merged
alice-i-cecile merged 17 commits intobevyengine:mainfrom
JaySpruce:insert_batch_and_whatnot
Oct 13, 2024
Merged

Add insert_batch and variations#15702
alice-i-cecile merged 17 commits intobevyengine:mainfrom
JaySpruce:insert_batch_and_whatnot

Conversation

@JaySpruce
Copy link
Copy Markdown
Member

@JaySpruce JaySpruce commented Oct 7, 2024 •
edited
Loading

Objective

insert_or_spawn_batch exists, but a version for just inserting doesn't

Solution

Add insert_batch, along with the most common insert variations:

  • World::insert_batch
  • World::insert_batch_if_new
  • World::try_insert_batch
  • World::try_insert_batch_if_new
  • Commands::insert_batch
  • Commands::insert_batch_if_new
  • Commands::try_insert_batch
  • Commands::try_insert_batch_if_new

Testing

Added tests, and added a benchmark for insert_batch.
Performance is slightly better than insert_or_spawn_batch when only inserting:

Code_HPnUN0QeWe

old benchmark

This was before reworking it to remove the UnsafeWorldCell:

Code_QhXJb8sjlJ

Showcase

Usage is the same as insert_or_spawn_batch:

use bevy_ecs::{entity::Entity, world::World, component::Component};
#[derive(Component)]
struct A(&'static str);
#[derive(Component, PartialEq, Debug)]
struct B(f32);

let mut world = World::new();
let entity_a = world.spawn_empty().id();
let entity_b = world.spawn_empty().id();
world.insert_batch([
    (entity_a, (A("a"), B(0.0))),
    (entity_b, (A("b"), B(1.0))),
]);

assert_eq!(world.get::<B>(entity_a), Some(&B(0.0)));

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 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-Performance A change motivated by improving speed, memory usage or compile times C-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 7, 2024
@alice-i-cecile alice-i-cecile mentioned this pull request Oct 7, 2024
Closed
@hymm hymm self-requested a review October 7, 2024 18:42
BD103
BD103 approved these changes Oct 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@BD103 BD103 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! I have some nits and notes, but nothing blocking.

Do note that I'm unfamiliar with ECS internals, so I'd like someone with more experience to check over those parts of the PR.

@BD103 BD103 added D-Unsafe Touches with unsafe code in some way D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 7, 2024
@Trashtalk217
Copy link
Copy Markdown
Contributor

Trashtalk217 commented Oct 7, 2024

This would be hugely helpful for deprecating insert_or_spawn_batch, which is part of #15459. I do however want to point to #15614 as an example of a way to add this functionality without doubling the API surface. Can you look if this may be possible for insert variants as well?

@JaySpruce
Copy link
Copy Markdown
Member Author

JaySpruce commented Oct 7, 2024 •
edited
Loading

I'm far from an expert, but I'm not sure if it's doable in the same way.

The World::entity variations deal with Single vs Plural and Static vs Dynamic, things that work nicely with Rust types.
The insert variations are Replace vs Keep and Any Conditional, which I can only think of ugly solutions for.

I'd imagine that we would want to include all the other insert functions, to keep things consistent. I've seen ideas about chaining commands to achieve a similar nice API, which would work for the insert Commands, but I don't know if the functions on the World can do that.

The best I've got is passing in an enum or something, but that's obvious so there's probably a reason that's not how it works already (too fragile, ugly, etc).

Notably, a lower level insert (on BundleInserter) does use an InsertMode enum to determine whether to replace or keep, it's just not exposed to users

Edit from days later: I think I missed the main part of the question, which is just adding insert and making it optionally function with batches. That's something that might need some higher up approval though

@JaySpruce
Copy link
Copy Markdown
Member Author

JaySpruce commented Oct 8, 2024 •
edited
Loading

Maybe this could be its own PR, but the original PR has some unhealthy usage of UnsafeWorldCell, so I think this is a worthwhile fix.

Here's the context:

The problem was that I needed access to both the World's Entities storage and a &mut World to make a new BundleInserter.

insert_or_spawn_batch snuck around this by making a BundleSpawner first, which internally made an UnsafeWorldCell that was then able to be used to access Entities while still using a &mut World.

insert_batch, of course, doesn't need a BundleSpawner. However, if you try to recreate the effect with a BundleInserter, you'll find that it additionally needs an ArchetypeId, which you get from an EntityLocation... which you get from Entities.

I originally used an UnsafeWorldCell directly to get around this, which worked, but it turns out the documentation specifically says to not do that.

So, eventually I figured out a (hopefully) proper solution: deal with the first entity in the iterator on its own, so you can make a BundleInserter with its ArchetypeId without triggering the borrow checker, and without dealing in unsafe (well, unsafe unsafe) operations.

And as a bonus, it's slightly faster now!
Code_ta6N8YPp6M

hymm
hymm reviewed Oct 10, 2024
View reviewed changes
@JaySpruce
Copy link
Copy Markdown
Member Author

JaySpruce commented Oct 10, 2024

Should I remove the try_ variants from the World? I've realized that they're only really useful for Commands

hymm
hymm reviewed Oct 10, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@hymm hymm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Am I understanding correctly that this could potentially allocate twice if we're moving multiple entities into the same archetype? And also make multiple moves of entities in the old table as we're moving entities out of it? Probably some room for future optimizations if so.

edit: I guess it would depend on how it's being used if it's worth it or not.

@JaySpruce
Copy link
Copy Markdown
Member Author

JaySpruce commented Oct 10, 2024

I think keeping multiple inserters allocated would run into borrow problems, since they all need a &mut World

@hymm
Copy link
Copy Markdown
Contributor

hymm commented Oct 10, 2024

Should I remove the try_ variants from the World? I've realized that they're only really useful for Commands

I'm not sure I follow. We do have a try_despawn on world. I think it's more unlikely that an entity might not exist when working with World, but not impossible. You could be referencing an entity that is manually updated in a resource.

I think keeping multiple inserters allocated would run into borrow problems, since they all need a &mut World

It would definitely require something different from BundleInserter and probably a bunch of unsafe.

@JaySpruce
Copy link
Copy Markdown
Member Author

JaySpruce commented Oct 10, 2024

I think it's more unlikely that an entity might not exist when working with World, but not impossible. You could be referencing an entity that is manually updated in a resource.

Just wasn't sure if it was possible, I'll leave them alone then

@JMS55 JMS55 added this to the 0.15 milestone Oct 13, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 13, 2024
Merged via the queue into bevyengine:main with commit 3d6b248 Oct 13, 2024
@JaySpruce JaySpruce deleted the insert_batch_and_whatnot branch October 14, 2024 02:44
@pablo-lua pablo-lua mentioned this pull request Oct 18, 2024
Open
@mnmaita mnmaita mentioned this pull request Dec 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@hymm hymm hymm left review comments

@BD103 BD103 BD103 approved these changes

@Trashtalk217 Trashtalk217 Trashtalk217 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way 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

Milestone

0.15

Development

Successfully merging this pull request may close these issues.

Add Commands::insert_batch Add Commands::insert_batch for faster component modification

6 participants

@JaySpruce @Trashtalk217 @hymm @BD103 @alice-i-cecile @JMS55