Reviewable status: 1 unresolved discussion, LGTM missing from assignee sadraddini, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r1 (raw file):

  DRAKE_THROW_UNLESS(!regions_.empty());

  std::vector<VectorXd> maybe_edge_offsets;

nit why you call it maybe_edge_offsets? I see this always filled (?).maybe reminds me of std::optional types

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sadraddini, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r1 (raw file):

If the user gives a nullptr for edge_offsets, then we populate that variable with the edge offsets, and set edge_offsets to point to it. Otherwise, it's just left empty and unused, since edge_offsets already points to the relevant data.

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r1 (raw file):

I see. I suggest making it a std::optional object to make it clear. You may add a DRAKE_DEMAND(maybe_edge_offsets.has_value()) somewhere before reading the offsets.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r1 (raw file):

Done.

I didn't see a place where it's obvious to drop in a DRAKE_DEMAND, since we pretty use it right away.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 7 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r2 (raw file):

  DRAKE_THROW_UNLESS(!regions_.empty());

  std::optional<std::vector<VectorXd>> maybe_edge_offsets = std::nullopt;

BTW Consider if you can find a more precise adjective than "maybe" here -- the purpose of this variable is slightly unclear. Perhaps "autocomputed_edge_offsets" or "edge_offsets_temporary_storage" or ...? Or perhaps a comment "// This will hold the edge offsets if they weren't given as arguments." would help clarify, instead of renaming.

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r2 (raw file):

  DRAKE_THROW_UNLESS(!regions_.empty());

  std::optional<std::vector<VectorXd>> maybe_edge_offsets = std::nullopt;

nit Assigning = std::nullopt; here (and similarly below) is somewhat unconventional. The plain std::optional<std::vector<VectorXd>> maybe_edge_offsets; already defaults to nullopt even with no arguments given. If the goal is to emphasize that the null-ness is acutely important that it's fine, but that did not seem important to me.

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 241 at r2 (raw file):

    std::vector<std::vector<std::pair<double, double>>> continuous_bboxes;
    continuous_bboxes.reserve(ssize(regions));
    for (int i = 0; i < ssize(regions); ++i) {

nit Since we don't care about the value of i inside of the loop, we should prefer to use a range-for loop for (const auto& region : regions)).

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 243 at r2 (raw file):

    for (int i = 0; i < ssize(regions); ++i) {
      continuous_bboxes.push_back(
          geometry::optimization::internal::

nit The functions GetMinimum... here and ComputeOffsets... below already have a using ... atop this file, so the geometry::optimization::internal:: namespace qualifier here are superfluous and should be removed. Ditto a few lines below, and similarly later in this file.

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 255 at r2 (raw file):

                  continuous_bboxes[i], continuous_bboxes[j]));
    }
    edge_offsets = std::addressof(maybe_edge_offsets.value());

Typical use of std::addressof is in templated (i.e., "generic") code to solve various safety/compatibility problems. In non-generic code, our convention is to just say &. Ditto throughout this PR.

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 821 at r2 (raw file):

      std::vector<std::vector<std::pair<double, double>>> continuous_bboxes_A;
      continuous_bboxes_A.reserve(ssize(from_subgraph.regions()));
      for (int i = 0; i < ssize(from_subgraph.regions()); ++i) {

nit Ditto on range-for loop instead of int i, and likewise for the "B" copy of the loop just below.

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2543 at r2 (raw file):

  gcs.AddEdges(subgraph2, goal, nullptr, &edges_2_goal);
  EXPECT_EQ(gcs.graph_of_convex_sets().Edges().size(), expected_num_edges);
  auto [traj_fail, result_fail] = gcs.SolvePath(start, goal);

nit I'm unclear what "fail" in the variable name is supposed to connote here.

Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r2 (raw file):

I've added a comment.

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 233 at r2 (raw file):

I'll remove it -- I didn't realize std::optional<T> default-initialized to std::nullopt. (Also removed in the spot where I used it later.)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 241 at r2 (raw file):

Done.

I called it region_ptr to denote that it's a ConvexSet*, not a ConvexSet, if that's alright with you.

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 243 at r2 (raw file):

Done.

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 255 at r2 (raw file):

Switched to &.

I had run into issues with grabbing the address of the data in std::optional<T> before, but I don't know enough about C++ to see why it works sometimes and not others. (Not super important -- I will try using & in the future.)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 821 at r2 (raw file):

Done.

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2543 at r2 (raw file):

It was a holdover from when this solve failed. I've renamed these variables.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 255 at r2 (raw file):

Sometimes there is an operator precedence problem (versus other things like array access or calls to member functions) where the & is being applied to not the thing we want. Usually adding some parentheses can solve that.