Feedback from the merged experimental branch

[aclysma]

This is a continuation of feedback given in #115 before it was merged. Some of this is new and some of it I'm just bringing over from the PR and previous discord chats so it's all in one place.

The main issue currently is that deserializing with the non-human-readable path fails.

• I was able to hard-code legion to always take the human-readable path (even when using bincode) and I was able to serialize a world and then deserialize it.
• When I hard-code to take the non-human-readable path, serialization/deserialization fails, despite working before.
• I did this with RON as I think it will be easier to figure out than with bincode. The error was "Message("84:29: Expected Array")", this is what it was reading: expected_array_84_29.txt

I wasn't able to sufficiently test clone_merge yet. It was time-consuming to update my code to compile and took a while to figure out the serialization issue.

Other issues:

• When deserializing prefabs or copying entities from one world to another, I'm having to make tempories on the stack and then copy them into legion storage. Before, these operations were in-place and did not need this copy. In some cases when dealing with multiple components (like in deserialization) this also requires heap allocations to build a Vec to pass into extend_memcopy_raw
• Could we make World::universe() public?
• For entity merging and serialization, I think the old APIs were much "lighter touch" and less opinionated than the current ones. In particular, I don't like that entity/UUID mappings are persisted for the lifetime of the universe. This means accumulated wasted memory if you were moving in a large, streamed environment. The previous APIs accomplished this without extra copies and without a continuously growing entity/UUID map.
• (I still think the loss of tags is unfortunate)

[TomGillen]

Looking just at that RON file, it looks like it is the component itself which is failing to deserialize (trying to read the tuple as an array). However, I don't see how that would behave any differently based upon the format of the surrounding world data, so this is unlikely to be the case. Do you have a link to the code somewhere?

When deserializing prefabs or copying entities from one world to another, I'm having to make tempories on the stack and then copy them into legion storage. Before, these operations were in-place and did not need this copy.

You need to do this when deserializing, because at some point you need to delegate to the component's deserialize serde impl, which will give you a T on the stack. The deserialize impl of Registry then memcopys this into the world. Serde always constructs its data on the stack, there is no way to ask it to construct its result at an arbitrary pre-allocated memory location. The code for this that is currently in master is based upon @kabergstrom 's serialization code, and this part functions just the same.

As for merging, you should not need to do any temp copies. The Merger impl is given access to the source and destination component storage and can move data directly between the two. The out-of-box Duplicate impl of Merger can even memcopy all components straight from one world to the other if the types are Copy in one go. If the types are Clone or you are performing a type transformation, then it has to construct the component on the stack and move them one by one. You have to do this; clone returns T, it doesn't let you give it a ptr to write into.

In some cases when dealing with multiple components (like in deserialization) this also requires heap allocations to build a Vec to pass into extend_memcopy_raw

It is difficult to determine why you would need to do this without seeing what you are trying to do. Registry doesn't need to do this, and it was built to be able to do everything that I could see the legion-prefab code doing. The deserialization code (at least the non-human-readable format, which is the performance optimized one) uses a custom seq visitor to write components straight into the destination world as the data is deserialized without needing a temp Vec. It is entirely possible that I missed something which renders that type unusable for you, but that is why I wanted your feedback because it is likely easier to make upstream changes to make either the WorldSerializer and WorldDeserializer traits do what you need or to directly expose it on Registry, than it is for you to try and re-implement this logic yourself outside of the library. Registry exposes just about everything that the traits require in a configurable manor. But I can't fix that without knowing more specifics about why the current traits (or the Registry impl) don't do what you need. They were designed in reference to how legion-prefab works and are supposed to be able to replace much of its code.

In particular, I don't like that entity/UUID mappings are persisted for the lifetime of the universe. This means accumulated wasted memory if you were moving in a large, streamed environment. The previous APIs accomplished this without extra copies and without a continuously growing entity/UUID map.

The previous API didn't need to ensure that the same entity in the serialized data will always be assigned the same runtime ID because it did not perform ID rewriting in components to fix up ID references. We could allow you to remove ID mappings from the Universe, which would cause entities to be assigned a new runtime ID the next time they are encountered (either by being deserialized themselves, or by being referenced inside another entity which is being deserialized), but the only time this will be truely safe to do is if you know that the user is not holding on to the entity's current runtime ID anywhere. I am not sure how you would know that. The old code still suffered from this exact problem; it just handed the problem of fixing up runtime ID references to the user - which doesn't solve the issues and is actually impossible for the user to do robustly (clone and deserialize are the only opportunity anyone has to access Entity fields deep inside a component type).

I am not at all opposed to allowing you to tell the universe to "forget" an ID mapping if you somehow know that you no longer need runtime ID stability for that entity. However, this issue is not caused by the new serialization code, but rather by the new entity reference rewriting feature; a feature which was needed and cannot be performed anywhere but inside deserialize/clone.

Could we make World::universe() public?

...I'm not sure why it isn't already actually.

(I still think the loss of tags is unfortunate)

Yes, but I don't think the feature justified itself. The use cases were niche, new users found the distinction between components and tags confusing, virtually every time I saw someone using it they were using them wrong (to the point that it would significantly harm performance), and tags significantly increased the complexity of much of legion's code.

I am not ruling out the possibility of re-implementing tags (or something similar), but I think it needs more thought before we would go ahead with that.

[kabergstrom]

In particular, I don't like that entity/UUID mappings are persisted for the lifetime of the universe. This means accumulated wasted memory if you were moving in a large, streamed environment. The previous APIs accomplished this without extra copies and without a continuously growing entity/UUID map.

The previous API didn't need to ensure that the same entity in the serialized data will always be assigned the same runtime ID because it did not perform ID rewriting in components to fix up ID references. We could allow you to remove ID mappings from the Universe, which would cause entities to be assigned a new runtime ID the next time they are encountered (either by being deserialized themselves, or by being referenced inside another entity which is being deserialized), but the only time this will be truely safe to do is if you know that the user is not holding on to the entity's current runtime ID anywhere. I am not sure how you would know that. The old code still suffered from this exact problem; it just handed the problem of fixing up runtime ID references to the user - which doesn't solve the issues and is actually impossible for the user to do robustly (clone and deserialize are the only opportunity anyone has to access Entity fields deep inside a component type).

An option is to allow the user to provide a trait implementation for ID lookups in all relevant operations (clone/merge/serialize/deserialize), and set this up in a TLS var to robustly support the ID patching.

[TomGillen]

When deserializing, either the serialized entity ID has already been assigned a runtime ID (in which case you want to use that), or it hasn't (in which case you want the world to assign a new runtime ID). There aren't any other ways to correctly handle this.

For merging, there are a few reasons why you might want to control how IDs are assigned and re-mapped. The Merger trait allows this:

/// Indicates how the merger wishes entity IDs to be adjusted while cloning a world.
fn entity_map(&mut self) -> EntityRewrite {
    EntityRewrite::default()
}

Where EntityRewrite is defined as:

/// Describes how a merger wishes `Entity` references inside cloned components to be
/// rewritten.
pub enum EntityRewrite {
    /// Replace references to entities which have been cloned with the ID of their clone.
    /// May also provide a map of additional IDs to replace.
    Auto(Option<HashMap<Entity, Entity, EntityHasher>>),
    /// Replace IDs according to the given map.
    Explicit(HashMap<Entity, Entity, EntityHasher>),
}

I don't think Duplicate exposes this as something that is configurable at the moment, though.

[aclysma]

The primary goal is to ensure that the functionality mentioned here still works: https://community.amethyst.rs/t/atelier-legion-integration-demo/1352/6
This is implemented partially in the prefab crate: https://github.com/aclysma/prefab/tree/legion_refactor
And the more opinionated parts are here: https://github.com/aclysma/minimum/tree/legion-0.3

These crates are compiling as long as World::universe() is made public. And I have that temporary fix mentioned above to force legion to always use the human-readable serialization path.

You need to do this when deserializing, because at some point you need to delegate to the component's deserialize serde impl, which will give you a T on the stack.

I can see that the previous code was putting a T on the stack, which I didn't realize. However there is still an extra heap allocation here due to the Vec. (NOTE TO SELF: deserialize_single_fn has a Vec alloc that may be avoidable)

Old:
https://github.com/aclysma/prefab/blob/f6f10412a857fdb1b409b185965756fcfdc25466/legion-prefab/src/registration.rs#L332
https://github.com/aclysma/prefab/blob/f6f10412a857fdb1b409b185965756fcfdc25466/legion-prefab/src/registration.rs#L27

New:
https://github.com/aclysma/prefab/blob/245c8e181180b1de88185582fd324b09a64e093b/legion-prefab/src/registration.rs#L328

As for merging, you should not need to do any temp copies.

Comparing the old/new version, I think the old way would avoid copies and the new one might require them. (To be fair, both implementations somewhat require LLVM to do smart things, but the new implementation is an explicit memcpy and not passing ownership of a temporary.)

Old:
https://github.com/aclysma/prefab/blob/f6f10412a857fdb1b409b185965756fcfdc25466/legion-prefab/src/registration.rs#L409

New:
https://github.com/aclysma/prefab/blob/245c8e181180b1de88185582fd324b09a64e093b/legion-prefab/src/registration.rs#L421

New implementation is based on legion's implementation:
https://github.com/TomGillen/legion/blob/fe9203256ed60ca3e616d87b8dd90c44924e801f/src/internals/world.rs#L1170

In some cases when dealing with multiple components (like in deserialization) this also requires heap allocations to build a Vec to pass into extend_memcpy_raw

It is difficult to determine why you would need to do this without seeing what you are trying to do.

In the first example above (deserialize_comp_fn) I was calling extend_memcopy_raw passing all components being deserialized as a slice. I can probably change that to call extend_memcopy_raw N times passing a single component rather than once passing a slice of all deserialized components. So this may be avoidable.

...it is likely easier to make upstream changes to make either the WorldSerializer and WorldDeserializer traits do what you need or to directly expose it on Registry, than it is for you to try and re-implement this logic yourself outside of the library

While I appreciate that you are trying to solve some of these problems for us, I explicitly DON'T want to rely on legion's behavior for a lot of this for several reasons:

• It will result in related logic (diffing and serializing components) getting partially implemented in two different crates.
• It's very possible that the approach we are taking today in the prefab crate may need to be changed. If we rely on logic in legion to do these things, it could put us in a difficult position of forcing these changes on other people that may not want them.
• I'm concerned that it could also go the other way, that I'll have to deal with upstream changes that put me in the same boat I'm in now.. spending weeks dealing with upstream changes instead of making progress on my projects.

What I want out of legion is an unopinionated data store. I think "push everything upstream into legion" is unsustainable because the more high-level logic that legion provides, the more difficult it will be to keep everyone using it happy.

The previous API didn't need to ensure that the same entity in the serialized data will always be assigned the same runtime ID because it did not perform ID rewriting in components to fix up ID references.

I'm not convinced that this is something legion should be doing. Ideally, I think Entity would be non-serializable and a new type (which I would implement in the prefab crate) could do this using Entity lookups stashed in TLS. This would mean any component that meant to be serializable must use that particular type to reference the Entity. That's fine as I already support separate design-time and run-time representations. In fact this is a desirable feature as entities in a prefab shouldn't reference entities outside their own prefab.

This avoids the continuously growing UUID/Entity map and doesn't require locks. Legion as you said is not aware of lifetimes for these mappings to be able to clean them up when they are no longer needed, but my code definitely is and I've been managing them fine in legion 0.2.4.

[TomGillen]

I can see that the previous code was putting a T on the stack, which I didn't realize. However there is still an extra heap allocation here due to the Vec. (NOTE TO SELF: deserialize_single_fn has a Vec alloc that may be avoidable)

You really shouldn't need to allocate a Vec here. The deserializer trait looks like:

/// Describes a type which knows how to deserialize the components in a world.
pub trait WorldDeserializer: CanonSource {
    /// The stable type ID used to identify each component type in the serialized data.
    type TypeId: for<'de> Deserialize<'de>;

    /// Converts the serialized type ID into a runtime component type ID.
    fn unmap_id(&self, type_id: &Self::TypeId) -> Result<ComponentTypeId, UnknownType>;

    /// Adds the specified component to the given entity layout.
    fn register_component(&self, type_id: Self::TypeId, layout: &mut EntityLayout);

    /// Deserializes a component and inserts it into the given storage.
    fn deserialize_insert_component<'de, D: Deserializer<'de>>(
        &self,
        type_id: ComponentTypeId,
        storage: &mut dyn UnknownComponentStorage,
        arch_index: ArchetypeIndex,
        deserializer: D,
    ) -> Result<(), D::Error>;

    /// Deserializes a single component and returns it as a boxed u8 slice.
    fn deserialize_component<'de, D: Deserializer<'de>>(
        &self,
        type_id: ComponentTypeId,
        deserializer: D,
    ) -> Result<Box<[u8]>, D::Error>;
}

It has two deserialize fns her because the "packed" and "entities" layouts are different enough that it would perform poorly if I tried to use just one fn for both. However, both of these expect just one component to be deserialized here. The implementation internally uses a custom seq visitor to move the component directly into the world without an intermediate allocation (which is as fast as we will ever get with how serde works). I don't understand how you could get anything working if you are trying to run that loop yourself. This is certainly why the non human readable format isn't working for you.

What I want out of legion is an unopinionated data store. I think "push everything upstream into legion" is unsustainable because the more high-level logic that legion provides, the more difficult it will be to keep everyone using it happy.

Being able to serialize a world is core functionality that virtually all users of the library are going to need. The problem is that while the old traits were suitable for your use case (they were designed specifically for your use-case), they were totally useless for users directly (requiring hundreds of lines of fairly complex code), which nessessitates that another crate be written to provide the "actual" API. legion-prefab is much larger in scope and with far more opinions on what should be done than just providing world serialization, which means regardless of what you do with legion-prefab I am going to have to write my own serialization module/crate for legion which provides simple (for the user) out-of-box serialization.

If both of us write our own serialization logic for legion, then we will end up fragmenting both the ecosystem and developer effort writing two crates which significantly overlap in functionality.

I'm not convinced that this is something legion should be doing. Ideally, I think Entity would be non-serializable and a new type (which I would implement in the prefab crate) could do this using Entity lookups stashed in TLS. This would mean any component that meant to be serializable must use that particular type to reference the Entity. That's fine as I already support separate design-time and run-time representations. In fact this is a desirable feature as entities in a prefab shouldn't reference entities outside their own prefab.

That might work in your prefab system, but most people using legion might not be using legion-prefab. Requiring that all serializable components use something other than Entity to store entity IDs - and potentially requiring a world clone to perform component type conversions between runtime and offline types - is essentially doing the same thing as converting Entity to/from a UUID... but with many extra steps.

This avoids the continuously growing UUID/Entity map and doesn't require locks. Legion as you said is not aware of lifetimes for these mappings to be able to clean them up when they are no longer needed, but my code definitely is and I've been managing them fine in legion 0.2.4.

This was required when I had a stricter canon name concept, but I removed that a while back and now only use UUIDs as a stable offline ID and Entity as a runtime ID. When I made this change, though, I didn't go back and rethink how bits of this worked. I have just pushed changes which move the entity/uuid map out of the universe and the world (de)serializer traits are now responsible for providing it during serialization. In fact, I have just removed Universe entirely as that was the last thing it actually did, so it is now entirely purposeless.

[kabergstrom]

It has two deserialize fns her because the "packed" and "entities" layouts are different enough that it would perform poorly if I tried to use just one fn for both. However, both of these expect just one component to be deserialized here. The implementation internally uses a custom seq visitor to move the component directly into the world without an intermediate allocation (which is as fast as we will ever get with how serde works). I don't understand how you could get anything working if you are trying to run that loop yourself. This is certainly why the non human readable format isn't working for you.

Due to erased_serde and "vtable" requirements in the component registrations, there is an indirect call to invoke the deserialize_*_component functions for each component type. In my previous design, I therefore let the user-provided deserialize functions do the loop internally, since that's the only way to get the best performance in this very important hot loop. I suggest keeping this design for the packed layout, since you have two separate functions anyway.

Being able to serialize a world is core functionality that virtually all users of the library are going to need. The problem is that while the old traits were suitable for your use case (they were designed specifically for your use-case), they were totally useless for users directly (requiring hundreds of lines of fairly complex code), which nessessitates that another crate be written to provide the "actual" API. legion-prefab is much larger in scope and with far more opinions on what should be done than just providing world serialization, which means regardless of what you do with legion-prefab I am going to have to write my own serialization module/crate for legion which provides simple (for the user) out-of-box serialization.

The old traits were intended to be largely unopinionated and be something to build other crates on top of, indeed by design. An issue I see with building the UUID-as-entity-ID choice into the core serde support is that UUIDs can be inappropriate for use-cases where serialized size is important. This is why I delegated this choice to the user - to allow applications like real-time networking libraries to be more optimized for their use-case (networking will never be amazing due to serde design). I think providing an out-of-the-box solution is great though, and I applaud using UUIDs as the default.

If both of us write our own serialization logic for legion, then we will end up fragmenting both the ecosystem and developer effort writing two crates which significantly overlap in functionality.

I agree that we should avoid fragmentation.

I'm not convinced that this is something legion should be doing. Ideally, I think Entity would be non-serializable and a new type (which I would implement in the prefab crate) could do this using Entity lookups stashed in TLS. This would mean any component that meant to be serializable must use that particular type to reference the Entity. That's fine as I already support separate design-time and run-time representations. In fact this is a desirable feature as entities in a prefab shouldn't reference entities outside their own prefab.

That might work in your prefab system, but most people using legion might not be using legion-prefab. Requiring that all serializable components use something other than Entity to store entity IDs - and potentially requiring a world clone to perform component type conversions between runtime and offline types - is essentially doing the same thing as converting Entity to/from a UUID... but with many extra steps.

I think Entity should be serializable, but I would like the API to be less opinionated about providing an exact set of operations, and just give me a callback with all the data available. As an example of something weird: in atelier-assets I use the serde::Serialize implementation of Handle<T> to gather dependencies for an asset. Similarly, if a hook was provided for Entity clone/serialization I could clone/serialize a component to find out which entities it references.

This avoids the continuously growing UUID/Entity map and doesn't require locks. Legion as you said is not aware of lifetimes for these mappings to be able to clean them up when they are no longer needed, but my code definitely is and I've been managing them fine in legion 0.2.4.

This was required when I had a stricter canon name concept, but I removed that a while back and now only use UUIDs as a stable offline ID and Entity as a runtime ID. When I made this change, though, I didn't go back and rethink how bits of this worked. I have just pushed changes which move the entity/uuid map out of the universe and the world (de)serializer traits are now responsible for providing it during serialization. In fact, I have just removed Universe entirely as that was the last thing it actually did, so it is now entirely purposeless.

Sorry I haven't been keeping up with the recent commits. Feel free to poke when you have pushed important changes.

[TomGillen]

Due to erased_serde and "vtable" requirements in the component registrations, there is an indirect call to invoke the deserialize_*_component functions for each component type. In my previous design, I therefore let the user-provided deserialize functions do the loop internally, since that's the only way to get the best performance in this very important hot loop. I suggest keeping this design for the packed layout, since you have two separate functions anyway.

I suppose I could do that, but the cost difference between a fn call and a virtual fn call, when considering that the code being called is deserialization code which is typically also going to be invoking IO, is rather inconsequential. The more significant gain would be that the implementation is likely to perform a hashmap lookup, which could be moved out of the loop. Although, even then, given that ComponentTypeId can have essentially a no-op hash function, the cost here is still not all that large compared the the deserialization code itself.

The trade off for moving this loop into user code is that the user needs to write their own seq visitor to avoid a temp vec allocation.

The old traits were intended to be largely unopinionated and be something to build other crates on top of, indeed by design. An issue I see with building the UUID-as-entity-ID choice into the core serde support is that UUIDs can be inappropriate for use-cases where serialized size is important. This is why I delegated this choice to the user - to allow applications like real-time networking libraries to be more optimized for their use-case (networking will never be amazing due to serde design). I think providing an out-of-the-box solution is great though, and I applaud using UUIDs as the default.

This could perhaps be made configurable, but it would require the Serialize and Deserialize impl for Entity to erase the serde serializer types, and then to invoke a user supplied type in TLS (rather than directly using Canon) via another virtual call. This would be more expensive although, just as I said above, rather inconsequentially so. I'll have a look at this.

[kabergstrom]

I suppose I could do that, but the cost difference between a fn call and a virtual fn call, when considering that the code being called is deserialization code which is typically also going to be invoking IO, is rather inconsequential.

I think this is an outdated sentiment. IO is much faster than compute right now, and IO throughput is doubling every 2 years or so. Consumer NVMe PCIe 4 SSDs provide > 5GBps, datacenters commonly have >40Gbps on a single VM, trending towards 100Gbps. Kernel APIs are being improved to reduce IO overhead to near-zero (DMA into user-space for aligned buffers).

This is a decision that can mean the difference between an optimized hot loop that can deserialize more than 1 million entities per second, or an indirect call within it that can increase the fixed overhead which could limit throughput to less than 100k entities per second. It's a small change in the API with potentially large impact on throughput. Why bother have a packed layout if the data is not accessible to the user in a contiguous manner?

Although, even then, given that ComponentTypeId can have essentially a no-op hash function, the cost here is still not all that large compared the the deserialization code itself.

Deserializing a simple component with float/matrix fields basically boil down to memcopy from the serializer input stream with bincode. Bincode can easily do more than 300MB/s of deserialization for even complex structs. A hashmap lookup would take a significant percentage of the execution time.

The trade off for moving this loop into user code is that the user needs to write their own seq visitor to avoid a temp vec allocation.

It's a pain, yes, but aren't you already providing an implementation of WorldDeserializer for Registry to cover the out-of-the-box use-case?

This could perhaps be made configurable, but it would require the Serialize and Deserialize impl for Entity to erase the serde serializer types, and then to invoke a user supplied type in TLS (rather than directly using Canon) via another virtual call. This would be more expensive although, just as I said above, rather inconsequentially so. I'll have a look at this.

Since this could be a rather uncommon use-case, it's OK to hide this behind a feature flag. It can probably be made into a strictly additive feature. It will be rare to do in a production game, mostly in editor or tool code.

[aclysma]

I'd still like to get the prefab crate updated to work with the changes, but work is busier at the moment and I have less time available in the short term to work on it.

All the work so far has been pushed into branches linked in my earlier comments. The main issue now is that the Entity/UUID mapping code needs to be un-removed.

[TomGillen]

I am currently part way through rearranging the serialization code. I have managed to pull everything but the Entity serde impls (which simply proxy the actual impl to a trait object in TLS) out into a sub-crate which does not rely upon any internal APIs. This was much easier to do with 0.3 than it would have been in 0.2.

If you need to do something totally different, then, it should be easy to build an entirely separate serialization impl.

I do still hope that what is provided by legion should be capable of everything you need, though - but if you are worried about issues with evolving requirements or divergence between legion and legion-prefab, then there should always be this escape hatch available.

[TomGillen]

So the core library only provides hooks to choose how to serialize Entity, and this will be behind a feature flag if you want to disallow direct Entity struct serialization.

The Registry in legion's native serialization sub-crate allows (actually expects) the user to specify what serialized representation to use for Entity and for component type Ids.

Bypassing Registry, you can impl the world serialize traits to customise how components and slices of components are serialized, in addition to what Registry itself exposes.

[TomGillen]

@aclysma @kabergstrom Feeback in this? Legion should be even less opinionated on serialization than 0.2.3 was.

[kabergstrom]

@TomGillen That sounds really good - is master where I can look at your current progress to give more specific feedback?

[TomGillen]

Yea, I just pushed up some progress on this. The serialization module hasn't been moved into a subcrate yet, but the prep work for that has been done. I also changed the world serialization traits to move the component loop into the trait impl as you suggested.

[aclysma]

Sounds great, looking forward to taking a look soon!

[aclysma]

Very promising so far:

• A couple serialization functions are still unimplemented, but everything else is compiling (but not yet running)
• I think UnknownType will need to be visible.
• There's also a possibility I'll need to have access to UnknownComponentStorage in UnknownComponentWriter because UnknownComponentWriter has a lifetime parameter and I need to pass it through a dynamic-invoked trait function.

But again, so far this is looking like a huge improvement. Not needing to pass around a universe and not needing to worry if an entity has been initialized yet in canon is really great!

[TomGillen]

I think UnknownType will need to be visible.

Oh, yes, it certainly will. I need to do a proper review of what is exported as there are likely a few things that are not part of the main user-facing API but do need to be exported for those going a bit deeper.

There's also a possibility I'll need to have access to UnknownComponentStorage in UnknownComponentWriter because UnknownComponentWriter has a lifetime parameter and I need to pass it through a dynamic-invoked trait function.

That lifetime parameter is the lifetime of the reference to the UnknownComponentStorage. Surely you would still run into the same issue trying to pass the storage around directly? Any situation where you could elide the lifetime in the &mut dyn UnknownComponentStorage is one where you should also be able to elide the lifetime for UnknownComponentWriter. Registry does the dynamic dispatch here with function pointers, where we can omit these lifetimes.

[kabergstrom]

Hey @TomGillen I had a stab at making our prefab stuff work, and had to change a little. Could you have a look? Mainly it was about making the traits a bit more flexible, with fewer strict types like Mutex<Box<dyn EntitySerializer>>.
kabergstrom@28bc254
Serialization-related traits seem to need to take &self instead of &mut self because serde::Serialize takes &self, so I had to split off Canon's EntitySerializer impl into a CustomEntitySerializer, and I ended up moving the serialization-related parts out of that too, to make it simpler. It's really just a mapping for the serialized entity ID, and it might make more sense to actually specify which CustomEntitySerializer (please fix the naming 😢 ) to use at the World::serialize callsite rather than in the Registry type itself. Could open up some use-cases like different ID-spaces for networking vs file serialization, or something.

[aclysma]

Using @kabergstrom's changes I think I'm very close to having minimum up and running using the update prefab crate with legion 0.3. Hoping it will be good to go by end of weekend.

[TomGillen]

@kabergstrom I'll take a look at this later tonight. I never liked the rather horrible Mutex<Box<dyn EntitySerializer>>, and you may be right about separating entity serialization and component serialization.

[aclysma]

Good news, I still need to implement the serialize/deserialize slice, but I was able to load a prefab, modify it, undo, redo, play, reset, save, and hot reload. There is cleanup to do for sure, but it's almost done!

[aclysma]

I think everything is working now in the prefab crate and minimum. (there's a decent amount of functionality that is currently only exercised in minimum right now).

• Requires @kabergstrom's change
• Also made EntityHasher public

Compiling branches:

• Legion: https://github.com/aclysma/legion/tree/legion-0.3-prefab-support
• Prefab: https://github.com/aclysma/prefab/tree/legion_refactor
• Minimum: https://github.com/aclysma/minimum/tree/legion-0.3

[TomGillen]

@kabergstrom Do you think you could open a PR with your serialization changes?

[kabergstrom]

@TomGillen You mean without @aclysma 's change in his branch here: https://github.com/aclysma/legion/tree/legion-0.3-prefab-support ?

[TomGillen]

Ah, no we'll want that too, but that is a trivial change.

[kabergstrom]

Done @TomGillen #180