[systems] Add DiagramBuilder::RemoveSystem

[jwnimmer-tri]

Towards #19158.

---

This change is 

[jwnimmer-tri]

+@rpoyner-tri for feature review, please.

+@sherm1 for high-level feature review, please -- as well as platform review.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),sherm1(platform) (waiting on @jwnimmer-tri, @rpoyner-tri, and @sherm1)

---

systems/framework/diagram_builder.cc line 98 at r3 (raw file):

  }

  // Disconnect any internal connections associated with this system.

Whoops. I forgot to test coverage for this block.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignees rpoyner-tri(platform),sherm1(platform) (waiting on @rpoyner-tri and @sherm1)

---

systems/framework/diagram_builder.cc line 98 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Whoops. I forgot to test coverage for this block.

Done.

[rpoyner-tri]

Reviewed 2 of 6 files at r1, 2 of 4 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform) (waiting on @jwnimmer-tri and @sherm1)

---

systems/framework/diagram_builder.h line 255 at r4 (raw file):

  /// decrementing the indices of all higher-numbered ports that remain.
  ///
  /// @warning Because a DigramBuilder owns the objects it contains, the system

typo

Suggestion:

DiagramBuilder

[rpoyner-tri]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

[sherm1]

It's not clear to me how this is related to render camera preview in #19158 -- please explain if that is the motivation for this PR.

High-level feature, and platform with a few minor comments

Reviewed 2 of 6 files at r1, 2 of 4 files at r2, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions (waiting on @jwnimmer-tri)

---

systems/framework/diagram_builder.h line 256 at r5 (raw file):

  ///
  /// @warning Because a DiagramBuilder owns the objects it contains, the system
  /// will be deleted.

BTW consider mentioning that this must be done prior to Build() -- might not be obvious to someone who doesn't realize Build() transfers ownership out.

---

systems/framework/diagram_builder.cc line 51 at r5 (raw file):

                     return item.get() == &system;
                   }));
  DRAKE_DEMAND(system_index < registered_systems_.size());

BTW I doubt there is a performance concern here, but you could use a find_if==end result to issue the above error message without doing the search twice. I think system_index==registered_systems_.size() is the same condition as systems_.count()==0.

---

systems/framework/test/diagram_builder_test.cc line 64 at r5 (raw file):

  //
  // This setup is carefully chosen such that removing 'adder' will cover all
  // branching conditions within the implementation.

BTW confirmed with kcov

The only thing that wasn't covered was CheckInvariants() which is called only in Debug. You could consider a manual call here in the test cases FWIW.

[jwnimmer-tri]

It's not clear to me how this is related to render camera preview in #19158 -- please explain if that is the motivation for this PR.

That PR contains a call to this new function:

            # Disable LCM image transmission. It has a non-trivial cost, and
            # at the moment Meldis can't display LCM images anyway.
            self._builder.builder().RemoveSystem(camera_publisher)

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

---

systems/framework/diagram_builder.h line 256 at r5 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider mentioning that this must be done prior to Build() -- might not be obvious to someone who doesn't realize Build() transfers ownership out.

That's true of all functions in the the header. I won't try to fix it in this PR.

---

systems/framework/diagram_builder.cc line 51 at r5 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW I doubt there is a performance concern here, but you could use a find_if==end result to issue the above error message without doing the search twice. I think system_index==registered_systems_.size() is the same condition as systems_.count()==0.

I don't think performance carries any weight here.

---

systems/framework/test/diagram_builder_test.cc line 64 at r5 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW confirmed with kcov

The only thing that wasn't covered was CheckInvariants() which is called only in Debug. You could consider a manual call here in the test cases FWIW.

For my own taste, running bazel test //systems/framework:diagram_builder_test -c dbg during development was sufficient. Checking the invariants buys us the most when they are running continuously in all test cases, not just one line here.

[jwnimmer-tri]

Merging now, to unblock a train of coming PRs. If there are other lingering improvements I'm happy to fix in a follow-up.

[sherm1]

systems/framework/diagram_builder.cc line 18 at r5 (raw file):

/* Erases the i'th element of vec, shifting everything after it over by one. */
template <typename StdVector>
void VectorErase(StdVector* vec, size_t i) {

FYI (irrelevent here where performance is not a concern, but I should have at least mentioned it):
When an std::vector is used as a set (rather than a sequence) you can do O(1) erasure of element i by moving the last element of the vector to position i and shortening the vector by 1. Less renumbering required also.

[jwnimmer-tri]

systems/framework/diagram_builder.cc line 18 at r5 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

FYI (irrelevent here where performance is not a concern, but I should have at least mentioned it):
When an std::vector is used as a set (rather than a sequence) you can do O(1) erasure of element i by moving the last element of the vector to position i and shortening the vector by 1. Less renumbering required also.

I actually did that initially. It fails to meet the API promise that the relative order of the systems and ports remains unchanged.