Feedback Deleting collider invalidates collider id in ObiPinConstraintsBatch.colliderIndices

[nicity]

Hello,
Our upcoming game uses Obi 6.5.4. It is co-op game with two players that are connected via tether and also both players have controllable whip, all three objects are ObiRopes.
Both players have ObiColliders with PinConstraint for both ends of the tether.
On the scene we have some attachable objects with ObiCollider that are dynamically spawned for limited time, with Enable / Disable calls.

From time to time we observe index out of bounds in PinConstraintsBatchJob.Execute.
Locally we added validation of colliderIndex wrt shapes array bounds. 
if (colliderIndex < 0 || colliderIndex >= shapes.Length)
{
    UnityEngine.Debug.LogError($"Collider index {colliderIndex} is out of range of '{shapes.Length}' Length at {i}");
    continue;
}
After fixing couple of issues on our side due to attaching whip to invalid objects (colliderIndex == -1) we started (rarely) observe of colliderIndex being beyond shapes.Length.
After extensive tracking down we found that invalidation of colliders in ObiColliderWorld.DestroyCollider may change collider index that is used in ObiPinConstraintsBatch and possibly in other places.
This situation happens for us for ObiColliders in players and their ids are near the colliderHandles.Count 
Invalidation of another collider will take collider at the end of colliderHandles and insert it to the invalidated collider place changing its index. 
int index = handle.index;
int lastIndex = colliderHandles.Count - 1;
colliderHandles.Swap(index, lastIndex);
colliderHandles[index].index = index;

ObiPinConstraintBatch will still use old collider id that will more than shapes.Length in PinConstraintsBatchJob
For what I see in the sources of ObiRope 7.1 the problem can also happen there.
I'd appreciate ideas how to address the issue easy and cleanly.

[josemendez]

From time to time we observe index out of bounds in PinConstraintsBatchJob.Execute.
Locally we added validation of colliderIndex wrt shapes array bounds. 
if (colliderIndex < 0 || colliderIndex >= shapes.Length)
{
    UnityEngine.Debug.LogError($"Collider index {colliderIndex} is out of range of '{shapes.Length}' Length at {i}");
    continue;
}
[color=#333333][size=small][font=Tahoma, Verdana, Arial, sans-serif]After fixing couple of issues on our side due to attaching whip to invalid objects (colliderIndex == -1) we started (rarely) observe of colliderIndex being beyond shapes.Length.

Hi!

It's common for collider index to be >= shapes.Length, since colliders that need to be deleted are moved to the end of the shapes array and then its length reduced by 1 (while keeping its capacity intact).

After extensive tracking down we found that invalidation of colliders in ObiColliderWorld.DestroyCollider may change collider index that is used in ObiPinConstraintsBatch and possibly in other places.

Yes, this is the intended behavior: destroying a collider changes the index of both the collider being destroyed and of the collider at the end of the list at that moment, since we use a remove at swap back pattern for deletion (similar to this).

For this reason particle attachments check for changes to the handle's collider index and mark the constraint dirty in case it has changed, so that the collider index is updated in ObiPinConstraintsBatch. See ObiParticleAttachment's UpdateAttachment() method for reference:

Code:

// in case the handle has been updated/invalidated (for instance, when disabling the target collider) rebuild constraints:
if (attachedCollider != null &&
     attachedCollider.Handle != null &&
     attachedCollider.Handle.index != attachedColliderHandleIndex)
{
     attachedColliderHandleIndex = attachedCollider.Handle.index;
     m_Actor.SetConstraintsDirty(Oni.ConstraintType.Pin);
}

ObiPinConstraintBatch will still use old collider id that will more than shapes.Length in PinConstraintsBatchJob

In case you're manually managing pin constraints, you must manually update their corresponding collider index when needed and set the constraints dirty (as you would when modifying any constraint data).

Remember that these are indices, not IDs, so they do not persist over time. The same applies to particle indices engine-wide. This is the reason ObiResourceHandle<T> and ObiColliderHandle exist - otherwise we could just work with a value type (a simple integer), instead of passing a reference type: by passing a class instance around the engine can modify its data and anyone holding a reference to it can immediately see it change, no need for the engine to know who is looking at the instance.

If you're encountering this issue when using particle attachments instead of manually managed pin constraints, would it be possible for you to share a project that reproduces the problem by sending it to support(at)virtualmethodstudio.com?

let me know if you need further help. kind regards,

[nicity]

Thanks for the quick reply! I will take a look for ObiParticleAttachment's UpdateAttachment
Changed type of the thread to be "Feedback", as the observed behaviour is a bit unexpected, of course it is for the performance sake

Hi!

It's common for collider index to be >= shapes.Length, since colliders that need to be deleted are moved to the end of the shapes array and then its length reduced by 1 (while keeping its capacity intact).


Yes, this is the intended behavior: destroying a collider changes the index of both the collider being destroyed and of the collider at the end of the list at that moment, since we use a remove at swap back pattern for deletion (similar to this).

For this reason particle attachments check for changes to the handle's collider index and mark the constraint dirty in case it has changed, so that the collider index is updated in ObiPinConstraintsBatch. See ObiParticleAttachment's UpdateAttachment() method for reference:

Code:

// in case the handle has been updated/invalidated (for instance, when disabling the target collider) rebuild constraints:
if (attachedCollider != null &&
     attachedCollider.Handle != null &&
     attachedCollider.Handle.index != attachedColliderHandleIndex)
{
     attachedColliderHandleIndex = attachedCollider.Handle.index;
     m_Actor.SetConstraintsDirty(Oni.ConstraintType.Pin);
}

In case you're manually managing pin constraints, you must manually update their corresponding collider index when needed and set the constraints dirty (as you would when modifying any constraint data).

Remember that these are indices, not IDs, so they do not persist over time. The same applies to particle indices engine-wide. This is the reason ObiResourceHandle<T> and ObiColliderHandle exist - otherwise we could just work with a value type (a simple integer), instead of passing a reference type: by passing a class instance around the engine can modify its data and anyone holding a reference to it can immediately see it change, no need for the engine to know who is looking at the instance.

If you're encountering this issue when using particle attachments instead of manually managed pin constraints, would it be possible for you to share a project that reproduces the problem by sending it to support(at)virtualmethodstudio.com?

let me know if you need further help. kind regards,

[josemendez]

Thanks for the quick reply! I will take a look for ObiParticleAttachment's UpdateAttachment
Changed type of the thread to be "Feedback", as the observed behaviour is a bit unexpected, of course it is for the performance sake

Sure It's a combination of performance + a Job system limitation:

Deleting an element by taking it out from a large array is O(n) in the worst case, since you need to move all elements after it one position back. Using swap is O(1) instead, so much more performant. You only change the index of 2 elements (the one being deleted and the last one) instead of potentially the indices of all of them. This is the reason why we swap entries in the collider arrays.

Either way, we're moving data around in the array. For the user to keep a reference to the data even when its moved around, the system must return a handle, which is a reference type.

The problem with this is that jobs in Unity don't accept managed reference types, so we can't pass the handle directly to a job and have it grab the index automatically. Instead, we pass the index inside the handle to the job by copying it, since it is a value type. This means that when the handle changes, the user must take action to update the data that gets passed to the jobs.

In future updates we'll try to figure out a way to simplify or entirely get rid of this pitfall. If you need help figuring out the exact fix for your current code, I can help if you share it.

kind regards,