Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Value type components #47

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

stackotter
Copy link

@stackotter stackotter commented Mar 26, 2022

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:

  • 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

  • I've read the Contribution Guidelines
  • 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
Copy link
Author

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

@ctreffs ctreffs added the enhancement New feature or request label Mar 28, 2022
@ctreffs
Copy link
Member

ctreffs commented Mar 28, 2022

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
Copy link
Member

ctreffs commented Mar 28, 2022

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
Copy link

codecov bot commented Mar 28, 2022

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
Copy link
Member

ctreffs commented Mar 28, 2022

Implement read-only and read-write families

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

@ctreffs
Copy link
Member

ctreffs commented Mar 28, 2022

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
Copy link
Author

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
Copy link
Member

ctreffs commented Mar 28, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants