Allow AssignRole(IllustrationProperties, kReplace) valid

This allows illustration properties to be replaced. It hasn't been
allowed previously because there is no formal connection between
changing those properties and guaranteeing they proppagate into
the visualizer.

This "addresses" that guarantee by printing a warning to the console
introducing a TODO for making it more robust.

bindings/pydrake/geometry_py.cc

@@ -758,8 +758,8 @@ void DoScalarIndependentDefinitions(py::module m) {
             [abstract_value_cls](Class& self, const std::string& group_name,
                 const std::string& name, py::object value) {
               py::object abstract = abstract_value_cls.attr("Make")(value);
-              self.AddPropertyAbstract(group_name, name,
-                  abstract.cast<const AbstractValue&>());
+              self.AddPropertyAbstract(
+                  group_name, name, abstract.cast<const AbstractValue&>());
             },
             py::arg("group_name"), py::arg("name"), py::arg("value"),
             cls_doc.AddProperty.doc)
@@ -768,8 +768,8 @@ void DoScalarIndependentDefinitions(py::module m) {
             [abstract_value_cls](Class& self, const std::string& group_name,
                 const std::string& name, py::object value) {
               py::object abstract = abstract_value_cls.attr("Make")(value);
-              self.UpdatePropertyAbstract(group_name, name,
-                  abstract.cast<const AbstractValue&>());
+              self.UpdatePropertyAbstract(
+                  group_name, name, abstract.cast<const AbstractValue&>());
             },
             py::arg("group_name"), py::arg("name"), py::arg("value"),
             cls_doc.UpdateProperty.doc)

bindings/pydrake/test/geometry_test.py

@@ -403,7 +403,7 @@ def test_geometry_properties_api(self):
             prop.GetProperty(group_name=default_group, name="to_update"), 17)
 
         prop.UpdateProperty(group_name=default_group, name="to_update",
-                         value=20)
+                            value=20)
         self.assertTrue(prop.HasProperty(group_name=default_group,
                                          name="to_update"))
         self.assertEqual(

geometry/geometry_properties.cc

@@ -52,11 +52,11 @@ void GeometryProperties::UpdatePropertyAbstract(const string& group_name,
         if (value_iter != group.end() &&
             group.at(name)->type_info() != value.type_info()) {
           throw std::logic_error(
-              fmt::format("AddProperty(): Trying to update property ('{}', '{}'"
-                          "); The property already exists and is of different "
-                          "type. New type {}, existing type {}",
-                          group_name, name, group.at(name)->GetNiceTypeName(),
-                          value.GetNiceTypeName()));
+              fmt::format("UpdateProperty(): Trying to update property ('{}', "
+                          "'{}'); The property already exists and is of "
+                          "different type. New type {}, existing type {}",
+                          group_name, name, value.GetNiceTypeName(),
+                          group.at(name)->GetNiceTypeName()));
         }
       });
 }

geometry/geometry_properties.h

@@ -266,7 +266,7 @@ class GeometryProperties {
 
   /** Updates the named property (`group_name`, `name`) with the given `value`.
    If the property doesn't already exist, it is equivalent to calling
-   `AddProperty`. If the property does exist, and its value the same type as
+   `AddProperty`. If the property does exist, and its value has the same type as
    `value`, the property value will be updated.
 
    @param group_name   The group name.
@@ -281,7 +281,7 @@ class GeometryProperties {
     UpdatePropertyAbstract(group_name, name, Value(value));
   }
 
-  /** Adds the named property (`group_name`, `name`) with the giventype-erased
+  /** Adds the named property (`group_name`, `name`) with the given type-erased
    `value`. Adds the group if it doesn't already exist.
 
    @param group_name   The group name.
@@ -342,7 +342,7 @@ class GeometryProperties {
     }
   }
 
-  /** Retrieves that type-erased value for the property (`group_name`, `name`)
+  /** Retrieves the type-erased value for the property (`group_name`, `name`)
    from this set of properties.
 
    @param group_name  The name of the group to which the property belongs.
@@ -406,7 +406,7 @@ class GeometryProperties {
     return kDefaultGroup.access();
   }
 
-  /** Removes the ('group_name', 'name') property (if it exists). Upon
+  /** Removes the (`group_name`, `name`) property (if it exists). Upon
    completion the property will not be in the set.
    @returns `true` if the property existed prior to the call.  */
   bool RemoveProperty(const std::string& group_name, const std::string& name);

geometry/geometry_state.cc

@@ -739,6 +739,12 @@ template <typename T>
 void GeometryState<T>::AssignRole(SourceId source_id, GeometryId geometry_id,
                                   PerceptionProperties properties,
                                   RoleAssign assign) {
+  if (assign == RoleAssign::kReplace) {
+    throw std::logic_error(
+        "AssignRole() with RoleAssign::kReplace does not work for perception "
+        "properties");
+  }
+
   InternalGeometry& geometry =
       ValidateRoleAssign(source_id, geometry_id, Role::kPerception, assign);
 
@@ -779,14 +785,26 @@ void GeometryState<T>::AssignRole(SourceId source_id, GeometryId geometry_id,
         "roles");
   }
 
+  // TODO(SeanCurtis-TRI): We want to remove this warning. For that to happen,
+  // we need systems dependent on illustration properties to recognize if there
+  // has been a change since last they processed the state. The simplest way to
+  // do that is to have a monotonically increasing serial number on
+  // GeometryState. It would increment for *every* change to GeometryState. A
+  // visualizer could query to determine if the serial number matches the value
+  // it had when it last initialized. If not, it can re-intialize (whatever that
+  // entails). It might also be advisable to have a collection of serial numbers
+  // with reduced scope (i.e., so a change to proximity properties doesn't
+  // cause illustration systems to reinitialize).
+  if (assign == RoleAssign::kReplace) {
+    static logging::Warn log_once(
+        "Updating illustration role properties must be done before visualizer "
+        "initialization to have an affect. When in doubt, after making "
+        "property changes, force the visualizer to re-initialize via its API.");
+  }
+
   InternalGeometry& geometry =
       ValidateRoleAssign(source_id, geometry_id, Role::kIllustration, assign);
-  // TODO(SeanCurtis-TRI): Ideally, if assign == RoleAssign::kReplace, this
-  //  should cause the visualization to change. I.e., if I've loaded an object
-  //  then I change its color here, it would be great if the visualization
-  //  reflected this. That is a *huge* issue that is not easily resolved.
-  //  Alternatively, I need to document that this *doesn't* happen and that it
-  //  is up to the visualizer to re-initialize itself.
+
   geometry.SetRole(std::move(properties));
 }
 
@@ -1184,12 +1202,6 @@ InternalGeometry& GeometryState<T>::ValidateRoleAssign(SourceId source_id,
                                                        GeometryId geometry_id,
                                                        Role role,
                                                        RoleAssign assign) {
-  if (assign == RoleAssign::kReplace &&
-      (role == Role::kPerception || role == Role::kIllustration)) {
-    throw std::logic_error(
-        "AssignRole() for updating properties currently only supports "
-        "proximity properties");
-  }
   if (!BelongsToSource(geometry_id, source_id)) {
     throw std::logic_error("Given geometry id " + to_string(geometry_id) +
         " does not belong to the given source id " +

geometry/internal_geometry.h

@@ -177,12 +177,9 @@ class InternalGeometry {
     proximity_props_ = std::move(properties);
   }
 
-  /** Assigns a illustration role to this geometry. Fails if it has already been
-   assigned.  */
+  /** Assigns a illustration role to this geometry, replacing any properties that
+   were previously assigned.  */
   void SetRole(IllustrationProperties properties) {
-    if (illustration_props_) {
-      throw std::logic_error("Geometry already has illustration role assigned");
-    }
     illustration_props_ = std::move(properties);
   }
 

geometry/proximity/test/hydroelastic_internal_test.cc

@@ -683,9 +683,9 @@ void TestPropertyErrors(
     // This error message comes from GeometryProperties::GetProperty().
     DRAKE_EXPECT_THROWS_MESSAGE(
         maker(shape_spec, wrong_value), std::logic_error,
-        fmt::format(".*The property '{}' .+ exists, but is of a different "
-                    "type.+string'",
-                    property_name));
+        fmt::format(".*The property \\('{}', '{}'\\) exists, but is of a "
+                    "different type.+string'",
+                    group_name, property_name));
   }
 
   // Error case: property value is not positive.

geometry/scene_graph.cc

@@ -271,6 +271,11 @@ void SceneGraph<T>::AssignRole(Context<T>* context, SourceId source_id,
                                GeometryId geometry_id,
                                IllustrationProperties properties,
                                RoleAssign assign) const {
+  static const logging::Warn one_time(
+      "Due to a bug (see issue #13597), changing the illustration roles or "
+      "properties in the context will not have any apparent effect in, at "
+      "least, drake_visualizer. Please change the illustration role in the "
+      "model prior to allocating the context");
   auto& g_state = mutable_geometry_state(context);
   g_state.AssignRole(source_id, geometry_id, std::move(properties), assign);
 }

geometry/scene_graph.h

@@ -534,7 +534,7 @@ class SceneGraph final : public systems::LeafSystem<T> {
 
    <h4>Changing the properties for an assigned role</h4>
 
-   If  the geometry has previously been assigned a role, the properties for
+   If the geometry has previously been assigned a role, the properties for
    that role can be modified with the following code (using ProximityProperties
    as an example):
 
@@ -560,13 +560,7 @@ class SceneGraph final : public systems::LeafSystem<T> {
    // Remove a property previously assigned.
    new_props.RemoveProperty("old_group", "old_name_1");
    // Update the *value* of an existing property (but enforce same type).
-   new_props.AddProperty("old_group", "old_name_2", new_value,
-                         AddPolicy::kUpdate);
-   // Overwrite the type and value of an existing property. This is the most
-   // dangerous modification; only do it if you're confident a type change is
-   // appropriate.
-   new_props.AddProperty("old_group", "type_A_prop", type_B_value,
-                         AddPolicy::kOverwrite);
+   new_props.UpdateProperty("old_group", "old_name_2", new_value);
    scene_graph.AssignRole(source_id, geometry_id, new_props,
                           RoleAssign::kReplace);
    @endcode
@@ -575,9 +569,11 @@ class SceneGraph final : public systems::LeafSystem<T> {
    role; it will simply eliminate possibly necessary properties. To remove
    the role completely, call `RemoveRole()`.
 
-   @warning Currently, only __proximity__ properties can be updated via this
-   mechanism. Updating illustration and perception will throw an exception (to
-   be implemented in the near future).
+   @warning Currently, only __proximity__ and __illustration__ properties can be
+   updated via this mechanism. Updating illustration properties has limitations
+   (see @ref AssignRole(SourceId,GeometryId,IllustrationProperties,RoleAssign)
+   "AssignRole(..., IllustrationProperties)" below). Attempting to update
+   perception will throw an exception (to be implemented in the near future).
 
    All invocations of `AssignRole()` will throw an exception if:
 
@@ -637,6 +633,13 @@ class SceneGraph final : public systems::LeafSystem<T> {
                   RoleAssign assign = RoleAssign::kNew) const;
 
   /** Assigns the illustration role to the geometry indicated by `geometry_id`.
+
+   @warning When changing illustration properties
+   (`assign = RoleAssign::kReplace`), there is no guarantee that these changes
+   will affect the visualization. The visualizer needs to be able to
+   "initialize" itself after changes to properties that will affect how a
+   geometry appears. If changing a geometry's illustration properties doesn't
+   seem to be affecting the visualization, retrigger its initialization action.
    @pydrake_mkdoc_identifier{illustration_direct}
    */
   void AssignRole(SourceId source_id, GeometryId geometry_id,
@@ -647,6 +650,19 @@ class SceneGraph final : public systems::LeafSystem<T> {
    @ref AssignRole(SourceId,GeometryId,IllustrationProperties) "AssignRole()"
    for illustration properties. Rather than modifying %SceneGraph's model, it
    modifies the copy of the model stored in the provided context.
+
+   @warning When changing illustration properties
+   (`assign = RoleAssign::kReplace`), there is no guarantee that these changes
+   will affect the visualization. The visualizer needs to be able to
+   "initialize" itself after changes to properties that will affect how a
+   geometry appears. If changing a geometry's illustration properties doesn't
+   seem to be affecting the visualization, retrigger its initialization action.
+
+   @warning Due to a bug (see issue
+   <a href="https://github.com/RobotLocomotion/drake/issues/13597">#13597</a>),
+   changing the illustration roles or properties in the context will not have
+   any apparent effect in, at least, drake_visualizer. Please change the
+   illustration role in the model prior to allocating the context.
    @pydrake_mkdoc_identifier{illustration_context}
    */
   void AssignRole(systems::Context<T>* context, SourceId source_id,

geometry/test/geometry_state_test.cc

@@ -2422,8 +2422,9 @@ TEST_F(GeometryStateTest, AssignRolesToGeometry) {
   EXPECT_TRUE(has_expected_roles(anchored_geometry_, true, false, true));
 }
 
-// Test the ability to reassign properties to a role that already exists.
-TEST_F(GeometryStateTest, ModifyRoleProperties) {
+// Test the ability to reassign proximity properties to a geometry that already
+// has the proximity role.
+TEST_F(GeometryStateTest, ModifyProximityProperties) {
   SetUpSingleSourceTree();
   // No roles assigned.
   EXPECT_EQ(gs_tester_.proximity_engine().num_geometries(), 0);
@@ -2481,27 +2482,66 @@ TEST_F(GeometryStateTest, ModifyRoleProperties) {
             props2.GetProperty<int>("prop2", "value"));
   EXPECT_EQ(hydroelastic_geometries.hydroelastic_type(geometries_[0]),
             internal::HydroelasticType::kRigid);
+}
+
+// Test the ability to reassign illustration properties to a geometry that
+// already has the illustration role.
+TEST_F(GeometryStateTest, ModifyIllustrationProperties) {
+  SetUpSingleSourceTree();
+
+  // The geometry we're going to play with. Confirm that it starts without the
+  // illustration role.
+  const GeometryId geometry_id = geometries_[1];
+  ASSERT_EQ(geometry_state_.GetIllustrationProperties(geometry_id), nullptr);
 
-  // Case: confirm throw for perception and illustration properties.
+  IllustrationProperties empty_props;
+  IllustrationProperties props1;
+  props1.AddProperty("prop1", "value", 1);
+  IllustrationProperties props2;
+  props2.AddProperty("prop2", "value", 2);
+
+  // Generally, we assume that illustration property replacement exercises the
+  // same infrastructure as perception
+  EXPECT_NO_THROW(
+      geometry_state_.AssignRole(source_id_, geometry_id, empty_props));
+  const IllustrationProperties* props =
+      geometry_state_.GetIllustrationProperties(geometry_id);
+  ASSERT_NE(props, nullptr);
+  ASSERT_EQ(props->num_groups(), 1);  // Just the default group.
+
+  // Now re-assign.
+  EXPECT_NO_THROW(geometry_state_.AssignRole(source_id_, geometry_id, props1,
+                                             RoleAssign::kReplace));
+  props = geometry_state_.GetIllustrationProperties(geometry_id);
+  ASSERT_NE(props, nullptr);
+  ASSERT_TRUE(props->HasProperty("prop1", "value"));
+  ASSERT_FALSE(props->HasProperty("prop2", "value"));
+
+  // Now re-assign again.
+  EXPECT_NO_THROW(geometry_state_.AssignRole(source_id_, geometry_id, props2,
+                                             RoleAssign::kReplace));
+  props = geometry_state_.GetIllustrationProperties(geometry_id);
+  ASSERT_NE(props, nullptr);
+  ASSERT_FALSE(props->HasProperty("prop1", "value"));
+  ASSERT_TRUE(props->HasProperty("prop2", "value"));
+}
+
+// Test that attemptint to reassign perception properties to a geometry that
+// already has the perception role fails.
+TEST_F(GeometryStateTest, ModifyPerceptionProperties) {
+  SetUpSingleSourceTree();
+
+  // Case: confirm throw for perception properties.
   PerceptionProperties perception_props =
       render_engine_->accepting_properties();
   perception_props.AddProperty("label", "id", RenderLabel(10));
-  geometry_state_.AssignRole(source_id_, geometries_[1], perception_props);
+  EXPECT_NO_THROW(
+      geometry_state_.AssignRole(source_id_, geometries_[1], perception_props));
   DRAKE_EXPECT_THROWS_MESSAGE(
       geometry_state_.AssignRole(source_id_, geometries_[1], perception_props,
                                  RoleAssign::kReplace),
       std::logic_error,
-      "AssignRole.. for updating properties currently only supports proximity "
-      "properties");
-
-  geometry_state_.AssignRole(source_id_, geometries_[1],
-                             IllustrationProperties());
-  DRAKE_EXPECT_THROWS_MESSAGE(
-      geometry_state_.AssignRole(source_id_, geometries_[1],
-                                 IllustrationProperties(),
-                                 RoleAssign::kReplace),
-      std::logic_error,
-      "AssignRole.. for updating properties currently only supports proximity "
+      "AssignRole\\(\\) with RoleAssign::kReplace does not work for perception "
       "properties");
 }