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

Make WorldId a SystemParam #7736

Closed
alice-i-cecile opened this issue Feb 18, 2023 · 2 comments
Closed

Make WorldId a SystemParam #7736

alice-i-cecile opened this issue Feb 18, 2023 · 2 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@alice-i-cecile
Copy link
Member

          While I don't think this is a bad idea, the justification of using `Local<WorldId>` has me set off a bit (akin to how `Query<Option<With<T>>>` does). Could we not make `WorldId` a `SystemParam`?

Originally posted by @james7132 in #7726 (review)

This should be able to draw directly from the implementation in the linked PR: it should return the WorldId of the world that the system is running on.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 18, 2023
@LiamGallagher737
Copy link
Member

I have tested this, and it seems to work in practice, but I have no idea if it's safe or if it's the correct way to do this.

/// SAFETY: No idea
unsafe impl ReadOnlySystemParam for WorldId {}

/// SAFETY: No idea
unsafe impl SystemParam for WorldId {
    type State = WorldId;

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

    fn init_state(world: &mut super::World, _: &mut crate::system::SystemMeta) -> Self::State {
        world.id
    }

    unsafe fn get_param<'world, 'state>(
        _: &'state mut Self::State,
        _: &crate::system::SystemMeta,
        world: &'world super::World,
        _: u32,
    ) -> Self::Item<'world, 'state> {
        world.id
    }
}

@james7132
Copy link
Member

james7132 commented Feb 18, 2023

That looks generally correct to me beyond the safety comments. Something along the lines of read-only access to shared World metadata for ReadOnlySystemParam, and A World's ID is immutable and fetching it from the World does not borrow anything for SystemParam should work, bar some wordsmithing.

Another alternative is to just read it from the System itself, since systems are bound to the World they're initialized on.

@james7132 james7132 added the S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! label Feb 18, 2023
@bors bors bot closed this as completed in 64b0f19 Feb 19, 2023
myreprise1 pushed a commit to myreprise1/bevy that referenced this issue Feb 20, 2023
# Objective

Fixes bevyengine#7736

## Solution

Implement the `SystemParam` trait for `WorldId`

## Changelog

- `WorldId` can now be taken as a system parameter and will return the id of the world the system is running in
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-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants