Entity Spawner Revamp #259

jkeon · 2023-05-31T19:39:29Z

Entity Spawners are now job structs that allow for creating an Entity in a job and getting back the deferred Entity from the Entity Command Buffer that will create it.

What is the current behaviour?

Spawning Entities are fire and forget. You have to develop some way to pick up created Entities later on.
This is really difficult when you need to assign specific instances to "slots" or associate relationships.

What is the new behaviour?

You can acquire an EntitySpawner struct off of an EntitySpawnSystem and use that in your jobs.
EntitySpawners can create any IEntitySpawnDefinition, no need to have it typed beforehand anymore.
They will return the deferred Entity that the underlying EntityCommandBuffer creates. You can use the API on the EntitySpawner to assign relationships that will be patched later on.

What issues does this resolve?

None

What PRs does this depend on?

Atomic Burstable ID Provider #258

Does this introduce a breaking change?

Yes - Spawning is different now although the API migration is relatively straight-forward.
No

mbaker3

Looking good but spawners are no longer ACV compatible now and look much more difficult to bring into task drivers. Can we fix this?

Unfortunately I also don't think this will be of use to the CreateAndIntegrateTaskDriver since we're not able to patch the reference in the EPD off the ECB.

mbaker3 · 2023-06-06T19:00:33Z

Scripts/Runtime/Entities/Lifecycle/EntitySpawnSystem.cs

+                return m_InstanceIDQueue.Dequeue();
+            }
+            //If we don't, then increment our pool of IDs and return that instead.
+            m_LargestInstanceIDIssued++;


Better served as an IDProvider? UShortIDProvider if you're feeling fancy.

Could be, I think this is fine for now. decline-cookies/anvil-csharp-core#147 Would make this part go away since the Pool logic would handle adding new ints

mbaker3 · 2023-06-06T19:03:37Z

Scripts/Runtime/Entities/Lifecycle/EntitySpawnSystem.cs

+            if (m_InstanceIDQueue.Count > 0)
+            {
+                return m_InstanceIDQueue.Dequeue();
+            }


Should we have a PooledIDProvider or use Pool<T> for this?

I looked at the Pool<T> implementation and it would be trivial to support value types.

Yes I originally wanted to use a Pool but couldn't and it was a tangent to go down.

Added a TODO and linked to: decline-cookies/anvil-csharp-core#147

Looks like all you have to do is change one line to support value types in Pool<T> that defaults the acquired value to null

mbaker3 · 2023-06-06T19:08:09Z

Scripts/Runtime/Entities/Lifecycle/EntitySpawnSystem.cs

+            entitySpawner.Playback(EntityManager);
+            entitySpawner.DisposeECB();


I'm not sure we should be playing back immediately on an immediate release. Shouldn't we still be waiting for the ECB playback moment that this EntitySpawnSystem is configured to execute?

If you need a job handle for m_CommandBufferSystem.AddJobHandleForProducer you can just pass default since your "work" with the ECB is done.

Oh I see, this isn't actually an ACV compatible type. Is there a way to require these in jobs without setting up a custom Schedule function?

Would really like to maintain compatibility with the "Require" pattern.

These functions are private.

The public facing ones are

public JobHandle AcquireAsync(out EntitySpawner value)
public void ReleaseAsync(JobHandle releaseAccessDependency)

I'm ok to implement one of the IAccessController interfaces but they add in a bunch of extra stuff like Dispose and the synchronous Acquire functions that don't quite make sense.

I could explicitly implement those functions and just toss a NotSupported exception but that does feel a bit dirty.

IAccessController doesn't have Dispose

For the sync interface there's no real benefit over SpawnImmediate but no a problem to support either. Maybe you want to schedule an entity to exist at the system's sync point and not until then.

Ok, I added support with the JobConfigs for Require and Fulfill so you can get an EntitySpawner.

I don't think the ACV approach really makes sense for an EntitySpawner since it is the underlying Prototypes hash map that is actually ACV'd.

You can still use the System like usual with SpawnDeferred or SpawnImmediate for main thread spawning. Or you can AcquireAsync for custom job spawning.

TaskDrivers can easily use it via RequireEntitySpawner

hmm, ok. The way the EntitySpawnWriter's adapted before was the same idea where the writer wasn't actually what required the access control it was the data that backed it.

All these special interfaces just create headaches whenever we need to use them in generic contexts (like JobConfig). I guess we can wait until I hit that some day and convert it then.

Discussed and while not great, it's ok enough for now.

mbaker3 · 2023-06-06T19:22:08Z

Scripts/Runtime/Entities/Lifecycle/JobInteraction/EntitySpawner.cs

+        {
+            ID = id;
+            m_ECB = ecb;
+            m_ECBWriter = m_ECB.AsParallelWriter();


Is there any overhead associated with always asking for a parallel writer?
In most TaskDriver scenarios the writer won't be written to in parallel.

There is probably some small amount of overhead just in terms of making the different lanes that a parallel writer can write to but that's a necessary evil.

With EntitySpawners being included in jobs to directly spawn, we have to allow for parallel jobs to write otherwise we'd be constraining all jobs to execute on one thread only.

In most TaskDriver scenarios the writer won't be written to in parallel.

I'm not sure that's true? All TaskDriver jobs are designed to go wide with most using BatchStrategy.MaximizeChunk so with low volume it MIGHT only execute on one thread, but with more than one chunk worth of data it will definitely go on other threads.

With EntitySpawners being included in jobs to directly spawn, we have to allow for parallel jobs to write otherwise we'd be constraining all jobs to execute on one thread only.

We couldn't have an opt in to a parallel writer? Most jobs that I've written so far the spawn are running on a single thread. They tend to be low volume.

I'm not sure that's true? All TaskDriver jobs are designed to go wide with most using BatchStrategy.MaximizeChunk so with low volume it MIGHT only execute on one thread, but with more than one chunk worth of data it will definitely go on other threads.

Are they? Almost all the Task Drivers I've written use Schedule for the jobs not ScheduleParallel to avoid clobbering EPD entries. The BatchStrategy is irrelevant in that situation I thought.

Hmmmn good point.

I started down a road here but it's a bunch of work and I feel like the performance overhead isn't a problem right now.

The underlying parallel writer will still work just fine on a single thread.

Once we upgrade to entities 1.0, we can remove the SharedComponent aspects that make duplicating the work across a regular ECB and a ECB.ParallelWriter less arduous.

Added a TODO.

jkeon · 2023-06-07T19:45:34Z

@mbaker3 This is ready for re-review

# Conflicts: # Scripts/Runtime/Entities/TaskDriver/TaskSet/Job/JobConfig/NoOpJobConfig.cs

Scripts/Runtime/Entities/TaskDriver/TaskSet/Job/JobData/AbstractJobData.cs

jkeon added 2 commits May 31, 2023 14:37

Spawn system revamp

ef7f012

Docs pass

bccdca8

jkeon requested a review from mbaker3 May 31, 2023 19:39

jkeon assigned mbaker3 May 31, 2023

jkeon mentioned this pull request Jun 2, 2023

Massively optimizing TaskDriver performance #264

Merged

2 tasks

mbaker3 requested changes Jun 6, 2023

View reviewed changes

jkeon added 2 commits June 6, 2023 15:28

Merge branch 'atomic-burstable-id-provider' into entity-spawner-revamp

9e50531

PR Comments

c5974f1

jkeon requested a review from mbaker3 June 6, 2023 19:51

Base automatically changed from atomic-burstable-id-provider to main June 6, 2023 19:54

jkeon added 2 commits June 6, 2023 15:55

Merge remote-tracking branch 'origin/main' into entity-spawner-revamp

97de24e

PR Comments

f4b66f6

Merge remote-tracking branch 'origin/main' into entity-spawner-revamp

90f4370

# Conflicts: # Scripts/Runtime/Entities/TaskDriver/TaskSet/Job/JobConfig/NoOpJobConfig.cs

mbaker3 reviewed Jun 7, 2023

View reviewed changes

Scripts/Runtime/Entities/TaskDriver/TaskSet/Job/JobData/AbstractJobData.cs Outdated Show resolved Hide resolved

jkeon added 2 commits June 7, 2023 16:08

Removing non-fulfill path

e77945c

Merge remote-tracking branch 'origin/main' into entity-spawner-revamp

5b6b954

mbaker3 approved these changes Jun 7, 2023

View reviewed changes

jkeon merged commit dd747eb into main Jun 7, 2023

jkeon deleted the entity-spawner-revamp branch June 7, 2023 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entity Spawner Revamp #259

Entity Spawner Revamp #259

jkeon commented May 31, 2023

mbaker3 left a comment

mbaker3 Jun 6, 2023

jkeon Jun 6, 2023

mbaker3 Jun 6, 2023

jkeon Jun 6, 2023

mbaker3 Jun 6, 2023

mbaker3 Jun 6, 2023

mbaker3 Jun 6, 2023

jkeon Jun 6, 2023

mbaker3 Jun 6, 2023

jkeon Jun 7, 2023

mbaker3 Jun 7, 2023

jkeon Jun 7, 2023

mbaker3 Jun 6, 2023

jkeon Jun 6, 2023

mbaker3 Jun 6, 2023

jkeon Jun 7, 2023

jkeon commented Jun 7, 2023

		entitySpawner.Playback(EntityManager);
		entitySpawner.DisposeECB();

Entity Spawner Revamp #259

Entity Spawner Revamp #259

Conversation

jkeon commented May 31, 2023

What is the current behaviour?

What is the new behaviour?

What issues does this resolve?

What PRs does this depend on?

Does this introduce a breaking change?

mbaker3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkeon commented Jun 7, 2023