Skip to content

Conversation

@2ne1ugly
Copy link
Contributor

@2ne1ugly 2ne1ugly commented Aug 24, 2025

Objective

Solution

  • Implement Map, PartialReflect, Reflect, Typed, FromReflect, GetTypeRegistration for indexmap::IndexMap and indexmap::IndexSet
  • Add #[derive(Reflect)] to EntityIndexSet
  • Add indexmap feature to bevy_reflect and Update docs accordingly

Testing

  • Added test on indexmap::IndexMap

I initially tried using impl_reflect_for_hashmap, but ran into two issues:

  1. IndexMap::drain requires a range parameter, unlike HashMap::drain.
  2. IndexMap::remove defaults to swap_remove, and is now deprecated.

I went with shift_remove, since I think it matches user expectations.
That said, it’s O(n), while swap_remove is O(1).
Would love feedback on which behavior we should prefer.

@2ne1ugly 2ne1ugly changed the title implement Reflect for indexmap and adjust docs accordingly implement Reflect for indexmap Aug 24, 2025
@2ne1ugly 2ne1ugly changed the title implement Reflect for indexmap Implement Reflect for indexmap::IndexMapand indexmap::IndexSet Aug 24, 2025
@Victoronz Victoronz added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 24, 2025
@james7132 james7132 added this to the 0.18 milestone Sep 22, 2025
Copy link

@IronGremlin IronGremlin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@pablo-lua pablo-lua left a 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

@pablo-lua pablo-lua 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 Dec 13, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 14, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 15, 2025
@alice-i-cecile
Copy link
Member

@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.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 15, 2025
@alice-i-cecile alice-i-cecile removed this from the 0.18 milestone Dec 15, 2025
@2ne1ugly 2ne1ugly force-pushed the impl-reflect-indexmap branch from dbea407 to 105d9a9 Compare December 15, 2025 21:08
@2ne1ugly 2ne1ugly force-pushed the impl-reflect-indexmap branch from 105d9a9 to f7c9950 Compare December 15, 2025 21:11
@2ne1ugly
Copy link
Contributor Author

Fixed the feature flag issue!

@pablo-lua pablo-lua 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 15, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 16, 2025
Merged via the queue into bevyengine:main with commit 5e8c33c Dec 16, 2025
42 checks passed
@2ne1ugly 2ne1ugly deleted the impl-reflect-indexmap branch December 16, 2025 06:36
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes 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.

Reflect is not implemented for IndexMap/IndexSet

6 participants