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

fix material handling #1294

Merged

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Oct 4, 2018

This attempts to resolve #1222. As noted in #1222 (comment), I was able to track down the "memory leak" to material handling of robot links. Each time, a new rviz::RobotLink was created, the corresponding link material(s) were registered again with the Ogre::MaterialManager and never released until rviz finished.

This PR addresses the issue by essentially rewriting material handling of robot links: Instead of registering materials with the MaterialManager, they are directly created using new Ogre::Material(). This avoids both the unique naming problem, and the memory leak. However, the Ogre documentation explicitly suggests to use MaterialManager::create() instead of using the Material constructor. You need to decide whether you accept this.
If not, it is important to also remove the material(s) from MaterialManager in the RobotLink destructor, e.g. by calling MaterialManager::remove(material->getHandle()). However, I consider this overkill, because the material is explicitly not reused from the MaterialManager.

I also reworked #1079: Instead of searching the corresponding material by the passed name, I directly pass the urdf::MaterialSharedPtr. This ensures that the correct material is used, even when two different materials are identically named.

}
else
{
// Need to clone here due to how selection works. Once selection id is done per object and not per material,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wjwwood I'm not sure whether this code is already obsolete, because selection handling was changed.
Anyway, removing this code, I didn't observed any issues in selecting links. However, probably you better know, what to test...

@wxmerkt
Copy link
Contributor

wxmerkt commented Jan 10, 2019

Ping - is this PR ready for merging or what is blocking it? We'd really love to see this fix make its way into rviz :)

@wxmerkt
Copy link
Contributor

wxmerkt commented Jan 17, 2019

Hi @rhaschke,
I am extensively testing this bug fix and it indeed fixed the memory leak - CPU is still at 100% when rendering though while only achieving ~17fps.

However, when I sent invalid data (from a failed optimisation) rviz is now prone to crash with:

rviz: /build/ogre-1.9-B6QkmW/ogre-1.9-1.9.0+dfsg1/OgreMain/include/OgreAxisAlignedBox.h:252: void Ogre::AxisAlignedBox::setExtents(const Ogre::Vector3&, const Ogre::Vector3&): Assertion `(min.x <= max.x && min.y <= max.y && min.z <= max.z) && "The minimum corner of the box must be less than or equal to maximum corner"' failed.
[1]    12038 abort (core dumped)  rosrun rviz rviz

This does not happen with the same erroneous data using rviz from binaries.
Edit: It does also happen with system-binary rviz

Best,
Wolfgang

... and thus the need for boost::signals(1)
which was removed from boost in version 1.69
- package.xml, format 2
- duplicate find_package
- missing package dependencies, particularly rosunit + rostest
As materials are not reused, there is no need (or use) to handle them with the MaterialManager.
Using the MaterialManager actually causes a memory leak, as created materials will always be kept by
the resource manager.
In urdf::Link, only the first visual's material comprises actual color values, all others only have the name.
Thus to correctly show the color of all visuals, we need to search for the first visual with given material name,
much like ros-visualization#1079 has done. If the urdfdom fix (ros/urdfdom#114) is released,
this work-around can be reverted.
@ghost ghost assigned rhaschke Feb 17, 2019
@rhaschke rhaschke merged commit 83ac836 into ros-visualization:melodic-devel Feb 17, 2019
@ghost ghost removed the in progress label Feb 17, 2019
@rhaschke rhaschke deleted the fix-material-handling branch February 17, 2019 20:12
@aPonza aPonza mentioned this pull request Jul 15, 2019
rhaschke added a commit to rhaschke/rviz that referenced this pull request Jul 15, 2019
Fixes an issue introduced in ros-visualization#1294 (e906f82):
Materials of Collada mesh models need to be cloned for each instance of a robot link.
rhaschke added a commit that referenced this pull request Jul 16, 2019
Fixes an issue introduced in #1294 (e906f82):
Materials of Collada mesh models need to be cloned for each instance of a robot link.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

meshes of RobotLinks are not cached and reused?
2 participants