Deprecating PoseBundle

[SeanCurtis-TRI]

This deprecates PoseBundle as well as closely affiliated classes: PoseVector, PoseAggregator, PoseBundoeToDrawMessage.

There are two systems that have PoseBundle-valued output ports and two diagrams that export those ports.

The systems had explicit getters for the ports; those getters have been deprecated.

The diagrams were mixed. One had a getter, one relied on get_output_port(name). To generally aid the case where a leaf output port is exported, the calc methods of the two systems have been given a "log once" warning.

Bindings and unit tests have likewise been updated.

Resolves #10482
Resolves #13580

---

This change is 

[rpoyner-tri]

I'll bite. +@rpoyner-tri for feature review.

Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @rpoyner-tri)

[rpoyner-tri]

niggilng questions

Reviewed 21 of 32 files at r1, 17 of 17 files at r2.
Reviewable status: needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

---

bindings/pydrake/systems/planar_scenegraph_visualizer.py, line 129 at r2 (raw file):

        self._T_VW = T_VW

        # (2021-11-01) Remove at end of deprecation period.

BTW is this date advice outdated?

---

bindings/pydrake/systems/test/planar_scenegraph_visualizer_test.py, line 31 at r2 (raw file):

        self.assertTrue(cart_pole.geometry_source_is_registered())

        # During transition period, planar scene graph is constructing a

BTW there are a lot of call sites touching deprecation in this file, with a lot of comment copy-pasta. Consider maybe a paragraph at the top, and some reference-marker at the call sites?

Maybe:

# @ref pose_bundle_deprecation 2021-12-01 or so? I like having the date at the sites that need to be touched, but the description could be elsewhere. Of course this is abusing doxygen-style notation, but maybe close enough.

---

bindings/pydrake/systems/test/rendering_test.py, line 41 at r2 (raw file):

class TestRendering(unittest.TestCase):
    def test_pose_vector(self):
        with catch_drake_warnings(expected_count=1):

BTW I will assume that the PoseBundle-savvy reader will know why these calls sites give deprecation errors. Is it necessary to tag them with deprecation-date comments, or will the removal also be obvious?

[SeanCurtis-TRI]

Questions answered, code tweaked.

Reviewable status: needs at least two assigned reviewers (waiting on @rpoyner-tri)

---

bindings/pydrake/systems/planar_scenegraph_visualizer.py, line 129 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

BTW is this date advice outdated?

No. This is correct. This class has its own api that got deprecated prior to PoseBundle being deprecated. So, this will actually stop using PoseBundle a month before PoseBundle is actually removed. Understandable confusion, though.

---

bindings/pydrake/systems/test/planar_scenegraph_visualizer_test.py, line 31 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

BTW there are a lot of call sites touching deprecation in this file, with a lot of comment copy-pasta. Consider maybe a paragraph at the top, and some reference-marker at the call sites?

Maybe:

# @ref pose_bundle_deprecation 2021-12-01 or so? I like having the date at the sites that need to be touched, but the description could be elsewhere. Of course this is abusing doxygen-style notation, but maybe close enough.

A reasonable suggestion, but as this file has two independent deprecations taking place, I'd rather err on the verbose side.

---

bindings/pydrake/systems/test/rendering_test.py, line 41 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

BTW I will assume that the PoseBundle-savvy reader will know why these calls sites give deprecation errors. Is it necessary to tag them with deprecation-date comments, or will the removal also be obvious?

That's a big question. Ultimately, the compiler will let you know if you chase the dates. I tend to err on the side of "put dates everywhere" so you can remove them in one pass rather than remove -> compile -> remove -> compile -> etc.

The fact that I didn't do it here should be considered an oversight. Rectified.

[rpoyner-tri]

Reviewed 1 of 1 files at r3.
Reviewable status: needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

[jwnimmer-tri]

+@jwnimmer-tri for platform review.

[jwnimmer-tri]

Checkpoint on the C++ code. Need a break before I return to the Python.

Reviewed 15 of 32 files at r1, 13 of 17 files at r2.
Reviewable status: 12 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @SeanCurtis-TRI)

---

examples/manipulation_station/manipulation_station.cc, line 740 at r3 (raw file):

  }

  // TODO(#10482).  Deprecate the pose_bundle output port.

nit We can lose the TODO now?

---

geometry/scene_graph.cc, line 408 at r3 (raw file):

void SceneGraph<T>::CalcPoseBundle(const Context<T>& context,
                                   PoseBundle<T>* output) const {
  static const logging::Warn log_once(

nit For clarity, I think we need a classname (or filename) in this message? As it stands, I suspect it's unclear about whose PoseVector port is at fault.

---

geometry/scene_graph.cc, line 411 at r3 (raw file):

      "Do not use the PoseBundle-valued output port. It is deprecated for "
      "removal after 2021-12-01. Instead use the QueryObject-valued port and "
      "simply query for any information you need.");

nit When I'm writing deprecation messages, I often find myself adding phrses like "just [do foo]" or "simply [do bar]". But when I reflect on the experience as a user, they might not see it as a simple change; they are annoyed they have to change and it's just an obstacle. Thus, I try to avoid writing any text that imposes an belief on the end user. Possibly worth altering this text, given that perspective?

---

systems/rendering/BUILD.bazel, line 31 at r3 (raw file):

    srcs = ["pose_aggregator.cc"],
    hdrs = ["pose_aggregator.h"],
    copts = ["-Wno-deprecated-declarations"],

This addition seems spurious. If I remove it, nothing complains.

---

systems/rendering/frame_velocity.cc, line 11 at r3 (raw file):

namespace systems {
namespace rendering {

nit Spurious double blank line?

---

systems/rendering/render_pose_to_geometry_pose.cc, line 24 at r3 (raw file):

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
  using Input = PoseVector<T>;

Given that the sole input port type to this system is deprecated and scheduled for removal, I don't understand how the system itself can be expected to survive after three months? It seems like we should be deprecating this whole system?

---

systems/rendering/test/render_pose_to_geometry_pose_test.cc, line 8 at r3 (raw file):

#include "drake/systems/framework/test_utilities/scalar_conversion.h"

#pragma GCC diagnostic push

The class RenderPoseToGeometryPose is not deprecated, so I don't think we should nerf its tests across the board?

---

systems/sensors/rgbd_sensor.h, line 201 at r3 (raw file):

#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
  void CalcX_WB_pose_vector(const Context<double>& context,
                rendering::PoseVector<double>* pose_vector) const;

nit Ban indentation of the second argument here; needs either +4 from above, or aligned after open-paren.

---

systems/sensors/rgbd_sensor.h, line 310 at r3 (raw file):

  /** Returns the abstract-valued output port (containing a RigidTransform)
   which reports the pose of the body in the world frame (X_WB).  */

This comment idiom is inconsistent with the rest of this class. For the other getters, the original author has chosen to cite the authoritative documentation, rather than copy-paste it. I think we should use that idiom here as well, both for consistency, and to reduce documentation brittleness.

---

systems/sensors/rgbd_sensor.cc, line 81 at r3 (raw file):

  body_pose_in_world_output_port_ = &this->DeclareAbstractOutputPort(
      "body_pose_in_world", RigidTransformd(), &RgbdSensor::CalcX_WB);

nit No need to explicitly give the RigidTransformd() constructor here; a default-constructed model value is already the default for Calc functions, when omitted.

---

systems/sensors/rgbd_sensor.cc, line 163 at r3 (raw file):

    rendering::PoseVector<double>* pose_vector) const {
  static const logging::Warn log_once(
      "Do not use the PoseVector-valued output port. It is deprecated for "

nit For clarity, I think we need a classname (or filename) in this message? As it stands, I suspect it's unclear about whose PoseVector port is at fault.

---

systems/sensors/rgbd_sensor.cc, line 165 at r3 (raw file):

      "Do not use the PoseVector-valued output port. It is deprecated for "
      "removal after 2021-12-01. Instead use "
      "`body_pose_in_world_output_port().");

nit Missing closing backtick. (Or really, nix the opening backtick; we're speaking to a human, not a markdown renderer.)

---

systems/sensors/rgbd_sensor.cc, line 262 at r3 (raw file):

  }

  // No need to place a ZOH on pose output.

BTW I find this premise suspicious. Wouldn't the pose value experience tearing vs the images it's putative providing the frame for?

[jwnimmer-tri]

Reviewed 6 of 32 files at r1, 1 of 17 files at r2, 1 of 1 files at r3.
Reviewable status: 17 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @SeanCurtis-TRI)

---

bindings/pydrake/systems/planar_scenegraph_visualizer.py, line 117 at r3 (raw file):

                Default is None, which implies show=True unless
                matplotlib.get_backend() is 'template'.
        """

nit Missing docstring for need_pose_bundle here.

Though below I see you call it "secret". It it's really only a private flag not meant for end-users, then name it _need_pose_bundle in above, to indicate that it's a private argument.

(In any case, check my thread on the unit test warnings first. I suspect there is a better overall solution that would nix this argument anyway.)

---

bindings/pydrake/systems/planar_scenegraph_visualizer.py, line 137 at r3 (raw file):

            self._warned_pose_bundle_input_port_connected = False
        else:
            self._pose_bundle_port = None

nit It's bad form to have the set of member field vary with control flow. We should intialize self._warned_pose_bundle_input_port_connected to something here.

(In any case, check my thread on the unit test warnings first. I suspect there is a better overall solution that would nix this argument anyway.)

---

bindings/pydrake/systems/rendering_py.cc, line 41 at r3 (raw file):

  // something with an appropriately formatted deprecation warning.
  const std::string deprecated_rendering(
      " This class has been deprecated and will be removed by 2021-12-01.");

nit "by" is not accurate; "on or after", I think?

---

bindings/pydrake/systems/rendering_py.cc, line 47 at r3 (raw file):

  pose_vector  // BR
      .def(py_init_deprecated<PoseVector<T>>(
          doc.PoseVector.ctor.doc_0args + deprecated_rendering))

On this first constructor, we've lost the docstring -- we need to pass it both to py_init_deprecated as well as def.

---

bindings/pydrake/systems/test/planar_scenegraph_visualizer_test.py, line 31 at r3 (raw file):

        self.assertTrue(cart_pole.geometry_source_is_registered())

        # During transition period, planar scene graph is constructing a

Hmm, is the warning coming from AbstractValue.Make(PoseBundle(0)) within the implementation? I suspect that your users (and this code) would be much happier if the implementation suppressed the warning and/or caught the error at that one call site, instead of allowing it to percolate everywhere? The end user can't control whether or not the implementation is going to warn, so we're just giving them a headache they can't fix. Similar to how in C++ if drake itself has code that is triggering the warning, we put #pragma around our code to suppress the warning. I think we should do likewise in planar_scenegraph_visualizer.py.

[SeanCurtis-TRI]

Comments addressed (and hopefully no new errors...it's so tetchy.)

Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)

---

bindings/pydrake/systems/planar_scenegraph_visualizer.py, line 117 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Missing docstring for need_pose_bundle here.

Though below I see you call it "secret". It it's really only a private flag not meant for end-users, then name it _need_pose_bundle in above, to indicate that it's a private argument.

(In any case, check my thread on the unit test warnings first. I suspect there is a better overall solution that would nix this argument anyway.)

Done

---

bindings/pydrake/systems/planar_scenegraph_visualizer.py, line 137 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit It's bad form to have the set of member field vary with control flow. We should intialize self._warned_pose_bundle_input_port_connected to something here.

(In any case, check my thread on the unit test warnings first. I suspect there is a better overall solution that would nix this argument anyway.)

Done

---

bindings/pydrake/systems/rendering_py.cc, line 41 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit "by" is not accurate; "on or after", I think?

Done

---

bindings/pydrake/systems/rendering_py.cc, line 47 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

On this first constructor, we've lost the docstring -- we need to pass it both to py_init_deprecated as well as def.

Done

---

bindings/pydrake/systems/test/planar_scenegraph_visualizer_test.py, line 31 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hmm, is the warning coming from AbstractValue.Make(PoseBundle(0)) within the implementation? I suspect that your users (and this code) would be much happier if the implementation suppressed the warning and/or caught the error at that one call site, instead of allowing it to percolate everywhere? The end user can't control whether or not the implementation is going to warn, so we're just giving them a headache they can't fix. Similar to how in C++ if drake itself has code that is triggering the warning, we put #pragma around our code to suppress the warning. I think we should do likewise in planar_scenegraph_visualizer.py.

Done

---

examples/manipulation_station/manipulation_station.cc, line 740 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit We can lose the TODO now?

Done

---

geometry/scene_graph.cc, line 408 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit For clarity, I think we need a classname (or filename) in this message? As it stands, I suspect it's unclear about whose PoseVector port is at fault.

Done

---

geometry/scene_graph.cc, line 411 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit When I'm writing deprecation messages, I often find myself adding phrses like "just [do foo]" or "simply [do bar]". But when I reflect on the experience as a user, they might not see it as a simple change; they are annoyed they have to change and it's just an obstacle. Thus, I try to avoid writing any text that imposes an belief on the end user. Possibly worth altering this text, given that perspective?

Done

---

systems/rendering/BUILD.bazel, line 31 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This addition seems spurious. If I remove it, nothing complains.

I presume you did a test build using gcc. clang gets angry if you omit it.

---

systems/rendering/frame_velocity.cc, line 11 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Spurious double blank line?

Done

---

systems/rendering/render_pose_to_geometry_pose.cc, line 24 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Given that the sole input port type to this system is deprecated and scheduled for removal, I don't understand how the system itself can be expected to survive after three months? It seems like we should be deprecating this whole system?

As implied by the change to the test, it was supposed to be deprecated. What's embarassing is that I's missed another in this directory in my first pass and caught it in the follow up, but still missed this one.

---

systems/rendering/test/render_pose_to_geometry_pose_test.cc, line 8 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The class RenderPoseToGeometryPose is not deprecated, so I don't think we should nerf its tests across the board?

Ok
I'd intended to deprecate this one as well.

---

systems/sensors/rgbd_sensor.h, line 201 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Ban indentation of the second argument here; needs either +4 from above, or aligned after open-paren.

Done

---

systems/sensors/rgbd_sensor.h, line 310 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This comment idiom is inconsistent with the rest of this class. For the other getters, the original author has chosen to cite the authoritative documentation, rather than copy-paste it. I think we should use that idiom here as well, both for consistency, and to reduce documentation brittleness.

Done

---

systems/sensors/rgbd_sensor.cc, line 81 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit No need to explicitly give the RigidTransformd() constructor here; a default-constructed model value is already the default for Calc functions, when omitted.

Done

---

systems/sensors/rgbd_sensor.cc, line 163 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit For clarity, I think we need a classname (or filename) in this message? As it stands, I suspect it's unclear about whose PoseVector port is at fault.

Done

---

systems/sensors/rgbd_sensor.cc, line 165 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Missing closing backtick. (Or really, nix the opening backtick; we're speaking to a human, not a markdown renderer.)

Done

---

systems/sensors/rgbd_sensor.cc, line 262 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I find this premise suspicious. Wouldn't the pose value experience tearing vs the images it's putative providing the frame for?

I know what you mean. I have no idea how this value is used. It was included way back in the original RgbdCamera and it continued through to here (both the value and its "holded-ness"). I haven't had the time to truly find out if anyone is actually using it.

I understand that Eric wouldn't need/want to use it. He wants to specify a frame in an SDF file and have the pose of the camera w.r.t. that frame be the identity. So, he'd just need the pose of that frame in the world.

[jwnimmer-tri]

Reviewed 11 of 11 files at r4.
Reviewable status: 1 unresolved discussion (waiting on @SeanCurtis-TRI)

---

systems/rendering/render_pose_to_geometry_pose.cc, line 24 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

As implied by the change to the test, it was supposed to be deprecated. What's embarassing is that I's missed another in this directory in my first pass and caught it in the follow up, but still missed this one.

Given the file-wide pragma, I think we can lose this inner pair of pragmas now.

[SeanCurtis-TRI]

Resolving clang build failure required catching an #include in the warning deprecation net. :-P

Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)

---

systems/rendering/render_pose_to_geometry_pose.cc, line 24 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Given the file-wide pragma, I think we can lose this inner pair of pragmas now.

Done

[jwnimmer-tri]

Yup. I guess that's why I usually just throw copts = -Wno... on the doomed library and test files, instead of using pragmas.

Reviewed 2 of 2 files at r5.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @SeanCurtis-TRI)