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

Fix Added behaviour with QueryOne get #543

Merged
merged 1 commit into from
Oct 5, 2020
Merged

Fix Added behaviour with QueryOne get #543

merged 1 commit into from
Oct 5, 2020

Conversation

BorisBoutillier
Copy link
Contributor

Trying to fix #541

Comment on lines 57 to 68
self.world
.get_ref_at_location_unchecked(location)
.map_err(QueryError::ComponentError)
if Q::Fetch::get(
&self.world.archetypes[location.archetype as usize],
location.index,
)
.map_or(true, |f| f.should_skip())
{
Err(QueryError::ComponentError(ComponentError::NoSuchEntity))
} else {
self.world
.get_ref_at_location_unchecked(location)
.map_err(QueryError::ComponentError)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am clearly not confident on this part of PR and would gladly have a thorough review here.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm this has significant performance implicantions because we are now doing a "full lookup" of every component storage in the query. I'm not sure thats worth the tradeoff when we can use "Query One" when this behavior is required.

I'm down to merge the "query one" part of this PR now, but this change will require some serious thought / im pretty sure its not worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see and will update the PR to only include the query_one part.

@karroffel karroffel added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Sep 21, 2020
Query unchanged as impacts performances.
Added tests in bevy_ecs/hecs
@BorisBoutillier BorisBoutillier changed the title Fix Added behaviour with Query/QueryOne get Fix Added behaviour with QueryOne get Oct 3, 2020
@BorisBoutillier BorisBoutillier requested a review from cart October 3, 2020 20:09
@cart cart merged commit 1bdb9d3 into bevyengine:master Oct 5, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
Query unchanged as impacts performances.
Added tests in bevy_ecs/hecs
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.

Added<X> not properly filtered in QueryOne.get() and Query.get() methods
3 participants