Prevent creation of superfluous empty table

[omaskery]

Objective

• To fix a tiny bug in bevy_ecs::storage::Tables that, in one case, means it accidentally allocates an additional "empty" Table, resulting in two "empty" Tables:
  • The one pre-allocated empty table at index 0 whose index is designed to match up with TableId::empty()
  • One extra empty table, at some non-0 index, that does not match up with TableId::empty().
• This PR aims to prevent this extraneous Table, ensuring that entities with no components in table-storage reliably have their archetype's table ID be equal to TableId::empty().

Solution

Background

The issue occurs because:

• Tables contains:
  • tables: Vec<Table> - The set of all Tables allocated in the world.
  • table_ids: HashMap<Box<[ComponentId]>, TableId> - An index to rapidly lookup the Table in tables by a set of ComponentIds.
• When Tables is constructed it pre-populates the tables Vec with an empty Table.
  • This ensures that the first entry (index 0) is always the Table for entities with no components in table storage.
  • In particular, TableId::empty() is a utility that returns a TableId of 0.
• However, the table_ids map is not initialised to associate an empty [ComponentId] with TableId 0.
• This means, the first time a structural change tries to access a Table for an archetype with 0 table components:
  • Tables::get_id_or_insert is used to retrieve the target Table
  • The function attempts to lookup the entry in the table_ids HashMap whose key is the empty ComponentId set
  • The empty Table created at startup won't be found, because it was never inserted into table_ids
  • It will instead create a new table, insert it into the HashMap (preventing further instances of this issue), and return it.

Changes

• I considered simply initialising the table_ids HashMap to know about the pre-allocated Table
• However, I ended up using the proposed solution discussed on Discord #ecs-dev:
  • Make Tables::get_id_or_insert simply early-exit if the requested component_ids was empty.
  • This avoids unnecessarily hashing the empty slice and looking it up in the HashMap.
  • The table_ids HashMap is not exposed outside this struct, and is only used within get_id_or_insert, so it seems wasteful to defensively populate it with the empty Table.

Testing

This is my first Bevy contribution, so I don't really know the processes that well. That said:

• I have introduced a little test that exercises the original issue and shows that it is now resolved.
• I have run the bevy_ecs tests locally, so I have reasonable confidence I haven't broken that.
• I haven't run any further test suites, mostly as when I tried to run test suites for the whole project it filled my entire SSD with >600GB of target directory output 😱😱😱

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

Good find, and good tests! Very weird about the cargo test output!

[Victoronz]

Looks good!