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

Adopt a Fetch pattern for SystemParams #1074

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Conversation

cart
Copy link
Member

@cart cart commented Dec 16, 2020

Resolves #1032

Unfortunately the SystemParam approach currently used on master suffers from a safety hole. You can annotate system parameters like Res<'static, X> and suddenly we have a static pointer to a non-static resource! Thats bad!

To resolve this, I have adopted a "Fetch" pattern for SystemParams (the pattern used by WorldQueries).

This accomplishes two things:

  • Makes it impossible to convert a function with static references to a system
  • Removes the need to hack lifetimes inside of SystemParams, making the implementation much safer.

There is one major downside to this approach. We can no longer use this syntax:

app.add_system(movement)

Instead we must go back to doing this:

app.add_system(movement.system())

Yeah yeah ... I'm not happy about it either. But safety comes first. I'm hoping we find a magic implementation with no compromises, but until then something needs to give. And it can't be safety!

I encourage anyone interested to play around with the "minimal" examples in the link above. Maybe you can discover an ideal implementation.

@cart cart added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Dec 16, 2020
@mockersf
Copy link
Member

calling a function on a function (my_system_function.system()) was one of the strange and surprising part of the API.

could we have a macro system!(my_system_function) that would do it for us? It doesn't change anything but I feel it would make the API easier to grasp

@cart
Copy link
Member Author

cart commented Dec 16, 2020

calling a function on a function (my_system_function.system()) was one of the strange and surprising part of the API.

could we have a macro system!(my_system_function) that would do it for us? It doesn't change anything but I feel it would make the API easier to grasp

I'm pretty firmly anti-macro in that case. "no macros in the public ecs api" is one of my favorite parts of bevy, and is one of the things that distinguishes us from the other options out there.

@cart cart merged commit 841755a into bevyengine:master Dec 16, 2020
@J-F-Liu
Copy link
Contributor

J-F-Liu commented Dec 20, 2020

I also like the neat app.add_system(movement), after reading the code still can't understand why this is no longer possible.

@fopsdev fopsdev mentioned this pull request Jan 24, 2021
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to UseAfterFree in master branch
3 participants