RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.17k stars 1.24k forks source link

Improve GCS graphviz #21530

Open bernhardpg opened 3 weeks ago

bernhardpg commented 3 weeks ago

Adds a few features to the GCS Graphviz code when a result is passed, and additionally make it possible to turn on/off the visualization of different quantities.j

Changes:


This change is Reviewable

RussTedrake commented 17 hours ago

One test is failing because of linting issues. How do you typically go about debugging this?

You should be running the tests locally. bazel test //.... If you want to run the linters specifically, then use bazel test --config=lint //... . The error messages often tell you how to fix it.

Taking a look at the getvariable error

RussTedrake commented 17 hours ago

geometry/optimization/graph_of_convex_sets.h line 594 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…
Ah. This is now a breaking change. Technically GCS is still marked as experimental, so we _can_ make breaking changes to the API. But it would be kinder/better to also keep the original function signature, but implement that as a one-liner that uses this entry point. Then you can deprecate the original entry point for eventual removal.

https://drake.mit.edu/doxygen_cxx/drake__deprecated_8h.html#a847d827d084f28e4ae1788a0ae75be96 (btw -- the strategy then would be to leave the original tests in tact behind a gcc diagnostic push/pop which you can find in other unit tests with deprecation, and simply duplicate the tests with the change to the new API. when the deprecation is removed, then we remove the tests as well)

RussTedrake commented 17 hours ago

bindings/pydrake/geometry/test/optimization_test.py line 819 at r3 (raw file):

            "source",
            spp.GetGraphvizString(
                result=result, options=graphviz_options, active_path=[edge0]

actually there is a way to accept the struct fields as arguments here. if we did that, then python users wouldn't have to change their code. in that case, I think we could avoid the deprecation completely (I suspect almost all users of the GetGraphVizString are doing that from python).

RussTedrake commented 17 hours ago

bindings/pydrake/geometry/geometry_py_optimization.cc line 722 at r3 (raw file):

    const auto &cls_doc = doc.GcsGraphvizOptions;
    py::class_<GcsGraphvizOptions> gcs_options(m, "GcsGraphvizOptions");
    gcs_options.def(py::init<>())

consider the use of ParamInit (e.g. as used in the ContactVisualizerParams). I know the other structs here don't use them yet, but perhaps they should...?

But i think it does not actually make the original identical syntax go through.

So we probably should deprecate the python bindings, too. I would be ok with deprecating only the python bindings, and letting the c++ be a breaking change.