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
More readable code + factors #1
Conversation
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.
Please, add some description in the commit message as well
src/location-distance-utils.c
Outdated
|
||
static double r2d(double rad) | ||
// compensate for shrinking longitude towards poles | ||
float lng_scale(double lat) |
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.
static? also, why float and not double?
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.
Ah! it's my bad, it should be double,dist
type was float in my head for some reason.
I'm not sure what you mean by, the original r2d
was static.
src/location-distance-utils.c
Outdated
double longitude_s, | ||
double latitude_f, | ||
double longitude_f) | ||
double longitude_s, |
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.
extra change, please squash with the next commit where it is fixed.
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.
That's my formatter doing :p
src/location-distance-utils.h
Outdated
@@ -24,6 +24,8 @@ | |||
|
|||
G_BEGIN_DECLS | |||
|
|||
float lng_scale(double lat); |
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.
why is that exported? Is there any user of it outside location-distance-utils.c
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.
Ultimately yes, for terrain and bearing calculations, it shouldn't be long to add them.
src/location-distance-utils.c
Outdated
dist = dist * 1.609344; /* kilometers */ | ||
|
||
return dist; | ||
float dlat = (float)(latitude_f - latitude_s); |
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 haven't verified the maths here (it was too long ago I was playing with trigonometric functions when I was student), so could you provide some evidence that either original code does it wrongly or the replacement calculates the same values given same input values.
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.
The original code uses Spherical law of cosines
which gives good results down to distances as small as few meters,
what is used here is Equirectangular approximation
which is accurate for small distances only but performs well.
of course there's Haversine
which is accurate for all king of distances but considered relatively slow.
I could add Haversine
calculations, what do you think?
here's a link talks about all three laws
I think this is fine. @Dastan-glitch If you could keep the existing formatting (it's done with And yeah, a better commit message would be nice. |
No description provided.