systematic check of incoming markers #1275
Conversation
all warnings are now logged in marker status and ROS_WARN checks were moved from the individual marker classes to marker_utils.cpp a multitude of checks were added to validate documentation conformity.
|
@wjwwood any chances to see this merged in any (ROS1) branch? Any changes required? This is very helpful when working with students when they don't know what they are doing really. |
|
I'm sorry, but I don't have time to merge any features right now, or release them. I will say that if this changes API or ABI, I'll only take it for melodic or later, but I haven't checked this pr at all yet. |
There was a problem hiding this comment.
I like this cleanup in general. However, I'm a little bit concerned about slowdown.
You frequently allocate std::string from const char* and then push this into stringstream.
I think the std::string allocation can be avoided if you directly pass the stringstream to the checker functions. Using ostringstream::tellp() you can also efficiently decide whether to add a comma or not.
I need to test this in practice...
|
@jgueldenstein is currently finalizing his thesis (due in two weeks).
He promised me to take care of this afterwards.
|
Co-Authored-By: Robert Haschke <[email protected]>
... starting uppercase and finishing with a period.
|
Thank you very much for your patience, time and your review, I checked your commits and I think they do some nice cleanup and consistency. |
Checks were moved from the individual marker classes to marker_utils.cpp A multitude of checks was added to validate conformity to documentation. All warnings are logged to console and in display status. Bug fixes: * checking text in TEXT_VIEW_FACING marker to prevent crashes * ignore unknown marker type
|
Thank you for all the time you spent on this @rhaschke !
|
- invalid quaternion: issue warning only - triangle list accepts points.size()/3 colors as well (face colors) - don't create (empty) text marker for interactive marker control with empty description
- invalid quaternion: issue warning only - triangle list accepts points.size()/3 colors as well (face colors) - don't create (empty) text marker for interactive marker control with empty description
…vels - distinguish all ros::console::levels - issue rviz status warning for any message, but don't clutter console - use logger namespace to allow disabling console output
- invalid quaternion: issue warning only - triangle list accepts points.size()/3 colors as well (face colors) - don't create text marker for interactive marker control with empty description - allow more fine-grained log levels - distinguish all ros::console::levels - issue rviz status warning for any message, but avoid cluttering the console - use logger namespace to allow disabling console output
- invalid quaternion: issue warning only - triangle list accepts points.size()/3 colors as well (face colors) - don't create text marker for interactive marker control with empty description - allow more fine-grained log levels - distinguish all ros::console::levels - issue rviz status warning for any message, but avoid cluttering the console - use logger namespace to allow disabling console output
Using rviz Markers for debugging is often very useful, but sometimes the developer makes mistakes when filling in the fields of the marker.
In some cases the resulting markers are not visible or not displayed as intended.
In a few cases this lead to long debugging sessions when the actual problem was the visualization...
Until now there was little and unorganized debug output from rviz to assist the developer in some of these cases.
I have implemented a systematic check of all marker types as well as marker arrays
as described here and implemented in the Display.
During testing I found some other bugs as well:
TEXT_VIEW_FACINGMarkers with an empty text cause rviz to segfault.TEXT_VIEW_FACINGMarkers with text that consists exclusively of whitespace trigger a vertex buffer overflow.These two bugs are resolved in commit d30cda4
This is resolved in commit 9c15d9a
Apart from the bugfixes, this pull-request does not change the rendering behavior of rviz.
It simply provides additional warning output to the console and to the display status.
We would like to see this merged in kinetic&upwards, so the request intentionally targets
kinetic-devel.@v4hn