Skip to content

Add get origin func to COG#1840

Merged
sharkAndshark merged 16 commits intomaplibre:mainfrom
sharkAndshark:cog_web_2
May 21, 2025
Merged

Add get origin func to COG#1840
sharkAndshark merged 16 commits intomaplibre:mainfrom
sharkAndshark:cog_web_2

Conversation

@sharkAndshark
Copy link
Collaborator

It's the 2nd PR of #1814 to fix #1820

A func to get the origin coord from COG is added. It's highly inspired by geotiff.js and the content of B.6 of geotiff spec

@sharkAndshark sharkAndshark marked this pull request as ready for review May 20, 2025 07:59
Copilot AI review requested due to automatic review settings May 20, 2025 07:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a helper to extract the spatial origin from a COG’s GeoTIFF tags and integrates it into the metadata.

  • Introduce origin field in Meta and call a new get_origin function in get_meta
  • Implement get_origin to read either the first ModelTiepointTag or the Transformation matrix
  • Add corresponding error variant, unit tests, and update dependencies

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
martin/src/cog/source.rs Added origin field, implemented get_origin, integrated into get_meta, and added tests
martin/src/cog/errors.rs Introduced GetOriginFailed error variant
martin/Cargo.toml Added approx dev-dependency and reordered testcontainers-modules
Comments suppressed due to low confidence (2)

martin/src/cog/errors.rs:56

  • [nitpick] Consider enriching this error message with context, such as indicating whether both tie points and transformation tags were missing or invalid.
#[error("Get origin failed for {0}")]

martin/src/cog/source.rs:593

  • Add a unit test for the failure case (e.g., both tie_points and transformation are None) to cover the error branch in get_origin.
}

@sharkAndshark sharkAndshark requested a review from Copilot May 20, 2025 08:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new function to extract the origin coordinate from a Cloud Optimized GeoTIFF (COG) using either tie points or a transformation matrix as defined in the GeoTIFF specification. Key changes include:

  • Adding an origin field to the Meta struct in source.rs.
  • Implementing and integrating the get_origin function to calculate the origin based on provided tie points or transformation matrix.
  • Adding corresponding tests to verify get_origin behavior.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
martin/src/cog/source.rs Adds the origin field to Meta, implements get_origin, and integrates it within get_meta along with tests.
martin/src/cog/errors.rs Adds a GetOriginFailed error variant for failure cases in get_origin.
martin/Cargo.toml Adds approx as a dev-dependency for testing purposes.
Comments suppressed due to low confidence (1)

martin/src/cog/source.rs:576

  • [nitpick] The test parameters are named 'matrix' and 'tie_point', but they are passed to get_origin in reversed order relative to the function signature. Consider renaming the test parameters to match the function parameter order for clarity.
fn can_get_origin(

@sharkAndshark sharkAndshark requested a review from Copilot May 20, 2025 08:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for extracting the geographic origin from a Cloud-Optimized GeoTIFF by introducing a new origin field on Meta and a helper to compute it from GeoTIFF tags.

  • Extend Meta with a 3-element origin coordinate and populate it in get_meta.
  • Implement get_origin to derive the origin from either tie points or a transformation matrix, plus associated error variant and tests.
  • Add approx as a dev-dependency for floating-point comparisons in tests.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
martin/src/cog/source.rs Added origin field, implemented get_origin, and tests
martin/src/cog/errors.rs Introduced GetOriginFailed error variant
martin/Cargo.toml Added approx under [dev-dependencies] for test assertions
Comments suppressed due to low confidence (1)

martin/src/cog/source.rs:426

  • Add a test case for when both tie_points and transformation are None to exercise the GetOriginFailed error path.
fn get_origin(

Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

If you add some documentation in the form if doc comments what an origin is, I am fine with this.
I honestly don't have a good understanding what an origin is.

min_zoom: u8,
max_zoom: u8,
model: ModelInfo,
origin: [f64; 3],
Copy link
Member

Choose a reason for hiding this comment

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

Could you add doc comments explaining what the origin is?
This would be very handy as origin as 3*f64 is not quite self-explanatory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. I will try. Actually it's the coords: raster_space_to_model_space(0,0,0), I'm not good at this, maybe you have better one?
I add this to:

    // The geo coords of pixel(0, 0, 0) ordering in [x, y, z]
    origin: [f64; 3],

@sharkAndshark sharkAndshark merged commit f0ee1a7 into maplibre:main May 21, 2025
19 of 20 checks passed
@sharkAndshark sharkAndshark deleted the cog_web_2 branch May 21, 2025 03:43
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.

Make COG web friendly

2 participants

Comments