Search Unity

  1. Welcome to the Unity Forums! Please take the time to read our Code of Conduct to familiarize yourself with the forum rules and how to post constructively.
  2. Dismiss Notice

Feedback Feedback regarding the removal of AddInputDependency/GetOutputDependency

Discussion in 'Physics for ECS' started by Zec_, Mar 17, 2022.

  1. Zec_

    Zec_

    Joined:
    Feb 9, 2017
    Posts:
    148
    I've been trying to wrap my head around this new approach with RegisterPhysicsRuntimeSystemReadOnly and RegisterPhysicsRuntimeSystemReadWrite. Interesting way to use the singleton to sync dependencies, but it feels like it's more limited now than it used to be.

    It seems harder to work against a physics world outside of ECS systems now. Some simple examples would be:
    • Static utility methods that internally fetch the PhysicsWorld and work against it.
    • Any kind of non-system code that wants to fetch the PhysicsWorld and work against it.
    Previously we would just fetch the world along with GetOutputDependency and either complete the dependency or schedule jobs against it, but now it seems trickier. Especially if we want to schedule asynchronous access against the PhysicsWorld in a non-system location.

    I suspect one can "solve" the synchronous access by fetching the PhysicsSystemRuntimeData singleton wherever one wants to read from the CollisionWorld as that should complete it's dependency? It does feel like a quite un-intuitive way to work with it, but maybe it's a question of getting used to. I guess I could make an extension method to fetch the PhysicsWorld and the singleton, and then return the PhysicsWorld.

    Personally I think we will re-implement the GetOutputDependency/AddInputDependency methods in the physics library as it's far easier for us to work with for now, it would be a quite simple modification. Maybe we'll look at using these new workflows in potential fuure DOTS projects.
     
    argibaltzi likes this.
  2. papopov

    papopov

    Joined:
    Jun 29, 2020
    Posts:
    32
    Hey,
    Thanks for the feedback.
    You need to call RegisterPhysicsRuntimeSystemReadOnly() or RW() only once, in OnStartRunning() preferably.
    Once it is called, a read or write dependency will always be included in the system's OnUpdate() without requiring further calls.
    If you need PhysicsWorld on the main thread right there, just call Dependency.Complete(), and it will be ready. And if you want to schedule jobs with the world, you don't need to complete dependency.

    We are working now on a way to retrieve PhysicsWorld using DOTS approach, something like GetSingleton<PhysicsWorld>(), which will solve dependency problems automatically and remove the need of RegisterPhysicsRuntimeSystemReadOnly / ReadWrite.
     
    lclemens and Occuros like this.
  3. tertle

    tertle

    Joined:
    Jan 25, 2011
    Posts:
    3,626
    Personally I'm happy you have taken this approach as physics dependency handling was my biggest complaint about the physics package. I actually wrote a very similar implementation to this a while ago to handle it and I've found it worked very well. Glad to see something similar make it into the core package.
     
    Last edited: Mar 19, 2022
    lclemens likes this.
  4. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    Thanks for that explanation! I like this. It is much less confusing than before.
     
  5. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    I think maybe I misinterpreted the concept.

    I did this:

    Code (CSharp):
    1.  
    2. public partial class TestSystem : SystemBase
    3. {
    4.     BuildPhysicsWorld m_world;
    5.     protected override void OnCreate()
    6.     {
    7.         m_world = World.GetOrCreateSystem<BuildPhysicsWorld>();
    8.     }
    9.     protected override void OnStartRunning()
    10.     {
    11.         base.OnStartRunning();
    12.         m_world.RegisterPhysicsRuntimeSystemReadOnly(); // <---- register here doesn't fix the error
    13.     }
    14.     protected override void OnUpdate()
    15.     {
    16.         CollisionWorld world = m_world.PhysicsWorld.CollisionWorld;
    17.         //Dependency = JobHandle.CombineDependencies(Dependency, m_world.GetOutputDependency());  <---- this used to work
    18.         //m_world.RegisterPhysicsRuntimeSystemReadOnly(); // <---- uncommenting this will fix the error.
    19.         Entities.ForEach((ref MyDataData mydata, in FooData foo) => { // <---- error occurs here
    20.             // use world as read-only here
    21.         }).WithReadOnly(world).ScheduleParallel();
    22.     }
    23. }
    24.  
    I thought you were implying that calling RegisterPhysicsRuntimeSystemReadOnly() only once in OnStartRunning() would be enough.

    But the above code will give the runtime error:

    InvalidOperationException: The previously scheduled job Broadphase:prepareStaticBodyDataJob writes to the Unity.Collections.NativeArray`1[Unity.Physics.CollisionFilter] PrepareStaticBodyDataJob.FiltersOut. You are trying to schedule a new job UnitTargeterSystem:UnitTargeterSystem_LambdaJob_0_Job, which reads from the same Unity.Collections.NativeArray`1[Unity.Physics.CollisionFilter] (via UnitTargeterSystem_LambdaJob_0_Job.JobData.world.Broadphase.m_StaticTree.BodyFilters). To guarantee safety, you must include Broadphase:prepareStaticBodyDataJob as a dependency of the newly scheduled job.
    Unity.Jobs.LowLevel.Unsafe.JobsUtility.ScheduleParallelFor (Unity.Jobs.LowLevel.Unsafe.JobsUtility+JobScheduleParameters& parameters, System.Int32 arrayLength, System.Int32 innerloopBatchCount) (at <3be1a7ff939c43f181c0a10b5a0189ac>:0)
    Unity.Entities.JobEntityBatchExtensions.ScheduleInternal[T] (T& jobData, Unity.Entities.EntityQuery query, Unity.Jobs.JobHandle dependsOn, Unity.Jobs.LowLevel.Unsafe.ScheduleMode mode, System.Boolean isParallel) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/IJobEntityBatch.cs:660)
    Unity.Entities.JobEntityBatchExtensions.ScheduleParallel[T] (T jobData, Unity.Entities.EntityQuery query, Unity.Jobs.JobHandle dependsOn) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/IJobEntityBatch.cs:279)
    UnitTargeterSystem.UnitTargeterSystem_LambdaJob_0_Execute (FactionMatrix factionMatrix, Unity.Physics.CollisionWorld world) (at Temp/GeneratedCode/Assembly-CSharp/UnitTargeterSystem__System_264805703.g.cs:323)
    UnitTargeterSystem.OnUpdate () (at Assets/Scripts/Systems/UnitTargeterSystem.cs:50)
    Unity.Entities.SystemBase.Update () (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/SystemBase.cs:409)
    Unity.Entities.ComponentSystemGroup.UpdateAllSystems () (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:583)
    UnityEngine.Debug:LogException(Exception)
    Unity.Debug:LogException(Exception) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/Stubs/Unity/Debug.cs:19)
    Unity.Entities.ComponentSystemGroup:UpdateAllSystems() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:588)
    Unity.Entities.ComponentSystemGroup:OnUpdate() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:523)
    Unity.Entities.ComponentSystem:Update() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystem.cs:114)
    Unity.Entities.DummyDelegateWrapper:TriggerUpdate() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ScriptBehaviourUpdateOrder.cs:426)
     
  6. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    What what's even more confusing... super duper confusing.... I have a totally unrelated system... which doesn't use physics at all - not one single bit. It's a very simple system. Yet....

    InvalidOperationException: The previously scheduled job PhysicsWorldExporter:ExportDynamicBodiesJob writes to the ComponentTypeHandle<Unity.Transforms.Translation> ExportDynamicBodiesJob.JobData.PositionType. You must call JobHandle.Complete() on the job PhysicsWorldExporter:ExportDynamicBodiesJob, before you can read from the ComponentTypeHandle<Unity.Transforms.Translation> safely.


    So strange....

    I don't even know how to fix it. I put RegisterPhysicsRuntimeSystemReadOnly() in every OnStartRunning() and in every Update() where I use the physics world, and it's still giving the error in my job that doesn't use physics at all.

    Where on earth would one start when trying to fix an error that points to a completely innocent file that has nothing to do with the error?? This error-in-wrong-file issue is what happened before and why I had to put in JobHandle.CombineDependencies() everywhere I used the physics world. Somehow I got lucky and got that working fine in 0.17, but now with 0.50 I don't know what to do.
     
    Last edited: Mar 21, 2022
  7. papopov

    papopov

    Joined:
    Jun 29, 2020
    Posts:
    32
    You also need to put proper [UpdateAfter]/[UpdateBefore] and [UpdateInGroup] attributes on system, as was done before.
    To fix your problem, I think this would solve it.
    Put [UpdateInGroup(typeof(FixedStepSimulationGroup))], [UpdateAfter(typeof(ExportPhysicsWorld))], [UpdateBefore(typeof(EndFramePhysicsSystem))]

    And also, don't call m_world.RegisterPhysicsSystemsReadOnly()
    You should call this.RegisterPhysicsSystemsReadOnly()
     
  8. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    Thanks for the information @papopov , however, I added those groups and changed to using the "this" version and I'm still getting runtime errors.

    I get the error in my EnemyTargeter system. A stripped down version of it looks like this:

    Code (CSharp):
    1.  
    2. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    3. [UpdateAfter(typeof(ExportPhysicsWorld))]
    4. [UpdateBefore(typeof(EndFramePhysicsSystem))]
    5. public partial class EnemyTargeterSystem : SystemBase
    6. {
    7.     BuildPhysicsWorld m_world;
    8.  
    9.     protected override void OnCreate()
    10.     {
    11.         base.OnCreate();
    12.         m_world = World.GetOrCreateSystem<BuildPhysicsWorld>();
    13.     }
    14.  
    15.     protected override void OnStartRunning()
    16.     {
    17.         base.OnStartRunning();
    18.         this.RegisterPhysicsRuntimeSystemReadOnly();
    19.     }
    20.  
    21.     protected override void OnUpdate()
    22.     {
    23.         CollisionWorld world = m_world.PhysicsWorld.CollisionWorld;
    24.  
    25.         EntityQuery query = GetEntityQuery(ComponentType.ReadOnly<CommandCenterTag>(), ComponentType.ReadOnly<Translation>());
    26.         NativeArray<Translation> homePosDataArray = query.ToComponentDataArray<Translation>(Allocator.TempJob);
    27.         NativeArray<Entity> homeEntityArray = query.ToEntityArray(Allocator.TempJob);
    28.  
    29.         Entities.ForEach((ref TargetData target, ref TargetDistanceData distance, in Translation pos) => {
    30.             //............. perform target searching here using world ...........
    31.         }).WithReadOnly(homePosDataArray).WithReadOnly(homeEntityArray).WithReadOnly(world)
    32.         .WithDisposeOnCompletion(homePosDataArray).WithDisposeOnCompletion(homeEntityArray).ScheduleParallel();
    33.     }
    34. }
    If I comment out the GetEntityQuery(), the error goes away... so it seems that the ForEach() works fine, but there is something about that GetEntityQuery that breaks it.

    If I comment out GetEntityQuery() above, then my EnemyTargeterSystem runs fine, but then I get the error in my LinearMovementAndCollisionSystem, which looks like this:

    Code (CSharp):
    1.  
    2. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    3. [UpdateAfter(typeof(ExportPhysicsWorld))]
    4. [UpdateBefore(typeof(EndFramePhysicsSystem))]
    5. public partial class LinearMovementAndCollisionSystem : SystemBase
    6. {
    7.     BuildPhysicsWorld m_world;
    8.  
    9.     protected override void OnCreate()
    10.     {
    11.         m_world = World.GetOrCreateSystem<BuildPhysicsWorld>();
    12.     }
    13.  
    14.     protected override void OnStartRunning()
    15.     {
    16.         base.OnStartRunning();
    17.         this.RegisterPhysicsRuntimeSystemReadOnly();
    18.     }
    19.  
    20.     protected override void OnUpdate()
    21.     {
    22.         CollisionWorld world = m_world.PhysicsWorld.CollisionWorld;
    23.  
    24.         Entities.ForEach((int entityInQueryIndex, Entity entity, ref Translation translation, in LinearVelocityData velocity) => {
    25.             // .........move object forward, changing translation.........
    26.             //..........world is used to do a raycast..........
    27.             //..........i also use an ecb buffer here to add a component and append to a buffer.........
    28.         }).WithReadOnly(world).ScheduleParallel();
    29.     }
    30. }
    The exact error that this one gives is:
    InvalidOperationException: The previously scheduled job LinearMovementAndCollisionSystem:LinearMovementAndCollisionSystem_LambdaJob_0_Job writes to the ComponentTypeHandle<Unity.Transforms.Translation> LinearMovementAndCollisionSystem_LambdaJob_0_Job.JobData.__translationTypeHandle. You must call JobHandle.Complete() on the job LinearMovementAndCollisionSystem:LinearMovementAndCollisionSystem_LambdaJob_0_Job, before you can read from the ComponentTypeHandle<Unity.Transforms.Translation> safely.
    Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle.CheckReadAndThrowNoEarlyOut (Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle handle) (at <3be1a7ff939c43f181c0a10b5a0189ac>:0)
    Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle.CheckReadAndThrow (Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle handle) (at <3be1a7ff939c43f181c0a10b5a0189ac>:0)
    Unity.Entities.EntityQueryImpl.ToComponentDataArray[T] (Unity.Collections.Allocator allocator, Unity.Entities.EntityQuery outer) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/Iterators/EntityQuery.cs:800)
    Unity.Entities.EntityQuery.ToComponentDataArray[T] (Unity.Collections.Allocator allocator) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/Iterators/EntityQuery.cs:1804)
    HealthStationScanSystem.OnUpdate () (at Assets/Scripts/Systems/HealthStationScanSystem.cs:25)
    Unity.Entities.SystemBase.Update () (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/SystemBase.cs:409)
    Unity.Entities.ComponentSystemGroup.UpdateAllSystems () (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:583)
    UnityEngine.Debug:LogException(Exception)
    Unity.Debug:LogException(Exception) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/Stubs/Unity/Debug.cs:19)
    Unity.Entities.ComponentSystemGroup:UpdateAllSystems() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:588)
    Unity.Entities.ComponentSystemGroup:OnUpdate() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:523)
    Unity.Entities.ComponentSystem:Update() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystem.cs:114)
    Unity.Entities.DummyDelegateWrapper:TriggerUpdate() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ScriptBehaviourUpdateOrder.cs:426)

    What's strange is that the above error mentions "HealthStationScanSystem.OnUpdate ()", which has nothing to do with physics and looks like this:

    Code (CSharp):
    1.  
    2. public partial class HealthStationScanSystem : SystemBase
    3. {
    4.     protected override void OnUpdate()
    5.     {
    6.         if (!EntityUtils.Exists<HealthRechargeData>(EntityManager)) { return; }
    7.  
    8.         EntityQuery query = GetEntityQuery(ComponentType.ReadOnly<HealthRechargeData>(), ComponentType.ReadOnly<Translation>());
    9.         NativeArray<Translation> stationPosDataArray = query.ToComponentDataArray<Translation>(Allocator.TempJob);
    10.         NativeArray<Entity> stationEntityArray = query.ToEntityArray(Allocator.TempJob);
    11.  
    12.         Entities.ForEach((ref NearestHealthStationData nearest, in Translation pos, in FactionData faction) => {
    13.          //......... do non-physics stuff here ....
    14.         }).WithReadOnly(stationPosDataArray).WithReadOnly(stationEntityArray).WithDisposeOnCompletion(stationPosDataArray).WithDisposeOnCompletion(stationEntityArray).ScheduleParallel();
    15.     }
    16. }
    I think the "common thing" between the two errors is the use of Translation and GetEntityQuery(). I know that previously things worked fine when I was using 0.17 with Dependency = JobHandle.CombineDependencies(Dependency, m_world.GetOutputDependency()); in the updates wherever I used CollisionWorld.
     
  9. tertle

    tertle

    Joined:
    Jan 25, 2011
    Posts:
    3,626

    Somehow I forgot to include it in my compilation post, but I found with ToEntityArray and ToComponentDataArray you need to call query.CompleteDependency() now to be safe
    I'd recommend you use ToEntityArrayAsync and ToComponentDataArrayAsync instead
     
    lclemens likes this.
  10. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    I found a "fix"... but it seems pretty hacky. [EDIT - Better fix below thanks to tertle's observation]

    So in HealthStationScanSystem... I put this.CompleteDependency(); right before GetEntityQuery(). I also put this.CompleteDependency(); right before GetEntityQuery() in EnemyTargeterSystem.

    The hacky part is that HealthStationScanSystem has NOTHING to do with physics other than it queries for READ-ONLY position data. I don't see why my physics systems would care if I'm reading position data in another system.

    This is the error that comes up when I don't have the complete-dependency call in the HealthStationScanSystem:

    InvalidOperationException: The previously scheduled job PhysicsWorldExporter:ExportDynamicBodiesJob writes to the ComponentTypeHandle<Unity.Transforms.Translation> ExportDynamicBodiesJob.JobData.PositionType. You must call JobHandle.Complete() on the job PhysicsWorldExporter:ExportDynamicBodiesJob, before you can read from the ComponentTypeHandle<Unity.Transforms.Translation> safely.
    Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle.CheckReadAndThrowNoEarlyOut (Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle handle) (at <3be1a7ff939c43f181c0a10b5a0189ac>:0)
    Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle.CheckReadAndThrow (Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle handle) (at <3be1a7ff939c43f181c0a10b5a0189ac>:0)
    Unity.Entities.EntityQueryImpl.ToComponentDataArray[T] (Unity.Collections.Allocator allocator, Unity.Entities.EntityQuery outer) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/Iterators/EntityQuery.cs:800)
    Unity.Entities.EntityQuery.ToComponentDataArray[T] (Unity.Collections.Allocator allocator) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/Iterators/EntityQuery.cs:1804)
    HealthStationScanSystem.OnUpdate () (at Assets/Scripts/Systems/HealthStationScanSystem.cs:28)
    Unity.Entities.SystemBase.Update () (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/SystemBase.cs:409)
    Unity.Entities.ComponentSystemGroup.UpdateAllSystems () (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:583)
    UnityEngine.Debug:LogException(Exception)
    Unity.Debug:LogException(Exception) (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/Stubs/Unity/Debug.cs:19)
    Unity.Entities.ComponentSystemGroup:UpdateAllSystems() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:588)
    Unity.Entities.ComponentSystemGroup:OnUpdate() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystemGroup.cs:523)
    Unity.Entities.ComponentSystem:Update() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ComponentSystem.cs:114)
    Unity.Entities.DummyDelegateWrapper:TriggerUpdate() (at Library/PackageCache/com.unity.entities@0.50.0-preview.24/Unity.Entities/ScriptBehaviourUpdateOrder.cs:426)

    The only class of mine that the error points to is HealthStationScanSystem.

    It is very disturbing that physics can cause errors in totally unrelated classes that normally would work fine. I was hoping this new system would improve the confusion but to be honest I'm just as confused as ever.
     
    Last edited: Mar 21, 2022
  11. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    Ah, that is good to know! query.CompleteDependency() certainly does the trick. I went through all my code wherever I had ToComponentDataArray() and ToEntityArray() and added the query complete. Nice find!!

    If anyone is wondering about the async mode... Something like this works great:

    Code (CSharp):
    1.         EntityQuery query = GetEntityQuery(ComponentType.ReadOnly<CommandCenterTag>(), ComponentType.ReadOnly<Translation>(), ComponentType.ReadOnly<FactionData>());
    2.         NativeArray<Translation> homePosDataArray = query.ToComponentDataArrayAsync<Translation>(Allocator.TempJob, out JobHandle h1);
    3.         NativeArray<Entity> homeEntityArray = query.ToEntityArrayAsync(Allocator.TempJob, out JobHandle h2);
    4.         NativeArray<FactionData> homeFactionArray = query.ToComponentDataArrayAsync<FactionData>(Allocator.TempJob, out JobHandle h3);
    5.         Dependency = JobHandle.CombineDependencies(Dependency, JobHandle.CombineDependencies(h1, h2, h3));
    On a slightly separate topic... I changed my systems that are only ever reading from the world to use:

    Code (CSharp):
    1. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    2. [UpdateAfter(typeof(BuildPhysicsWorld))]
    I did this because the 0.50 documentation section for collisions ( https://docs.unity3d.com/Packages/com.unity.physics@0.50/manual/collision_queries.html ), states:

    "Collision queries use physics simulation data (read-only access), more precisely an internal structure called broadphase, which is built by a job scheduled in BuildPhysicsWorld. Collision queries can be performed any time before or after BuildPhysicsWorld, but if you do it before make sure you provide your job handle to BuildPhysicsWorld.AddInputDependencyToComplete()"

    If you are only ever reading with collision queries, [UpdateAfter(typeof(BuildPhysicsWorld))] ensures that your systems run after BuildPhysicsWorld so you don't have to worry about calling AddInputDependencyToComplete(). The documentation on the collision page doesn't discuss putting "this.RegisterPhysicsRuntimeSystemReadOnly();" in OnStartRunning(), but I'm pretty sure it's necessary because without it I get errors all over the place. I'm not an expert in this, so if this theory is wrong, hopefully someone can point out my mistake.
     
    Last edited: Mar 22, 2022
  12. tertle

    tertle

    Joined:
    Jan 25, 2011
    Posts:
    3,626
    There's no reason to do async like that. The whole point of async is to not have a sync point. If you need to cause a sync point just use the non-async versions.

    var h4 =JobHandle.CombineDependencies(h1, h2, h3);
    Dependency =JobHandle.CombineDependencies(Dependency, h4);

    then just read the arrays in the job and it'll be fine.
     
    lclemens likes this.
  13. papopov

    papopov

    Joined:
    Jun 29, 2020
    Posts:
    32
    This got me interested, so I made a repro similar to EnemyTargeterSystem that you showed here, and here's what I found. In the next code snippet, I've commented out all code that doesn't seem to introduce your problem.

    Code (CSharp):
    1. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    2. [UpdateAfter(typeof(ExportPhysicsWorld))]
    3. [UpdateBefore(typeof(EndFramePhysicsSystem))]
    4. public partial class EnemyTargeterSystem : SystemBase
    5. {
    6.     BuildPhysicsWorld m_world;
    7.  
    8.     protected override void OnCreate()
    9.     {
    10.         base.OnCreate();
    11.         m_world = World.GetOrCreateSystem<BuildPhysicsWorld>();
    12.     }
    13.  
    14.     protected override void OnStartRunning()
    15.     {
    16.         //base.OnStartRunning();
    17.         //this.RegisterPhysicsRuntimeSystemReadOnly();
    18.     }
    19.  
    20.     protected override void OnUpdate()
    21.     {
    22.         //CollisionWorld world = m_world.PhysicsWorld.CollisionWorld;
    23.  
    24.         EntityQuery query = GetEntityQuery(ComponentType.ReadOnly<Translation>());
    25.         NativeArray<Translation> homePosDataArray = query.ToComponentDataArray<Translation>(Allocator.TempJob);
    26.         NativeArray<Entity> homeEntityArray = query.ToEntityArray(Allocator.TempJob);
    27.  
    28.         // Entities
    29.         //     //.WithReadOnly(world)
    30.         //     .ForEach((ref DummyTestComponentData dtc, in Translation pos) =>
    31.         // {
    32.         //     RaycastInput input = new RaycastInput
    33.         //     {
    34.         //         Start = Unity.Mathematics.float3.zero,
    35.         //         End = new Unity.Mathematics.float3(10, 10, 10),
    36.         //         Filter = CollisionFilter.Default
    37.         //     };
    38.  
    39.         //     dtc.Value += 1;
    40.  
    41.         //     //if (world.CastRay(input))
    42.         //     //{
    43.         //     //    dtc.Value += 1;
    44.         //     //}
    45.  
    46.         // })
    47.         //.ScheduleParallel(Dependency);
    48.  
    49.         Dependency.Complete();
    50.         homePosDataArray.Dispose();
    51.         homeEntityArray.Dispose();
    52.     }
    53. }
    So here, as you said, the problem seems to be query.ToComponentDataArray() call. Previously scheduled job ExportPhysicsWorld is writng to Translation data (which it is), and that is causing problems. I would expect when one calls ToComponentDataArray(), that the dependencies which write to Translation component data would be completed automatically by the ToComponentDataArray() function. Looks like that is not the case, and it seems to be a bug in the entities/jobs/collections.
    Calling Dependency.Complete() or query.CompleteDependency() makes the problem go away, but you lose a lot of perf that way, since you force the sync point on main thread, and potentially a lot of jobs could be writing to Translation component data.

    The real fix here seems to use async component data arrays, as mentioned above.
    Here's a working sample that I got working and I've uncommented all the commented code above.


    Code (CSharp):
    1. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    2. [UpdateAfter(typeof(ExportPhysicsWorld))]
    3. [UpdateBefore(typeof(EndFramePhysicsSystem))]
    4. public partial class EnemyTargeterSystem : SystemBase
    5. {
    6.     BuildPhysicsWorld m_world;
    7.  
    8.     protected override void OnCreate()
    9.     {
    10.         base.OnCreate();
    11.         m_world = World.GetOrCreateSystem<BuildPhysicsWorld>();
    12.     }
    13.  
    14.     protected override void OnStartRunning()
    15.     {
    16.         base.OnStartRunning();
    17.         this.RegisterPhysicsRuntimeSystemReadOnly();
    18.     }
    19.  
    20.     protected override void OnUpdate()
    21.     {
    22.         CollisionWorld world = m_world.PhysicsWorld.CollisionWorld;
    23.  
    24.         EntityQuery query = GetEntityQuery(ComponentType.ReadOnly<Translation>());
    25.         NativeArray<Translation> homePosDataArray = query.ToComponentDataArrayAsync<Translation>(Allocator.TempJob, out JobHandle jbh1);
    26.         NativeArray<Entity> homeEntityArray = query.ToEntityArrayAsync(Allocator.TempJob, out JobHandle jbh2);
    27.  
    28.         Dependency = JobHandle.CombineDependencies(Dependency, jbh1);
    29.         Dependency = JobHandle.CombineDependencies(Dependency, jbh2);
    30.  
    31.         Entities
    32.             .WithReadOnly(world)
    33.             .WithReadOnly(homePosDataArray)
    34.             .WithReadOnly(homeEntityArray)
    35.             .ForEach((ref DummyTestComponentData dtc, in Translation pos) =>
    36.         {
    37.             RaycastInput input = new RaycastInput
    38.             {
    39.                 Start = Unity.Mathematics.float3.zero,
    40.                 End = new Unity.Mathematics.float3(10, 10, 10),
    41.                 Filter = CollisionFilter.Default
    42.             };
    43.  
    44.             for (int i = 0; i < homePosDataArray.Length + homeEntityArray.Length; i++)
    45.             {
    46.                 if (world.CastRay(input))
    47.                 {
    48.                     dtc.Value += 1;
    49.                 }
    50.             }
    51.         })
    52.             .WithDisposeOnCompletion(homePosDataArray)
    53.             .WithDisposeOnCompletion(homeEntityArray)
    54.             .ScheduleParallel();
    55.     }
    56. }
     
    lclemens likes this.
  14. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    Nice work!!! Thanks so much for verifying that - it's good to have another person reach the same conclusion. The async versions that @tertle suggested seem to be working well for me.

    I don't mind using the async versions as a fix in 0.50. The thing that bothers me is having to use the async versions (or using this.CompleteDependency()) everywhere - even in systems that have nothing to do with physics. It is really confusing because it can be a perfectly valid system that was working just fine and then suddenly after doing some physics stuff in a totally unrelated system, the previously working system breaks. Or it could be the other way around - you have a working physics system and then you add a new system that uses ToEntityDataArray() and suddenly you're getting a cryptic physics error that doesn't reference any of your code except for the new system you just added.
     
    Last edited: Mar 22, 2022
  15. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    One thing I don't get...

    I'm doing these 3 things for the physics systems:
    1. calling this.RegisterPhysicsRuntimeSystemReadOnly() in OnStart().
    2. ordering execution to be [UpdateAfter(typeof(BuildPhysicsWorld))]
    3. using .WithReadOnly(world) in the Entities.ForEach() call.
    Three things that are saying - READ ONLY... I don't have any desire to write. I just want to see if a ray or aabb box intersects things.

    So why is it complaining about writing to Translation anyway???
     
  16. papopov

    papopov

    Joined:
    Jun 29, 2020
    Posts:
    32
    All the steps you've mentioned 1-3 are correct, and you don't have a problem with that.
    Physics systems are writing to a Translation component, and you're calling .ToComponentDataArray<Translation>() (which isn't automatically completing required dependencies), and that is giving you the trouble.

    Here's a repro of the same problem that you're facing, in simple code.

    Code (CSharp):
    1. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    2. public partial class WriteTranslationSystem : SystemBase
    3. {
    4.     protected override void OnUpdate()
    5.     {
    6.         Entities
    7.             .ForEach((ref Translation t) =>
    8.             {
    9.                 t.Value = t.Value + float3.zero;
    10.             })
    11.             .Schedule();
    12.     }
    13. }
    14.  
    15. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    16. [UpdateAfter(typeof(WriteTranslationSystem))]
    17. public partial class ReadTranslationSystem : SystemBase
    18. {
    19.     protected override void OnUpdate()
    20.     {
    21.         var query = GetEntityQuery(ComponentType.ReadOnly<Translation>());
    22.         var arr = query.ToComponentDataArray<Translation>(Unity.Collections.Allocator.TempJob);
    23.         arr.Dispose();
    24.     }
    25. }
    This also gives you the same error, because ReadTranslationSystem is calling ToComponentDataArray<Translation>(), without completing the previous dependency, or using an async version.
     
  17. Resshin27

    Resshin27

    Joined:
    Apr 21, 2018
    Posts:
    31
    lclemens likes this.
  18. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    Last edited: Mar 25, 2022
  19. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    714
    I think I figured out where I was going wrong. I was thinking that all of my systems that perform raycast and aabb did not write to translation so they are all read-only. However, I just realized that my building-placement system's foreach does raycasts and it also has a parameter of "ref Translation pos" that is used to clamp the building to the ground. I think that if I removed that system the errors from using ToComponentDataArray (without completing the previous dependency) in my other systems would go away.

    Thanks for all your help @papopov !
     
    papopov likes this.
  20. argibaltzi

    argibaltzi

    Joined:
    Nov 13, 2014
    Posts:
    216
    Is there any way to continue using the AddInputDependency/GetOutputDependency?
     
    lclemens likes this.