-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Implement Reflect for indexmap::IndexMapand indexmap::IndexSet
#20734
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
Implement Reflect for indexmap::IndexMapand indexmap::IndexSet
#20734
Conversation
indexmap::IndexMapand indexmap::IndexSet
IronGremlin
left a comment
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.
LGTM
pablo-lua
left a comment
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.
Looks pretty much straightforward and even comes with tests. LGTM
|
@2ne1ugly I think your feature gates are slightly wrong: check the CI logs for details. I'd like to ship this in 0.18, but it's not a blocker, so I'm cutting it from the milestone and we'll add it in a later release candidate if it happens to get in. |
dbea407 to
105d9a9
Compare
105d9a9 to
f7c9950
Compare
|
Fixed the feature flag issue! |
Objective
Solution
indexmap::IndexMapandindexmap::IndexSet#[derive(Reflect)]toEntityIndexSetbevy_reflectand Update docs accordinglyTesting
I initially tried using
impl_reflect_for_hashmap, but ran into two issues:IndexMap::drainrequires a range parameter, unlikeHashMap::drain.IndexMap::removedefaults toswap_remove, and is now deprecated.I went with
shift_remove, since I think it matches user expectations.That said, it’s
O(n), whileswap_removeisO(1).Would love feedback on which behavior we should prefer.