Skip to content

Conversation

@MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Aug 20, 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 dedicated 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.

// 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:

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

// AFTER
let type_id = array_info.item_ty().id();

@MrGVSV MrGVSV added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 20, 2024
Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

Examples run, and after reading the documentation and files, I think this looks good (coming from a person who hasn't really touched reflect code before).

Copy link
Contributor

@killercup killercup left a comment

Choose a reason for hiding this comment

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

Looks like a good refactor!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 23, 2024
@alice-i-cecile
Copy link
Member

Merging on Monday; feel free to clean it up in the meantime :)

@MrGVSV
Copy link
Member Author

MrGVSV commented Aug 25, 2024

Updated the code to use Type for FunctionInfo and ArgInfo.

Also renamed Type::type_id to Type::id (like how we have Type::path), as suggested by @killercup.

@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/type branch from 843e04d to ad513ee Compare August 25, 2024 17:49
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 25, 2024
Merged via the queue into bevyengine:main with commit 3892adc Aug 25, 2024
@MrGVSV MrGVSV deleted the mrgvsv/reflect/type branch August 25, 2024 18:42
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants