Skip to content

[Merged by Bors] - Implement Clone for Fetches#2641

Closed
msvbg wants to merge 4 commits intobevyengine:mainfrom
msvbg:entity-fetch-clone
Closed

[Merged by Bors] - Implement Clone for Fetches#2641
msvbg wants to merge 4 commits intobevyengine:mainfrom
msvbg:entity-fetch-clone

Conversation

@msvbg
Copy link
Contributor

@msvbg msvbg commented Aug 12, 2021

Objective

This:

use bevy::prelude::*;

fn main() {
    App::new()
    .add_system(test)
    .run();
}

fn test(entities: Query<Entity>) {
    let mut combinations = entities.iter_combinations_mut();
    while let Some([e1, e2]) = combinations.fetch_next() {    
        dbg!(e1);
    }
}

fails with the message "the trait bound bevy::ecs::query::EntityFetch: std::clone::Clone is not satisfied".

Solution

It works after adding the naive clone implementation to EntityFetch. I'm not super familiar with ECS internals, so I'd appreciate input on this.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 12, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior E-Help-Wanted and removed S-Needs-Triage This issue needs to be labelled labels Aug 12, 2021
@alice-i-cecile
Copy link
Member

Since the access in EntityFetch is read-only, this should be safe. I don't think we've had any use for this before, but your example should definitely be supported.

@msvbg
Copy link
Contributor Author

msvbg commented Aug 12, 2021

OptionFetch fails similarly, so I added an implementation for that and for ChangeTrackersFetch for good measure.

@msvbg msvbg changed the title Implement Clone for EntityFetch Implement Clone for Fetches Aug 12, 2021
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.

Nice catch! This is all safe. We could merge this as-is, but theres one place to improve flexibility (see comment).

@cart
Copy link
Member

cart commented Aug 13, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 13, 2021
# Objective

This:

```rust
use bevy::prelude::*;

fn main() {
    App::new()
    .add_system(test)
    .run();
}

fn test(entities: Query<Entity>) {
    let mut combinations = entities.iter_combinations_mut();
    while let Some([e1, e2]) = combinations.fetch_next() {    
        dbg!(e1);
    }
}
```

fails with the message "the trait bound `bevy::ecs::query::EntityFetch: std::clone::Clone` is not satisfied". 


## Solution

It works after adding the naive clone implementation to EntityFetch. I'm not super familiar with ECS internals, so I'd appreciate input on this.
@bors
Copy link
Contributor

bors bot commented Aug 13, 2021

Build failed:

@cart
Copy link
Member

cart commented Aug 14, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 14, 2021
# Objective

This:

```rust
use bevy::prelude::*;

fn main() {
    App::new()
    .add_system(test)
    .run();
}

fn test(entities: Query<Entity>) {
    let mut combinations = entities.iter_combinations_mut();
    while let Some([e1, e2]) = combinations.fetch_next() {    
        dbg!(e1);
    }
}
```

fails with the message "the trait bound `bevy::ecs::query::EntityFetch: std::clone::Clone` is not satisfied". 


## Solution

It works after adding the naive clone implementation to EntityFetch. I'm not super familiar with ECS internals, so I'd appreciate input on this.
@bors bors bot changed the title Implement Clone for Fetches [Merged by Bors] - Implement Clone for Fetches Aug 14, 2021
@bors bors bot closed this Aug 14, 2021
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.

3 participants