Skip to content

Conversation

@bushrat011899
Copy link
Contributor

Objective

Solution

  • Added mappings to the EntityMapper trait, which returns an iterator over currently tracked Entity to Entity mappings.
  • Added DynEntityMapper as an object safe 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:
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.

Provides an iterator over currently tracked entity mappings.

Since this change causes `EntityMapper` to no longer be object safe, I've also provided a `DynEntityMapper` (similar to the existing `DynEq` and `DynHash` traits). It maintains object safety by returning a concrete `Vec` instead of an `impl Iterator` on the `mappings` method. This trait is automatically implemented and is therefor a drop-in replacement for cases where `EntityMapper` was used as a trait object.

To ensure object safety guarantees aren't inadvertently broken, I've added a unit test which will fail to compile if that contract is violated. I've also extended this testing to `DynEq` and `DynHash`.
@bushrat011899 bushrat011899 added C-Feature A new feature, making something new possible D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 7, 2024
@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Jun 7, 2024
@alice-i-cecile
Copy link
Member

Neat, the assert_object_safe tests and the Dyn trait variants are compelling. I think I like this solution overall: this is perf-sensitive code so the complexity is worth it.

Definitely more complicated than Trivial though: I tend to reserve that for "could someone that's never written Rust review this PR" :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Feature A new feature, making something new possible labels Jun 7, 2024
Co-Authored-By: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile alice-i-cecile enabled auto-merge June 8, 2024 12:45
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 8, 2024
Merged via the queue into bevyengine:main with commit 3bfc427 Jun 8, 2024
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-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

Development

Successfully merging this pull request may close these issues.

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

3 participants