fix material handling #1294
Merged
fix material handling #1294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhaschke
commented
Oct 4, 2018
| } | ||
| else | ||
| { | ||
| // Need to clone here due to how selection works. Once selection id is done per object and not per material, |
There was a problem hiding this comment.
@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...
|
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 :) |
|
Hi @rhaschke,
Best, |
rhaschke
force-pushed
the
fix-material-handling
branch
from
ed3b0b3 to
d0c7fc1
Compare
... 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
…on#1346), and remove-boost-signals1 (ros-visualization#1344)
This reverts commit 3f1b388.
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.
alternative implementation for ros-visualization#1079
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.
rhaschke
force-pushed
the
fix-material-handling
branch
from
d0c7fc1 to
83ac836
Compare
ghost
assigned
rhaschke
added a commit
that referenced
this pull request
Feb 17, 2019
Closed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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::RobotLinkwas created, the corresponding link material(s) were registered again with theOgre::MaterialManagerand 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 usingnew Ogre::Material(). This avoids both the unique naming problem, and the memory leak. However, the Ogre documentation explicitly suggests to useMaterialManager::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.