Open Rudde opened 2 years ago
Thanks for this. A few points:
GeoLocation
needs to have its Equals
and ToString
methods updated to account for the new value.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.
- 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.
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?
I've updated:
Equals()
GetHashCode()
to incorporate AltitudeToString()
will return 1.23, 4.56, 8M
, or 1.23, 4.56
if altitude is null. Unshipped.txt
files as suggested by Rosylin in the IDEnull
meansShould 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?
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.