EcsRx / ecsrx

A reactive take on the ECS pattern for .net game developers
MIT License
520 stars 36 forks source link

made GroupBindingSystemHandler consider CollectionAffinityAttribute #25

Closed Fijo closed 3 years ago

Fijo commented 3 years ago

I'm not quite sure if it'd be better to maybe have the CollectionIds be an optional parameter for the FromGroup- / FromComponents-Attributes and only pull it from the CollectionAffinityAttribute of the System when falling back on the IGroupSystem.Group property. For I decided to just always consider it.

I did a minor version bump but theoretically it would have to be a mayor version as the change could be breaking. However since you aren't having individual Version on each plugin and the plugin is very new I felt like that could be neglected to keep the mayor version low.

Fijo commented 3 years ago

Update: As I was going though my own code I noticed myself that I'd probably prefer to have more individual control over the CollectionAffinityAttribute when using multiple FromGroup- / FromComponents- Attributes.

I could also imagine just putting that CollectionAffinityAttribute on the property itself in those instances where the main systems group isn't used.

If we can agree on a more refined solution I'm happy enough to updated this PR and implement it. So what do you think?

grofit commented 3 years ago

Yeah this is a very good point, there is already an attribute for this, while I think we can probably use this its tricky as it is consistent then, but currently I think that can only be used on classes, but we can expand it to be used on properties and fields too. I mean to be honest this would probably be better as this way we can just have the singular handler hook into it all and check for it rather than each of these new attributes having to handle it.

Thing I guess is attributes do not guarantee anything, they are just metadata, so I am happy enough to augment the attribute for affinities to become usable on properties and fields. If you are happy to do the work and add into the system handler + tests im happy to do PR, if not but you agree push to a branch and I will happily do the rest of the work.

Fijo commented 3 years ago

I had a look at the code again and the way I'm currently thinking of doing it is that the GroupBindingSystemHandler should still consider the CollectionAffinityAttribute on the class but only for the main group of an IGroupSystem. If the attribute on a property/ field does not refer to the main group, only CollectionAffinityAttributes on that property/ field should be considered.

For example:

[CollectionAffinity(3)]
class MySystem : IGroupSystem {
  public IGroup Group => new Group(typeof(XYZ));
  [FromGroup]
  public IObservableGroup ObservableGroup { get; set; }
  [FromGroup(typeof(MyGroup))]
  public IObservableGroup MyObservableGroup { get; set; }
}

would mean that

Do you agree to having it work in that way?

grofit commented 3 years ago

Yeah that sounds fine, as you say it already factors that in so would be more in line with current functionality, for the latter scenario we can always add support later for affinities at that level through the attribute but this should be fine.