-
Notifications
You must be signed in to change notification settings - Fork 400
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
--[BE][WIP] Remove Eigen dependency #2301
base: main
Are you sure you want to change the base?
Conversation
78eee5f
to
76938f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could take a const array view while you're at it.
Mn::Vector3 position; | ||
Mn::Vector3 normal; | ||
Mn::Vector2 texCoords; | ||
Mn::Vector3ui rgb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably meant Vector3ub
instead, or Color3ub
? 8 bits per component, not 32.
Too bad this datatool thing has no test coverage at all.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know, I just don't want to make that decision unilaterally. @aclegg3 Thoughts?
fdb649c
to
3f6882e
Compare
// template <typename T> | ||
// std::ostream& operator<<(std::ostream& os, const Map<T>& m) { | ||
// return os << m.format(kJsonFormat); | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Magnum::Math::cross(fromNorm, halfVec).normalized(), | ||
Magnum::Math::dot(fromNorm, halfVec)) | ||
.normalized(); | ||
} // quatRotFromTwoVectors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably too much to ask in this context, but I'd like to have this contributed to Magnum :)
@@ -988,7 +995,7 @@ struct NavMeshTileHeader { | |||
}; | |||
|
|||
struct Triangle { | |||
std::vector<vec3f> v; | |||
std::vector<Mn::Vector3> v; | |||
Triangle() { v.resize(3); } | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
CORRADE_ASSERT((abs(orientation.array()) >= 0).any(), | ||
"SensorSpec::sanityCheck(): orientation is illegal", ); | ||
// CORRADE_ASSERT((abs(position) >= 0).any(), | ||
// "SensorSpec::sanityCheck(): position is illegal", ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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
auto v = agentBodyNode_->translation(); | ||
Cr::Utility::formatInto(res, res.length(), | ||
"Agent position : [{} {} {}] ", v.x(), v.y(), | ||
v.z()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
4aa0dff
to
2f43fa1
Compare
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
afae3b5
to
1507e23
Compare
e382b1c
to
b371c7b
Compare
4d28775
to
aef35d7
Compare
7b25699
to
f773e28
Compare
49557ad
to
d58931c
Compare
rebase error
1cddab4
to
7d0c646
Compare
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
Checklist