Skip to content

Make AccessConflicts::is_empty public #18688

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

Merged
merged 1 commit into from
Apr 28, 2025
Merged

Conversation

angelthorns
Copy link
Contributor

Objective

When implementing SystemParam for an object which contains a mutable
reference to World, which cannot be derived due to a required lifetime parameter, it's necessary to check that there aren't any conflicts.

As far as I know, the is_empty method is the only way provided to check for no conflicts at all

Copy link
Contributor

github-actions bot commented Apr 2, 2025

Welcome, new contributor!

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

@Bleachfuel
Copy link
Contributor

Could u give a example of how you would implement SystemParam for a type like that? I could try to make it derivable.

@angelthorns
Copy link
Contributor Author

pub struct NativeOM<'a> {
    pub world: &'a mut World,
}

unsafe impl SystemParam for NativeOM<'_> {
    type State = ();

    type Item<'world, 'state> = NativeOM<'world>;

    fn init_state(
        _world: &mut World,
        system_meta: &mut bevy_ecs::system::SystemMeta,
    ) -> Self::State {
        let mut access = Access::default();
        access.write_all();

        if !system_meta
            .archetype_component_access()
            .is_compatible(&access)
        {
            panic!("NativeOM borrows &mut World and conflicts with other system parameters");
        }

        unsafe {
            system_meta.archetype_component_access_mut().extend(&access);
        }

        let mut filtered_access = FilteredAccess::default();

        filtered_access.write_all();
        
        if !(match system_meta
            .component_access_set()
            .get_conflicts_single(&filtered_access)
        {
            bevy_ecs::query::AccessConflicts::All => false,
            bevy_ecs::query::AccessConflicts::Individual(fixed_bit_set) => fixed_bit_set.is_empty(),
        }) {
            panic!("NativeOM borrows &mut World and conflicts with other system parameters");
        }

        unsafe {
            system_meta.component_access_set_mut().add(filtered_access);
        }

        ()
    }

    unsafe fn get_param<'world, 'state>(
        _state: &'state mut Self::State,
        _system_meta: &bevy_ecs::system::SystemMeta,
        world: bevy_ecs::world::unsafe_world_cell::UnsafeWorldCell<'world>,
        _change_tick: bevy_ecs::component::Tick,
    ) -> Self::Item<'world, 'state> {
        NativeOM::new(world.world_mut())
    }
}

This is how I implemented it after reading the implementation for &'w World (although for now i'm unaware as to how System trait requirements work for &mut World as there is actually not a SystemParam implementation for it, and haven't looked too deeply with it) so I improvised this a bit.

@angelthorns
Copy link
Contributor Author

if !(match system_meta
            .component_access_set()
            .get_conflicts_single(&filtered_access)
        {
            bevy_ecs::query::AccessConflicts::All => false,
            bevy_ecs::query::AccessConflicts::Individual(fixed_bit_set) => fixed_bit_set.is_empty(),
        })

This part is where i would've used AccessConflicts::is_empty

@chescock
Copy link
Contributor

chescock commented Apr 3, 2025

It's not actually sound to impl SystemParam for something containing &mut World at the moment. We'd need something like #16622 so that we can mark the system as exclusive, and so that we can check for conflicts between &mut World and parameters like &Archetypes and Query<Entity> that have no access but require the structure not change.

If you're able to use DeferredWorld, that does impl SystemParam; otherwise I think you need to use a custom ExclusiveSystemParam.

This PR seems fine, though! It is silly that this method isn't pub when it doesn't do anything you can't already do with the public API.

@IQuick143 IQuick143 added D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Apr 5, 2025
@mockersf mockersf added this pull request to the merge queue Apr 28, 2025
Merged via the queue into bevyengine:main with commit 9fca353 Apr 28, 2025
39 checks passed
splo pushed a commit to splo/bevy that referenced this pull request Apr 29, 2025
# Objective

When implementing `SystemParam` for an object which contains a mutable
reference to World, which cannot be derived due to a required lifetime
parameter, it's necessary to check that there aren't any conflicts.

As far as I know, the is_empty method is the only way provided to check
for no conflicts at all
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
# Objective

When implementing `SystemParam` for an object which contains a mutable
reference to World, which cannot be derived due to a required lifetime
parameter, it's necessary to check that there aren't any conflicts.

As far as I know, the is_empty method is the only way provided to check
for no conflicts at all
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 D-Trivial Nice and easy! A great choice to get started with Bevy 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.

5 participants