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 SystemMeta::new public #11155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Dec 30, 2023

Objective

Without public SystemMeta::new it is hard to construct ExclusiveSystemParam.

While technically it is possible (probably accidentally): by calling SystemState::<PhantomData<T>>::new().meta().clone(). So this new function being public does not allow anything was not possible before. Just makes it work naturally and more efficient.

Context. Trying to implement custom systems. For non-exclusive systems, there's convenient SystemState utility.

There's no such utility for ExclusiveFunctionSystem.

Alternatively we can kick SystemState from ExclusiveSystemParam signatures: it is not used in any implementations, and cannot do a lot of useful things. #11163

Solution

Make SystemMeta::new public.

Changelog

  • SystemMeta::new is made public

@matiqo15 matiqo15 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 30, 2023
@mockersf
Copy link
Member

Off topic for this PR, but you opened a few PRs now and never use the template. Could you try to keep the PR template and fill the sections? The Migration Guide is only needed for breaking change PR, but the others are often interesting, and we use the markdown headers in our tooling to help with release notes

@stepancheg
Copy link
Contributor Author

@mockersf these sections add noise to trivial commits IMHO. I tried reformatting this PR, and now it has "SystemMeta::new public" said four times.

But if it is important to use this structure, I don't mind.

@stepancheg stepancheg force-pushed the system-meta-new-public branch from 0a2081a to 6e70eda Compare December 31, 2023 14:57
@mockersf
Copy link
Member

thanks!

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

The reason it isn't pub is to try and make it not possible to remove accesses in SystemParam::init. So the question is whether exposing SystemMeta::new changes the safety requirements of implementing the SystemParam trait. Currently the safety requirements are:

The implementor must ensure the following is true.

* [SystemParam::init_state](https://docs.rs/bevy/latest/bevy/ecs/system/trait.SystemParam.html#tymethod.init_state) correctly registers all [World](https://docs.rs/bevy/latest/bevy/ecs/world/struct.World.html) accesses used by [SystemParam::get_param](https://docs.rs/bevy/latest/bevy/ecs/system/trait.SystemParam.html#tymethod.get_param) with the provided [system_meta](https://docs.rs/bevy/latest/bevy/ecs/system/struct.SystemMeta.html).
* None of the world accesses may conflict with any prior accesses registered on system_meta.

We might want to add that you're not allowed to remove accesses previously registered.
But I don't think exposing SystemMeta::new will make it any harder to implement the trait safely.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants