[lcm] SerializerInterface is now cloneable

[jwnimmer-tri]

Modernize "= default" boilerplate while we're here.

This is breaking change because we add a new pure-virtual method to the C++ SerializerInterface base class.

---

This change is 

[jwnimmer-tri]

+@sammy-tri for feature review, please.

[sammy-tri]

Reviewed 5 of 5 files at r1.
Reviewable status: needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

systems/lcm/serializer.h, line 68 at r1 (raw file):

  ~Serializer() override = default;

  std::unique_ptr<SerializerInterface> Clone() const override {

BTW Huh... a Clone method which exists just to clone a vtable. Sure, why not?

[jwnimmer-tri]

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

---

systems/lcm/serializer.h, line 68 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW Huh... a Clone method which exists just to clone a vtable. Sure, why not?

Hah! That's a good observation. I guess another valid implementation here would be to lose the inheritance and just have a single class with four bare function pointers (not even std::function), since there is not captured data for any of these callbacks.

[jwnimmer-tri]

+@EricCousineau-TRI for platform review per schedule (tomorrow), please.

[sammy-tri]

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI)

---

systems/lcm/serializer.h, line 68 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hah! That's a good observation. I guess another valid implementation here would be to lose the inheritance and just have a single class with four bare function pointers (not even std::function), since there is not captured data for any of these callbacks.

Eh, this is fine with me. It's a familiar pattern in drake. Just got lost for a second thinking about the philosophical implications. :)

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI)

---

systems/lcm/serializer.h, line 68 at r1 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Eh, this is fine with me. It's a familiar pattern in drake. Just got lost for a second thinking about the philosophical implications. :)

Yeah. On further thought, we probably do need to keep the vtable + internal state, for pydrake. It uses runtime data for the Python message types, instead of compile-time statics.

[EricCousineau-TRI]

with a blocking comment (workflow question more-or-less)

Just to double-check, this is a breaking change b/c it introduces a new pure-virtual function to the Python SerializerInterface bindings?

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

a discussion (no related file):
nit Should __copy__ and __deepcopy__ be supported, and for copy.copy (and copy.deepcopy) to be tested?
(Motivation for PR not immediately clear, hard to know if this is a defect or just a nit - is it just for cloning a Diagram, or a user issue?)

e.g.

class PySerializer(...):
  def __copy__(self): return self.Clone()
  def __deepcopy__(self): return self.Clone()

(it is not necessary to add these methods to base class)

---

[EricCousineau-TRI]

(er, not blocking - just a nit; sorry)

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

[jwnimmer-tri]

(I added more breaking change info to the PR description.)

Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Should __copy__ and __deepcopy__ be supported, and for copy.copy (and copy.deepcopy) to be tested?
(Motivation for PR not immediately clear, hard to know if this is a defect or just a nit - is it just for cloning a Diagram, or a user issue?)

e.g.

class PySerializer(...):
  def __copy__(self): return self.Clone()
  def __deepcopy__(self): return self.Clone()

(it is not necessary to add these methods to base class)

My use case is only in C++. (I always try to add the pydrake bindings as a courtesy.)

I'm happy to take advice about whether __copy__ etc make sense here. The class has no mutable member data, so I'm not sure that a user would need to copy these? In C++, I only need to clone them to have a copyable_unique_ptr<SerializerInterface> due to sole ownership. I'll hypothesize that for Python, the GC and refcounted ownership would suffice on its own?

---

[EricCousineau-TRI]

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

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My use case is only in C++. (I always try to add the pydrake bindings as a courtesy.)

I'm happy to take advice about whether __copy__ etc make sense here. The class has no mutable member data, so I'm not sure that a user would need to copy these? In C++, I only need to clone them to have a copyable_unique_ptr<SerializerInterface> due to sole ownership. I'll hypothesize that for Python, the GC and refcounted ownership would suffice on its own?

OK Gotcha! If it's not too much overhead, then yeah, having Clone() go hand-in-hand with __copy__ and __deepcopy__ is easiest to think about.
(I'll PR a brief blurb to pydrake_doxygen.h to encode this)

Still not entirely sure if I understand workflow for copyable_unique_ptr<> - is it similar to Russ's workflow mentioned in Slack (e.g. this thread)? (not a huge deal, though!)

---

---

bindings/pydrake/systems/lcm_py.cc, line 125 at r1 (raw file):

    // interface will call the trampoline implementation methods above.
    cls  // BR
        .def("Clone", &Class::Clone, cls_doc.Clone.doc)

BTW Using DefClone() would define this in addition to __copy__ and __deepcopy__, albeit at the cost of losing the docstring (at present).

[jwnimmer-tri]

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Gotcha! If it's not too much overhead, then yeah, having Clone() go hand-in-hand with __copy__ and __deepcopy__ is easiest to think about.
(I'll PR a brief blurb to pydrake_doxygen.h to encode this)

Still not entirely sure if I understand workflow for copyable_unique_ptr<> - is it similar to Russ's workflow mentioned in Slack (e.g. this thread)? (not a huge deal, though!)

I'm fine to lose the docstring. I'll use DefClone here.

---

[jwnimmer-tri]

OK I'm using DefClone now, that's definitely an improvement. I knew it existed, but had forgotten.

Still not entirely sure if I understand workflow ...

An LcmPublisherSystem (for example) takes a unique_ptr<SerializerInterface> in its constructor. Say that the user gives me their serializer, and now I need to use the same serializer for two different (but similar) publishers. Which publisher gets it? To use it for both, I need to clone it. Or, if I need to stash it for later (or maybe a vector of them for later), I need them all to be cloneable, and if I'm using a std::vector of them that I need to copy, I need to wrap them in a copyable_unique_ptr to thunk the copy constructor into the Clone function so the vector can be copied without explicitly looping.

[EricCousineau-TRI]

Gotcha - thanks for explaining!

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),EricCousineau-TRI(platform) (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'm fine to lose the docstring. I'll use DefClone here.

Thanks!

---

[jwnimmer-tri]

Still not entirely sure if I understand workflow ...

An LcmPublisherSystem (for example) takes a unique_ptr<SerializerInterface> in its constructor. Say that the user gives me their serializer, and now I need to use the same serializer for two different (but similar) publishers. Which publisher gets it? To use it for both, I need to clone it. Or, if I need to stash it for later (or maybe a vector of them for later), I need them all to be cloneable, and if I'm using a std::vector of them that I need to copy, I need to wrap them in a copyable_unique_ptr to thunk the copy constructor into the Clone function so the vector can be copied without explicitly looping.

To circle back here, the alternative solution that we missed was to switch to shared_ptr<const SerializerInterface>. I've proposed that now in #19369.