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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add 's (state) lifetime to Fetch #2515

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Add 's (state) lifetime to Fetch #2515

wants to merge 2 commits into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Jul 22, 2021

Allows iterators to return things that borrow data from QueryState, needed this in my relations PR figure might be worth landing separately maybe

@github-actions github-actions bot added S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 22, 2021
@bjorn3 bjorn3 added A-ECS Entities, components, systems, and events C-Enhancement A new feature and removed S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 22, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Jul 22, 2021

Right, I think I now understand these changes (and the similar changes for the render rework)

To be clear, this change is only useful for functions which use direct world access, right? I.e. this has no effect for within systems (since they cannot make any assumptions about how long the system's state lasts).

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jul 22, 2021 •
edited

Yea this change is only necessary for when you're working with direct world access as the Query type just uses the same lifetime for both

To be even more clear: nothing in bevy main currently needs this change, only the renderer rework and relations seems to want it afaict

@mockersf
Copy link
Member

From render rework : https://github.com/cart/bevy/blob/6604d473b45c01415a19e765e628e793128a7267/crates/bevy_ecs/src/query/fetch.rs#L48

I would be for merging that now, it would remove some changes from the render rework

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@cart
Copy link
Member

cart commented Jul 29, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jul 29, 2021
@BoxyUwU
Add 's (state) lifetime to Fetch (#2515)
155068a 
Allows iterators to return things that borrow data from `QueryState`, needed this in my relations PR figure might be worth landing separately maybe
@bors
Copy link
Contributor

bors bot commented Jul 29, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add 's (state) lifetime to Fetch [Merged by Bors] - Add 's (state) lifetime to Fetch Jul 29, 2021
@bors bors bot closed this Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@BoxyUwU @DJMcNab @mockersf @cart @bjorn3