--[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

mosra · 2024-01-16T20:52:19Z

src/utils/datatool/Mp3dInstanceMeshData.cpp

+    Mn::Vector3 position;
+    Mn::Vector3 normal;
+    Mn::Vector2 texCoords;
+    Mn::Vector3ui rgb;


Probably meant Vector3ub instead, or Color3ub? 8 bits per component, not 32.

Too bad this datatool thing has no test coverage at all.

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.

mosra · 2024-01-23T11:09:39Z

src/esp/sensor/Sensor.cpp

-  CORRADE_ASSERT((abs(orientation.array()) >= 0).any(),
-                 "SensorSpec::sanityCheck(): orientation is illegal", );
+  // CORRADE_ASSERT((abs(position) >= 0).any(),
+  //                "SensorSpec::sanityCheck(): position is illegal", );


(Math::abs(position) >= Vector3{0.0f}).any() and similarly below should work, but I'm not sure what the check is supposed to actually do. So maybe a better message also? :D

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

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 May 21, 2024 16:48

--remove eigen refs from viewer.

fdc69b0

--remove unused eigen typedefs

4278f96

--many refactors across system

c599595

--fix replicascene semantics load

75e20a2

--fix elementwise pathfinder min/max

4cd5bcb

--return data array of quaternion in binding

6316b6c

--External Utils : datatool, scene loader and Mp3dInstanceMeshData tools

a764c51

--update mesh joining

87238f2

--remove more unused Eigen typedefs

ea4b246

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

45b4b72

--semantic bindings issue

daf4755

--fix objectControls

31697c5

--fix agent

28880a1

--more cleanup.

66e1a1e

--bugfixes

9cc272f

--bug fixes

266a00e

--more bugfixes

4235091

--still more bugfixes

8f69e25

--oops

d8b2ccd

--reviewer suggestions

0523d09

--clang-tidy

11aefac

--reviewer comments

2d407e9

--missing include

72f1c16

--reviewer suggestions

a239e2e

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

c5da190

--cleanup;

50ad57b

--use join function

fbf740f

--fix bbox bindings issue

fa4fb6c

--fix broken constructor

1cddab4

rebase error

jturner65 force-pushed the BE_RemoveEigenDeps branch from 2be7f79 to 1cddab4 Compare May 21, 2024 20:48

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

mosra left a comment

mosra Jan 16, 2024

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 23, 2024

mosra Jan 27, 2024 •

edited

--[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

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

mosra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosra Jan 27, 2024 • edited

Choose a reason for hiding this comment

jturner65 commented Jan 15, 2024 •

edited by aclegg3

mosra Jan 27, 2024 •

edited