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

WIP: Adding support for exif Altitude parsing #325

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rudde
Copy link

@Rudde Rudde commented Sep 2, 2022

I've added support for exif altitude parsing. The example photos used for my unit tests are not really in-line with the other test images. But they are real word examples. I'm not sure if any what the guidelines for tests photos are.

@drewnoakes
Copy link
Owner

Thanks for this. A few points:

  • We don't add images with unit tests in this repo. Instead we use the metadata-extractor-images repo to run regression tests across a large number of images.
  • I suspect your builds are failing because you changed the public API without updating the API files. You can update the files using a Roslyn codefix on the diagnostic in your IDE.
  • Rather than changing the current constructor, please add a new overload so that people's existing code doesn't break.
  • GeoLocation needs to have its Equals and ToString methods updated to account for the new value.
  • Please document what an altitude value of null means.

An alternative here would be to leave GeoLocation as just lat/long, and have the method return altitude as a separate property somehow.

@Rudde
Copy link
Author

Rudde commented Sep 5, 2022

  • Please document what an altitude value of null means.

Before I do the changes here, about this. I've made an assumptions here, and that is that we prefer nullable double and null in cases where altitude doesn't exist, however the other obvious approach would be to use Double.NaN insted of null. This would let us keep altitude as an double insted of a nullable double. Do you have any opinion on this?

  • We don't add images with unit tests in this repo. Instead we use the metadata-extractor-images repo to run regression tests across a large number of images.

How will I test if altitude is parsed correctly out of an image I know have altitude, as the rest of the images don't have that.

  • GeoLocation needs to have its Equals and ToString methods updated to account for the new value.

Proposal for ToString -> lat, lng, altM where alt i omitted if it's not set, and where M represent meters and is prefixed with - if the value is below sea level. And doing the same for ToDmsString() I can't find any standard for how they're suppose to be represented with altitude.

@drewnoakes
Copy link
Owner

the other obvious approach would be to use Double.NaN insted of null

Personally I prefer null. It's clearer that the value might not be present (which is the case most of the time).

I'd considered changing the types of lat/lng to decimal too, as that would more accurately preserve the value from the file. There's no NaN equivalent for decimal.

How will I test if altitude is parsed correctly out of an image I know have altitude, as the rest of the images don't have that

We can add such an image to the test data library if none already exists. I can do that for you, so you don't have to clone that repo (it's pretty huge).

Proposal for ToString -> lat, lng, altM where alt i omitted if it's not set, and where M represent meters and is prefixed with - if the value is below sea level

Can you provide an example of what the output would look like? I'm not sure there's a clear way of doing this without adding the word "altitude".

doing the same for ToDmsString()

I'm less sure about that. I wouldn't change that.

These last two points make me wonder whether altitude belongs on GeoLocation. Thoughts?

@Rudde
Copy link
Author

Rudde commented Sep 7, 2022

I've updated:

  • New constructor using overloading
  • Updated Equals()
  • Updated GetHashCode() to incorporate Altitude
  • Updated ToString() will return 1.23, 4.56, 8M, or 1.23, 4.56 if altitude is null.
  • Updated the Unshipped.txt files as suggested by Rosylin in the IDE
  • Added explanation for what Altitude null means

Should IsZero also be updated to check for null on altitude?

Looking at the RFC 7946 GeoJSON they do include altitude or elevation, so I do believe it belongs in the GeoLocation object. They also do not use a prefix after your altitude, so maybe meters should be implicit and not prefixed in the ToString() function.

I think it is a debate to be had for other objects in the exif GPS data like accuracy, speed, or direction which I also hope to get into the project.

I also do not know how to write the unit tests if I don't have the pictures to reference? How will these tests be written when the images I wish to test is in an external project?

@Rudde Rudde changed the title Adding support for exif Altitude parsing WIP: Adding support for exif Altitude parsing Sep 7, 2022
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