Unexpected side effect with GetReverseParameterCurve

[_gile]

The Curve2d (as the Curve3d) class has a GetReverseParameterCurve method which returns a newly created Curve2d (or Curve3d) with reversed parameters as expected.

But this method also unexpectedly reverses the source curve.

var ed = Application.DocumentManager.MdiActiveDocument.Editor;
var line1 = new LineSegment2d(new Point2d(0, 0), new Point2d(2, 1));
ed.WriteMessage($"\nline1.StartPoint = {line1.StartPoint} line1.EndPoint: {line1.EndPoint}");
var line2 = line1.GetReverseParameterCurve();
ed.WriteMessage($"\nReferenceEquals(line1, line2): {ReferenceEquals(line1, line2)}");
ed.WriteMessage($"\nline1.StartPoint = {line1.StartPoint} line1.EndPoint: {line1.EndPoint}");
ed.WriteMessage($"\nline2.StartPoint = {line2.StartPoint} line2.EndPoint: {line2.EndPoint}");

line1.StartPoint = (0,0) line1.EndPoint: (2,1)
ReferenceEquals(line1, line2): False
line1.StartPoint = (2,1) line1.EndPoint: (0,0)
line2.StartPoint = (2,1) line2.EndPoint: (0,0)

[ActivistInvestor]

---

@_gile wrote:

The Curve2d (as the Curve3d) class has a GetReverseParameterCurve method which returns a newly created Curve2d (or Curve3d) with reversed parameters as expected.

But this method also unexpectedly reverses the source curve.

---

Hi @_gile . Glad you brought this up as I was bitten by this same assumption as well.

GetReverseParameterCurved() doesn't create a modified copy of the instance you call the method on. It modifies and returns the same curve you call the method on, in a new managed wrapper.

I think if you try this, it should confirm that:

Curve3d originaCurve = // assign to a Curve3d
Curve3d reversedCopy = originalCurve.GetReverseParameterCurve();

originalCurve == reversedCopy  // should be true.

IOW, the result of GetReverseParameterCurve() is just a different managed wrapper that wraps the same underlying AcGeCurve2d/3d instance which the method was invoked on.

The managed docs for that API are a verbatim copy of the native docs for the underlying AcGeCurve2d/3d::reverseParam() method, and are worded to suggest that the input curve is modified, verses it returning a modified copy of same. I would expect that if the API did return a copy of the input curve, it would make that clear.

If you have a case where you need both the original curve and a reversed clone of it then you could do something like this:

public static Curve3d GetReversedClone(this Curve3d curve)
{
   return ((Curve3d)curve.Clone()).GetReverseParameterCurve();
}

You may have noticed that there's a bug in the TryCreatePolyline() method I posted in the other thread which is directly related to this, because if it fails to create a polyline, it may reverse some of the input curves, which may be used in another way. I'm working that and a few other bugs I've found and hope to have an update on the repo soon.

You can see the bug in the attached DWG. If you try using the RegionExplodeOverrule on it, it fails.

The other bug is that when the Optimize() method that I posted encounters a closed curve that has lines/arcs at both the start and the end of the sequence, it fails to join them in to a single polyline, which it should do.

[_gile]

---

@ActivistInvestor  a écrit :

---

@_gile wrote:

The Curve2d (as the Curve3d) class has a GetReverseParameterCurve method which returns a newly created Curve2d (or Curve3d) with reversed parameters as expected.

But this method also unexpectedly reverses the source curve.

---

Hi @_gile . GetReverseParameterCurved() doesn't create a modified copy of the instance you call the method on. It returns the same curve you call the method on, in a new managed wrapper.

I think if you try this, it should confirm that:

Curve3d originaCurve = // assign to a Curve3d
Curve3d reversedCopy = originalCurve.GetReverseParameterCurve();

originalCurve == reversedCopy  // should be true.

IOW, the result of GetReverseParameterCurve() is just a different managed wrapper that wraps the same underlying AcGeCurve2d/3d instance which the method was invoked on.

The managed docs for that API are a verbatim copy of the native docs for the underlying AcGeCurve2d/3d::reverseParam() method, and are worded to suggest that the input curve is modified, verses it returning a modified copy of same. I would expect that if the API did return a copy of the input curve, it would make that clear.

If you have a case where you need both the original curve and a copy of it then you could do something like this:

public static Curve3d GetReversedClone(this Curve3d curve)
{
   return ((Curve3d)curve.Clone()).GetReverseParameterCurve();
}

---

I think GetReverseParameterCurve does create a new instance of Curve[23]d, because the following returns False.

ReferenceEquals(originalCurve, reversedCopy)

The override Curve3d.Equals method (as the == operator) seems to compare curves geometry as it does for Point3d.

var pt1 = new Point3d(1, 2, 3);
var pt2 = new Point3d(1, 2, 3);
ed.WriteMessage($"\nReferenceEquals(pt1, pt2): {ReferenceEquals(pt1, pt2)}"); // returns false
ed.WriteMessage($"\npt1 == pt2): {pt1 == pt2}"); // returns true

Anyway, if the method reverses the parameters of the instance the method is called on, it should return void instead of Curve3d (as the TransformBy method does).

[ActivistInvestor]

---

@_gile wrote:
So, whyReferenceEquals(originalCurve, reversedCopy)

returns False?

Anyway, if the method reverses the parameters of the the instance you call the method on, it should return void instead of Curve3d (as the TransformBy method does).

---

ReferenceEquals() compares managed wrappers, and GetReverseParameterCurve() returns a new managed wrapper for the Curve2d/3d that you called the method on. IOW, two managed wrapper instances can both refer to the same underlying 'wrapped' native object. That is why ReferenceEquals() can return false, while object.Equals() returns true both given the same arguments.

The override Curve3d.Equals method (as the == operator) seems to compare curves geometry as it does for Point3d.

No, it doesn't do that. Curve3d does not override object.Equals() it uses the override defined by DisposableWrapper(), and it doesn't compare the contents or geometry of two Curve3d instances for equality.

The DisposableWrapper base type overrides the object.Equals() method (and the ==/!= operators) to compare the values of the UnmanagedObject property of the two managed wrappers, to determine if both managed wrappers are wrapping the same underlying native object, which is what defines equality of two DisposableWrappers.

The GetReverseParameterCurve() method attempts to maintain parity with the underlying AcGeCurve2d/3d::reverseParam() method, which returns a reference to the instance the method was invoked on, and yes, that has a lot to do with the confusion about this:

AcGeCurve3d& reverseParam();

[ActivistInvestor]

@_gile, I was able to resolve the bugs that were causing the failure in TryCreatePolyline() as was posted earlier. Your implementation may have the same issue, so I'm sharing my fix in advance of committing it to the repo. See the comments.

/// <summary>
/// Attempts to create a CompositeCurve3d that can be
/// used to create a Polyline, from an input sequence 
/// of Curve3d elements. If the input sequence cannot
/// be used to create a Polyline, this returns null.
/// If the input sequence can be used to create a
/// Polyline, this method normalizes the input elements
/// to be in traversal order and direction before using
/// them to create the result.
/// 
/// This method returns null if the input sequence is 
/// empty or contains a single element, regardless of 
/// its type.
/// </summary>
/// <param name="curves"></param>
/// <param name="validate"></param>
/// <param name="tol"></param>
/// <returns></returns>
/// <exception cref="ArgumentNullException"></exception>
/// <exception cref="InvalidOperationException"></exception>

public static CompositeCurve3d TryCreatePolyline(this IEnumerable<Curve3d> curves,
   bool validate = false,
   Tolerance tol = default(Tolerance))
{
   if(curves is null)
      throw new ArgumentNullException(nameof(curves));
   var input = curves as Curve3d[] ?? curves.ToArray();
   if(input.Length < 2)
      return null;
   if(tol.Equals(default(Tolerance)))
      tol = Tolerance.Global;
   int count = input.Length;
   var joined = new bool[count];
   Curve3d current = input[0];
   Curve3d next = input[1];
   if(!current.IsPolySegment())
      return null;

   /// Bug fix: This code failed if the start point of the
   /// first curve is coincident with the end point of the
   /// following curve. The code wasn't checking or reversing
   /// the first curve as it does with all following curves:
   
   if(current.StartPoint.IsEqualTo(next.EndPoint, tol))
      current = current.GetReversedClone();

   if(validate)
      current.AssertIsValid();
   var output = new Curve3d[count];
   output[0] = current;
   joined[0] = true;
   var spInput = input.AsSpan();
   var spOutput = output.AsSpan();
   var spJoined = joined.AsSpan();
   int index = 1;
   while(index < count)
   {
      Point3d endPoint = spOutput[index - 1].EndPoint;
      bool found = false;
      for(int i = 0; i < count; i++)
      {
         if(spJoined[i])
            continue;
         current = spInput[i];
         if(!current.IsPolySegment())
            return null;
         if(validate)
            current.AssertIsValid();
         if(endPoint.IsEqualTo(current.StartPoint, tol))
         {
            spOutput[index] = current;
            spJoined[i] = true;
            found = true;
            break;
         }
         else if(endPoint.IsEqualTo(current.EndPoint, tol))
         {
            // Bug fix: Because this method may return null
            // and the input curves may be used in other ways,
            // clones of curves are reversed and replace the
            // original curve to avoid modifying same:
            spOutput[index] = current.GetReversedClone();
            spJoined[i] = true;
            found = true;
            break;
         }
      }
      if(!found)
      {
         throw new InvalidOperationException($"Disjoint curves (index = {index})");
      }
      index++;
   }
   return new CompositeCurve3d(output);
}

public static Curve3d GetReversedClone(this Curve3d curve)
{
   return ((Curve3d)curve.Clone()).GetReverseParameterCurve();
}

[_gile]

@ActivistInvestor

I noticed that sometimes a segment have to be joined to start of the composite curve, so, here's the way I solved it.

/// <summary>
/// Tries to convert the Curve3d sequence into a CompositeCurve3d.
/// </summary>
/// <param name="source">Collection this method applies to.</param>
/// <param name="compositeCurve">Output composite curve.</param>
/// <param name="tolerance">Tolerance used to compare end points.</param>
/// <param name="predicate">Predicate used to filter input curves 3d.</param>
/// <returns>true, if the composite curve could be created; false otherwise.</returns>
/// <exception cref="ArgumentNullException">ArgumentNullException is thrown if <paramref name="source"/> is null.</exception>
public static bool TryConvertToCompositeCurve(
    this IEnumerable<Curve3d> source,
    out CompositeCurve3d compositeCurve,
    Tolerance tolerance = default,
    Predicate<Curve3d> predicate = null)
{
    Assert.IsNotNull(source, nameof(source));

    var isValid = predicate ?? (_ => true);

    if (tolerance.Equals(default(Tolerance)))
        tolerance = Tolerance.Global;

    compositeCurve = default;

    var input = source as Curve3d[] ?? source.ToArray();

    if (!isValid(input[0]))
        return false;

    int length = input.Length;
    if (length < 2)
        return false;

    var output = new Curve3d[length];
    var done = new bool[length];

    output[0] = input[0];
    done[0] = true;
    int count = 1;
    var startPoint = output[0].StartPoint;
    var endPoint = output[0].EndPoint;

    while (count < length)
    {
        bool found = false;

        for (int i = 0; i < length; i++)
        {
            if (done[i])
                continue;

            var current = input[i];
            if (!isValid(current))
                return false;

            if (endPoint.IsEqualTo(current.StartPoint, tolerance))
            {
                endPoint = current.EndPoint;
                output[count] = current;
                found = done[i] = true;
                break;
            }
            else if (endPoint.IsEqualTo(current.EndPoint, tolerance))
            {
                endPoint = current.StartPoint;
                output[count] = current.GetReverseParameterCurve();
                found = done[i] = true;
                break;
            }
            else if (startPoint.IsEqualTo(current.EndPoint, tolerance))
            {
                startPoint = current.StartPoint;
                for (int j = count; j > 0; j--)
                    output[j] = output[j - 1];
                output[0] = current;
                found = done[i] = true;
                break;
            }
            else if (startPoint.IsEqualTo(current.StartPoint, tolerance))
            {
                startPoint = current.EndPoint;
                for (int j = count; j > 0; j--)
                    output[j] = output[j - 1];
                output[0] = current.GetReverseParameterCurve();
                found = done[i] = true;
                break;
            }
        }

        if (!found)
            return false;

        count++;
    }
    compositeCurve = new CompositeCurve3d(output);
    return true;
}

[ActivistInvestor]

Hi @_gile, yes, that's what I described above (a set of curves that starts with lines/arcs and ends with lines/arcs, which should be joined into a single polyline). There are also other special cases, such as when there is only a single line or arc between two segments that are neither, and must be left as-is.

[ActivistInvestor]

---

@_gile wrote:

@ActivistInvestor

I noticed that sometimes a segment have to be joined to start of the composite curve, so, here's the way I solved it.

---

@_gile, I looked at your code again, and I'm not sure about something.

Do you have examples of when a segment must be joined to the start of the composite curve?  In my last post, I mentioned that sometimes a BoundaryLoop's curves can start with lines/arcs and end with lines/arcs, with a spline or elliptical arc between them.  And, in that case, all of the lines/arcs should be joined into a single polyline.  But perhaps you are citing a more-general use case where user-selected curves may not form a closed figure. 

For example, given these curves extracted from a BoundaryLoop in the order shown:

0: LineSegment3d

1: ArcSegment3d

2: NurbsCurve3d

3: LineSegment3d

4: NurbsCurve3d

5: LineSegment3d

6: ArcSegment3d

The resulting set of Curve3ds that are converted to Curve entities must be:

0: CompositeCurve3d   (input elements 5, 6, 0, & 1, in that order)

1: NurbsCurve3d            (input element 2)

2: LineSegment3d         (input element 3)

3: NurbsCurve3d           (input element 4)

The LineSegment3d in the result is a special case, where a single line or arc segment that lies between two non-line/arc segments does not get converted to a polyline.

[_gile]

---

@ActivistInvestor  a écrit : Do you have examples of when a segment must be joined to the start of the composite curve?  In my last post, I mentioned that sometimes a BoundaryLoop's curves can start with lines/arcs and end with lines/arcs, with a spline or elliptical arc between them.  And, in that case, all of the lines/arcs should be joined into a single polyline.  But perhaps you are citing a more-general use case where user-selected curves may not form a closed figure. 

What I was talking about is cases where some segment have to be joined at the start to the collection in progress. The code post in reply #5 only checks if the start/end point of the current segment matches with the end point of the collection in progress.

---

[ActivistInvestor]

---

@_gile wrote:

---

What I was talking about is cases where some segment have to be joined at the start to the collection in progress. The code post in reply #5 only checks if the start/end point of the current segment matches with the end point of the collection in progress.

Hi @_gile . 

The code in reply #5 was intended only for use with curves extracted from a BoundaryLoop.

Because it is a 'loop', you always end up back where you started (e.g., the last curve added to the result should always be connected to the first curve in the result). For that purpose, I don't see any need for adding curves to the start of the chain, or the additional testing needed to do that.

If you intended your code to be for other, more general-purpose use cases involving curves that are selected by a user or what have you, then the additional testing may be needed, if the input curves are not ordered in any way, and you can't assume they form a closed, non self-intersecting figure.

But, because we can make certain assumptions about the curves extracted from a BoundaryLoop (e.g, they must form a closed figure), I would rather avoid that additional overhead when dealing only with BoundaryLoop curves, because I don't think it's needed in those cases.

[_gile]

---

@ActivistInvestor  a écrit :
The code in reply #5 was intended only for use with curves extracted from a BoundaryLoop.

---

OK, for my part, I was trying to do something that would work whatever the Curve3d collection.

[ActivistInvestor]

---

@_gile wrote:

---

@ActivistInvestor  a écrit :
The code in reply #5 was intended only for use with curves extracted from a BoundaryLoop.

---

OK, for my part, I was trying to do something that would work whatever the Curve3d collection.

---

That's fine for more general-purposes use cases, but I wanted to optimize the code that deals specifically with BoundaryLoop curves.  

I also have a more general-purpose boundary recognition solution, but it builds multiple chains of curves concurrently, and uses quad-tree spatial indexing to achieve better performance. It will take any set of Curves and join then into one or more contiguous chains. Then, then it will try to join the chains together, which avoids the need to add elements at the start of a chain, since insertions are expensive.

In your code, you might consider replacing this:

for (int j = count; j > 0; j--)
   output[j] = output[j - 1];
output[0] = current;

With:

// Access array elements through Span<T>
var spanOutput = output.AsSpan();

// Insert new element at position 0
// and shift existing elements up by 1:
spanOutput.Slice(0, count).CopyTo(spanOutput.Slice(1));
spanOutput[0] = current;