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.

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

Move EntityHash related types into bevy_ecs #11498

Merged
merged 19 commits into from
Feb 12, 2024

Conversation

doonv
Copy link
Contributor

@doonv doonv commented Jan 23, 2024

Objective

Reduce the size of bevy_utils (#11478)

Solution

Move EntityHash related types into bevy_ecs. This also allows us access to Entity, which means we no longer need EntityHashMap's first generic argument.


Changelog

  • Moved bevy::utils::{EntityHash, EntityHasher, EntityHashMap, EntityHashSet} into bevy::ecs::entity::hash .
  • Removed EntityHashMap's first generic argument. It is now hardcoded to always be Entity.

Migration Guide

  • Uses of bevy::utils::{EntityHash, EntityHasher, EntityHashMap, EntityHashSet} now have to be imported from bevy::ecs::entity::hash.
  • Uses of EntityHashMap no longer have to specify the first generic parameter. It is now hardcoded to always be Entity.

@alice-i-cecile alice-i-cecile added A-Utils Utility functions and types A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Jan 23, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

LGTM, though the migration guide should mention that EntityHashMap now only has one generic argument (the value type), instead of two.

CHANGELOG.md Outdated Show resolved Hide resolved
@doonv doonv requested a review from SkiFire13 February 9, 2024 14:27
@doonv doonv mentioned this pull request Feb 9, 2024
16 tasks
@SkiFire13
Copy link
Contributor

The migration guide could be changed to mention what needs to be adapter for the new changes, rather than the new changes themself. For example:

Migration guide

  • Uses of bevy::utils::{EntityHash, EntityHasher, EntityHashMap, EntityHashSet} now have to import them from bevy::ecs::entity::hash.
  • Uses of EntityHashMap no longer have to specify its first generic parameter. It is now hardcoded to always be Entity.

@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 9, 2024
@james7132 james7132 added this pull request to the merge queue Feb 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 11, 2024
@alice-i-cecile
Copy link
Member

CI failure is real: looks like you forgot to feature flag imports.

@doonv
Copy link
Contributor Author

doonv commented Feb 12, 2024

It doesn't look like my problem. If it is, what am I doing wrong?

Copy link
Contributor

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

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

These should be the cause of the CI failure.

crates/bevy_ecs/src/entity/hash.rs Show resolved Hide resolved
crates/bevy_ecs/src/entity/mod.rs Show resolved Hide resolved
crates/bevy_ecs/src/reflect/mod.rs Show resolved Hide resolved
@doonv doonv requested a review from SkiFire13 February 12, 2024 12:33
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Second time's the charm.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 12, 2024
Merged via the queue into bevyengine:main with commit 1c67e02 Feb 12, 2024
26 of 27 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jun 3, 2024
# Objective

- Follow-up on some changes in #11498
- Unblock using `Identifier` to replace `ComponentId` internals.

## Solution

- Implement the same `Reflect` impls from `Entity` onto `Identifier` as
they share same/similar purposes,

## Testing

- No compile errors. Currently `Identifier` has no serialization impls,
so there's no need to test a serialization/deserialization roundtrip to
ensure correctness.

---

## Changelog

### Added

- Reflection implementations on `Identifier`.
github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
# Objective

`EntityHash` and related types were moved from `bevy_utils` to
`bevy_ecs` in #11498, but seemed to have been accidentally reintroduced
a week later in #11707.

## Solution

Remove the old leftover code.

---

## Migration Guide

- Uses of `bevy::utils::{EntityHash, EntityHasher, EntityHashMap,
EntityHashSet}` now have to be imported from `bevy::ecs::entity`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change 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