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

Use EntityHashMap for EntityMap #10391

Closed
nicopap opened this issue Nov 5, 2023 · 0 comments · Fixed by #10415
Closed

Use EntityHashMap for EntityMap #10391

nicopap opened this issue Nov 5, 2023 · 0 comments · Fixed by #10415
Labels
A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@nicopap
Copy link
Contributor

nicopap commented Nov 5, 2023

What problem does this solve or what need does it fill?

  • We have a specialized data structure for fast Entity lookup: EntityHashMap
  • We use a HashMap<Entity, Entity> in EntityMapper

What solution would you like?

We should use the more performant implementation.

@nicopap nicopap added C-Feature A new feature, making something new possible D-Trivial Nice and easy! A great choice to get started with Bevy C-Performance A change motivated by improving speed, memory usage or compile times A-Scenes Serialized ECS data stored on the disk labels Nov 5, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 7, 2023
# Objective

- There is a specialized hasher for entities:
[`EntityHashMap`](https://docs.rs/bevy/latest/bevy/utils/type.EntityHashMap.html)
- [`EntityMapper`] currently uses a normal `HashMap<Entity, Entity>`
- Fixes #10391

## Solution

- Replace the normal `HashMap` with the more performant `EntityHashMap`

## Questions

- This does change public API. Should a system be implemented to help
migrate code?
  - Perhaps an `impl From<HashMap<K, V, S>> for EntityHashMap<K, V>`
- I updated to docs for each function that I changed, but I may have
missed something

---

## Changelog

- Changed `EntityMapper` to use `EntityHashMap` instead of normal
`HashMap`

## Migration Guide

If you are using the following types, update their listed methods to use
the new `EntityHashMap`. `EntityHashMap` has the same methods as the
normal `HashMap`, so you just need to replace the name.

### `EntityMapper`

- `get_map`
- `get_mut_map`
- `new`
- `world_scope`

### `ReflectMapEntities`

- `map_all_entities`
- `map_entities`
- `write_to_world`

### `InstanceInfo`

- `entity_map`
  - This is a property, not a method.

---

This is my first time contributing in a while, and I'm not familiar with
the usage of `EntityMapper`. I changed the type definition and fixed all
errors, but there may have been things I've missed. Please keep an eye
out for me!
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
# Objective

- There is a specialized hasher for entities:
[`EntityHashMap`](https://docs.rs/bevy/latest/bevy/utils/type.EntityHashMap.html)
- [`EntityMapper`] currently uses a normal `HashMap<Entity, Entity>`
- Fixes bevyengine#10391

## Solution

- Replace the normal `HashMap` with the more performant `EntityHashMap`

## Questions

- This does change public API. Should a system be implemented to help
migrate code?
  - Perhaps an `impl From<HashMap<K, V, S>> for EntityHashMap<K, V>`
- I updated to docs for each function that I changed, but I may have
missed something

---

## Changelog

- Changed `EntityMapper` to use `EntityHashMap` instead of normal
`HashMap`

## Migration Guide

If you are using the following types, update their listed methods to use
the new `EntityHashMap`. `EntityHashMap` has the same methods as the
normal `HashMap`, so you just need to replace the name.

### `EntityMapper`

- `get_map`
- `get_mut_map`
- `new`
- `world_scope`

### `ReflectMapEntities`

- `map_all_entities`
- `map_entities`
- `write_to_world`

### `InstanceInfo`

- `entity_map`
  - This is a property, not a method.

---

This is my first time contributing in a while, and I'm not familiar with
the usage of `EntityMapper`. I changed the type definition and fixed all
errors, but there may have been things I've missed. Please keep an eye
out for me!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant