Look over some old FIXMEs

BoxyUwU · BoxyUwU · commit bb0dcae09d2f · 2021-04-15T07:35:49.000+01:00
diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs
@@ -704,9 +704,6 @@ impl<'w, 's, T: Component> Iterator for RelationAccess<'w, 's, T> {
                 },
                 Either::U(target_iter) => {
                     let target = target_iter.next()?;
-                    // FIXME(Relationships) do we want `T None` to be yielded from a `Relation<T>` query
-                    // honestly there are a lot of places we need to think about this, for example the
-                    // add_relation_filter method takes `Entity` not `Option<Entity>` lol
                     match columns.1.get(target) {
                         Some(col) => unsafe {
                             let ptr = col.get_unchecked(*current_idx) as *mut T;
@@ -722,9 +719,6 @@ impl<'w, 's, T: Component> Iterator for RelationAccess<'w, 's, T> {
                 iter,
                 ..
             } => {
-                // FIXME(Relationships) do we want `T None` to be yielded from a `Relation<T>` query
-                // honestly there are a lot of places we need to think about this, for example the
-                // add_relation_filter method takes `Entity` not `Option<Entity>` lol
                 let target = match iter {
                     Either::T(target_iter) => target_iter.next()?,
                     Either::U(target_iter) => Some(*target_iter.next()?),
@@ -992,9 +986,6 @@ impl<'w, 's, T: Component> Iterator for RelationAccessMut<'w, 's, T> {
                 },
                 Either::U(target_iter) => {
                     let target = target_iter.next()?;
-                    // FIXME(Relationships) do we want `T None` to be yielded from a `Relation<T>` query
-                    // honestly there are a lot of places we need to think about this, for example the
-                    // add_relation_filter method takes `Entity` not `Option<Entity>` lol
                     match columns.1.get(target) {
                         Some(col) => unsafe {
                             let ptr = col.get_unchecked(*current_idx) as *mut T;
@@ -1022,9 +1013,6 @@ impl<'w, 's, T: Component> Iterator for RelationAccessMut<'w, 's, T> {
                 change_tick,
                 ..
             } => {
-                // FIXME(Relationships) do we want `T None` to be yielded from a `Relation<T>` query
-                // honestly there are a lot of places we need to think about this, for example the
-                // add_relation_filter method takes `Entity` not `Option<Entity>` lol
                 let target = match iter {
                     Either::T(target_iter) => target_iter.next()?,
                     Either::U(target_iter) => Some(*target_iter.next()?),
diff --git a/crates/bevy_ecs/src/query/relation_filter.rs b/crates/bevy_ecs/src/query/relation_filter.rs
@@ -59,8 +59,12 @@ impl<Q: WorldQuery, F: WorldQuery> QueryRelationFilter<Q, F> {
         Self::default()
     }
 
-    // FIXME(Relationships) should the behaviour for this be target AND other_target AND other_other_target
-    // or should it be target OR other_target OR other_other_target
+    // FIXME(Relationships) currently we cant set a target filter of `None`- rather than allowing this
+    // we should stop storing `target` as an `Option<Entity>` and instead have a statically knowable
+    // ID represent the `None` case
+    // FIXME(Relationships) implement ability to say `target1` OR `target2`, currently it works like
+    // `target1` AND `target2`. This mirrors how queries normally work `Query<(&T, &U)>`- `T` AND `U`
+    // but we need to also be able to mirror `Or<(T, U)>`
     pub fn add_target_filter<T: Component, Path>(mut self, target: Entity) -> Self
     where
         Self: SpecifiesRelation<T, Path, RelationFilter = Self>,
diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs
@@ -393,7 +393,6 @@ mod tests {
                     .get_id(std::any::TypeId::of::<(i32, bool)>())
                     .expect("Bundle used to spawn entity should exist");
                 let bundle_info = bundles.get(bundle_id).unwrap();
-                // FIXME(Relationships) make sure this is using `(RelationKindId, Option<Entity>)`
                 let mut bundle_components = bundle_info.components().to_vec();
                 bundle_components.sort();
                 assert_eq!(
diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs
@@ -215,7 +215,7 @@ where
             .get_unchecked_manual(self.world, entity, self.last_change_tick, self.change_tick)
     }
 
-    // FIXME(Relationships) we likely want get_relation methods for both `T *` and `T some_entity`
+    // FIXME(Relationships) implement get_relation methods for both `T *` and `T some_entity`
 
     /// Gets a reference to the entity's component of the given type. This will fail if the entity
     /// does not have the given component type or if the given component type does not match
diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs
@@ -34,7 +34,6 @@ pub trait System: Send + Sync + 'static {
     fn name(&self) -> Cow<'static, str>;
     fn id(&self) -> SystemId;
     fn new_archetype(&mut self, archetype: &Archetype);
-    // FIXME(Relationships) this is wrong, we need to be able to say we access either `ChildOf *` or `ChildOf parent`
     fn component_access(&self) -> &Access<RelationKindId>;
     fn archetype_component_access(&self) -> &Access<ArchetypeComponentId>;
     fn is_send(&self) -> bool;
diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs
@@ -103,7 +103,6 @@ where
     }
 
     fn new_archetype(&mut self, archetype: &Archetype, system_state: &mut SystemState) {
-        // FIXME(Relationships) investigate this function
         for (relation_filter, cache) in self.relation_filter_accesses.iter_mut() {
             Self::new_archetype(
                 &self.fetch_state,
diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs
@@ -481,7 +481,6 @@ impl World {
         QueryState::new(self)
     }
 
-    // FIXME(Relationships) add a method for doing this for `T *`
     /// Returns an iterator of entities that had components of type `T` removed
     /// since the last call to [World::clear_trackers].
     pub fn removed<T: Component>(&self) -> std::iter::Cloned<std::slice::Iter<'_, Entity>> {
@@ -492,6 +491,7 @@ impl World {
         }
     }
 
+    // FIXME(Relationships) implement a way to set `*` for the target
     /// Returns an iterator of entities that had components with the given `component_id` removed
     /// since the last call to [World::clear_trackers].
     ///
@@ -831,9 +831,6 @@ impl World {
                 resource_archetype_components.insert(
                     component_id,
                     (
-                        // FIXME(Relationships) I'm not sure if we should really be overwriting the entry in
-                        // resource_archetype_components, this *is* for resources so we should never really
-                        // be inserting anything into the hashmap so this is probably fine /shrug
                         Some(ArchetypeComponentInfo {
                             archetype_component_id: ArchetypeComponentId::new(
                                 *archetype_component_count,
diff --git a/crates/bevy_ecs/src/world/tests.rs b/crates/bevy_ecs/src/world/tests.rs
@@ -144,8 +144,6 @@ fn relation_access_raw(storage_type: StorageType) {
     assert_eq!(
         accessor.next().unwrap(),
         (
-            // FIXME(Relationships) honestly having Option<Entity> is really annoying
-            // i should just make a statically knowable entity to represent None...
             Some(parent1),
             &ChildOf {
                 despawn_recursive: true
@@ -165,8 +163,6 @@ fn relation_access_raw(storage_type: StorageType) {
     assert_eq!(
         accessor.next().unwrap(),
         (
-            // FIXME(Relationships) honestly having Option<Entity> is really annoying
-            // i should just make a statically knowable entity to represent None...
             Some(parent2),
             &ChildOf {
                 despawn_recursive: false

Original file line number	Diff line number	Diff line change
`@@ -215,7 +215,7 @@ where`
`215`	`215`	`.get_unchecked_manual(self.world, entity, self.last_change_tick, self.change_tick)`
`216`	`216`	`}`
`217`	`217`
`218`		- // FIXME(Relationships) we likely want get_relation methods for both `T *` and `T some_entity`
	`218`	+ // FIXME(Relationships) implement get_relation methods for both `T *` and `T some_entity`
`219`	`219`
`220`	`220`	`/// Gets a reference to the entity's component of the given type. This will fail if the entity`
`221`	`221`	`/// does not have the given component type or if the given component type does not match`
Original file line number	Diff line number	Diff line change
`@@ -103,7 +103,6 @@ where`
`103`	`103`	`}`
`104`	`104`
`105`	`105`	`fn new_archetype(&mut self, archetype: &Archetype, system_state: &mut SystemState) {`
`106`		`- // FIXME(Relationships) investigate this function`
`107`	`106`	`for (relation_filter, cache) in self.relation_filter_accesses.iter_mut() {`
`108`	`107`	`Self::new_archetype(`
`109`	`108`	`&self.fetch_state,`
Original file line number	Diff line number	Diff line change
`@@ -481,7 +481,6 @@ impl World {`
`481`	`481`	`QueryState::new(self)`
`482`	`482`	`}`
`483`	`483`
`484`		- // FIXME(Relationships) add a method for doing this for `T *`
`485`	`484`	/// Returns an iterator of entities that had components of type `T` removed
`486`	`485`	`/// since the last call to [World::clear_trackers].`
`487`	`486`	`pub fn removed<T: Component>(&self) -> std::iter::Cloned<std::slice::Iter<'_, Entity>> {`
`@@ -492,6 +491,7 @@ impl World {`
`492`	`491`	`}`
`493`	`492`	`}`
`494`	`493`
	`494`	+ // FIXME(Relationships) implement a way to set `*` for the target
`495`	`495`	/// Returns an iterator of entities that had components with the given `component_id` removed
`496`	`496`	`/// since the last call to [World::clear_trackers].`
`497`	`497`	`///`
`@@ -831,9 +831,6 @@ impl World {`
`831`	`831`	`resource_archetype_components.insert(`
`832`	`832`	`component_id,`
`833`	`833`	`(`
`834`		`- // FIXME(Relationships) I'm not sure if we should really be overwriting the entry in`
`835`		`- // resource_archetype_components, this is for resources so we should never really`
`836`		`- // be inserting anything into the hashmap so this is probably fine /shrug`
`837`	`834`	`Some(ArchetypeComponentInfo {`
`838`	`835`	`archetype_component_id: ArchetypeComponentId::new(`
`839`	`836`	`*archetype_component_count,`