Add VisitEntities for generic and reflectable Entity iteration

[jrobsonchase]

Objective

• Provide a generic and reflectable way to iterate over contained entities

Solution

Adds two new traits:

• VisitEntities: Reflectable iteration, accepts a closure rather than producing an iterator. Implemented by default for IntoIterator implementing types. A proc macro is also provided.
• A Mut variant of the above. Its derive macro uses the same field attribute to avoid repetition.

Testing

Added a test for VisitEntities that also transitively tests its derive macro as well as the default MapEntities impl.

[alice-i-cecile]

@cBournhonesque @Shatur I'd appreciate your opinions on which of these PRs you prefer.

[Shatur]

Thanks for the ping!
Not really like the idea of deriving two traits (opposing to one) manually for all my components with entities.
But #15422 looks good!

[alice-i-cecile]

Can we borrow from Reflect and have the derive macro do both traits? A bit cheaty, but IMO worth it.

[Shatur]

That would be the best approach!

[Shatur]

But I think that we need to try to implement it first to see if it's possible and after that rework the MapEntities trait.

[jrobsonchase]

Not really like the idea of deriving two traits (opposing to one) manually for all my components with entities.

Yeah, I hear this. It's not my favorite either. A couple of alternatives to consider with some pros and cons:

• Remove the supertrait bounds from the *Mut traits
  Pro: You only need one trait to get the current MapEntities behavior. Also, the read-only variant isn't actually needed for the *Mut traits, so maybe the bound doesn't belong in the first place.
  Con: The read-only trait is now ignorable, even though it's useful in contexts where you don't have or want mutable access to the component, like in Add generic "relations" iteration to bevy_hierarchy #15426.
• Combine the read-only/*Mut traits into one
  Pro: One trait 🎉
  Con: Makes it impossible to use with read-only access, which rules this option out completely in my mind Edit: nvm, this makes no sense. Not sure where my head was.

I think removing the supertrait bound is the best immediate compromise, since the read-only map/iterator traits are strictly new functionality and are unnecessary for previous behavior.

The derive idea sounds interesting, but I'm not sure how it would work in practice. I think there's too much type-knowledge needed to implement the IntoIterator traits automatically/ergonomically, but the IterEntities traits would probably be easier. I'll take a crack at it when I get a chance.

[jrobsonchase]

Ok, I've brought in the changes from #15426, but hidden the fact that the descendant/ancestor iterators are now implemented via the generic RelatedIter because, while neat, I'm not sure it's ready to be discussed as a part of the public API.

[Shatur]

I'm sorry if it wasn't clear, but in my opinion we need to ensure that it's possible to use iteration with entity mapping.
Having them as unrelated features may result in future redesigns. We need to architecture it right.

[jrobsonchase]

in my opinion we need to ensure that it's possible to use iteration with entity mapping.

It isn't. At least not while also supporting types like HashSet. The only way you could implement VisitEntitiesMut for a HashSet would be to hide a full rebuild of the set behind the visitor, which feels very wrong. So the current overall shape of MapEntities needs to stay. It would still need to rebuild the HashSet, but at least it doesn't pretend to do so in-place with mutable references to entities.

We could bring back VisitEntitiesMut and have a blanket impl for MapEntities that covers everything-but-HashSet, but then we're back in the state where you would derive/implement one trait and reflect another.

I'm about ready to shelve/close this PR for now. It's only served as a distraction from getting #15422 merged, which solves a much more real problem.

[Shatur]

At least not while also supporting types like HashSet.

Yes, I understand the problem.

We could bring back VisitEntitiesMut and have a blanket impl for MapEntities that covers everything-but-HashSet, but then we're back in the state where you would derive/implement one trait and reflect another.

And that's exactly what I suggesting! Yes, it's not nice to derive one trait and reflect another, but if it lets us avoid manually implementing MapEntities (in most cases) - it's totally worth it. Again, it's just my opinion. Feel free to oppose or suggest alternatives.

I'm about ready to shelve/close this PR for now.

That's up to you. Sometimes adding changes requires a bit of discussion/iteration. When it's a big project like Bevy, it's better to try to architecture things properly. It's impossible to avoid having breaking changes at all, but it's good to try to minimize it. Sorry if it demotivated you a little.

[jrobsonchase]

What's demotivating is that we've more or less been in the state you're now asking for as of roughly this commit.

I then took the "too many traits, only keep what's needed" feedback and stated my plan to decouple this change from MapEntities entirely and put us more or less in the current state of the branch. That got a 👍 before I made the change.

Then, the feedback was "too standalone, needs to be used elsewhere in the engine. Either bring in changes from #15426 or the blanket MapEntities impl." I acquiesced and brought in the #15426 changes, only to be told that you didn't actually want that, and really wanted the MapEntities blanket impl back.

What's more, the VisitEntitiesMut trait/derive/reflect and blanket impl for MapEntities on it will have nothing at all to do with the read-only VisitEntites trait/derive/reflect. They'll be similar, but unrelated changes. I don't think our goals are aligned.

[Shatur]

I then took the "too many traits, only keep what's needed" feedback

I just asked here your opinion about keeping only a single trait for accessing entities. I suggested a wrong one since we can't reflect IterEntities, but I didn't mean making this change unrelated to MapEntities. And then I asked it again here.

and #15425 (comment) to decouple this change from MapEntities entirely and put us more or less in the current state of the branch. That got a 👍 before I made the change.

Sorry for this. Not sure about @alice-i-cecile, but I get your plan wrong. It's my fault, I should have read it more carefully :(

Then, the feedback was "too standalone, needs to be used elsewhere in the engine.

This isn't what I meant; there was probably a miscommunication. Yes, a PR like this is more likely to be merged when combined with this PR, but I believe we need to utilize visiting for use in mapping. This is what I was trying to explain here. You said that the change should stand alone. I disagreed here and insisted that it should be used with mappings, explaining why. But you brought that changes from #15426.
I'm not a native speaker, so I'm very sorry if what I say is not clear.

What's more, the VisitEntitiesMut trait/derive/reflect and blanket impl for MapEntities on it will have nothing at all to do with the read-only VisitEntites trait/derive/reflect.

This is true, but I think that we need to introduce both in a single PR. Just to ensure that we won't need to rewrite it later. Sometimes you see issues after the implementation. Like the problem with HashMap/HashSet. Again, I just expressing the opinion.

[jrobsonchase]

OK, so if I bring back VisitEntitiesMut, a #[derive(VisitEntitiesMut)], a #[reflect(VisitEntitiesMut)], a blanket impl<T: VisitEntitiesMut> MapEntities for T {}, and back out the extra hierarchy changes I brought in, can we move forward?

[Shatur]

Pretty much, yes!

[Shatur]

LGTM!