Gis objects cleaning #181
Gis objects cleaning #181
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #181 +/- ##
======================================
Coverage ? 66.53%
======================================
Files ? 245
Lines ? 9975
Branches ? 1065
======================================
Hits ? 6637
Misses ? 2993
Partials ? 345 ☔ View full report in Codecov by Sentry. |
src/Speckle.Objects/GIS/CRS.cs
Outdated
| @@ -5,9 +5,9 @@ namespace Speckle.Objects.GIS; | |||
| [SpeckleType("Objects.GIS.CRS")] | |||
| public class CRS : Base | |||
| { | |||
There was a problem hiding this comment.
wondering if we need a specific class for this, or if it would make more sense to attach these properties to the layer collection if that's the only place CRS is used.
There was a problem hiding this comment.
the plan is to move this to root eventually, and it needs required properties that we cannot enforce on Layer (or anywhere else)
There was a problem hiding this comment.
what does wkt mean? is it necessary for data extraction workflows? (not interop?)
the other props are all nullable, so this is not following our best practices class design
(if a class has mostly nullable props, should it be a class? )
There was a problem hiding this comment.
WKT (Well Known Text) gives all the information about the coordinate reference system (which is required). Name is also required, and Authority ID is useful, but not always available (e.g. for custom coordinate systems, there are no authority ID). UnitsNative are helpful if some of the properties on the features are stored in the original layer units (before reprojecting and sending to Speckle).
Let me think about offsets and rotation. These are the arguments supported in QGIS with custom functionality we added, but not yet implemented in ArcGIS
src/Speckle.Objects/GIS/GisLayer.cs
Outdated
| @@ -0,0 +1,12 @@ | |||
| using Speckle.Sdk.Models; | |||
There was a problem hiding this comment.
not sure if we need this extra class - you could use the layer:collection class and just add the missing props from crs dynamically
There was a problem hiding this comment.
it would be hard to enforce required properties, without which GisLayers don't make sense
There was a problem hiding this comment.
Is CRS required for data extraction? it seems like it's only used on send, not receive
There was a problem hiding this comment.
yes, for GIS data, the data must have the georeference
|
|
||
| [SpeckleType("Objects.Geometry.Polygon")] | ||
| public class Polygon : Base | ||
| { |
There was a problem hiding this comment.
let's add a required keyword to the boundary, and reconsider the proeprty names based off of other apps that may use polygons
|
|
||
| namespace Speckle.Objects.Data; | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Comment should be updated - I will add this class in another pr, so no need to fix
Full description here: specklesystems/speckle-sharp-connectors#416