Use assimp to load stl #1063
Use assimp to load stl #1063
Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
There was a problem hiding this comment.
I only have one concern with this PR, which is whether all of the STL files we used to be able to load still load after this. There are like 600 in the Rolling sources right now, which is obviously too many to test by hand.
I'm not quite sure how to test this; @ahcorde what's your feeling here?
This behaviour is already working on ROS 1. Just to avoid discrepancies between them when porting to ROS 2. |
|
friendly ping @clalancette |
There was a problem hiding this comment.
I've left a few minor comments on things to cleanup.
I have to say that while I think this is the correct thing to do, I'm a little nervous about it. It seems like we'll be rejecting more STL files now. But I guess the best we can do is try it out and see what happens.
| /// Load a "potentially" valid STL binary file with bigger size than the | ||
| /// expected. The extra "unexpected" data at the end of the file should be | ||
| /// ignored. |
There was a problem hiding this comment.
I think we should remove this comment now, right? That is, we will not succeed on these STL files anymore, if I understand the change.
| @@ -126,6 +125,13 @@ TEST_F(MeshLoaderTestFixture, loading_invalid_stl_files_fail) { | |||
| ASSERT_FALSE(rviz_rendering::loadMeshFromResource(mesh_path)); | |||
| } | |||
|
|
|||
| TEST_F(MeshLoaderTestFixture, loading_valid_ascii_stl_file) { | |||
There was a problem hiding this comment.
Isn't this actually loading an "invalid" ascii STL file now? That is, we are changing this from valid -> invalid, so we should rename this (and probably the filename as well).
| @@ -135,13 +141,13 @@ TEST_F(MeshLoaderTestFixture, loading_invalid_stl_files_should_fail) { | |||
| ASSERT_FALSE(rviz_rendering::loadMeshFromResource(mesh_path)); | |||
| } | |||
|
|
|||
| TEST_F(MeshLoaderTestFixture, loading_almost_valid_stl_files_should_succed) { | |||
| TEST_F(MeshLoaderTestFixture, loading_almost_valid_stl_files_should_fail) { | |||
| /// Load a "potentially" valid STL binary file with bigger size than the | |||
| /// expected. The extra "unexpected" data at the end of the file should be | |||
| /// ignored. | |||
| std::string mesh_path = "package://rviz_rendering_tests/test_meshes/valid_extra.stl"; | |||
There was a problem hiding this comment.
Same comment here; I think we should probably rename this to invalid_extra.stl.
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
We are rejecting files which are not well-formed and this is the current behaviour of ROS 1. |
|
|
delete-merged-branch
bot
deleted the
ahcorde/rolling/assimp_to_load_stl
branch
Related with this issue #712, this is the current behaviour of the ROS 1 version https://github.com/ros-visualization/rviz/pull/1365/files#diff-931ce7a05c370df33e2308f9adb590d3bde08241cf0e03dc86037731e3a5d83f