Upgrading to Entities 1.0.16 #301
Conversation
There was a problem hiding this comment.
I'm just going to go ahead and mention that #296 was created as part of this PR to make digging up the context easier later.
Thanks for doing this. Looks like a lot of work and some tricky gotchas. This is looking good. Think we just need to make some style/clarity decisions.
Removed all [BurstCompatible] and [NotBurstCompatible] attributes.
To communicate intent should we replace with our own attribute? Or just use comments?
While Upgrade to Entities 1.0 #86 identifies a lot of the work that was done in this PR, the TODO's throughout the code refer to additional work that this PR does not address.
Can you check off what is complete in that issue? Or if it makes more sense move the other pending work to new issues.
| @@ -12,7 +12,6 @@ namespace Anvil.Unity.Collections | |||
| /// A collection of extension methods for <see cref="FixedList32Bytes{T}"/> (and friends) that require internal | |||
| /// access to function. | |||
| /// </summary> | |||
| [BurstCompatible] | |||
| public static unsafe class FixedListInternalExtension | |||
There was a problem hiding this comment.
Do you think we should put the [BurstCompile] attribute on this or in any place we've replaced the attribute?
OR
Since we're no longer using this attribute should we communicate methods/types that are/aren't burst compatible somehow? Our own attribute or maybe just comments?
There was a problem hiding this comment.
Let's chat about this.
Putting [BurstCompile] won't do anything so I think that's confusing to add.
See: https://forum.unity.com/threads/when-where-and-why-to-put-burstcompile-with-mild-under-the-hood-explanation.1344539/ for more in-depth explanations for when to use the attributes.
A class/function etc isn't necessarily just Burst or not. It could be both depending on where you call it.
For example, if you called this FixedListInternalExtension.IndexOf from within a bursted job it would execute as Bursted code. Great.
But in the same runtime, if you called the same function FixedListInternalExtension.IndexOf from the main thread in OO land, it would just execute as regular non-bursted code.
The compiler would have generated two different lower level paths.
We could add in the updated test attribute of GenerateTestsForBurstCompatibility which replaces BurstCompatible but that also doesn't do anything unless you run the editor commands for generating tests for your code base and running them. There is also no replacement for NotBurstCompatible.
If we added our own attributes, that'd be fine, but again they can't really do anything. There's nothing we can use them for to check or gate or validate for.
Comments could be fine, but I'm not sure if it's worthwhile since it will depend on the context that you all it.
In my mind, based on how things work and the feedback we get from the IDE, there are the following scenarios:
- Calling from Bursted Context
- Calling code that can't be bursted: IDE will give you a warning, compiler will give you a Burst error.
- This is great, prevents issues where you want to be bursted but messed up and didn't notice.
- Calling code that can be bursted: All good, IDE will tell you that it's Bursted code.
- Calling code that can't be bursted: IDE will give you a warning, compiler will give you a Burst error.
- Calling from Non Bursted Context
- Doesn't matter what it is, the code will not be Bursted even if it has the
BurstCompileattribute.
- Doesn't matter what it is, the code will not be Bursted even if it has the
Given this, I'm not sure we need to communicate anything to the developer since really they just need to understand where they are calling from.
There was a problem hiding this comment.
Ok, I misunderstood how [BurstCompile] worked. That's fine.
I think comments might be helpful. It's fair enough that the IDE will warn you when calling non-burstable code from a bursted context but it doesn't really help when you're writing bursted code and poking around the codebase to see what you can find. Until you type it out (or read the whole class) you won't know if a piece of existing functionality that you've found will be compatible.
Maybe that doesn't happen often enough to justify writing comments and I'm inventing work here but I feel like, while writing bursted code, I'm often asking myself "can I use this method/type?"
There was a problem hiding this comment.
The counter-argument to comments is that it might be too general and the underlying code changes and whoever does that forgets to update the comment at the top of the file?
In the scenario you're describing, I feel like the effort to type it out (which is pretty fast with autocomplete) to get the Burst warning in the IDE is the same or less then opening things up and reading comments which would be more to maintain?
If we really wanted to signify to the developer that these are Burst safe things to use, I would argue that we should note that in the Class name or Method name instead?
There was a problem hiding this comment.
Let's try without the comments for now and see how it goes.
When I'm devising a solution I'm not going to type out and check every method/type as I go. I'm going to collect a path or collection of types that "will work" and then go to implement.
Adding burst to all the type and method names is insane but if you're considering that I'll just drop this topic. You've backed me away from the ledge...
Scripts/Runtime/Entities/Lifecycle/Migration/EntityWorldMigrationSystem.cs
Outdated
Show resolved
Hide resolved
Scripts/Runtime/Entities/TaskDriver/TaskSet/Job/JobData/CancelJobData.cs
Show resolved
Hide resolved
Scripts/Runtime/Entities/TaskDriver/TaskSet/Job/JobType/TaskCancelJobForDeferExtension.cs
Show resolved
Hide resolved
| @@ -5,15 +5,15 @@ | |||
| namespace Anvil.Unity.DOTS.Entities.TaskDriver | |||
| { | |||
| internal class CDFEAccessWrapper<T> : AbstractAccessWrapper | |||
There was a problem hiding this comment.
Should we rename to CDLAccessWrapper? ComponentLookupAccessWrapper? CDAccessWrapper?
Based on the existing DynamicBufferAccessWrapper maybe ComponentDataAccessWrapper is the most appropriate.
There was a problem hiding this comment.
I'm fine with renaming but we should talk about the knock-on renaming that should occur as part of this.
CDFEAccessWrapper gives access to CDFEReader and CDFEWriter which used to come from ComponentDataFromEntity which we shortened to CDFE.
It's now ComponentLookup (which is nicer) so should we do:
Option 1
ComponentLookupReaderComponentLookupWriterComponentLookupAccessWrapper
This one is a bit odd since we're not reading/writing the lookup, but it's more the access we have to the data in the lookup.
Option 2
ComponentDataReaderComponentDataWriterComponentDataAccessWrapper
This is better but maybe obfuscates that it's reading or writing the data via a lookup? You might think you're getting in sequence memory access because we call things like that as DataStreamActiveReader and DataStreamPendingWriter.
Option 3
ComponentLookupROorCLROComponentLookupRWorCLRWComponentLookupAccessWrapperorCLAccessWrapper
When we used CDFE that gave my brain a shortcut to know we were dealing with a Lookup and we had specific access to it.
This, I think, gives us that same shortcut but better terminology to what Unity is providing already. They use RW and RO for queries and getting access to data etc.
The same thing is then propagated to the DBFEForRead and DBFEForExclusiveWrite.
There was a problem hiding this comment.
Yea, naming is tough. Regardless of what we decide let's make the change in a new PR so we don't clutter this one further.
I think I prefer Option 2 the most or leaving it as is. "Component Data From Entity" is still a valid description of the behaviour.
To me, I'm not worried about implying sequential access. The usage of the reader/writer is the biggest hint that it's random access. Getting data off of an Entity key is clearly a lookup operation.
I vote either ComponentDataReader or CDFEReader with a preference to choose the latter and leave it as is. It's short and descriptive.
This isn't a hard position though and I'm open to other opinions.
There was a problem hiding this comment.
I've created the issue here: #303
But I also agree, I'm cool with leaving as CDFE.
...ts/Runtime/Entities/TaskDriver/TaskSet/TaskData/JobDataInteraction/DataStreamActiveReader.cs
Show resolved
Hide resolved
| @@ -35,19 +35,9 @@ public static void AddMissingStandardComponents(Entity entity, EntityManager ent | |||
| { | |||
| Debug.Assert(entityManager.Exists(entity)); | |||
|
|
|||
| if (!entityManager.HasComponent<Translation>(entity)) | |||
| if (!entityManager.HasComponent<LocalTransform>(entity)) | |||
There was a problem hiding this comment.
I'm not sure this util method is even required anymore now that it's just one type.
If you just want to make a TODO issue for this that's fine.
|
Updated based on your comments. Quite a few of them I had responses for that require your input or an discussion via a call.
Yes, updated and checked off everything except for the one remaining effort which is more involved. #86 now refers here and is about going through the code base to create issues for the specific post entities 1.0 upgrade work to do. |
|
@mbaker3 Additional comments made and/or changes made. Back over to you! |
Upgrading the code base to Entities 1.0.16
What is the current behaviour?
Currently on 0.51
What is the new behaviour?
[BurstCompatible]and[NotBurstCompatible]attributes.structfor most generic constraints. Must useunmanagedTypeIndexinstead ofintsWorldInternalno longer hasOnWorldCreatedorOnWorldDestroyedevents.Worldand handle any specific logic in the Constructor/Dispose method.Translation,Rotation, andScalecomponents no longer exist, instead aLocalTransformcomponent is used.World.GetOrCreateSystemand related API's are nowWorld.GetOrCreateSystemManagedand related APIs.DynamicBufferSharedWriteHandleand the associated bloat that was necessary withWorldCache.DBFEWriterBufferFromEntityis nowBufferLookupComponentDataFromEntityis nowComponentLookupNativeArraynow returnNativeListto account for disabled entities.IJobEntityBatchare nowIJobChunkParallelAccessUtilsince it's now much more simplified.ITaskForDefer,ITaskCancelJobForDeferandITaskUpdateJobForDeferto no longer be generic jobs.ISystems for Burstable systems and generic job types are no longer allowed.Executefunction will give an index which can then be used to get the right element from the reader.What issues does this resolve?
What PRs does this depend on?
Does this introduce a breaking change?