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

Coordinate Transformations #21

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

Conversation

gschulze
Copy link
Collaborator

@gschulze gschulze commented Oct 7, 2024

This PR implements all coordinate transformation methods used in GeoTIFF:

  • Tie Point and Pixel Scale
  • Affine Transform
  • Multiple Tie Points (Ground Control Points)

In addition, the implementation considers the raster type (RasterPixelIsArea or RasterPixelIsPoint) to determine the origin of the raster space coordinate system.

The Tie Point and Pixel Scale and Affine Transform methods are relatively straightforward. The Multiple Tie Points transformation roughly works as follows:

  • Create a Delaunay triangulation of the tie points in raster space
  • Apply the resulting triangulation to the tie points in model space
  • When doing coordinate transformations, use the containing triangle of a coordinate to determine the three points to be used for the transformation
  • Determine the relative position of the coordinate within the triangle in the model space coordinate system
  • Use the relative position to linearly interpolate the coordinate within the corresponding triangle in the raster space coordinate system

@weiji14 please have a look.

Cargo.toml Outdated Show resolved Hide resolved
@gschulze gschulze force-pushed the feature/coordinate-transformation branch from 23b7ac0 to 16ddd6b Compare October 8, 2024 03:21
@gschulze gschulze force-pushed the feature/coordinate-transformation branch from 084bcdf to 0e43e48 Compare October 8, 2024 06:49
@gschulze gschulze self-assigned this Oct 8, 2024
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Just a couple of quick comments on the organization of the code. I'll try to find some time to do a deeper dive over the weekend!

Cargo.toml Outdated Show resolved Hide resolved
tests/integration.rs Outdated Show resolved Hide resolved
@kylebarron
Copy link
Member

I'm not familiar with how GCPs work, so if @weiji14 has time to review I'll defer to him

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Did an initial look through the changes and looks good so far! Unit tests are solid (you're lucky I did a exchange in Salzvurg, Austria many years ago, so familiar with the geography 😃), and I do appreciate the changes to get_pixel_value to return Some/None.

Leaving a few small comments or now, I'd be keen on a bit more documentation around the public enums/structs if possible (a link to the relevant subsection in the GeoTIFF spec will do). Will squeeze out some time next week to do a deeper review on the affine and Delaunay triangulation math.

Comment on lines +668 to +671
pub enum RasterType {
RasterPixelIsArea = 1,
RasterPixelIsPoint = 2,
}
Copy link
Member

Choose a reason for hiding this comment

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

A couple of thoughts here:

  1. Should we add enum variants for RasterType=0 (undefined/unknown) and RasterType=32767 (user-defined), even though they're not recommended at https://docs.ogc.org/is/19-008r4/19-008r4.html#_requirements_class_gtrastertypegeokey?
  2. Could you add a link to the GeoTIFF spec's GTRasterTypeGeoKey requirements for this please

Comment on lines +21 to +22
#[derive(Debug)]
pub enum CoordinateTransform {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Ok, I've checked the affine transform code and it looks sound. The Delaunay triangulation logic is a bit over my head, but I trust that it is ok (if not, someone will hopefully notice and file a bug report).

Don't want to hold this back any longer because I'm getting into a busy period, so I'll pre-approve this. I have some questions below around the get_value_at method, but hopefully those are non-issues.

Comment on lines +107 to +111
pub fn get_value_at<T: FromPrimitive + 'static>(
&self,
coord: &Coord,
sample: usize,
) -> Option<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this method needs some documentation. Keep getting confused on whether coord is meant to be x/y pixel (model) coordinates or raster (geospatial) coordinates.

Comment on lines +144 to +151
let mut coord = match coordinate_transform {
None => *coord,
Some(transform) => transform.transform_to_raster(coord),
};

let raster_offset = self.raster_offset();
coord.x -= raster_offset;
coord.y -= raster_offset;
Copy link
Member

@weiji14 weiji14 Oct 23, 2024

Choose a reason for hiding this comment

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

Just want to double-check the logic here because. If I'm not mistaken, the code is:

  1. Transforming model coordinates to raster coordinates via transform_to_raster
  2. Applying an offset of -0.5 (in the case of PixelisPoint) to the raster coordinates?

Is it correct to apply an offset of -0.5 (which should be a model offset) to raster coordinates?

@gschulze
Copy link
Collaborator Author

Thanks @weiji14 for your comments. I've been busy the last two weeks, but hopefully, I'll find time to address the remaining issues next week.

Comment on lines +261 to +272
if c.x < min_x {
min_x = c.x;
}
if c.x > min_x {
min_x = c.x;
}
if c.y < min_x {
min_x = c.y;
}
if c.y > min_x {
min_x = c.y;
}

Choose a reason for hiding this comment

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

Perhaps you meant this instead?

                if c.x < min_x {
                    min_x = c.x;
                }
                if c.x > max_x {
                    max_x = c.x;
                }
                if c.y < min_y {
                    min_y = c.y;
                }
                if c.y > max_y {
                    max_y = c.y;
                }

or even this?

                min_x = min_x.min(c.x);
                max_x = max_x.max(c.x);
                min_y = min_y.min(c.y);
                max_y = max_y.max(c.y);

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.

4 participants