-
Notifications
You must be signed in to change notification settings - Fork 23
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
base: main
Are you sure you want to change the base?
Conversation
23b7ac0
to
16ddd6b
Compare
084bcdf
to
0e43e48
Compare
There was a problem hiding this 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!
I'm not familiar with how GCPs work, so if @weiji14 has time to review I'll defer to him |
There was a problem hiding this 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.
pub enum RasterType { | ||
RasterPixelIsArea = 1, | ||
RasterPixelIsPoint = 2, | ||
} |
There was a problem hiding this comment.
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:
- Should we add enum variants for
RasterType=0
(undefined/unknown) andRasterType=32767
(user-defined), even though they're not recommended at https://docs.ogc.org/is/19-008r4/19-008r4.html#_requirements_class_gtrastertypegeokey? - Could you add a link to the GeoTIFF spec's
GTRasterTypeGeoKey
requirements for this please
#[derive(Debug)] | ||
pub enum CoordinateTransform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document this by adding a link to https://docs.ogc.org/is/19-008r4/19-008r4.html#_raster_to_model_coordinate_transformation_requirements?
There was a problem hiding this 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.
pub fn get_value_at<T: FromPrimitive + 'static>( | ||
&self, | ||
coord: &Coord, | ||
sample: usize, | ||
) -> Option<T> { |
There was a problem hiding this comment.
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.
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; |
There was a problem hiding this comment.
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:
- Transforming model coordinates to raster coordinates via
transform_to_raster
- 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?
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. |
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; | ||
} |
There was a problem hiding this comment.
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);
This PR implements all coordinate transformation methods used in GeoTIFF:
In addition, the implementation considers the raster type (
RasterPixelIsArea
orRasterPixelIsPoint
) 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:
@weiji14 please have a look.