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

Add World::iter_resources() and World::iter_resources_mut() methods #12019

Closed
wants to merge 5 commits into from

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Feb 21, 2024

Objective

  • I would like to be able to easily iterate over all the resources in a given World;
  • As I'm doing so, I'd like be able to query ComponentInfo (e.g. for reflection) and also read (and optionally mutate) the resources directly.

Solution

  • Introduce World::iter_resources() and World::iter_resources_mut() methods;
  • These methods return impl Iterator<Item = (&ComponentInfo, Ptr<'_>)> and impl Iterator<Item = (&ComponentInfo, MutUntyped<'_>)>, respectively.

Note: My original implementation in the remote protocol branch also added methods for !Send resources, but as suggested by WrongShoe on discord, I'm holding off on that since #9122 is likely to land soon.

Note 2: Related to #4955 (But much smaller/more constrained, this requires World access and unsafe)


Changelog

  • Added World::iter_resources() and World::iter_resources_mut() methods for iterating all resources dynamically at run time;

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events A-Scenes Serialized ECS data stored on the disk A-Networking Sending data between clients, servers and machines A-Editor Graphical tools to make Bevy games labels Feb 21, 2024
@alice-i-cecile
Copy link
Member

@james-j-obrien, can I get your review / help with this? It very much feels like a sibling of the dynamic query work.

@coreh
Copy link
Contributor Author

coreh commented Feb 21, 2024

@james-j-obrien Thanks! That makes me more confident with what I was doing. Updated the safety comments with the explanations.

Edit: Added you as a co-author, if that's okay, since I really wouldn't have been able to verify this on my own (or even be able to fully reason about the safety of this without your explanations.)

Co-authored-by: James O'Brien <james.obrien@drafly.net>
Copy link
Contributor

@doonv doonv left a comment

Choose a reason for hiding this comment

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

This code looks good, but I have a question:

It was already possible possible to get all resources from a World in Bevy, just more complicated:

  1. Get the AppTypeRegistry resource
  2. Get a RwLockReadGuard<TypeRegistry> from AppTypeRegistry::read
  3. Filter the registrations by if they are a resource in the World
 registry_lock
            .iter()
            .filter(|registration| {
                world
                    .components()
                    .get_resource_id(registration.type_id())
                    .is_some()
            })
  1. You now have an iterator of &TypeRegistrations, which you can use to get references to the resources.

Aside from being simpler, how does this differ from the above solution?

@coreh
Copy link
Contributor Author

coreh commented Feb 22, 2024

Hmm honestly I wasn't aware you could do that, TIL.

If I understand it correctly, besides being more easily discoverable and somewhat shorter to use, the API in this PR has the benefits of:

  • Not requiring the filtering (so it should be faster, depending on how many non-resource components are in the world. Not sure how relevant that is in practice for real scenarios)
  • It allows you to easily mutate the resources as you iterate them, which would be cumbersome otherwise, since the filter closure needs a read reference to the world

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

This change currently assumes that Resources::iter only returns present resources, when in practice it returns all resources ever initialized within the World, some of which may have been removed. The implementation here can be significantly simpler and not involve panics.

@@ -2796,4 +2987,63 @@ mod tests {
let mut world = World::new();
world.spawn(());
}

#[test]
fn iterate_resources() {
Copy link
Member

Choose a reason for hiding this comment

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

These need a test case to ensure that iteration doesn't panic if you remove a resource. Also for miri.

}

#[test]
fn iterate_resources_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

These need a test case to ensure that iteration doesn't panic if you remove a resource. Also for miri.

Comment on lines +2270 to +2280
self.storages.resources.iter().map(|(component_id, data)| {
let component_info = self.components.get_info(component_id).unwrap_or_else(|| {
panic!("ComponentInfo should exist for all resources in the world, but it does not for {:?}", component_id);
});
let ptr = data.get_data().unwrap_or_else(|| {
panic!(
"When iterating all resources, resource of type {} was supposed to exist, but did not.",
component_info.name()
)
});
(component_info, ptr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.storages.resources.iter().map(|(component_id, data)| {
let component_info = self.components.get_info(component_id).unwrap_or_else(|| {
panic!("ComponentInfo should exist for all resources in the world, but it does not for {:?}", component_id);
});
let ptr = data.get_data().unwrap_or_else(|| {
panic!(
"When iterating all resources, resource of type {} was supposed to exist, but did not.",
component_info.name()
)
});
(component_info, ptr)
self.storages.resources.iter().filter_map(|(component_id, data)| {
Some((component_id, data.get_ptr()?)

Comment on lines +2345 to +2372
self.storages.resources.iter().map(|(component_id, data)| {
let component_info = self.components.get_info(component_id).unwrap_or_else(|| {
panic!("ComponentInfo should exist for all resources in the world, but it does not for {:?}", component_id);
});

let (ptr, ticks) = data.get_with_ticks().unwrap_or_else(|| {
panic!(
"When iterating all resources, resource of type {} was supposed to exist, but did not.",
component_info.name()
)
});

// SAFETY:
// - We have exclusive access to the world, so no other code can be aliasing the `TickCells`
// - We only hold one `TicksMut` at a time, and we let go of it before getting the next one
let ticks = unsafe {
TicksMut::from_tick_cells(ticks, self.last_change_tick(), self.read_change_tick())
};

let mut_untyped = MutUntyped {
// SAFETY:
// - We have exclusive access to the world, so no other code can be aliasing the `Ptr`
// - We iterate one resource at a time, and we let go of each `PtrMut` before getting the next one
value: unsafe { ptr.assert_unique() },
ticks,
};

(component_info, mut_untyped)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.storages.resources.iter().map(|(component_id, data)| {
let component_info = self.components.get_info(component_id).unwrap_or_else(|| {
panic!("ComponentInfo should exist for all resources in the world, but it does not for {:?}", component_id);
});
let (ptr, ticks) = data.get_with_ticks().unwrap_or_else(|| {
panic!(
"When iterating all resources, resource of type {} was supposed to exist, but did not.",
component_info.name()
)
});
// SAFETY:
// - We have exclusive access to the world, so no other code can be aliasing the `TickCells`
// - We only hold one `TicksMut` at a time, and we let go of it before getting the next one
let ticks = unsafe {
TicksMut::from_tick_cells(ticks, self.last_change_tick(), self.read_change_tick())
};
let mut_untyped = MutUntyped {
// SAFETY:
// - We have exclusive access to the world, so no other code can be aliasing the `Ptr`
// - We iterate one resource at a time, and we let go of each `PtrMut` before getting the next one
value: unsafe { ptr.assert_unique() },
ticks,
};
(component_info, mut_untyped)
self.storages.resources.iter().filter_map(|(component_id, data)| {
let (ptr, ticks) = data.get_with_ticks()?;
// SAFETY:
// - We have exclusive access to the world, so no other code can be aliasing the `TickCells`
// - We only hold one `TicksMut` at a time, and we let go of it before getting the next one
let ticks = unsafe {
TicksMut::from_tick_cells(ticks, self.last_change_tick(), self.read_change_tick())
};
let mut_untyped = MutUntyped {
// SAFETY:
// - We have exclusive access to the world, so no other code can be aliasing the `Ptr`
// - We iterate one resource at a time, and we let go of each `PtrMut` before getting the next one
value: unsafe { ptr.assert_unique() },
ticks,
};
Some((component_info, mut_untyped))

github-merge-queue bot pushed a commit that referenced this pull request Apr 3, 2024
# Objective

- Closes #12019
- Related to #4955
- Useful for dev_tools and networking

## Solution

- Create `World::iter_resources()` and `World::iter_resources_mut()`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: James Liu <contact@jamessliu.com>
Co-authored-by: Pablo Reinhardt <126117294+pablo-lua@users.noreply.github.com>
chompaa pushed a commit to chompaa/bevy that referenced this pull request Apr 5, 2024
# Objective

- Closes bevyengine#12019
- Related to bevyengine#4955
- Useful for dev_tools and networking

## Solution

- Create `World::iter_resources()` and `World::iter_resources_mut()`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: James Liu <contact@jamessliu.com>
Co-authored-by: Pablo Reinhardt <126117294+pablo-lua@users.noreply.github.com>
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-Editor Graphical tools to make Bevy games A-Networking Sending data between clients, servers and machines A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants