Fix PartialReflect::apply for maps, remove get_at/_mut from Map trait

[Azorlogh]

Objective

• Fixes Remove get_at method from Map trait (reflection) #14328
• DynamicMap::drain was broken (indices weren't cleared, causing a panic when reading later)
• PartialReflect::apply was broken for maps and sets, because they don't remove entries from the self map that aren't in the applied map.
  • I discovered this bug when implementing MapEntities on a Component containing a HashMap<Entity, _>. Because apply is used to reapply the changes to the reflected map, the map ended up littered with a ton of outdated entries.

Solution

• Remove the separate Vec in DynamicMap and use the HashTable directly, like it is in DynamicSet.
• Replace MapIter by Box<dyn Iterator> (like for DynamicSet), and Map::get_at and Map::get_at_mut which are now unused.
• Now assume DynamicMap types are unordered and adjust documentation accordingly.
• Fix documentation of DynamicSet (ordered -> unordered)
• Added Map::retain and Set::retain, and use them to remove excess entries in PartialReflect::apply implementations.

Testing

• Added map::tests::apply and set::tests::apply to validate <DynamicMap as PartialReflect>::apply and <DynamicSet as PartialReflect>::apply

[alice-i-cecile]

@RobWalt, can I get your review here too?

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[alice-i-cecile]

I'm broadly happy with this, but it needs a migration guide :)

[RobWalt]

Looks great! Also really nice docs 💯 It was a pleasant review :)

[RobWalt]

Nice catch! How did this not lead to a failed test before? o.O

I mean it kind of makes sense that this code

        let mut map = DynamicMap::default();
        map.insert(2usize, 3i8);
        map.insert(3usize, 4i8);
        foo_patch.insert("d", map);

replaces the old map completely and hence doesn't include the (1,1) entry. The change here makes me wonder how it worked before though. The test makes it seem like previously the (1,1) entry would have been preserved.

I probably need to look a bit further into the changes..

[RobWalt]

Ahhh I guess that's where the removed entry (1,1) comes from.

Yeah I also second @alice-i-cecile message. We should really highlight this in a migration guide as, if you're used to the previous behavior, it seems like a big trip wire since it's so implicit.

[Azorlogh]

Thanks for the review! I added the migration guide with all the behavior changes.
I would consider the non-removal of the extra entry a bug so hopefully no one is actually relying on that in the wild! 😅