Browse Source

Build Islands from Constraints was activating bodies that were not taken into account when running single threaded (and in rare cases not with multiple threads) causing non-deterministic simulation, fixed this race condition (#199)

Jorrit Rouwe 3 years ago
parent
commit
22c5cbffdf

+ 1 - 1
Docs/Architecture.md

@@ -354,7 +354,7 @@ This job will go through all non-contact constraints and determine which constra
 
 
 This job will go through all non-contact constraints and assign the involved bodies and constraint to the same island. Since we allow concurrent insertion/removal of bodies we do not want to keep island data across multiple simulation steps, so we recreate the islands from scratch every simulation step. The operation is lock-free and O(N) where N is the number of constraints. 
 This job will go through all non-contact constraints and assign the involved bodies and constraint to the same island. Since we allow concurrent insertion/removal of bodies we do not want to keep island data across multiple simulation steps, so we recreate the islands from scratch every simulation step. The operation is lock-free and O(N) where N is the number of constraints. 
 
 
-If a constraint connects an active and a non-active body, the non-active body is woken up.
+If a constraint connects an active and a non-active body, the non-active body is woken up. One find collisions job will not start until this job has finished in order to pick up any collision testing for newly activated bodies.
 
 
 ### Find Collisions
 ### Find Collisions
 
 

File diff suppressed because it is too large
+ 0 - 0
Docs/PhysicsSystemUpdate.gliffy


File diff suppressed because it is too large
+ 0 - 0
Docs/PhysicsSystemUpdate.svg


+ 7 - 2
Jolt/Physics/PhysicsSystem.cpp

@@ -183,7 +183,9 @@ void PhysicsSystem::Update(float inDeltaTime, int inCollisionSteps, int inIntegr
 	int num_determine_active_constraints_jobs = max(1, min(((int)mConstraintManager.GetNumConstraints() + cDetermineActiveConstraintsBatchSize - 1) / cDetermineActiveConstraintsBatchSize, max_concurrency - 2));
 	int num_determine_active_constraints_jobs = max(1, min(((int)mConstraintManager.GetNumConstraints() + cDetermineActiveConstraintsBatchSize - 1) / cDetermineActiveConstraintsBatchSize, max_concurrency - 2));
 
 
 	// Number of find collisions jobs to run depends on number of active bodies.
 	// Number of find collisions jobs to run depends on number of active bodies.
-	int num_find_collisions_jobs = max(1, min(((int)num_active_bodies + cActiveBodiesBatchSize - 1) / cActiveBodiesBatchSize, max_concurrency));
+	// Note that when we have more than 1 thread, we always spawn at least 2 find collisions jobs so that the first job can wait for build islands from constraints
+	// (which may activate additional bodies that need to be processed) while the second job can start processing collision work.
+	int num_find_collisions_jobs = max(max_concurrency == 1? 1 : 2, min(((int)num_active_bodies + cActiveBodiesBatchSize - 1) / cActiveBodiesBatchSize, max_concurrency));
 
 
 	// Number of integrate velocity jobs depends on number of active bodies.
 	// Number of integrate velocity jobs depends on number of active bodies.
 	int num_integrate_velocity_jobs = max(1, min(((int)num_active_bodies + cIntegrateVelocityBatchSize - 1) / cIntegrateVelocityBatchSize, max_concurrency));
 	int num_integrate_velocity_jobs = max(1, min(((int)num_active_bodies + cIntegrateVelocityBatchSize - 1) / cIntegrateVelocityBatchSize, max_concurrency));
@@ -237,10 +239,12 @@ void PhysicsSystem::Update(float inDeltaTime, int inCollisionSteps, int inIntegr
 			step.mFindCollisions.resize(num_find_collisions_jobs);
 			step.mFindCollisions.resize(num_find_collisions_jobs);
 			for (int i = 0; i < num_find_collisions_jobs; ++i)
 			for (int i = 0; i < num_find_collisions_jobs; ++i)
 			{
 			{
+				// Build islands from constraints may activate additional bodies, so the first job will wait for this to finish in order to not miss any active bodies
+				int num_dep_build_islands_from_constraints = i == 0? 1 : 0;
 				step.mFindCollisions[i] = inJobSystem->CreateJob("FindCollisions", cColorFindCollisions, [&step, i]() 
 				step.mFindCollisions[i] = inJobSystem->CreateJob("FindCollisions", cColorFindCollisions, [&step, i]() 
 					{ 
 					{ 
 						step.mContext->mPhysicsSystem->JobFindCollisions(&step, i); 
 						step.mContext->mPhysicsSystem->JobFindCollisions(&step, i); 
-					}, num_apply_gravity_jobs + num_determine_active_constraints_jobs + 1); // depends on: apply gravity, determine active constraints, finish building jobs
+					}, num_apply_gravity_jobs + num_determine_active_constraints_jobs + 1 + num_dep_build_islands_from_constraints); // depends on: apply gravity, determine active constraints, finish building jobs, build islands from constraints
 			}
 			}
 
 
 			if (is_first_step)
 			if (is_first_step)
@@ -290,6 +294,7 @@ void PhysicsSystem::Update(float inDeltaTime, int inCollisionSteps, int inIntegr
 				{ 
 				{ 
 					context.mPhysicsSystem->JobBuildIslandsFromConstraints(&context, &step);
 					context.mPhysicsSystem->JobBuildIslandsFromConstraints(&context, &step);
 
 
+					step.mFindCollisions[0].RemoveDependency(); // The first collisions job cannot start running until we've finished building islands and activated all bodies
 					step.mFinalizeIslands.RemoveDependency(); 
 					step.mFinalizeIslands.RemoveDependency(); 
 				}, num_determine_active_constraints_jobs + 1); // depends on: determine active constraints, finish building jobs
 				}, num_determine_active_constraints_jobs + 1); // depends on: determine active constraints, finish building jobs
 
 

Some files were not shown because too many files changed in this diff