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

Add buffers #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add buffers #114

wants to merge 1 commit into from

Conversation

DrSmugleaf
Copy link
Collaborator

Buffer is added as a component to entities, and the world API allows adding/removing these buffers manually or indirectly changing their elements.
It currently stores elements in a List and has a method if the target framework is .NET 5 or above to get its elements as a span.

This also allows entities to be added to other entities through buffers. See https://www.flecs.dev/flecs/md_docs_Relationships.html

@mikhail-dcl
Copy link
Contributor

Hey there
I am looking into the most recent PRs and I have the following concerns:

  • It looks like Arch is diverging from the approach of the bare minimum.
  • The core of Arch is very performance-oriented while this particular addition has nothing to provide in these terms

Those two factors were decisive for us at Decentraland when we picked Arch as an ECS framework over the others

In my opinion Buffers should go to the Extended framework as there is nothing special a user can't write on their own

Otherwise, I would propose thinking about the following modifications to this PR:

  • Lists pooling (PooledList?)
  • Lists initial capacity
  • Change Lists to something custom/unmanaged to provide ref-like access and Span for any .NET version

In general (especially for use with Unity) I pursue a more granular control over GC allocations and objects lifecycle that is not present here.

@DrSmugleaf
Copy link
Collaborator Author

DrSmugleaf commented May 21, 2023

Hey there I am looking into the most recent PRs and I have the following concerns:

  • It looks like Arch is diverging from the approach of the bare minimum.
  • The core of Arch is very performance-oriented while this particular addition has nothing to provide in these terms

Those two factors were decisive for us at Decentraland when we picked Arch as an ECS framework over the others

In my opinion Buffers should go to the Extended framework as there is nothing special a user can't write on their own

Which PRs? Arch's default API surface hasn't gotten any new additions for months. For performance, both #103 and #108 were focused on improving the performance of existing code. The first one has benchmarks for the only API that has changed in performance, as an optimization, if performance degradation is one of your concerns.

#98 was also designed to have both the smallest API surface possible and best performance by not adding anything more than the bare minimum, leaving more elaborate event bus implementations such as those that filter by multiple components, or those that allow for multi-component matching, up to forks, the user, or Arch.Extended.
It also is hidden behind a feature flag, which when disabled leaves the existing code paths identical to what they were before its introduction, as is required by any feature implemented into Arch.

Both that PR and #97 were compared at the time as well to find the best solution for events, which are necessary to implement in Arch itself, and cannot be sanely done in the same way in Arch.Extended.

If you are referring to #111 then that should be brought up there, unless this is the one you have issue with, which was split off from that one at the request of @genaray to provide an API similar to Unity buffers. That PR also has no meaningful changes to any existing code paths.

As for having nothing to provide in performance terms, there is very little to provide around a wrapper for a list other than exposing a span and maybe providing a set amount of elements outside of the list. The latter is waiting on @genaray to finish his thesis to discuss how best to implement this, similar to what Unity does. This could be a fixed size buffer for unmanaged types or source generated for any other type. This is at the moment no different than adding a list as a component to an entity.

If nothing is changed that requires access to the internals of Arch then this PR could easily be transferred to Arch.Extended. It was however left open here in the interim, as the main PR that the implementation comes from is still in review and that one does require being coded into the main Arch repository.

Otherwise, I would propose thinking about the following modifications to this PR:

  • Lists pooling (PooledList?)
  • Lists initial capacity
  • Change Lists to something custom/unmanaged to provide ref-like access and Span for any .NET version

In general (especially for use with Unity) I pursue a more granular control over GC allocations and objects lifecycle that is not present here.

If that's the use case, it might be better to let the user choose the collection that the buffer is wrapping entirely, as all a pooled list does is pool the array instance that it uses internally. If not, you don't need to use a custom implementation as PooledList already exposes a span, even in netstandard2.1.

Exposing only the initial capacity as a constructor argument would pose a similar problem with letting the user choose the collection, unless overloads are provided which I don't think is an ideal solution.

Changing it to something unmanaged would likely result in either compromising too much for managed types or making them impossible to use with the same implementation. An array (or a PooledList) does not have either of these problems, however if you are looking to use these to store a list of entities then you might want to look into using an unmanaged array or list from Arch.Extended.

As for controlling GC allocations and lifecycles, using an array internally (and maybe pooling them by default) as well as letting the user choose which instance to utilize would both address both of those concerns and provide the simplest API, alongside letting you get a span and a ref in netstandard2.1. Though if you are stuck using it, I assume because of Unity, then I don't think getting a ref to a list element is the biggest of your performance worries.

If that change fits your use case and addresses your concerns though tell me, as that's the currently planned change for buffers unless a better one comes up.

@genaray
Copy link
Owner

genaray commented May 21, 2023

I am looking into the most recent PRs and I have the following concerns:

  • It looks like Arch is diverging from the approach of the bare minimum.
  • The core of Arch is very performance-oriented while this particular addition has nothing to provide in these terms

I support Smug here. The recently introduced events can not just be stripped out. They belong into the core.
An alternative is to just dont use them or use the EventBus to simulate such events. And there were a lot of performance related prs in the last few weeks.

Relationships and buffers could potentially move to Arch.Extended however. But in this case it would make most sense to merge those PRs first and move them later to Arch.Extended to see if any other API or internals should go public as well.

@mikhail-dcl
Copy link
Contributor

Hey guys

Which PRs?

I was referring to Buffers and Relations, I didn't mean any performance improvements

Though if you are stuck using it, I assume because of Unity, then I don't think getting a ref to a list element is the biggest of your performance worries.

Yes, with Unity one is always stuck in older .NET versions. We have many performance worries 😄 What I want to make sure of (and it's out of your concern, of course) is if there is something new we can use with Unity straight away without a performance worry 😉 Anyways, most likely if we want something like this we will be using UnsafeArray/List or Unity's Native or Fixed counterparts

If that change fits your use case and addresses your concerns though tell me, as that's the currently planned change for buffers unless a better one comes up.

Yes, on a higher level it mostly addresses my concerns, thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants