Dui3 124 receiving curves arcs ellipses nurbs curves #3521
Conversation
|
Just a general comment: it would be good to align the ArcGIS converter project to have the same structure as the other DUI3 connectors: |
|
|
||
| if (angleStart == null || fullAngle == null || radius == null) | ||
| if ( | ||
| target.plane.normal.x != 0 || target.plane.normal.y != 0 || target.plane.xdir.z != 0 || target.plane.ydir.z != 0 |
There was a problem hiding this comment.
Here it looks like you're checking for xy planarity on the world xy plane: if arcgis can't support xy planar curves on other planes (z is non-zero), then the exception message should be more specific since 2d arc can mean planar arcs on planes in any 3d orientation
There was a problem hiding this comment.
Also, just a caution that we can't guarantee that the plane property is always accurate: the absolute safest way to test for planarity is to check the start and end points
| @@ -28,6 +28,12 @@ public ACG.Polyline Convert(SOG.Circle target) | |||
| { | |||
| throw new SpeckleConversionException("Conversion failed: Circle doesn't have a radius"); | |||
| } | |||
| if ( | |||
There was a problem hiding this comment.
Since there's no direct conversion from Speckle Curve to a native element and you're just converting the displayvalue, my preference is to remove this converter and use fallback conversion for Speckle Curves.
| if (target.firstRadius == null || target.secondRadius == null) | ||
| { | ||
| throw new SpeckleConversionException("Conversion failed: Ellipse doesn't have 1st and 2nd radius"); | ||
| throw new ArgumentException("Invalid Ellipse provided"); |
There was a problem hiding this comment.
More specific exception here: "Ellipse is missing a first or second radius" eg
| ); | ||
|
|
||
| pointsOriginal.Add(pointOnEllipse); | ||
| throw new ArgumentException("Only 2d-Ellipse shape is supported"); |
| { | ||
| pointsOriginal.Add(pointsOriginal[0]); | ||
| majorAxeRadius = (double)target.secondRadius; |
|
|
||
| // reverse new segment if needed | ||
| if (points.Count > 0 && newPts.Count > 0 && points[^1] != newPts[0] && points[^1] == newPts[^1]) | ||
| if ( |
There was a problem hiding this comment.
This should take into account the edge case where the first segment is reversed - super annoying but can't assume that the first polycurve segment endpoint is correct
There was a problem hiding this comment.
this line is checking for this edge case, no?
There was a problem hiding this comment.
concluded that we should throw here if segments are incorrectly oriented, instead of attempting to reverse them
There was a problem hiding this comment.
This looks fine code-wise, but won't this heavily conflict with your other PRs?
Not sure we should be keeping an order of merging @KatKatKateryna
|
@AlanRynne only 1 draft PR is waiting for these changes, will only affect several files. The other PRs are not touching converters. After @clairekuang approves requested changes, can be safely merged (blocked atm) |
KatKatKateryna
deleted the
DUI3-124-Receiving-Curves-Arcs-Ellipses-Nurbs-Curves
branch
Description & motivation
Changes:
To-do before merge:
Screenshots:
Validation of changes:
Checklist:
References