Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice

Bug PhysicsShapeGroup2D generates ArgumentOutOfRangeException

Discussion in '2D' started by LoupAndSnoop, Nov 18, 2023.

  1. LoupAndSnoop

    LoupAndSnoop

    Joined:
    Jun 23, 2023
    Posts:
    39
    I am making shapes in a CustomCollider2D.
    Part 1: Making the shape by GetShapes on CompositeCollider2D compCol. Setting them into CustomCollider2D custCollider:
    Code (CSharp):
    1. custCollider.ClearCustomShapes();
    2.  
    3. // Make shapes: Polygon shape + outline shape.
    4. compCol.GenerateGeometry();
    5. compCol.GetShapes(shapes);
    6. // Change geometry type between outline and polygons
    7. compCol.geometryType = (CompositeCollider2D.GeometryType)(1 - (int)compCol.geometryType);
    8.  
    9. compCol.GenerateGeometry();
    10. compCol.GetShapes(tempShapes);
    11. shapes.Add(tempShapes); // shapes is now WRONG. See log below
    12. custCollider.SetCustomShapes(shapes);
    In debug mode, after calling .Add, the contents of shapes is wrong.
    upload_2023-11-18_10-41-3.png
    upload_2023-11-18_10-40-16.png
    You see that after calling .Add, shape 1 has 4 vertices, and start index 0 (correct). However, shape2 then has vertexStartIndex = 5. which is incorrect (it should be 4).

    This makes the following method throw an ArgumentOutOfRange exception.
    Code (CSharp):
    1. public static string ToStringVertices(this PhysicsShapeGroup2D shapes) {
    2.      List<Vector2> vertices = new();
    3.      for (int i = 0; i < shapes.shapeCount; i++) {
    4.           shapes.GetShapeVertices(i, vertices); // This breaks at shape index = 1.
    5.      }
    tl;dr I think PhysicsShapeGroup2D.Add is making the wrong vertexStart index.
    This is in Unity 2022.3.2f1
     
  2. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
    To clarify, are you simply saying you have a collider and ask for the shapes and what is returned is wrong in terms of the vertex start index on the second shape or that the collider has a single shape, you add one and it's in the wrong position? It's not clear at all what parts of the first code are important i.e. shapes, tempshapes, switching the composite etc.

    Would you mind sending me a simple reproduction of this either here or DM on the forum?

    EDIT: At first I thought a bug might have been introduced because of a recent fix but that fix landed in 2022.3.13f1 so it cannot be related and must be an existing issue. Curiously enough the fix allows these kinds of gaps but Add should not introduce them in any version.
     
  3. LoupAndSnoop

    LoupAndSnoop

    Joined:
    Jun 23, 2023
    Posts:
    39
    I also found that the gizmo to display a CustomCollider2D does not match the contents of its PhysicsShapeGroup2D. I have one shape which is just one single looping Edge shape (with edge radius), and several polygon shapes. That generates this gizmo:
    upload_2023-11-18_11-51-9.png
    The broad lines inside the polygon reflect the radius of the edge shape. However, those diagonals are not part of the edge shape (looking into the PhysicsShapeGroup2D in debug mode), and the polygon shapes should not be able to have a radius.
     
    Last edited: Nov 18, 2023
  4. LoupAndSnoop

    LoupAndSnoop

    Joined:
    Jun 23, 2023
    Posts:
    39
    The issue originates in .Add. My workaround is to use .AddEdges and .AddPolygon, which produces the right result.

    Looking into the source code, it looks like .Add uses List.AddRange to add the list of vertices from the new shapes to the end of the list (of all vertices of the main group). Then, it changes the vertexStartIndex of all shapes previously in the object to start later. This means .Add is completely wrong, because it's moving the index of the old shapes without moving the vertices. If the new vertices added are being added to the end of the list (of all vertices), then it's just the new shape that just needs to know vertexStartIndex.

    -------------

    Anyway, part of how I noticed all this was by making and using this extension method for debugging. It doesn't display start vertexIndex, but it's still handy. Hopefully helps the team in their debugging.
    Code (CSharp):
    1. /// <summary> Make a string of all the vertices in all the shapes in the shape group for debugging purposes. </summary>
    2. public static string ToStringDetailed(this PhysicsShapeGroup2D shapes) {
    3.     System.Text.StringBuilder sb = new();
    4.     List<Vector2> vertices = new();
    5.     for (int i = 0; i < shapes.shapeCount; i++) {
    6.         shapes.GetShapeVertices(i, vertices);
    7.         sb.Append($"Shape #{i}: {shapes.GetShape(i).shapeType}, {vertices.Count} vertices");
    8.  
    9.         // Special case of empty shape.
    10.         if (vertices.Count == 0) { sb.Append(".\n"); continue; }
    11.         sb.Append(" at ");
    12.  
    13.         foreach (Vector2 vertex in vertices) {
    14.             sb.Append(vertex + ", ");
    15.         }
    16.         sb.Remove(sb.Length - 2, 2); // Remove last comma
    17.         sb.Append(".\n");
    18.     }
    19.     return sb.ToString();
    20. }
    Output looks like:
    Shape #0: Edges, 9 vertices at (2.94, 0.06), (2.94, 5.94), (0.94, 5.94), (0.94, 6.94), (0.06, 6.94), (0.06, 5.06), (2.06, 5.06), (2.06, 0.06), (2.94, 0.06).
    Shape #1: Polygon, 4 vertices at (2.94, 0.06), (2.94, 5.94), (2.06, 5.06), (2.06, 0.06).
    Shape #2: Polygon, 4 vertices at (2.94, 5.94), (0.94, 5.94), (0.06, 5.06), (2.06, 5.06).
    Shape #3: Polygon, 4 vertices at (0.94, 5.94), (0.94, 6.94), (0.06, 6.94), (0.06, 5.06).
     
    Last edited: Nov 18, 2023
  5. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
    I was initially confused by this but taking a look at the code and the unit-tests (and there are a lot of them!) I can see now why that was missed because it's not going to to check the start-index (similar to the code example here). That's definately wrong but a trivial fix too.

    I'll get a fix done for that over the weekend and backported.

    Thanks for the report!
     
    LoupAndSnoop likes this.
  6. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
    Yes, this line is wrong.

    It should be:
    Code (CSharp):
    1. var startVertexOffset = m_GroupState.m_Vertices.Count;
    i.e. use the vertex count of the existing shape-group and not the incoming one being added.

    I've fixed that and added the missing parts of the unit-test there to check the vertexStartIndex of all shapes.
     
  7. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
    Just to update you that I'm queuing the fix into 2023.3, 2023.2 and 2022.3 LTD so it'll go for review next week.
     
  8. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
    Can you elaborate on this or provide a project to reproduce it. The gizmos here, like all physics gimozs, don't use shape-groups or anything else, they simply look at the actual physics shape that are active in the collider so them being wrong seems very strange indeed.

    Polygon shapes can have a radius. You use that for BoxCollider2D for instance.
     
  9. LoupAndSnoop

    LoupAndSnoop

    Joined:
    Jun 23, 2023
    Posts:
    39
    PolygonCollider2D and CompositeCollider2D (polygon mode) did not have radius (neither did CustomCollider2D.AddPolygon), so I assumed the radius did not make a difference. I see now I was wrong. This makes it clear that the Gizmo thing is not a bug. Gizmo is accurate.

    A whole project might be tough to send, but I used this method to generate it. You might still want this code for the next bug I found:

    Code (CSharp):
    1. private static PhysicsShapeGroup2D tempShapes = new(); // Temporary for non-alloc calculation.
    2. private static List<Vector2> tempVertices = new(); // Temporary for non-alloc calc
    3. /// <summary> Take in a custom collider, and fill it based on the shape from the composite collider.
    4. /// Assume the composite collider's geometry is already up-to-date. </summary>
    5. public static void MakeRoundedComposite(CustomCollider2D custCollider,
    6.         CompositeCollider2D compCol, ref PhysicsShapeGroup2D shapes) {
    7.     custCollider.CopySettingsFrom(compCol); // Copy overrides etc.
    8.     custCollider.ClearCustomShapes();
    9.  
    10.     // Make shapes: Polygon shape + outline shape.
    11.     compCol.GetShapes(shapes);
    12.     Debug.Assert(shapes.shapeCount > 0, "The composite collider should have some shapes " +
    13.         "generated before we start. There are no shapes in this collider!");
    14.  
    15.     // This line changes geometry type from outline to polygons, or polygons to outlines.
    16.     compCol.geometryType = (CompositeCollider2D.GeometryType)(1 - (int)compCol.geometryType);
    17.     compCol.GenerateGeometry();
    18.     compCol.GetShapes(tempShapes);
    19.  
    20.     // We would use .Add, but that is bugged for shapegroups... This is all equivalent to shapes.Add(tempShapes);
    21.     for (int i = 0; i < tempShapes.shapeCount; i++) {
    22.         tempShapes.GetShapeVertices(i, tempVertices);
    23.         if (compCol.geometryType == CompositeCollider2D.GeometryType.Outlines)
    24.             shapes.AddEdges(tempVertices, compCol.edgeRadius);
    25.         else shapes.AddPolygon(tempVertices);
    26.     }
    27.  
    28.     custCollider.SetCustomShapes(shapes);
    29. }
    30.  
    31. /// <summary> Copy all settings from a given Collider2D into this collider2D. </summary>
    32. public static void CopySettingsFrom(this Collider2D copyReceiver, Collider2D originalToBeCopied) {
    33.     // Set generic values
    34.     copyReceiver.enabled = originalToBeCopied.enabled;
    35.     copyReceiver.isTrigger = originalToBeCopied.isTrigger;
    36.     if (!(copyReceiver is CustomCollider2D)) // Types that can't be composited
    37.         copyReceiver.usedByComposite = originalToBeCopied.usedByComposite;
    38.     copyReceiver.usedByEffector = originalToBeCopied.usedByEffector;
    39.  
    40.     // Layer overrides
    41.     copyReceiver.includeLayers = originalToBeCopied.includeLayers;
    42.     copyReceiver.excludeLayers = originalToBeCopied.excludeLayers;
    43.     copyReceiver.callbackLayers = originalToBeCopied.callbackLayers;
    44.     copyReceiver.contactCaptureLayers = originalToBeCopied.contactCaptureLayers;
    45.     copyReceiver.forceSendLayers = originalToBeCopied.forceSendLayers;
    46.     copyReceiver.forceReceiveLayers = originalToBeCopied.forceReceiveLayers;
    47.     copyReceiver.layerOverridePriority = originalToBeCopied.layerOverridePriority;
    48.  
    49.     if (originalToBeCopied.attachedRigidbody.useAutoMass) copyReceiver.density = originalToBeCopied.density;
    50.     copyReceiver.sharedMaterial = originalToBeCopied.sharedMaterial;
    51.  
    52.     copyReceiver.offset = originalToBeCopied.offset;
    53. }
    54.  
    Just have a TilemapCollider2D, CompositeCollider2D using it, and CustomCollider2D on the same gameobject. Call this method to populate the CustomCollider2D with the CompositeCollider's shapes. Then disable the TilemapCollider2D, and destroy the CompositeCollider2D to clean it up.

    ----------
    Collider2D.Distance Issue
    I have also found another bug with these shapes. Using Collider2D.Cast, Collider2D.Distance, Collider2D.Overlap methods work fine with these CustomCollider2D. If I call .Distance from this collider to a BoxCollider2D/CompositeCollider2D/EdgeCollider2D, etc works. Collider2D.Distance also works if I leave out the Edge shape from this CustomCollider2D. However, if I call Collider2D.Distance from one of these CustomCollider2D to another CustomCollider2D (both have these sorts of shapes), I trip the assertion:
    "Assertion failed on expression: 'contact != NULL'
    UnityEngine.Collider2D.Distance (UnityEngine.Collider2D)"

    One call of .Distance can trip this assertion multiple times. I believe this to be in error (or otherwise, I would like to know what is going on.
    tl;dr On this bug: x.Distance(y) is okay as long as x and y are not BOTH CustomCollider2D (as generated by the code above).

    ------------
    EDIT: The Gizmo is accurate to the shape (which is good). I simplified my code/shape to no longer have the extra edge shape, so I do not need help on the .Distance bug (because it now disappears when simplifying the shape). That said, I think it is still worth finding out why Collider2D.Distance constantly trips assertions for this sort of shape.

    Two suggestions that might simplify this in future would be to make edgeRadius work on Polygon mode Composite colliders (or some other field to modify it). Also, it would be helpful if CustomCollider2D were not a sealed class, so we could make our own Collider2D with custom fields/parameters etc.
     
    Last edited: Nov 19, 2023
  10. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
    Sorry, I'm trying to narrow things down and more stuff is being posted on the thread so it's spinning too many plates.

    Don't mistake what colliders expose to what the low-level shapes expose. The whole point of them are that they are low-level and expose raw properties. Polygon shapes can have a radius otherwise a BoxCollider2D couldn't produce a polygon shape with a radius! We don't expose a radius on the PolygonCollider2D because it means doesn't all sorts of extra work to ensure that only the exterior polygon shapes use it and not the interior ones and that's a lot of extra calculations. Same goes for the CompositeCollider2D but there's nothing stopping you performing the path->polygons with the same libraries we use and using a radius for instance. :)

    If you think you've found a bug then I'd ask that you submit a bug report so it can be reproduced. It won't be with those shapes, it's more likely that the specific Distance call is finding something unexpected and by the looks of it, the Box2D time-of-impact calculation found the distance but asking for a contact produced no contact and this is a failing in Box2D with an inconsistency between the two and often, the radius is the problem. It's why we have to do some hacks like inflate the radius when we ask for a contact. Likely here's it's slightly away by fractional amounts. It's been a real PITA in Box2D for a bunch of queries.
     
  11. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
  12. MelvMay

    MelvMay

    Unity Technologies

    Joined:
    May 24, 2013
    Posts:
    11,409
    FYI: The fix is complete and will be in:
    • 2022.3.15f1
    • 2023.2.3f1
    • 2023.3.0a17
     
    LoupAndSnoop likes this.