Skip to content

Fixes #13466, implementing resource_scope_by_id #18527

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Kugel-Blitz
Copy link

Objective

Testing

  • Tested in the same way resource_scope was tested. Used the old precedent to test our new function by pulling the component id first and passing it in.

Showcase

Allows mutable retrieval of resources using component id in a way they are then inserted back into the world. Similar to resource_scope.

let mut world = World::default();

// Insert the resources into the world with initial values
world.insert_resource(MyResource { value: 42 });
world.insert_resource(MyOtherResource { value: 100 });

// Get the component ID for MyResource
let component_id = world
    .components()
    .get_resource_id(TypeId::of::<MyResource>())
    .expect("Failed to get component ID for MyResource");

println!("component_id of MyResource =  {}", component_id.index());

// Use resource_scope_by_id to modify MyResource safely
world.resource_scope_by_id(component_id, |world: &mut World, mut res: Mut<MyResource>| {
    println!("Resource before: {}", res.value);
    res.value += 10;  // Modify the res
    println!("Resource after modification: {}", res.value);

    // Now, let's access another resource inside the closure
    // Get the component ID for MyOtherResource
    let other_component_id = world
           .components()
           .get_resource_id(TypeId::of::<MyOtherResource>())
           .expect("Failed to get component ID for MyOtherResource");

    // Access MyOtherResource using resource_scope_by_id
    world.resource_scope_by_id(other_component_id, |_world: &mut World, mut other_res: Mut<MyResource>| {
        println!("Second Resource before: {}", other_res.value);
        other_res.value += 20;  // Modify the second resource
        println!("Second Resource after modification: {}", other_res.value);
    });
});

Copy link
Contributor

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 X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Feature A new feature, making something new possible labels Mar 25, 2025
///
/// This enables safe simultaneous mutable access to both a resource and the rest of the [`World`].
/// For more complex access patterns, consider using [`SystemState`](crate::system::SystemState).
/// /// # Example
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
/// /// # Example
///
/// # Example

Two points: you had an extra ///, and you were missing a newline before the heading. I suspect these are related!

/// #[derive(Resource)]
/// struct A(u32);
/// #[derive(Component)]
/// struct B(u32);
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
/// struct B(u32);
/// struct B(u32);
///

/// ```
/// use bevy_ecs::prelude::*;
/// use std::any::TypeId;
/// #[derive(Resource)]
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
/// #[derive(Resource)]
///
/// #[derive(Resource)]

/// .get_resource_id(TypeId::of::<A>())
/// .expect("Failed to get component ID for MyResource");
/// world.resource_scope_by_id(component_id, |world: &mut World, mut a: Mut<A>| {
/// let b = world.get_mut::<B>(entity).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

You should assert that you can't access A inside of this scope :)

/// assert_eq!(world.get_resource::<A>().unwrap().0, 2);
/// ```
///
/// See also [`try_resource_scope_by_id`](Self::try_resource_scope_by_id).
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
/// See also [`try_resource_scope_by_id`](Self::try_resource_scope_by_id).
/// See also [`try_resource_scope_by_id`](Self::try_resource_scope_by_id) for a non-panicking variant.

/// assert_eq!(world.get_resource::<A>().unwrap().0, 2);
/// ```
///
/// See also [`try_resource_scope_by_id`](Self::try_resource_scope_by_id).
Copy link
Member

Choose a reason for hiding this comment

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

These docs would benefit from a # Panics section :)

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.

In general, we're trying to move away from implicit panics, and towards explicit error handling. I think that we should collapse this down into one method, which returns an Option here. That added safety + hassle + lower API surface is appropriate for a niche tool like this.

@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 and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 26, 2025
pub fn resource_scope_by_id<R: Resource, U>(
&mut self,
id: ComponentId,
f: impl FnOnce(&mut World, Mut<R>) -> U,
Copy link
Contributor

@notmd notmd Mar 26, 2025

Choose a reason for hiding this comment

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

The callback should accept MutUntyped instead. If you already know the concrete type why would you still use dynamic API?

Copy link

@mhsalem36 mhsalem36 Apr 3, 2025

Choose a reason for hiding this comment

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

Hi @notmd, if we use MutUntyped. How could we read the ptr's value?
let mut value = unsafe { ptr.read::<R>() };

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi sorry I totally forgot this. MutUntyped can construct directly with PtrMut, you don't need to read to concrete type. See https://dev-docs.bevyengine.org/bevy/ecs/change_detection/struct.MutUntyped.html

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 D-Unsafe Touches with unsafe code in some way S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add World::resource_scope_by_id
5 participants