Add base drake_ros_viz package.

[hidmic]

Spin off from #2. Built on top of #21. Only the package skeleton to keep a low LOC count.

---

This change is 

[EricCousineau-TRI]

Reviewed 17 of 32 files at r3, all commit messages.
Reviewable status: 17 of 32 files reviewed, 9 unresolved discussions (waiting on @hidmic and @sloretz)

---

drake_ros_core/README.md, line 28 at r3 (raw file):

    1. Build this package `colcon build --packages-select drake_ros_core`
    
    **CMake**

nit (here and in new package) Consider removing pure CMake instructions.

I think colcon is the most useful (for use in ROS2 ecosystem, where the tool is available.)

---

drake_ros_tf2/README.md, line 26 at r3 (raw file):

    1. Build this package and its dependencies `colcon build --packages-up-to drake_ros_tf2`

    **CMake**

nit (Same as above)

---

drake_ros_tf2/src/utilities/internal_name_conventions.h, line 1 at r3 (raw file):

// Copyright 2022 Open Source Robotics Foundation, Inc.

nit Given that this is part of TRI contract work, I think the copyright should be written for TRI?

---

drake_ros_tf2/src/utilities/internal_name_conventions.h, line 20 at r3 (raw file):

namespace drake_ros_tf2 {
namespace utilities {

nit (here and elsewhere) The generic name utilities may have poor scoping, and thus grow oddly.

Consider removing this namespace, and just use the root-level package's namespace.
Alternatively, use more descriptive naming, e.g. conversion

---

drake_ros_viz/README.md, line 18 at r3 (raw file):

1. Activate the drake virtual environment.
1. Build it using Colcon, or using CMake directly.
    

nit Trailing spaces (here and elsewhere)

---

drake_ros_viz/README.md, line 26 at r3 (raw file):

    1. Build this package `colcon build --packages-up-to drake_ros_viz`
    
    **CMake**

nit (Same as above)

---

drake_ros_viz/src/utilities/internal_name_conventions.h, line 36 at r3 (raw file):

}

/** Formulate marker namespace given the model instance name, body name, body

nit (here and elsewhere) This is a private component, consider demoting docstring to private, /* */, not /** */

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

  return internal::CalcMarkerNamespace(
      inspector.GetOwningSourceName(geometry_id),
      inspector.GetName(geometry_id), geometry_id.get_value());

Hm... is there precedent for using geometry_id always for this name? (is that from @calderpg-tri's code?)

---

drake_ros_core/src/python_bindings/module_drake_ros_core.cpp, line 1 at r1 (raw file):

// Copyright 2020 Open Source Robotics Foundation, Inc.

My read is that Python bindings are deferred to separate PR - is that correct?

[sloretz]

Reviewed 15 of 32 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @calderpg-tri, @EricCousineau-TRI, and @hidmic)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Hm... is there precedent for using geometry_id always for this name? (is that from @calderpg-tri's code?)

In drake_ros_tf2 the version of GetTfFrameName that accepts a GeometryId uses SceneGraphInspector to grab the FrameId associated with that geometry and uses that to get a name, so it's not there. This seems to be intentionally using the Geometry's name.

drake-ros/drake_ros_tf2/src/utilities/name_conventions.cc

Lines 48 to 53 in d10bbdc

	std::string GetTfFrameName(
	const drake::geometry::SceneGraphInspector<double>& inspector,
	const std::unordered_set<const drake::multibody::MultibodyPlant<double>*>&
	plants,
	const drake::geometry::GeometryId& geometry_id) {
	return GetTfFrameName(inspector, plants, inspector.GetFrameId(geometry_id));

Can multiple Geometry IDs be associated with one Frame? The GeoemtryId might be used for the name here because it there is at least one visualization_msgs::msg::Marker for each geometry:

drake-ros/drake_ros_viz/src/scene_markers_system.cpp

Lines 340 to 346 in 8d2b950

	for (const drake::geometry::FrameId& frame_id : inspector.GetAllFrameIds()) {
	for (const drake::geometry::GeometryId& geometry_id :
	inspector.GetGeometries(frame_id, impl_->params.role)) {
	SceneGeometryToMarkers(impl_->params)
	.Populate(inspector, impl_->plants, geometry_id, output_value);
	}
	}

I suppose the Frame name seems like a good namespace to group the geometry markers with, but it will still need to be unique when combined with the id field. The geometry_id value itself might not work as an ID because the Drake geometry ID value is 64 bits, while the Marker Message ID is 32 bits. Maybe that could be done by counting the geometries associated with the frame like you did here. Do geometries have a specific order that can be relied on?

---

drake_ros_core/src/python_bindings/module_drake_ros_core.cpp, line 1 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

My read is that Python bindings are deferred to separate PR - is that correct?

It looks like the Python bindings are in #24

I meant the review as a comment. I'm not sure what button I clicked in reviewable to make it approved.

[sloretz]

---

drake_ros_tf2/src/utilities/internal_name_conventions.h, line 1 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Given that this is part of TRI contract work, I think the copyright should be written for TRI?

Good question! I asked about this internally. Edit: Answered OOB

[aaronchongth]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @calderpg-tri, @EricCousineau-TRI, @hidmic, and @sloretz)

---

drake_ros_tf2/src/utilities/internal_name_conventions.h, line 1 at r3 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Good question! I asked about this internally. It looks like open source code gets the copyright assigned to Open Source Robotics Foundation, Inc. even when it's funded by contract work. How important is it for the copyright line to say TRI in this case?

+1 on that question. I see that there is also the choice of adding another line for the copyright, for example https://github.com/RobotLocomotion/drake/blob/master/third_party/com_github_bazelbuild_rules_cc/whole_archive.bzl#L4. Let me know what you think

[aaronchongth]

Reviewable status: 20 of 38 files reviewed, 9 unresolved discussions (waiting on @aaronchongth, @calderpg-tri, @EricCousineau-TRI, and @sloretz)

---

drake_ros_core/README.md, line 28 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (here and in new package) Consider removing pure CMake instructions.

I think colcon is the most useful (for use in ROS2 ecosystem, where the tool is available.)

Gotcha, removed in fbb4ad9

---

drake_ros_tf2/README.md, line 26 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (Same as above)

Removed in fbb4ad9

---

drake_ros_tf2/src/utilities/internal_name_conventions.h, line 20 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (here and elsewhere) The generic name utilities may have poor scoping, and thus grow oddly.

Consider removing this namespace, and just use the root-level package's namespace.
Alternatively, use more descriptive naming, e.g. conversion

I removed the utilities scope, as there are helper functions for creating name conventions alongside conversion functions, fbb4ad9

---

drake_ros_viz/README.md, line 18 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Trailing spaces (here and elsewhere)

Thanks for catching that, removed in fbb4ad9

---

drake_ros_viz/README.md, line 26 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (Same as above)

Removed in fbb4ad9

---

drake_ros_viz/src/utilities/internal_name_conventions.h, line 36 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (here and elsewhere) This is a private component, consider demoting docstring to private, /* */, not /** */

Got it, thanks for pointing that out, fbb4ad9

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

the Drake geometry ID value is 64 bits, while the Marker Message ID is 32 bits.

thanks for catching that! I like the idea of having a single source of truth (output of GetTfFrameName) for both the frame_id and marker_namespace, and counting up for every unique instance. GetMarkerNamespace is conveniently only used once in scene_markers_system.cpp too, so removing it will be trivial. How does that sound?

[aaronchongth]

Reviewable status: 20 of 38 files reviewed, 9 unresolved discussions (waiting on @aaronchongth, @calderpg-tri, @EricCousineau-TRI, and @sloretz)

---

drake_ros_core/src/python_bindings/module_drake_ros_core.cpp, line 1 at r1 (raw file):

Previously, sloretz (Shane Loretz) wrote…

It looks like the Python bindings are in #24

yup! These files were removed as there were still .cpp and hpp file extensions and differently named source files. I must have accidentally left them in during the rebase and had to clean up afterwards, sorry for the messy commit history

[EricCousineau-TRI]

Reviewed 1 of 32 files at r3, 17 of 18 files at r4.
Reviewable status: 37 of 38 files reviewed, 1 unresolved discussion (waiting on @calderpg-tri and @EricCousineau-TRI)

---

drake_ros_tf2/src/utilities/internal_name_conventions.h, line 1 at r3 (raw file):

Previously, aaronchongth (Aaron Chong) wrote…

+1 on that question. I see that there is also the choice of adding another line for the copyright, for example https://github.com/RobotLocomotion/drake/blob/master/third_party/com_github_bazelbuild_rules_cc/whole_archive.bzl#L4. Let me know what you think

OK Gotcha, that sounds good! I will request written documentation out-of-band, just so we have it if/when someone else asks this same question.

---

drake_ros_core/src/python_bindings/module_drake_ros_core.cpp, line 1 at r1 (raw file):

Previously, aaronchongth (Aaron Chong) wrote…

yup! These files were removed as there were still .cpp and hpp file extensions and differently named source files. I must have accidentally left them in during the rebase and had to clean up afterwards, sorry for the messy commit history

OK Sounds good!

[sloretz]

Reviewed 1 of 18 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @calderpg-tri)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

I like the idea of having a single source of truth (output of GetTfFrameName) for both the frame_id and marker_namespace, and counting up for every unique instance. GetMarkerNamespace is conveniently only used once in scene_markers_system.cpp too, so removing it will be trivial. How does that sound?

Sounds good to me! I see in #23 that where GetMakerNamespace() is called it has access to the SceneGraphInspector so passing that to the GetTfFrameName API should be fine.

and counting up for every unique instance.

I have a question for @EricCousineau-TRI about that - do Geometries have a specific order that can be relied on? If not, maybe a map between a geometry ID and a marker ID is needed.

[EricCousineau-TRI]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @calderpg-tri)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I like the idea of having a single source of truth (output of GetTfFrameName) for both the frame_id and marker_namespace, and counting up for every unique instance. GetMarkerNamespace is conveniently only used once in scene_markers_system.cpp too, so removing it will be trivial. How does that sound?

Sounds good to me! I see in #23 that where GetMakerNamespace() is called it has access to the SceneGraphInspector so passing that to the GetTfFrameName API should be fine.

and counting up for every unique instance.

I have a question for @EricCousineau-TRI about that - do Geometries have a specific order that can be relied on? If not, maybe a map between a geometry ID and a marker ID is needed.

I believe that geometry ids should always increment (given the global pool you saw), and they should generally be created in the same order (e.g. from parsing or what not).
So yes, I think the ordering is stable, at least for a given revision of the code.

From looking at Calder's code in Anzu (), I see the following:

  const GeometryNamespaceFunction geometry_namespace_fn = [&] (
      const drake::multibody::ModelInstanceIndex,
      const drake::multibody::BodyIndex,
      const drake::geometry::GeometryId geometry_id) {
    const std::string shape_namespace = marker_namespace_prefix.value_or(
        inspector.GetOwningSourceName(geometry_id));
    const auto* illustration_properties =
        inspector.GetIllustrationProperties(geometry_id);
    const auto* proximity_properties =
        inspector.GetProximityProperties(geometry_id);
    if (illustration_properties != nullptr) {
      return shape_namespace + "_visual";
    } else if (proximity_properties != nullptr) {
      return shape_namespace + "_collision";
    } else {
      return std::string();
    }
  };

...

  for (const auto geometry_id : all_geometries) {
    ...
    // Get the ShapeDescription of the geometry
    const ShapeDescription description =
        MakeShapeDescription(plant, plant_context, geometry_id);
    ...
    // Convert to RViz markers
    try {
      const std::string marker_namespace =
          geometry_namespace_fn(model_instance_index, body_index, geometry_id);
      auto rviz_markers = MakeGeometryDisplayMarkers(
          description, (override_color) ? override_color : shape_color);
      for (auto& rviz_marker : rviz_markers.markers) {
        namespace_marker_counts[marker_namespace] += 1;
        rviz_marker.ns = marker_namespace;
        rviz_marker.id = namespace_marker_counts.at(marker_namespace);
        display_markers.markers.push_back(rviz_marker);
      }
    ...
  }

[EricCousineau-TRI]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I believe that geometry ids should always increment (given the global pool you saw), and they should generally be created in the same order (e.g. from parsing or what not).
So yes, I think the ordering is stable, at least for a given revision of the code.

From looking at Calder's code in Anzu (), I see the following:

  const GeometryNamespaceFunction geometry_namespace_fn = [&] (
      const drake::multibody::ModelInstanceIndex,
      const drake::multibody::BodyIndex,
      const drake::geometry::GeometryId geometry_id) {
    const std::string shape_namespace = marker_namespace_prefix.value_or(
        inspector.GetOwningSourceName(geometry_id));
    const auto* illustration_properties =
        inspector.GetIllustrationProperties(geometry_id);
    const auto* proximity_properties =
        inspector.GetProximityProperties(geometry_id);
    if (illustration_properties != nullptr) {
      return shape_namespace + "_visual";
    } else if (proximity_properties != nullptr) {
      return shape_namespace + "_collision";
    } else {
      return std::string();
    }
  };

...

  for (const auto geometry_id : all_geometries) {
    ...
    // Get the ShapeDescription of the geometry
    const ShapeDescription description =
        MakeShapeDescription(plant, plant_context, geometry_id);
    ...
    // Convert to RViz markers
    try {
      const std::string marker_namespace =
          geometry_namespace_fn(model_instance_index, body_index, geometry_id);
      auto rviz_markers = MakeGeometryDisplayMarkers(
          description, (override_color) ? override_color : shape_color);
      for (auto& rviz_marker : rviz_markers.markers) {
        namespace_marker_counts[marker_namespace] += 1;
        rviz_marker.ns = marker_namespace;
        rviz_marker.id = namespace_marker_counts.at(marker_namespace);
        display_markers.markers.push_back(rviz_marker);
      }
    ...
  }

Working: Still trying to figure out full mapping to see if/when geometry id might leak in.

@sloretz If you have time to inspect it, the code in Anzu is in rviz_geometry_visualizer.cc; I am on vacation next week, so may not be able to find it then.

[aaronchongth]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Still trying to figure out full mapping to see if/when geometry id might leak in.

@sloretz If you have time to inspect it, the code in Anzu is in rviz_geometry_visualizer.cc; I am on vacation next week, so may not be able to find it then.

Thanks for sharing this, @EricCousineau-TRI! Just checking, I'm assuming marker_namespace_prefix is user defined, since it is outside of this example's scope, is it of any significance?

We had a brief discussion during our meetings, and thought that constructing the marker namespace from just GetOwningSourceName and GeometryId might leave out a lot of information and make mapping from marker namespace back to the geometry in the multibody plant difficult. What do you think about constructing the namespace using all of model, body and geometry id? Then we can count up the marker ID for when namespaces are duplicated, like what was done in the example you provided.

[EricCousineau-TRI]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):
Hm... Upon inspecting Calder's viz code, and running in on my side, I think we should allow marker namespace customization has he had done in Anzu - meaning the translator function should take in a function similar to his, so that way the user can customize namespacing (albeit for one geometry role).

[...] make mapping from marker namespace back to the geometry in the multibody plant difficult [...]

I'm not sure if I understand this workflow - when might we want to map from visualization back to plant?
While that's a nice-to-have, not sure if we want to support it yet.
Additionally, b/c geometry_id is a *global` singleton (and thus may get affected by construction order, etc.), then that means the namespace may be unstable, and the RViz configuration would then get invalidated depending on when you create the MbP.

So yeah, my suggestions are:

• Ensure that there is a callback to determine the marker namespace for visualization
• Perhaps make the above geometry_namespace_fn a default implementation, e.g. MakeFlatGeometryNamespace
• Then make a stable nested / hierarchical namespace (if possible), e.g. MakeStableNestedGeometryNamespace
• If really desired, then make an unstable but 1-to-1 mappable namespace, e.g. MakeUnstableGeometryNamespace (I would suggest deferring for now)

Does that make sense?

[aaronchongth]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

when might we want to map from visualization back to plant?

So far I've not come across any instances where this mapping might be useful. At a stretch, maybe something like an rviz plugin that will in turn manipulate the MbP. But I like the idea of supporting different levels of namespacing, the unstable implementation can be considered if we ever come to that.

[aaronchongth]

Reviewable status: 34 of 38 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @sloretz)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, aaronchongth (Aaron Chong) wrote…

when might we want to map from visualization back to plant?

So far I've not come across any instances where this mapping might be useful. At a stretch, maybe something like an rviz plugin that will in turn manipulate the MbP. But I like the idea of supporting different levels of namespacing, the unstable implementation can be considered if we ever come to that.

I've opted for helper functions that generate these functors. MarkerNamespaceFunction will be used when the rest of drake_ros_rviz is introduced in #23. Let me know what you think @EricCousineau-TRI

[EricCousineau-TRI]

Reviewable status: 34 of 38 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @sloretz)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, aaronchongth (Aaron Chong) wrote…

I've opted for helper functions that generate these functors. MarkerNamespaceFunction will be used when the rest of drake_ros_rviz is introduced in #23. Let me know what you think @EricCousineau-TRI

Looks better!

However, I still see the presence of geometry_id in there, which is "unstable" when considering a MultibodyPlant / SceneGraph in isolation.

Per above, we should have an option to have a uniqueness counter, not a geometry_id value, help distinguish values. Otherwise, we may invalidate RViz configurations.

Conceretely, see this:
https://github.com/EricCousineau-TRI/repro/blob/6642ed8638d028891f6a250d17812ceb01604795/drake_stuff/notebooks/geometry_id_global_counter.ipynb?short_path=aa000f3#L81-L110
(sorry, would post notebook, but GitHub rendering is dropping output for some reason...)

Does that make sense?

[aaronchongth]

Reviewable status: 34 of 38 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @sloretz)

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Looks better!

However, I still see the presence of geometry_id in there, which is "unstable" when considering a MultibodyPlant / SceneGraph in isolation.

Per above, we should have an option to have a uniqueness counter, not a geometry_id value, help distinguish values. Otherwise, we may invalidate RViz configurations.

Conceretely, see this:
https://github.com/EricCousineau-TRI/repro/blob/6642ed8638d028891f6a250d17812ceb01604795/drake_stuff/notebooks/geometry_id_global_counter.ipynb?short_path=aa000f3#L81-L110
(sorry, would post notebook, but GitHub rendering is dropping output for some reason...)

Does that make sense?

Ahh! Thanks for the example, that makes it much clearer now. I don't know if it applies to body indices as well, but I've removed the uses of all these IDs during the formulation of the namespaces. geometry_id is currently only used for GetName or GetOwningSourceName.

The same considerations might need to be done for drake_ros_tf2 as well, I'll get those changes in as well once we agree on this approach.

As for the uniqueness counter, I was considering implementing that in the subsequent PR, where the namespaces are created, https://github.com/RobotLocomotion/drake-ros/pull/23/files#diff-323171292b532a621f0059b13b1543f9f8a5c703a0773edaa02b6dd68d15ac12R52-R65.

let me know what you think!

[EricCousineau-TRI]

Reviewed 1 of 2 files at r6, 3 of 3 files at r7.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @hidmic)

---

drake_ros_viz/src/utilities/internal_name_conventions.h, line 26 at r7 (raw file):

bool IsEmptyName(const std::string& name) {
  if (name.empty() || name == "::" || name == "/") {

nit It's a bit disconcerting that we'd get delimiters on their own - it means we're not doing great bookkeeping elsewhere.

Can you see if you can fix this, or add a TODO to clean this up?

---

drake_ros_viz/src/utilities/name_conventions.cc, line 45 at r3 (raw file):

Previously, aaronchongth (Aaron Chong) wrote…

Ahh! Thanks for the example, that makes it much clearer now. I don't know if it applies to body indices as well, but I've removed the uses of all these IDs during the formulation of the namespaces. geometry_id is currently only used for GetName or GetOwningSourceName.

The same considerations might need to be done for drake_ros_tf2 as well, I'll get those changes in as well once we agree on this approach.

As for the uniqueness counter, I was considering implementing that in the subsequent PR, where the namespaces are created, https://github.com/RobotLocomotion/drake-ros/pull/23/files#diff-323171292b532a621f0059b13b1543f9f8a5c703a0773edaa02b6dd68d15ac12R52-R65.

let me know what you think!

OK Looks better!

FTR, Body uses indices, not ids. For Drake, that's an important distinction:
https://github.com/RobotLocomotion/drake/blob/v1.1.0/common/type_safe_index.h
https://github.com/RobotLocomotion/drake/blob/v1.1.0/common/identifier.h

Indices are local, and thus stable (w.r.t. how we want them).

---

drake_ros_viz/src/utilities/name_conventions.cc, line 58 at r7 (raw file):

    return internal::CalcHierarchicalMarkerNamespace(
        prefix, inspector.GetOwningSourceName(geometry_id),
        inspector.GetName(geometry_id));

nit From looking at inspector.GetName(), it seems like this may have more components than we want.

EricCousineau-TRI/repro@c44615e
Can you add a TODO to further refine this?

---

drake_ros_viz/include/drake_ros_viz/utilities/name_conventions.h, line 46 at r7 (raw file):

/** Returns a functor that generates a hierarchical namespace (if possible) in
  the form of 'model_instance_name/body_name/geometry_id_value'. This is

nit geometry_id_value => geometry_nameb

---

drake_ros_viz/include/drake_ros_viz/utilities/name_conventions.h, line 51 at r7 (raw file):

  @returns the functor for generating the marker namespace.
 */
MarkerNamespaceFunction GetHierarchicalMarkerNamspaceFunction(

nit Spelling, "Namspace" => "Namespace"

[EricCousineau-TRI]

Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hidmic)

[EricCousineau-TRI]

for moving forward, with TODO's etc. for the future
Thanks!

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hidmic)

[aaronchongth]

Reviewed 1 of 4 files at r8.
Reviewable status: 35 of 38 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI)

---

drake_ros_viz/src/utilities/internal_name_conventions.h, line 26 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit It's a bit disconcerting that we'd get delimiters on their own - it means we're not doing great bookkeeping elsewhere.

Can you see if you can fix this, or add a TODO to clean this up?

That's a good point. I found that there is nothing stopping users from adding bodies or model instances with names like :: and /, but for all intents and purposes, it could be intentional and there might be some workflow that requires it. I have removed this check, and just used a std::string::empty() check in it's place.

---

drake_ros_viz/src/utilities/name_conventions.cc, line 58 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit From looking at inspector.GetName(), it seems like this may have more components than we want.

EricCousineau-TRI/repro@c44615e
Can you add a TODO to further refine this?

Just added a TODO for now, thanks for testing that out!

---

drake_ros_viz/include/drake_ros_viz/utilities/name_conventions.h, line 46 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit geometry_id_value => geometry_nameb

Done!

---

drake_ros_viz/include/drake_ros_viz/utilities/name_conventions.h, line 51 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Spelling, "Namspace" => "Namespace"

thanks for catching that! Fixed

[EricCousineau-TRI]

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hidmic)