Custom Entity Migration #232

jkeon · 2023-04-29T12:05:46Z

Unity will automatically remap Entity references on Components and in DynamicBuffers when Entities move from one World to another.

Entities that exist in TaskDrivers or EntityPersistentData however will not.

This PR gives functionality to provide a framework for allowing anything to be remapped and implements it for TaskDrivers and EntityPersistentData

What is the current behaviour?

No way to provide custom remapping.

What is the new behaviour?

Framework

Anything can implement the IMigrationObserver and register with the WorldEntityMigrationSystem to receive a notification when Entities have migrated to a new World.

The callback will provide the necessary information to perform the custom logic to remap Entity references.

MigrationUtil provides the functionality to actually do the remapping via RegisterTypeForEntityPatching and then the call to PatchEntityReferences.

All changes to the world should happen through WorldEntityMigrationSystem.MigrateTo. If you use the EntityManager.MoveEntitiesFrom vanilla function, we won't know and custom migration won't happen.

TaskDrivers and EntityPersistentData

I've handled both of these systems to automatically register their types and patch references.
Everything is jobified so that we can use Burst and in practice ends up being pretty quick.

TaskDrivers now can have a unique migration suffix to identify them if you happen to have two or more of the same type of TaskDriver located at the same place in the heirarchy.

Tag-Alongs

Fixed ENABLE_UNITY_COLLECTIONS_CHECKS to ANVIL_DEBUG_SAFETY where I found them.
Changed some type restrictions from struct to unmanaged
Added a GetKeyValueArrays to EntityPersistentDataWriter

What's Next

Deep Migration #233 There are still some ways you can miss Entities. If you have an EPD then you have a relationship of Key: Entity to Value: Something.
- The Key can be remapped no problem
- The Something might be a Collection like UnsafeParallelHashMap. If the key or something in that collection's value has an Entity reference, there's no way right now for us to know.
- I think I can scan this an account for it, but I might have to add the check for each type of Unsafe Collection.
Migration Integrity #234 Integrity.
- As we talked about, we should be able to build a lookup of all Entity references before the Migration occurs. After the Migration, we can run through that lookup and see if anything of our references were broken.
- Either A) We referred to something that left us behind.
- Or B) We referred to something that we left behind.
- We can then alert the developer who didn't clean up before the migration.

What issues does this resolve?

Resolves World Moving - Remap Entity data in VirtualData/TaskDrivers #42

What PRs does this depend on?

None

Does this introduce a breaking change?

Yes - If you were changing Worlds through EntityManager, you should use WorldEntityMigrationSystem now instead.
No

…he same

mbaker3

Super exciting feature! Generally just things to discuss and TODOs to create.

Scripts/Runtime/Entities/Lifecycle/Migration/IMigrationObserver.cs

Scripts/Runtime/Entities/Lifecycle/Migration/MigrationUtil.cs

mbaker3 · 2023-05-01T15:46:40Z

Scripts/Runtime/Entities/Lifecycle/Migration/MigrationUtil.cs

+        private sealed class SharedTypeOffsetInfo
+        {
+            public static readonly SharedStatic<UnsafeParallelHashMap<long, TypeOffsetInfo>> REF = SharedStatic<UnsafeParallelHashMap<long, TypeOffsetInfo>>.GetOrCreate<MigrationUtilContext, SharedTypeOffsetInfo>();
+        }
+
+        private sealed class SharedEntityOffsetInfo
+        {
+            public static readonly SharedStatic<IntPtr> REF = SharedStatic<IntPtr>.GetOrCreate<MigrationUtilContext, SharedEntityOffsetInfo>();
+        }
+
+        private sealed class SharedBlobAssetRefInfo
+        {
+            public static readonly SharedStatic<IntPtr> REF = SharedStatic<IntPtr>.GetOrCreate<MigrationUtilContext, SharedBlobAssetRefInfo>();
+        }
+
+        private sealed class SharedWeakAssetRefInfo
+        {
+            public static readonly SharedStatic<IntPtr> REF = SharedStatic<IntPtr>.GetOrCreate<MigrationUtilContext, SharedWeakAssetRefInfo>();
+        }


Why are these wrapped in individual types vs just being statics in the MigrationUtil class?
If they have to be wrapped to they need to each be wrapped int heir own type or could they all be put into on private class?

This is just how SharedStatic's work. Unity itself has to do a similar scan for IComponentTypes and we need a way to differentiate between their IntPtr and my IntPtr. By wrapping in private sealed classes, I ensure my uniqueness and they ensure theirs.

It's a bunch of boiler plate but that's why I relegated it to the bottom of this class hidden away.

cool, weird pattern.

SharedStatic and FunctionPointers for Burst in general are weird patterns I agree, but I guess I see how they need to ensure the uniqueness of a memory address without using any managed code and that's really only possible by using a bunch of compile time generated generics with unique class types.

Annoying though.

Scripts/Runtime/Entities/Lifecycle/Migration/MigrationUtil.cs

Scripts/Runtime/Entities/TaskDriver/Migration/TaskDriverMigrationData.cs

mbaker3 · 2023-05-01T16:50:42Z

...TaskDriver/TaskSet/TaskData/DataStream/DataSource/AbstractEntityProxyInstanceIDDataSource.cs

+            {
+                //Can't modify while iterating so we collapse down to a single array and clean the underlying stream.
+                //We'll build this stream back up if anything should still remain
+                NativeArray<EntityProxyInstanceID> currentInstanceArray = m_CurrentStream.ToNativeArray(Allocator.Temp);


future optimization but would it be possible to build up the new stream (if needed) at the same time? The same way we do on each iteration of a task driver?

Not 100% sure what you mean, but I think we already are?

Entities that weren't moved get rewritten back to this world's stream. Entities that were moved get remapped and written to the new world's stream.

Right now you're copying your existing stream, clearing and then re-adding to it if the entity isn't remapped.
If you instead created a new stream you could avoid the full temp array copy.

The same way when the task driver system job updates it processes the current stream and writes any instances that need to continue to a new stream for the next frame. Similar to a double buffer approach.

Alternatively, if stream could support some sort of removeSwapBack type operation then maybe we could leave the current stream intact and just cherry pick out the entities that do get remapped to the new world?...unless we think order matters in a stream.

Ohhhh I see what you mean. Interesting. I've added a TODO and referred to this.

I don't think order matters in the stream... removeSwapBack is really neat though, I think that could be possible and is probably the way to do this instead of adding a second stream and double buffering them.

yea just want to be explicit in our docs that DataStreams are unordered.

...ntime/Entities/TaskDriver/TaskSet/TaskData/DataStream/DataSource/CancelProgressDataSource.cs

.../Runtime/Entities/TaskDriver/TaskSet/TaskData/DataStream/DataSource/EntityProxyDataSource.cs

mbaker3 · 2023-05-01T17:01:44Z

Scripts/Runtime/Entities/TaskDriver/TaskSet/TaskSet.cs

+
+            AddToMigrationLookup(
+                parentPath, 
+                typeof(CancelCompleteDataStream).GetReadableName(), 


Mild concern about using readable name for a uniqueID type situation.
Readable name is used for human readable formatting and if we ever changed that formatting it could break the path. Should we just use C#'s built in?

Hmmmn I'm not sure. If we changed the formatting it would break, but only if we changed the formatting mid-run. It's a runtime only id.

We could use the built in, it's just harder to parse when debugging. Thoughts?

I'm not so concerned about changing the formatting mid run as I am that our formatting doesn't provide proper uniqueness at some point.

In fact, today, this doesn't guarantee uniqueness between types. namespace1.MyType and namespace2.MyType will both resolve to the same readable name.

Maybe the type hash is a better solution? Then for debug-ability our ToString() methods can parse out and map the hash back to the type and generate the readable name?

Hmmmn ok, fair point. I changed it to FullName for now to keep the uniqueness.

I added a TODO to hash the FullName to get shorter paths.

teeechhnically you want the Assembly qualified one

Switched to using BurstRuntime to get the hash of the type which uses the Assembly qualified name under the hood

…e "System"

# Conflicts: # Scripts/Runtime/Entities/TaskDriver/AbstractTaskDriver.cs # Scripts/Runtime/Entities/TaskDriver/System/AbstractTaskDriverSystem.cs # Scripts/Runtime/Entities/TaskDriver/TaskSet/TaskData/DataStream/EntityProxyDataStream.cs # Scripts/Runtime/Entities/TaskDriver/TaskSet/TaskSet.cs

jkeon · 2023-05-01T20:19:08Z

@mbaker3 Ready for re-review.

Also contains changing the name of the Task Driver System to the outside to be System and the internal one to be TaskDriverSystem instead of the flip-flop it was before.

mbaker3 · 2023-05-02T14:38:13Z

All addressed. Some discussions to sort out but this is looking good!

jkeon added 11 commits April 24, 2023 09:13

WIP

755edc2

WIP

97dc150

Merge branch 'main' into taskdriver-migration

87632a0

Working Patching but order of Worlds messes up if we're not careful

bae9b8e

More migration work

8c94539

Fixing an error with Entity Patching and assuming all entities were t…

b4324dd

…he same

Working for Burst

9618b65

Docs pass

59b135f

Adding support for Cancel migration

6a85ec9

Ensuring we get all Persistent Data

9a72efa

Removing TODO

d750a0c

jkeon requested a review from mbaker3 April 29, 2023 12:05

jkeon assigned mbaker3 Apr 29, 2023

This was referenced Apr 30, 2023

Deep Migration #233

Open

Migration Integrity #234

Open

mbaker3 reviewed May 1, 2023

View reviewed changes

jkeon mentioned this pull request May 1, 2023

i233 Deep Migration Warning #237

Merged

2 tasks

jkeon added 5 commits May 1, 2023 14:49

Merging in main

2fafb2e

Renaming the public facing reference for Task Driver System to just b…

8f00359

…e "System"

PR Comments

3986537

Ready for re-review

fe2784b

jkeon requested a review from mbaker3 May 1, 2023 20:18

jkeon added 2 commits May 2, 2023 10:55

PR Comments

21d5b1c

Fixing migration uniqueness

24e55f5

mbaker3 approved these changes May 2, 2023

View reviewed changes

jkeon merged commit 7de8eda into main May 2, 2023

jkeon deleted the taskdriver-migration branch May 2, 2023 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Entity Migration #232

Custom Entity Migration #232

jkeon commented Apr 29, 2023 •

edited

Loading

mbaker3 left a comment

mbaker3 May 1, 2023

jkeon May 1, 2023

mbaker3 May 2, 2023

jkeon May 2, 2023

mbaker3 May 1, 2023

jkeon May 1, 2023

mbaker3 May 2, 2023

jkeon May 2, 2023

mbaker3 May 2, 2023

mbaker3 May 1, 2023

jkeon May 1, 2023

mbaker3 May 2, 2023

jkeon May 2, 2023

mbaker3 May 2, 2023

jkeon May 2, 2023

jkeon commented May 1, 2023

mbaker3 commented May 2, 2023

Custom Entity Migration #232

Custom Entity Migration #232

Conversation

jkeon commented Apr 29, 2023 • edited Loading

What is the current behaviour?

What is the new behaviour?

Framework

TaskDrivers and EntityPersistentData

Tag-Alongs

What's Next

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

jkeon commented May 1, 2023

mbaker3 commented May 2, 2023

jkeon commented Apr 29, 2023 •

edited

Loading