-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
base: master
Are you sure you want to change the base?
Add buffers #114
Conversation
Hey there
Those two factors were decisive for us at Decentraland when we picked In my opinion Otherwise, I would propose thinking about the following modifications to this PR:
In general (especially for use with Unity) I pursue a more granular control over |
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. 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.
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. |
I support Smug here. The recently introduced events can not just be stripped out. They belong into the core. 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. |
Hey guys
I was referring to Buffers and Relations, I didn't mean any performance improvements
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
Yes, on a higher level it mostly addresses my concerns, thanks |
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