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.

Sign up for GitHub

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

Jump to bottom

provide createMarker() #1183

Merged
merged 5 commits into from
Feb 23, 2018
Merged

provide createMarker() #1183

merged 5 commits into from
Feb 23, 2018

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Jan 5, 2018

May I ask for a central createMarker() function to create an rviz Marker given a specific marker type?
This functionality was already coded twice in rviz itself and - as I need it in an external project now - I would go for another copy otherwise.

This PR provides the requested functionality to create markers based on the known marker types in visualization_msgs::Marker at the core location, which knows about all these types.

@rhaschke
Copy link
Contributor Author

@dhood: ping

dhood
dhood suggested changes Feb 15, 2018
View reviewed changes
Copy link
Contributor

@dhood dhood left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor! (and for your patience 😄 ) This looks good overall, I just have a couple of comments

src/rviz/default_plugin/markers/marker_base.cpp Outdated
#include "rviz/default_plugin/markers/points_marker.h"
#include "rviz/default_plugin/markers/text_view_facing_marker.h"
#include "rviz/default_plugin/markers/mesh_resource_marker.h"
#include "rviz/default_plugin/markers/triangle_list_marker.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the new function to a separate file so this doesn't need to know about the derived classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'm not good at inventing new (file) names. I went for marker_utils.h/cpp.

src/rviz/default_plugin/markers/marker_base.cpp Outdated
return new rviz::TriangleListMarker(owner, context, parent_node);

default:
ROS_ERROR("Unknown marker type: %d", marker_type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the decision to print the error message to the callers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rhaschke
Copy link
Contributor Author

rhaschke commented Feb 15, 2018 •
edited
Loading

@dhood I applied the requested changes, but I face some unrelated problems building on my system. Was there a recent change to class_loader?

In file included from /opt/ros/kinetic/include/pluginlib/class_loader.h:35:0,
                 from /homes/rhaschke/src/ros/src/rviz/src/rviz/pluginlib_factory.h:40,
                 from /homes/rhaschke/src/ros/src/rviz/src/rviz/view_manager.h:36,
                 from view_manager.sip:8:
/opt/ros/kinetic/include/pluginlib/./class_loader.hpp:57:46: error: ‘UniquePtr’ in ‘class class_loader::ClassLoader’ does not name a template type
 using UniquePtr = class_loader::ClassLoader::UniquePtr<T>;

@mikaelarguedas
Copy link
Contributor

Was there a recent change to class_loader?

A UniquePtr interface has been added for platforms supporting c++11 by default. But that was a while ago so I dont expect that to be the cause of the problem here. As the 2 PR jobs passed my guess is that there is something different between your setup and the farm. There have been some changes in pluginlib recently but your console output seems the include the latest headers so I'm not sure what is different between your setup and the buildfarm...

@rhaschke
Copy link
Contributor Author

Rebuilding my whole workspace solved my local build issues. Tests succeeded.

wjwwood
wjwwood approved these changes Feb 22, 2018
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Changes lgtm, and it looks like @dhood's comments were addressed, though I'll let her rereview before merging.

I made a few small style changes.

@dhood dhood merged commit 772da5f into ros-visualization:kinetic-devel Feb 23, 2018
@dhood
Copy link
Contributor

dhood commented Feb 23, 2018

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhood dhood dhood requested changes

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rhaschke @mikaelarguedas @dhood @wjwwood