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

Provide entities_iter() on EntityMapper to get an iterator of foreign entities #13703

Closed
aristaeus opened this issue Jun 6, 2024 · 0 comments · Fixed by #13727
Closed

Provide entities_iter() on EntityMapper to get an iterator of foreign entities #13703

aristaeus opened this issue Jun 6, 2024 · 0 comments · Fixed by #13727
Labels
C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled

Comments

@aristaeus
Copy link
Contributor

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

I want to get a list of all the entities that an EntityMapper is tracking. If we have a concrete hash map this is easy, but in generic code we have to work with the trait.

What solution would you like?

Add a new function to EntityMapper (returning Vec rather than impl Iterator for trait object reasons)

fn entities(&self) -> Vec<(Entity, Entity)>;

What alternative(s) have you considered?

Add a separate trait with this function. This would reduce breakage by not requiring the ecosystem to immediately implement a new function.

@aristaeus aristaeus added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jun 6, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 8, 2024
# Objective

- Fixes #13703

## Solution

- Added `mappings` to the `EntityMapper` trait, which returns an
iterator over currently tracked `Entity` to `Entity` mappings.
- Added `DynEntityMapper` as an [object
safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety)
alternative to `EntityMapper`.
- Added `assert_object_safe` as a helper for ensuring traits are object
safe.

## Testing

- Added new unit test `entity_mapper_iteration` which tests the
`SceneEntityMapper` implementation of `EntityMapper::mappings`.
- Added unit tests to ensure `DynEntityMapper`, `DynEq` and `DynHash`
are object safe.
- Passed CI on my Windows 10 development environment

---

## Changelog

- Added `mappings` to `EntityMapper` trait.

## Migration Guide

- If you are implementing `EntityMapper` yourself, you can use the below
as a stub implementation:

```rust
fn mappings(&self) -> impl Iterator<Item = (Entity, Entity)> {
    unimplemented!()
}
```

- If you were using `EntityMapper` as a trait object (`dyn
EntityMapper`), instead use `dyn DynEntityMapper` and its associated
methods.

## Notes

- The original issue proposed returning a `Vec` from `EntityMapper`
instead of an `impl Iterator` to preserve its object safety. This is a
simpler option, but also forces an allocation where it isn't strictly
needed. I've opted for this split into `DynEntityMapper` and
`EntityMapper` as it's been done several times across Bevy already, and
provides maximum flexibility to users.
- `assert_object_safe` is an empty function, since the assertion
actually happens once you try to use a `dyn T` for some trait `T`. I
have still added this function to clearly document what object safety is
within Bevy, and to create a standard way to communicate that a given
trait must be object safe.
- Other traits should have tests added to ensure object safety, but I've
left those off to avoid cluttering this PR further.

---------

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
Labels
C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant