Ralith / hecs

A handy ECS
Apache License 2.0
924 stars 81 forks source link

Add `Ref::map` and `RefMut::map`. #314

Closed burtonageo closed 1 year ago

burtonageo commented 1 year ago

This commit adds analogues of std::cell::Ref::map and std::cell::RefMut::map, which allow the contents of a Ref/RefMut to be transformed without reborrowing. This can make it easier to write certain kinds of functions which return data borrowed from components.

An example of this kind of code is below:

trait MyComponentTrait {
    fn get_state(&self) -> &dyn MyState;
}

fn entity_get_state<T: MyComponentTrait>(ent: EntityRef<'_>) -> Option<Ref<'_, dyn MyState>> {
    let component = ent.get::<&'_ T>();
    component.map(|comp_ref| Ref::map(comp_ref, |comp| comp.get_state()))
}

struct MyComponentInstance {
    // state unspecified...
}

impl MyComponentTrait for MyComponmentInstance {
    // implementation unspecified...
}

let mut comp_ty_to_state_getter: HashMap<TypeId, fn(EntityRef<'_>) -> Option<Ref<'_, dyn MyState>> = Default::default();
comp_ty_to_state_getter.insert(TypeId::of::<MyComponmentInstance>(), entity_get_state::<MyComponmentInstance>);

From there, when you have an entity, you could iterate over the TypeIds of the components in that entity, and then call use the comp_ty_to_state_getter to call common functionality on all of them. Examples of where this could be useful include serialisation, or visiting component fields to render them in a GUI inspector.

As part of this change, the sizes of the Ref and RefMut types have increased to store the TypeId of the original component, and the Archetype can now be released polymorphically using that TypeId.

Let me know if there is anything else I should change or if there are any tests I should add.

Ralith commented 1 year ago

Thanks for the PR! I agree that this API is well motivated. Because it should be impossible to construct a Ref or RefMut with an incorrect state field wrt. its archetype, I think we should skip the extra TypeId and instead add an unsafe internal API to Archetype to borrow/release by state index.

burtonageo commented 1 year ago

Done

burtonageo commented 1 year ago

I'd also like to rebase this onto https://github.com/Ralith/hecs/pull/315 when that's approved, as the Debug impls could be expanded to require ?Sized + Debug

Ralith commented 1 year ago

I like the refactoring, but bundling it together with the API extension will make this take longer to review. You might also want to rebase to fix the conflict.

burtonageo commented 1 year ago

Rebased!

Sorry about the extra refactoring. I decided to add a test, which exposed a bug in the impl of Ref::clone(), where A. The #[derive(Clone)] was putting an extra requirement that impl<T: Clone> Clone for Ref<T> which was unnecessary, and also not incrementing the borrow ref counts in the archetype for immutable borrows of the component.

Therefore, I decided to refactor the borrowing permissions into the ComponentBorrow and ComponentBorrowMut types, so that these could manage the borrowing lifetime from the Archetype, and then the Ref and RefMut types could focus on being useful wrappers over the NonNull<T>. This fixed the case in test where the extra clone of the Ref<T> would cause a panic due to imbalanced references, and it also simplified the {Ref, RefMut}::map implementations, as the borrow could be passed to the new Ref, rather than having to forget the old ref.

Thanks for your patience, please lmk if there is anything else I can do. I think this should be everything I want to change in this PR.

burtonageo commented 1 year ago

Comments all (hopefully) addressed

Ralith commented 1 year ago

Published in 0.10.3.