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

Upstream reflect_query #8816

Closed
wants to merge 1 commit into from
Closed

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jun 11, 2023

Read the cuicui_reflect_query README for motivation.

https://crates.io/crates/cuicui_reflect_query

Objective

Bevy's ReflectComponent allows extracting a value from a EntityRef or
EntityMut, this can be useful, but generally not enough.

ReflectComponent is missing ways to query for the reflected thing.
Without this ability, you would be stuck iterating over all EntityRef and
checking if it contains the component in question
. This is O(n) (where n is
the total count of entities) while querying just for a single entity is O(1).

Solution

  1. Split the reflect.rs module in bevy_ecs into smaller parts.

  2. Add the following methods to ReflectComponent:

  • reflect_ref: This is similar to ReflectComponent::reflect, but also includes
    change tick data. This allows reading change ticks on the reflected component
    in an immutable way.

    Furthermore, the reflect_mut method has lifetime limitations, this might be a good
    way to work around them.
  • query{,_entities,_ref,_mut}: Iterate over all entities with the reflected component.
    Like with world.query, you need to call .query(world) on the return value
    to iterate over the query results.
  • get_single{,_entity,_ref,_mut}: similar to world.get_single, it will return
    a value only if there is exactly one entity containing the reflected component.

A bit of precision:

  • The _entity variants return an Entity or an iterator of entities instead
    of the dyn Reflect version of the component.
  • The _ref variants return a Ref<_> over _, this let you read change information
    in an immutable maner.
// In ReflectComponentFns
    pub reflect_ref: fn(EntityRef) -> Option<Ref<dyn Reflect>>,

    pub get_single: fn(&mut World) -> SingleResult<&dyn Reflect>,
    pub get_single_entity: fn(&mut World) -> SingleResult<Entity>,
    pub get_single_ref: fn(&mut World) -> SingleResult<Ref<dyn Reflect>>,
    pub get_single_mut: fn(&mut World) -> SingleResult<Mut<dyn Reflect>>,

    pub query: fn(&mut World) -> Querydyn,
    pub query_entities: fn(&mut World) -> EntityQuerydyn,
    pub query_ref: fn(&mut World) -> RefQuerydyn,
    pub query_mut: fn(&mut World) -> MutQuerydyn,
}

Similar work


Changelog

  • Added methods on ReflectComponent to allow querying from a World the components in question:
    • reflect_ref: fn(EntityRef) -> Option<Ref<dyn Reflect>>,
    • get_single: fn(&mut World) -> SingleResult<&dyn Reflect>,
    • get_single_entity: fn(&mut World) -> SingleResult<Entity>,
    • get_single_ref: fn(&mut World) -> SingleResult<Ref<dyn Reflect>>,
    • get_single_mut: fn(&mut World) -> SingleResult<Mut<dyn Reflect>>,
    • query: fn(&mut World) -> Querydyn,
    • query_entities: fn(&mut World) -> EntityQuerydyn,
    • query_ref: fn(&mut World) -> RefQuerydyn,
    • query_mut: fn(&mut World) -> MutQuerydyn,
  • Added EntityRef::get_ref to get a value + change tick.
  • Updated Ref::map method to accept unsized Us

Read the cuicui_reflect_query README for motivation.

<https://crates.io/crates/cuicui_reflect_query>
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Reflection Runtime information about types labels Jun 11, 2023
@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Jun 11, 2023
/// [`map_all_entities`](Self::map_all_entities), this is applied to specific entities, not all values
/// in the [`EntityMap`].
///
/// This is useful mostly for when you need to be careful not to update components that already contain valid entity
Copy link
Member

Choose a reason for hiding this comment

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

It sounds like there could be a map_absent_entities method built in to abstract over this pattern.

@@ -0,0 +1,169 @@
//! Define wrapper types & traits for bevy's `QueryState` and `QueryIter`.
Copy link
Member

Choose a reason for hiding this comment

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

Doc links please.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I like the split, docs are excellent. I think this makes more sense upstream in Bevy than as a third-party crate. This is effectively the last missing piece to making dynamic components work properly.

Macro-generated docs are a bit controversial, but I'm fine with them here.

I'd like to see:

  • tests
  • an end user example that demonstrates a semi-realistic use of this for teaching and API evaluation purposes
  • expanded module level docs to explain what reflection is and why users might actually care

@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Jun 11, 2023
@nicopap
Copy link
Contributor Author

nicopap commented Jun 11, 2023

yeah FYI I'm not sure this deserves being merged. I won't spend much time on this unless there is a clear interest from various people.

I think a solution similar to #6390 is closer to what we want. There is no reasons we can't have a QueryState with dynamic values.

I'll probably open a PR to split the reflection.rs file though, it's kind of a nice code cleanup.

@alice-i-cecile
Copy link
Member

I'll probably open a PR to split the reflection.rs file though, it's kind of a nice code cleanup.

I agree, this should be done regardless and is best suited to a quick merge.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 11, 2023
@MrGVSV MrGVSV self-requested a review June 12, 2023 00:44
@nicopap nicopap marked this pull request as draft June 19, 2023 15:42
@nicopap
Copy link
Contributor Author

nicopap commented Sep 14, 2023

Superseded by #9774

@nicopap nicopap closed this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants