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

Use assimp to load STL files #1365

Merged
merged 10 commits into from
Aug 31, 2019

Conversation

simonschmeisser
Copy link
Contributor

@simonschmeisser simonschmeisser commented Apr 6, 2019

Description

rviz currently uses home grown code for loading STL files which does not support ASCII format but only binary format. As discussed in #1364 this is probably for historical reasons, ie stl_loader predates usage of assimp for loading other file formats (most notably collada dae)

This PR removes STL_loader and uses assimp instead.

Remaining issues

  • Loads a sample setup from ros-i/fanuc
  • assumes all files to be collada and tries to read scaling info from stl
    [ERROR] [1554565767.625447979]: XML parse error [package://fanuc_cr7ia_support/meshes/cr7ia/visual/base_link.stl]: XML_ERROR_PARSING_TEXT
  • Unit test fails to load stl files, most likely due to incomplete setup
  • test for regressions on a subset of ros-i meshes
  • discuss regression of last sample file (file contains more binary data than specified in header)

Simon Schmeisser added 2 commits April 6, 2019 18:01
exchanging absolute paths to 'package://rviz/...' allows loading files. There still seem to be some initialization/cleanup issues
@simonschmeisser
Copy link
Contributor Author

Quick note on the ascii test file included with rviz:
if contains end loop and end facet while typical files and the sample at wikipedia have endloop and endfacet (assimp seems to start a new facet instead of ending the current at the space between end and facet)

shows one regression of this code, last test file now fails to load but did not before.
@simonschmeisser
Copy link
Contributor Author

@rhaschke I had another look at this testcase and there seems to be no way to run this truly headless (unset DISPLAY). What works however is running it with
xvfb-run make run_tests_rviz_gtest_stl_loader_test Do you think this is a viable approach and if so do you know how to combine it with moveit_ci? I see two options, either telling catkin_add_gtest somehow to prefix xvfb-run to just this test or running the whole test within xvfb-run. (Starting Xvfb as a background process unfortunately lead to a segfault when running the test for me)

@simonschmeisser simonschmeisser marked this pull request as ready for review May 19, 2019 21:26
@rhaschke
Copy link
Contributor

The test fails on the ROS build farm, which indeed doesn't provide a (virtual) X server. moveit_ci in Travis does. What we do in MoveIt in such a case is disabling the test, see here.
I'm puzzled though, why this PR is not tested on Travis as well.

@simonschmeisser
Copy link
Contributor Author

I had the "WIP" set until a moment ago, maybe that stops travis from running

ogre cannot be initialized in a headless mode, therefore disable this
test on the ros build farm. It will still be run on travis/moveit_ci where
Xvfb is running
@rhaschke
Copy link
Contributor

I had the "WIP" set until a moment ago, maybe that stops Travis from running.

Interesting. Looks like this disables Travis indeed. Didn't know so far...

As your PR changes ABI, we can only schedule it for Noetic.

@simonschmeisser
Copy link
Contributor Author

Thanks for your hint for Xvfb!

I fixed ABI compatibility by re-adding the stl_loader class. I guess I should add a deprecated of some sort to it? Will this "dead-code" then be removed somehow?

I re-added the files using git checkout HEAD~4 filename, does this preserve history (after squashing)?

@rhaschke
Copy link
Contributor

Yes, please mark the class (or its constructor(s)) as deprecated with a TODO comment when to remove it (in Noetic). Squashing creates a new commit based on the actual remaining differences, so yes, history will be preserved.

g++ 7.4 does not accept a description string unfortunately
@simonschmeisser
Copy link
Contributor Author

I added a deprecation note and comment. Please note that this will be quite noisy without #1367

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

In general looks good to me. @wjwwood, I would like to have your opinion as well before merging.

src/test/stl_loader_test.cpp Outdated Show resolved Hide resolved
src/test/stl_loader_test.cpp Show resolved Hide resolved
This is intended to work on Collada (XML) files only.
All other mesh files loaded via assimp would cause console noise.
@simonschmeisser
Copy link
Contributor Author

thanks for merging @rhaschke

@rhaschke rhaschke mentioned this pull request Sep 1, 2019
rhaschke added a commit to rhaschke/rviz that referenced this pull request Sep 8, 2019
rhaschke added a commit to rhaschke/rviz that referenced this pull request Sep 9, 2019
rhaschke added a commit to rhaschke/rviz that referenced this pull request Sep 9, 2019
rhaschke added a commit to rhaschke/rviz that referenced this pull request Sep 10, 2019
rhaschke added a commit to rhaschke/rviz that referenced this pull request Sep 20, 2019
rhaschke added a commit to rhaschke/moveit_resources that referenced this pull request Oct 27, 2019
Since ros-visualization/rviz#1365, link 4 is shown black only.
Just loading and re-saving the STL with meshlab fixed the problem.
@simonschmeisser simonschmeisser deleted the assimp_for_stl branch March 18, 2021 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants