Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use TypeIdMap whenever possible #11684

Merged
merged 7 commits into from
Feb 3, 2024
Merged

Use TypeIdMap whenever possible #11684

merged 7 commits into from
Feb 3, 2024

Conversation

SpecificProtagonist
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist commented Feb 3, 2024 •
edited by alice-i-cecile
Loading

Use TypeIdMap<T> instead of HashMap<TypeId, T>

  • TypeIdMap was in bevy_ecs. I've kept it there because of Remove or shrink bevy_utils #11478
  • I haven't swapped bevy_reflect over because it doesn't depend on bevy_ecs, but I'd also be happy with moving TypeIdMap to bevy_utils and then adding a dependency to that
  • this is a slight change in the public API of DrawFunctionsInternal, does this need to go in the changelog?

Changelog

  • moved TypeIdMap to bevy_utils
  • changed DrawFunctionsInternal::indices to TypeIdMap

Migration Guide

  • TypeIdMap now lives in bevy_utils
  • DrawFunctionsInternal::indices now uses a TypeIdMap.

@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 3, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 3, 2024 •
edited
Loading

Yeah, we should note the change.

Non-blocking, but I think we should move this into bevy_utils for now: it definitely doesn't belong in bevy_ecs and moving it out might help us see other more natural groupings rather than a catch-all utility crate.

NthTensor
NthTensor approved these changes Feb 3, 2024
View reviewed changes
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Very happy to see this moved into bevy_util. Nice work.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 3, 2024
@alice-i-cecile
Copy link
Member

Added a migration guide and resolved merge conflicts. Attempting merge, fingers crossed I formatted the import statement correctly 😅

auto-merge was automatically disabled February 3, 2024 23:03

Head branch was pushed to by a user without write access

@SpecificProtagonist
Copy link
Contributor Author

Needs to be re-added to merge queue; I've just moved the corresponding test over to bevy_utils

auto-merge was automatically disabled February 3, 2024 23:11

Head branch was pushed to by a user without write access

@SpecificProtagonist
Copy link
Contributor Author

…and fixed the doc link. Sorry.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 3, 2024
@alice-i-cecile
Copy link
Member

No worries, this is why we have CI :)

Merged via the queue into bevyengine:main with commit 21aa5fe Feb 3, 2024
23 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
@SpecificProtagonist @alice-i-cecile @tjamaan
Use TypeIdMap whenever possible (bevyengine#11684)
f2a8400 
Use `TypeIdMap<T>` instead of `HashMap<TypeId, T>`

- ~~`TypeIdMap` was in `bevy_ecs`. I've kept it there because of
bevyengine#11478~~
- ~~I haven't swapped `bevy_reflect` over because it doesn't depend on
`bevy_ecs`, but I'd also be happy with moving `TypeIdMap` to
`bevy_utils` and then adding a dependency to that~~
- ~~this is a slight change in the public API of
`DrawFunctionsInternal`, does this need to go in the changelog?~~

## Changelog
- moved `TypeIdMap` to `bevy_utils`
- changed `DrawFunctionsInternal::indices` to `TypeIdMap`

## Migration Guide

- `TypeIdMap` now lives in `bevy_utils`
- `DrawFunctionsInternal::indices` now uses a `TypeIdMap`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@NthTensor NthTensor NthTensor approved these changes

Assignees
No one assigned
Labels
C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SpecificProtagonist @alice-i-cecile @NthTensor