JoJoJet / bevy-trait-query

adds trait queries to the bevy game engine
Apache License 2.0
65 stars 11 forks source link

Bevy 0.13 Update PR #55

Closed RobWalt closed 3 months ago

RobWalt commented 4 months ago

Ready for review ✅

The PR is in a review-ready state now. All tests are green and we get now compile errors.


Irrelevant and superseded by the section above now, just leaving this first version of the description for transparency This is most probably a very naive try to update this crate to bevy 0.13. The main things that I changed were: - bump versions - split the `ReadOnlyWorldQuery` impl into `QueryData` + `ReadOnlyQueryData` impls - add `QueryFilter` impls for `OneAdded` and `OneChanged` - add the missing `get_state` method on the `WorldQuery` impl - the `get_state` change includes adding a new `get` method for `TraitQueryState` which mostly follows the `init` method - adjusting the proc macros to reflect these changes Most of these adjustments are super naive. I tried to mirror existing code as closely as possible (e.g. I consulted the bevy main repo for the trait impls). Most notably: - For starters: At least it compiles :D - The tests all still fail D: - I didn't find any counter part for `update_archetype_component_access` in bevy 0.13 I'm happy to continue working on this update (and also future update) but I'm afraid I need a bit of mentoring as I'm stuck now

Related to #54

RobWalt commented 4 months ago

I tried looking deeper into the failing tests and here's a short analysis of the all1 test.

Here's the relevant test code:

```rust use super::*; use bevy_ecs::prelude::*; use std::fmt::{Debug, Display}; // Required for proc macros. use crate as bevy_trait_query; #[derive(Resource, Default)] pub struct Output(Vec); #[queryable] pub trait Person { fn name(&self) -> &str; fn age(&self) -> u32; fn set_age(&mut self, age: u32); } #[derive(Component)] struct Fem; #[derive(Component)] pub struct Human(String, u32); impl Person for Human { fn name(&self) -> &str { &self.0 } fn age(&self) -> u32 { self.1 } fn set_age(&mut self, age: u32) { self.1 = age; } } #[derive(Component)] pub struct Dolphin(u32); impl Person for Dolphin { fn name(&self) -> &str { "Reginald" } fn age(&self) -> u32 { self.0 } fn set_age(&mut self, age: u32) { self.0 = age; } } #[test] fn all1() { let mut world = World::new(); world.init_resource::(); world .register_component_as::() .register_component_as::(); world.spawn(Human("Henry".to_owned(), 22)); world.spawn((Human("Eliza".to_owned(), 31), Fem, Dolphin(6))); world.spawn((Human("Garbanzo".to_owned(), 17), Fem, Dolphin(17))); world.spawn(Dolphin(27)); let mut schedule = Schedule::default(); schedule.add_systems((print_all_info, (age_up_fem, age_up_not)).chain()); schedule.run(&mut world); schedule.run(&mut world); assert_eq!( world.resource::().0, &[ "All people:", "Henry: 22", "Eliza: 31", "Reginald: 6", "Garbanzo: 17", "Reginald: 17", "Reginald: 27", "", "All people:", "Henry: 23", "Eliza: 32", "Reginald: 7", "Garbanzo: 18", "Reginald: 18", "Reginald: 28", "", ] ); } // Prints the name and age of every `Person`. fn print_all_info(people: Query<&dyn Person>, mut output: ResMut) { output.0.push("All people:".to_string()); for all in &people { for person in all { output .0 .push(format!("{}: {}", person.name(), person.age())); } } output.0.push(Default::default()); } fn age_up_fem(mut q: Query<&mut dyn Person, With>) { for all in &mut q { for mut p in all { let age = p.age(); p.set_age(age + 1); } } } fn age_up_not(mut q: Query<&mut dyn Person, Without>) { for all in &mut q { for mut p in all { let age = p.age(); p.set_age(age + 1); } } } ```

The results of the test are:

thread 'tests::all1' panicked at src/tests.rs:143:5:
assertion `left == right` failed
  left: ["All people:", "Eliza: 31", "Reginald: 6", "Garbanzo: 17", "Reginald: 17", "", "All people:", "Eliza: 32", "Reginald: 7", "Garbanzo: 18", "Reginald: 18", ""]
 right: ["All people:", "Henry: 22", "Eliza: 31", "Reginald: 6", "Garbanzo: 17", "Reginald: 17", "Reginald: 27", "", "All people:", "Henry: 23", "Eliza: 32", "Reginald: 7", "Garbanzo: 18", "Reginald: 18", "Reginald: 28", ""]

If we look closer at the results, it seems like the query only iterates over the entities which have two entities which implement the trait. Actually it seems like it iterates only over entities which have all of the components which implement the trait. Let's quickly verify that. I added:

#[derive(Component)]
pub struct Alien(u32);

impl Person for Alien {
    fn name(&self) -> &str {
        "Goobert"
    }
    fn age(&self) -> u32 {
        self.0
    }
    fn set_age(&mut self, age: u32) {
        self.0 = age;
    }
}

...

    world
        .register_component_as::<dyn Person, Human>()
        .register_component_as::<dyn Person, Alien>()
        .register_component_as::<dyn Person, Dolphin>();

    world.spawn(Human("Henry".to_owned(), 22));
    world.spawn(Alien(222));
    world.spawn((Human("Eliza".to_owned(), 31), Fem, Dolphin(6), Alien(100)));
    world.spawn((Human("Garbanzo".to_owned(), 17), Fem, Dolphin(17)));
    world.spawn(Dolphin(27));

...

And the test result reports

thread 'tests::all1' panicked at src/tests.rs:160:5:
assertion `left == right` failed
  left: ["All people:", "Eliza: 31", "Goobert: 100", "Reginald: 6", "", "All people:", "Eliza: 32", "Goobert: 101", "Reginald: 7", ""]
 right: ["All people:", "Henry: 22", "Eliza: 31", "Reginald: 6", "Garbanzo: 17", "Reginald: 17", "Reginald: 27", "", "All people:", "Henry: 23", "Eliza: 32", "Reginald: 7", "Garbanzo: 18", "Reginald: 18", "Reginald: 28", ""]

So that might be true.

RobWalt commented 4 months ago

Big shoutout and credit to @Azorlogh for helping me out here!

RobWalt commented 4 months ago

@SkiFire13 First: Thanks for the review!

I'm still not quiet sure what the required field even does, but I made the changes anyways. Note, that the tests passed even before the change. Nevertheless they seem to be even greener than before now 😄 and it proved to be pretty useful since it surfaced a bug, fixed in https://github.com/JoJoJet/bevy-trait-query/pull/55/commits/df16eeb537c30554232fbbef7830fb5027f14d18

JoJoJet commented 4 months ago

Thanks for updating this guys :) I'll try to review this on the weekend

brandon-reinhart commented 4 months ago

Need any help with getting this one wrapped up?

allsey87 commented 4 months ago

I have not had a close look at the code, but I have played around with this PR and haven't found any issues so far. I tried both a system-based query and a world query (World::query) which both worked nicely.

RobWalt commented 4 months ago

One question that just popped up for us is:

Previously you could use it in the filter position, now it doesn't work anymore. It might be misleading to have it work in both positions, so maybe another struct WithOne which only implements QueryFilter (similar to OneAdded) might make sense.

Note to self: AtLeastOne is probably the better filter name as it comes closer to what it actually does, I think it's querying for components with exactly one impl, so the name idea was good already

RobWalt commented 3 months ago

FYI: PR seems to be unaffected by 0.13.1 - the tests are still green

brandon-reinhart commented 3 months ago

FWIW, I've been using this PR in my project for a few weeks and it works great.