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

Misleading function names: ExportSurface(s)ForDisplay #1

Open
dmcconachie opened this issue Nov 29, 2018 · 1 comment
Open

Misleading function names: ExportSurface(s)ForDisplay #1

dmcconachie opened this issue Nov 29, 2018 · 1 comment

Comments

@dmcconachie
Copy link

dmcconachie commented Nov 29, 2018

The following 2 functions could potentially benefit from a rename

visualization_msgs::Marker ExportSurfacesForDisplay(
    const CollisionMap& collision_map,
    const std_msgs::ColorRGBA& collision_color,
    const std_msgs::ColorRGBA& free_color,
    const std_msgs::ColorRGBA& unknown_color)
visualization_msgs::Marker ExportSurfaceForDisplay(
    const CollisionMap& collision_map,
    const std::unordered_map<
        common_robotics_utilities::voxel_grid::GridIndex, uint8_t>& surface,
    const std_msgs::ColorRGBA& surface_color)

In general the 2nd function can export any arbitrary set of points, not just a surface. These could probably benefit from some renaming or at least some comments of some sort describing what they actually do and how they are intended to be used (mostly for the 2nd function). It's not clear what the surface input should be.

(and matching changes in ExportVoxelGridSurfaceToRViz and similar functions)

@calderpg
Copy link
Owner

calderpg commented Dec 6, 2018

I have renamed these functions from Surface to IndexMap for the time being before more detailed doxygen is added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants