refactor(arcgis): overhaul of arcgis connector and converter patterns to enable data extraction #439

clairekuang · 2024-12-07T19:08:54Z

Description & motivation

This pr is a big refactor of the arcgis connector to enable data extraction workflows (sending ArcgisObject: IDataObject). It includes the following changes to align arcgis with all other data extractor patterns:

Objects of CoreObjectBase type are now passed to the single TopLevelConverter. Previously, this was a mixture of objevcts and layers
All layer logic is handled in the connector. Previously, LasDatasetLayers were passed to the converter. These are now passing individual points to the converter, instead of sending the layer as a pointcloud
DisplayValueExtractor and PropertiesExtractor classes added to the converter to assist in object creation in the top level converter.
ColorUnpacker class added to connector to handle color proxy creation during mapmember iteration
CRS information is added to the root collection, instead of to every single layer

Some async logic was refactored in the connector to improve safety.
Some poor conversion logic in the converters was removed to improve performance.
Reduced complexity in root object builder, removed pragma warning disable.

Note: receive needs an equally large refactor to handle DataObjects only. This pr will disable the receive binding until that refactor is completed. Metrics from qgis v2 show that sending is much more common than receiving (~2500 incidents vs 220). Arcgis currently has 1 MAU.

Changes:

arcgis converter and connector

To-do before merge:

Test sending
disable receiving
add send caching

Screenshots:

In arcgis file:

With this pr:

https://latest.speckle.systems/projects/2295cb26a0/models/874439f06c

Validation of changes:

Tested sending the sample arcgis file. Note: our pro license does not include 3d extensions, meaning I couldn't test point clouds.

…up-arcgis-to-enable-data-workflows

linear · 2024-12-07T19:08:56Z

CNX-830 Refactor and cleanup ArcGIS to enable data workflows

codecov · 2024-12-07T19:11:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 9.43%. Comparing base (9bab0ff) to head (792edea).
Report is 1 commits behind head on arcgis_refactor.

Additional details and impacted files

@@               Coverage Diff               @@
##           arcgis_refactor    #439   +/-   ##
===============================================
  Coverage             9.43%   9.43%           
===============================================
  Files                  223     223           
  Lines                 4208    4208           
  Branches               487     487           
===============================================
  Hits                   397     397           
  Misses                3795    3795           
  Partials                16      16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

oguzhankoral

Few change requests
Mostly questions

@clairekuang happy to do live PR review together over my comments

oguzhankoral · 2024-12-10T23:57:57Z

Connectors/ArcGIS/Speckle.Connectors.ArcGIS3/HostApp/ArcGISLayerUnpacker.cs

    }

-    return gisLayer;
+    parentCollection.elements.Add(collection);


seems like we pass parentCollection to just add it at the end of function. I would put it outside and rename function CreateAddMapMemberCollection for single responsibilty

oguzhankoral · 2024-12-10T23:59:47Z

Connectors/CSi/Speckle.Connectors.CSiShared/Speckle.Connectors.CSiShared.projitems

@@ -16,14 +16,12 @@
    <Compile Include="$(MSBuildThisFileDirectory)HostApp\CSiSendCollectionManager.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)Operations\Send\CSiRootObjectBuilder.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)Plugin\CSiPluginBase.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)Plugin\SpeckleFormBase.cs">
-      <SubType>Form</SubType>


hmm, why removed underlying Form? maybe missing context here

will remove all csi related changes

oguzhankoral · 2024-12-11T00:00:58Z

Converters/ArcGIS/Speckle.Converters.ArcGIS3/Extensions/SpeckleApplicationIdExtensions.cs

@@ -0,0 +1,21 @@
+namespace Speckle.Converters.ArcGIS3.Extensions;
+
+public static class SpeckleApplicationIdExtensions


i like these classes over host apps which application ids are mystery

oguzhankoral · 2024-12-11T00:02:00Z

Converters/ArcGIS/Speckle.Converters.ArcGIS3/ToHost/Raw/GeometryToHostConverter.cs

@@ -8,17 +8,14 @@ public class GeometryToHostConverter : ITypedConverter<IReadOnlyList<Base>, ACG.
 {
  private readonly ITypedConverter<List<SOG.Polyline>, ACG.Polyline> _polylineConverter;
  private readonly ITypedConverter<List<SOG.Point>, ACG.Multipoint> _multipointConverter;
-  private readonly ITypedConverter<List<SOG.Polygon>, ACG.Polygon> _polygonConverter;


out of curiosity what is the difference Polygon and Polyline apart from isClosed prop

oguzhankoral · 2024-12-11T00:03:12Z

Converters/ArcGIS/Speckle.Converters.ArcGIS3/ToSpeckle/Helpers/DisplayValueExtractor.cs

+        break;
+
+      default:
+        yield break;


should we log?

oguzhankoral · 2024-12-11T00:17:32Z

...rters/ArcGIS/Speckle.Converters.ArcGIS3/ToSpeckle/Raw/MultipatchFeatureToSpeckleConverter.cs

+
+    for (int i = 0; i < points.Count; i++)
+    {
+      if (i >= 2 && (i + 1) % 3 == 0) // every 3 new points is a new triangle


similar thoughts as above comment

oguzhankoral · 2024-12-11T00:19:14Z

...rters/ArcGIS/Speckle.Converters.ArcGIS3/ToSpeckle/Raw/MultipatchFeatureToSpeckleConverter.cs

+  }
+
+  private SOG.Mesh GetMeshFromTriangleFanPatch(List<ACG.MapPoint> points)
+  {


I want to know what are they and from what to what we are trying to convert. mostly it is a question for @KatKatKateryna. could you document these scenarios on figma by giving some examples with shapes or real scenarios?

oguzhankoral · 2024-12-11T00:20:44Z

Converters/ArcGIS/Speckle.Converters.ArcGIS3/ToSpeckle/Raw/PointToSpeckleConverter.cs

-        GeometryEngine.Instance.Project(target, _settingsStore.Current.ActiveCRSoffsetRotation.SpatialReference)
-        is not MapPoint reprojectedPt
+        ACG.GeometryEngine.Instance.Project(target, _settingsStore.Current.ActiveCRSoffsetRotation.SpatialReference)
+        is not ACG.MapPoint reprojectedPt
      )
      {
        throw new ValidationException(


i know this was pre-existing but curious what is the ValidationException, don't we suppose to use some conversion typed exceptions

oguzhankoral · 2024-12-11T00:22:28Z

...S/Speckle.Converters.ArcGIS3/ToSpeckle/TopLevel/CoreObjectsBaseToSpeckleTopLevelConverter.cs

+    Dictionary<string, object?> properties = _propertiesExtractor.GetProperties(target);
+
+    // get application id. test for subtypes before defaulting to base type.
+    string applicationId = target is ACD.Row row


conditions a bit confusing but...

oguzhankoral · 2024-12-11T00:23:49Z

Converters/ArcGIS/Speckle.Converters.ArcGIS3/Utils/ArcGISFieldUtils.cs

  {
-    if (target["attributes"] is Base attributes)
+    if (target["fields"] is Dictionary<string, string> attributes)


do we attach properties to collections in arcgis?

yes, some applications like arcgis and navis have meaningful properties on collections (not just objects) - we don't have a convention for handling this, could be some DataCollection: Collection (IDataCollection:IProperties) class in the future

…up-arcgis-to-enable-data-workflows

KatKatKateryna

Color Proxies are sending wrong colors for some objects: https://latest.speckle.systems/projects/2af60ce1b6/models/f537cf39e4@cc67e77a17

and https://latest.speckle.systems/projects/2af60ce1b6/models/f537cf39e4@ccff19500a
Error without clear stack trace when sending group layers:

KatKatKateryna · 2024-12-12T14:59:15Z

Connectors/ArcGIS/Speckle.Connectors.ArcGIS3/Operations/Send/ArcGISRootObjectBuilder.cs

+        ["name"] = sr.Name,
+        ["unit"] = sr.Unit.Name,
+        ["centralMeridian"] = sr.CentralMeridian,
+        ["wkt"] = sr.Wkt,


Authority ID ("Wkid" property) seems to be removed, and I believe should be added back (it's a universal identifier of the coordinate system, and will always exist unless the user created a custom CRS)

at the same time, CentralMeridian doesn't seem to be that important

* registed layer unpacker * nested layers logic moved to layerUnpacker * killed VectorLayerConverter * exposed Feature converter and Attributes in RootObjBuilder * send clean GisObject * refactor Rasters and PointClouds * send everything through 1 converter * proper object type * clean GIS objects and conversions * better layer names on Receive * fix raster position on send * switching to GIS layer, removing native geometry type checks (but keeping field check) * fix revit materials on child objects * remove IGisFeature interface * typo * remove empty converter * switch from attributes to properties * add required keyword * required attribute fix * attach dynamic "geometry" field to converted Polygons * refactor(arcgis): overhaul of arcgis connector and converter patterns to enable data extraction (#439) * rework 1 * additional restructuring of root object builder and converters * finalizes classes and patterns for converter and connector * fixes build errors * Update ArcGISHostObjectBuilder.cs * fixes some polygon conversions * refactor color manager to separate color unpacker * Delete GISLayerGeometryType.cs * disables receive * pr review issues * Update ArcGISRootObjectBuilder.cs * Update ArcGISRootObjectBuilder.cs * Update ArcGISRootObjectBuilder.cs * Update ArcGISRootObjectBuilder.cs * fixed color bugs * Update ArcGISLayerUnpacker.cs * don't cache repeated layers * Revert "don't cache repeated layers" This reverts commit 48af025. * remove flattened layers from root commit builder * Revert "remove flattened layers from root commit builder" This reverts commit e0e7b1f. * layer order and duplication fixed * fix applicationIds * fix raster appId * yield; add comment * move appId to extensions --------- Co-authored-by: Claire Kuang <[email protected]>

clairekuang added 5 commits December 7, 2024 14:25

rework 1

9f2f12c

Merge branch 'arcgis_refactor' into claire/cnx-830-refactor-and-clean…

14b9077

…up-arcgis-to-enable-data-workflows

additional restructuring of root object builder and converters

f788d5d

finalizes classes and patterns for converter and connector

e8e0a23

fixes build errors

cd47000

clairekuang changed the base branch from dev to arcgis_refactor December 7, 2024 19:17

clairekuang requested review from KatKatKateryna and oguzhankoral December 7, 2024 19:17

clairekuang marked this pull request as draft December 7, 2024 19:17

clairekuang added 4 commits December 9, 2024 09:11

Update ArcGISHostObjectBuilder.cs

6e0a397

fixes some polygon conversions

bc87ea1

refactor color manager to separate color unpacker

4fb661f

Delete GISLayerGeometryType.cs

fa967d4

clairekuang requested a review from didimitrie December 10, 2024 09:35

clairekuang marked this pull request as ready for review December 10, 2024 09:35

disables receive

84c4aa3

oguzhankoral requested changes Dec 11, 2024

View reviewed changes

clairekuang added 2 commits December 12, 2024 12:06

pr review issues

2b3202a

Merge branch 'arcgis_refactor' into claire/cnx-830-refactor-and-clean…

76448c4

…up-arcgis-to-enable-data-workflows

clairekuang requested a review from oguzhankoral December 12, 2024 12:11

clairekuang added 4 commits December 12, 2024 13:41

Update ArcGISRootObjectBuilder.cs

43202e0

Update ArcGISRootObjectBuilder.cs

fdbdd06

Update ArcGISRootObjectBuilder.cs

9ab4c25

Update ArcGISRootObjectBuilder.cs

792edea

oguzhankoral approved these changes Dec 12, 2024

View reviewed changes

oguzhankoral merged commit ac4d9f2 into arcgis_refactor Dec 12, 2024
5 checks passed

oguzhankoral deleted the claire/cnx-830-refactor-and-cleanup-arcgis-to-enable-data-workflows branch December 12, 2024 15:37

KatKatKateryna requested changes Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(arcgis): overhaul of arcgis connector and converter patterns to enable data extraction #439

refactor(arcgis): overhaul of arcgis connector and converter patterns to enable data extraction #439

clairekuang commented Dec 7, 2024 •

edited

Loading

linear bot commented Dec 7, 2024

codecov bot commented Dec 7, 2024 •

edited

Loading

oguzhankoral left a comment

oguzhankoral Dec 10, 2024

clairekuang Dec 12, 2024

oguzhankoral Dec 10, 2024

clairekuang Dec 12, 2024

oguzhankoral Dec 11, 2024

oguzhankoral Dec 11, 2024

oguzhankoral Dec 11, 2024

oguzhankoral Dec 11, 2024

oguzhankoral Dec 11, 2024

oguzhankoral Dec 11, 2024

oguzhankoral Dec 11, 2024

oguzhankoral Dec 11, 2024

clairekuang Dec 12, 2024 •

edited

Loading

KatKatKateryna left a comment

KatKatKateryna Dec 12, 2024

KatKatKateryna Dec 12, 2024

		@@ -0,0 +1,21 @@
		namespace Speckle.Converters.ArcGIS3.Extensions;

		public static class SpeckleApplicationIdExtensions

refactor(arcgis): overhaul of arcgis connector and converter patterns to enable data extraction #439

refactor(arcgis): overhaul of arcgis connector and converter patterns to enable data extraction #439

Conversation

clairekuang commented Dec 7, 2024 • edited Loading

Description & motivation

Changes:

To-do before merge:

Screenshots:

Validation of changes:

linear bot commented Dec 7, 2024

codecov bot commented Dec 7, 2024 • edited Loading

Codecov Report

oguzhankoral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairekuang Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

KatKatKateryna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairekuang commented Dec 7, 2024 •

edited

Loading

codecov bot commented Dec 7, 2024 •

edited

Loading

clairekuang Dec 12, 2024 •

edited

Loading