Serialization and deserialization of the world changes world behaviour.

[Oberdiah]

Below is a failing test that demonstrates the bug, using bincode for serialization/deserialization. Serde JSON exhibits the same behaviour:

#[derive(Clone, Copy, Debug, Serialize, Deserialize, PartialEq, Eq)]
pub struct TestComponent();
pub type TestWorldRegistry = Registry!(TestComponent);
#[test]
pub fn test_serialize_world() {
	let test_component = TestComponent();

	let mut world = World::<TestWorldRegistry>::new();
	world.insert(entity!(test_component));

	let serialized = bincode::serialize(&world).unwrap();
	let mut deserialized_world: World<TestWorldRegistry> = bincode::deserialize(&serialized).unwrap();

	deserialized_world.insert(entity!(test_component));
	world.insert(entity!(test_component));

	assert_eq!(world, deserialized_world); // This should pass, but does not.
}

The deserialized world debug prints to the following:

World {
    archetypes: {
        [
            "TestComponent",
        ]: {
            0: Row {
                identifier: Identifier {
                    index: 0,
                    generation: 0,
                },
                components: {
                    "TestComponent": TestComponent,
                },
            },
        },
        [
            "TestComponent",
        ]: {
            0: Row {
                identifier: Identifier {
                    index: 1,
                    generation: 0,
                },
                components: {
                    "TestComponent": TestComponent,
                },
            },
        },
    },
    entity_allocator: Allocator {
        slots: [
            Slot {
                generation: 0,
                location: Some(
                    Location {
                        identifier: [
                            "TestComponent",
                        ],
                        index: 0,
                    },
                ),
            },
            Slot {
                generation: 0,
                location: Some(
                    Location {
                        identifier: [
                            "TestComponent",
                        ],
                        index: 0,
                    },
                ),
            },
        ],
        free: [],
    },
    len: 2,
    resources: {},
}

Which is definitely incorrect - there should never be two archetypes containing exactly the same components.

[Oberdiah]

Upon further inspection, it appears as if the issue stems from the fact that archetypes use the hashed pointer address of their identifiers as a shorthand to represent their component list, so if the world is duplicated like this these pointer addresses change, and so the component set is not recognised as identical to one already in the archetypes list. This doesn't seem like it's going to be an easy fix :/

[Anders429]

@Oberdiah thanks for raising this issue! This is indeed a bug, and your minimal example was super helpful. I've been able to write a similar version of your example as a unit test.

You're right that the issue is with the hashed pointer addresses. If the identifiers were hashed simply by their values, this wouldn't be an issue, but the hashing is done by pointer address for optimization purposes.

But there are actually two additional tables for looking up the identifiers owned by the archetypes stored in Archetypes: type_id_lookup, which maps from the TypeId of an entity to its associated identifier, and foreign_identifier_lookup, which maps from the internal slice of an identifier to the owned identifier. In the case of deserializing, this allows creating new identifiers and ensuring they don't already have an identical identifier in the archetype. You can see that in action here:

brood/src/archetypes/impl_serde.rs

Lines 87 to 93 in ec18ed4

	if let Err(archetype) = archetypes.insert(archetype) {
	return Err(de::Error::custom(format_args!(
	"non-unique `Identifier` {:?}, expected {}",
	// SAFETY: This identifier will not outlive the archetype.
	unsafe { archetype.identifier() },
	(&self as &dyn Expected)
	)));

. That means that during deserialization, we are guaranteed to only have one archetype per unique set of components.

So the issue is coming when we are inserting a new entity, and it attempts to look up the archetype. This calls get_mut_or_insert_new_for_entity() (here:

brood/src/world/mod.rs

Line 219 in ec18ed4

.get_mut_or_insert_new_for_entity::<<Registry as contains::entity::Sealed<Entity, Indices>>::Canonical, <Registry as contains::entity::Sealed<Entity, Indices>>::CanonicalContainments>()

), so I suspect our problem is there. There are three ways we insert new archetypes into Archetypes:

• get_mut_or_insert_new_for_entity()
• get_mut_or_insert_new()
• insert()

The one used for deserialization is insert(). It checks foreign_identifier_lookup, and if there is an identifier there it returns its Archetype. Otherwise, it creates a new Archetype, stores it, and makes a mapping in foreign_identifier_lookup.

But looking at get_mut_or_insert_new_for_entity(), I see the issue: after failing to find in type_id_lookup, it neglects to check foreign_identifier_lookup, instead just skipping straight to using the hash of the created identifier (here:

brood/src/archetypes/mod.rs

Lines 221 to 227 in ec18ed4

	if let Some(archetype_bucket) = self.raw_archetypes.find(
	hash,
	Self::equivalent_identifier(
	// SAFETY: The `IdentifierRef` obtained here does not live longer than the
	// `identifier_buffer`.
	unsafe { identifier.as_ref() },
	),

), which will never find anything because the identifier was just created and therefore has a unique pointer location. So the solution is to instead use the foreign_identifier_lookup here, as that will actually find the existing archetype if there is one.

Looks like this was accidentally introduced in this commit: 59b4b22. I didn't account for the fact that lookup in the else statement would no longer find the archetype.

I've just pushed a fix to this branch, and I'll get a PR merged and a patch released to crates.io ASAP. Thanks again for raising this :)

[Oberdiah]

Blimey, that was fast! Once again, thanks for your work on this crate, it’s really such a godsend for my hobby project.

[Anders429]

No problem, I'm glad to help! I've just published the fix as version 0.9.1 on crates.io, let me know if you run into any other issues :)