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

HList and Plucker for components #388

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Defman
Copy link
Member

@Defman Defman commented Mar 7, 2021

pub trait SendMessage<'a, Index> {
    fn send_message(&'a self, message: &str);
}

impl<'a, T, Index> SendMessage<'a, Index> for (&'a Entity, T)
where
    T: TupleRef<'a>,
    <T as TupleRef<'a>>::HList: PluckerRef<Player, Index>,
{
    fn send_message(&'a self, _message: &str) {
        let _player: &Player = PluckerRef::<Player, Index>::pluck(&foo);
    }
}

for (entity, (player,)) in game.query::<(&Player,)>() {
      (&entity, (&player,)).send_message("Hello world!");
      (&entity, (&player,)).send_message("Hello world!");
}

It's still missing support for mutable references and in addition it might also be possible to change the signature a such that &(Entity, (Player,)) also works.

@Defman Defman requested a review from caelunshun March 7, 2021 11:18
@Defman Defman marked this pull request as draft March 7, 2021 11:18
@Defman
Copy link
Member Author

Defman commented Mar 7, 2021

My implementation can most likely be made more generic. We can maybe get away with not having TupleRef and only Tuple.

<T as Tuple>::HList: PluckerRef<Player, Index>,
{
fn send_message(&self, _message: &str) {
// let _player: &Player = PluckerRef::<Player, Index>::pluck(self.1.hlist());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not what the lifetimes should be here, since right now Tuple is only implemented for the owned T and not &T.

@caelunshun
Copy link
Member

caelunshun commented Mar 8, 2021

I'm still concerned about the complexity this introduces for Quill users and developers. It's not intuitive to call methods on tuples, especially when the tuples require trailing commas, and forgetting to insert that comma gives an unhelpful compile error:

error[E0599]: no method named `send_message` found for tuple `(&Entity, &quill::entities::Player)` in the current scope
  --> quill/example-plugins/query-entities/src/lib.rs:56:28
   |
56 |         (&entity, &player).send_message("foo");
   |                            ^^^^^^^^^^^^ method not found in `(&Entity, &quill::entities::Player)`
   |
   = note: the method `send_message` exists but the following trait bounds were not satisfied:
           `&quill::entities::Player: Tuple`
           which is required by `(&Entity, &quill::entities::Player): SendMessage<_>`

Also, Feather developers wanting to add new functionality need to figure out the HList system, which is difficult to understand unless you come from a Haskell background.

A solution similar to the current state of this PR can work, but in my opinion, we need better ergonomics and less type-system magic.

@Defman
Copy link
Member Author

Defman commented Mar 8, 2021

My intentions are not for the user to write (&entity, (&component,)) instead

let query = world.query::<(Player, Position)>();
for player in query.iter() {
  player.send_message("");
}
// Or when it becomes stable
for player @ (entity, (_, Position)) in query.iter() {
  player.send_message("");
}

Also this is more of an experiment to see what can be done, this is far from final and I'm open for suggestions.

@Defman
Copy link
Member Author

Defman commented Mar 8, 2021

An alternative approach would be make query! macro which generates a QueryResult struct and implement AsRef and AsMut depending on access level.

#[macro_export]
macro_rules! query {
    {$(
        $ident:ident: $type:ty
    ),* $(,)?} => {{
        struct QueryResult {
            $($ident: $type),*
        }

        $(
            impl ::std::convert::AsRef<$type> for QueryResult {
                fn as_ref(&self) -> &$type {
                    &self.$ident
                }
            }
        )*

        $(
            impl ::std::convert::AsMut<$type> for QueryResult {
                fn as_mut(&mut self) -> &mut $type {
                    &mut self.$ident
                }
            }
        )*

        struct Query;

        impl Query {
            #[allow(dead_code)]
            fn iter(&self, game: &mut ::quill::Game) -> impl ::std::iter::Iterator<Item = QueryResult> {
                game.query::<($(&$type,)*)>().map(|(_, ($($ident),*))| QueryResult {
                    $($ident),*
                })
            }
        }

        Query
    }};
}

I kind of like this approach and using it would look something like

pub trait SendMessage {
    fn send_message(&self, message: &str);
}

impl<T> SendMessage for T
where
    T: AsRef<Player>
{
    fn send_message(&self, _message: &str) {
        ...
    }
}

let query = query! {
    player: Player,
    position: Position,
};

for entity in query.iter(game) {
    entity.send_message("test");
    entity.position.x += 10;
}

I can expand this to deal with lifetimes, when support is added.
Also mutable references are going to be **** since extracting each field would require a mutable lock reference to the entire T.

@Schuwi
Copy link
Member

Schuwi commented Mar 12, 2021

My intentions are not for the user to write (&entity, (&component,)) instead

let query = world.query::<(Player, Position)>();
for player in query.iter() {
  player.send_message("");
}
// Or when it becomes stable
for player @ (entity, (_, Position)) in query.iter() {
  player.send_message("");
}

Also this is more of an experiment to see what can be done, this is far from final and I'm open for suggestions.

The tracking issue for the relevant bindings_after_at feature for reference: rust-lang/rust#65490
It seems to be close to stabilisation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants