POC Migrate rmf_site_format out of RefTrait #338
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New feature implementation
Implemented feature
This PR removes the
RefTraitgeneric throughout the codebase.Opening for visibility and early feedback if the new behavior / API does not look desirable. The migration is finished but there are potential followups that are not addressed yet (i.e. using scenes for loading / saving)
The bulk of the changes are migration to remove the generic, the only new implementation is SiteID
Implementation description
Previously,
RefTraitwas used to allow a generic implementation of sites where identifiers could either be:Entityused in the bulk of the Bevy implementation and allows, among others, referencing other entities through mechanisms likePoint(Entity).u32mainly used in serialization, deserialization and to allow a human readable ID (in the inspector widget).In this PR, I am proposing using a newtyped
Entity, specifically changingSiteID(u32)toSiteID(Entity)as an identifier.Since this struct wraps an
Entity, it can be used to reference entities in a Bevy world (unlikeu32), the conversion back and forth betweenu32andSiteIDjust uses the entity index, specifically using from_raw foru32 -> Entityand index forEntity -> u32.The main case we have for our
u32is serialization and deserialization of the world. What's nifty is that Bevy guarantees that, in a certain snapshot of the world, there will never be two entities with the same index, so we can be sure that there will be no collisions when "downcasting" the 64 bit entities to a 32 bit identifier.I also introduced a custom serialization / deserialization that effectively only uses the index so the new struct really behaves like a u32 wrapper, when saving and loading is involved.
What are the pros?
Well we can stop having this generic throughout the codebase, but it doesn't end there.
I have been experimenting with using Bevy's Scene to load / save the world with a good degree of success. If we spawn our site as a
Sceneand all our references are throughEntity, we can use theMapEntitiestrait by marking fields as #[entity], Bevy will automatically translate references in the scene to references in a newly spawned world. We could potentially remove the need to create our own hash map at site loading time.We will also not need to manually assign site IDs since the site ID will just be the current Entity's
index, so we can get rid of a fair bit of logic at saving time.What are the cons?
The biggest con is that relying on entity values, rather than manually updated IDs, will mean that the values might change in different runs, resulting in less stable IDs, which will result in, for example, larger diffs if the saved files are tracked in version control. I am not currently aware of a way to preserve ID stability in a simple way.
Furthermore, the identifier values will not be "as human readable". Because they are entity indexes and there are a lot of entities spawned throughout the world, the values will be generally more sparse and higher.
However I would argue that the SiteID logic was fairly rudimentary, and a simpler implementation of what Entities do with the potential flaw of not being able to reuse SiteIDs that were despawned between versions of a site as is clear here.
So the very first saved site will have nice and simple contiguous IDs, but as items that are serialized / deserialized get despawned and respawned the SiteIDs will get increasingly sparse.
Now we could fix this by doing some book-keeping but it really sounds like what Bevy is already doing behind the scenes with generational indexes, so there is no point for us to implement it again.
Another minor con is that this will force rmf_site_format to have
bevy, or specificallybevy_ecsas a dependency. I tried this out andbevy_ecsis a very small crate that brings very few dependencies so the impact on compile time is negligible.Why not just use Entity as identifier?
While possible in theory, using
Entityas an identifier has a few pitfalls:GenAI Use
We follow OSRA's policy on GenAI tools