Skip to content
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

(WIP) Changes to get gtsam to compile in Windows #85

Merged
merged 16 commits into from
Jul 19, 2019
Merged

Conversation

jlblancoc
Copy link
Member

@jlblancoc jlblancoc commented Jul 11, 2019

Follow up of #84 . Fixes #82 .

To-do:

  • remove whitespace changes,
  • remove the definition of NOMINMAX in the gtsam .h file, and add it via cmake definitions, which is a much cleaner way.

This change is Reviewable

@jlblancoc
Copy link
Member Author

@cntaylor , @izzys, @varunagrawal Could you please give it a try to this branch, to verify that it still builds correctly for MATLAB?
Thanks!

@cntaylor
Copy link
Contributor

I have had significant problems getting MATLAB toolbox to build (see issue #86 just submitted) but I figured just getting gtsam library by itself to compile was progress, so I submitted that pull request. Hopefully that's okay!

@varunagrawal
Copy link
Collaborator

@jlblancoc still doesn't work well. Now the project name is being set to gtsam_unstable and the same condition is failing. I assume we need to set the project in gtsam_unstable/CMakeLists.txt to get it to compile on Windows?

@jlblancoc
Copy link
Member Author

Now the project name is being set to gtsam_unstable ...

That is fixed with the latest commits in this branch, but the link errors in #86 are still there.
Will take a look at it when possible; if someone else wants to give it a try, please start working on top of this branch.

* The check.base unit tests all pass now.
* The gstam_matlab_wrapper class compiles with now errors now.
* Note that I had to remove all LieMatrix, LieVector, and LieScalar stuff
to get this to work...
@cntaylor
Copy link
Contributor

Just submitted pull request #87 that fixes the compilation problems. There are still a lot of unit tests that fail in the geometry section and other places, but the gtsam_matlab_wrapper library compiles with no errors now. I believe this pull will close off issue #86, but as I mentioned in some comments there, I'm a bit uncomfortable with how this issue resolved itself.

@dellaert
Copy link
Member

PR #87 is not a correct fix, but it does add a GTSAM_EXPORT that was missing. Not sure whether the lack of cpp files (as @cntaylor hypothesized) is the issue. Maybe the issue is that LieXXX live in a subdirectory as opposed to other header-only classes (if there are any).

@izzys
Copy link

izzys commented Jul 14, 2019

I have compiled this branch on Linux 16.04 with MATALB flag ON and it works.

However, the following examples from the MATLAB toolbox have errors ( I have tested with MATLAB R2019a):

1: gtsam_examples/CameraFlyingExample.m
Unrecognized method, property, or field 'dist' for class 'gtsam.Point2'.

Error in CameraFlyingExample (line 123)
if i > 1 && baseCentroid{i}.dist(baseCentroid{j}) < options.cylinder.radius * 2

2: gtsamExamples (GUI not working)
Dot indexing is not supported for variables of this type.
Error in gtsamExamples>Odometry_Callback (line 88)
axes(handles.axes3);

3: gtsam_examples/Pose2SLAMwSPCG.m

Error using gtsam_wrapper
Exception from gtsam:
Unknown linear solver type in SuccessiveLinearizationOptimizer

Error in gtsam.LevenbergMarquardtParams/setLinearSolverType (line 238)
gtsam_wrapper(1470, this, varargin{:});

Error in Pose2SLAMwSPCG (line 57)
params.setLinearSolverType('CONJUGATE_GRADIENT');

4: unstable_examples/TransformProjectionFactorExampleISAM.m
Unrecognized method, property, or field 'compose' for class 'gtsam.Point3'.

Error in TransformProjectionFactorExampleISAM (line 50)
initial.insert(100+i,landmarks{i}.compose(y_shift));

5: unstable_examples/TransformProjectionFactorExample.m
Unrecognized method, property, or field 'compose' for class 'gtsam.Point3'.

Error in TransformProjectionFactorExample (line 36)
initial.insert(100+i,landmarks{i}.compose(y_shift));

6: unstable_examples/TransformCalProjectionFactorIMUExampleISAM.m
Unrecognized method, property, or field 'compose' for class 'gtsam.Point3'.

Error in TransformCalProjectionFactorIMUExampleISAM (line 56)
initial.insert(100+i,landmarks{i}.compose(y_shift));

7: unstable_examples/TransformCalProjectionFactorExampleISAM.m
Unrecognized method, property, or field 'compose' for class 'gtsam.Point3'.

Error in TransformCalProjectionFactorExampleISAM (line 48)
initial.insert(100+i,landmarks{i}.compose(y_shift));

8: unstable_examples/IMUKittiExampleVO.m
data file is missing

9: unstable_examples/IMUKittiExampleAdvanced.m
data file is missing

10: unstable_examples/FlightCameraTransformIMU.m
Unrecognized method, property, or field 'at' for class 'gtsam.Values'.

Error in FlightCameraTransformIMU (line 248)
currentVelocityGlobal = result.at(currentVelKey);

@cntaylor
Copy link
Contributor

@izzys Most of those commands are behind a DEFINE for GTSAM_ALLOW_DEPRECATED_SINCE_V4. Can you make sure that is on (I know I turned it off sometimes) and see if your MATLAB examples work?

@jlblancoc
Copy link
Member Author

@dellaert @chrisbeall : Just requested AppVeyor access to this ORG, so I can integrate their Windows MSVC build CI service, probably in this PR, ok? Let me know when you approve the app access.

@jlblancoc
Copy link
Member Author

Merging to fix matlab, will add AppVeyor in another pr.

@jlblancoc jlblancoc merged commit ec04369 into develop Jul 19, 2019
@dellaert
Copy link
Member

AppVeyor was approved...

@dellaert
Copy link
Member

@izzys Thanks for checking the MATLAB examples. I think we should be able to run the examples with GTSAM_ALLOW_DEPRECATED_SINCE_V4, so would you mind opening an issue? If you feel up to it, the fixes should be fairly straightforward for a PR :-)

@dellaert dellaert deleted the msvc-fixes branch August 1, 2019 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmake error when building with MATLAB flag ON
5 participants