Clear root transformation on imported Collada meshes. #52

Merged
merged 3 commits into from Sep 19, 2016

Conversation

Projects
None yet
5 participants
@de-vri-es
Member

de-vri-es commented Aug 25, 2016

Assimp enforces a Y_UP convention when importing file formats that store this metadata, such as the collada format with the <up_axis> tag. If an imported model is using a different convention such as Z_UP, the whole model is rotated until the Y axis of the model is pointing up [1]. This is both confusing and not in-line with the ROS convention where the Z axis is pointing up.

Even worse, it causes collision checks on most collada models to use this wrongly rotated model. Rviz is already undoing this rotation[2], so the collision mesh used by moveit doesn't even match the collision mesh as visualized by rviz.

It is conceivable though that the root node transformation may be used for legit purposes with other file types, so this PR limits itself to files with dae or zae extension.

I made a small test case which publishes three meshes as markers [3]: a collada model with Z_UP, one with Y_UP and an stl model. Without this patch the Z_UP model is rotated, with the patch all three models are the same and the metadata is basically ignored.

Another option would be to assume Y is pointing up after import and rotate the model to make Z point up again. This would be in line with the ROS convention. However, this would still leave a mismatch with rviz, and frankly I think that Assimp adjusting the model orientation based on this metadata is misguided in the first place. I think that what was X in the CAD program or 3D modelling software should remain X after importing.

[1] https://github.com/assimp/assimp/blob/23baecaff30b7a280648834368b1c0c792e4092e/code/ColladaLoader.cpp#L192
[2] https://github.com/ros-visualization/rviz/blob/0d423d6249a39f7f3ddd591007952c6dc940b6ac/src/rviz/mesh_loader.cpp#L232
[3] https://github.com/de-vri-es/test_load_mesh

@de-vri-es de-vri-es changed the title from Clear root transformation on imported Assimp models for Collada models. to Clear root transformation on imported Collada models. Aug 25, 2016

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 25, 2016

Member

Not confirmed yet, but I think this moveit issue would be solved by this PR: ros-planning/moveit#159

Member

de-vri-es commented Aug 25, 2016

Not confirmed yet, but I think this moveit issue would be solved by this PR: ros-planning/moveit#159

@de-vri-es de-vri-es changed the title from Clear root transformation on imported Collada models. to Clear root transformation on imported Collada meshes. Aug 25, 2016

@de-vri-es de-vri-es referenced this pull request in ros-planning/moveit Aug 25, 2016

Closed

Collision checking ignores some collada meshes #159

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 26, 2016

Member

Wow, dae loading is badly fubar...

In my opinion our code has to load all models the same way RViz does.
People use RViz for the validation of their URDFs, so this is what they expect.
If this request achieves this +1 to the idea, I don't know enough about asset loading
to review the request though.

Member

v4hn commented Aug 26, 2016

Wow, dae loading is badly fubar...

In my opinion our code has to load all models the same way RViz does.
People use RViz for the validation of their URDFs, so this is what they expect.
If this request achieves this +1 to the idea, I don't know enough about asset loading
to review the request though.

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

Fair enough, I agree that what rviz shows should be what moveit and others use internally as well. That basically means removing the if (hint == ...).

It also means tracking any changes made by rviz in the future, although it is not likely that rviz will change this in the near future. If they do we'll have a pretty nasty backwards compatibility problem.

Member

de-vri-es commented Aug 26, 2016

Fair enough, I agree that what rviz shows should be what moveit and others use internally as well. That basically means removing the if (hint == ...).

It also means tracking any changes made by rviz in the future, although it is not likely that rviz will change this in the near future. If they do we'll have a pretty nasty backwards compatibility problem.

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

Updated to unconditionally clear the root node transformation, same as rviz.

Member

de-vri-es commented Aug 26, 2016

Updated to unconditionally clear the root node transformation, same as rviz.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 26, 2016

Member

On Fri, Aug 26, 2016 at 03:37:58AM -0700, Maarten de Vries wrote:

Updated to unconditionally clear the root node transformation, same as rviz.

Alternatively you are surely very welcome to fix this in RViz kinetic and then update it here. :-)

Member

v4hn commented Aug 26, 2016

On Fri, Aug 26, 2016 at 03:37:58AM -0700, Maarten de Vries wrote:

Updated to unconditionally clear the root node transformation, same as rviz.

Alternatively you are surely very welcome to fix this in RViz kinetic and then update it here. :-)

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

I'm afraid that what rviz does is probably the best solution. As far as I can tell the root node transformation is in practise only used for things like "fixing" the orientation and "correcting" for unit scale. However, all of those adjustments mean that 1 unit along the X, Y or Z axes of the 3D modelling software is not the same as 1 unit along the same axes after importing. I would say that undoing these "corrections" is best.

The only problem is that there are no guarantees that the root node transformation is only used for these metadata adjustments. I don't know if there are importers now that use the root node transformation for something we'd want to keep, let alone what importers may do in the future.

Still, the current situation has to be fixed, and I don't see a better solution than simply clearing the root node transformation.

A long term solution should be implemented in Assimp, possibly in the form of a flag to disable the use of this type of metadata. I can poke around there, but at best it will be a while before that is universally available.

Member

de-vri-es commented Aug 26, 2016

I'm afraid that what rviz does is probably the best solution. As far as I can tell the root node transformation is in practise only used for things like "fixing" the orientation and "correcting" for unit scale. However, all of those adjustments mean that 1 unit along the X, Y or Z axes of the 3D modelling software is not the same as 1 unit along the same axes after importing. I would say that undoing these "corrections" is best.

The only problem is that there are no guarantees that the root node transformation is only used for these metadata adjustments. I don't know if there are importers now that use the root node transformation for something we'd want to keep, let alone what importers may do in the future.

Still, the current situation has to be fixed, and I don't see a better solution than simply clearing the root node transformation.

A long term solution should be implemented in Assimp, possibly in the form of a flag to disable the use of this type of metadata. I can poke around there, but at best it will be a while before that is universally available.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 26, 2016

Member

Again, I can't review this properly, but could you please add a comment saying that this behavior is supposes to match RViz?

Member

v4hn commented Aug 26, 2016

Again, I can't review this properly, but could you please add a comment saying that this behavior is supposes to match RViz?

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

Comment added.

Also, because a picture is worth a thousand words:

Unpatched:
collada_unpatched_geometric_shapes

Patched:
collada_patched_geometric_shapes

Both images generated with the test case at [1]. The first one without this PR, the second with. The three shapes are all the same model, the first two collada with Z_UP and Y_UP respectively, the third one the same model as STL.

[1] https://github.com/de-vri-es/test_load_mesh

Member

de-vri-es commented Aug 26, 2016

Comment added.

Also, because a picture is worth a thousand words:

Unpatched:
collada_unpatched_geometric_shapes

Patched:
collada_patched_geometric_shapes

Both images generated with the test case at [1]. The first one without this PR, the second with. The three shapes are all the same model, the first two collada with Z_UP and Y_UP respectively, the third one the same model as STL.

[1] https://github.com/de-vri-es/test_load_mesh

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 26, 2016

Member

This could potentially break applications that have worked around this bug by a hacked transform flip, correct? So we are only targeting this for kinetic and maybe jade?

Perhaps @wjwwood, maintainer of Rviz, could weight in his opinion.

I love the pictures!

You mentioned a small test case - think you could turn that into a unit test in this PR?

Thanks for looking into this!

Member

davetcoleman commented Aug 26, 2016

This could potentially break applications that have worked around this bug by a hacked transform flip, correct? So we are only targeting this for kinetic and maybe jade?

Perhaps @wjwwood, maintainer of Rviz, could weight in his opinion.

I love the pictures!

You mentioned a small test case - think you could turn that into a unit test in this PR?

Thanks for looking into this!

@@ -243,14 +243,23 @@ Mesh* createMeshFromBinary(const char *buffer, std::size_t size, const Eigen::Ve
const aiScene* scene = importer.ReadFileFromMemory(reinterpret_cast<const void*>(buffer), size,
aiProcess_Triangulate |
aiProcess_JoinIdenticalVertices |
- aiProcess_SortByPType |
- aiProcess_OptimizeGraph |
- aiProcess_OptimizeMeshes,

This comment has been minimized.

@davetcoleman

davetcoleman Aug 26, 2016

Member

why remove these optimizations? is this the part that rotates the axis?

@davetcoleman

davetcoleman Aug 26, 2016

Member

why remove these optimizations? is this the part that rotates the axis?

This comment has been minimized.

@de-vri-es

de-vri-es Aug 26, 2016

Member

They're not removed, just moved down later. Those two optimizations heavily optimize the imported meshes and scene graph. One of the results was that the root node transformation got flattened into child nodes. That made clearing the root node transformation a NOP, but clearing the child node transformations is not an option since it would modify the mesh.

But the optimizations are still performed after clearing the root node transform here: https://github.com/ros-planning/geometric_shapes/pull/52/files#diff-f5ec0f576aaf3b44a4c42c739e51980dR260

@de-vri-es

de-vri-es Aug 26, 2016

Member

They're not removed, just moved down later. Those two optimizations heavily optimize the imported meshes and scene graph. One of the results was that the root node transformation got flattened into child nodes. That made clearing the root node transformation a NOP, but clearing the child node transformations is not an option since it would modify the mesh.

But the optimizations are still performed after clearing the root node transform here: https://github.com/ros-planning/geometric_shapes/pull/52/files#diff-f5ec0f576aaf3b44a4c42c739e51980dR260

This comment has been minimized.

@davetcoleman

davetcoleman Aug 26, 2016

Member

got it

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

This could potentially break applications that have worked around this bug by a hacked transform flip, correct? So we are only targeting this for kinetic and maybe jade?

I don't think any program could have worked around it reliably after the mesh left this function. You need knowledge about the loaded file to undo the effect: best would be the root node transformation or otherwise you should know if the imported file was a collada model with something other than <up_axis>Y_UP</up_axis>.

That does leave the possibility that someone worked around it unreliably, although I think most people would have just rotated the mesh and/or export as STL rather than apply a rotation in code. Still, it is worth considering. In the strictest sense it is indeed a backward incompatible change.

You mentioned a small test case - think you could turn that into a unit test in this PR?

+1. The test case so far was geared for visual inspection, but it is possible to make a unit test for this with simpler meshes. Considering the importance of correctly importing meshes for collision models and the possibility that Assimp might change the behaviour in the future, I agree that a unit test is a very good idea. I'll start work on one :)

Member

de-vri-es commented Aug 26, 2016

This could potentially break applications that have worked around this bug by a hacked transform flip, correct? So we are only targeting this for kinetic and maybe jade?

I don't think any program could have worked around it reliably after the mesh left this function. You need knowledge about the loaded file to undo the effect: best would be the root node transformation or otherwise you should know if the imported file was a collada model with something other than <up_axis>Y_UP</up_axis>.

That does leave the possibility that someone worked around it unreliably, although I think most people would have just rotated the mesh and/or export as STL rather than apply a rotation in code. Still, it is worth considering. In the strictest sense it is indeed a backward incompatible change.

You mentioned a small test case - think you could turn that into a unit test in this PR?

+1. The test case so far was geared for visual inspection, but it is possible to make a unit test for this with simpler meshes. Considering the importance of correctly importing meshes for collision models and the possibility that Assimp might change the behaviour in the future, I agree that a unit test is a very good idea. I'll start work on one :)

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

That does leave the possibility that someone worked around it unreliably, although I think most people would have just rotated the mesh and/or export as STL rather than apply a rotation in code. Still, it is worth considering. In the strictest sense it is indeed a backward incompatible change.

I just realised that the workaround of rotating the collada mesh will still be broken by this change. They would have had to notice the wrong orientation though, which still wasn't very easy considering RViz showed the expected collision model.

Member

de-vri-es commented Aug 26, 2016

That does leave the possibility that someone worked around it unreliably, although I think most people would have just rotated the mesh and/or export as STL rather than apply a rotation in code. Still, it is worth considering. In the strictest sense it is indeed a backward incompatible change.

I just realised that the workaround of rotating the collada mesh will still be broken by this change. They would have had to notice the wrong orientation though, which still wasn't very easy considering RViz showed the expected collision model.

@mikeferguson

This comment has been minimized.

Show comment
Hide comment
@mikeferguson

mikeferguson Aug 26, 2016

Member

@de-vri-es Was there previously any way to actually visualize the mesh when using a workaround? Presumably all the RVIZ plugins showed the incorrect rotation, right? My expectation is that people have probably fixed their meshes to work around the bug?

Member

mikeferguson commented Aug 26, 2016

@de-vri-es Was there previously any way to actually visualize the mesh when using a workaround? Presumably all the RVIZ plugins showed the incorrect rotation, right? My expectation is that people have probably fixed their meshes to work around the bug?

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

@de-vri-es Was there previously any way to actually visualize the mesh when using a workaround? Presumably all the RVIZ plugins showed the incorrect rotation, right? My expectation is that people have probably fixed their meshes to work around the bug?

I believe the RViz plugins actually show the correct rotation. They don't seem to be using the mesh as imported by geometric_shapes. To get the wrong rotation visible, the mesh would have to have been converted to a marker and send to RViz, as my test case does.

/update: Although I must add that I haven't used the rviz plugins in a while.

Member

de-vri-es commented Aug 26, 2016

@de-vri-es Was there previously any way to actually visualize the mesh when using a workaround? Presumably all the RVIZ plugins showed the incorrect rotation, right? My expectation is that people have probably fixed their meshes to work around the bug?

I believe the RViz plugins actually show the correct rotation. They don't seem to be using the mesh as imported by geometric_shapes. To get the wrong rotation visible, the mesh would have to have been converted to a marker and send to RViz, as my test case does.

/update: Although I must add that I haven't used the rviz plugins in a while.

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 26, 2016

Member

To clarify: the moveit rviz plugins seem to use rviz::Robot as a base for visualizing robots, which uses RViz's method of loading meshes.

Member

de-vri-es commented Aug 26, 2016

To clarify: the moveit rviz plugins seem to use rviz::Robot as a base for visualizing robots, which uses RViz's method of loading meshes.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 26, 2016

Member

On Fri, Aug 26, 2016 at 11:12:15AM -0700, Dave Coleman wrote:

This could potentially break applications that have worked around this bug by a hacked transform flip, correct?
So we are only targeting this for kinetic and maybe jade?

This is clearly a bug fix and probably even @hersh tripped over this without understanding the problem.
So if someone analyzed this, and worked around it and never even cared to report it, it's definitely their very own fault.

Bug Fixes should never not be merged because someone might have worked around the broken behavior.

Member

v4hn commented Aug 26, 2016

On Fri, Aug 26, 2016 at 11:12:15AM -0700, Dave Coleman wrote:

This could potentially break applications that have worked around this bug by a hacked transform flip, correct?
So we are only targeting this for kinetic and maybe jade?

This is clearly a bug fix and probably even @hersh tripped over this without understanding the problem.
So if someone analyzed this, and worked around it and never even cared to report it, it's definitely their very own fault.

Bug Fixes should never not be merged because someone might have worked around the broken behavior.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 26, 2016

Member

That does leave the possibility that someone worked around it unreliably

I probably have done this in the past, but its no problem to undo the hack

Bug Fixes should never not be merged because someone might have worked around the broken behavior.

I wouldn't say never, and I think there's lots of examples of the contrary, but in this case I agree

Member

davetcoleman commented Aug 26, 2016

That does leave the possibility that someone worked around it unreliably

I probably have done this in the past, but its no problem to undo the hack

Bug Fixes should never not be merged because someone might have worked around the broken behavior.

I wouldn't say never, and I think there's lots of examples of the contrary, but in this case I agree

@mikeferguson

This comment has been minimized.

Show comment
Hide comment
@mikeferguson

mikeferguson Aug 26, 2016

Member

My suggestion would be to make sure we announce it on the moveit mailing list that we have fixed/merged this -- and since I/J syncs just happened yesterday, we will have significant soak time for anyone to test, catch, and fix regressions that might occur with their robots before it goes live to the world.

Member

mikeferguson commented Aug 26, 2016

My suggestion would be to make sure we announce it on the moveit mailing list that we have fixed/merged this -- and since I/J syncs just happened yesterday, we will have significant soak time for anyone to test, catch, and fix regressions that might occur with their robots before it goes live to the world.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 26, 2016

Member

I wouldn't say never, and I think there's lots of examples of the contrary, but in this case I agree

Citing the by now classic Ulrich Drepper's "Good Practices in Library Design":

One sort of changes which should be made without hesitation are bug fixes. They
should be made even if the API is changed. Some people argue that some programs are
depending on the old, broken behavior. But this argumentation is flawed. Equally well
there are programs which depend on the correct behavior, written by people who read
the specification. No program which depends on broken behavior deserves protection.
If a programmer has found such a discrepancy and changed the application to use it
instead of reporting it to the library maintainer s/he gets what s/he deserves.

I fully agree with him.

My suggestion would be to make sure we announce it on the moveit mailing list

Definitely!

Member

v4hn commented Aug 26, 2016

I wouldn't say never, and I think there's lots of examples of the contrary, but in this case I agree

Citing the by now classic Ulrich Drepper's "Good Practices in Library Design":

One sort of changes which should be made without hesitation are bug fixes. They
should be made even if the API is changed. Some people argue that some programs are
depending on the old, broken behavior. But this argumentation is flawed. Equally well
there are programs which depend on the correct behavior, written by people who read
the specification. No program which depends on broken behavior deserves protection.
If a programmer has found such a discrepancy and changed the application to use it
instead of reporting it to the library maintainer s/he gets what s/he deserves.

I fully agree with him.

My suggestion would be to make sure we announce it on the moveit mailing list

Definitely!

test/test_create_mesh.cpp
+
+namespace {
+
+std::string makeDaeModel(std::string const & up_axis) {

This comment has been minimized.

@de-vri-es

de-vri-es Aug 28, 2016

Member

Is this an accepted method to embed text files into tests, or should I put it in a file loaded at runtime?

I like how this is self-contained without any runtime dependencies on the filesystem or environment, but at the same time it isn't the prettiest in code.

/edit: Best to look at the full changes, otherwise you only see the function signature.

@de-vri-es

de-vri-es Aug 28, 2016

Member

Is this an accepted method to embed text files into tests, or should I put it in a file loaded at runtime?

I like how this is self-contained without any runtime dependencies on the filesystem or environment, but at the same time it isn't the prettiest in code.

/edit: Best to look at the full changes, otherwise you only see the function signature.

This comment has been minimized.

@v4hn

v4hn Aug 29, 2016

Member

On Sun, Aug 28, 2016 at 01:48:27PM -0700, Maarten de Vries wrote:

Is this an accepted method to embed text files into tests, or should I put it in a file loaded at runtime?

This makes it quite hard to load the same dae file with a different tool, right..
I would prefer a solution where the test files are separate and complete (i.e. with a specified direction).

@v4hn

v4hn Aug 29, 2016

Member

On Sun, Aug 28, 2016 at 01:48:27PM -0700, Maarten de Vries wrote:

Is this an accepted method to embed text files into tests, or should I put it in a file loaded at runtime?

This makes it quite hard to load the same dae file with a different tool, right..
I would prefer a solution where the test files are separate and complete (i.e. with a specified direction).

This comment has been minimized.

@de-vri-es

de-vri-es Aug 29, 2016

Member

True. What is the preferred method of making the file available at runtime then? Use a package:// URL (already supported by geometric_shapes, so no new dependency) and require the environment to be set up with a proper ROS_PACKAGE_PATH? I imagine that the run_tests target of the generated Makefile already sets up the environment correctly?

Alternatively the file can exist in the source tree and be linked in to the executable as a binary blob, but I imagine this is less standard for ROS packages.

@de-vri-es

de-vri-es Aug 29, 2016

Member

True. What is the preferred method of making the file available at runtime then? Use a package:// URL (already supported by geometric_shapes, so no new dependency) and require the environment to be set up with a proper ROS_PACKAGE_PATH? I imagine that the run_tests target of the generated Makefile already sets up the environment correctly?

Alternatively the file can exist in the source tree and be linked in to the executable as a binary blob, but I imagine this is less standard for ROS packages.

This comment has been minimized.

@v4hn

v4hn Aug 29, 2016

Member

I believe @rhaschke recently worked with external files & unit tests. Could you please comment?

@v4hn

v4hn Aug 29, 2016

Member

I believe @rhaschke recently worked with external files & unit tests. Could you please comment?

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 28, 2016

Member

I added a unit test to verify loading a simple mesh (see also the line comment regarding embedding the mesh).

One more important thing to note about this PR: it also undoes scaling from the <unit> metadata tag. Again, this is the same thing RViz does, and I think this behaviour matches expectations best, but it is an important detail nonetheless.

For completeness the unit test should also test STL, and verify that the <unit> tag is indeed ignored. I will add those later, but I would like feedback on the line comment :)

Member

de-vri-es commented Aug 28, 2016

I added a unit test to verify loading a simple mesh (see also the line comment regarding embedding the mesh).

One more important thing to note about this PR: it also undoes scaling from the <unit> metadata tag. Again, this is the same thing RViz does, and I think this behaviour matches expectations best, but it is an important detail nonetheless.

For completeness the unit test should also test STL, and verify that the <unit> tag is indeed ignored. I will add those later, but I would like feedback on the line comment :)

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Aug 31, 2016

Member

Updated tests to use files and the already available #include "resources/config.h". Also added an STL version of the mesh.

Member

de-vri-es commented Aug 31, 2016

Updated tests to use files and the already available #include "resources/config.h". Also added an STL version of the mesh.

test/test_create_mesh.cpp
+
+namespace {
+
+ void assertMesh(shapes::Mesh * mesh) {

This comment has been minimized.

@davetcoleman

davetcoleman Sep 8, 2016

Member

bracket on new line

@davetcoleman

davetcoleman Sep 8, 2016

Member

bracket on new line

test/test_create_mesh.cpp
+
+ shapes::Mesh * loadMesh(const std::string & path)
+ {
+ return shapes::createMeshFromResource("file://" + std::string(TEST_RESOURCES_DIR) + "/" + path);

This comment has been minimized.

@davetcoleman

davetcoleman Sep 8, 2016

Member

indent of 4 spaces...

@davetcoleman

davetcoleman Sep 8, 2016

Member

indent of 4 spaces...

This comment has been minimized.

@davetcoleman

davetcoleman Sep 8, 2016

Member

this method for loading meshes looks perfect

@davetcoleman

davetcoleman Sep 8, 2016

Member

this method for loading meshes looks perfect

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 8, 2016

Member
[==========] Running 5 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 5 tests from CreateMesh
[ RUN      ] CreateMesh.stl
[       OK ] CreateMesh.stl (1 ms)
[ RUN      ] CreateMesh.daeNoUp
[       OK ] CreateMesh.daeNoUp (0 ms)
[ RUN      ] CreateMesh.daeYUp
[       OK ] CreateMesh.daeYUp (0 ms)
[ RUN      ] CreateMesh.daeZUp
[       OK ] CreateMesh.daeZUp (1 ms)
[ RUN      ] CreateMesh.daeXUp
[       OK ] CreateMesh.daeXUp (0 ms)
[----------] 5 tests from CreateMesh (2 ms total)

Tests pass, +1

I think this should be back-ported to I/J, also

Member

davetcoleman commented Sep 8, 2016

[==========] Running 5 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 5 tests from CreateMesh
[ RUN      ] CreateMesh.stl
[       OK ] CreateMesh.stl (1 ms)
[ RUN      ] CreateMesh.daeNoUp
[       OK ] CreateMesh.daeNoUp (0 ms)
[ RUN      ] CreateMesh.daeYUp
[       OK ] CreateMesh.daeYUp (0 ms)
[ RUN      ] CreateMesh.daeZUp
[       OK ] CreateMesh.daeZUp (1 ms)
[ RUN      ] CreateMesh.daeXUp
[       OK ] CreateMesh.daeXUp (0 ms)
[----------] 5 tests from CreateMesh (2 ms total)

Tests pass, +1

I think this should be back-ported to I/J, also

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Sep 8, 2016

Member

Fixed code style issues in the test.

Member

de-vri-es commented Sep 8, 2016

Fixed code style issues in the test.

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Sep 8, 2016

Member

Also added a test to verify that <unit> tags are ignored.

Member

de-vri-es commented Sep 8, 2016

Also added a test to verify that <unit> tags are ignored.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 8, 2016

Member

Just noticed this PR. Nice catch @de-vri-es, but I'm a bit unsure about this part:

One more important thing to note about this PR: it also undoes scaling from the <unit> metadata tag. Again, this is the same thing RViz does, and I think this behaviour matches expectations best, but it is an important detail nonetheless.

Units support in Collada is something that is frequently used, and without it, meshes that haven't been stored with their scaling set to meters will not load properly any more in ROS. Even if RViz does this, is it a good idea to deliberately remove support for a format feature like this?

Undoing/ignoring the root transform seems like a dubious thing to do as well -- at least to me. It'd be deliberately introducing behaviour that is different from the spec. Doesn't the fact that RViz does it make it essentially a buggy implementation of the Collada spec? @v4hn's comment in #52 (comment) would seem to apply here as well then?

A (seemingly) related issue: ros-visualization/rviz#853. Also: Collada (.dae) meshes with unit != "meter" broken? and Did RViz become unit-aware?.

Member

gavanderhoorn commented Sep 8, 2016

Just noticed this PR. Nice catch @de-vri-es, but I'm a bit unsure about this part:

One more important thing to note about this PR: it also undoes scaling from the <unit> metadata tag. Again, this is the same thing RViz does, and I think this behaviour matches expectations best, but it is an important detail nonetheless.

Units support in Collada is something that is frequently used, and without it, meshes that haven't been stored with their scaling set to meters will not load properly any more in ROS. Even if RViz does this, is it a good idea to deliberately remove support for a format feature like this?

Undoing/ignoring the root transform seems like a dubious thing to do as well -- at least to me. It'd be deliberately introducing behaviour that is different from the spec. Doesn't the fact that RViz does it make it essentially a buggy implementation of the Collada spec? @v4hn's comment in #52 (comment) would seem to apply here as well then?

A (seemingly) related issue: ros-visualization/rviz#853. Also: Collada (.dae) meshes with unit != "meter" broken? and Did RViz become unit-aware?.

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Sep 8, 2016

Member

@gavanderhoorn: I understand your concern, and I'm also not entirely comfortable with clearing the root transform.

However, using the metadata tags only makes sense when you can specify matching parameters for the scene/world that the models are being imported in. If we have no knowledge of the scene/world we don't know how to incorporate the model metadata.

We could assume some set of standards for the scene/world: Z up and a 1 meter units. However, the 1 meter is not a official ROS standard. If I recall correctly, the advise is to use a unit such that "1" is as close as possible to size of your robot. (Actually, not any unit, but meter combined with an SI prefix.)

If no scene/world metadata is available, I do think that ignoring the metadata of the model is the best approach. Then at least a distance along an axis in the modelling software is the same distance along the same axis of the imported model.

Additionally, Assimp doesn't give us the tools required to convert to Z_UP reliably. It only transforms the model to Y_UP when a <up_axis> tag is present, and of course it leaves STL files alone. It also does not expose the loaded metadata.

We could extract the unit size from the root transform and only clear the rotation. But that does mean leaving a mismatch with RViz.

It may make more sense to first adjust geometric_shapes to match RViz and then raise this issue with RViz? On the other hand, that means multiple transitions for users of geometric_shapes if RViz decides to change.

Member

de-vri-es commented Sep 8, 2016

@gavanderhoorn: I understand your concern, and I'm also not entirely comfortable with clearing the root transform.

However, using the metadata tags only makes sense when you can specify matching parameters for the scene/world that the models are being imported in. If we have no knowledge of the scene/world we don't know how to incorporate the model metadata.

We could assume some set of standards for the scene/world: Z up and a 1 meter units. However, the 1 meter is not a official ROS standard. If I recall correctly, the advise is to use a unit such that "1" is as close as possible to size of your robot. (Actually, not any unit, but meter combined with an SI prefix.)

If no scene/world metadata is available, I do think that ignoring the metadata of the model is the best approach. Then at least a distance along an axis in the modelling software is the same distance along the same axis of the imported model.

Additionally, Assimp doesn't give us the tools required to convert to Z_UP reliably. It only transforms the model to Y_UP when a <up_axis> tag is present, and of course it leaves STL files alone. It also does not expose the loaded metadata.

We could extract the unit size from the root transform and only clear the rotation. But that does mean leaving a mismatch with RViz.

It may make more sense to first adjust geometric_shapes to match RViz and then raise this issue with RViz? On the other hand, that means multiple transitions for users of geometric_shapes if RViz decides to change.

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Sep 8, 2016

Member

On a side note: it would appear that Assimp can ignore the up direction for Collada files more cleanly by setting a property on the importer [1]:

importer->setPropertyInteger("IMPORT_COLLADA_IGNORE_UP_DIRECTION", 1)

That means we could still honor the unit tag easily (that is: even without manually decomposing the root transform and removing the rotation). I'm not convinced we should scale everything to meters though, unless there is a specification stating that meters are the units used throughout ROS?

[1] https://github.com/assimp/assimp/blob/master/code/ColladaLoader.cpp#L130

Member

de-vri-es commented Sep 8, 2016

On a side note: it would appear that Assimp can ignore the up direction for Collada files more cleanly by setting a property on the importer [1]:

importer->setPropertyInteger("IMPORT_COLLADA_IGNORE_UP_DIRECTION", 1)

That means we could still honor the unit tag easily (that is: even without manually decomposing the root transform and removing the rotation). I'm not convinced we should scale everything to meters though, unless there is a specification stating that meters are the units used throughout ROS?

[1] https://github.com/assimp/assimp/blob/master/code/ColladaLoader.cpp#L130

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 8, 2016

Member

However, using the metadata tags only makes sense when you can specify matching parameters for the scene/world that the models are being imported in. If we have no knowledge of the scene/world we don't know how to incorporate the model metadata.

We could assume some set of standards for the scene/world: Z up and a 1 meter units. However, the 1 meter is not a official ROS standard. If I recall correctly, the advise is to use a unit such that "1" is as close as possible to size of your robot.

To me, only 'meters' make sense as a scale for meshes. ROS uses meters throughout, and meters for meshes makes them align perfectly with TF and all of the other infrastructure that has a notion of distance embedded in it.

I can't find it right now, but I seem to remember that using meters for mesh scale is actually a (strong) recommendation.

We could assume some set of standards for the scene/world: Z up and a 1 meter units

Afaik -- but I could've been living in my own private ROS universe :) -- that is already at least a convention. Units, chirality and coord orientation are defined in REP-103. Perhaps not explicitly stated anywhere, but anything else in RViz, MoveIt or geometric_shapes makes little sense to me.

It may make more sense to first adjust geometric_shapes to match RViz and then raise this issue with RViz? On the other hand, that means multiple transitions for users of geometric_shapes if RViz decides to change.

Hm. Well I don't think I can properly estimate the impact of such a change, but if something like this is changed, I'd be in favour of doing it once. And doing it right.

Member

gavanderhoorn commented Sep 8, 2016

However, using the metadata tags only makes sense when you can specify matching parameters for the scene/world that the models are being imported in. If we have no knowledge of the scene/world we don't know how to incorporate the model metadata.

We could assume some set of standards for the scene/world: Z up and a 1 meter units. However, the 1 meter is not a official ROS standard. If I recall correctly, the advise is to use a unit such that "1" is as close as possible to size of your robot.

To me, only 'meters' make sense as a scale for meshes. ROS uses meters throughout, and meters for meshes makes them align perfectly with TF and all of the other infrastructure that has a notion of distance embedded in it.

I can't find it right now, but I seem to remember that using meters for mesh scale is actually a (strong) recommendation.

We could assume some set of standards for the scene/world: Z up and a 1 meter units

Afaik -- but I could've been living in my own private ROS universe :) -- that is already at least a convention. Units, chirality and coord orientation are defined in REP-103. Perhaps not explicitly stated anywhere, but anything else in RViz, MoveIt or geometric_shapes makes little sense to me.

It may make more sense to first adjust geometric_shapes to match RViz and then raise this issue with RViz? On the other hand, that means multiple transitions for users of geometric_shapes if RViz decides to change.

Hm. Well I don't think I can properly estimate the impact of such a change, but if something like this is changed, I'd be in favour of doing it once. And doing it right.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 8, 2016

Member

I'm not convinced we should scale everything to meters though, unless there is a specification stating that meters are the units used throughout ROS?

Bit 'broken record', but REP-103?

Member

gavanderhoorn commented Sep 8, 2016

I'm not convinced we should scale everything to meters though, unless there is a specification stating that meters are the units used throughout ROS?

Bit 'broken record', but REP-103?

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Sep 8, 2016

Member

Hmm, REP-103 it is only informal and not very strongly worded, but that could still be enough justification to enforce Z up and 1 meter units. Although it would be nice if it can be disabled at the geometric_shapes level if people want to deviate from Z up or meter units.

However, the information Assimp gives is not enough to enforce Z up. Apparently gazebo decided to parse the collada files themselves to extract the <up_axis> tag. Not very pretty, but it doesn't look like Assimp gives us any choice if we want to enforce any convention.

That leaves 3 options:

  1. Keep it as is, ignore both <up_axis> and <unit> metadata the way RViz currently does it.
  2. Scale to meters using the <unit> metadata, ignore <up_axis>. A bit half-assed but doesn't require parsing the Collada file.
  3. Scale to meters and enforce Z_UP. Requires parsing the Collada ourselves.

If we decide to go for 2 or 3 we should make sure to discuss with RViz so we end up with the same solution. In that case I would also like to make the use of the metadata tags optional or even configurable to other conventions in the geometric_shapes API (with default values that match Z up and meter units).

/edit: I think 2 is a pretty bad idea, btw

Member

de-vri-es commented Sep 8, 2016

Hmm, REP-103 it is only informal and not very strongly worded, but that could still be enough justification to enforce Z up and 1 meter units. Although it would be nice if it can be disabled at the geometric_shapes level if people want to deviate from Z up or meter units.

However, the information Assimp gives is not enough to enforce Z up. Apparently gazebo decided to parse the collada files themselves to extract the <up_axis> tag. Not very pretty, but it doesn't look like Assimp gives us any choice if we want to enforce any convention.

That leaves 3 options:

  1. Keep it as is, ignore both <up_axis> and <unit> metadata the way RViz currently does it.
  2. Scale to meters using the <unit> metadata, ignore <up_axis>. A bit half-assed but doesn't require parsing the Collada file.
  3. Scale to meters and enforce Z_UP. Requires parsing the Collada ourselves.

If we decide to go for 2 or 3 we should make sure to discuss with RViz so we end up with the same solution. In that case I would also like to make the use of the metadata tags optional or even configurable to other conventions in the geometric_shapes API (with default values that match Z up and meter units).

/edit: I think 2 is a pretty bad idea, btw

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 8, 2016

Member

@de-vri-es wrote:

Hmm, REP-103 it is only informal and not very strongly worded, but that could still be enough justification to enforce Z up and 1 meter units

the only thing that is not explicit enough afaict is that the same conventions should be applied to / followed for mesh data / robot geometry. I would think the Units and Coordinate Frame Conventions sections pretty much explicitly cover the rest.

Regarding your suggestions: I think the important thing would be that we communicate whatever we end up choosing. Personally, I'd like units to be respected (if at all possible, but the Gazebo devs apparently found it too confusing / they found that modelling tools aren't consistent enough with it) and the model to be rotated so that Z is up (ie: rotate everything until the mesh-local coord frame is aligned with the ROS global coord frame.

Rotating objects like that isn't even very special: iirc, there are lots of tools that do that (game engines, viewers, etc).

But I really think this is something that could benefit from some more input. @wjwwood, @tfoote: comments?

Member

gavanderhoorn commented Sep 8, 2016

@de-vri-es wrote:

Hmm, REP-103 it is only informal and not very strongly worded, but that could still be enough justification to enforce Z up and 1 meter units

the only thing that is not explicit enough afaict is that the same conventions should be applied to / followed for mesh data / robot geometry. I would think the Units and Coordinate Frame Conventions sections pretty much explicitly cover the rest.

Regarding your suggestions: I think the important thing would be that we communicate whatever we end up choosing. Personally, I'd like units to be respected (if at all possible, but the Gazebo devs apparently found it too confusing / they found that modelling tools aren't consistent enough with it) and the model to be rotated so that Z is up (ie: rotate everything until the mesh-local coord frame is aligned with the ROS global coord frame.

Rotating objects like that isn't even very special: iirc, there are lots of tools that do that (game engines, viewers, etc).

But I really think this is something that could benefit from some more input. @wjwwood, @tfoote: comments?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 8, 2016

Member

If you like to see this fixed in any way other than exactly how RViz is doing it, please file an issue/pull-request against rviz first.

It is right, that we should respect the format specs. However, it looks like RViz (and gazebo?) decided to intentionally restrict the specs to a subset of the file format.
As everyone I know who ever used ROS at some point used RViz for visualization, they have all noticed potential differences and changed their files to adhere to the restricted specs.
So, very sadly, the specs for geometric_shapes to adhere to are not so much the specs for the file format, but more the restricted implementation by RViz.
(but yeah, you pretty much got me here @gavanderhoorn -.-)

Member

v4hn commented Sep 8, 2016

If you like to see this fixed in any way other than exactly how RViz is doing it, please file an issue/pull-request against rviz first.

It is right, that we should respect the format specs. However, it looks like RViz (and gazebo?) decided to intentionally restrict the specs to a subset of the file format.
As everyone I know who ever used ROS at some point used RViz for visualization, they have all noticed potential differences and changed their files to adhere to the restricted specs.
So, very sadly, the specs for geometric_shapes to adhere to are not so much the specs for the file format, but more the restricted implementation by RViz.
(but yeah, you pretty much got me here @gavanderhoorn -.-)

@de-vri-es de-vri-es referenced this pull request in ros-visualization/rviz Sep 8, 2016

Open

Metadata from Collada files #1045

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Sep 8, 2016

Member

If you like to see this fixed in any way other than exactly how RViz is doing it, please file an issue/pull-request against rviz first.

I filed ros-visualization/rviz#1045. We should indeed match RViz, but it is worth waiting if RViz is going to change soon.

Member

de-vri-es commented Sep 8, 2016

If you like to see this fixed in any way other than exactly how RViz is doing it, please file an issue/pull-request against rviz first.

I filed ros-visualization/rviz#1045. We should indeed match RViz, but it is worth waiting if RViz is going to change soon.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 8, 2016

Member

@gavanderhoorn wrote:

Rotating objects like that isn't even very special: iirc, there are lots of tools that do that (game engines, viewers, etc).

Just as an example of this: the documentation on the Collada loader in the Torque 3D engine (from here):

Up Axis

The coordinate system in Torque 3D is equivalent to Z_UP, and the loader will automatically convert models using X_UP or Y_UP to this coordinate system. Some COLLADA exporters may generate models with a wrong or missing <up_axis> element (Z_UP is assumed if <up_axis> is not specified). In this case, the COLLADA import dialog can be used to override the value.

The same link also has a section on units.

Member

gavanderhoorn commented Sep 8, 2016

@gavanderhoorn wrote:

Rotating objects like that isn't even very special: iirc, there are lots of tools that do that (game engines, viewers, etc).

Just as an example of this: the documentation on the Collada loader in the Torque 3D engine (from here):

Up Axis

The coordinate system in Torque 3D is equivalent to Z_UP, and the loader will automatically convert models using X_UP or Y_UP to this coordinate system. Some COLLADA exporters may generate models with a wrong or missing <up_axis> element (Z_UP is assumed if <up_axis> is not specified). In this case, the COLLADA import dialog can be used to override the value.

The same link also has a section on units.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 8, 2016

Member

I agree with @v4hn we should make geometric_shapes behave exactly as Rviz. Perhaps someone more familiar with Rviz could comment on the possibility of creating a shared code path for this functionality, e.g. Rviz depend on geometric_shapes or vice versa?

I doubt we will see any big changes like this to Rviz in the near future since it has such a HUGE user base. We will probably want to merge this PR

It would also be great if @de-vri-es or @gavanderhoorn could sum up this discussion into a new tutorial on how to use meshes (scaling, orientation, file format) with MoveIt! in moveit_tutorials

Member

davetcoleman commented Sep 8, 2016

I agree with @v4hn we should make geometric_shapes behave exactly as Rviz. Perhaps someone more familiar with Rviz could comment on the possibility of creating a shared code path for this functionality, e.g. Rviz depend on geometric_shapes or vice versa?

I doubt we will see any big changes like this to Rviz in the near future since it has such a HUGE user base. We will probably want to merge this PR

It would also be great if @de-vri-es or @gavanderhoorn could sum up this discussion into a new tutorial on how to use meshes (scaling, orientation, file format) with MoveIt! in moveit_tutorials

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 16, 2016

Member

@de-vri-es I'm pretty certain this will not change in rviz kinetic anymore.
Is this request in your opinion ready to merge into i/j/k?

Member

v4hn commented Sep 16, 2016

@de-vri-es I'm pretty certain this will not change in rviz kinetic anymore.
Is this request in your opinion ready to merge into i/j/k?

@de-vri-es

This comment has been minimized.

Show comment
Hide comment
@de-vri-es

de-vri-es Sep 16, 2016

Member

You're probably right. No reply yet at ros-visualization/rviz#1045.

It is ready to merge I think. The unit tests are in place to verify the behaviour both for <up_axis> and <unit>.

Member

de-vri-es commented Sep 16, 2016

You're probably right. No reply yet at ros-visualization/rviz#1045.

It is ready to merge I think. The unit tests are in place to verify the behaviour both for <up_axis> and <unit>.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 18, 2016

Member

ok. +1 as is. This should be merged to indigo and jade too.

Member

v4hn commented Sep 18, 2016

ok. +1 as is. This should be merged to indigo and jade too.

@davetcoleman davetcoleman merged commit b072796 into ros-planning:kinetic-devel Sep 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 19, 2016

Member

thanks @de-vri-es. cherry-picked to indigo

Member

davetcoleman commented Sep 19, 2016

thanks @de-vri-es. cherry-picked to indigo

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