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 EntityHashMap for EntityMapper #10415

Merged
merged 4 commits into from
Nov 7, 2023
Merged

Use EntityHashMap for EntityMapper #10415

merged 4 commits into from
Nov 7, 2023

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Nov 6, 2023 •
edited
Loading

Objective

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!

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events 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 labels Nov 6, 2023
@alice-i-cecile
Copy link
Member

This does change public API. Should a system be implemented to help migrate code?

Breaking changes are generally fine.

Perhaps an impl From<HashMap<K, V, S>> for EntityHashMap<K, V>

I think that we should probably just use the FromIterator trait. Useful idea though, might as well do that here.

I updated to docs for each function that I changed, but I may have missed something

Looks good!

@BD103
Copy link
Member Author

BD103 commented Nov 6, 2023 •
edited
Loading

I checked out the CI error, but I'm unsure why it's happening. Cargo.lock has async_lock 2.8.0 and 3.0.0 both. The blocking crate calls get_or_init_blocking here. blocking requires async_lock 3.0.0. Maybe it's trying to pull in the 2.8.0 version?

Edit:

get_or_init_blocking and related code are disabled when target_family = "wasm". Is there an easy fix / another PR for this?

@alice-i-cecile
Copy link
Member

Failure was #10412: not your fault!

@nicopap nicopap added this to the 0.13 milestone Nov 6, 2023
@BD103
Copy link
Member Author

BD103 commented Nov 6, 2023

Failure was #10412: not your fault!

Glad to hear it wasn't something I did! Could you please re-run the CI?

nicopap
nicopap approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Nice! LGTM. Need to fix the PR description before merging though.

@BD103
Copy link
Member Author

BD103 commented Nov 6, 2023

Nice! LGTM. Need to fix the PR description before merging though.

I updated the description. Please tell me if the migration guide or changelog is not adequate. :)

@alice-i-cecile alice-i-cecile mentioned this pull request Nov 6, 2023
Closed
@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 Nov 6, 2023
@BD103
Copy link
Member Author

BD103 commented Nov 6, 2023

I don't think the CI will run unless I make another commit. Could someone with write privileges please manually re-run it?

@alice-i-cecile
Copy link
Member

Re-running :)

@BD103
Copy link
Member Author

BD103 commented Nov 6, 2023

Darn, the CI is still failing. It appears that larger changes are going to need to occur in async_executor before things can get running again. @ Bluefinger suggested removing all references to LocalExecutor when compiling for WASM and making them use wasm-bindgen-futures instead, but that is a larger change that is definitely out of scope for this PR.

smol-rs/async-executor#72 (comment)
smol-rs/async-executor#73 (comment)
smol-rs/async-executor#73 (comment)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 7, 2023
Merged via the queue into bevyengine:main with commit 04ceb46 Nov 7, 2023
22 checks passed
@BD103 BD103 deleted the entityhashmap branch November 8, 2023 12:58
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@BD103
Use EntityHashMap for EntityMapper (bevyengine#10415)
ae4c0e7 
# 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
Reviewers

@nicopap nicopap nicopap approved these changes

@mockersf mockersf mockersf approved these changes

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

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events 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
0.13
Development

Successfully merging this pull request may close these issues.

Use EntityHashMap for EntityMap
4 participants
@BD103 @alice-i-cecile @mockersf @nicopap