-
Notifications
You must be signed in to change notification settings - Fork 170
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
enable to modify caption contents for Plotter2D and PieChart #636
Conversation
|
0485821
to
2e3de5d
Compare
not sure why, but did not compile on jade https://travis-ci.org/jsk-ros-pkg/jsk_visualization/jobs/159271307, switch to jenkins... |
@@ -359,6 +363,11 @@ namespace jsk_rviz_plugins | |||
|
|||
} | |||
|
|||
void PieChartDisplay::updateCaption() | |||
{ | |||
caption_ = caption_property_->getStdString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not below implementation for backward compatibility of current existing rviz config file, and convenience.
std::string caption = caption_property_->getStdString();
if (caption.empty()) {
caption_ = std::string(getName()); // the name of display
} else {
caption_ = caption;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried to use getName()
but did not know where is this function and name_
defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, caption_property_->getStdString();
is initialized at https://github.com/jsk-ros-pkg/jsk_visualization/pull/636/files#diff-21915262cfae72cf9ac2af03ff4ea8e7R91, so if you worried about null caption name, then we can set at there.
If you think we do not have to define caption_
, but can use name_
, I do not have confident on modifying name_
because did not know how is this variable used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried to use getName() but did not know where is this function and name_ defined.
Maybe this? http://docs.ros.org/diamondback/api/rviz/html/classrviz_1_1Display.html#a7f3a75feb307f0cc307f7c78897d4237
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, caption_property_->getStdString(); is initialized at https://github.com/jsk-ros-pkg/jsk_visualization/pull/636/files#diff-21915262cfae72cf9ac2af03ff4ea8e7R91, so if you worried about null caption name, then we can set at there.
If you think we do not have to define caption_, but can use name_, I do not have confident on modifying name_ because did not know how is this variable used
Probably name_
is the name of display (below) and should not be changed, so I think caption_
variable is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, please look at .rviz file in this commit. It uses old format and works ok, so we do not need #636 (comment) for backward compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
finally, it passed the test, i have to
if no one knows how to deal with this situation, I think using ccache is dangerous, it took 8 hours for me...... |
Really.. I don't know how to fix this, but why not automatically remove the cache at reboot of jenkins? |
Closes #634
you can try this with
roslaunch jsk_rviz_plugins 2dplot_sample.launch