borglab / GTDynamics

Full kinodynamics constraints for arbitrary robot configurations with factor graphs.
BSD 2-Clause "Simplified" License
39 stars 10 forks source link

Remove extraneous DEBUG flags #317

Closed varunagrawal closed 2 years ago

varunagrawal commented 2 years ago

GTDynamics doesn't compile on M1 Macs when GTSAM's march=native flag is set and it inherits this value. Even if GTSAM explicitly doesn't have march=native set, the way the CMake is setup means that this flag will be set when the GTSAM cmake files are included within GTDynamics.

To allow one to set the flag off, this PR introduces a new flag for GTDynamics.

PR to remove some extra DEBUG flags which aren't necessary.

dellaert commented 2 years ago

So, I never had a problem with this: I turn the native off for GTSAM, and then it works for GTDynamics as well. Am I mistaken?If I’m not mistaken, I fail to see what the use case is of this PR. I may not fully understand….

varunagrawal commented 2 years ago

That's interesting. I explicitly looked at the CMake output for both, and GTSAM doesn't have that flag set but GTD does.

I encounter this when building the scripts directory in particular, so maybe try that?

varunagrawal commented 2 years ago

Plus, this flag gives explicit control irrespective of GTSAM. That's a nice thing to have in general.

dellaert commented 2 years ago

I disagree: we should have one canonical way of dealing with this flag on the M1 platform. What should really happen is detect the platform on the GTSAM side and set the default accordingly. GTSAM does not compile without setting this flag to false on M1, so I don’t understand what the value is in setting it independently. Just user confusion.

ProfFan commented 2 years ago

I agree w/Frank on this, because GTD builds fine on my M1 without issue after GTSAM builds.

varunagrawal commented 2 years ago

Okay, let me double check. If I am the only one having this issue, then I am clearly doing something wrong. :slightly_smiling_face:

dellaert commented 2 years ago

Please close if you resolve the issue on your machine via GTSAM

varunagrawal commented 2 years ago

So I double checked and this is a legitimate issue. Since the CMake syntax for GTSAM_BUILD_WITH_MARCH_NATIVE uses it as an option rather than a set variable, when we include(GtsamBuildTypes) in the main CMakeLists.txt of GTD, the value gets set to the default which is ON.

@ProfFan can you please try building again with the GTDYNAMICS_BUILD_SCRIPTS flag set to ON? This issue only arises in that case.

varunagrawal commented 2 years ago

Another way to double check is to add message(">>>> ${GTSAM_BUILD_WITH_MARCH_NATIVE}") right below include(GtsamBuildTypes).

When you run cmake .. it should unequivocally tell us what the value is.

varunagrawal commented 2 years ago

Huh this issue also comes up when running make check. I definitely didn't have this before, so I guess something changed on my mac?

Here's my CMake output. Can someone add theirs so we can compare notes?

-- GTSAM include directory:  /usr/local/lib/cmake/GTSAM/../../../include
-- Looking for ignition-math6 -- found version 6.8.0
-- Searching for dependencies of ignition-math6
-- Building universal_robot
-- Building utils
-- Building factors
-- Building optimizer
-- Building kinematics
-- Building statics
-- Building dynamics
-- Building cablerobot/factors
-- Building jumpingrobot/factors
-- Build Python Wrapper: ON
-- Checking Python Version
-- Setting Python version for wrapper
-- pybind11 v2.6.0 dev1
-- Wrapper Python Version: 3.9.7
-- ===============================================================
-- ================  Configuration Options  ======================
-- Project                                     : gtdynamics
-- CMAKE_CXX_COMPILER_ID type                  : AppleClang
-- CMAKE_CXX_COMPILER_VERSION                  : 13.0.0.13000029
-- GTSAM Version                               : 4.1.0
-- Boost Version                               : 1.76.0
-- SDFormat Version                            : 10.5.0
-- Build Python                                : ON
-- Python Version                              : 3.9.7
-- Build march=native                          : ON
-- Build Scripts                               : ON
-- Build Examples                              : ON
-- ===============================================================
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/varunagrawal/borglab/GTDynamics/build
varunagrawal commented 2 years ago

Okay, after closer inspection, I can directly set the GTSAM_BUILD_WITH_MARCH_NATIVE flag value via ccmake for GTDynamics. I am updating the scope of this PR to just remove the superfluous DEBUG flags.