ecs icon indicating copy to clipboard operation
ecs copied to clipboard

Value type components

Open stackotter opened this issue 2 years ago • 8 comments

Description

This PR aims to address the current performance and thread-safety limitations of the ECS by switching to using value type components. This allows better memory usage patterns, better performance, easier thread safety, and opens up many possible alleys for improvement (such as creating a system scheduler that analyses read and write dependencies between systems to automatically run systems in parallel where possible).

This will also address issue #46.

Detailed Design

These changes will overhaul massive portions of the API and will likely warrant a bump of the major version.

Here are the main tasks to be completed:

  • [x] Remove AnyObject conformance from Component protocol
  • [ ] Update Nexus API to add methods for mutating components
  • [ ] Simplify the nexus API to enhance maintainability (there are lots of unnecessary methods that could probably be removed, what's your opinion on this @ctreffs? my main rationale behind simplifying the API is that it seems pretty difficult to maintain when there are so many methods to update)
  • [ ] Implement read-only and read-write families
  • [ ] Get the FSM API working again

Possible future optimisations

After implementing the basis of value type components there are many optimisations that can be implemented.

  • [ ] Allow families to have a mixture of read-only and read-write components
  • [ ] Create a System API to formalise how systems should be declared in a type-safe and thread-safe manner
  • [ ] Create a TickScheduler API which can run systems in a specified order and maybe even run systems in parallel when they are identified to have no common dependencies

Swift limitations

The main limitation that I have identified so far is that there isn't a way (that I know of) to restrict a protocol to only value types (unlike restricting to reference types which is possible with AnyObject. The only workaround I can see for this is putting advice all throughout the documentation saying to use value types for components. It technically shouldn't break the ECS if a component is a reference type, however it breaks a lot of thread-safety guarantees and would probably make for pretty confusing code.

Testing

Once I am finished, I will make sure that all the tests pass and, most importantly, are still valid with the new component semantics. Currently they all pass but they most definitely shouldn't.

Source Impact

This will break every single package which depends on the ECS.

Checklist

  • [x] I've read the Contribution Guidelines
  • [x] I've followed the coding style of the rest of the project.
  • [ ] I've added tests covering all new code paths my change adds to the project (to the extent possible).
  • [ ] I've added benchmarks covering new functionality (if appropriate).
  • [ ] I've verified that my change does not break any existing tests or introduce unexpected benchmark regressions.
  • [ ] I've updated the documentation (if appropriate).

stackotter avatar Mar 26 '22 03:03 stackotter

It seems like I accidentally included my commit to change the family size limit from 8 to 16 components, is that ok?

stackotter avatar Mar 26 '22 03:03 stackotter

It seems like I accidentally included my commit to change the family size limit from 8 to 16 components, is that ok?

To be honest - since this will be a huge undertaking anyways I'd like to keep this out of the PR for now - maybe just open another PR that we can merge before we continue with this and have it already in main.

ctreffs avatar Mar 28 '22 11:03 ctreffs

Simplify the nexus API to enhance maintainability (there are lots of unnecessary methods that could probably be removed, what's your opinion on this @ctreffs? my main rationale behind simplifying the API is that it seems pretty difficult to maintain when there are so many methods to update)

The outward facing API is not very clean right now I agree. However we have to be mindful about which APIs will be deprecated. Let's do a 2 step with the following steps:

  1. Deprecate API that seems not necessary in a separate PR
  2. Re-merge that PR into the current one

This will allow reasoning about API separate from the structure based refactor.

ctreffs avatar Mar 28 '22 11:03 ctreffs

Codecov Report

Merging #47 (ad7349a) into master (6f1ddd2) will decrease coverage by 36.44%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master      #47       +/-   ##
===========================================
- Coverage   98.09%   61.65%   -36.45%     
===========================================
  Files          30       23        -7     
  Lines        1368     1523      +155     
===========================================
- Hits         1342      939      -403     
- Misses         26      584      +558     

codecov[bot] avatar Mar 28 '22 11:03 codecov[bot]

Implement read-only and read-write families

Should be an add-on PR following after this one.

ctreffs avatar Mar 28 '22 11:03 ctreffs

The main limitation that I have identified so far is that there isn't a way (that I know of) to restrict a protocol to only value types (unlike restricting to reference types which is possible with AnyObject. The only workaround I can see for this is putting advice all throughout the documentation saying to use value types for components. It technically shouldn't break the ECS if a component is a reference type, however it breaks a lot of thread-safety guarantees and would probably make for pretty confusing code.

There are multiple aspects to this:

a) not using structs will break memory-locality/cache-friendlyness so using classes for components is never recommended b) we do not use a registration based approach (so the Nexus does not know about all available component types) - this is by design and we don't want to change that - so we do not have the luxury to check on registerComponent as other ECS do c) there is indeed no way of enforcing only structs being extended with the Component protocol

However I propose a mixed approach. We do not want to hamper performance by always checking if a component is a struct, but we can do a little bit better as not checking at all. We can use a Mirror on the component types when a family is defined. Mirror provides the ability to check for DisplayStyle where the given type can be checked for being a struct. I would only check this only on debug builds so this will only be affecting performance for non-release builds and help the programmer make educated choices.

Another way would be to analyse what Apple is doing with the ECS (using structs) in RealityKit https://developer.apple.com/documentation/realitykit/component

ctreffs avatar Mar 28 '22 12:03 ctreffs

I would only check this only on debug builds so this will only be affecting performance for non-release builds and help the programmer make educated choices.

I think adding debug checks is probably the best solution to that issue right now

Implement read-only and read-write families Should be an add-on PR following after this one.

By this I'm assuming you mean only implement read-write families in this pr? because otherwise the ecs would not be properly functioning and the pr would break families

This will allow reasoning about API separate from the structure based refactor.

Yep, that sounds sensible. I'll work on that PR first then.

To be honest - since this will be a huge undertaking anyways I'd like to keep this out of the PR for now - maybe just open another PR that we can merge before we continue with this and have it already in main.

Ok, I guess I'll have to once again google how to revert a commit from the git history lol. And then I'll create a PR for that change.

stackotter avatar Mar 28 '22 12:03 stackotter

By this I'm assuming you mean only implement read-write families in this pr? because otherwise the ecs would not be properly functioning and the pr would break families

exactly - we have to keep the scope manageable and not get ahead of ourselves. All these ideas are great and should be available ultimately. Just one step at a time :-)

ctreffs avatar Mar 28 '22 12:03 ctreffs