Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geometry Overhaul #48

Merged
merged 6 commits into from
May 2, 2024
Merged

Geometry Overhaul #48

merged 6 commits into from
May 2, 2024

Conversation

JosiahParry
Copy link
Collaborator

@JosiahParry JosiahParry commented May 1, 2024

This PR is a ground up rewrite of the geometry serialization from sf objects to EsriJSON. The current implementation using Rcpp and jsonify is too prone to errors and has been causing a number of headaches. The intent behind this is to use Rust and serde definitions to create geometries in a way that we know will always adhere to the format required. And if not, we will get a (messy) error.

This PR is massive with a lot of duplicative code but its the best I can do for now. It could potentially be simplified using a number of macros.

Additionally the functions as_featureset(), as_esri_featureset() and as_features() and as_esri_features() have a lot of duplicative checking that can probably be simplified into a couple of helpers.

This closes R-ArcGIS/arcgislayers#184

TODO:

  • document as_esri_geometry()
  • document as_features() and as_esri_features()
  • document as_featureset() and as_esri_featureset()
  • write examples for all geometry functions. These will act as the tests as they will be executed in R CMD check
  • Update NEWS.md
    • note that as_geometry() is deprecated
    • check to see if arguments have changed
  • Update README with note about installing from source. suggest using r-universe for development versions

CC @mmachir @elipousson

@JosiahParry JosiahParry marked this pull request as ready for review May 2, 2024 17:11
@JosiahParry
Copy link
Collaborator Author

There is an issue with windows CI on 4.1 where core rust library is not being installed.

I have tested publishing with this but still needs more testing for

  • Point
  • MultiPoint
  • LineString
  • MultiLineString
  • Polygon
  • MultiPolygon

to be very sure that it works. But I am comfortable with merging this in.

Once {arcgisgeocode} is done, we will adjust arcgisutils to use vendored dependencies and then publish the new packages.

@JosiahParry JosiahParry merged commit af00dd2 into main May 2, 2024
7 of 8 checks passed
@JosiahParry JosiahParry changed the title [Draft] Geometry Overhaul Geometry Overhaul May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

publish_layer() fails with mapply error
1 participant