Skip to content

Conversation

@myreprise1
Copy link
Contributor

@myreprise1 myreprise1 commented Feb 11, 2023

Objective

  • Make defining several structs in bevy_reflect less verbose by replacing certain fields with ValueInfo. Since these are private fields, no outward changes should be needed.

Solution

Replaced fields in the form:

struct FooInfo {
    bar_name: &'static str,
    bar_id: TypeId,
}

impl FooInfo {
    fn new<T: BarType>() -> Self {
        Self {
            bar_name: std::any::type_name::<T>(),
            bar_id: TypeId::of::<T>(),
        }
    }
}

With the following:

struct FooInfo {
    bar_value: ValueInfo,
}

impl FooInfo {
    fn new<T: BarType>() -> Self {
        Self {
            bar_value: ValueInfo::new::<T>(),
        }
    }
}

@myreprise1 myreprise1 changed the title Use ValueInfo inside Use ValueInfo inside stucts in bevy_reflect Feb 11, 2023
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types labels Feb 11, 2023
@myreprise1 myreprise1 changed the title Use ValueInfo inside stucts in bevy_reflect Use ValueInfo inside structs in bevy_reflect Feb 11, 2023
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Simpler, cleaner, DRYer. I don't see much reason not to do this (at least for now).

Comment on lines +48 to +49
type_value: ValueInfo,
item_type_value: ValueInfo,
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed: I think these names are fine, but here are some others to consider:

  1. container_type, item_type, key_type, field_type, etc.
  2. type_info, item_type_info, key_type_info, field_type_info, etc.

For me, I think I'd lean more towards either keeping what you have now or going with Alternative 1 (2 is okay, but has the implication that it contains a TypeInfo not a ValueInfo).

I’m curious if you or anyone else has any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just replaced name and id with value for consistency. I'm thinking maybe key_type_value and item_type_value.

@MrGVSV
Copy link
Member

MrGVSV commented Feb 20, 2023

@myreprise1 this may need to be rebased

github-merge-queue bot pushed a commit that referenced this pull request Aug 25, 2024
# Objective

Closes #7622.

I was working on adding support for reflecting generic functions and
found that I wanted to use an argument's `TypeId` for hashing and
comparison, but its `TypePath` for debugging and error messaging.

While I could just keep them separate, place them in a tuple or a local
struct or something, I think I see an opportunity to make a dedicate
type for this.

Additionally, we can use this type to clean up some duplication amongst
the type info structs in a manner similar to #7622.

## Solution

Added the `Type` type. This should be seen as the most basic
representation of a type apart from `TypeId`. It stores both the
`TypeId` of the type as well as its `TypePathTable`.

The `Hash` and `PartialEq` implementations rely on the `TypeId`, while
the `Debug` implementation relies on the `TypePath`.

This makes it especially useful as a key in a `HashMap` since we get the
speed of the `TypeId` hashing/comparisons with the readability of
`TypePath`.

With this type, we're able to reduce the duplication across the type
info structs by removing individual fields for `TypeId` and
`TypePathTable`, replacing them with a single `Type` field. Similarly,
we can remove many duplicate methods and replace it with a macro that
delegates to the stored `Type`.

### Caveats

It should be noted that this type is currently 3x larger than `TypeId`.
On my machine, it's 48 bytes compared to `TypeId`'s 16. While this
doesn't matter for `TypeInfo` since it would contain that data
regardless, it is something to keep in mind when using elsewhere.

## Testing

All tests should pass as normal:

```
cargo test --package bevy_reflect
```

---

## Showcase

`bevy_reflect` now exports a `Type` struct. This type contains both the
`TypeId` and the `TypePathTable` of the given type, allowing it to be
used like `TypeId` but have the debuggability of `TypePath`.

```rust
// We can create this for any type implementing `TypePath`:
let ty = Type::of::<String>();

// It has `Hash` and `Eq` impls powered by `TypeId`, making it useful for maps:
let mut map = HashMap::<Type, i32>::new();
map.insert(ty, 25);

// And it has a human-readable `Debug` representation:
let debug = format!("{:?}", map);
assert_eq!(debug, "{alloc::string::String: 25}");
```

## Migration Guide

Certain type info structs now only return their item types as `Type`
instead of exposing direct methods on them.

The following methods have been removed:

- `ArrayInfo::item_type_path_table`
- `ArrayInfo::item_type_id`
- `ArrayInfo::item_is`
- `ListInfo::item_type_path_table`
- `ListInfo::item_type_id`
- `ListInfo::item_is`
- `SetInfo::value_type_path_table`
- `SetInfo::value_type_id`
- `SetInfo::value_is`
- `MapInfo::key_type_path_table`
- `MapInfo::key_type_id`
- `MapInfo::key_is`
- `MapInfo::value_type_path_table`
- `MapInfo::value_type_id`
- `MapInfo::value_is`

Instead, access the `Type` directly using one of the new methods:

- `ArrayInfo::item_ty`
- `ListInfo::item_ty`
- `SetInfo::value_ty`
- `MapInfo::key_ty`
- `MapInfo::value_ty`

For example:

```rust
// BEFORE
let type_id = array_info.item_type_id();

// AFTER
let type_id = array_info.item_ty().id();
```
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-Code-Quality A section of code that is hard to understand or change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants