Skip to content

Commit

Permalink
Dedicated Reflect implementation for Set-like things (bevyengine#…
Browse files Browse the repository at this point in the history
…13014)

# Objective

I just wanted to inspect `HashSet`s in `bevy-inspector-egui` but I
noticed that it didn't work for some reason. A few minutes later I found
myself looking into the bevy reflect impls noticing that `HashSet`s have
been covered only rudimentary up until now.

## Solution

I'm not sure if this is overkill (especially the first bullet), but
here's a list of the changes:

- created a whole new trait and enum variants for `ReflectRef` and the
like called `Set`
- mostly oriented myself at the `Map` trait and made the necessary
changes until RA was happy
- create macro `impl_reflect_for_hashset!` and call it on `std::HashSet`
and `hashbrown::HashSet`

Extra notes:

- no `get_mut` or `get_mut_at` mirroring the `std::HashSet`
- `insert[_boxed]` and `remove` return `bool` mirroring `std::HashSet`,
additionally that bool is reflect as I thought that would be how we
handle things in bevy reflect, but I'm not sure on this
- ser/de are handled via `SeqAccess`
- I'm not sure about the general deduplication property of this impl of
`Set` that is generally expected? I'm also not sure yet if `Map` does
provide this. This mainly refers to the `Dynamic[...]` structs
- I'm not sure if there are other methods missing from the `trait`, I
felt like `contains` or the set-operations (union/diff/...) could've
been helpful, but I wanted to get out the bare minimum for feedback
first

---

## Changelog

### Added
- `Set` trait for `bevy_reflect`

### Changed
- `std::collections::HashSet` and `bevy_utils::hashbrown::HashSet` now
implement a more complete set of reflect functionalities instead of
"just" `reflect_value`
- `TypeInfo` contains a new variant `Set` that contains `SetInfo`
- `ReflectKind` contains a new variant `Set`
- `ReflectRef` contains a new variant `Set`
- `ReflectMut` contains a new variant `Set`
- `ReflectOwned` contains a new variant `Set`

## Migration Guide

- The new `Set` variants on the enums listed in the change section
should probably be considered by people working with this level of the
lib
### Help wanted! 

I'm not sure if this change is able to break code. From my understanding
it shouldn't since we just add functionality but I'm not sure yet if
theres anything missing from my impl that would be normally provided by
`impl_reflect_value!`
  • Loading branch information
RobWalt authored Jul 24, 2024
1 parent eabb58a commit 52a2a3b
Show file tree
Hide file tree
Showing 8 changed files with 831 additions and 20 deletions.
233 changes: 221 additions & 12 deletions crates/bevy_reflect/src/impls/std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ use crate::utility::{
reflect_hasher, GenericTypeInfoCell, GenericTypePathCell, NonGenericTypeInfoCell,
};
use crate::{
self as bevy_reflect, impl_type_path, map_apply, map_partial_eq, map_try_apply, ApplyError,
Array, ArrayInfo, ArrayIter, DynamicMap, DynamicTypePath, FromReflect, FromType,
GetTypeRegistration, List, ListInfo, ListIter, Map, MapInfo, MapIter, MaybeTyped, Reflect,
ReflectDeserialize, ReflectFromPtr, ReflectFromReflect, ReflectKind, ReflectMut, ReflectOwned,
ReflectRef, ReflectSerialize, TypeInfo, TypePath, TypeRegistration, TypeRegistry, Typed,
ValueInfo,
self as bevy_reflect, impl_type_path, map_apply, map_partial_eq, map_try_apply, set_apply,
set_partial_eq, set_try_apply, ApplyError, Array, ArrayInfo, ArrayIter, DynamicMap, DynamicSet,
DynamicTypePath, FromReflect, FromType, GetTypeRegistration, List, ListInfo, ListIter, Map,
MapInfo, MapIter, MaybeTyped, Reflect, ReflectDeserialize, ReflectFromPtr, ReflectFromReflect,
ReflectKind, ReflectMut, ReflectOwned, ReflectRef, ReflectSerialize, Set, SetInfo, TypeInfo,
TypePath, TypeRegistration, TypeRegistry, Typed, ValueInfo,
};
use bevy_reflect_derive::{impl_reflect, impl_reflect_value};
use std::fmt;
Expand Down Expand Up @@ -97,8 +97,6 @@ impl_reflect_value!(::std::path::PathBuf(
));
impl_reflect_value!(::std::any::TypeId(Debug, Hash, PartialEq,));
impl_reflect_value!(::std::collections::BTreeSet<T: Ord + Eq + Clone + Send + Sync>());
impl_reflect_value!(::std::collections::HashSet<T: Hash + Eq + Clone + Send + Sync, S: TypePath + Clone + Send + Sync>());
impl_reflect_value!(::bevy_utils::hashbrown::HashSet<T: Hash + Eq + Clone + Send + Sync, S: TypePath + Clone + Send + Sync>());
impl_reflect_value!(::core::ops::Range<T: Clone + Send + Sync>());
impl_reflect_value!(::core::ops::RangeInclusive<T: Clone + Send + Sync>());
impl_reflect_value!(::core::ops::RangeFrom<T: Clone + Send + Sync>());
Expand Down Expand Up @@ -216,10 +214,6 @@ impl_reflect_value!(::std::ffi::OsString(
impl_reflect_value!(::std::ffi::OsString(Debug, Hash, PartialEq));
impl_reflect_value!(::alloc::collections::BinaryHeap<T: Clone>);

impl_type_path!(::bevy_utils::NoOpHash);
impl_type_path!(::bevy_utils::EntityHash);
impl_type_path!(::bevy_utils::FixedState);

macro_rules! impl_reflect_for_veclike {
($ty:path, $insert:expr, $remove:expr, $push:expr, $pop:expr, $sub:ty) => {
impl<T: FromReflect + MaybeTyped + TypePath + GetTypeRegistration> List for $ty {
Expand Down Expand Up @@ -662,6 +656,221 @@ crate::func::macros::impl_function_traits!(::bevy_utils::hashbrown::HashMap<K, V
>
);

macro_rules! impl_reflect_for_hashset {
($ty:path) => {
impl<V, S> Set for $ty
where
V: FromReflect + TypePath + GetTypeRegistration + Eq + Hash,
S: TypePath + BuildHasher + Send + Sync,
{
fn get(&self, value: &dyn Reflect) -> Option<&dyn Reflect> {
value
.downcast_ref::<V>()
.and_then(|value| Self::get(self, value))
.map(|value| value as &dyn Reflect)
}

fn len(&self) -> usize {
Self::len(self)
}

fn iter(&self) -> Box<dyn Iterator<Item = &dyn Reflect> + '_> {
let iter = self.iter().map(|v| v as &dyn Reflect);
Box::new(iter)
}

fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {
self.into_iter()
.map(|value| Box::new(value) as Box<dyn Reflect>)
.collect()
}

fn clone_dynamic(&self) -> DynamicSet {
let mut dynamic_set = DynamicSet::default();
dynamic_set.set_represented_type(self.get_represented_type_info());
for v in self {
dynamic_set.insert_boxed(v.clone_value());
}
dynamic_set
}

fn insert_boxed(&mut self, value: Box<dyn Reflect>) -> bool {
let value = V::take_from_reflect(value).unwrap_or_else(|value| {
panic!(
"Attempted to insert invalid value of type {}.",
value.reflect_type_path()
)
});
self.insert(value)
}

fn remove(&mut self, value: &dyn Reflect) -> bool {
let mut from_reflect = None;
value
.downcast_ref::<V>()
.or_else(|| {
from_reflect = V::from_reflect(value);
from_reflect.as_ref()
})
.map_or(false, |value| self.remove(value))
}

fn contains(&self, value: &dyn Reflect) -> bool {
let mut from_reflect = None;
value
.downcast_ref::<V>()
.or_else(|| {
from_reflect = V::from_reflect(value);
from_reflect.as_ref()
})
.map_or(false, |value| self.contains(value))
}
}

impl<V, S> Reflect for $ty
where
V: FromReflect + TypePath + GetTypeRegistration + Eq + Hash,
S: TypePath + BuildHasher + Send + Sync,
{
fn get_represented_type_info(&self) -> Option<&'static TypeInfo> {
Some(<Self as Typed>::type_info())
}

fn into_any(self: Box<Self>) -> Box<dyn Any> {
self
}

fn as_any(&self) -> &dyn Any {
self
}

fn as_any_mut(&mut self) -> &mut dyn Any {
self
}

#[inline]
fn into_reflect(self: Box<Self>) -> Box<dyn Reflect> {
self
}

fn as_reflect(&self) -> &dyn Reflect {
self
}

fn as_reflect_mut(&mut self) -> &mut dyn Reflect {
self
}

fn apply(&mut self, value: &dyn Reflect) {
set_apply(self, value);
}

fn try_apply(&mut self, value: &dyn Reflect) -> Result<(), ApplyError> {
set_try_apply(self, value)
}

fn set(&mut self, value: Box<dyn Reflect>) -> Result<(), Box<dyn Reflect>> {
*self = value.take()?;
Ok(())
}

fn reflect_kind(&self) -> ReflectKind {
ReflectKind::Set
}

fn reflect_ref(&self) -> ReflectRef {
ReflectRef::Set(self)
}

fn reflect_mut(&mut self) -> ReflectMut {
ReflectMut::Set(self)
}

fn reflect_owned(self: Box<Self>) -> ReflectOwned {
ReflectOwned::Set(self)
}

fn clone_value(&self) -> Box<dyn Reflect> {
Box::new(self.clone_dynamic())
}

fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
set_partial_eq(self, value)
}
}

impl<V, S> Typed for $ty
where
V: FromReflect + TypePath + GetTypeRegistration + Eq + Hash,
S: TypePath + BuildHasher + Send + Sync,
{
fn type_info() -> &'static TypeInfo {
static CELL: GenericTypeInfoCell = GenericTypeInfoCell::new();
CELL.get_or_insert::<Self, _>(|| TypeInfo::Set(SetInfo::new::<Self, V>()))
}
}

impl<V, S> GetTypeRegistration for $ty
where
V: FromReflect + TypePath + GetTypeRegistration + Eq + Hash,
S: TypePath + BuildHasher + Send + Sync,
{
fn get_type_registration() -> TypeRegistration {
let mut registration = TypeRegistration::of::<Self>();
registration.insert::<ReflectFromPtr>(FromType::<Self>::from_type());
registration
}

fn register_type_dependencies(registry: &mut TypeRegistry) {
registry.register::<V>();
}
}

impl<V, S> FromReflect for $ty
where
V: FromReflect + TypePath + GetTypeRegistration + Eq + Hash,
S: TypePath + BuildHasher + Default + Send + Sync,
{
fn from_reflect(reflect: &dyn Reflect) -> Option<Self> {
if let ReflectRef::Set(ref_set) = reflect.reflect_ref() {
let mut new_set = Self::with_capacity_and_hasher(ref_set.len(), S::default());
for value in ref_set.iter() {
let new_value = V::from_reflect(value)?;
new_set.insert(new_value);
}
Some(new_set)
} else {
None
}
}
}
};
}

impl_type_path!(::bevy_utils::NoOpHash);
impl_type_path!(::bevy_utils::EntityHash);
impl_type_path!(::bevy_utils::FixedState);

impl_reflect_for_hashset!(::std::collections::HashSet<V,S>);
impl_type_path!(::std::collections::HashSet<V, S>);
#[cfg(feature = "functions")]
crate::func::macros::impl_function_traits!(::std::collections::HashSet<V, S>;
<
V: Hash + Eq + FromReflect + TypePath + GetTypeRegistration,
S: TypePath + BuildHasher + Default + Send + Sync
>
);

impl_reflect_for_hashset!(::bevy_utils::hashbrown::HashSet<V,S>);
impl_type_path!(::bevy_utils::hashbrown::HashSet<V, S>);
#[cfg(feature = "functions")]
crate::func::macros::impl_function_traits!(::bevy_utils::hashbrown::HashSet<V, S>;
<
V: Hash + Eq + FromReflect + TypePath + GetTypeRegistration,
S: TypePath + BuildHasher + Default + Send + Sync
>
);

impl<K, V> Map for ::std::collections::BTreeMap<K, V>
where
K: FromReflect + MaybeTyped + TypePath + GetTypeRegistration + Eq + Ord,
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_reflect/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ mod list;
mod map;
mod path;
mod reflect;
mod set;
mod struct_trait;
mod tuple;
mod tuple_struct;
Expand Down Expand Up @@ -531,6 +532,7 @@ pub use list::*;
pub use map::*;
pub use path::*;
pub use reflect::*;
pub use set::*;
pub use struct_trait::*;
pub use tuple::*;
pub use tuple_struct::*;
Expand Down
9 changes: 8 additions & 1 deletion crates/bevy_reflect/src/reflect.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
array_debug, enum_debug, list_debug, map_debug, serde::Serializable, struct_debug, tuple_debug,
tuple_struct_debug, Array, DynamicTypePath, Enum, List, Map, Struct, Tuple, TupleStruct,
tuple_struct_debug, Array, DynamicTypePath, Enum, List, Map, Set, Struct, Tuple, TupleStruct,
TypeInfo, TypePath, Typed, ValueInfo,
};
use std::{
Expand All @@ -24,6 +24,7 @@ macro_rules! impl_reflect_enum {
Self::List(_) => ReflectKind::List,
Self::Array(_) => ReflectKind::Array,
Self::Map(_) => ReflectKind::Map,
Self::Set(_) => ReflectKind::Set,
Self::Enum(_) => ReflectKind::Enum,
Self::Value(_) => ReflectKind::Value,
}
Expand All @@ -39,6 +40,7 @@ macro_rules! impl_reflect_enum {
$name::List(_) => Self::List,
$name::Array(_) => Self::Array,
$name::Map(_) => Self::Map,
$name::Set(_) => Self::Set,
$name::Enum(_) => Self::Enum,
$name::Value(_) => Self::Value,
}
Expand All @@ -60,6 +62,7 @@ pub enum ReflectRef<'a> {
List(&'a dyn List),
Array(&'a dyn Array),
Map(&'a dyn Map),
Set(&'a dyn Set),
Enum(&'a dyn Enum),
Value(&'a dyn Reflect),
}
Expand All @@ -78,6 +81,7 @@ pub enum ReflectMut<'a> {
List(&'a mut dyn List),
Array(&'a mut dyn Array),
Map(&'a mut dyn Map),
Set(&'a mut dyn Set),
Enum(&'a mut dyn Enum),
Value(&'a mut dyn Reflect),
}
Expand All @@ -96,6 +100,7 @@ pub enum ReflectOwned {
List(Box<dyn List>),
Array(Box<dyn Array>),
Map(Box<dyn Map>),
Set(Box<dyn Set>),
Enum(Box<dyn Enum>),
Value(Box<dyn Reflect>),
}
Expand Down Expand Up @@ -149,6 +154,7 @@ pub enum ReflectKind {
List,
Array,
Map,
Set,
Enum,
Value,
}
Expand All @@ -162,6 +168,7 @@ impl std::fmt::Display for ReflectKind {
ReflectKind::List => f.pad("list"),
ReflectKind::Array => f.pad("array"),
ReflectKind::Map => f.pad("map"),
ReflectKind::Set => f.pad("set"),
ReflectKind::Enum => f.pad("enum"),
ReflectKind::Value => f.pad("value"),
}
Expand Down
Loading

0 comments on commit 52a2a3b

Please sign in to comment.