geometry: Support Convex mesh. #9471
Conversation
|
It's still WIP. Please help me check on style compliance before I put more code. @sherm1 for feature review, please. |
|
A design question to @SeanCurtis-TRI. Since We only need a temporary solution before the new FCL is available. |
|
a discussion (no related file): |
|
a discussion (no related file): Previously, DamrongGuoy (Damrong Guoy) wrote…
CI is ok now. Thanks. |
There was a problem hiding this comment.
Looks very nice, thanks! A few comments below, PTAL.
Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: 15 unresolved discussions, platform LGTM missing
geometry/geometry_visualization.cc, line 114 at r2 (raw file):
} void ImplementGeometry(const Convex&, void*) override {
BTW I would suggest adding a TODO before this line, like
// TODO(DamrongGuoy) Implement this.Then it is easy to spot not-yet-done code.
geometry/proximity_engine.cc, line 645 at r2 (raw file):
// // Convert vertices from tinyobj format to FCL format
Nit: should end with a period per style guide (GSG).
geometry/proximity_engine.cc, line 654 at r2 (raw file):
// = { v0, v1, v2, ...} // // The size of `attrib.vertices` is three times the number of vertices.
BTW great documentation!
geometry/proximity_engine.cc, line 659 at r2 (raw file):
const double scale) const { int num_coords = attrib.vertices.size(); DRAKE_ASSERT(num_coords % 3 == 0);
Nit: consider using DRAKE_DEMAND here if there is no performance issue -- that way it is checked always, not just in Debug builds.
geometry/proximity_engine.cc, line 664 at r2 (raw file):
auto iter = attrib.vertices.begin(); while (iter != attrib.vertices.end()) {
Minor: I was going to suggest using a range-for here but now I see that the incrementing is unusual so this might be more clear as is. But it is unusual enough that I would like to see a short comment pointing out the increment-by-3 feature.
geometry/proximity_engine.cc, line 675 at r2 (raw file):
// // Convert faces from tinyobj to FCL
Nit: should end with a period per style guide (GSG).
geometry/proximity_engine.cc, line 678 at r2 (raw file):
// // // A tinyobj mesh has an array of integer storing the number of vertices of
Nit: "array of integer" -> either "integer array" or "array of integers"
geometry/proximity_engine.cc, line 685 at r2 (raw file):
// face2 has n2 vertices. // ... // A tinyobj mesh has a vector of vertex that belongs to the faces.
Nit: "vector of vertex that belongs to" -> "vector of vertices that belong to"
geometry/proximity_engine.cc, line 700 at r2 (raw file):
// ...} // where n_i is the number of vertices of face_i. //
BTW nice docs!!
geometry/proximity_engine.cc, line 706 at r2 (raw file):
for (const int& num : mesh.num_face_vertices) { faces->push_back(num); std::for_each(iter, iter + num, [&](const tinyobj::index_t& index) {
Nit: since only faces is accessed and it is a pointer, consider capturing just [faces](...) (by value) rather than capturing everything by reference.
geometry/proximity_engine.cc, line 722 at r2 (raw file):
std::string err; // We keep polygonal faces without triangulating them. bool do_tinyobj_triangulation = false;
Minor: should we be doing this? Maybe it would be better to pass only triangle meshes through to FCL if tinyobj will fix them up for us. The hydrostatic contact model requires triangles. What do you think @SeanCurtis-TRI ?
geometry/proximity_engine.cc, line 725 at r2 (raw file):
bool ret = tinyobj::LoadObj(&attrib, &shapes, &materials, &err, convex.filename().c_str(), "", do_tinyobj_triangulation);
Nit: the "" here is mysterious (to me). A short comment would help.
geometry/proximity_engine.cc, line 737 at r2 (raw file):
std::vector<Vector3d> vertices = TinyObjToFclVertices(attrib, convex.scale()); // We support only the first shape.
Minor: this seems like an important restriction that should be documented at an API level if it isn't already.
geometry/proximity_engine.cc, line 738 at r2 (raw file):
TinyObjToFclVertices(attrib, convex.scale()); // We support only the first shape. assert(shapes.begin() != shapes.end());
Minor: use DRAKE_ASSERT() or DRAKE_DEMAND().
geometry/proximity_engine.cc, line 751 at r2 (raw file):
auto fcl_convex = make_shared<fcl::Convexd>( vertices.size(), vertices.data(), num_faces, faces.data()); TakeShapeOwnership(fcl_convex, user_data);
BTW does this need a TODO?
geometry/shape_specification.h, line 195 at r2 (raw file):
}; //** Support for convex shapes. */
Minor: start doxygen comment with /** (has an extra slash).
geometry/shape_specification.h, line 200 at r2 (raw file):
DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(Convex) /** Constructs a convex shape specification from the file located at the
Minor: should say what kind(s) of files are supported, and any limitations on our treatment of those files (like only reads the first shape).
geometry/shape_specification.h, line 201 at r2 (raw file):
/** Constructs a convex shape specification from the file located at the given _absolute_ file path. Optionally uniformly scaled by the given scale factor.
Nit: line too long (lint doesn't always catch these in a comment).
geometry/test/shape_specification_test.cc, line 44 at r2 (raw file):
received_user_data_ = data; convex_made_ = true; }
Minor: looks like this needs a TODO for missing unit test?
There was a problem hiding this comment.
Done with most of the suggestions. I will work on the two remaining points again. Thanks!
Reviewable status: 2 unresolved discussions, platform LGTM missing
geometry/geometry_visualization.cc, line 114 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW I would suggest adding a
TODObefore this line, like// TODO(DamrongGuoy) Implement this.Then it is easy to spot not-yet-done code.
Done.
geometry/proximity_engine.cc, line 645 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: should end with a period per style guide (GSG).
Done.
geometry/proximity_engine.cc, line 654 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW great documentation!
Thanks!
geometry/proximity_engine.cc, line 659 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: consider using
DRAKE_DEMANDhere if there is no performance issue -- that way it is checked always, not just in Debug builds.
Done.
geometry/proximity_engine.cc, line 664 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: I was going to suggest using a range-for here but now I see that the incrementing is unusual so this might be more clear as is. But it is unusual enough that I would like to see a short comment pointing out the increment-by-3 feature.
Done.
geometry/proximity_engine.cc, line 675 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: should end with a period per style guide (GSG).
Done.
geometry/proximity_engine.cc, line 678 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: "array of integer" -> either "integer array" or "array of integers"
Done.
geometry/proximity_engine.cc, line 685 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: "vector of vertex that belongs to" -> "vector of vertices that belong to"
Done.
geometry/proximity_engine.cc, line 700 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW nice docs!!
Thanks!
geometry/proximity_engine.cc, line 706 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: since only
facesis accessed and it is a pointer, consider capturing just[faces](...)(by value) rather than capturing everything by reference.
Done. Thanks!
geometry/proximity_engine.cc, line 737 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: this seems like an important restriction that should be documented at an API level if it isn't already.
Done at API level (shape_specification.h). This ProximityEngine::Impl is hidden from users.
geometry/proximity_engine.cc, line 738 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: use
DRAKE_ASSERT()orDRAKE_DEMAND().
Done.
geometry/proximity_engine.cc, line 751 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW does this need a TODO?
Done. I will do it in the next commit.
geometry/shape_specification.h, line 195 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: start doxygen comment with
/**(has an extra slash).
Done.
geometry/shape_specification.h, line 200 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: should say what kind(s) of files are supported, and any limitations on our treatment of those files (like only reads the first shape).
Done.
geometry/shape_specification.h, line 201 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: line too long (lint doesn't always catch these in a comment).
Done. I have CLion draw a vertical line at the 80th character now.
geometry/test/shape_specification_test.cc, line 44 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: looks like this needs a TODO for missing unit test?
Done.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, platform LGTM missing
geometry/proximity_engine.cc, line 722 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: should we be doing this? Maybe it would be better to pass only triangle meshes through to FCL if tinyobj will fix them up for us. The hydrostatic contact model requires triangles. What do you think @SeanCurtis-TRI ?
We will need to decide together again whether to triangulate or not. Or make it an option at the API level in shape_specification?
geometry/proximity_engine.cc, line 725 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: the
""here is mysterious (to me). A short comment would help.
I will have to check tinyobj again whether it accepts an empty filepath ("") with the full path as a part of filename. In any case, I will need to comment why I use "".
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3.
Reviewable status: 2 unresolved discussions, platform LGTM missing
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, platform LGTM missing, commit history needs curation
geometry/proximity_engine.cc, line 725 at r2 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I will have to check tinyobj again whether it accepts an empty
filepath("") with the full path as a part offilename. In any case, I will need to comment why I use"".
I checked drake/attic/multibody/shapes/geometry.cc, the class DrakeShapes::Mesh has facilities to
- check whether the file with the
objextension exists. - if not, change to ".obj" extension and check again.
- the
filenamecan either contain a path to the file or not (search in working directory).
I suppose we want to do the same thing in ProximityEngine. I hope it is all right to copy some code from DrakeShapes::Mesh to ProximityEngine. In the future, I can imagine a centralized File I/O module to avoid code duplication.
Alternatively, we can assume exact match to the file name and look in the working directory only. In that case, the parameter will be nullptr, and I will write a comment to explain what it means.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, platform LGTM missing, commit history needs curation
geometry/proximity_engine.cc, line 722 at r2 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
We will need to decide together again whether to triangulate or not. Or make it an option at the API level in shape_specification?
This is resolved for now as "keep polygonal faces". But I think it needs a TODO to verify that:
- all faces are planar (to machine precision)
- the mesh is actually convex.
We should also discuss where to add those checks.
geometry/proximity_engine.cc, line 725 at r2 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I checked
drake/attic/multibody/shapes/geometry.cc, the class DrakeShapes::Mesh has facilities to
- check whether the file with the
objextension exists.- if not, change to ".obj" extension and check again.
- the
filenamecan either contain a path to the file or not (search in working directory).
I suppose we want to do the same thing in ProximityEngine. I hope it is all right to copy some code from DrakeShapes::Mesh to ProximityEngine. In the future, I can imagine a centralized File I/O module to avoid code duplication.Alternatively, we can assume exact match to the file name and look in the working directory only. In that case, the parameter will be
nullptr, and I will write a comment to explain what it means.
Sorry, I'm not following. As the reader of the code I see the "" but don't know what it means. Comments here in Reviewable don't resolve that problem.
Are you introducing a new topic here?
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, platform LGTM missing, commit history needs curation
geometry/proximity_engine.cc, line 722 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
This is resolved for now as "keep polygonal faces". But I think it needs a TODO to verify that:
- all faces are planar (to machine precision)
- the mesh is actually convex.
We should also discuss where to add those checks.
Added TODO at line 776 in the new commit. I will be happy to work on it as another PR.
geometry/proximity_engine.cc, line 725 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Sorry, I'm not following. As the reader of the code I see the "" but don't know what it means. Comments here in Reviewable don't resolve that problem.
Are you introducing a new topic here?
I am sorry for not explaining it. I should have done face-to-face discussion. The "" means only looking at the current working directory. The other code (attic/.../geometry.cc) can specify a path. I'll explain to you face-to-face tomorrow (Fri).
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, platform LGTM missing, commit history needs curation
geometry/proximity_engine.cc, line 767 at r4 (raw file):
// vertices each. In the future, as we expand FCL, this assumption might // change, and we might want to triangulate in a controlled way; for // example, we might use Delaunay triangulation.
Minor: per f2f, reword so the comment doesn't require updating. E.g., some convex geometry algorithms perform better with fewer features ...
geometry/proximity_engine.cc, line 770 at r4 (raw file):
bool do_tinyobj_triangulation = false; bool ret = tinyobj::LoadObj(&attrib, &shapes, &materials, &err, obj_file_name.c_str(), path.c_str(), do_tinyobj_triangulation);
Per f2f currently this is passing the path twice if obj_file_name was absolute or relative with a slash, like geometry/cube.obj. Please clarify.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM missing, commit history needs curation
geometry/proximity_engine.cc, line 767 at r4 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: per f2f, reword so the comment doesn't require updating. E.g., some convex geometry algorithms perform better with fewer features ...
Done.
geometry/proximity_engine.cc, line 770 at r4 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Per f2f currently this is passing the path twice if obj_file_name was absolute or relative with a slash, like geometry/cube.obj. Please clarify.
Previously I misunderstood the meaning of that parameter. Actually it's a path to "mtl" file (material description), not a path to obj file. Now I use the default value "mtl_basedir = nullptr" with the comment explaining the meaning.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: 4 unresolved discussions, platform LGTM missing, commit history needs curation
geometry/proximity_engine.cc, line 770 at r4 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Previously I misunderstood the meaning of that parameter. Actually it's a path to "mtl" file (material description), not a path to obj file. Now I use the default value "mtl_basedir = nullptr" with the comment explaining the meaning.
Great, thanks!
geometry/proximity_engine.cc, line 767 at r5 (raw file):
int num_faces = TinyObjToFclFaces(mesh, &faces); convex_objects_.push_back(ConvexData(std::move(vertices), num_faces,
Minor: this might be making an expensive copy of ConvexData because push_back has to default-construct before copying or moving from its argument. Would be better I think to use emplace_back here so that the ConvexData object gets constructed in-place in the convex_objects vector.
geometry/proximity_engine.cc, line 769 at r5 (raw file):
convex_objects_.push_back(ConvexData(std::move(vertices), num_faces, std::move(faces))); auto c = convex_objects_.rbegin();
Nit: a few suggestions here.
cis too abbreviated for the GSG style which discourages obscure variable names unless there is a good reason for them- generally better to work with objects than pointers (or iterators) when you can (e.g. no need to check for nullptr or end())
- work with
constobjects when possible (not certain whether FCL allows that but hopefully it does)
Consider:
const ConvexData& object = convex_objects_.back(); // or a better name
auto fcl_convex = make_shared<fcl::Convexd>(
object.vertices().size(), object.vertices().data(), num_faces,
object.faces().data());geometry/proximity_engine.cc, line 1028 at r5 (raw file):
// for efficiency. ConvexData(std::vector<Vector3d>&& v, int n, std::vector<int>&& f): vertices_(v), num_faces_(n), faces_(f) {
This copies rather than moves because v, n, and f are lvalues here (because they have names). To do what you wanted, this should read:
vertices_(std::move(v)), num_faces_(n), faces_(std::move(f))geometry/proximity_engine.cc, line 1032 at r5 (raw file):
std::vector<Vector3d>& vertices() { return vertices_; } int& num_faces() { return num_faces_; } std::vector<int>& faces() { return faces_; }
Do these have to be non-const? I would have expected const references here (and just a value return from num_faces().
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: 4 unresolved discussions, platform LGTM missing, commit history needs curation
geometry/proximity_engine.cc, line 767 at r5 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Minor: this might be making an expensive copy of ConvexData because
push_backhas to default-construct before copying or moving from its argument. Would be better I think to useemplace_backhere so that the ConvexData object gets constructed in-place in the convex_objects vector.
FYI With overload (2) https://en.cppreference.com/w/cpp/container/vector/push_back, I think the performance would actually be okay, but I would still suggest using emplace_back by convention / for readability.
geometry/proximity_engine.cc, line 1021 at r5 (raw file):
// The data needed by fcl::Convex. class ConvexData { public:
FYI All classes needs DRAKE_DEFAULT_COPY.... or DRAKE_NO_COPY.... macro here.
However, also I don't understand why this is a class -- by convention, I think we'd usually use a struct in this situation -- a private type that we want to be copyable and merely gather a few pieces up, without trying to restrict access to them or enforce any invariants. Using a struct would also obviate all of the constructor- and getter-signature discussions below.
|
The reviewable assignments post missed the |
DamrongGuoy
changed the title
There was a problem hiding this comment.
Thanks @jwnimmer-tri for reminding me about + ahead of the @user.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee seancurtis-tri, LGTM missing from assignee sherm1, platform LGTM missing, commit history needs curation
a discussion (no related file):
I removed [wip] from the title now. As soon as the feature review passes, I'll request a platform review. Thank you!
geometry/proximity_engine.cc, line 767 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
FYI With overload (2) https://en.cppreference.com/w/cpp/container/vector/push_back, I think the performance would actually be okay, but I would still suggest using emplace_back by convention / for readability.
Changed to emplace_back() now. It's shorter too. Thanks!
geometry/proximity_engine.cc, line 769 at r5 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: a few suggestions here.
cis too abbreviated for the GSG style which discourages obscure variable names unless there is a good reason for them- generally better to work with objects than pointers (or iterators) when you can (e.g. no need to check for nullptr or end())
- work with
constobjects when possible (not certain whether FCL allows that but hopefully it does)Consider:
const ConvexData& object = convex_objects_.back(); // or a better name auto fcl_convex = make_shared<fcl::Convexd>( object.vertices().size(), object.vertices().data(), num_faces, object.faces().data());
Done. Thanks!
geometry/proximity_engine.cc, line 1021 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
FYI All classes needs DRAKE_DEFAULT_COPY.... or DRAKE_NO_COPY.... macro here.
However, also I don't understand why this is a class -- by convention, I think we'd usually use a struct in this situation -- a private type that we want to be copyable and merely gather a few pieces up, without trying to restrict access to them or enforce any invariants. Using a struct would also obviate all of the constructor- and getter-signature discussions below.
Thanks! Changed to to struct now.
geometry/proximity_engine.cc, line 1028 at r5 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
This copies rather than moves because v, n, and f are lvalues here (because they have names). To do what you wanted, this should read:
vertices_(std::move(v)), num_faces_(n), faces_(std::move(f))
Done. Thanks!
geometry/proximity_engine.cc, line 1032 at r5 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Do these have to be non-const? I would have expected const references here (and just a value return from num_faces().
I changed ConvexData to struct as Jeremy suggested, so we don't have this problem anymore.
There was a problem hiding this comment.
Feature 
+@sammy-tri for platform review please, per rotation.
Reviewed 5 of 5 files at r7.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1], commit history needs curation
geometry/proximity_engine.cc, line 768 at r7 (raw file):
convex_objects_.emplace_back(move(vertices), num_faces, move(faces)); ConvexData& object = convex_objects_.back();
Nit: can this be const?
geometry/test/proximity_engine_test.cc, line 1148 at r7 (raw file):
TEST_F(BoxPenetrationTest, TangentConvex1) { // TODO(DamrongGuoy): We should check why we cannot use a smaller tolerance. TestCollision1(TangentConvex, 1e-3);
BTW Yikes! That's an awfully loose tolerance.
There was a problem hiding this comment.
First pass done.
Reviewed 3 of 4 files at r3, 5 of 5 files at r7.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1], commit history needs curation
geometry/geometry_visualization.cc, line 115 at r7 (raw file):
// TODO(DamrongGuoy) Implement this. void ImplementGeometry(const Convex&, void*) override {
Given that you're merely reading an obj file, you should simply mirror the Mesh implementation for the Convex.
geometry/proximity_engine.cc, line 672 at r7 (raw file):
double y = *(iter++) * scale; double z = *(iter++) * scale; vertices.push_back(Vector3d(x, y, z));
BTW this could be vertices.emplace_back(x, y, z);.
geometry/proximity_engine.cc, line 734 at r7 (raw file):
convex.filename().c_str(), mtl_basedir, do_tinyobj_triangulation); if (!ret || !err.empty()) { throw std::runtime_error("Error parsing file \"" + convex.filename() +
BTW You can avoid escaping the " character by using the single quote '. It's pretty common in error messages in Drake.
geometry/proximity_engine.cc, line 738 at r7 (raw file):
} // TODO(DamrongGuoy) Check that the input is a valid convex polyhedron.
BTW This validation should ultimately belong inside FCL. So, Drake should get this for free.
geometry/proximity_engine.cc, line 751 at r7 (raw file):
// We support only the first shape. DRAKE_DEMAND(shapes.begin() != shapes.end());
nit: This test should come before you convert the vertices.
geometry/shape_specification.h, line 204 at r7 (raw file):
factor. @param absolute_filename The file name with absolute path. We support an Obj file with one object. If the file
Do you know what it means for an "obj" to have more than one "object"?
In the current state, we only consume objs. So, that means this documentation should allow someone to look at their OBJ and recognize how it conforms to these requirements. I'm not convinced that this documentation does it.
geometry/shape_specification.h, line 220 at r7 (raw file):
};
nit: Extra line of whitespace.
geometry/test/proximity_engine_test.cc, line 1148 at r7 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW Yikes! That's an awfully loose tolerance.
BTW Have you seen the TangentProneCylinder test? 1e-1.
geometry/test/shape_specification_test.cc, line 42 at r7 (raw file):
} void ImplementGeometry(const Convex&, void* data) override { // TODO(DamrongGuoy): Implement this.
Is this todo still relevant?
geometry/test/shape_specification_test.cc, line 79 at r7 (raw file):
EXPECT_FALSE(half_space_made_); EXPECT_FALSE(cylinder_made_); EXPECT_FALSE(box_made_);
All of these tests should EXPECT_FALSE(convex_made_);.
geometry/test/shape_specification_test.cc, line 106 at r7 (raw file):
EXPECT_FALSE(half_space_made_); EXPECT_FALSE(cylinder_made_); EXPECT_TRUE(box_made_);
There should be a Convex::Reify() call here.
DamrongGuoy
force-pushed
the
CnvxMesh
branch
from
cab9306
to
895ea64
Compare
There was a problem hiding this comment.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
geometry/geometry_visualization.cc, line 115 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Given that you're merely reading an obj file, you should simply mirror the Mesh implementation for the Convex.
Thanks! I created issue #9511 as my next task.
geometry/proximity_engine.cc, line 768 at r7 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: can this be const?
Sorry, it couldn't be const because currently the constructor fcl::Convex() does not like the const. In the future, the following FCL PR will allow us to use const: fcl:Change convex shape constructor#338.
geometry/test/proximity_engine_test.cc, line 1148 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Have you seen the TangentProneCylinder test? 1e-1.
I created issue #9512 to check it again later.
There was a problem hiding this comment.
Reviewed 1 of 4 files at r8.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
geometry/test/shape_specification_test.cc, line 42 at r7 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
With the extra code below, this TODO is done. Thanks!
Done.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
geometry/geometry_visualization.cc, line 115 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'm not wildly in favor of making that a separate issue. It's a matter of (simply copying 6 lines from the previous function into this one. If you want to be slightly more sophisticated, you could refactor it into a common function that both of these invoke. I would advocate in favor of copy and paste.
Adding
Convexto Drake is just six lines of basically being complete. It would be a shame to defer those six lines.(With a side note: because you pre-scale the vertices, you should make sure you set the scale factor to 1s.)
I tried it before. I thought I need to do something like:
geometry_data_.type = geometry_data.CONVEX;
But I got stuck at lcmt_viewer_geometry_data because I thought I need to add CONVEX to lcmt_viewer_geometry_data.hpp. However, CLion showed something like a very long path to a file outside my drake clone.
Or I can just copy the six lines from ImplementGeometry(Mesh) treating Convex as a kind of Mesh for visualization? That would be easy. I'll update the code this afternoon and close #9511 afterwards.
I don't need to set up a test of Convex in geometry_visualization_test.cc, correct?
geometry/shape_specification.h, line 204 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The change to the comment doesn't address my issue.
If I were to look at the contents of an OBJ file, how would I be able to predict how this comment will affect what is used in the file? This is a very detailed kind of question. I suspect, if you're not familiar with the OBJ format, that you probably don't know the answer. I have a guess.
Correct. I don't know enough about OBJ format to answer. I know about multiple shapes only from tinyobj reader. I can look up OBJ format this afternoon.
There was a problem hiding this comment.
First pass complete.
Reviewed 4 of 5 files at r7, 4 of 4 files at r8.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
geometry/geometry_visualization.cc, line 115 at r7 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I tried it before. I thought I need to do something like:
geometry_data_.type = geometry_data.CONVEX;But I got stuck at lcmt_viewer_geometry_data because I thought I need to add CONVEX to lcmt_viewer_geometry_data.hpp. However, CLion showed something like a very long path to a file outside my drake clone.
Or I can just copy the six lines from ImplementGeometry(Mesh) treating Convex as a kind of Mesh for visualization? That would be easy. I'll update the code this afternoon and close #9511 afterwards.
I don't need to set up a test of
Convexingeometry_visualization_test.cc, correct?
Correct; no major testing required. Sloppy, but this code is slated to die.
geometry/shape_specification.h, line 204 at r7 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Correct. I don't know enough about OBJ format to answer. I know about multiple shapes only from
tinyobjreader. I can look up OBJ format this afternoon.
If you aren't able to resolve this today, we can discuss it in the morning.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
geometry/geometry_visualization.cc, line 115 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Correct; no major testing required. Sloppy, but this code is slated to die.
Done. Thanks!
geometry/proximity_engine.cc, line 672 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
You missed half of the comment. You'll note that in addition to swapping "push" for "emplace", I also dropped the explicit constructor.
Done. Thanks!
geometry/proximity_engine.cc, line 734 at r8 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Is it my imagination that this is the back tick and not the single quote?
Done. Changed the back tick to the single quote. Sorry for confusion.
geometry/shape_specification.h, line 204 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
If you aren't able to resolve this today, we can discuss it in the morning.
Thanks. Let's discuss tomorrow morning. I quickly went through a spec of Object Files. The only relevance is the "g" for group keyword, but I'm still not sure how it works.
geometry/test/shape_specification_test.cc, line 114 at r8 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Nice file name. :)
Thanks!
There was a problem hiding this comment.
Reviewed 1 of 3 files at r9.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
There was a problem hiding this comment.
Reviewed 1 of 4 files at r8, 3 of 3 files at r9.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1]
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1], commit history needs curation
geometry/shape_specification.h, line 204 at r7 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Thanks. Let's discuss tomorrow morning. I quickly went through a spec of Object Files. The only relevance is the "g" for group keyword, but I'm still not sure how it works.
Done. Per f2f discussion, we will throw an exception when the OBJ file has two or more objects. New test ExceptionTwoObjectsInObjFileForConvex in proximity_engine_test.cc. The test file (forbidden_two_cubes.obj) has two object-name statements (o object_name).
There was a problem hiding this comment.
Reviewed 5 of 5 files at r10.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1], commit history needs curation
geometry/shape_specification.h, line 208 at r10 (raw file):
@param scale An optional scale to coordinates. \warning Gives error if the OBJ file has two or more objects, i.e.,
BTW @throws std::runtime_error if... is a more typical formulation of this kind of statement in Drake's doxygen usage.
There was a problem hiding this comment.
I wouldn't be me if I didn't quibble about details. :) Couple of last thoughts.
Reviewed 1 of 3 files at r9, 5 of 5 files at r10.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1], commit history needs curation
geometry/proximity_engine.cc, line 747 at r10 (raw file):
nit: Too many "only"s in this sentence. Alternative:
For Convex geometry, the OBJ file must have one and only one object defined in it.
The "one and only one" message makes this error message appropriate whether the obj file is empty or if there are two many objects.
Also, the comment precedes an exception that says literally the same thing. Therefore, the comment doesn't contribute anything.
geometry/shape_specification.h, line 208 at r10 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
BTW
@throws std::runtime_error if...is a more typical formulation of this kind of statement in Drake's doxygen usage.
nit: I'd up-vote that: I'd go so far to call it a defect.
geometry/shape_specification.h, line 209 at r10 (raw file):
I suspect this condition is not quite right. I don't know for a fact, but I bet I could create an obj file with a single object-name statement which would nevertheless register multiple objects.
I'd recommend:
@throws std::runtime_error if the OBJ file doesn't define a single object. This can happen if it is empty, if there are multiple object-name statements (e.g.,
o object name), or if there are faces defined outside of a single object-name statement.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1], commit history needs curation
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, LGTM missing from assignee seancurtis-tri, platform LGTM from [sherm1], commit history needs curation
geometry/proximity_engine.cc, line 747 at r10 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Too many "only"s in this sentence. Alternative:
For Convex geometry, the OBJ file must have one and only one object defined in it.
The "one and only one" message makes this error message appropriate whether the obj file is empty or if there are two many objects.
Also, the comment precedes an exception that says literally the same thing. Therefore, the comment doesn't contribute anything.
Done. Thanks!
geometry/shape_specification.h, line 208 at r10 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: I'd up-vote that: I'd go so far to call it a defect.
Done. Thanks a lot! I completely missed the @throws.
geometry/shape_specification.h, line 209 at r10 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I suspect this condition is not quite right. I don't know for a fact, but I bet I could create an obj file with a single object-name statement which would nevertheless register multiple objects.
I'd recommend:
@throws std::runtime_error if the OBJ file doesn't define a single object. This can happen if it is empty, if there are multiple object-name statements (e.g.,
o object name), or if there are faces defined outside of a single object-name statement.
Done. Thanks!
There was a problem hiding this comment.
Huzzah!
Reviewed 2 of 2 files at r11.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri, platform LGTM from [sherm1, seancurtis-tri], commit history needs curation
There was a problem hiding this comment.
Reviewed 1 of 1 files at r12.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri, platform LGTM from [sherm1, seancurtis-tri], commit history needs curation
There was a problem hiding this comment.
Reviewed 2 of 5 files at r10, 2 of 2 files at r11, 1 of 1 files at r12.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, platform LGTM from [sherm1, seancurtis-tri], commit history needs curation
geometry/proximity_engine.cc, line 748 at r12 (raw file):
if (shapes.size() != 1) { throw std::runtime_error("For Convex geometry, the OBJ file must have one"
BTW is "OBJ" the right way to identify these files? You could also do it by suffix (".obj"). I have no opinion either way, just wanted to mention it for your consideration.
geometry/shape_specification.h, line 211 at r12 (raw file):
This can happen if it is empty, if there are multiple object-name statements (e.g., o object name), or if there are faces defined
Nit: typo? "o object name"
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri, platform LGTM from [sherm1, seancurtis-tri], commit history needs curation
geometry/proximity_engine.cc, line 748 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW is "OBJ" the right way to identify these files? You could also do it by suffix (".obj"). I have no opinion either way, just wanted to mention it for your consideration.
Changed OBJ to .obj now. Thanks!
geometry/shape_specification.h, line 208 at r10 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Done. Thanks a lot! I completely missed the @throws.
@sammy-tri, could you please approve this change that we use @throws instead of \warning ?
geometry/shape_specification.h, line 211 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Nit: typo? "o object name"
It's "o" tag followed by the name of the object. Now I make it clearer using double quote ("). Thanks!
There was a problem hiding this comment.
Reviewed 3 of 3 files at r13.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri, platform LGTM from [sherm1, seancurtis-tri], commit history needs curation
geometry/shape_specification.h, line 211 at r12 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
It's "o" tag followed by the name of the object. Now I make it clearer using double quote ("). Thanks!
Got it -- thanks!
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri, platform LGTM from [sherm1, seancurtis-tri], commit history needs curation
There was a problem hiding this comment.
Reviewed 3 of 3 files at r13.
Reviewable status: all discussions resolved, platform LGTM from [sherm1, seancurtis-tri, sammy-tri], commit history needs curation
DamrongGuoy
force-pushed
the
CnvxMesh
branch
from
3740cd2
to
0c12d02
Compare
Fix #9443
WIP. I still need to set up unit tests and to complete the ImplementGeometry(Convex) in ReifierTest and ShapeToLcm.
This change is