Allow Limbo to work on windows.#324
Conversation
costashatz
left a comment
There was a problem hiding this comment.
@nickanthony-dgl thanks a lot for this PR. Great that limbo works in Windows as well with so little changes :) See one minor improvement requested. Thanks again! 😄
src/limbo/model/gp.hpp
Outdated
| #define LIMBO_MODEL_GP_HPP | ||
|
|
||
| #ifdef _WIN32 | ||
| #include <corecrt_math_defines.h> // This brings in the M_PI define which is not in the C or C++ standard. |
There was a problem hiding this comment.
I think I prefer the following alternative:
#define _USE_MATH_DEFINES
#include <cmath>There was a problem hiding this comment.
That doesn't appear to be working for me, I get "M_PI": identifier not found. I think the issue is that if <cmath> has already been included somewhere earlier in the compilation then:
#define _USE_MATH_DEFINES
#include <cmath>will have no effect. You have to make sure that #define _USE_MATH_DEFINES happens before the first inclusion of <cmath> which can be difficult for a header-only library.
This same issue is discussed here and here
How would you feel about skipping the use of the M_PI preprocessor constant altogether and instead use something like:
namespace limbo {
constexpr double PI = 3.14159265359;
...There was a problem hiding this comment.
Most probably you need to put it here: https://github.com/resibots/limbo/blob/master/src/limbo/tools/math.hpp#L50. If this is not working, please enabling editing by admins for the PR and I will make the define from waf to be globally defined.
There was a problem hiding this comment.
How would you feel about skipping the use of the M_PI preprocessor constant altogether and instead use something like
My issue with that is that we would need to define a bigger set of constants, otherwise we are inconsistent. And this is a Windows issue. We will find a way to by-pass it.
There was a problem hiding this comment.
putting it in math.hpp didn't work but putting it in bo_base.hpp (the only place I see where gp.hpp is used) did. Please see the most recent commit.
I will go ahead and enable editing on this PR. Edit: It looks like "Allow edits by maintainers" is already enabled.
There was a problem hiding this comment.
Thanks a lot for the effort! I will try to have a look asap!
This PR makes 2 minor changes to allow Limbo to compile on a Windows machine. These changes are guarded by the
#ifdef _WIN32preprocessor so they will not affect non-Windows builds.M_PIpreprocessor define will be defined