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 entity and component lifecycle events #98

Merged
merged 12 commits into from
May 8, 2023

Conversation

DrSmugleaf
Copy link
Collaborator

@DrSmugleaf DrSmugleaf commented Apr 28, 2023

See #25
This doesn't (currently) support custom user-defined events.

Adds a way to handle entity created/destroyed events and component add/set/remove events.
It only uses arrays for data structures unless a component of type object is passed, at which point it needs to use a dictionary to find its id.

Few things:

  • EventType and ComponentType should be merged as they do effectively the same thing, I can do that in this pr or it can be done later since it doesn't change the API.
  • There is one quirk where doing _world.Create<EventTestComponentOne>(); fires a set event but not an add event, should that be changed?
  • With this PR some source generated operations rent an array of entities to store which ones to fire events on, since Shift removes them from the archetype and they become inaccessible otherwise.
    • Should Shift be changed?
    • If not, the array could be replaced with a stackalloc if the amount of entities is under a certain size. What size should this be, and should it depend on if PURE_ECS is enabled, which makes the Entity struct hold less data? Or should it just store its int Id?
  • Should the Events class that holds the handlers be a struct instead? This would require changing how the non generic calls work slightly or using manual field offsets.
  • The handlers are stored in a list instead of an event Action field so that this can more easily support ordering later if necessary.
  • Should the handler array loops use the same code as the rest of Arch's loops to avoid bound checks? With Unsafe.Add and DangerousGetReference

@genaray
Copy link
Owner

genaray commented Apr 30, 2023

Gonna look at this later! :)

@genaray
Copy link
Owner

genaray commented May 1, 2023

Havent had time yet, last few days were hell. But tomorrow :)

@genaray genaray self-assigned this May 3, 2023
@genaray genaray added the enhancement New feature or request label May 3, 2023
@genaray genaray linked an issue May 3, 2023 that may be closed by this pull request
@genaray
Copy link
Owner

genaray commented May 3, 2023

Sorry for the late reply, im extremely happy when that damn thesis does not consume 99% of my time anymore.

  • EventType and ComponentType should be merged as they do effectively the same thing, I can do that in this pr or it can be done later since it doesn't change the API.

That can be done later ^^ Probably its even "clean" to separate them or probably its my tired brain forcing me into that thinking.

  • There is one quirk where doing _world.Create<EventTestComponentOne>(); fires a set event but not an add event, should that be changed?

I think it makes the most sense to fire a add event only or? Well actually a create operation is a add and a set operation, but for the user it only notable as one single add operation i guess.

  • With this PR some source generated operations rent an array of entities to store which ones to fire events on, since Shift removes them from the archetype and they become inaccessible otherwise.

I think there's a way around this. Its not that well documented since its dark magic (actually i just have no clue how i should explain it). EntityInfo.Shift updated the EntityInfo mapping only. However, they are not really inaccessible, they still exist in their origin archetype. and after the copy operation also in the target archetype. We know where those were attached to by using the cached LastSlot stuff. And therefore we can then just iterate from the previous last slot to the newest last slot. Those are the newly added entities and we can fire add and set events for them without the need of an additional array. Thats extremely performant and also reduces the code. I think some method already uses this technique, it probably even is the EntityInfo.Shift itself. Or some archetype.SetRange method... i need to check this.

  • Should the Events class that holds the handlers be a struct instead? This would require changing how the non generic calls work slightly or using manual field offsets.

Thats a potential improvement, but is that really necessary? ^^ Theres only one Events class, correct?

  • The handlers are stored in a list instead of an event Action field so that this can more easily support ordering later if necessary.

Thats actually very useful. I could also imagine that this approach is "better" with e.g. queryable events and stuff.

  • Should the handler array loops use the same code as the rest of Arch's loops to avoid bound checks? With Unsafe.Add and DangerousGetReference

It should (to enforce a common codestyle here) i guess ^^


Im gonna look at that add/set problematic to remove the need of array renting :)
And i will probably also remove the World from the Archetype and add those capabilities to the world directly. This will require an exposing of the Entity-Event methods to the public so that the user can call them upon his own needs for such a scenario ^^

@DrSmugleaf
Copy link
Collaborator Author

DrSmugleaf commented May 4, 2023

  • EventType and ComponentType should be merged as they do effectively the same thing, I can do that in this pr or it can be done later since it doesn't change the API.

That can be done later ^^ Probably its even "clean" to separate them or probably its my tired brain forcing me into that thinking.

Keeping them separate does mean that no array space is wasted accounting for components that don't have events subscribed to them.

  • There is one quirk where doing _world.Create<EventTestComponentOne>(); fires a set event but not an add event, should that be changed?

I think it makes the most sense to fire a add event only or? Well actually a create operation is a add and a set operation, but for the user it only notable as one single add operation i guess.

It now raises both events with this commit, since it doesn't check if the component passed is of default value before raising the set event. Should it? That would be the only way I can think of fixing it.

  • Should the Events class that holds the handlers be a struct instead? This would require changing how the non generic calls work slightly or using manual field offsets.

Thats a potential improvement, but is that really necessary? ^^ Theres only one Events class, correct?

There's one events class and its generic counterpart that inherits it, with an array that holds those event classes, which are casted using Unsafe.As

  • Should the handler array loops use the same code as the rest of Arch's loops to avoid bound checks? With Unsafe.Add and DangerousGetReference

It should (to enforce a common codestyle here) i guess ^^

Checked this and it isn't possible to do while targeting netstandard2, since DangerousGetReference can't be called on a list and no span can be obtained from it without a call to CollectionsMarshal, which was introduced in .NET 5. A preprocessor directive could be used for #if NET5_0_OR_GREATER, if that's considered worth it.

And i will probably also remove the World from the Archetype and add those capabilities to the world directly. This will require an exposing of the Entity-Event methods to the public so that the user can call them upon his own needs for such a scenario ^^

World has been removed from the archetype in this commit and the event-raising methods are public now with this commit
I didn't fix the array renting yet though.

@genaray
Copy link
Owner

genaray commented May 4, 2023

So i think this should work as a replacement for the renting arrays:

       // Update the entityInfo of all copied entities.
        for (var chunkIndex = archetypeSlot.ChunkIndex; chunkIndex >= newArchetypeSlot.ChunkIndex; --chunkIndex)
        {
            // Get data
            ref var chunk = ref archetype.GetChunk(chunkIndex);
            ref var entityFirstElement = ref chunk.Entity(0);

            // Only move within the range, depening on which chunk we are at.
            var isStart = chunkIndex == archetypeSlot.ChunkIndex;
            var isEnd = chunkIndex == newArchetypeSlot.ChunkIndex;

            var upper = isStart ? archetypeSlot.Index : chunk.Size-1;
            var lower = isEnd ? newArchetypeSlot.Index : 0;

            for (var index = upper; index >= lower; --index)
            {
                ref readonly var entity = ref Unsafe.Add(ref entityFirstElement, index);
                // Fire event for entity
            }
        }

This is done exactly like this during the shift operation and since no EntityInfo is accessed, it will also work after the shift since all the data still exists.

I'll try to incorporate this next :) probably even with a small enumerator to simplify code since no one likes to write this shit everytime.

It now raises both events with this commit, since it doesn't check if the component passed is of default value before raising the set event.

I think that's fine :) Default value is default value and its still being set, so it makes sense to still fire it.

Checked this and it isn't possible to do while targeting netstandard2, since DangerousGetReference can't be called on a list and no span can be obtained from it without a call to CollectionsMarshal, which was introduced in .NET 5. A preprocessor directive could be used for #if NET5_0_OR_GREATER, if that's considered worth it.

Then we will just leave it like this for the first. Putting more directives into this would just reduce the readability.
A custom array or list could come in handy here, probably even as an addition to Arch.Extended ^^ But for now its good like this. Probably the JIT compiler also removes the bound checks by itself since its a linear operation.

@genaray genaray merged commit 2003198 into genaray:master May 8, 2023
1 check passed
@genaray genaray mentioned this pull request May 9, 2023
@DrSmugleaf DrSmugleaf mentioned this pull request May 21, 2023
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
Status: Done
Development

Successfully merging this pull request may close these issues.

Entity Events
2 participants