-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Compile Time Query checking and System Parameters #17837
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
b374ef0
20f086a
f2d8a31
214e84c
a99b68c
7b1e737
747b881
e9866ad
c81744b
44aa6fb
d00b235
a556045
e1a20c6
7517ac8
47e4414
bd6b324
1cd9270
ad0b84e
148a1f3
5d2cb10
8f798a4
48a92ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| use crate::system::const_param_checking::{ | ||
| AccessTreeContainer, ComponentAccess, ComponentAccessTree, | ||
| }; | ||
| use crate::{ | ||
| archetype::{Archetype, Archetypes}, | ||
| bundle::Bundle, | ||
|
|
@@ -11,6 +14,7 @@ use crate::{ | |
| FilteredEntityMut, FilteredEntityRef, Mut, Ref, World, | ||
| }, | ||
| }; | ||
| use bevy_ecs::system::const_param_checking::{WithFilterTree, WithoutFilterTree}; | ||
| use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref}; | ||
| use core::{cell::UnsafeCell, marker::PhantomData, panic::Location}; | ||
| use smallvec::SmallVec; | ||
|
|
@@ -284,6 +288,12 @@ pub unsafe trait QueryData: WorldQuery { | |
| /// and is visible to the end user when calling e.g. `Query<Self>::get`. | ||
| type Item<'a>; | ||
|
|
||
| const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = ComponentAccessTree { | ||
| this: ComponentAccess::Ignore, | ||
| left: None, | ||
| right: None, | ||
| }; | ||
|
|
||
| /// This function manually implements subtyping for the query items. | ||
| fn shrink<'wlong: 'wshort, 'wshort>(item: Self::Item<'wlong>) -> Self::Item<'wshort>; | ||
|
|
||
|
|
@@ -1203,10 +1213,15 @@ unsafe impl<T: Component> WorldQuery for &T { | |
| } | ||
|
|
||
| /// SAFETY: `Self` is the same as `Self::ReadOnly` | ||
| unsafe impl<T: Component> QueryData for &T { | ||
| unsafe impl<T: Component> QueryData for &T | ||
| where | ||
| for<'a> &'a T: crate::system::const_param_checking::AccessTreeContainer, | ||
| { | ||
| type ReadOnly = Self; | ||
| type Item<'w> = &'w T; | ||
|
|
||
| const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = Self::COMPONENT_ACCESS_TREE; | ||
|
|
||
| fn shrink<'wlong: 'wshort, 'wshort>(item: &'wlong T) -> &'wshort T { | ||
| item | ||
| } | ||
|
|
@@ -1568,10 +1583,15 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { | |
| } | ||
|
|
||
| /// SAFETY: access of `&T` is a subset of `&mut T` | ||
| unsafe impl<'__w, T: Component<Mutability = Mutable>> QueryData for &'__w mut T { | ||
| unsafe impl<'__w, T: Component<Mutability = Mutable>> QueryData for &'__w mut T | ||
| where | ||
| for<'a> &'a mut T: crate::system::const_param_checking::AccessTreeContainer, | ||
| { | ||
| type ReadOnly = &'__w T; | ||
| type Item<'w> = Mut<'w, T>; | ||
|
|
||
| const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = Self::COMPONENT_ACCESS_TREE; | ||
|
|
||
| fn shrink<'wlong: 'wshort, 'wshort>(item: Mut<'wlong, T>) -> Mut<'wshort, T> { | ||
| item | ||
| } | ||
|
|
@@ -1710,7 +1730,10 @@ unsafe impl<'__w, T: Component> WorldQuery for Mut<'__w, T> { | |
| } | ||
|
|
||
| // SAFETY: access of `Ref<T>` is a subset of `Mut<T>` | ||
| unsafe impl<'__w, T: Component<Mutability = Mutable>> QueryData for Mut<'__w, T> { | ||
| unsafe impl<'__w, T: Component<Mutability = Mutable>> QueryData for Mut<'__w, T> | ||
| where | ||
| for<'a> &'a mut T: crate::system::const_param_checking::AccessTreeContainer, | ||
| { | ||
| type ReadOnly = Ref<'__w, T>; | ||
| type Item<'w> = Mut<'w, T>; | ||
|
|
||
|
|
@@ -2052,6 +2075,8 @@ macro_rules! impl_tuple_query_data { | |
| type ReadOnly = ($($name::ReadOnly,)*); | ||
| type Item<'w> = ($($name::Item<'w>,)*); | ||
|
|
||
| const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = impl_tuple_query_data!(@tree $($name),*); | ||
|
|
||
| fn shrink<'wlong: 'wshort, 'wshort>(item: Self::Item<'wlong>) -> Self::Item<'wshort> { | ||
| let ($($name,)*) = item; | ||
| ($( | ||
|
|
@@ -2076,6 +2101,36 @@ macro_rules! impl_tuple_query_data { | |
| unsafe impl<$($name: ReadOnlyQueryData),*> ReadOnlyQueryData for ($($name,)*) {} | ||
|
|
||
| }; | ||
|
|
||
| // Handle empty case | ||
| (@tree) => { | ||
| ComponentAccessTree { | ||
| this: ComponentAccess::Ignore, | ||
| left: None, | ||
| right: None, | ||
| } | ||
| }; | ||
| // Handle single item case | ||
| (@tree $t0:ident) => { | ||
| $t0::COMPONENT_ACCESS_TREE_QUERY_DATA | ||
| }; | ||
| // Handle two item case | ||
| (@tree $t0:ident, $t1:ident) => { | ||
| ComponentAccessTree::combine( | ||
| &$t0::COMPONENT_ACCESS_TREE_QUERY_DATA, | ||
| &$t1::COMPONENT_ACCESS_TREE_QUERY_DATA, | ||
| ) | ||
| }; | ||
| // Handle three or more items case | ||
| (@tree $t0:ident, $t1:ident, $($rest:ident),+) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need separate cases for two and three items? It seems like you could make the two-item case take Alternately, I think you can do this without recursion. It might be simpler as something like const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = const {
let tree = ComponentAccessTree { this: ComponentAccess::Ignore, left: None, right: None };
$(
let tree = ComponentAccessTree::combine(tree, $name);
)*
tree
};
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly it seems like this doesn't work. But if you could elaborate on how to do the two item case instead I would like to switch to that!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see. I was expecting it to do static promotion... but that's only possible if the inner values are themselves
The simplest change is (@tree) => {
&ConstTreeInner::Empty
};
(@tree $t0:ident) => {
$t0::COMPONENT_ACCESS_TREE_QUERY_DATA
};
(@tree $t0:ident, $($rest:ident),+) => {
&ConstTreeInner::<ComponentAccess>::combine(
$t0::COMPONENT_ACCESS_TREE_QUERY_DATA,
impl_tuple_query_data!(@tree $($rest),+)
)
};But you could go one further with // Base case
(@tree) => {
&ConstTreeInner::Empty
};
// Inductive case
(@tree $t0:ident, $($rest:ident),*) => {
&ConstTreeInner::<ComponentAccess>::combine(
$t0::COMPONENT_ACCESS_TREE_QUERY_DATA,
impl_tuple_query_data!(@tree $($rest),*)
)
};for the cost of an extra ... and for that matter, since we're already generating that same code for smaller tuples, you could use their trees rather than doing a second recursion: // Base case
(@tree) => {
&ConstTreeInner::Empty
};
// Inductive case
(@tree $t0:ident, $($rest:ident),*) => {
&ConstTreeInner::<ComponentAccess>::combine(
$t0::COMPONENT_ACCESS_TREE_QUERY_DATA,
<($($rest,)*)>::COMPONENT_ACCESS_TREE_QUERY_DATA
)
};That should generate a little less code. |
||
| ComponentAccessTree::combine( | ||
| &$t0::COMPONENT_ACCESS_TREE_QUERY_DATA, | ||
| &ComponentAccessTree::combine( | ||
| &$t1::COMPONENT_ACCESS_TREE_QUERY_DATA, | ||
| &impl_tuple_query_data!(@tree $($rest),+) | ||
| ) | ||
| ) | ||
| }; | ||
| } | ||
|
|
||
| macro_rules! impl_anytuple_fetch { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make this value deterministic? I think that's preferable from it randomly changing whenever
cargo clean && cargo buildis ran.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Using something like
type_id::<Self>()would be ideal, but you'd need a nightly feature flag to make it const.I guess you could do some sort of hashing that takes for input the component's ast, but that still probably isn't enough to avoid collisions with other components. It would also drastically increase the complexity of this pr.
I agree it'd be preferable, but what benefits are there to making it deterministic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah using type_id:: would be ideal, but we don't want to use any sort of deterministic count because I've heard from the grape vine that rustc might mess with things so we could end up with UB. Unless I have a guarantee that IDs are never going to repeat I would rather safely stick with a random u128.