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

wip: proj transform #22

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Apr 13, 2022

This is a demo of an API I'd like to be able to use with geozero to do more kinds of processing with the library.

Currently, it seems like processing is usually limited to "input -> output", which works great for converting formats and for some kinds of processing:

let wkt = WktStr("POINT (4760096.421921 3744293.729449)");
wkt
  .to_json()
  .unwrap()

But often I want to do some kind of pipeline of operations. One such example here:

  1. read geometry from some input format
  2. project the geometries
  3. write to some output format (maybe the same as input)
let from = "EPSG:2230";
let to = "EPSG:26946";
let proj = Proj::new_known_crs(from, to, None).unwrap();

let wkt = WktStr("POINT (4760096.421921 3744293.729449)");
wkt
  .to_projected(proj)
  .to_json()
  .unwrap());

I personally think the user facing API is pretty good - but implementing it has some problems that I'll highlight below.

fn xy(&mut self, x: f64, y: f64, idx: usize) -> Result<()> {
let (x, y) = self.proj.convert((x, y)).unwrap();
self.output.xy(x, y, idx)
}
Copy link
Member Author

@michaelkirk michaelkirk Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the crux - we transform the points before passing them in to the "next" processor in the pipeline.

We only "need" these two methods coordinate / xy, but we have to explicitly delegate our other impls to the "output" processor, otherwise we'd inherit the "no-op" implementations for the GeomProcessor trait. (which is really painful)

Effectively, I'm going for some kind of interceptor pattern here.

As well as being very verbose, it also seems like it'd be easy to break if we added a new method to GeomProcessor/FeatureProcessor/PropertyProcessor and failed to update these long list of delegations below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm currently short on time and leaving for Easter holidays soon. My only input would be that maybe a delegation pattern like https://github.com/flatgeobuf/flatgeobuf/blob/845111d2ba6765c8f43379877eb1f3440b78d4b6/src/rust/src/file_writer.rs#L302 could work here as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a delegation pattern

If I understand correctly, that's exactly what I'm doing - I did use the delegate crate macro to make it a little more concise, but it seems like a lot of boilerplate.

I'm searching for, but have so far failed to find, a better way to do this.

@pka
Copy link
Member

pka commented May 2, 2022

I'm currently experimenting with a new event based API, which should support chaining of processors. Other advantages of an event stream would be serializing and buffering.

@michaelkirk
Copy link
Member Author

Excited to see what you come up with!

@michaelkirk michaelkirk mentioned this pull request May 10, 2022
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.

None yet

2 participants