Pull GeographicLib into develop.
Issue #32
resolved
I'm not an expert on the cmake structure, so someone needs to dot the i's on this new third-party library and make sure it works with all cmake flags. After fixing that in 2.4, it needs to be pulled into 3.0.
Comments (23)
-
reporter
-
Ok sure, should I do this before IROS or can it wait until after?
-
reporter
It can wait
-
reporter
- edited description
-
@fdellaert For this, does it suffice to just merge 2.4 into develop? That brings some new factors as well.
-
reporter
No, I think you would need to dot the i's on this new third-party library and make sure it works with all cmake flags, then merge into develop.
-
Ok, I took a look. I think the biggest risk is that GeographicLib doesn't compile on somebody's system (at least not with our default compile settings for it) and there needs to be a way to disable it... or alternatively should it be disabled by default? I can put in the flags to disable it if you think that's the best option.
-
reporter
Why would it not compile?
-
For example on some embedded platform, or due to compiler changes, e.g. on a new Mac os version. Always building it when it will rarely be used seems a little risky, since we'll then be responsible for any compile problems that people have with it.
On the other hand, I can see how it's useful to include it in GTSAM to convert coordinates. And introducing an option to enable it adds complexity.
I don't know what's best for this... up to you and I can do it either way.
-
reporter
No, why do you think this is less likely to compile than everything else? Is there something specific that makes it different?
-
reporter
If so - and you can identify this - we could add an option to not build it, but I'd like to build it by default.
-
Not that I know of - it's just a lot of code that we'd be taking on responsibility for. I'll keep it as is for now then, it's anyway not hard to add an option in the future if we decide we need one.
Everything looked fine to me in the way you set up the library, given the current cmake structure in GTSAM. The linking is a little strange due to the way unit tests are added in GTSAM. I'm working on a feature branch today (adding-scripts-simplify) that fixes a lot of the mess in adding unit tests and examples, so that will help to clean up the way the unit tests link to GeographicLib.
For now it looks good to merge into develop. I'll simplify it a bit using the changes in the adding-scripts-simplify branch later today. But for now should I merge 2.4 into develop?
-
reporter
Hold on one second. I will remove RENORM in Sphere2, as suggested by @atrevor , and then we can also release 2.4 as source.
-
reporter
@richardroberts @atrevor I removed RENORM, but see new issue I will create
-
@fdellaert FYI, I also had to disable the unit tests and "tools" in GeographicLib for make check to work correctly. Otherwise it tried to run the GeographicLib unit tests which all fail with a strange error.
We may also want to consider preventing GeographicLib installing itself... it could overwrite or take precedence over a GeographicLib version that a user already has installed?
-
This actually came up in a meeting Friday, and it was suggested that it could be handled like Eigen? Give the user a choice?
-
reporter
Yes. Default install, but like Eigen allow outside
-
Is it true that the only GeographicLib functions a user would need to interact with GTSAM are those to convert coordinates? If so, what about creating a few "wrapper" functions to do the conversions, and build them into GTSAM, so that we make those functions available without installing GeographicLib?
-
And then if someone wants to do other things that GeographicLib does, which are not normally necessary for interacting with GTSAM, they can install GeographicLib.
-
reporter
wrapper, or actual functions?? These conversions are not easy
-
Ok, I added the flag we talked about in commit 42d5399. I tested it and it all seems to work properly.
-
reporter
Yay !
-
- changed status to resolved
- Log in to comment
Note, would also be good to have it not build all its executables.