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

Clone and IntoIter support for queries #1631

Closed
alice-i-cecile opened this issue Mar 12, 2021 · 10 comments
Closed

Clone and IntoIter support for queries #1631

alice-i-cecile opened this issue Mar 12, 2021 · 10 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Itertools has a great number of fantastic, ergonomic ways to work with iterators, like cartesian product, combinations etc that would be regularly used in games. I would love to be able to use this with my queries, but I can't due to lacking trait impls.

What solution would you like?

impl Clone, IntoIter and any other necessary traits on both Query and QueryIter.

What alternative(s) have you considered?

Write replacements for these methods by hand, either in the engine itself or in each game's code.

Additional context

One of the features of itertools (exactly_one) was reinvented in #1263 already.

Here's a nice code snippet that I would like to be able to compile showing the building block methods:

   use bevy::prelude::*;
   
    struct Foo;
    fn my_system(query: Query<&Foo>) {
        query.clone();
        query.into_iter();
        query.iter();
        query.iter().clone();
    }
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 12, 2021
@alice-i-cecile
Copy link
Member Author

Two more interesting use cases:

  1. Reduce boilerplate by enabling the for e in query: sugar
  2. Free ordered iteration ala Ordered iteration over Queries #1470.

@alice-i-cecile
Copy link
Member Author

Followup conversation with @cart and @TheRawMeatball on this topic. Conclusions:

  • IntoIter isn't the only blocker on using iter_tools: we would also need Clone, which isn't feasible for mutable queries
  • iter_tools like functionality is very much desired because it's currently very frustrating to work with more than one entity at once in queries, like for collisions
  • IntoIter support is desirable anyways
  • IntoIter should resolve to iter_mut, because it's supported for both mutable and immutable queries

@Esteth
Copy link

Esteth commented Apr 8, 2021

I'm a relative rust noob, but will this require Generic Associated Types?

AFAICT you want a signature like this:

impl<'w, 's, Q: WorldQuery, F: WorldQuery> IntoIterator for QueryState<Q, F> {
    type Item = <Q::Fetch as Fetch<'w>>::Item;
    type IntoIter = QueryIter<'w, 's, Q, F>; 

    fn into_iter(self) -> Self::IntoIter {

That leaves w and s unconstrained.

@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Dec 12, 2021
@alice-i-cecile
Copy link
Member Author

Related to #2042, which would provide building blocks for iter_tools-like functionality.

@alice-i-cecile
Copy link
Member Author

Closing in favor of directly implementing these methods as use cases come up: the Clone bound is unworkable.

@Mathspy
Copy link

Mathspy commented Dec 14, 2021

Is the Clone bound unworkable even on QueryIter?

@alice-i-cecile
Copy link
Member Author

Is the Clone bound unworkable even on QueryIter?

No, this should be quite simple to add and probably reasonably useful. If you'd like it, feel free to make a simple PR for it :)

@TheRawMeatball
Copy link
Member

It'd only be valid if the query in question is read only.

@Mathspy
Copy link

Mathspy commented Dec 14, 2021

No, this should be quite simple to add and probably reasonably useful. If you'd like it, feel free to make a simple PR for it :)

Gotcha! I’ll definitely PR it if I find a usecase where I need Cartesian product where one of the two sides is immutable, since Itertools Cartesian product requires only one of the two iterators to be Clone

It'd only be valid if the query in question is read only.

This is in regards to acquiring a QueryIter from a mutable query via .iter()? “Read-only” here being immutable query? Or does that term mean something else?

@TheRawMeatball
Copy link
Member

yes, read only as in an immutable query.

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

4 participants