bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.75k stars 3.53k forks source link

Remove `get_at` method from `Map` trait (reflection) #14328

Open alice-i-cecile opened 3 months ago

alice-i-cecile commented 3 months ago

I also feel like this doesn't belong in there. I was mainly just replicating the Map trait which also has this. Is there a better explanation for the existence of this method in Map?

_Originally posted by @RobWalt in https://github.com/bevyengine/bevy/pull/13014#discussion_r1677490084_

By default, map-like data structures aren't ordered. If they're also ordered, they should just implement List as well.

RobWalt commented 3 months ago

I had some time to properly look into this. It seems like the whole reason this exists is that we impl a struct MapIter for iteration which just keeps the dyn Map and an index around and uses get_at in the Iterator impl of SetIter. Here's the relevant code:

/// An iterator over the key-value pairs of a [`Map`].
pub struct MapIter<'a> {
    map: &'a dyn Map,
    index: usize,
}

...

  fn next(&mut self) -> Option<Self::Item> {
      let value = self.map.get_at(self.index);
      self.index += value.is_some() as usize;
      value
  }

...

Maybe there's a better way to implement this by now. Git tells me the code was last touched 2020 which should be pre-GATS so maybe that's an approach?

I also investigated a bit how this iter for HashMap is implemented in the std lib. The std-lib uses hashbrown which has it's own pretty complex and unsafe implementation of the iterator. C.f.

https://docs.rs/hashbrown/latest/src/hashbrown/raw/mod.rs.html#2295

soqb commented 2 months ago

yeah i ran into this the other day and forgot to make a note. i think the reflection traits should return Box<dyn Iterator>. i'll make a PR if i have time.

RobWalt commented 2 months ago

@soqb I experimented with your idea for a work-in-progress PR that is also affected. Is this what you had envisioned? https://github.com/bevyengine/bevy/commit/600765bf30325818e4b695d67a76fa8f6a54352f#diff-24f58bdc4ff7b1265a9d17a76b01970afbd3265f198f1735f0497955235b68d5R57

soqb commented 2 months ago

yes! that's exactly what i mean.