fireblade-engine / ecs

A dependency free, lightweight, fast Entity-Component System (ECS) implementation in Swift
MIT License
110 stars 11 forks source link

Value type components #47

Open stackotter opened 2 years ago

stackotter commented 2 years ago

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:

Possible future optimisations

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

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

stackotter commented 2 years ago

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

ctreffs commented 2 years ago

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 commented 2 years ago

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.

codecov[bot] commented 2 years ago

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     
ctreffs commented 2 years ago

Implement read-only and read-write families

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

ctreffs commented 2 years ago

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

stackotter commented 2 years ago

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.

ctreffs commented 2 years ago

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 :-)