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

Add ability to convert 3d coordinates #176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PaulWagener
Copy link

@PaulWagener PaulWagener commented Aug 6, 2023

Fixes #150

This PR adds the ability to convert 3d coordinates.

The Coord trait is changed to add a Z coordinate so at least a minor version bump is warranted with this change.

@PaulWagener PaulWagener changed the title Add ability to convert 3d coordinates. Fixes #150 Add ability to convert 3d coordinates Aug 6, 2023
@urschrei
Copy link
Member

Hi @PaulWagener, thanks for the PR!

As ever with 2D -> 3D, there's likely to be some discussion…

At minimum, I don't think we can remove the xy functions in one fell swoop as that would break a great deal of downstream code. I think marking them as deprecated in favour of the new ones is better, here.

As for Coord, we're looking at the same problem. In a sense, this is our fault for using an overly general name, but changing the signature will break a lot of code. How do people feel about a new Coord3 (or maybe Coord4 to include M values) struct and deprecating Coord.

Even more radically: we could adopt Rust-Geodesy's coordinate struct…

@PaulWagener
Copy link
Author

As long as 3D coordinates will be supported I will be happy :)

@Oreilles
Copy link

What more is needed for this feature to be added ? I'd be happy to help.

@lx-88
Copy link

lx-88 commented Nov 1, 2024

I just wanted to bring up that this issue hurt me on a recent project. It would be great to see a solution to 3d transformations, or at least allowing transformation into ECEF, into the project and released.

@michaelkirk
Copy link
Member

@lx-88 - does using this branch work for you?

@michaelkirk
Copy link
Member

michaelkirk commented Nov 1, 2024

How do people feel about a new Coord3 (or maybe Coord4 to include M values) struct and deprecating Coord.

A new struct or a new trait?

I thought the whole "bring your own struct" thing was kind of nice, so users don't have to first convert their data to an intermediate proj-crate format.

Just brainstorming... if avoiding api-breakage is a concern, could we introduce the new 3d method with a default impl?

 trait Coord {
   ...
+  #[deprecated(note = "implement from_xyz instead, this method will be removed in a future release")]
   fn from_xy(x: f64, y: f64, z:f64) -> Self;

   // at some point we'd remove the default impl, which would be a breaking release - presumably in the same release as we remove `from_xy`
+  fn from_xyz(x: f64, y: f64, z:f64) -> Self {
+     Self::from_xy(x, y)
+  }
 }

@@ -744,22 +765,24 @@ impl Proj {
let coords = PJ_LPZT {
lam: c_x,
phi: c_y,
z: 0.0,
z: c_z,
t: f64::INFINITY,
};
unsafe {
proj_errno_reset(self.c_proj);
// PJ_DIRECTION_* determines a forward or inverse projection
let trans = proj_trans(self.c_proj, inv, PJ_COORD { lpzt: coords });
// output of coordinates uses the PJ_XY struct
Copy link
Member

Choose a reason for hiding this comment

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

Comment is out of date.

@@ -67,7 +67,8 @@ where
{
fn x(&self) -> T;
fn y(&self) -> T;
fn from_xy(x: T, y: T) -> Self;
fn z(&self) -> T;
fn from_xyz(x: T, y: T, z: T) -> Self;
Copy link
Member

Choose a reason for hiding this comment

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

Trait doc comments are now out of date.

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.

Need ability to convert 3D coordinates
5 participants