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

Add stl2obj to deal with robot stl meshes #14219

Merged
merged 1 commit into from Oct 20, 2020

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Oct 16, 2020

This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

FYI Just to be sure, I tested this on macOS and is passed.

I need to finish writing more docs and a test, but before I go that far let me ask ...

@EricCousineau-TRI and @SeanCurtis-TRI do you support this general direction? In short, that when we have third-party models with *.stl meshes, instead of manually converting to obj and committing the objs, we add a BUILD rule that calls this tool to automatically convert them during bazel build //....

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may also be a good model for how we can do mesh preprocessing in general. We are likely to need to perform operations like: tetrahedralization, computation of pressure fields, convexification, catching non-manifold surfaces, etc. at some point, which are very expensive and better to do once during build than on the fly.

BTW stl->obj is not an entirely straightforward conversion because stl has independent vertices for every triangle which have to be merged to produce an obj. VTK does that by default but doesn't specify a tolerance so could result in leaky objs. But I still think doing this automatically is a good idea, esp. if we can catch bad meshes downstream.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

@EricCousineau-TRI
Copy link
Contributor

Yuppers, this is what we use in Anzu, so I am 100% in support of this at the high-level!

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review October 19, 2020 15:22
@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for feature review, please.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM: With a vague wish for a bit more user guidance. There are some non-obvious implications on the use of this tool.

Reviewed 43 of 43 files at r1.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):

DEFINE_string(input, "", "STL filename to read");
DEFINE_string(output, "", "OBJ filename to write");

BTW There's some part of my soul that hopes for further guidance in using this. Given STL is the source, the OBJ produced will have a limited scope of utility. I.e., there are no texture coordinates (so texturing is out), the resulting is a triangle soup (so probably not useful for some geometric operations that require water-tight meshes without post-processing, etc.)

Right now this looks a bit too "magic-wandish".

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW There's some part of my soul that hopes for further guidance in using this. Given STL is the source, the OBJ produced will have a limited scope of utility. I.e., there are no texture coordinates (so texturing is out), the resulting is a triangle soup (so probably not useful for some geometric operations that require water-tight meshes without post-processing, etc.)

Right now this looks a bit too "magic-wandish".

If you want to provide some text, I'll paste it in. For my own part, the PR2 looks great in drake_visualizer, and I have no idea what to say about the topics you allude to.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If you want to provide some text, I'll paste it in. For my own part, the PR2 looks great in drake_visualizer, and I have no idea what to say about the topics you allude to.

Maybe the question to focus on is: If a user knows they have an stl, and knows they want an obj, in what way will the output of this tool not be what they want? I don't immediately see how they would get into trouble.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Maybe the question to focus on is: If a user knows they have an stl, and knows they want an obj, in what way will the output of this tool not be what they want? I don't immediately see how they would get into trouble.

In that context, are you assuming they understand what it means to have an STL? Let me try some concrete examples:

If the user had an STL file of a convex shape, attempted to pass it to Drake as a Convex shape, in the near future, the fact that it isn't a water tight representation will cause an exception to be thrown (despite the fact that the geometry is, in some sense, equivalent to a watertight mesh).

Another, currently, they color the stl by providing diffuse information in the urdf/sdf. If they attempted to assign a texture instead of an rgba value, the texture would not appear in rendering.

Using such an obj for hydroelastic contact (rigid surface) would probably work. I'd have to ponder that. The triangle-soup-iness definitely has some undesirable implications, but, it may be that those implications disappear into the noise for a typical user.

So, you decide what the significance of those kinds of things are. If you don't feel it's worth some educational text, we'll go as is. If you think it is, I'll write up some text emphasizing the difference between STL and OBJ and how those differences might affect downstream Drake OBJ consumers.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri and @SeanCurtis-TRI)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

In that context, are you assuming they understand what it means to have an STL? Let me try some concrete examples:

If the user had an STL file of a convex shape, attempted to pass it to Drake as a Convex shape, in the near future, the fact that it isn't a water tight representation will cause an exception to be thrown (despite the fact that the geometry is, in some sense, equivalent to a watertight mesh).

Another, currently, they color the stl by providing diffuse information in the urdf/sdf. If they attempted to assign a texture instead of an rgba value, the texture would not appear in rendering.

Using such an obj for hydroelastic contact (rigid surface) would probably work. I'd have to ponder that. The triangle-soup-iness definitely has some undesirable implications, but, it may be that those implications disappear into the noise for a typical user.

So, you decide what the significance of those kinds of things are. If you don't feel it's worth some educational text, we'll go as is. If you think it is, I'll write up some text emphasizing the difference between STL and OBJ and how those differences might affect downstream Drake OBJ consumers.

@SeanCurtis-TRI from reading the vtk api docs for the STL reader it looked to me that they attempt to combine vertices by default. In that case the obj wouldn't be a triangle soup, right?

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri and @sherm1)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):
Good catch. I glanced at the documentation here:

https://vtk.org/doc/nightly/html/classvtkSTLReader.html#details

I stopped reading at:

By setting the Merging boolean you can control whether the point data is merged after reading.

Of course, the very next sentence is, "Merging is performed by default." :-/ That's some inconsistent rhetoric.

So, soup is probably not a problem. But texture coordinates is definitely out.

Convert the pr2 example to use stl2obj.  Previously, we had both copies
of the meshes in git.  Now, we keep only the upstream stl meshes in git
and we create the obj meshes during the build.

As a side-effect, we now create and install kinect_mount.obj, whereas
previously it was only installed in *.stl form.
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Good catch. I glanced at the documentation here:

https://vtk.org/doc/nightly/html/classvtkSTLReader.html#details

I stopped reading at:

By setting the Merging boolean you can control whether the point data is merged after reading.

Of course, the very next sentence is, "Merging is performed by default." :-/ That's some inconsistent rhetoric.

So, soup is probably not a problem. But texture coordinates is definitely out.

Done.

The relevant issue wrt this tool seems to be the user blithely assuming that the conversion will be satisfactory, vs the guts of the tool which just delegate all responsibility to VTK. I think a reminder of that (added here as a NOTE) should be a sufficient tickle to make them think twice.

@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for platform review per schedule (tomorrow), please.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform) (waiting on @rpoyner-tri)


manipulation/util/stl2obj.cc, line 8 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done.

The relevant issue wrt this tool seems to be the user blithely assuming that the conversion will be satisfactory, vs the guts of the tool which just delegate all responsibility to VTK. I think a reminder of that (added here as a NOTE) should be a sufficient tickle to make them think twice.

Perfect

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 42 of 43 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants