--[BE][WIP] Remove Eigen dependency #2301

jturner65 · 2024-01-15T20:32:33Z

Motivation and Context

This PR removes Eigen library/dependency from Habitat-sim. It is intended to be an invisible change, and may require iterations to get everything.

Still a WIP.

How Has This Been Tested

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

mosra

Hasty review while waiting for the CI.

You might want to clean up remains of Eigen in dependencies.cmake afterwards as well, including not enabling the EigenIntegration library anymore.

src/esp/core/Utility.h

src/esp/geo/CoordinateFrame.h

mosra · 2024-01-16T20:44:08Z

src/esp/geo/Geo.cpp

@@ -18,20 +20,21 @@ using Magnum::Math::Literals::operator""_rgb;
 namespace esp {
 namespace geo {

-std::vector<vec2f> convexHull2D(const std::vector<vec2f>& points) {
+std::vector<Mn::Vector2> convexHull2D(const std::vector<Mn::Vector2>& points) {


Could take a const array view while you're at it.

src/esp/geo/OBB.cpp

src/esp/geo/OBB.h

src/utils/datatool/Mp3dInstanceMeshData.cpp

src/utils/datatool/Mp3dInstanceMeshData.h

src/utils/datatool/Mp3dInstanceMeshData.cpp

src/utils/datatool/Mp3dInstanceMeshData.h

mosra · 2024-01-16T20:57:58Z

src/utils/datatool/SceneLoader.cpp

+        const Mn::Vector3 xyz_scene =
+            Mn::Vector3::from(&assimpMesh.mVertices[v].x);
+        const Mn::Vector3 xyz_esp =
+            alignSceneToEspGravity.transformVectorNormalized(xyz_scene);
        mesh.vbo.push_back(xyz_esp);

        if (assimpMesh.mNormals) {


Um, there's a whole AssimpImporter for this kind of thing. Can't the datatool be just deleted or something?

A cleaner and safer way (bounds checking, type compatiblity checking...) instead of Mn::Vector3::from() etc would be to take the whole mNormals array, make an array view and cast it:

Cr::Containers::ArrayView<const Mn::Vector3> normals = Cr::Containers::arrayCast<const Mn::Vector3>(Cr::Containers::arrayView(assimpMesh.mNormals, assimpMesh.mNumVertices));

And similar for positions etc.

Yeah, I know, I just don't want to make that decision unilaterally. @aclegg3 Thoughts?

mosra · 2024-01-23T10:44:32Z

src/esp/core/EspEigen.h

+// template <typename T>
+// std::ostream& operator<<(std::ostream& os, const Map<T>& m) {
+//   return os << m.format(kJsonFormat);
+// }


Not sure what you plan here, but one way to efficiently format these might be with Utility::JsonWriter array support and vector.data() / matrix.data(), which both return a statically sized array reference.

mosra · 2024-01-23T10:45:10Z

src/esp/core/Utility.h

+             Magnum::Math::cross(fromNorm, halfVec).normalized(),
+             Magnum::Math::dot(fromNorm, halfVec))
+      .normalized();
+}  // quatRotFromTwoVectors


Probably too much to ask in this context, but I'd like to have this contributed to Magnum :)

src/esp/geo/CoordinateFrame.cpp

src/esp/geo/CoordinateFrame.h

mosra · 2024-01-23T10:51:21Z

src/esp/nav/PathFinder.cpp

@@ -988,7 +995,7 @@ struct NavMeshTileHeader {
 };

 struct Triangle {
-  std::vector<vec3f> v;
+  std::vector<Mn::Vector3> v;
  Triangle() { v.resize(3); }
 };


Oh, wow, ew, can't this be just a Mn::Vector3 v[3]? :D Or a Containers::StaticArray if you want bounds check in debug.

src/esp/sensor/Sensor.cpp

src/tests/GeoTest.cpp

mosra · 2024-01-23T11:15:57Z

src/utils/viewer/viewer.cpp

+      auto v = agentBodyNode_->translation();
+      Cr::Utility::formatInto(res, res.length(),
+                              "Agent position  : [{} {} {}] ", v.x(), v.y(),
+                              v.z());


Unnecessarily overcomplicated, just use the builtin debug output ~~redirected to a stringstream~~. The redirection and if(res.size() > 0) is also redundant, if it's going to get printed it's non-empty already anyway.

ESP_DEBUG() << "Agent position:" << agentBodyNode_->translation();

Same below. If you want a more compact output for vectors (and quats), use Mn::Debug::packed << agentBodyNode_->translation() instead.

src/utils/viewer/viewer.cpp

mosra · 2024-01-27T20:21:45Z

src/esp/nav/PathFinder.cpp

  const int numVerts = mesh.vbo.size();
  const int numIndices = mesh.ibo.size();
  const float mf = std::numeric_limits<float>::max();
-  vec3f bmin(mf, mf, mf);
-  vec3f bmax(-mf, -mf, -mf);
+  Mn::Vector3 bmin(mf, mf, mf);
+  Mn::Vector3 bmax(-mf, -mf, -mf);

  for (int i = 0; i < numVerts; ++i) {
-    const vec3f& p = mesh.vbo[i];
-    bmin = bmin.cwiseMin(p);
-    bmax = bmax.cwiseMax(p);
+    const Mn::Vector3& p = mesh.vbo[i];
+    bmin = Mn::Math::min(bmin, p);
+    bmax = Mn::Math::max(bmax, p);
  }


Was working on something unrelated and came across MeshTools::boundingRange(), which is basically just a call to Math::minmax(). No fancy SIMD internals there yet, but it's a candidate.

rebase error

…trices

With the Corrade update, no need to use Matrix3x3

magnum points can be directly compared

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 15, 2024

jturner65 requested a review from aclegg3 January 15, 2024 20:32

jturner65 force-pushed the BE_RemoveEigenDeps branch 6 times, most recently from 78eee5f to 76938f1 Compare January 16, 2024 17:57

mosra reviewed Jan 16, 2024

View reviewed changes

jturner65 force-pushed the BE_RemoveEigenDeps branch 4 times, most recently from fdb649c to 3f6882e Compare January 22, 2024 21:14

mosra reviewed Jan 23, 2024

View reviewed changes

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from 4aa0dff to 2f43fa1 Compare January 25, 2024 15:11

mosra reviewed Jan 27, 2024

View reviewed changes

mosra mentioned this pull request Jan 29, 2024

Update Magnum and m.css to latest #2309

Merged

jturner65 force-pushed the BE_RemoveEigenDeps branch 3 times, most recently from afae3b5 to 1507e23 Compare February 5, 2024 13:58

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from e382b1c to b371c7b Compare February 13, 2024 14:12

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from 4d28775 to aef35d7 Compare February 21, 2024 14:32

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from 7b25699 to f773e28 Compare February 29, 2024 17:05

jturner65 force-pushed the BE_RemoveEigenDeps branch 3 times, most recently from 49557ad to d58931c Compare March 5, 2024 20:02

jturner65 added 29 commits November 13, 2024 14:50

--address obbs (change box3f to Magnum Range 3D)

6949ae4

--semantic bindings issue

ed8847a

--more cleanup.

87a77ec

--bugfixes

7cdf3e1

--bug fixes

2f1042c

--more bugfixes

a1ad0b0

--still more bugfixes

bf39c7f

--oops

f523cca

--reviewer suggestions

c54b151

--clang-tidy

2fcccdf

--reviewer comments

7705442

--missing include

f5c376f

--reviewer suggestions

f489497

--fix an error - shouldn't normalize the cross product.

75baa67

--cleanup;

2e3ee8e

--use join function

8eeda7b

--fix bbox bindings issue

afe9cd1

--fix broken constructor

0d952ff

rebase error

--another ortho test mistake

9f4b7c6

--replace quat rotation from 2 vectors function with magnum version

b1347d1

--Specify rotations as Matrix3x3 instead of full 2D transformation ma…

89f4a84

…trices

--cleanup

6a9c3dd

--missing include

27b34cd

--cleanup

b85dda5

--return to Matrix3

f6feabe

With the Corrade update, no need to use Matrix3x3

--remove call to obviated function

f4b3609

--remove unused include

69564ae

--cleanup

a70ca1e

--fix test

4defdec

magnum points can be directly compared

jturner65 force-pushed the BE_RemoveEigenDeps branch from ee7cbc2 to 4defdec Compare November 13, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--[BE][WIP] Remove Eigen dependency #2301

--[BE][WIP] Remove Eigen dependency #2301

jturner65 commented Jan 15, 2024 •

edited by aclegg3

Loading

mosra left a comment

mosra Jan 16, 2024

mosra Jan 16, 2024

jturner65 Jan 16, 2024

mosra Jan 23, 2024

mosra Jan 23, 2024

mosra Jan 23, 2024

mosra Jan 23, 2024

mosra Jan 27, 2024 •

edited

Loading

--[BE][WIP] Remove Eigen dependency #2301

Are you sure you want to change the base?

--[BE][WIP] Remove Eigen dependency #2301

Conversation

jturner65 commented Jan 15, 2024 • edited by aclegg3 Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

mosra left a comment

Choose a reason for hiding this comment

mosra Jan 16, 2024

Choose a reason for hiding this comment

mosra Jan 16, 2024

Choose a reason for hiding this comment

jturner65 Jan 16, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

jturner65 commented Jan 15, 2024 •

edited by aclegg3

Loading

mosra Jan 27, 2024 •

edited

Loading