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

Resource change tracking #388

Merged
merged 7 commits into from
Sep 10, 2020
Merged

Resource change tracking #388

merged 7 commits into from
Sep 10, 2020

Conversation

BimDav
Copy link
Contributor

@BimDav BimDav commented Aug 29, 2020

Add mutated tracker on resources and ChangedRes query for added or mutated resources.

#62

The ChangedRes<T: Resource> query yields Option<&T>, with None if the resource has not been added or mutated.

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Aug 30, 2020
@BimDav
Copy link
Contributor Author

BimDav commented Aug 31, 2020

It may be more user friendly that FetchResource::get() always return an Option, that ChangedRes returns Some only if the resource has changed, and that in into_system the systems are called only if the resources are all Some.
An Or<T: ResourceQuery> would also have to be implemented.

@cart
Copy link
Member

cart commented Sep 8, 2020

I'm taking a look at this now. Thanks for your patience!

@cart
Copy link
Member

cart commented Sep 8, 2020

I accidentally submitted the review too early (im trying out the VSCode pull request extension). I'd say don't make any of the suggested changes yet, I'm still forming opinions. I'll respond here when im finished.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

It may be more user friendly that FetchResource::get() always return an Option, that ChangedRes returns Some only if the resource has changed, and that in into_system the systems are called only if the resources are all Some.

Returning an Option does seem like a cleaner/more user friendly approach. It looks like you implemented that and reverted it. What motivated that?

crates/bevy_ecs/src/resource/resource_query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/resource/resources.rs Outdated Show resolved Hide resolved
@BimDav
Copy link
Contributor Author

BimDav commented Sep 10, 2020

Returning an Option does seem like a cleaner/more user friendly approach. It looks like you implemented that and reverted it. What motivated that?

To implement OrRes, I had to be able to get data even in the None case. But it is better now by separating get() into get() and is_some().

@cart
Copy link
Member

cart commented Sep 10, 2020

Yeah that seems reasonable to me. For now, this seems good. In general Option vs is_some is abstracted out from user code, so I'm cool with using this for now / seeing how people feel about it.

@cart cart merged commit 4ef18e2 into bevyengine:master Sep 10, 2020
@cart cart mentioned this pull request Sep 10, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
* Add mutated tracker on resources and ChangedRes query for added or mutated resources.

* ResMut:::new() now takes a reference to a 'mutated' flag in its archetype.

* Change FetchResource so that get() returns an Option. Systems using Resources will only be called if all fetched Resources are Some(). This is done to implement ChangedRes, which is Some iff the Resource has been changed.

* Add OrRes for a logical or in tuples of Resource queries.

* Separate resource query get() in is_some() and get() methods for clarity

* Remove unneeded unsafe

* Change ResMut::new()
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants