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

QueryIter implements ExactSizeIterator, but doesnt provide an exact size_hint #1686

Closed
MinerSebas opened this issue Mar 18, 2021 · 4 comments
Labels
C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times

Comments

@MinerSebas
Copy link
Contributor

Bevy version

cd4c684

What you did

I read the documentation of ExactSizeIterator, and notice that this requirement isn't true:

When implementing an ExactSizeIterator, you must also implement Iterator.
When doing so, the implementation of Iterator::size_hint must return the exact size of the iterator.

In the standard implementation this is checked with an assertion.

Instead of providing an exact size_hint, Bevy overrides that function, which removes the need for that invariant to be true.

While it isn't a problem here, it would still be nice if QueryIter would provide an exact size_hint if it can.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times labels Mar 18, 2021
@mockersf
Copy link
Member

mockersf commented Mar 18, 2021

This may not be possible currently in rust without trait specialisation: rust-lang/rust#31844

size_hint comes from Iterator impl, which is:

impl<'w, 's, Q: WorldQuery, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, F>
where
F::Fetch: FilterFetch,

but ExactSizeIterator impl fixes the F type param to ()

impl<'w, 's, Q: WorldQuery> ExactSizeIterator for QueryIter<'w, 's, Q, ()> {

so without a specialisation of Iterator for QueryIter<'w, 's, Q, ()> we can't have a different size_hint for that case.

@MinerSebas
Copy link
Contributor Author

MinerSebas commented Mar 18, 2021

Playing around I got this almost working:

    fn size_hint(&self) -> (usize, Option<usize>) {
        if TypeId::of::<F>() == TypeId::of::<()>() {
            let temp = self
                .query_state
                .matched_archetypes
                .ones()
                .map(|index| self.world.archetypes[ArchetypeId::new(index)].len())
                .sum();

            return (temp, Some(temp));
        }

        (0, None)
    }

But then F needs to be 'static and naively adding that constraint creates even more demands for 'static.

Edit: Annotating the other locations with 'static allows Bevy to compile (with all tests passing), but I don't know if this is the proper solution.

@cart
Copy link
Member

cart commented Mar 19, 2021

If we can't meet the ExactSizeIter requirements for all cases, maybe we're better of removing ExactSizeIterator in favor of a manual len() impl for unfiltered queries.

bors bot pushed a commit that referenced this issue Mar 19, 2021
This PR overrides the default size_hint for QueryIter.
This is mainly done to provide inline documentation of Issue #1686.
nicopap added a commit to nicopap/bevy that referenced this issue Mar 17, 2022
This fixes bevyengine#1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation.
nicopap added a commit to nicopap/bevy that referenced this issue Mar 17, 2022
This fixes bevyengine#1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation.
nicopap added a commit to nicopap/bevy that referenced this issue Mar 17, 2022
This fixes bevyengine#1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful
  minima when it's possible to predict the exact size of the query.

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation.
bors bot pushed a commit that referenced this issue Apr 26, 2022
## Objective

This fixes #1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

## Solution

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

## Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
bors bot pushed a commit that referenced this issue Apr 26, 2022
## Objective

This fixes #1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

## Solution

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

## Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
bors bot pushed a commit that referenced this issue Apr 26, 2022
## Objective

This fixes #1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

## Solution

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

## Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
bors bot pushed a commit that referenced this issue Apr 26, 2022
## Objective

This fixes #1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

## Solution

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

## Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
nicopap added a commit to nicopap/bevy that referenced this issue Apr 27, 2022
This fixes bevyengine#1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful
  minima when it's possible to predict the exact size of the query.

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation.
bors bot pushed a commit that referenced this issue Apr 27, 2022
## Objective

This fixes #1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

## Solution

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

## Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
exjam pushed a commit to exjam/bevy that referenced this issue May 22, 2022
## Objective

This fixes bevyengine#1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

## Solution

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

## Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
@james7132
Copy link
Member

This was resolved in #4244. It's currently inaccurate for partially iterated iterators, which is documented in #5149 with a fix in #5214. Closing this.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
## Objective

This fixes bevyengine#1686.

`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

## Solution

To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
  this constant tells us when it is safe to assume that the `Fetch`
  relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful

## Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants