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

Feature GeoTIFF #100

Merged
merged 9 commits into from
Nov 16, 2020
Merged

Feature GeoTIFF #100

merged 9 commits into from
Nov 16, 2020

Conversation

ruffson
Copy link
Contributor

@ruffson ruffson commented Oct 19, 2020

Hi there,

this branch attempts to implement some support for geotiffs (#98). This can become quite involved and if we implement everything about geotiff, we end up with a re-implementation of gdal.
So instead, I wrote something that gets all of the missing geo data out of geotiffs.

Summary

  • The missing tags are retrieved: ModelPixelScaleTag, ModelTransformationTag, ModelTiepointTag, GeoKeyDirectoryTag, GeoDoubleParamsTag, GeoAsciiParamsTag, GdalNodata.
  • The data from the geokey directory (GeoKeyDirectoryTag) is read out and saved in a hashmap.
  • All of the geotiff keys mentioned in the spec are defined in an enum which is the key for the hashmap

Open questions

  • It has to be decided how the values are to be accessed from outside, so how to make the geokey dir data available. To me there are three possibilities: 1. just make the hashmap public, 2. return a copy of the hashmap or 3. Offer a method like the get_tag methods which returns an option of a value given a geokey key.
  • I did not know how I can get the tags::Tag value so I can check for it in my for loop in geo_key.rs, e.g. instead of explicitely checking for a value of 34736 I would rather compare against Tag::GeoDoubleParamsTag as u16 but this does not work because of the tags! macro around the Tag enum.

Things to keep in mind

  • I noticed that all my geotiff's ModelTiepointTag only contained one tiepoint, and that gdalinfo shows all 5 tie point maps (top right, top bottom, center, etc.). This is apparently calculated, so this needs to take into account, the ModelPixelScaleTag, the width and height of the raster image and the projection type. And then they must be calculating the other tie points by calculating raster points and then project those. I don't think it makes sense to implement that in this package (at this point anyways), to me this belongs more into a geo-tools package or something, which parses values from geotiff. I will have to implement this so I can report back at some point when I have something to show.

Please let me know what you guys think and what is missing.

@ruffson ruffson changed the title Feature geotiff Feature GeoTIFF Oct 19, 2020
Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

I think it's too much in one PR. I'd rather merge the smaller features such as newly recognized tags and value types quickly and leave the mechanisms of parsing the types as a separate draft as everything involved needs quite a lot of polish and probably require a lot of extra work if it were to be maintenaned in the current state.

src/decoder/ifd.rs Show resolved Hide resolved
src/decoder/ifd.rs Outdated Show resolved Hide resolved
src/decoder/mod.rs Outdated Show resolved Hide resolved
src/decoder/mod.rs Outdated Show resolved Hide resolved
src/geo/geo_key.rs Outdated Show resolved Hide resolved
src/geo/geo_key.rs Outdated Show resolved Hide resolved
src/geo/geo_key.rs Outdated Show resolved Hide resolved
src/geo/geo_key.rs Outdated Show resolved Hide resolved
src/geo/geo_key.rs Outdated Show resolved Hide resolved
src/geo/geo_key.rs Outdated Show resolved Hide resolved
@ruffson
Copy link
Contributor Author

ruffson commented Oct 31, 2020

I think it's too much in one PR. I'd rather merge the smaller features such as newly recognized tags and value types quickly and leave the mechanisms of parsing the types as a separate draft as everything involved needs quite a lot of polish and probably require a lot of extra work if it were to be maintenaned in the current state.

Okay I will then remove everything except for the types and tags and create another repo for my own use with the contents of the geo folder and do more parsing of the values, do projections, etc. there. I will look through your edits soon and make changes accordingly. Thank you for your review!

@HeroicKatora
Copy link
Member

Sounds good. I do appreciate the work and would like to see it merged—or independently published—at some point. Don't hesitate to ask or propose additional interface in case something needs additional changes in tiff.

@ruffson
Copy link
Contributor Author

ruffson commented Nov 2, 2020

I removed the geo stuff and put it in my own repository.

@ruffson
Copy link
Contributor Author

ruffson commented Nov 16, 2020

Rebased.

@HeroicKatora HeroicKatora merged commit c9703dc into image-rs:master Nov 16, 2020
@ruffson ruffson deleted the feature_geotiff branch November 16, 2020 18:20
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.

2 participants