NSSTC / sim-ecs

Batteries included TypeScript ECS
https://nsstc.github.io/sim-ecs/
Mozilla Public License 2.0
85 stars 12 forks source link

de-/serializers should be able to handle (references to) entities #19

Closed minecrawler closed 2 years ago

minecrawler commented 4 years ago

De-/serializers must be able to handle Entity fields. One common case where this becomes important is when creating a hierarchy. The usual strategy is to use a Parent component, which is a wrapper with a reference to an entity in the same world (see Amethyst and Bevy for reference). So, sim-ecs needs to store that reference and re-create it on load.

// example of a Parent component
class Parent {
  constructor(public entity: IEntity) {}
}

My current idea is to give each entity an id (could be a UUID or counter) which enables the implementation of such a feature and leave the implementation to the user, since they will likely create their custom components with entity fields.

I am open to opinions, though.

Everston commented 2 years ago

I've been exploring sim-ecs for a week now and I'm really loving its feature set. I checked out all of the ecs systems available in the performance lists noctjs/ecs-benchmark and ddmills/js-ecs-benchmarks to look for something that had support for js objects as components and built in serialization. geotic was the only library that matched this criteria but the lack of typescript support made it a nonstarter. I ended up finding sim-ecs through sheer chance while googling for "typescript ecs" and it's been amazing outside of the lack of entity id serialization.

I think this issue of serializing and de-serializing entity references is a convenience feature that's obscuring larger issues.

Creating a default entity id generator

The implementation of entity ids feels indecisive. You're forced to supply your own id generator to start using entity ids which means you have entity id defined as an optional property in Typescript. When you actually want to use these ids you have to use non-null assertions (entity.id!) across the codebase. An incrementing counter (converted to a string) could work as a default generator if you don't want to use a uuid library as a dependency. This would alleviate the non-null assertion issue as well as lay the groundwork for entity id serialization.

As a side note, I also think the getEntity should also be available in the same places as getEntities as it feels hacky using it as a global import.

Serializing/Deserializing Entity id

It's comfortable to reference entities in components by storing their ids.

class MyComponent {
  constructor(public friendEntityId: string) {}
}

When creating components like this you can use getEntity(myComponent.friendEntityId) to obtain the reference to that entity.

It was disappointing to see that sims-ecs wasn't able to serialize and de-serialize entity ids at all. Simply adding that functionality would enable this common use case to be viable and makes the issue of serializing actual entity references both easier to consider and less of a necessity.

minecrawler commented 2 years ago

Hi and welcome @Everston ! Thank you for your ideas.

An incrementing counter (converted to a string) could work as a default generator

There were reasons why I didn't want to add a fallback, but I cannot think of them anymore. In the current state, a simple counter might be a good fallback and I'll make it available, soon. Entity IDs are important for a lot of things with different requirements, though, so I won't remove the possibility to change the generator.

As a side note, I also think the getEntity should also be available in the same places as getEntities as it feels hacky using it as a global import.

The rationale here is that UUIDs are globally unique (as the name suggests), so there should be a global way to get a registered entity. getEntities() is part of the API of IPartialWorld, which would tie getting an entity to a single world. What if it's not in there, but was moved to another world, or removed from all worlds? I'm not entirely settled on if adding getEntity() to IPartialWorld is a good idea or not, since it may be confusing.

It was disappointing to see that sims-ecs wasn't able to serialize and de-serialize entity ids at all. Simply adding that functionality would enable this common use case to be viable and makes the issue of serializing actual entity references both easier to consider and less of a necessity.

Since I didn't need that feature, yet, I just left it in the hope someone would implement it or I would when I needed it myself. As you can see, I planned it for the 0.6 release - so it's upcoming. If this is a blocker for you, I can try to dedicate some time to implement it early, or you can open a PR anytime 😀

minecrawler commented 2 years ago

Hi @Everston , I just added reference serialization for Entities. You can check out the changes on the master branch. Is it working for you?

Also, entities now have IDs by default, with a counter-based approach as default ID-function. I needed it for the serialization anyway.

Everston commented 2 years ago

@minecrawler Thanks a lot for doing that. I don't have a ton of time today to experiment with it but I'll definitely check it out tomorrow. I was poking around the repository based on the changes you made a couple of days ago and did something a little different by purely storing/restoring the entity id. I'll see if it's worth making a pull request depending on how it fits into your changes today.

https://github.com/Everston/sim-ecs/tree/feature/serde-entity-id

minecrawler commented 2 years ago

@Everston I should add, though, that there are more stories concerning the save file, so the current implementation may not yield the final format for v0.6. I will try to get v0.6 stories closed and create the release.

If you feel like there are more blockers, necessary changes or simply feature requests, feel free to open separate GitHub Issues, so I can plan them better :)

Everston commented 2 years ago

@minecrawler Alright, sounds good.

I was trying out the new changes on the master branch and ran into a problem where a component created from a third party library using custom serialization/deserialization no longer works.

https://codesandbox.io/s/sim-ecs-third-party-serde-example-v1vp8 This is a sandbox example of it working in v0.5.1. It's a modified counter example with a second component created using the mobx-keystone library. It saves the world on one interval and loads it on another interval and outputs to the console. You'll need to download the sandbox and utilize the master branch instead of v0.5.1 to see it fail to deserialize the third party component.

minecrawler commented 2 years ago

@Everston I'll take a look at it and fix it

minecrawler commented 2 years ago

@Everston I published master onto npm as alpha version. Then I forked your code example and upgraded it to the new version. It seems to work for me. Please take a look at my code. Did you maybe not upgrade the return value of the custom deserializer? The deserializer now needs to return an object with a specific signature:

  .withComponent(ThirdPartyComponent, {
    serDe: {
      deserializer: (ic: any) => ({
        containsRefs: false,
        data: fromSnapshot(ic),
      }),
      serializer: (ic: any) => getSnapshot(ic)
    }
  })

This is in order to limit reference resolution to only components which actually do have some. Going over all components and all of their nested fields is rather slow, and I didn't find any good way to get branch info from the JSON parser. For custom deserialization, you probably can always say false here.

Everston commented 2 years ago

Thanks, that was indeed the problem. I forgot that fromSnapshot pulls any for the type if it's unspecified which was the case because I was re-using the custom serDe definition across multiple components.

minecrawler commented 2 years ago

I'll close this as implemented for now, then. If there are bugs, just let me know :)