Support non-archetypal QueryData
#21581
Conversation
chescock
added
C-Feature
| entity: Entity, | ||
| table_row: TableRow, | ||
| ) -> Self::Item<'w, 's>; | ||
| ) -> Option<Self::Item<'w, 's>>; |
There was a problem hiding this comment.
isn't this adding branching into every fetch? I suppose in the common cases it might get optimized out. We should check the assembly and see.
There was a problem hiding this comment.
isn't this adding branching into every fetch? I suppose in the common cases it might get optimized out. We should check the assembly and see.
Yeah, I'm hoping and expecting that the optimizer will remove them after inlining a function that always returns Some, but I haven't actually tested it yet. I'll do some tests with cargo asm and report back!
| let entity = self.entity_iter.next()?; | ||
| // SAFETY: `entity` is passed from `entity_iter` the first time. | ||
| unsafe { self.fetch_next(entity).into() } | ||
| while let Some(entity) = self.entity_iter.next() { |
There was a problem hiding this comment.
If i understand correctly, this while let Some() isn't strictly necessary since the only public way of getting QuerySortedIter is via the various QueryIter.sort() methods, which provide an entity_list containing only items where fetch returns Some(D::Item) (and not None).
However it's still good to have as a precaution for internal code if we manually create a QuerySortedIter?
There was a problem hiding this comment.
No, it's actually necessary here. The sort() methods evaluate the QueryFilter, but they don't evaluate the QueryData. So now that the QueryData can perform filtering, it's possible for this to need to loop.
To run through an example: If you have Query<(&mut A, Parent<&B>), Changed<C>)> and do query.iter().sort::<&A>(), it will internally create a Query<(&A, Entity), Changed<C>>. That means it's already evaluated the Changed<C> filter while creating the entity_list. But it hasn't evaluated the data, so if Parent<&B> filters out an entity then we need a loop here. (And we can't just internally create a Query<(D, L), F>, since for this example that would try to query ((&mut A, Parent<&B>), &A), which has conflicting access.)
| /// [`QueryIter`](crate::query::QueryIter) that contains archetype-level filters. | ||
| /// | ||
| /// The trait must only be implemented for query data where its corresponding [`QueryData::IS_ARCHETYPAL`] is [`prim@true`]. | ||
| pub trait ArchetypeQueryData: QueryData {} |
There was a problem hiding this comment.
This would not be needed if we could use nightly and const_generics, right?
There was a problem hiding this comment.
This would not be needed if we could use nightly and
const_generics, right?
Oh, I don't know! ... Yup, it looks like #![feature(associated_const_equality)] would let us get rid of this, along with ArchetypeFilter and ReadOnlyQueryData!
I think there's also a proposal for #![feature(downcast_trait)] that would go the other direction and let us remove the associated constants in favor of the traits.
|
I ran a benchmark (on laptop): Seems to be within noise. But i don't know how to interpret the results; an LLM said that the Option was indeed optimized away |
|
Previous approval was a mistake, but I don't know how to remove it.. |
Co-authored-by: Periwink <[email protected]>
| let entity = self.entity_iter.next_back()?; | ||
| // SAFETY: `entity` is passed from `entity_iter` the first time. | ||
| unsafe { self.fetch_next(entity).into() } | ||
| while let Some(entity) = self.entity_iter.next_back() { |
There was a problem hiding this comment.
I find these while let loops a bit worrying.
One of the big problems of mutable iter_many iteration is that we cannot produce an iterator out of a while let loop.
Here, I believe this works because the assumption is that Entity is Copy.
But if Item = Entity were to switch over to Item: EntityEquivalent, then we could run into the same lifetime problem here.
Sure, we could make the decision that fetching should only ever involve bare entities, but that feels like a decision we do not yet have the data to make.
6 participants
Objective
For #17647, we want to create a
QueryDatathat can follow a relation and query data from an entity's parent. If the parent does not have the queried data, the child entity should be skipped in the query. However, there is no way to tell from the child's archetype whether the parent will match! So, we need to support non-archetypalQueryData, just as we support non-archetypalQueryFilters forAddedandChanged.That is, if
Query<Parent<&T>>yields&T, and we do:then
querymust yield a row forchild1but not forchild2, even though they have the same archetype.Solution
Change
QueryData::fetchto returnOptionso that entities can be filtered during fetching by returningNone.To support
ExactSizeIterator, introduce anArchetypeQueryDatatrait and anQueryData::IS_ARCHETYPALassociated constant, similar toArchetypeFilterandQueryFilter::IS_ARCHETYPAL. Implement this trait on existingQueryDatatypes. ModifyExactSizeIteratorimplementations to requireD: ArchetypeQueryData, and thesize_hint()methods to return a minimum size of0if!D::IS_ARCHETYPAL.Alternatives
We could do nothing here, and have
Query<Parent<&T>>yieldOption<&T>. That makes the API less convenient, though. Note that if one wants to query forOption, they can use eitherQuery<Option<Parent<&T>>orQuery<Parent<Option<&T>>, depending on whether they want to include entities with no parent.Another option is to re-use the
ArchetypeFiltertrait instead of introducing a new one. There are no places where we want to abstract over both, however, and it would require writing bounds likeD: QueryData + ArchetypeFilter, F: QueryFilter + ArchetypeFilterinstead of simplyD: ArchetypeQueryData, F: ArchetypeFilter.