search

reeseschultz / ReeseUnityDemos

Unity packages and demos—emphasizing ECS, jobs and the Burst compiler—by Reese and others.
https://reese.codes
MIT License
515 stars 45 forks source link

Unity Physics PhysicsBody set to Dynamic or Kinematic on a NavAgent causes NavSurfaceTrackingJob to throw errors #38

Closed Tricnic closed 4 years ago

Tricnic commented 4 years ago

If the GameObject that NavAgent is added to when converted to an entity contains a PhysicsBody script anywhere in its hierarchy that has its Motion Type set to Dynamic or Kinematic when it is converted via ConvertEntity, an InvalidOperationException is thrown each frame complaining about unsafe reading from the Unity.Physics.MotionData NativeArray. The error is not thrown when Motion Type is set to Static.

To reproduce: Open the ReeseUnityDemos project Find the DinosaurPrefab prefab Add the PhysicsBody script to the prefab and set Motion Type to Static Click Play and verify that the scene plays as normal Stop the scene and change Motion Type to Dynamic or Kinematic on the DinosaurPrefab prefab Click Play and see the InvalidOperationException error

You get the same result with Unity Physics v0.3.2 and v0.4.0.

Stack trace of the error:

InvalidOperationException: The previously scheduled job NavSurfaceSystem:<>c__DisplayClass_NavSurfaceTrackingJob reads from the NativeArray <>c__DisplayClass_NavSurfaceTrackingJob.JobData.physicsWorld.DynamicsWorld.m_MotionVelocities. You are trying to schedule a new job Solver:ParallelApplyGravityAndCopyInputVelocitiesJob, which writes to the same NativeArray (via ParallelApplyGravityAndCopyInputVelocitiesJob.MotionVelocities). To guarantee safety, you must include NavSurfaceSystem:<>c__DisplayClass_NavSurfaceTrackingJob as a depen
Unity.Jobs.LowLevel.Unsafe.JobsUtility.ScheduleParallelFor (Unity.Jobs.LowLevel.Unsafe.JobsUtility+JobScheduleParameters& parameters, System.Int32 arrayLength, System.Int32 innerloopBatchCount) (at <3dc54541a2574ac7826a004a212a4332>:0)
Unity.Jobs.IJobParallelForExtensions.Schedule[T] (T jobData, System.Int32 arrayLength, System.Int32 innerloopBatchCount, Unity.Jobs.JobHandle dependsOn) (at <3dc54541a2574ac7826a004a212a4332>:0)
Unity.Physics.Solver.ScheduleApplyGravityAndCopyInputVelocitiesJob (Unity.Collections.NativeArray`1[T] motionVelocities, Unity.Collections.NativeArray`1[T] inputVelocities, Unity.Mathematics.float3 gravityAcceleration, Unity.Jobs.JobHandle inputDeps, System.Int32 threadCountHint) (at Library/PackageCache/com.unity.physics@0.4.0-preview.5/Unity.Physics/Dynamics/Solver/Solver.cs:167)
Unity.Physics.Simulation.ScheduleStepJobs (Unity.Physics.SimulationStepInput input, Unity.Physics.SimulationCallbacks callbacksIn, Unity.Jobs.JobHandle inputDeps, System.Int32 threadCountHint) (at Library/PackageCache/com.unity.physics@0.4.0-preview.5/Unity.Physics/Dynamics/Simulation/Simulation.cs:363)
Unity.Physics.Systems.StepPhysicsWorld.OnUpdate () (at Library/PackageCache/com.unity.physics@0.4.0-preview.5/Unity.Physics/ECS/Systems/StepPhysicsWorld.cs:100)
Unity.Entities.SystemBase.Update () (at Library/PackageCache/com.unity.entities@0.11.1-preview.4/Unity.Entities/SystemBase.cs:414)
Unity.Entities.ComponentSystemGroup.UpdateAllSystems () (at Library/PackageCache/com.unity.entities@0.11.1-preview.4/Unity.Entities/ComponentSystemGroup.cs:445)
UnityEngine.Debug:LogException(Exception)
Unity.Debug:LogException(Exception) (at Library/PackageCache/com.unity.entities@0.11.1-preview.4/Unity.Entities/Stubs/Unity/Debug.cs:19)
Unity.Entities.ComponentSystemGroup:UpdateAllSystems() (at Library/PackageCache/com.unity.entities@0.11.1-preview.4/Unity.Entities/ComponentSystemGroup.cs:450)
Unity.Entities.ComponentSystemGroup:OnUpdate() (at Library/PackageCache/com.unity.entities@0.11.1-preview.4/Unity.Entities/ComponentSystemGroup.cs:398)
Unity.Entities.ComponentSystem:Update() (at Library/PackageCache/com.unity.entities@0.11.1-preview.4/Unity.Entities/ComponentSystem.cs:109)
Unity.Entities.DummyDelegateWrapper:TriggerUpdate() (at Library/PackageCache/com.unity.entities@0.11.1-preview.4/Unity.Entities/ScriptBehaviourUpdateOrder.cs:192)
Mdlich commented 4 years ago

I can confirm that happening in my project too. Trying to understand why it's happening, but my dots/unity.physics/current package understanding is lacking. ><

reeseschultz commented 4 years ago

Thanks @Tricnic for the details and @Mdlich for confirming.

Sounds like an issue with how the nav package controls system update order. To understand present state of the nav package, the NavSurfaceSystem updates after the NavBasisSystem, which updates after BuildPhysicsWorld. These systems mainly exist to ensure the jumping functionality works⁠—there is some amortized raycasting conducted using Unity.Physics to accomplish this. Other systems are ordered according to those, so I find myself juggling them more often than I would like.

I haven't dedicated much attention to the idea of physical agents, in and of themselves. For my use cases I would just remove the NavAgent component at runtime and add it back as needed. If you could share what you're trying to do specifically, that may help me better help you.

With all that said, I'll toy with the system update order and get back to you; hopefully that's all that needs doing. You're welcome to troubleshoot in the meantime and help investigate if interested. Right now testing changes is unfortunately restricted to ensuring all the nav-related demos function as expected.

Tricnic commented 4 years ago

My use case is a tower defense game where towers can be placed anywhere on the field and the creep will need to be able to path around them as needed. When creep (which has the NavAgent on it) get close enough to a tower, the Physics trigger enter event allows a system to add the creep to the tower's DynamicBuffer of creep within range to attack. When it gets far enough away, the trigger exit event removes it from that buffer. I'd rather not have to check a creep's position against tower positions each frame to see if it is within range. I plan to have hundreds of towers and thousands of creep on screen at once and that many distance checks could get very expensive.

Mdlich commented 4 years ago

I'm not sure if that's related, but unity.physics docs explain how to hook up into the simulation and get data from it without breaking any dependencies and stuff: https://docs.unity3d.com/Packages/com.unity.physics@0.4/manual/simulation_modification.html But maybe that's only for modifying the physics simulation behaviour..?

Otherwise, perhaps FinalJobHandle being obsolete is related: image After checking it in the ide, "GetOutputDependency" doesn't seem to exist yet...

Update: Okay it is there after updating Unity.physics to 0.4. But changing FinalJobHandle to GetOutputDependency doesn't seem to affect the issue in any way...

reeseschultz commented 4 years ago

@Mdlich It may well be possible that some upgrading is due. I've been busy working on my own game, so I've neglected this project for the past couple weeks. Like I said, the core issue here is probably system update order. I'll get back to you within the next couple days.

reeseschultz commented 4 years ago

Patched in nav/v0.10.3. How you want to handle gravity is up to you—I zeroed it in my testing. If you experience other problems, please submit more issues. Hopefully this change helps.

Edit: FYI, it was just a simple change to the system update order as expected.